Issue #6473 - canonicalPath refactor & fix alias check in PathResource (Jetty-10) #6478
Conversation
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
* Reverted %-escape handling for URI query parts. * Performing canonicalization in ServletContext.getResource(), and improving alias checking in ContextHandler.getResource(). * Performing canonicalization checks in Resource.addPath() to avoid navigation above of the root. * Test added and fixed. * Various cleanups. * Improved javadoc and comments Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Co-authored-by: Greg Wilkins <gregw@webtide.com>
| public interface HttpURI | ||
| { | ||
| /** | ||
| * Violations of safe URI interpretations | ||
| */ | ||
| enum Violation |
There was a problem hiding this comment.
This class was called Ambiguous so this is a breaking change.
Also, why does it not implement ComplianceViolation, which seems to summarize the violation concept?
On a closer look, I think this class here is a complete duplicate of UriCompliance.Violation.
It's here because in 9.4.x we don't have UriCompliance (so it's a merge side-effect).
Should it not be completely removed, so that in 10 we only use UriCompliance.Violation?
There was a problem hiding this comment.
Ambiguous was public for a few releases, but not used in any interfaces at all, so I think it is OK to change.
But worth investigating if we can actually use UriCompliance.Violation here....
| * @return Set of violations in the URI. | ||
| */ | ||
| Collection<Violation> getViolations(); | ||
|
|
There was a problem hiding this comment.
See comment above, I think methods that expose Violation should be removed or changed to use UriCompliance.Violation.
| @@ -1216,6 +1298,10 @@ else if (c == '/') | |||
| { | |||
| switch (encodedValue) | |||
| { | |||
| case 0: | |||
| // Byte 0 cannot be present in a UTF-8 sequence in any position | |||
| // other than as the NUL ASCII byte which we do not wish to allow. | |||
| return violation.getMessage(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
See previous comment. This mapping from HttpURI.Violation to UriCompliance.Violation should not be necessary.
HttpURI should just use UriCompliance.Violation.
| @@ -2305,6 +2308,8 @@ public InputStream getResourceAsStream(String path) | |||
| @Override | |||
| public Set<String> getResourcePaths(String path) | |||
| { | |||
| if (path == null) | |||
| return null; | |||
There was a problem hiding this comment.
Should not the logic here be similar to getResource()? I think we should canonicalize here too.
And also in getRealPath().
I think we need to review the ServletContext implementation more carefully.
| UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS))); | ||
| assertThat(_connector.getResponse(request), Matchers.allOf( | ||
| startsWith("HTTP/1.1 200"), | ||
| containsString("pathInfo=/path/ambiguous/.././info"))); |
There was a problem hiding this comment.
Seems like a legit test. Why was it removed?
There was a problem hiding this comment.
becaues the compliance mode it was testing no longer exists.
| @@ -420,8 +419,7 @@ else if (_servletContext instanceof ContextHandler.Context) | |||
| } | |||
| else | |||
| { | |||
| URL u = _servletContext.getResource(pathInContext); | |||
| r = _contextHandler.newResource(u); | |||
There was a problem hiding this comment.
Why was this changed? Can _servletContext never be other than ContextHandler.Context?
| @@ -471,6 +471,7 @@ public static String decodePath(String path, int offset, int length) | |||
| if (u == 'u') | |||
| { | |||
| // UTF16 encoding is only supported with UriCompliance.Violation.UTF16_ENCODINGS. | |||
| // This is wrong. This is a codepoint not a char | |||
There was a problem hiding this comment.
This needs to be fixed then? 9.4.x too?
There was a problem hiding this comment.
Nope - this is just how we implemented %u. It is not strictly correct, but let's not improve a feature we really want to remove.
Signed-off-by: Greg Wilkins <gregw@webtide.com>
Issue #6473
Merge of PR #6474 and PR #6477 for Jetty-10.