-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 66609 - invalid XML in directory listing with file names containing "&" and "'" #621
Conversation
alexkachanov
commented
May 24, 2023
- fix and junit test
"&" and "'" * fix and junit test
@@ -40,6 +40,7 @@ public final class URLEncoder implements Cloneable { | |||
|
|||
public static final URLEncoder DEFAULT = new URLEncoder(); | |||
public static final URLEncoder QUERY = new URLEncoder(); | |||
public static final URLEncoder XML = new URLEncoder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added new XML encoder that encodes strings that are valid in XML
@@ -95,6 +96,8 @@ public final class URLEncoder implements Cloneable { | |||
QUERY.addSafeCharacter('_'); | |||
QUERY.addSafeCharacter('='); | |||
QUERY.addSafeCharacter('&'); | |||
|
|||
XML.addSafeCharacter('/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"/" is a valid symbol
@@ -1650,7 +1654,7 @@ protected InputStream renderXml(HttpServletRequest request, String contextPath, | |||
.append('\''); | |||
sb.append(" urlPath='") | |||
.append(rewrittenContextPath) | |||
.append(rewriteUrl(directoryWebappPath + entry)) | |||
.append(rewriteUrl(directoryWebappPath + entry, URLEncoder.XML)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using XML encoder to encode path attribute which may contain "'" and "&" symbols
@@ -578,6 +578,44 @@ public void testBug57601() throws Exception { | |||
|
|||
tomcat.stop(); | |||
} | |||
|
|||
@Test | |||
public void testBug66609() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added test to test the fix
(Ab)using the URLEncoder for this seems wrong. Tomcat has a class specifically for escaping values in content - Further, it appears that there are still multiple implementations of XML escaping in the Tomcat code base. These need to be rationalised to use I agree having a test case for this is an improvement. I'd like to see that test case cover all the XML characters that need to be escaped and also test the WebDAV case as well as the directory listing case. |
I was using URLEncoder class because it was used in original rewriteUrl method. So I wanted to keep it consistent and keep the changes it to minimum. I see that Escape.xml(String content) makes sense here. Let me change the patch |
"&" and "'" * fix and junit test
@@ -1650,7 +1650,7 @@ protected InputStream renderXml(HttpServletRequest request, String contextPath, | |||
.append('\''); | |||
sb.append(" urlPath='") | |||
.append(rewrittenContextPath) | |||
.append(rewriteUrl(directoryWebappPath + entry)) | |||
.append(Escape.xml(directoryWebappPath + entry)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using Escape.xml instead of rewriteUrl
"&" and "'" * missing change
…ov/tomcat into Bug_66609_invalid_XML
Based on #621 by Alex Kachanov
Thanks for the PR. Applied manually with a few tweaks (e.g. CheckStyle config). |
Based on #621 by Alex Kachanov
Based on #621 by Alex Kachanov
Based on #621 by Alex Kachanov