From c2d5a82c1c35f26455f096c97be50d3cddbda73d Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 28 Jun 2021 15:52:14 +1000 Subject: [PATCH] Issue #6473 - canonicalPath refactor & fix alias check in PathResource Signed-off-by: Lachlan Roberts --- .../eclipse/jetty/http/HttpCompliance.java | 43 +++ .../java/org/eclipse/jetty/http/HttpURI.java | 136 +++++++--- .../org/eclipse/jetty/http/HttpURITest.java | 131 ++++----- .../maven/plugin/JettyWebAppContext.java | 15 +- .../jetty/rewrite/handler/RedirectUtil.java | 4 +- .../rewrite/handler/ValidUrlRuleTest.java | 18 +- .../org/eclipse/jetty/server/Dispatcher.java | 35 ++- .../org/eclipse/jetty/server/Request.java | 29 +- .../org/eclipse/jetty/server/Response.java | 4 +- .../jetty/server/handler/ContextHandler.java | 31 +-- .../jetty/server/handler/ResourceHandler.java | 1 - .../jetty/server/HttpConnectionTest.java | 6 - .../ContextHandlerGetResourceTest.java | 34 ++- .../eclipse/jetty/servlet/DefaultServlet.java | 4 +- .../eclipse/jetty/servlet/RequestURITest.java | 4 +- .../servlets/DataRateLimitedServlet.java | 7 +- .../org/eclipse/jetty/servlets/PutFilter.java | 8 +- .../java/org/eclipse/jetty/util/URIUtil.java | 250 +++++++++--------- .../eclipse/jetty/util/Utf8Appendable.java | 1 - .../jetty/util/resource/FileResource.java | 1 - .../jetty/util/resource/PathResource.java | 20 +- .../eclipse/jetty/util/resource/Resource.java | 1 - .../util/resource/ResourceCollection.java | 3 +- .../jetty/util/resource/URLResource.java | 4 +- .../jetty/util/URIUtilCanonicalPathTest.java | 27 +- .../jetty/util/Utf8AppendableTest.java | 29 ++ .../eclipse/jetty/webapp/WebAppContext.java | 15 +- .../jetty/webapp/WebAppContextTest.java | 72 +++-- .../jsp/JspAndDefaultWithoutAliasesTest.java | 51 +++- 29 files changed, 602 insertions(+), 382 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index fd0dece4b80a..319b4afb87c0 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.http; +import java.util.EnumMap; import java.util.EnumSet; import java.util.HashMap; import java.util.Map; @@ -214,4 +215,46 @@ public EnumSet sections() { return _sections; } + + private static final EnumMap __uriViolations = new EnumMap<>(HttpURI.Violation.class); + static + { + // create a map from Violation to compliance in a loop, so that any new violations added are detected with ISE + for (HttpURI.Violation violation : HttpURI.Violation.values()) + { + switch (violation) + { + case SEPARATOR: + __uriViolations.put(violation, HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS); + break; + case SEGMENT: + __uriViolations.put(violation, HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS); + break; + case PARAM: + __uriViolations.put(violation, HttpComplianceSection.NO_AMBIGUOUS_PATH_PARAMETERS); + break; + case ENCODING: + __uriViolations.put(violation, HttpComplianceSection.NO_AMBIGUOUS_PATH_ENCODING); + break; + case EMPTY: + __uriViolations.put(violation, HttpComplianceSection.NO_AMBIGUOUS_EMPTY_SEGMENT); + break; + case UTF16: + __uriViolations.put(violation, HttpComplianceSection.NO_UTF16_ENCODINGS); + break; + default: + throw new IllegalStateException(); + } + } + } + + public static String checkUriCompliance(HttpCompliance compliance, HttpURI uri) + { + for (HttpURI.Violation violation : HttpURI.Violation.values()) + { + if (uri.hasViolation(violation) && (compliance == null || compliance.sections().contains(__uriViolations.get(violation)))) + return violation.getMessage(); + } + return null; + } } 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 4087291dacbb..5dc6db1302aa 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 @@ -36,22 +36,45 @@ /** * Http URI. * Parse an HTTP URI from a string or byte array. Given a URI - * http://user@host:port/path/info;param?query#fragment - * this class will split it into the following undecoded optional elements:
    + * http://user@host:port/path;param1/%2e/info;param2?query#fragment + * this class will split it 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 #hasViolation(Violation)} 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 class HttpURI { private enum State @@ -69,28 +92,49 @@ private enum State ASTERISK } - enum Violation + /** + * Violations of safe URI interpretations + */ + public enum Violation { - SEGMENT, - SEPARATOR, - PARAM, - ENCODING, - EMPTY, - UTF16 + /** + * Ambiguous path segments e.g. /foo/%2E%2E/bar + */ + SEGMENT("Ambiguous path segments"), + /** + * Ambiguous path separator within a URI segment e.g. /foo%2Fbar + */ + SEPARATOR("Ambiguous path separator"), + /** + * Ambiguous path parameters within a URI segment e.g. /foo/..;/bar + */ + PARAM("Ambiguous path parameters"), + /** + * Ambiguous double encoding within a URI segment e.g. /%2557EB-INF + */ + ENCODING("Ambiguous double encoding"), + /** + * Ambiguous empty segments e.g. /foo//bar + */ + EMPTY("Ambiguous empty segments"), + /** + * Non standard UTF-16 encoding eg /foo%u2192bar. + */ + UTF16("Non standard UTF-16 encoding"); + + private final String _message; + + Violation(String message) + { + _message = message; + } + + String getMessage() + { + return _message; + } } - /** - * The concept of URI path parameters was originally specified in - * RFC2396, but that was - * obsoleted by - * RFC3986 which removed - * a normative definition of path parameters. Specifically it excluded them from the - * Remove Dot Segments - * algorithm. This results in some ambiguity as dot segments can result from later - * parameter removal or % encoding expansion, that are not removed from the URI - * by {@link URIUtil#canonicalPath(String)}. Thus this class flags such ambiguous - * path segments, so that they may be rejected by the server if so configured. - */ private static final Trie __ambiguousSegments = new ArrayTrie<>(); static @@ -179,6 +223,22 @@ public HttpURI(HttpURI uri) _emptySegment = false; } + public HttpURI(HttpURI schemeHostPort, HttpURI uri) + { + _scheme = schemeHostPort._scheme; + _user = schemeHostPort._user; + _host = schemeHostPort._host; + _port = schemeHostPort._port; + _path = uri._path; + _param = uri._param; + _query = uri._query; + _fragment = uri._fragment; + _uri = uri._uri; + _decodedPath = uri._decodedPath; + _violations.addAll(uri._violations); + _emptySegment = false; + } + public HttpURI(String uri) { _port = -1; @@ -506,6 +566,8 @@ else if (c == '/') { switch (encodedValue) { + case 0: + throw new IllegalArgumentException("Illegal character in path"); case '/': _violations.add(Violation.SEPARATOR); break; @@ -677,10 +739,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 decodeNonCanonical = URIUtil.decodePath(_path); + _decodedPath = URIUtil.canonicalPath(decodeNonCanonical); + if (_decodedPath == null) + throw new IllegalArgumentException("Bad URI"); } } @@ -794,6 +858,11 @@ public boolean hasViolations() return !_violations.isEmpty(); } + public boolean hasViolation(Violation violation) + { + return _violations.contains(violation); + } + /** * @return True if the URI encodes UTF-16 characters with '%u'. */ @@ -839,6 +908,11 @@ public String getDecodedPath() return _decodedPath; } + /** + * 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 + */ public String getParam() { return _param; 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 ddb71f29d940..2489c75cf34e 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 @@ -87,6 +87,11 @@ public void testParse() uri.parse("http://foo/bar"); 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. + uri.parse("http://fo\000/bar"); + assertThat(uri.getHost(), is("fo\000")); + assertThat(uri.getPath(), is("/bar")); } @Test @@ -316,6 +321,7 @@ 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%u0001%u0001/bar", "/f\001\001/bar", EnumSet.of(Violation.UTF16)}, // illegal paths {"//host/../path/info", null, EnumSet.noneOf(Violation.class)}, @@ -325,32 +331,37 @@ 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)}, + {"%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.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", "/info", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.SEGMENT)}, + {"%2e/info", "info", EnumSet.of(Violation.SEGMENT)}, + {"%u002e/info", "info", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, + + {"%2e", "", EnumSet.of(Violation.SEGMENT)}, + {"%u002e", "", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, // empty segment treated as ambiguous {"/foo//bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, @@ -368,20 +379,18 @@ public static Stream decodePathTests() {"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.PARAM)}, + {"/path/.;param/info", "/path/info", EnumSet.of(Violation.PARAM)}, + {"/path/..;/info", "/info", EnumSet.of(Violation.PARAM)}, + {"/path/..;param/info", "/info", EnumSet.of(Violation.PARAM)}, + {".;/info", "info", EnumSet.of(Violation.PARAM)}, + {".;param/info", "info", EnumSet.of(Violation.PARAM)}, // 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.SEPARATOR)}, // ambiguous encoding {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.ENCODING)}, @@ -391,9 +400,9 @@ public static Stream decodePathTests() {"/path/%u0025../info", "/path/%../info", EnumSet.of(Violation.ENCODING, Violation.UTF16)}, // 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.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)}, // Non ascii characters // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck @@ -448,21 +457,23 @@ 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.SEGMENT)}, + {"path/%2e/info/", "path/info/", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.SEGMENT)}, + {"%2e/info", "info", EnumSet.of(Violation.SEGMENT)}, + {"%2e", "", EnumSet.of(Violation.SEGMENT)}, // empty segment treated as ambiguous {"/", "/", EnumSet.noneOf(Violation.class)}, @@ -496,20 +507,18 @@ public static Stream testPathQueryTests() {"", "", 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.PARAM)}, + {"/path/.;param/info", "/path/info", EnumSet.of(Violation.PARAM)}, + {"/path/..;/info", "/info", EnumSet.of(Violation.PARAM)}, + {"/path/..;param/info", "/info", EnumSet.of(Violation.PARAM)}, + {".;/info", "info", EnumSet.of(Violation.PARAM)}, + {".;param/info", "info", EnumSet.of(Violation.PARAM)}, // 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.SEPARATOR)}, // ambiguous encoding {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.ENCODING)}, @@ -517,9 +526,9 @@ public static Stream testPathQueryTests() {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.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.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)}, }).map(Arguments::of); } diff --git a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyWebAppContext.java b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyWebAppContext.java index 04502e336cea..d2efd9018989 100644 --- a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyWebAppContext.java +++ b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyWebAppContext.java @@ -39,7 +39,6 @@ import org.eclipse.jetty.servlet.ServletHolder; import org.eclipse.jetty.servlet.ServletMapping; import org.eclipse.jetty.util.StringUtil; -import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.resource.Resource; @@ -451,16 +450,12 @@ public Resource getResource(String uriInContext) throws MalformedURLException // If no regular resource exists check for access to /WEB-INF/lib or /WEB-INF/classes if ((resource == null || !resource.exists()) && uriInContext != null && _classes != null) { - String uri = URIUtil.canonicalPath(uriInContext); - if (uri == null) - return null; - try { // Replace /WEB-INF/classes with candidates for the classpath - if (uri.startsWith(WEB_INF_CLASSES_PREFIX)) + if (uriInContext.startsWith(WEB_INF_CLASSES_PREFIX)) { - if (uri.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX) || uri.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX + "/")) + if (uriInContext.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX) || uriInContext.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX + "/")) { //exact match for a WEB-INF/classes, so preferentially return the resource matching the web-inf classes //rather than the test classes @@ -476,7 +471,7 @@ else if (_testClasses != null) int i = 0; while (res == null && (i < _webInfClasses.size())) { - String newPath = StringUtil.replace(uri, WEB_INF_CLASSES_PREFIX, _webInfClasses.get(i).getPath()); + String newPath = StringUtil.replace(uriInContext, WEB_INF_CLASSES_PREFIX, _webInfClasses.get(i).getPath()); res = Resource.newResource(newPath); if (!res.exists()) { @@ -487,11 +482,11 @@ else if (_testClasses != null) return res; } } - else if (uri.startsWith(WEB_INF_LIB_PREFIX)) + else if (uriInContext.startsWith(WEB_INF_LIB_PREFIX)) { // Return the real jar file for all accesses to // /WEB-INF/lib/*.jar - String jarName = StringUtil.strip(uri, WEB_INF_LIB_PREFIX); + String jarName = StringUtil.strip(uriInContext, WEB_INF_LIB_PREFIX); if (jarName.startsWith("/") || jarName.startsWith("\\")) jarName = jarName.substring(1); if (jarName.length() == 0) 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 6fc476debc2f..4754173ca315 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 @@ -45,14 +45,14 @@ 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)); + location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location)); if (!location.startsWith("/")) url.append('/'); } 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 8fd346ac5562..fff775784b97 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 @@ -18,8 +18,8 @@ package org.eclipse.jetty.rewrite.handler; +import org.eclipse.jetty.http.HttpURI; 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; @@ -65,7 +65,7 @@ public void testInvalidUrlWithReason() throws Exception { _rule.setCode("405"); _rule.setReason("foo"); - _request.setURIPathQuery("/%00/"); + _request.setURIPathQuery("/%01/"); String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response); @@ -78,20 +78,8 @@ public void testInvalidJsp() throws Exception { _rule.setCode("405"); _rule.setReason("foo"); - _request.setURIPathQuery("/jsp/bean1.jsp%00"); - String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response); - - assertEquals(405, _response.getStatus()); - assertEquals("foo", _response.getReason()); - } - - @Test - public void testInvalidShamrock() throws Exception - { - _rule.setCode("405"); - _rule.setReason("foo"); - _request.setURIPathQuery("/jsp/shamrock-%00%E2%98%98.jsp"); + _request.setHttpURI(new HttpURI("/jsp/bean1.jsp\000")); String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response); 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 eaf19377c3ba..1720261af194 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 @@ -20,6 +20,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; @@ -30,7 +31,9 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.BadMessageException; +import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpURI; +import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.MultiMap; @@ -86,7 +89,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); @@ -106,6 +109,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); attr._requestURI = _uri.getPath(); @@ -133,7 +140,7 @@ public void include(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(); @@ -161,6 +168,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); + ForwardAttributes attr = new ForwardAttributes(old_attr); //If we have already been forwarded previously, then keep using the established @@ -184,9 +195,8 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc attr._servletPath = old_servlet_path; } - HttpURI uri = new HttpURI(old_uri.getScheme(), old_uri.getHost(), old_uri.getPort(), - _uri.getPath(), _uri.getParam(), _uri.getQuery(), _uri.getFragment()); - + // Combine old and new URIs. + HttpURI uri = new HttpURI(old_uri, _uri); baseRequest.setHttpURI(uri); baseRequest.setContextPath(_contextHandler.getContextPath()); @@ -245,6 +255,21 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc } } + private static void checkUriViolations(HttpURI uri, Request baseRequest) + { + if (uri.hasViolations()) + { + HttpChannel channel = baseRequest.getHttpChannel(); + Connection connection = channel == null ? null : channel.getConnection(); + HttpCompliance compliance = connection instanceof HttpConnection + ? ((HttpConnection)connection).getHttpCompliance() + : channel != null ? channel.getConnector().getBean(HttpCompliance.class) : null; + String illegalState = HttpCompliance.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 48b706c1cb92..635550aec58e 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 @@ -66,7 +66,6 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HostPortHttpField; import org.eclipse.jetty.http.HttpCompliance; -import org.eclipse.jetty.http.HttpComplianceSection; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; @@ -1824,7 +1823,6 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) setMethod(request.getMethod()); HttpURI uri = request.getURI(); - boolean ambiguous = false; if (uri.hasViolations()) { // Replaced in jetty-10 with URICompliance from the HttpConfiguration. @@ -1833,23 +1831,9 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) ? ((HttpConnection)connection).getHttpCompliance() : _channel != null ? _channel.getConnector().getBean(HttpCompliance.class) : null; - if (uri.hasUtf16Encoding() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_UTF16_ENCODINGS))) - throw new BadMessageException("UTF16 % encoding not supported"); - - ambiguous = uri.isAmbiguous(); - if (ambiguous) - { - if (uri.hasAmbiguousSegment() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS))) - throw new BadMessageException("Ambiguous segment in URI"); - if (uri.hasAmbiguousEmptySegment() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_EMPTY_SEGMENT))) - throw new BadMessageException("Ambiguous empty segment in URI"); - if (uri.hasAmbiguousSeparator() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS))) - throw new BadMessageException("Ambiguous separator in URI"); - if (uri.hasAmbiguousParameter() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_PARAMETERS))) - throw new BadMessageException("Ambiguous path parameter in URI"); - if (uri.hasAmbiguousEncoding() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_ENCODING))) - throw new BadMessageException("Ambiguous path encoding in URI"); - } + String badMessage = HttpCompliance.checkUriCompliance(compliance, uri); + if (badMessage != null) + throw new BadMessageException(badMessage); } _originalURI = uri.isAbsolute() && request.getHttpVersion() != HttpVersion.HTTP_2 ? uri.toString() : uri.getPathQuery(); @@ -1864,13 +1848,6 @@ public void setMetaData(org.eclipse.jetty.http.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, previous behaviour was to always normalize - // so we will continue to do so. If an application wishes to see ambiguous URIs, then they can look - // at the encoded form of the URI - if (ambiguous) - path = URIUtil.canonicalPath(path); } else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod())) { 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 41b00cdf2600..97610f31535b 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 @@ -547,14 +547,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 4e1609584f70..c7c93a6fab66 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 @@ -34,7 +34,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; @@ -1551,14 +1550,11 @@ public boolean isProtectedTarget(String target) if (target == null || _protectedTargets == null) return false; - while (target.startsWith("//")) - { + if (target.startsWith("//")) target = URIUtil.compactPath(target); - } - for (int i = 0; i < _protectedTargets.length; i++) + for (String t : _protectedTargets) { - String t = _protectedTargets[i]; if (StringUtil.startsWithIgnoreCase(target, t)) { if (target.length() == t.length()) @@ -1946,9 +1942,13 @@ public Resource getResource(String path) throws MalformedURLException if (_baseResource == null) return null; + // Does the path go above the current scope? + path = URIUtil.canonicalPath(path); + if (path == null) + return null; + try { - path = URIUtil.canonicalPath(path); Resource resource = _baseResource.addPath(path); if (checkAlias(path, resource)) @@ -1977,9 +1977,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()) @@ -2032,7 +2031,6 @@ public Set getResourcePaths(String path) { try { - path = URIUtil.canonicalPath(path); Resource resource = getResource(path); if (resource != null && resource.exists()) @@ -2243,8 +2241,7 @@ public String getMimeType(String file) @Override public RequestDispatcher getRequestDispatcher(String uriInContext) { - // uriInContext is encoded, potentially with query - + // uriInContext is encoded, potentially with query. if (uriInContext == null) return null; @@ -2254,11 +2251,7 @@ public RequestDispatcher getRequestDispatcher(String uriInContext) try { HttpURI uri = new HttpURI(null, null, 0, uriInContext); - - String pathInfo = URIUtil.canonicalPath(uri.getDecodedPath()); - if (pathInfo == null) - return null; - + String pathInfo = uri.getDecodedPath(); String contextPath = getContextPath(); if (contextPath != null && contextPath.length() > 0) uri.setPath(URIUtil.addPaths(contextPath, uri.getPath())); @@ -2306,6 +2299,8 @@ else if (path.charAt(0) != '/') @Override public URL getResource(String path) throws MalformedURLException { + if (path == null) + return null; Resource resource = ContextHandler.this.getResource(path); if (resource != null && resource.exists()) return resource.getURI().toURL(); @@ -2342,6 +2337,8 @@ public InputStream getResourceAsStream(String path) @Override public Set getResourcePaths(String 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 b5b0db05611a..43709a297223 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 @@ -177,7 +177,6 @@ public Resource getResource(String path) if (_baseResource != null) { - path = URIUtil.canonicalPath(path); r = _baseResource.addPath(path); if (r != null && r.isAlias() && (_context == null || !_context.checkAlias(path, r))) 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 de3b00f811fc..9ce41675f850 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 @@ -829,12 +829,6 @@ public void testBadUTF8FallsbackTo8859() throws Exception Log.getLogger(HttpParser.class).info("badMessage: bad encoding expected ..."); String response; - response = connector.getResponse("GET /foo/bar%c0%00 HTTP/1.1\r\n" + - "Host: localhost\r\n" + - "Connection: close\r\n" + - "\r\n"); - checkContains(response, 0, "HTTP/1.1 200"); //now fallback to iso-8859-1 - response = connector.getResponse("GET /bad/utf8%c1 HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: close\r\n" + 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 a6a471b9347c..f3b26d86fd07 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 @@ -33,6 +33,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; @@ -223,24 +225,32 @@ 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/.././../"; - Resource resource = context.getResource(path); + Resource resource = context.getResource("/./index.html"); + assertNotNull(resource); + assertFalse(resource.isAlias()); + + resource = context.getResource("/down/../index.html"); + assertNotNull(resource); + assertFalse(resource.isAlias()); + + resource = context.getResource("//index.html"); assertNull(resource); + } + + @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 101a292a86f6..4abbf639393c 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 @@ -19,7 +19,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; @@ -424,8 +423,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 de3814accf20..02ac7ebaedb1 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 @@ -60,11 +60,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) @@ -73,7 +75,7 @@ public static Stream data() 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++) + for (int i = 0x1; i < 0x1f; i++) { String raw = String.format("/hello%%%02Xworld", i); ret.add(Arguments.of(raw, raw, null)); 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 def476ef82f0..17a16081e809 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 @@ -39,7 +39,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:

      @@ -47,8 +47,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: @@ -58,7 +57,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 86735ff5bf0e..2397fc052b01 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 @@ -62,6 +62,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"; @@ -85,7 +86,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"); @@ -95,7 +97,7 @@ public void init(FilterConfig config) throws ServletException } else { - File base = new File(_context.getRealPath("/")); + File base = new File(realPath); _baseURI = base.toURI().toString(); } @@ -289,7 +291,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 19ed06e0db13..3c6f7a53d343 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 @@ -475,7 +475,8 @@ public static String decodePath(String path, int offset, int length) char u = path.charAt(i + 1); if (u == 'u') { - // In Jetty-10 UTF16 encoding is only supported with UriCompliance.Violation.UTF16_ENCODINGS. // This is wrong. This is a codepoint not a char + // In Jetty-10 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; } @@ -780,143 +781,128 @@ 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) + * @see #canonicalURI(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. - *

+ * 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; @@ -925,8 +911,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) @@ -941,9 +927,6 @@ public static String canonicalEncodedPath(String path) slash = false; break; - case '?': - return path; - default: slash = false; } @@ -951,56 +934,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) @@ -1011,20 +969,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 f548ffa56e7c..ad73b863beb3 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 @@ -217,7 +217,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/FileResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java index 9ccfe9682a39..40ead6f17972 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java @@ -269,7 +269,6 @@ public Resource addPath(String path) throws IOException { assertValidPath(path); - path = org.eclipse.jetty.util.URIUtil.canonicalPath(path); if (path == null) throw new MalformedURLException(); 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 af377b19ebe4..22f936a62cfd 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 @@ -64,7 +64,7 @@ public class PathResource extends Resource private final URI uri; private final boolean belongsToDefaultFileSystem; - private final Path checkAliasPath() + private Path checkAliasPath() { Path abs = path; @@ -76,7 +76,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 @@ -93,9 +92,11 @@ private final Path checkAliasPath() } if (!abs.isAbsolute()) - { abs = path.toAbsolutePath(); - } + + Path normal = path.normalize(); + if (!abs.equals(normal)) + return normal; try { @@ -241,8 +242,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(); @@ -262,7 +262,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 += "/"; @@ -363,12 +363,10 @@ public boolean isSame(Resource resource) @Override public Resource addPath(final String subpath) throws IOException { - String cpath = URIUtil.canonicalPath(subpath); - - if ((cpath == null) || (cpath.length() == 0)) + if ((subpath == null) || (subpath.length() == 0)) throw new MalformedURLException(subpath); - if ("/".equals(cpath)) + if ("/".equals(subpath)) return this; // subpaths are always under PathResource 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 45744360415f..dc443becffb3 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 @@ -555,7 +555,6 @@ public String getListHTML(String base, boolean parent) throws IOException */ public String getListHTML(String base, boolean parent, String query) throws IOException { - base = URIUtil.canonicalPath(base); if (base == null || !isDirectory()) return null; diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java index 089e088abd4d..6612e931307f 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java @@ -300,7 +300,8 @@ public Resource addPath(String path) throws IOException { return new ResourceCollection(resources.toArray(new Resource[0])); } - return null; + + throw new MalformedURLException(); } @Override 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 ffd9c1c9702e..054f4d0d5ab6 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 @@ -272,9 +272,7 @@ public Resource addPath(String path) throws IOException { if (path == null) - return null; - - path = URIUtil.canonicalPath(path); + throw new MalformedURLException("null 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 82f2771a42cb..c40ff6a83c7e 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 @@ -27,10 +27,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 = { @@ -88,6 +89,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}, @@ -100,10 +102,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 @@ -125,7 +125,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"}, @@ -144,9 +143,23 @@ 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/.")); + } } + } 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 1fac71622a98..4cb8c79f7d3c 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 @@ -156,6 +156,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-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java index 67b9cfac21f9..8bf6fe0aad1f 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 @@ -379,7 +379,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) @@ -392,16 +392,16 @@ public Resource getResource(String uriInContext) throws MalformedURLException uriInContext = getResourceAlias(uriInContext); } - catch (IOException e) + catch (MalformedURLException e) { LOG.ignore(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; } @@ -1556,6 +1556,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 48e9b3fa3147..86c65880d697 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 @@ -54,12 +54,14 @@ import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.resource.PathResource; import org.eclipse.jetty.util.resource.Resource; +import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; 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 static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.instanceOf; @@ -249,8 +251,50 @@ public void testIsProtected() assertFalse(context.isProtectedTarget("/something-else/web-inf")); } - @Test - public void testProtectedTarget() throws Exception + @ParameterizedTest + @ValueSource(strings = { + "/test.xml", + "/%2e/%2e/test.xml", + "/foo/%2e%2e/test.xml" + }) + public void testProtectedTargetSuccess(String path) throws Exception + { + Server server = newServer(); + server.getConnectors()[0].getConnectionFactory(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.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); + + LocalConnector connector = new LocalConnector(server); + server.addConnector(connector); + + server.start(); + assertThat(HttpTester.parseResponse(connector.getResponse("GET " + path + " HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.OK_200)); + } + + @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(); server.getConnectors()[0].getConnectionFactory(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.LEGACY); @@ -269,27 +313,9 @@ public void testProtectedTarget() throws Exception server.addConnector(connector); server.start(); - assertThat(HttpTester.parseResponse(connector.getResponse("GET /test.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.OK_200)); - assertThat(HttpTester.parseResponse(connector.getResponse("GET /%2e/%2e/test.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.OK_200)); - assertThat(HttpTester.parseResponse(connector.getResponse("GET /%u002e/%u002e/test.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.OK_200)); - assertThat(HttpTester.parseResponse(connector.getResponse("GET /foo/%2e%2e/test.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.OK_200)); - assertThat(HttpTester.parseResponse(connector.getResponse("GET /foo/%u002e%u002e/test.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.OK_200)); - - assertThat(HttpTester.parseResponse(connector.getResponse("GET /WEB-INF 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/ 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/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/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 /%2e/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 /%u002e/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 /%2e/%2e/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 /%u002e/%u002e/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 /foo/%2e%2e/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 /foo/%u002e%u002e/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 /%2E/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 /%u002E/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/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)); - assertThat(HttpTester.parseResponse(connector.getResponse("GET /WEB-INF%u002ftest.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 " + 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 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 2c0ac83aef8b..3ed1f6150817 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 @@ -27,8 +27,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; @@ -61,13 +65,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(); } @@ -103,6 +115,31 @@ 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(new HttpConfiguration.Customizer() + { + @Override + public void customize(Connector connector, HttpConfiguration channelConfig, Request request) + { + HttpURI uri = request.getHttpURI(); + if (uri.getPath().contains("_00_")) + { + request.setHttpURI(new HttpURI( + uri.getScheme(), + uri.getHost(), + uri.getPort(), + uri.getPath().replace("_00_", "\000"), + uri.getParam(), + uri.getQuery(), + uri.getFragment() + )); + } + } + }); + }); + server.start(); int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort();