From 3c32afa05c5ffcf4ad77a7769a118df4e083b92d Mon Sep 17 00:00:00 2001 From: Lachlan Date: Tue, 29 Jun 2021 23:42:39 +1000 Subject: [PATCH] Issue #6473 - canonicalPath refactor & fix alias check in PathResource (Jetty-10) (#6478) Issue #6473 - canonicalPath refactor & fix alias check in PathResource * 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 * Compliance mode HttpURI uses UriCompliance.Violation Signed-off-by: Lachlan Roberts Signed-off-by: Simone Bordet Co-authored-by: Greg Wilkins --- .../java/org/eclipse/jetty/http/HttpURI.java | 258 ++++++++--------- .../org/eclipse/jetty/http/UriCompliance.java | 25 +- .../org/eclipse/jetty/http/HttpURITest.java | 245 +++++++++-------- .../maven/plugin/MavenWebAppContext.java | 1 + .../jetty/rewrite/handler/RedirectUtil.java | 8 +- .../rewrite/handler/ValidUrlRuleTest.java | 23 +- .../org/eclipse/jetty/server/Dispatcher.java | 26 +- .../org/eclipse/jetty/server/Request.java | 33 +-- .../org/eclipse/jetty/server/Response.java | 4 +- .../jetty/server/handler/ContextHandler.java | 32 ++- .../jetty/server/handler/ResourceHandler.java | 3 - .../jetty/server/HttpConnectionTest.java | 8 +- .../org/eclipse/jetty/server/RequestTest.java | 9 - .../ContextHandlerGetResourceTest.java | 39 ++- .../eclipse/jetty/servlet/DefaultServlet.java | 4 +- .../eclipse/jetty/servlet/RequestURITest.java | 51 +++- .../servlets/DataRateLimitedServlet.java | 7 +- .../org/eclipse/jetty/servlets/PutFilter.java | 8 +- .../java/org/eclipse/jetty/util/URIUtil.java | 259 +++++++++--------- .../eclipse/jetty/util/Utf8Appendable.java | 1 - .../jetty/util/resource/PathResource.java | 39 +-- .../eclipse/jetty/util/resource/Resource.java | 7 +- .../jetty/util/resource/URLResource.java | 8 +- .../jetty/util/URIUtilCanonicalPathTest.java | 47 +++- .../jetty/util/Utf8AppendableTest.java | 29 ++ .../jetty/util/resource/ResourceTest.java | 18 ++ .../eclipse/jetty/webapp/WebAppContext.java | 15 +- .../jetty/webapp/WebAppContextTest.java | 40 +++ .../jsp/JspAndDefaultWithoutAliasesTest.java | 37 ++- 29 files changed, 768 insertions(+), 516 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 42f829068e9c..1294874db368 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -15,8 +15,11 @@ import java.net.URI; import java.net.URISyntaxException; +import java.util.Collection; +import java.util.Collections; import java.util.EnumSet; +import org.eclipse.jetty.http.UriCompliance.Violation; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.Index; import org.eclipse.jetty.util.StringUtil; @@ -31,56 +34,48 @@ * via the static methods such as {@link #build()} and {@link #from(String)}. * * A URI such as - * http://user@host:port/path;ignored/info;param?query#ignored - * is split into the following undecoded elements:
    + * {@code http://user@host:port/path;param1/%2e/info;param2?query#fragment} + * is split into the following optional elements:
      *
    • {@link #getScheme()} - http:
      • *
    • {@link #getAuthority()} - //name@host:port
      • *
    • {@link #getHost()} - host
      • *
    • {@link #getPort()} - port
      • - *
    • {@link #getPath()} - /path/info
      • - *
    • {@link #getParam()} - param
      • + *
    • {@link #getPath()} - /path;param1/%2e/info;param2
      • + *
    • {@link #getDecodedPath()} - /path/info
      • + *
    • {@link #getParam()} - param2
      • *
    • {@link #getQuery()} - query
      • *
    • {@link #getFragment()} - fragment
      • *
    - *

    Any parameters will be returned from {@link #getPath()}, but are excluded from the - * return value of {@link #getDecodedPath()}. If there are multiple parameters, the - * {@link #getParam()} method returns only the last one. - */ + *

    The path part of the URI is provided in both raw form ({@link #getPath()}) and + * decoded form ({@link #getDecodedPath}), which has: path parameters removed, + * percent encoded characters expanded and relative segments resolved. This approach + * is somewhat contrary to RFC3986 + * which no longer defines path parameters (removed after + * RFC2396) and specifies + * that relative segment normalization should take place before percent encoded character + * expansion. A literal interpretation of the RFC can result in URI paths with ambiguities + * when viewed as strings. For example, a URI of {@code /foo%2f..%2fbar} is technically a single + * segment of "/foo/../bar", but could easily be misinterpreted as 3 segments resolving to "/bar" + * by a file system. + *

    + *

    + * Thus this class avoid and/or detects such ambiguities. Furthermore, by decoding characters and + * removing parameters before relative path normalization, ambiguous paths will be resolved in such + * a way to be non-standard-but-non-ambiguous to down stream interpretation of the decoded path string. + * The violations are recorded and available by API such as {@link #hasAmbiguousSegment()} so that requests + * containing them may be rejected in case the non-standard-but-non-ambiguous interpretations + * are not satisfactory for a given compliance configuration. + *

    + *

    + * Implementations that wish to process ambiguous URI paths must configure the compliance modes + * to accept them and then perform their own decoding of {@link #getPath()}. + *

    + *

    + * If there are multiple path parameters, only the last one is returned by {@link #getParam()}. + *

    + **/ public interface HttpURI { - enum Violation - { - /** - * URI contains ambiguous path segments e.g. {@code /foo/%2e%2e/bar} - */ - SEGMENT, - - /** - * URI contains ambiguous empty segments e.g. {@code /foo//bar} or {@code /foo/;param/}, but not {@code /foo/} - */ - EMPTY, - - /** - * URI contains ambiguous path separator within a URI segment e.g. {@code /foo/b%2fr} - */ - SEPARATOR, - - /** - * URI contains ambiguous path encoding within a URI segment e.g. {@code /%2557EB-INF} - */ - ENCODING, - - /** - * URI contains ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar} - */ - PARAM, - - /** - * Contains UTF16 encodings - */ - UTF16 - } - static Mutable build() { return new Mutable(); @@ -147,6 +142,11 @@ static Immutable from(String scheme, String host, int port, String pathQuery) String getHost(); + /** + * Get a URI path parameter. Multiple and in segment parameters are ignored and only + * the last trailing parameter is returned. + * @return The last path parameter or null + */ String getParam(); String getPath(); @@ -166,41 +166,73 @@ static Immutable from(String scheme, String host, int port, String pathQuery) boolean isAbsolute(); /** - * @return True if the URI has either an {@link #hasAmbiguousParameter()}, {@link #hasAmbiguousSegment()} or {@link #hasAmbiguousSeparator()}. + * @return True if the URI has any ambiguous {@link Violation}s. */ boolean isAmbiguous(); /** - * @return True if the URI has any Violations. + * @return True if the URI has any {@link Violation}s. */ boolean hasViolations(); + /** + * @param violation the violation to check. + * @return true if the URI has the passed violation. + */ + boolean hasViolation(Violation violation); + + /** + * @return Set of violations in the URI. + */ + Collection getViolations(); + /** * @return True if the URI has a possibly ambiguous segment like '..;' or '%2e%2e' */ - boolean hasAmbiguousSegment(); + default boolean hasAmbiguousSegment() + { + return hasViolation(Violation.AMBIGUOUS_PATH_SEGMENT); + } /** * @return True if the URI empty segment that is ambiguous like '//' or '/;param/'. */ - boolean hasAmbiguousEmptySegment(); + default boolean hasAmbiguousEmptySegment() + { + return hasViolation(Violation.AMBIGUOUS_EMPTY_SEGMENT); + } /** * @return True if the URI has a possibly ambiguous separator of %2f */ - boolean hasAmbiguousSeparator(); + default boolean hasAmbiguousSeparator() + { + return hasViolation(Violation.AMBIGUOUS_PATH_SEPARATOR); + } /** * @return True if the URI has a possibly ambiguous path parameter like '..;' */ - boolean hasAmbiguousParameter(); + default boolean hasAmbiguousParameter() + { + return hasViolation(Violation.AMBIGUOUS_PATH_PARAMETER); + } /** * @return True if the URI has an encoded '%' character. */ - boolean hasAmbiguousEncoding(); + default boolean hasAmbiguousEncoding() + { + return hasViolation(Violation.AMBIGUOUS_PATH_ENCODING); + } - boolean hasUtf16Encoding(); + /** + * @return True if the URI has UTF16 '%u' encodings. + */ + default boolean hasUtf16Encoding() + { + return hasViolation(Violation.UTF16_ENCODINGS); + } default URI toURI() { @@ -408,7 +440,7 @@ public boolean isAbsolute() @Override public boolean isAmbiguous() { - return !_violations.isEmpty() && !(_violations.size() == 1 && _violations.contains(Violation.UTF16)); + return !_violations.isEmpty() && !(_violations.size() == 1 && _violations.contains(Violation.UTF16_ENCODINGS)); } @Override @@ -418,39 +450,15 @@ public boolean hasViolations() } @Override - public boolean hasAmbiguousSegment() - { - return _violations.contains(Violation.SEGMENT); - } - - @Override - public boolean hasAmbiguousEmptySegment() - { - return _violations.contains(Violation.EMPTY); - } - - @Override - public boolean hasAmbiguousSeparator() - { - return _violations.contains(Violation.SEPARATOR); - } - - @Override - public boolean hasAmbiguousParameter() - { - return _violations.contains(Violation.PARAM); - } - - @Override - public boolean hasAmbiguousEncoding() + public boolean hasViolation(Violation violation) { - return _violations.contains(Violation.ENCODING); + return _violations.contains(violation); } @Override - public boolean hasUtf16Encoding() + public Collection getViolations() { - return _violations.contains(Violation.UTF16); + return Collections.unmodifiableCollection(_violations); } @Override @@ -781,7 +789,7 @@ public boolean isAbsolute() @Override public boolean isAmbiguous() { - return !_violations.isEmpty() && !(_violations.size() == 1 && _violations.contains(Violation.UTF16)); + return !_violations.isEmpty() && !(_violations.size() == 1 && _violations.contains(Violation.UTF16_ENCODINGS)); } @Override @@ -791,39 +799,15 @@ public boolean hasViolations() } @Override - public boolean hasAmbiguousSegment() - { - return _violations.contains(Violation.SEGMENT); - } - - @Override - public boolean hasAmbiguousEmptySegment() - { - return _violations.contains(Violation.EMPTY); - } - - @Override - public boolean hasAmbiguousSeparator() + public boolean hasViolation(Violation violation) { - return _violations.contains(Violation.SEPARATOR); + return _violations.contains(violation); } @Override - public boolean hasAmbiguousParameter() + public Collection getViolations() { - return _violations.contains(Violation.PARAM); - } - - @Override - public boolean hasAmbiguousEncoding() - { - return _violations.contains(Violation.ENCODING); - } - - @Override - public boolean hasUtf16Encoding() - { - return _violations.contains(Violation.UTF16); + return Collections.unmodifiableCollection(_violations); } public Mutable normalize() @@ -928,9 +912,9 @@ public Mutable uri(HttpURI uri) _uri = null; _decodedPath = uri.getDecodedPath(); if (uri.hasAmbiguousSeparator()) - _violations.add(Violation.SEPARATOR); + _violations.add(Violation.AMBIGUOUS_PATH_SEPARATOR); if (uri.hasAmbiguousSegment()) - _violations.add(Violation.SEGMENT); + _violations.add(Violation.AMBIGUOUS_PATH_SEGMENT); return this; } @@ -1205,7 +1189,7 @@ else if (c == '/') { if (encodedCharacters == 2 && c == 'u' && !encodedUtf16) { - _violations.add(Violation.UTF16); + _violations.add(Violation.UTF16_ENCODINGS); encodedUtf16 = true; encodedCharacters = 4; continue; @@ -1216,11 +1200,15 @@ 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. + throw new IllegalArgumentException("Illegal character in path"); case '/': - _violations.add(Violation.SEPARATOR); + _violations.add(Violation.AMBIGUOUS_PATH_SEPARATOR); break; case '%': - _violations.add(Violation.ENCODING); + _violations.add(Violation.AMBIGUOUS_PATH_ENCODING); break; default: break; @@ -1301,26 +1289,11 @@ else if (c == '/') } case QUERY: { - switch (c) + if (c == '#') { - case '%': - encodedCharacters = 2; - break; - case 'u': - case 'U': - if (encodedCharacters == 1) - _violations.add(Violation.UTF16); - encodedCharacters = 0; - break; - case '#': - _query = uri.substring(mark, i); - mark = i + 1; - state = State.FRAGMENT; - encodedCharacters = 0; - break; - default: - encodedCharacters = 0; - break; + _query = uri.substring(mark, i); + mark = i + 1; + state = State.FRAGMENT; } break; } @@ -1335,7 +1308,9 @@ else if (c == '/') break; } default: + { throw new IllegalStateException(state.toString()); + } } } @@ -1387,10 +1362,12 @@ else if (c == '/') } else if (_path != null) { - String canonical = URIUtil.canonicalPath(_path); - if (canonical == null) - throw new BadMessageException("Bad URI"); - _decodedPath = URIUtil.decodePath(canonical); + // The RFC requires this to be canonical before decoding, but this can leave dot segments and dot dot segments + // which are not canonicalized and could be used in an attempt to bypass security checks. + String decodedNonCanonical = URIUtil.decodePath(_path); + _decodedPath = URIUtil.canonicalPath(decodedNonCanonical); + if (_decodedPath == null) + throw new IllegalArgumentException("Bad URI"); } } @@ -1409,9 +1386,10 @@ private void checkSegment(String uri, int segment, int end, boolean param) // This method is called once for every segment parsed. // A URI like "/foo/" has two segments: "foo" and an empty segment. // Empty segments are only ambiguous if they are not the last segment - // So if this method is called for any segment and we have previously seen an empty segment, then it was ambiguous + // So if this method is called for any segment and we have previously + // seen an empty segment, then it was ambiguous. if (_emptySegment) - _violations.add(Violation.EMPTY); + _violations.add(Violation.AMBIGUOUS_EMPTY_SEGMENT); if (end == segment) { @@ -1422,7 +1400,7 @@ private void checkSegment(String uri, int segment, int end, boolean param) // If this empty segment is the first segment then it is ambiguous. if (segment == 0) { - _violations.add(Violation.EMPTY); + _violations.add(Violation.AMBIGUOUS_EMPTY_SEGMENT); return; } @@ -1439,12 +1417,12 @@ private void checkSegment(String uri, int segment, int end, boolean param) if (ambiguous == Boolean.TRUE) { // The segment is always ambiguous. - _violations.add(Violation.SEGMENT); + _violations.add(Violation.AMBIGUOUS_PATH_SEGMENT); } else if (param && ambiguous == Boolean.FALSE) { // The segment is ambiguous only when followed by a parameter. - _violations.add(Violation.PARAM); + _violations.add(Violation.AMBIGUOUS_PATH_PARAMETER); } } } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index f9d0504283ab..be8ceffdc77e 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -25,7 +25,6 @@ import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableSet; import static java.util.EnumSet.allOf; -import static java.util.EnumSet.complementOf; import static java.util.EnumSet.noneOf; import static java.util.EnumSet.of; @@ -67,10 +66,6 @@ public enum Violation implements ComplianceViolation * Allow ambiguous path encoding within a URI segment e.g. /%2557EB-INF */ AMBIGUOUS_PATH_ENCODING("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path encoding"), - /** - * Allow Non canonical ambiguous paths. eg /foo/x%2f%2e%2e%/bar provided to applications as /foo/x/../bar - */ - NON_CANONICAL_AMBIGUOUS_PATHS("https://tools.ietf.org/html/rfc3986#section-3.3", "Non canonical ambiguous paths"), /** * Allow UTF-16 encoding eg /foo%u2192bar. */ @@ -125,10 +120,9 @@ public String getDescription() /** * Compliance mode that exactly follows RFC3986, - * including allowing all additional ambiguous URI Violations, - * except {@link Violation#NON_CANONICAL_AMBIGUOUS_PATHS}, thus ambiguous paths are canonicalized for safety. + * including allowing all additional ambiguous URI Violations. */ - public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", complementOf(of(Violation.NON_CANONICAL_AMBIGUOUS_PATHS))); + public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", allOf(Violation.class)); /** * Compliance mode that follows RFC3986 @@ -231,6 +225,11 @@ public static UriCompliance from(String spec) boolean exclude = element.startsWith("-"); if (exclude) element = element.substring(1); + + // Ignore removed name. TODO: remove in future release. + if (element.equals("NON_CANONICAL_AMBIGUOUS_PATHS")) + continue; + Violation section = Violation.valueOf(element); if (exclude) violations.remove(section); @@ -330,4 +329,14 @@ private static Set copyOf(Set violations) return EnumSet.noneOf(Violation.class); return EnumSet.copyOf(violations); } + + public static String checkUriCompliance(UriCompliance compliance, HttpURI uri) + { + for (UriCompliance.Violation violation : UriCompliance.Violation.values()) + { + if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(violation))) + return violation.getDescription(); + } + return null; + } } diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index 4c199e15d40b..83701d915624 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -19,7 +19,7 @@ import java.util.EnumSet; import java.util.stream.Stream; -import org.eclipse.jetty.http.HttpURI.Violation; +import org.eclipse.jetty.http.UriCompliance.Violation; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -36,6 +36,7 @@ import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.Assumptions.assumeTrue; +// @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck public class HttpURITest { @Test @@ -146,6 +147,12 @@ public void testParse() uri = builder.asImmutable(); assertThat(uri.getHost(), is("foo")); assertThat(uri.getPath(), is("/bar")); + + // We do allow nulls if not encoded. This can be used for testing 2nd line of defence. + builder.uri("http://fo\000/bar"); + uri = builder.asImmutable(); + assertThat(uri.getHost(), is("fo\000")); + assertThat(uri.getPath(), is("/bar")); } @Test @@ -350,7 +357,9 @@ public static Stream decodePathTests() // encoded paths {"/f%6f%6F/bar", "/foo/bar", EnumSet.noneOf(Violation.class)}, - {"/f%u006f%u006F/bar", "/foo/bar", EnumSet.of(Violation.UTF16)}, + {"/f%u006f%u006F/bar", "/foo/bar", EnumSet.of(Violation.UTF16_ENCODINGS)}, + {"/f%u0001%u0001/bar", "/f\001\001/bar", EnumSet.of(Violation.UTF16_ENCODINGS)}, + {"/foo/%u20AC/bar", "/foo/\u20AC/bar", EnumSet.of(Violation.UTF16_ENCODINGS)}, // illegal paths {"//host/../path/info", null, EnumSet.noneOf(Violation.class)}, @@ -360,75 +369,81 @@ public static Stream decodePathTests() {"/path/%2/F/info", null, EnumSet.noneOf(Violation.class)}, {"/path/%/info", null, EnumSet.noneOf(Violation.class)}, {"/path/%u000X/info", null, EnumSet.noneOf(Violation.class)}, + {"/path/Fo%u0000/info", null, EnumSet.noneOf(Violation.class)}, + {"/path/Fo%00/info", null, EnumSet.noneOf(Violation.class)}, + {"/path/Foo/info%u0000", null, EnumSet.noneOf(Violation.class)}, + {"/path/Foo/info%00", null, EnumSet.noneOf(Violation.class)}, + {"/path/%U20AC", null, EnumSet.noneOf(Violation.class)}, + {"%2e%2e/info", null, EnumSet.noneOf(Violation.class)}, + {"%u002e%u002e/info", null, EnumSet.noneOf(Violation.class)}, + {"%2e%2e;/info", null, EnumSet.noneOf(Violation.class)}, + {"%u002e%u002e;/info", null, EnumSet.noneOf(Violation.class)}, + {"%2e.", null, EnumSet.noneOf(Violation.class)}, + {"%u002e.", null, EnumSet.noneOf(Violation.class)}, + {".%2e", null, EnumSet.noneOf(Violation.class)}, + {".%u002e", null, EnumSet.noneOf(Violation.class)}, + {"%2e%2e", null, EnumSet.noneOf(Violation.class)}, + {"%u002e%u002e", null, EnumSet.noneOf(Violation.class)}, + {"%2e%u002e", null, EnumSet.noneOf(Violation.class)}, + {"%u002e%2e", null, EnumSet.noneOf(Violation.class)}, + {"..;/info", null, EnumSet.noneOf(Violation.class)}, + {"..;param/info", null, EnumSet.noneOf(Violation.class)}, // ambiguous dot encodings - {"scheme://host/path/%2e/info", "/path/./info", EnumSet.of(Violation.SEGMENT)}, - {"scheme:/path/%2e/info", "/path/./info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e/info", "/path/./info", EnumSet.of(Violation.SEGMENT)}, - {"path/%2e/info/", "path/./info/", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e/info", "/path/../info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;/info", "/path/../info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;param/info", "/path/../info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;param;other/info;other", "/path/../info", EnumSet.of(Violation.SEGMENT)}, - {"%2e/info", "./info", EnumSet.of(Violation.SEGMENT)}, - {"%u002e/info", "./info", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - {"%2e%2e/info", "../info", EnumSet.of(Violation.SEGMENT)}, - {"%u002e%u002e/info", "../info", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - {"%2e%2e;/info", "../info", EnumSet.of(Violation.SEGMENT)}, - {"%u002e%u002e;/info", "../info", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - {"%2e", ".", EnumSet.of(Violation.SEGMENT)}, - {"%u002e", ".", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - {"%2e.", "..", EnumSet.of(Violation.SEGMENT)}, - {"%u002e.", "..", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - {".%2e", "..", EnumSet.of(Violation.SEGMENT)}, - {".%u002e", "..", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - {"%2e%2e", "..", EnumSet.of(Violation.SEGMENT)}, - {"%u002e%u002e", "..", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - {"%2e%u002e", "..", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - {"%u002e%2e", "..", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, + {"scheme://host/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"scheme:/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"path/%2e/info/", "path/info/", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%2e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%u002e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.UTF16_ENCODINGS)}, + + {"%2e", "", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%u002e", "", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.UTF16_ENCODINGS)}, // empty segment treated as ambiguous - {"/foo//bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, - {"/foo//../bar", "/foo/bar", EnumSet.of(Violation.EMPTY)}, - {"/foo///../../../bar", "/bar", EnumSet.of(Violation.EMPTY)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//../bar", "/foo/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo///../../../bar", "/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"/foo/./../bar", "/bar", EnumSet.noneOf(Violation.class)}, - {"/foo//./bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, + {"/foo//./bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"foo/bar", "foo/bar", EnumSet.noneOf(Violation.class)}, {"foo;/bar", "foo/bar", EnumSet.noneOf(Violation.class)}, - {";/bar", "/bar", EnumSet.of(Violation.EMPTY)}, - {";?n=v", "", EnumSet.of(Violation.EMPTY)}, + {";/bar", "/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {";?n=v", "", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"?n=v", "", EnumSet.noneOf(Violation.class)}, {"#n=v", "", EnumSet.noneOf(Violation.class)}, {"", "", EnumSet.noneOf(Violation.class)}, {"http:/foo", "/foo", EnumSet.noneOf(Violation.class)}, // ambiguous parameter inclusions - {"/path/.;/info", "/path/./info", EnumSet.of(Violation.PARAM)}, - {"/path/.;param/info", "/path/./info", EnumSet.of(Violation.PARAM)}, - {"/path/..;/info", "/path/../info", EnumSet.of(Violation.PARAM)}, - {"/path/..;param/info", "/path/../info", EnumSet.of(Violation.PARAM)}, - {".;/info", "./info", EnumSet.of(Violation.PARAM)}, - {".;param/info", "./info", EnumSet.of(Violation.PARAM)}, - {"..;/info", "../info", EnumSet.of(Violation.PARAM)}, - {"..;param/info", "../info", EnumSet.of(Violation.PARAM)}, + {"/path/.;/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/.;param/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/..;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/..;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {".;/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {".;param/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, // ambiguous segment separators - {"/path/%2f/info", "/path///info", EnumSet.of(Violation.SEPARATOR)}, - {"%2f/info", "//info", EnumSet.of(Violation.SEPARATOR)}, - {"%2F/info", "//info", EnumSet.of(Violation.SEPARATOR)}, - {"/path/%2f../info", "/path//../info", EnumSet.of(Violation.SEPARATOR)}, + {"/path/%2f/info", "/path///info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"%2f/info", "//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"%2F/info", "//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"/path/%2f../info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, // ambiguous encoding - {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.ENCODING)}, - {"/path/%u0025/info", "/path/%/info", EnumSet.of(Violation.ENCODING, Violation.UTF16)}, - {"%25/info", "%/info", EnumSet.of(Violation.ENCODING)}, - {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.ENCODING)}, - {"/path/%u0025../info", "/path/%../info", EnumSet.of(Violation.ENCODING, Violation.UTF16)}, + {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"/path/%u0025/info", "/path/%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING, Violation.UTF16_ENCODINGS)}, + {"%25/info", "%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"/path/%u0025../info", "/path/%../info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING, Violation.UTF16_ENCODINGS)}, // combinations - {"/path/%2f/..;/info", "/path///../info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM)}, - {"/path/%u002f/..;/info", "/path///../info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM, Violation.UTF16)}, - {"/path/%2f/..;/%2e/info", "/path///.././info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM, Violation.SEGMENT)}, + {"/path/%2f/..;/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/%u002f/..;/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.UTF16_ENCODINGS)}, + {"/path/%2f/..;/%2e/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.AMBIGUOUS_PATH_SEGMENT)}, // Non ascii characters // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck @@ -447,15 +462,15 @@ public void testDecodedPath(String input, String decodedPath, EnumSet HttpURI uri = HttpURI.from(input); assertThat(uri.getDecodedPath(), is(decodedPath)); EnumSet ambiguous = EnumSet.copyOf(expected); - ambiguous.retainAll(EnumSet.complementOf(EnumSet.of(Violation.UTF16))); + ambiguous.retainAll(EnumSet.complementOf(EnumSet.of(Violation.UTF16_ENCODINGS))); assertThat(uri.isAmbiguous(), is(!ambiguous.isEmpty())); - assertThat(uri.hasAmbiguousSegment(), is(ambiguous.contains(Violation.SEGMENT))); - assertThat(uri.hasAmbiguousSeparator(), is(ambiguous.contains(Violation.SEPARATOR))); - assertThat(uri.hasAmbiguousParameter(), is(ambiguous.contains(Violation.PARAM))); - assertThat(uri.hasAmbiguousEncoding(), is(ambiguous.contains(Violation.ENCODING))); + assertThat(uri.hasAmbiguousSegment(), is(ambiguous.contains(Violation.AMBIGUOUS_PATH_SEGMENT))); + assertThat(uri.hasAmbiguousSeparator(), is(ambiguous.contains(Violation.AMBIGUOUS_PATH_SEPARATOR))); + assertThat(uri.hasAmbiguousParameter(), is(ambiguous.contains(Violation.AMBIGUOUS_PATH_PARAMETER))); + assertThat(uri.hasAmbiguousEncoding(), is(ambiguous.contains(Violation.AMBIGUOUS_PATH_ENCODING))); - assertThat(uri.hasUtf16Encoding(), is(expected.contains(Violation.UTF16))); + assertThat(uri.hasUtf16Encoding(), is(expected.contains(Violation.UTF16_ENCODINGS))); } catch (Exception e) { @@ -483,78 +498,78 @@ public static Stream testPathQueryTests() {"../path/info", null, null}, {"/path/%XX/info", null, null}, {"/path/%2/F/info", null, null}, + {"%2e%2e/info", null, null}, + {"%2e%2e;/info", null, null}, + {"%2e.", null, null}, + {".%2e", null, null}, + {"%2e%2e", null, null}, + {"..;/info", null, null}, + {"..;param/info", null, null}, // ambiguous dot encodings - {"/path/%2e/info", "/path/./info", EnumSet.of(Violation.SEGMENT)}, - {"path/%2e/info/", "path/./info/", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e/info", "/path/../info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;/info", "/path/../info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;param/info", "/path/../info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;param;other/info;other", "/path/../info", EnumSet.of(Violation.SEGMENT)}, - {"%2e/info", "./info", EnumSet.of(Violation.SEGMENT)}, - {"%2e%2e/info", "../info", EnumSet.of(Violation.SEGMENT)}, - {"%2e%2e;/info", "../info", EnumSet.of(Violation.SEGMENT)}, - {"%2e", ".", EnumSet.of(Violation.SEGMENT)}, - {"%2e.", "..", EnumSet.of(Violation.SEGMENT)}, - {".%2e", "..", EnumSet.of(Violation.SEGMENT)}, - {"%2e%2e", "..", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"path/%2e/info/", "path/info/", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%2e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%2e", "", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, // empty segment treated as ambiguous {"/", "/", EnumSet.noneOf(Violation.class)}, {"/#", "/", EnumSet.noneOf(Violation.class)}, {"/path", "/path", EnumSet.noneOf(Violation.class)}, {"/path/", "/path/", EnumSet.noneOf(Violation.class)}, - {"//", "//", EnumSet.of(Violation.EMPTY)}, - {"/foo//", "/foo//", EnumSet.of(Violation.EMPTY)}, - {"/foo//bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, - {"//foo/bar", "//foo/bar", EnumSet.of(Violation.EMPTY)}, + {"//", "//", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//", "/foo//", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"//foo/bar", "//foo/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"/foo?bar", "/foo", EnumSet.noneOf(Violation.class)}, {"/foo#bar", "/foo", EnumSet.noneOf(Violation.class)}, {"/foo;bar", "/foo", EnumSet.noneOf(Violation.class)}, {"/foo/?bar", "/foo/", EnumSet.noneOf(Violation.class)}, {"/foo/#bar", "/foo/", EnumSet.noneOf(Violation.class)}, {"/foo/;param", "/foo/", EnumSet.noneOf(Violation.class)}, - {"/foo/;param/bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, - {"/foo//bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, - {"/foo//bar//", "/foo//bar//", EnumSet.of(Violation.EMPTY)}, - {"//foo//bar//", "//foo//bar//", EnumSet.of(Violation.EMPTY)}, - {"/foo//../bar", "/foo/bar", EnumSet.of(Violation.EMPTY)}, - {"/foo///../../../bar", "/bar", EnumSet.of(Violation.EMPTY)}, + {"/foo/;param/bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//bar//", "/foo//bar//", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"//foo//bar//", "//foo//bar//", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//../bar", "/foo/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo///../../../bar", "/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"/foo/./../bar", "/bar", EnumSet.noneOf(Violation.class)}, - {"/foo//./bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, + {"/foo//./bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"foo/bar", "foo/bar", EnumSet.noneOf(Violation.class)}, {"foo;/bar", "foo/bar", EnumSet.noneOf(Violation.class)}, - {";/bar", "/bar", EnumSet.of(Violation.EMPTY)}, - {";?n=v", "", EnumSet.of(Violation.EMPTY)}, + {";/bar", "/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {";?n=v", "", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"?n=v", "", EnumSet.noneOf(Violation.class)}, {"#n=v", "", EnumSet.noneOf(Violation.class)}, {"", "", EnumSet.noneOf(Violation.class)}, // ambiguous parameter inclusions - {"/path/.;/info", "/path/./info", EnumSet.of(Violation.PARAM)}, - {"/path/.;param/info", "/path/./info", EnumSet.of(Violation.PARAM)}, - {"/path/..;/info", "/path/../info", EnumSet.of(Violation.PARAM)}, - {"/path/..;param/info", "/path/../info", EnumSet.of(Violation.PARAM)}, - {".;/info", "./info", EnumSet.of(Violation.PARAM)}, - {".;param/info", "./info", EnumSet.of(Violation.PARAM)}, - {"..;/info", "../info", EnumSet.of(Violation.PARAM)}, - {"..;param/info", "../info", EnumSet.of(Violation.PARAM)}, + {"/path/.;/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/.;param/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/..;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/..;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {".;/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {".;param/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, // ambiguous segment separators - {"/path/%2f/info", "/path///info", EnumSet.of(Violation.SEPARATOR)}, - {"%2f/info", "//info", EnumSet.of(Violation.SEPARATOR)}, - {"%2F/info", "//info", EnumSet.of(Violation.SEPARATOR)}, - {"/path/%2f../info", "/path//../info", EnumSet.of(Violation.SEPARATOR)}, + {"/path/%2f/info", "/path///info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"%2f/info", "//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"%2F/info", "//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"/path/%2f../info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, // ambiguous encoding - {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.ENCODING)}, - {"%25/info", "%/info", EnumSet.of(Violation.ENCODING)}, - {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.ENCODING)}, + {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"%25/info", "%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, // combinations - {"/path/%2f/..;/info", "/path///../info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM)}, - {"/path/%2f/..;/%2e/info", "/path///.././info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM, Violation.SEGMENT)}, - {"/path/%2f/%25/..;/%2e//info", "/path///%/.././/info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM, Violation.SEGMENT, Violation.ENCODING, Violation.EMPTY)}, + {"/path/%2f/..;/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/%2f/..;/%2e/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2f/%25/..;/%2e//info", "/path////info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_ENCODING, Violation.AMBIGUOUS_EMPTY_SEGMENT)}, }).map(Arguments::of); } @@ -572,11 +587,11 @@ public void testPathQuery(String input, String decodedPath, EnumSet e HttpURI uri = HttpURI.build().pathQuery(input); assertThat(uri.getDecodedPath(), is(decodedPath)); assertThat(uri.isAmbiguous(), is(!expected.isEmpty())); - assertThat(uri.hasAmbiguousEmptySegment(), is(expected.contains(Violation.EMPTY))); - assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Violation.SEGMENT))); - assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Violation.SEPARATOR))); - assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Violation.PARAM))); - assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Violation.ENCODING))); + assertThat(uri.hasAmbiguousEmptySegment(), is(expected.contains(Violation.AMBIGUOUS_EMPTY_SEGMENT))); + assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Violation.AMBIGUOUS_PATH_SEGMENT))); + assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Violation.AMBIGUOUS_PATH_SEPARATOR))); + assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Violation.AMBIGUOUS_PATH_PARAMETER))); + assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Violation.AMBIGUOUS_PATH_ENCODING))); } public static Stream parseData() @@ -793,4 +808,20 @@ public void testCompareToJavaNetURI(String input, String scheme, String host, In assertThat("[" + input + "] .fragment", httpUri.getFragment(), is(javaUri.getFragment())); assertThat("[" + input + "] .toString", httpUri.toString(), is(javaUri.toASCIIString())); } + + public static Stream queryData() + { + return Stream.of( + new String[]{"/path?p=%U20AC", "p=%U20AC"}, + new String[]{"/path?p=%u20AC", "p=%u20AC"} + ).map(Arguments::of); + } + + @ParameterizedTest + @MethodSource("queryData") + public void testEncodedQuery(String input, String expectedQuery) + { + HttpURI httpURI = HttpURI.build(input); + assertThat("[" + input + "] .query", httpURI.getQuery(), is(expectedQuery)); + } } diff --git a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java index c246f9688511..74a60614ba0a 100644 --- a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java +++ b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java @@ -361,6 +361,7 @@ public Resource getResource(String uriInContext) throws MalformedURLException // /WEB-INF/classes if ((resource == null || !resource.exists()) && uriInContext != null && _classes != null) { + // Canonicalize again to look for the resource inside /WEB-INF subdirectories. String uri = URIUtil.canonicalPath(uriInContext); if (uri == null) return null; diff --git a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java b/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java index 2f4e5b693053..750c4ad2ea4c 100644 --- a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java +++ b/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java @@ -40,20 +40,20 @@ public static String toRedirectURL(final HttpServletRequest request, String loca if (location.startsWith("/")) { // absolute in context - location = URIUtil.canonicalEncodedPath(location); + location = URIUtil.canonicalURI(location); } else { // relative to request String path = request.getRequestURI(); String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path); - location = URIUtil.canonicalPath(URIUtil.addEncodedPaths(parent, location)); - if (!location.startsWith("/")) + location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location)); + if (location != null && !location.startsWith("/")) url.append('/'); } if (location == null) - throw new IllegalStateException("path cannot be above root"); + throw new IllegalStateException("redirect path cannot be above root"); url.append(location); location = url.toString(); diff --git a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java b/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java index fbe12b887a80..17f370e60948 100644 --- a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java +++ b/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java @@ -16,11 +16,11 @@ import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.server.Dispatcher; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @SuppressWarnings("unused") @@ -62,7 +62,7 @@ public void testInvalidUrlWithMessage() throws Exception { _rule.setCode("405"); _rule.setMessage("foo"); - _request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/%00/")); + _request.setHttpURI(HttpURI.from("/%01/")); String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response); @@ -72,10 +72,17 @@ public void testInvalidUrlWithMessage() throws Exception @Test public void testInvalidJsp() throws Exception + { + assertThrows(IllegalArgumentException.class, () -> HttpURI.build(_request.getHttpURI(), "/jsp/bean1.jsp%00")); + } + + @Test + public void testInvalidJspWithNullByte() throws Exception { _rule.setCode("405"); _rule.setMessage("foo"); - _request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/jsp/bean1.jsp%00")); + + _request.setHttpURI(HttpURI.from("/jsp/bean1.jsp\000")); String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response); @@ -86,14 +93,7 @@ public void testInvalidJsp() throws Exception @Test public void testInvalidShamrock() throws Exception { - _rule.setCode("405"); - _rule.setMessage("foo"); - _request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/jsp/shamrock-%00%E2%98%98.jsp")); - - String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response); - - assertEquals(405, _response.getStatus()); - assertEquals("foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE)); + assertThrows(IllegalArgumentException.class, () -> HttpURI.build(_request.getHttpURI(), "/jsp/shamrock-%00%E2%98%98.jsp")); } @Test @@ -119,4 +119,3 @@ public void testCharacters() throws Exception //@checkstyle-enable-check : IllegalTokenText } } - diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java index fcd2dcef43db..e8a917424864 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java @@ -15,6 +15,7 @@ import java.io.IOException; import java.util.HashSet; +import java.util.Objects; import java.util.Set; import javax.servlet.DispatcherType; import javax.servlet.RequestDispatcher; @@ -27,6 +28,7 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpURI; +import org.eclipse.jetty.http.UriCompliance; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.MultiMap; @@ -76,7 +78,7 @@ public void error(ServletRequest request, ServletResponse response) throws Servl @Override public void include(ServletRequest request, ServletResponse response) throws ServletException, IOException { - Request baseRequest = Request.getBaseRequest(request); + Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request)); if (!(request instanceof HttpServletRequest)) request = new ServletRequestHttpWrapper(request); @@ -98,6 +100,10 @@ public void include(ServletRequest request, ServletResponse response) throws Ser } else { + Objects.requireNonNull(_uri); + // Check any URI violations against the compliance for this request + checkUriViolations(_uri, baseRequest); + IncludeAttributes attr = new IncludeAttributes( old_attr, baseRequest, @@ -131,7 +137,7 @@ public void forward(ServletRequest request, ServletResponse response) throws Ser protected void forward(ServletRequest request, ServletResponse response, DispatcherType dispatch) throws ServletException, IOException { - Request baseRequest = Request.getBaseRequest(request); + Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request)); Response baseResponse = baseRequest.getResponse(); baseResponse.resetForForward(); @@ -159,6 +165,10 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc } else { + Objects.requireNonNull(_uri); + // Check any URI violations against the compliance for this request + checkUriViolations(_uri, baseRequest); + // If we have already been forwarded previously, then keep using the established // original value. Otherwise, this is the first forward and we need to establish the values. // Note: the established value on the original request for pathInfo and @@ -230,6 +240,18 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc } } + private static void checkUriViolations(HttpURI uri, Request baseRequest) + { + if (uri.hasViolations()) + { + HttpChannel channel = baseRequest.getHttpChannel(); + UriCompliance compliance = channel == null || channel.getHttpConfiguration() == null ? null : channel.getHttpConfiguration().getUriCompliance(); + String illegalState = UriCompliance.checkUriCompliance(compliance, uri); + if (illegalState != null) + throw new IllegalStateException(illegalState); + } + } + @Override public String toString() { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 7c05e6239826..dc0a456a4976 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -67,6 +67,7 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HostPortHttpField; +import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField; import org.eclipse.jetty.http.HttpField; @@ -1687,28 +1688,13 @@ public void setMetaData(MetaData.Request request) _method = request.getMethod(); _httpFields = request.getFields(); final HttpURI uri = request.getURI(); - boolean ambiguous = false; UriCompliance compliance = null; if (uri.hasViolations()) { - ambiguous = uri.isAmbiguous(); compliance = _channel == null || _channel.getHttpConfiguration() == null ? null : _channel.getHttpConfiguration().getUriCompliance(); - if (uri.hasUtf16Encoding() && (compliance == null || !compliance.allows(UriCompliance.Violation.UTF16_ENCODINGS))) - throw new BadMessageException("UTF16 % encoding not supported"); - - if (ambiguous) - { - if (uri.hasAmbiguousSegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT))) - throw new BadMessageException("Ambiguous segment in URI"); - if (uri.hasAmbiguousEmptySegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_EMPTY_SEGMENT))) - throw new BadMessageException("Ambiguous empty segment in URI"); - if (uri.hasAmbiguousSeparator() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR))) - throw new BadMessageException("Ambiguous segment in URI"); - if (uri.hasAmbiguousParameter() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_PARAMETER))) - throw new BadMessageException("Ambiguous path parameter in URI"); - if (uri.hasAmbiguousEncoding() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_ENCODING))) - throw new BadMessageException("Ambiguous path encoding in URI"); - } + String badMessage = UriCompliance.checkUriCompliance(compliance, uri); + if (badMessage != null) + throw new BadMessageException(badMessage); } if (uri.isAbsolute() && uri.hasAuthority() && uri.getPath() != null) @@ -1740,7 +1726,6 @@ public void setMetaData(MetaData.Request request) } _uri = builder.asImmutable(); } - setSecure(HttpScheme.HTTPS.is(_uri.getScheme())); String encoded = _uri.getPath(); @@ -1751,17 +1736,15 @@ public void setMetaData(MetaData.Request request) else if (encoded.startsWith("/")) { path = (encoded.length() == 1) ? "/" : _uri.getDecodedPath(); - // Strictly speaking if a URI is legal and encodes ambiguous segments, then they should be - // reflected in the decoded string version. However, it can be ambiguous to provide a decoded path as - // a string, so we normalize again. If an application wishes to see ambiguous URIs, then they must - // set the {@link UriCompliance.Violation#NON_CANONICAL_AMBIGUOUS_PATHS} compliance. - if (ambiguous && (compliance == null || !compliance.allows(UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS))) - path = URIUtil.canonicalPath(path); } else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod())) + { path = encoded; + } else + { path = null; + } if (path == null || path.isEmpty()) { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index d79be8e96b04..e2adfed9bd63 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -568,14 +568,14 @@ public void sendRedirect(int code, String location, boolean consumeAll) throws I if (location.startsWith("/")) { // absolute in context - location = URIUtil.canonicalEncodedPath(location); + location = URIUtil.canonicalURI(location); } else { // relative to request String path = _channel.getRequest().getRequestURI(); String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path); - location = URIUtil.canonicalEncodedPath(URIUtil.addEncodedPaths(parent, location)); + location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location)); if (location != null && !location.startsWith("/")) buf.append('/'); } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 91d4d3e665cf..9e4735621c3a 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -29,7 +29,6 @@ import java.util.EventListener; import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Map; @@ -1490,7 +1489,6 @@ public boolean isProtectedTarget(String target) return false; if (target.startsWith("//")) - // ignore empty segments which may be discard by file system target = URIUtil.compactPath(target); ProtectedTargetType type = _protectedTargets.getBest(target); @@ -1930,7 +1928,9 @@ public Resource getResource(String path) throws MalformedURLException try { - path = URIUtil.canonicalPath(path); + // addPath with accept non-canonical paths that don't go above the root, + // but will treat them as aliases. So unless allowed by an AliasChecker + // they will be rejected below. Resource resource = _baseResource.addPath(path); if (checkAlias(path, resource)) @@ -1959,9 +1959,8 @@ public boolean checkAlias(String path, Resource resource) LOG.debug("Aliased resource: {}~={}", resource, resource.getAlias()); // alias checks - for (Iterator i = getAliasChecks().iterator(); i.hasNext(); ) + for (AliasCheck check : getAliasChecks()) { - AliasCheck check = i.next(); if (check.check(path, resource)) { if (LOG.isDebugEnabled()) @@ -2014,7 +2013,6 @@ public Set getResourcePaths(String path) { try { - path = URIUtil.canonicalPath(path); Resource resource = getResource(path); if (resource != null && resource.exists()) @@ -2214,6 +2212,7 @@ public String getMimeType(String file) @Override public RequestDispatcher getRequestDispatcher(String uriInContext) { + // uriInContext is encoded, potentially with query. if (uriInContext == null) return null; @@ -2223,10 +2222,9 @@ public RequestDispatcher getRequestDispatcher(String uriInContext) try { String contextPath = getContextPath(); - String pathInfo; - + // uriInContext is canonicalized by HttpURI. HttpURI.Mutable uri = HttpURI.build(uriInContext); - pathInfo = URIUtil.canonicalPath(uri.getDecodedPath()); + String pathInfo = uri.getDecodedPath(); if (StringUtil.isEmpty(pathInfo)) return null; @@ -2247,6 +2245,10 @@ public RequestDispatcher getRequestDispatcher(String uriInContext) @Override public String getRealPath(String path) { + // This is an API call from the application which may have arbitrary non canonical paths passed + // Thus we canonicalize here, to avoid the enforcement of only canonical paths in + // ContextHandler.this.getResource(path). + path = URIUtil.canonicalPath(path); if (path == null) return null; if (path.length() == 0) @@ -2275,6 +2277,12 @@ else if (path.charAt(0) != '/') @Override public URL getResource(String path) throws MalformedURLException { + // This is an API call from the application which may have arbitrary non canonical paths passed + // Thus we canonicalize here, to avoid the enforcement of only canonical paths in + // ContextHandler.this.getResource(path). + path = URIUtil.canonicalPath(path); + if (path == null) + return null; Resource resource = ContextHandler.this.getResource(path); if (resource != null && resource.exists()) return resource.getURI().toURL(); @@ -2305,6 +2313,12 @@ public InputStream getResourceAsStream(String path) @Override public Set getResourcePaths(String path) { + // This is an API call from the application which may have arbitrary non canonical paths passed + // Thus we canonicalize here, to avoid the enforcement of only canonical paths in + // ContextHandler.this.getResource(path). + path = URIUtil.canonicalPath(path); + if (path == null) + return null; return ContextHandler.this.getResourcePaths(path); } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java index bbbb960678a1..bd7ba44c5297 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java @@ -156,7 +156,6 @@ public Resource getResource(String path) throws IOException if (_baseResource != null) { - path = URIUtil.canonicalPath(path); r = _baseResource.addPath(path); if (r.isAlias() && (_context == null || !_context.checkAlias(path, r))) @@ -169,8 +168,6 @@ public Resource getResource(String path) throws IOException else if (_context != null) { r = _context.getResource(path); - if (r != null) - return r; } if ((r == null || !r.exists()) && path.endsWith("/jetty-dir.css")) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java index 4d1c0d29d4c1..b6d301285445 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java @@ -504,7 +504,7 @@ public void testBadPathDotDotPath() throws Exception { String response = connector.getResponse("GET /ooops/../../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); checkContains(response, 0, "HTTP/1.1 400 "); - checkContains(response, 0, "reason: Bad URI"); + checkContains(response, 0, "reason: Bad Request"); } @Test @@ -512,7 +512,7 @@ public void testBadDotDotPath() throws Exception { String response = connector.getResponse("GET ../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); checkContains(response, 0, "HTTP/1.1 400 "); - checkContains(response, 0, "reason: Bad URI"); + checkContains(response, 0, "reason: Bad Request"); } @Test @@ -520,7 +520,7 @@ public void testBadSlashDotDotPath() throws Exception { String response = connector.getResponse("GET /../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); checkContains(response, 0, "HTTP/1.1 400 "); - checkContains(response, 0, "reason: Bad URI"); + checkContains(response, 0, "reason: Bad Request"); } @Test @@ -827,7 +827,7 @@ public void testBadUTF8FallsbackTo8859() throws Exception "Host: localhost\r\n" + "Connection: close\r\n" + "\r\n"); - checkContains(response, 0, "HTTP/1.1 200"); //now fallback to iso-8859-1 + checkContains(response, 0, "HTTP/1.1 400"); response = connector.getResponse("GET /bad/utf8%c1 HTTP/1.1\r\n" + "Host: localhost\r\n" + diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index e31f303438d7..7f2f91d7a64b 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -1713,15 +1713,6 @@ public void testAmbiguousPaths() throws Exception assertThat(_connector.getResponse(request), Matchers.allOf( startsWith("HTTP/1.1 200"), containsString("pathInfo=/path/info"))); - - _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.from(EnumSet.of( - UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR, - UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT, - UriCompliance.Violation.AMBIGUOUS_PATH_PARAMETER, - UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS))); - assertThat(_connector.getResponse(request), Matchers.allOf( - startsWith("HTTP/1.1 200"), - containsString("pathInfo=/path/ambiguous/.././info"))); } @Test diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java index 9401376083bf..d304c0bb2160 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java @@ -28,6 +28,8 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.EnabledOnOs; import org.junit.jupiter.api.condition.OS; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.nullValue; @@ -218,24 +220,39 @@ public void testGetKnown() throws Exception } @Test - public void testNormalize() throws Exception + public void testDoesNotExistResource() throws Exception { - final String path = "/down/.././index.html"; - Resource resource = context.getResource(path); - assertEquals("index.html", resource.getFile().getName()); - assertEquals(docroot, resource.getFile().getParentFile()); - assertTrue(resource.exists()); - - URL url = context.getServletContext().getResource(path); - assertEquals(docroot, new File(url.toURI()).getParentFile()); + Resource resource = context.getResource("/doesNotExist.html"); + assertNotNull(resource); + assertFalse(resource.exists()); } @Test - public void testTooNormal() throws Exception + public void testAlias() throws Exception { - final String path = "/down/.././../"; + String path = "/./index.html"; Resource resource = context.getResource(path); assertNull(resource); + URL resourceURL = context.getServletContext().getResource(path); + assertFalse(resourceURL.getPath().contains("/./")); + + path = "/down/../index.html"; + resource = context.getResource(path); + assertNull(resource); + resourceURL = context.getServletContext().getResource(path); + assertFalse(resourceURL.getPath().contains("/../")); + + path = "//index.html"; + resource = context.getResource(path); + assertNull(resource); + resourceURL = context.getServletContext().getResource(path); + assertNull(resourceURL); + } + + @ParameterizedTest + @ValueSource(strings = {"/down/.././../", "/../down/"}) + public void testNormalize(String path) throws Exception + { URL url = context.getServletContext().getResource(path); assertNull(url); } diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java index 74702ec7cb8f..8315bcdab967 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java @@ -14,7 +14,6 @@ package org.eclipse.jetty.servlet; import java.io.IOException; -import java.net.URL; import java.util.ArrayList; import java.util.List; import java.util.StringTokenizer; @@ -420,8 +419,7 @@ else if (_servletContext instanceof ContextHandler.Context) } else { - URL u = _servletContext.getResource(pathInContext); - r = _contextHandler.newResource(u); + return null; } if (LOG.isDebugEnabled()) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java index 730f29165fe3..8c08fe06be09 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java @@ -57,11 +57,13 @@ public static Stream data() ret.add(Arguments.of("/hello?type=wo&rld", "/hello", "type=wo&rld")); ret.add(Arguments.of("/hello?type=wo%20rld", "/hello", "type=wo%20rld")); ret.add(Arguments.of("/hello?type=wo+rld", "/hello", "type=wo+rld")); + ret.add(Arguments.of("/hello?type=/a/../b/", "/hello", "type=/a/../b/")); ret.add(Arguments.of("/It%27s%20me%21", "/It%27s%20me%21", null)); // try some slash encoding (with case preservation tests) ret.add(Arguments.of("/hello%2fworld", "/hello%2fworld", null)); ret.add(Arguments.of("/hello%2Fworld", "/hello%2Fworld", null)); ret.add(Arguments.of("/%2f%2Fhello%2Fworld", "/%2f%2Fhello%2Fworld", null)); + // try some "?" encoding (should not see as query string) ret.add(Arguments.of("/hello%3Fworld", "/hello%3Fworld", null)); // try some strange encodings (should preserve them) @@ -69,8 +71,13 @@ public static Stream data() ret.add(Arguments.of("/hello%u0025world", "/hello%u0025world", null)); ret.add(Arguments.of("/hello-euro-%E2%82%AC", "/hello-euro-%E2%82%AC", null)); ret.add(Arguments.of("/hello-euro?%E2%82%AC", "/hello-euro", "%E2%82%AC")); - // test the ascii control characters (just for completeness) - for (int i = 0x0; i < 0x1f; i++) + + ret.add(Arguments.of("/hello/..;/world", "/hello/..;/world", null)); + ret.add(Arguments.of("/hello/..;?/world", "/hello/..;", "/world")); + + // Test the ascii control characters (just for completeness). + // Zero is not allowed in UTF-8 sequences so start from 1. + for (int i = 0x1; i < 0x1f; i++) { String raw = String.format("/hello%%%02Xworld", i); ret.add(Arguments.of(raw, raw, null)); @@ -194,7 +201,6 @@ public void testGetRequestURIHTTP10(String rawpath, String expectedReqUri, Strin // Read the response. String response = readResponse(client); - // TODO: is HTTP/1.1 response appropriate for an HTTP/1.0 request? assertThat(response, Matchers.containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.containsString("RequestURI: " + expectedReqUri)); assertThat(response, Matchers.containsString("QueryString: " + expectedQuery)); @@ -221,4 +227,43 @@ public void testGetRequestURIHTTP11(String rawpath, String expectedReqUri, Strin assertThat(response, Matchers.containsString("QueryString: " + expectedQuery)); } } + + public static Stream badData() + { + List ret = new ArrayList<>(); + ret.add(Arguments.of("/hello\000")); + ret.add(Arguments.of("/hello%00")); + ret.add(Arguments.of("/hello%u0000")); + ret.add(Arguments.of("/hello\000/world")); + ret.add(Arguments.of("/hello%00world")); + ret.add(Arguments.of("/hello%u0000world")); + ret.add(Arguments.of("/hello%GG")); + ret.add(Arguments.of("/hello%;/world")); + ret.add(Arguments.of("/hello/../../world")); + ret.add(Arguments.of("/hello/%#x/../world")); + ret.add(Arguments.of("/../hello/world")); + ret.add(Arguments.of("/hello%u00u00/world")); + ret.add(Arguments.of("hello")); + + return ret.stream(); + } + + @ParameterizedTest + @MethodSource("badData") + public void testGetBadRequestsURIHTTP10(String rawpath) throws Exception + { + try (Socket client = newSocket(serverURI.getHost(), serverURI.getPort())) + { + OutputStream os = client.getOutputStream(); + + String request = String.format("GET %s HTTP/1.0\r\n\r\n", rawpath); + os.write(request.getBytes(StandardCharsets.ISO_8859_1)); + os.flush(); + + // Read the response. + String response = readResponse(client); + + assertThat(response, Matchers.containsString("HTTP/1.1 400 ")); + } + } } diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DataRateLimitedServlet.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DataRateLimitedServlet.java index eca818907799..1492b1da1f24 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DataRateLimitedServlet.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DataRateLimitedServlet.java @@ -34,7 +34,7 @@ import org.eclipse.jetty.util.ProcessorUtils; /** - * A servlet that uses the Servlet 3.1 asynchronous IO API to server + * A demonstration servlet that uses the Servlet 3.1 asynchronous IO API to server * static content at a limited data rate. *

    * Two implementations are supported:

      @@ -42,8 +42,7 @@ * APIs, but produces more garbage due to the byte[] nature of the API. *
    • the JettyDataStream impl uses a Jetty API to write a ByteBuffer * and thus allow the efficient use of file mapped buffers without any - * temporary buffer copies (I did tell the JSR that this was a good idea to - * have in the standard!). + * temporary buffer copies. *
    *

    * The data rate is controlled by setting init parameters: @@ -53,7 +52,9 @@ *

    pool
    The size of the thread pool used to service the writes (defaults to available processors)
    * * Thus if buffersize = 1024 and pause = 100, the data rate will be limited to 10KB per second. + * @deprecated this is intended as a demonstration and not production quality. */ +@Deprecated public class DataRateLimitedServlet extends HttpServlet { private static final long serialVersionUID = -4771757707068097025L; diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PutFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PutFilter.java index f4ca579d4df8..f34676387dc6 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PutFilter.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PutFilter.java @@ -57,6 +57,7 @@ *
  • putAtomic - boolean, if true PUT files are written to a temp location and moved into place. *
*/ +@Deprecated public class PutFilter implements Filter { public static final String __PUT = "PUT"; @@ -80,7 +81,8 @@ public void init(FilterConfig config) throws ServletException _tmpdir = (File)_context.getAttribute("javax.servlet.context.tempdir"); - if (_context.getRealPath("/") == null) + String realPath = _context.getRealPath("/"); + if (realPath == null) throw new UnavailableException("Packed war"); String b = config.getInitParameter("baseURI"); @@ -90,7 +92,7 @@ public void init(FilterConfig config) throws ServletException } else { - File base = new File(_context.getRealPath("/")); + File base = new File(realPath); _baseURI = base.toURI().toString(); } @@ -284,7 +286,7 @@ public void handleDelete(HttpServletRequest request, HttpServletResponse respons public void handleMove(HttpServletRequest request, HttpServletResponse response, String pathInContext, File file) throws ServletException, IOException, URISyntaxException { - String newPath = URIUtil.canonicalEncodedPath(request.getHeader("new-uri")); + String newPath = URIUtil.canonicalURI(request.getHeader("new-uri")); if (newPath == null) { response.sendError(HttpServletResponse.SC_BAD_REQUEST); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java index f18e685f2e8b..1a8b65aad919 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java @@ -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 builder.append((char)(0xffff & TypeUtil.parseInt(path, i + 2, 4, 16))); i += 5; } @@ -532,7 +533,6 @@ public static String decodePath(String path, int offset, int length) { throw new IllegalArgumentException("cannot decode URI", e); } - } /* Decode a URI path and strip parameters of ISO-8859-1 path @@ -776,143 +776,138 @@ public static String parentPath(String p) } /** - * Convert an encoded path to a canonical form. + * Convert a partial URI to a canonical form. *

- * All instances of "." and ".." are factored out. + * All segments of "." and ".." are factored out. * Null is returned if the path tries to .. above its root. *

* - * @param path the path to convert, decoded, with path separators '/' and no queries. + * @param uri the encoded URI from the path onwards, which may contain query strings and/or fragments * @return the canonical path, or null if path traversal above root. + * @see #canonicalPath(String) */ - public static String canonicalPath(String path) + public static String canonicalURI(String uri) { - // See https://tools.ietf.org/html/rfc3986#section-5.2.4 - - if (path == null || path.isEmpty()) - return path; + if (uri == null || uri.isEmpty()) + return uri; - int end = path.length(); + boolean slash = true; + int end = uri.length(); int i = 0; - int dots = 0; + // Initially just loop looking if we may need to normalize loop: while (i < end) { - char c = path.charAt(i); + char c = uri.charAt(i); switch (c) { case '/': - dots = 0; + slash = true; break; case '.': - if (dots == 0) - { - dots = 1; + if (slash) break loop; - } - dots = -1; + slash = false; break; + case '?': + case '#': + // Nothing to normalize so return original path + return uri; + default: - dots = -1; + slash = false; } i++; } + // Nothing to normalize so return original path if (i == end) - return path; + return uri; - StringBuilder canonical = new StringBuilder(path.length()); - canonical.append(path, 0, i); + // We probably need to normalize, so copy to path so far into builder + StringBuilder canonical = new StringBuilder(uri.length()); + canonical.append(uri, 0, i); + // Loop looking for single and double dot segments + int dots = 1; i++; - while (i <= end) + loop : while (i < end) { - char c = i < end ? path.charAt(i) : '\0'; + char c = uri.charAt(i); switch (c) { - case '\0': - if (dots == 2) - { - if (canonical.length() < 2) - return null; - canonical.setLength(canonical.length() - 1); - canonical.setLength(canonical.lastIndexOf("/") + 1); - } - break; - case '/': - switch (dots) - { - case 1: - break; - - case 2: - if (canonical.length() < 2) - return null; - canonical.setLength(canonical.length() - 1); - canonical.setLength(canonical.lastIndexOf("/") + 1); - break; - - default: - canonical.append(c); - } + if (doDotsSlash(canonical, dots)) + return null; + slash = true; dots = 0; break; + case '?': + case '#': + // finish normalization at a query + break loop; + case '.': - switch (dots) - { - case 0: - dots = 1; - break; - case 1: - dots = 2; - break; - case 2: - canonical.append("..."); - dots = -1; - break; - default: - canonical.append('.'); - } + // Count dots only if they are leading in the segment + if (dots > 0) + dots++; + else if (slash) + dots = 1; + else + canonical.append('.'); + slash = false; break; default: - switch (dots) - { - case 1: - canonical.append('.'); - break; - case 2: - canonical.append(".."); - break; - default: - } + // Add leading dots to the path + while (dots-- > 0) + canonical.append('.'); canonical.append(c); - dots = -1; + dots = 0; + slash = false; } - i++; } + + // process any remaining dots + if (doDots(canonical, dots)) + return null; + + // append any query + if (i < end) + canonical.append(uri, i, end); + return canonical.toString(); } /** - * Convert a path to a cananonical form. - *

- * All instances of "." and ".." are factored out. - *

+ * @param path the encoded URI from the path onwards, which may contain query strings and/or fragments + * @return the canonical path, or null if path traversal above root. + * @deprecated Use {@link #canonicalURI(String)} + */ + @Deprecated + public static String canonicalEncodedPath(String path) + { + return canonicalURI(path); + } + + /** + * Convert a decoded URI path to a canonical form. *

+ * All segments of "." and ".." are factored out. * Null is returned if the path tries to .. above its root. *

* - * @param path the path to convert (expects URI/URL form, encoded, and with path separators '/') + * @param path the decoded URI path to convert. Any special characters (e.g. '?', "#") are assumed to be part of + * the path segments. * @return the canonical path, or null if path traversal above root. + * @see #canonicalURI(String) */ - public static String canonicalEncodedPath(String path) + public static String canonicalPath(String path) { if (path == null || path.isEmpty()) return path; @@ -921,8 +916,8 @@ public static String canonicalEncodedPath(String path) int end = path.length(); int i = 0; - loop: - while (i < end) + // Initially just loop looking if we may need to normalize + loop: while (i < end) { char c = path.charAt(i); switch (c) @@ -937,9 +932,6 @@ public static String canonicalEncodedPath(String path) slash = false; break; - case '?': - return path; - default: slash = false; } @@ -947,56 +939,31 @@ public static String canonicalEncodedPath(String path) i++; } + // Nothing to normalize so return original path if (i == end) return path; + // We probably need to normalize, so copy to path so far into builder StringBuilder canonical = new StringBuilder(path.length()); canonical.append(path, 0, i); + // Loop looking for single and double dot segments int dots = 1; i++; - while (i <= end) + while (i < end) { - char c = i < end ? path.charAt(i) : '\0'; + char c = path.charAt(i); switch (c) { - case '\0': case '/': - case '?': - switch (dots) - { - case 0: - if (c != '\0') - canonical.append(c); - break; - - case 1: - if (c == '?') - canonical.append(c); - break; - - case 2: - if (canonical.length() < 2) - return null; - canonical.setLength(canonical.length() - 1); - canonical.setLength(canonical.lastIndexOf("/") + 1); - if (c == '?') - canonical.append(c); - break; - default: - while (dots-- > 0) - { - canonical.append('.'); - } - if (c != '\0') - canonical.append(c); - } - + if (doDotsSlash(canonical, dots)) + return null; slash = true; dots = 0; break; case '.': + // Count dots only if they are leading in the segment if (dots > 0) dots++; else if (slash) @@ -1007,20 +974,66 @@ else if (slash) break; default: + // Add leading dots to the path while (dots-- > 0) - { canonical.append('.'); - } canonical.append(c); dots = 0; slash = false; } - i++; } + + // process any remaining dots + if (doDots(canonical, dots)) + return null; + return canonical.toString(); } + private static boolean doDots(StringBuilder canonical, int dots) + { + switch (dots) + { + case 0: + case 1: + break; + case 2: + if (canonical.length() < 2) + return true; + canonical.setLength(canonical.length() - 1); + canonical.setLength(canonical.lastIndexOf("/") + 1); + break; + default: + while (dots-- > 0) + canonical.append('.'); + } + return false; + } + + private static boolean doDotsSlash(StringBuilder canonical, int dots) + { + switch (dots) + { + case 0: + canonical.append('/'); + break; + case 1: + break; + case 2: + if (canonical.length() < 2) + return true; + canonical.setLength(canonical.length() - 1); + canonical.setLength(canonical.lastIndexOf("/") + 1); + break; + default: + while (dots-- > 0) + canonical.append('.'); + canonical.append('/'); + } + return false; + } + /** * Convert a path to a compact form. * All instances of "//" and "///" etc. are factored out to single "/" diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8Appendable.java b/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8Appendable.java index f5e70fc7f352..ea7305ff86e2 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8Appendable.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8Appendable.java @@ -213,7 +213,6 @@ public boolean append(byte[] b, int offset, int length, int maxChars) protected void appendByte(byte b) throws IOException { - if (b > 0 && _state == UTF8_ACCEPT) { _appendable.append((char)(b & 0xFF)); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java index 244ae2ad6338..69c438e4adab 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java @@ -56,7 +56,7 @@ public class PathResource extends Resource private final URI uri; private final boolean belongsToDefaultFileSystem; - private final Path checkAliasPath() + private Path checkAliasPath() { Path abs = path; @@ -68,7 +68,6 @@ private final Path checkAliasPath() * we will just use the original URI to construct the * alias reference Path. */ - if (!URIUtil.equalsIgnoreEncodings(uri, path.toUri())) { try @@ -85,9 +84,14 @@ private final Path checkAliasPath() } if (!abs.isAbsolute()) - { abs = path.toAbsolutePath(); - } + + // Any normalization difference means it's an alias, + // and we don't want to bother further to follow + // symlinks as it's an alias anyway. + Path normal = path.normalize(); + if (!isSameName(abs, normal)) + return normal; try { @@ -96,11 +100,8 @@ private final Path checkAliasPath() if (Files.exists(path)) { Path real = abs.toRealPath(FOLLOW_LINKS); - if (!isSameName(abs, real)) - { return real; - } } } catch (IOException e) @@ -233,8 +234,7 @@ public PathResource(Path path) LOG.debug("Unable to get real/canonical path for {}", path, e); } - // cleanup any lingering relative path nonsense (like "/./" and "/../") - this.path = absPath.normalize(); + this.path = absPath; assertValidPath(path); this.uri = this.path.toUri(); @@ -254,7 +254,7 @@ private PathResource(PathResource parent, String childPath) // Calculate the URI and the path separately, so that any aliasing done by // FileSystem.getPath(path,childPath) is visible as a difference to the URI // obtained via URIUtil.addDecodedPath(uri,childPath) - + // The checkAliasPath normalization checks will only work correctly if the getPath implementation here does not normalize. this.path = parent.path.getFileSystem().getPath(parent.path.toString(), childPath); if (isDirectory() && !childPath.endsWith("/")) childPath += "/"; @@ -348,22 +348,23 @@ public boolean isSame(Resource resource) } @Override - public Resource addPath(final String subpath) throws IOException + public Resource addPath(final String subPath) throws IOException { - String cpath = URIUtil.canonicalPath(subpath); - - if ((cpath == null) || (cpath.length() == 0)) - throw new MalformedURLException(subpath); + // Check that the path is within the root, + // but use the original path to create the + // resource, to preserve aliasing. + if (URIUtil.canonicalPath(subPath) == null) + throw new MalformedURLException(subPath); - if ("/".equals(cpath)) + if ("/".equals(subPath)) return this; - // subpaths are always under PathResource - // compensate for input subpaths like "/subdir" + // Sub-paths are always under PathResource + // compensate for input sub-paths like "/subdir" // where default resolve behavior would be // to treat that like an absolute path - return new PathResource(this, subpath); + return new PathResource(this, subPath); } private void assertValidPath(Path path) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java index a45421a2a2ac..585855dd0cb2 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java @@ -419,10 +419,12 @@ public abstract boolean renameTo(Resource dest) * Returns the resource contained inside the current resource with the * given name, which may or may not exist. * - * @param path The path segment to add, which is not encoded + * @param path The path segment to add, which is not encoded. The path may be non canonical, but if so then + * the resulting Resource will return true from {@link #isAlias()}. * @return the Resource for the resolved path within this Resource, never null * @throws IOException if unable to resolve the path - * @throws MalformedURLException if the resolution of the path fails because the input path parameter is malformed. + * @throws MalformedURLException if the resolution of the path fails because the input path parameter is malformed, or + * a relative path attempts to access above the root resource. */ public abstract Resource addPath(String path) throws IOException, MalformedURLException; @@ -477,6 +479,7 @@ public URI getAlias() */ public String getListHTML(String base, boolean parent, String query) throws IOException { + // This method doesn't check aliases, so it is OK to canonicalize here. base = URIUtil.canonicalPath(base); if (base == null || !isDirectory()) return null; diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java index 74a260c2ecda..6ed655080333 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java @@ -280,9 +280,11 @@ public String[] list() public Resource addPath(String path) throws IOException { - Objects.requireNonNull(path, "Path may not be null"); - - path = URIUtil.canonicalPath(path); + // Check that the path is within the root, + // but use the original path to create the + // resource, to preserve aliasing. + if (URIUtil.canonicalPath(path) == null) + throw new MalformedURLException(path); return newResource(URIUtil.addEncodedPaths(_url.toExternalForm(), URIUtil.encodePath(path)), _useCaches); } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java index 6a9c88d494ea..d6e11da38ce1 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java @@ -22,10 +22,11 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; public class URIUtilCanonicalPathTest { - public static Stream data() + public static Stream paths() { String[][] canonical = { @@ -83,6 +84,7 @@ public static Stream data() {"/foo/../bar//", "/bar//"}, {"/ctx/../bar/../ctx/all/index.txt", "/ctx/all/index.txt"}, {"/down/.././index.html", "/index.html"}, + {"/aaa/bbb/ccc/..", "/aaa/bbb/"}, // Path traversal up past root {"..", null}, @@ -95,10 +97,8 @@ public static Stream data() {"a/../..", null}, {"/foo/../../bar", null}, - // Query parameter specifics - {"/ctx/dir?/../index.html", "/ctx/index.html"}, - {"/get-files?file=/etc/passwd", "/get-files?file=/etc/passwd"}, - {"/get-files?file=../../../../../passwd", null}, + // Encoded ? + {"/ctx/dir%3f/../index.html", "/ctx/index.html"}, // Known windows shell quirks {"file.txt ", "file.txt "}, // with spaces @@ -120,7 +120,6 @@ public static Stream data() {"/%2e%2e/", "/%2e%2e/"}, // paths with parameters are not elided - // canonicalPath() is not responsible for decoding characters {"/foo/.;/bar", "/foo/.;/bar"}, {"/foo/..;/bar", "/foo/..;/bar"}, {"/foo/..;/..;/bar", "/foo/..;/..;/bar"}, @@ -139,9 +138,43 @@ public static Stream data() } @ParameterizedTest - @MethodSource("data") + @MethodSource("paths") public void testCanonicalPath(String input, String expectedResult) { + // Check canonicalPath assertThat(URIUtil.canonicalPath(input), is(expectedResult)); + + // Check canonicalURI + if (expectedResult == null) + { + assertThat(URIUtil.canonicalURI(input), nullValue()); + } + else + { + // mostly encodedURI will be the same + assertThat(URIUtil.canonicalURI(input), is(expectedResult)); + // but will terminate on fragments and queries + assertThat(URIUtil.canonicalURI(input + "?/foo/../bar/."), is(expectedResult + "?/foo/../bar/.")); + assertThat(URIUtil.canonicalURI(input + "#/foo/../bar/."), is(expectedResult + "#/foo/../bar/.")); + } + } + + public static Stream queries() + { + String[][] data = + { + {"/ctx/../dir?/../index.html", "/dir?/../index.html"}, + {"/get-files?file=/etc/passwd", "/get-files?file=/etc/passwd"}, + {"/get-files?file=../../../../../passwd", "/get-files?file=../../../../../passwd"} + }; + return Stream.of(data).map(Arguments::of); + } + + @ParameterizedTest + @MethodSource("queries") + public void testQuery(String input, String expectedPath) + { + String actual = URIUtil.canonicalURI(input); + assertThat(actual, is(expectedPath)); } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/Utf8AppendableTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/Utf8AppendableTest.java index 384a2aaf6458..4786da17eb01 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/Utf8AppendableTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/Utf8AppendableTest.java @@ -151,6 +151,35 @@ public void testInvalidUTF8(Class impl) throws UnsupportedEncodi }); } + @ParameterizedTest + @MethodSource("implementations") + public void testInvalidZeroUTF8(Class impl) throws UnsupportedEncodingException + { + // From https://datatracker.ietf.org/doc/html/rfc3629#section-10 + assertThrows(Utf8Appendable.NotUtf8Exception.class, () -> + { + Utf8Appendable buffer = impl.getDeclaredConstructor().newInstance(); + buffer.append((byte)0xC0); + buffer.append((byte)0x80); + }); + } + + @ParameterizedTest + @MethodSource("implementations") + public void testInvalidAlternateDotEncodingUTF8(Class impl) throws UnsupportedEncodingException + { + // From https://datatracker.ietf.org/doc/html/rfc3629#section-10 + assertThrows(Utf8Appendable.NotUtf8Exception.class, () -> + { + Utf8Appendable buffer = impl.getDeclaredConstructor().newInstance(); + buffer.append((byte)0x2f); + buffer.append((byte)0xc0); + buffer.append((byte)0xae); + buffer.append((byte)0x2e); + buffer.append((byte)0x2f); + }); + } + @ParameterizedTest @MethodSource("implementations") public void testFastFail1(Class impl) throws Exception diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java index 1ee036f58548..b1787fe2d726 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java @@ -16,6 +16,7 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.net.MalformedURLException; import java.net.URI; import java.net.URL; import java.nio.file.Path; @@ -40,6 +41,8 @@ import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; public class ResourceTest { @@ -320,4 +323,19 @@ public void testEqualsWindowsCaseInsensitiveDrive() throws Exception assertEquals(rb, ra); } + + @Test + public void testClimbAboveBase() throws Exception + { + Resource resource = Resource.newResource("/foo/bar"); + assertThrows(MalformedURLException.class, () -> resource.addPath("..")); + + Resource same = resource.addPath("."); + assertNotNull(same); + assertTrue(same.isAlias()); + + assertThrows(MalformedURLException.class, () -> resource.addPath("./..")); + + assertThrows(MalformedURLException.class, () -> resource.addPath("./../bar")); + } } diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java index ecd65c66fcc6..1a2af27da4c7 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java @@ -400,7 +400,7 @@ public Resource getResource(String uriInContext) throws MalformedURLException if (uriInContext == null || !uriInContext.startsWith(URIUtil.SLASH)) throw new MalformedURLException(uriInContext); - IOException ioe = null; + MalformedURLException mue = null; Resource resource = null; int loop = 0; while (uriInContext != null && loop++ < 100) @@ -413,16 +413,16 @@ public Resource getResource(String uriInContext) throws MalformedURLException uriInContext = getResourceAlias(uriInContext); } - catch (IOException e) + catch (MalformedURLException e) { LOG.trace("IGNORED", e); - if (ioe == null) - ioe = e; + if (mue == null) + mue = e; } } - if (ioe instanceof MalformedURLException) - throw (MalformedURLException)ioe; + if (mue != null) + throw mue; return resource; } @@ -1438,6 +1438,9 @@ public void checkListener(Class listener) throws Illega @Override public URL getResource(String path) throws MalformedURLException { + if (path == null) + return null; + Resource resource = WebAppContext.this.getResource(path); if (resource == null || !resource.exists()) return null; diff --git a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java index 83f67fdc2d2d..765a21867813 100644 --- a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java +++ b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java @@ -53,6 +53,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -328,6 +329,45 @@ public void testProtectedTarget() throws Exception assertThat(HttpTester.parseResponse(connector.getResponse("GET //WEB-INF/test.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.NOT_FOUND_404)); assertThat(HttpTester.parseResponse(connector.getResponse("GET /WEB-INF%2ftest.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.NOT_FOUND_404)); } + + @ParameterizedTest + @ValueSource(strings = { + "/WEB-INF", + "/WEB-INF/", + "/WEB-INF/test.xml", + "/web-inf/test.xml", + "/%2e/WEB-INF/test.xml", + "/%2e/%2e/WEB-INF/test.xml", + "/foo/%2e%2e/WEB-INF/test.xml", + "/%2E/WEB-INF/test.xml", + "//WEB-INF/test.xml", + "/WEB-INF%2ftest.xml", + "/.%00/WEB-INF/test.xml", + "/WEB-INF%00/test.xml" + }) + public void testProtectedTargetFailure(String path) throws Exception + { + Server server = newServer(); + + LocalConnector connector = new LocalConnector(server); + server.addConnector(connector); + connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.LEGACY); + + HandlerList handlers = new HandlerList(); + ContextHandlerCollection contexts = new ContextHandlerCollection(); + WebAppContext context = new WebAppContext(); + Path testWebapp = MavenTestingUtils.getProjectDirPath("src/test/webapp"); + context.setBaseResource(new PathResource(testWebapp)); + context.setContextPath("/"); + server.setHandler(handlers); + handlers.addHandler(contexts); + contexts.addHandler(context); + + server.start(); + + assertThat(HttpTester.parseResponse(connector.getResponse("GET " + path + " HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), + Matchers.anyOf(is(HttpStatus.NOT_FOUND_404), is(HttpStatus.BAD_REQUEST_400))); + } @Test public void testNullPath() throws Exception diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/jsp/JspAndDefaultWithoutAliasesTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/jsp/JspAndDefaultWithoutAliasesTest.java index 3cbf66f8ac37..f841c023ecc4 100644 --- a/tests/test-integration/src/test/java/org/eclipse/jetty/test/jsp/JspAndDefaultWithoutAliasesTest.java +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/jsp/JspAndDefaultWithoutAliasesTest.java @@ -22,8 +22,12 @@ import java.util.List; import java.util.stream.Stream; +import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.security.HashLoginService; +import org.eclipse.jetty.server.Connector; +import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.NetworkConnector; +import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.servlet.DefaultServlet; import org.eclipse.jetty.servlet.ServletContextHandler; @@ -56,13 +60,21 @@ public static Stream aliases() data.add(Arguments.of("/dump.jsp")); data.add(Arguments.of("/dump.jsp/")); - data.add(Arguments.of("/dump.jsp%00")); - data.add(Arguments.of("/dump.jsp%00x")); - data.add(Arguments.of("/dump.jsp%00x/dump.jsp")); - data.add(Arguments.of("/dump.jsp%00/dump.jsp")); - data.add(Arguments.of("/dump.jsp%00/index.html")); - data.add(Arguments.of("/dump.jsp%00/")); - data.add(Arguments.of("/dump.jsp%00x/")); + data.add(Arguments.of("/dump.jsp%1e")); + data.add(Arguments.of("/dump.jsp%1ex")); + data.add(Arguments.of("/dump.jsp%1ex/dump.jsp")); + data.add(Arguments.of("/dump.jsp%1e/dump.jsp")); + data.add(Arguments.of("/dump.jsp%1e/index.html")); + data.add(Arguments.of("/dump.jsp%1e/")); + data.add(Arguments.of("/dump.jsp%1ex/")); + // The _00_ is later replaced with a real null character in a customizer + data.add(Arguments.of("/dump.jsp_00_")); + data.add(Arguments.of("/dump.jsp_00_")); + data.add(Arguments.of("/dump.jsp_00_/dump.jsp")); + data.add(Arguments.of("/dump.jsp_00_/dump.jsp")); + data.add(Arguments.of("/dump.jsp_00_/index.html")); + data.add(Arguments.of("/dump.jsp_00_/")); + data.add(Arguments.of("/dump.jsp_00_/")); return data.stream(); } @@ -98,6 +110,17 @@ public static void startServer() throws Exception // add context server.setHandler(context); + // Add customizer to convert "_00_" to a real null + server.getContainedBeans(HttpConfiguration.class).forEach(config -> + { + config.addCustomizer((connector, channelConfig, request) -> + { + HttpURI uri = request.getHttpURI(); + if (uri.getPath().contains("_00_")) + request.setHttpURI(HttpURI.build(uri, uri.getPath().replace("_00_", "\000"), uri.getParam(), uri.getQuery())); + }); + }); + server.start(); int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort();