From 36332ab052b0ca70684a660097f5bcfe7b239e83 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 20 May 2021 12:36:16 +1000 Subject: [PATCH 01/13] Issue #6302 - Treat empty path segments are ambiguous. Signed-off-by: Lachlan Roberts --- .../main/java/org/eclipse/jetty/http/HttpURI.java | 5 +++-- .../java/org/eclipse/jetty/http/HttpURITest.java | 13 +++++++++++++ .../main/java/org/eclipse/jetty/server/Request.java | 2 +- .../jetty/server/handler/ContextHandler.java | 4 ++++ .../java/org/eclipse/jetty/server/RequestTest.java | 11 +++++++++++ 5 files changed, 32 insertions(+), 3 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 730021871333..7cf2b46a731e 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 @@ -377,13 +377,13 @@ public boolean isAmbiguous() @Override public boolean hasAmbiguousSegment() { - return _ambiguous.contains(Mutable.Ambiguous.SEGMENT); + return _ambiguous.contains(Ambiguous.SEGMENT); } @Override public boolean hasAmbiguousSeparator() { - return _ambiguous.contains(Mutable.Ambiguous.SEPARATOR); + return _ambiguous.contains(Ambiguous.SEPARATOR); } @Override @@ -453,6 +453,7 @@ private enum State .with("%2e%2e", Boolean.TRUE) .with(".%2e", Boolean.TRUE) .with("%2e.", Boolean.TRUE) + .with("", Boolean.TRUE) .with("..", Boolean.FALSE) .with(".", Boolean.FALSE) .build(); 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 7c23d38eb2ed..e52868b779fd 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 @@ -361,6 +361,13 @@ public static Stream decodePathTests() {".%2e", "..", EnumSet.of(Ambiguous.SEGMENT)}, {"%2e%2e", "..", EnumSet.of(Ambiguous.SEGMENT)}, + // empty segment treated as ambiguous + {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo//../bar", "/foo/bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo///../../../bar", "/bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo/./../bar", "/bar", EnumSet.noneOf(Ambiguous.class)}, + {"/foo//./bar", "/foo//bar", EnumSet.of(Ambiguous.SEGMENT)}, + // ambiguous parameter inclusions {"/path/.;/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)}, {"/path/.;param/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)}, @@ -377,6 +384,11 @@ public static Stream decodePathTests() {"%2F/info", "//info", EnumSet.of(Ambiguous.SEPARATOR)}, {"/path/%2f../info", "/path//../info", EnumSet.of(Ambiguous.SEPARATOR)}, + // ambiguous encoding + {"/path/%25/info", "/path/%/info", EnumSet.of(Ambiguous.ENCODING)}, + {"%25/info", "%/info", EnumSet.of(Ambiguous.ENCODING)}, + {"/path/%25../info", "/path/%../info", EnumSet.of(Ambiguous.ENCODING)}, + // combinations {"/path/%2f/..;/info", "/path///../info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM)}, {"/path/%2f/..;/%2e/info", "/path///.././info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM, Ambiguous.SEGMENT)}, @@ -401,6 +413,7 @@ public void testDecodedPath(String input, String decodedPath, EnumSet assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Ambiguous.SEGMENT))); assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Ambiguous.SEPARATOR))); assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Ambiguous.PARAM))); + assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Ambiguous.ENCODING))); } catch (Exception e) { 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 997dcf2acb6c..e25ba66f0131 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 @@ -1252,7 +1252,7 @@ public String getRemoteUser() public RequestDispatcher getRequestDispatcher(String path) { // path is encoded, potentially with query - + // TODO: why do we do this? path = URIUtil.compactPath(path); if (path == null || _context == null) 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 ef4eb96ab6a6..d325fc32bf1a 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 @@ -1238,6 +1238,7 @@ public void doScope(String target, Request baseRequest, HttpServletRequest reque // check the target. if (DispatcherType.REQUEST.equals(dispatch) || DispatcherType.ASYNC.equals(dispatch)) { + // TODO: remove this once isCompact() has been deprecated for several releases. if (isCompactPath()) target = URIUtil.compactPath(target); if (!checkContext(target, baseRequest, response)) @@ -1798,7 +1799,9 @@ public void setMaxFormKeys(int max) /** * @return True if URLs are compacted to replace multiple '/'s with a single '/' + * @deprecated use {@code CompactPathRule} with {@code RewriteHandler} instead. */ + @Deprecated public boolean isCompactPath() { return _compactPath; @@ -1807,6 +1810,7 @@ public boolean isCompactPath() /** * @param compactPath True if URLs are compacted to replace multiple '/'s with a single '/' */ + @Deprecated public void setCompactPath(boolean compactPath) { _compactPath = compactPath; 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 7fdcd8097aa2..1c8fc82824cc 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 @@ -1790,6 +1790,17 @@ public void testAmbiguousEncoding() throws Exception _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.UNSAFE); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); } + + @Test + public void testDoubleSlash() throws Exception + { + _handler._checker = (request, response) -> true; + String request = "GET //../foo HTTP/1.0\r\n" + + "Host: whatever\r\n" + + "\r\n"; + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.DEFAULT); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400")); + } @Test public void testPushBuilder() From c30875e7f260ba16389e6a368951086ea42e37d5 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 21 May 2021 16:57:33 +1000 Subject: [PATCH 02/13] Fix false empty segments being reported + changes from review. Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpURI.java | 27 ++++++++++++++----- .../org/eclipse/jetty/server/Request.java | 4 --- .../org/eclipse/jetty/server/RequestTest.java | 10 +++++-- 3 files changed, 29 insertions(+), 12 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 7cf2b46a731e..2c6c77f7dff2 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 @@ -450,12 +450,12 @@ private enum State private static final Index __ambiguousSegments = new Index.Builder() .caseSensitive(false) .with("%2e", Boolean.TRUE) - .with("%2e%2e", Boolean.TRUE) - .with(".%2e", Boolean.TRUE) - .with("%2e.", Boolean.TRUE) - .with("", Boolean.TRUE) - .with("..", Boolean.FALSE) - .with(".", Boolean.FALSE) + .with("%2e%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation + .with(".%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation + .with("%2e.", Boolean.TRUE) // Is real dot dot segment not removed by normalisation + .with("", Boolean.TRUE) // An empty segment may be interpreted differently by file systems + .with("..", Boolean.FALSE) // If followed by a parameter is not removed by dot dot normalisation + .with(".", Boolean.FALSE) // If followed by a parameter is not removed by dot normalisation .build(); private String _scheme; @@ -1282,6 +1282,21 @@ private void checkSegment(String uri, int segment, int end, boolean param) { if (!_ambiguous.contains(Ambiguous.SEGMENT)) { + if (segment == end) + { + // Falsely reported empty segment before the first slash. + if (segment == 0) + return; + + // If the segment starts after the URI string it means the string ended with a '/'. + if (segment > uri.length() - 1) + return; + + // It is only a true empty segment if it actually ends with a '/' character. + if (uri.charAt(end) != '/') + return; + } + Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); if (ambiguous == Boolean.TRUE) _ambiguous.add(Ambiguous.SEGMENT); 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 e25ba66f0131..48ac4cc13267 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 @@ -1251,10 +1251,6 @@ public String getRemoteUser() @Override public RequestDispatcher getRequestDispatcher(String path) { - // path is encoded, potentially with query - // TODO: why do we do this? - path = URIUtil.compactPath(path); - if (path == null || _context == null) return null; 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 1c8fc82824cc..d9e63d2fb3cb 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 @@ -1792,14 +1792,20 @@ public void testAmbiguousEncoding() throws Exception } @Test - public void testDoubleSlash() throws Exception + public void testAmbiguousDoubleSlash() throws Exception { _handler._checker = (request, response) -> true; - String request = "GET //../foo HTTP/1.0\r\n" + + String request = "GET /ambiguous/doubleSlash// HTTP/1.0\r\n" + "Host: whatever\r\n" + "\r\n"; _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.DEFAULT); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400")); + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.LEGACY); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.UNSAFE); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); } @Test From 3206dabcd88c8df5ea011b94090295de7b7d12d0 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 24 May 2021 09:59:18 +1000 Subject: [PATCH 03/13] Fix broken test + improve comments. Signed-off-by: Lachlan Roberts --- .../main/java/org/eclipse/jetty/http/HttpURI.java | 14 +++++++------- .../jetty/server/handler/NcsaRequestLogTest.java | 12 ++++-------- 2 files changed, 11 insertions(+), 15 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 2c6c77f7dff2..fb26bf04b921 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 @@ -449,13 +449,13 @@ private enum State */ private static final Index __ambiguousSegments = new Index.Builder() .caseSensitive(false) - .with("%2e", Boolean.TRUE) - .with("%2e%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation - .with(".%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation - .with("%2e.", Boolean.TRUE) // Is real dot dot segment not removed by normalisation - .with("", Boolean.TRUE) // An empty segment may be interpreted differently by file systems - .with("..", Boolean.FALSE) // If followed by a parameter is not removed by dot dot normalisation - .with(".", Boolean.FALSE) // If followed by a parameter is not removed by dot normalisation + .with("%2e", Boolean.TRUE) // Is real dot segment not removed by normalisation. + .with("%2e%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. + .with(".%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. + .with("%2e.", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. + .with("", Boolean.TRUE) // An empty segment may be interpreted differently by file systems. + .with("..", Boolean.FALSE) // If followed by a parameter is not removed by dot dot normalisation. + .with(".", Boolean.FALSE) // If followed by a parameter is not removed by dot normalisation. .build(); private String _scheme; diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java index 7a6f915a69a1..a998a6abd6aa 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java @@ -136,21 +136,17 @@ public void testRequestLine(String logType) throws Exception setup(logType); testHandlerServerStart(); - String log; - - /* _connector.getResponse("GET /foo?data=1 HTTP/1.0\nhost: host:80\n\n"); - log = _entries.poll(5, TimeUnit.SECONDS); + String log = _entries.poll(5, TimeUnit.SECONDS); assertThat(log, containsString("GET /foo?data=1 HTTP/1.0\" 200 ")); -*/ + _connector.getResponse("GET //bad/foo?data=1 HTTP/1.0\n\n"); log = _entries.poll(5, TimeUnit.SECONDS); - assertThat(log, containsString("GET //bad/foo?data=1 HTTP/1.0\" 200 ")); -/* + assertThat(log, containsString("GET //bad/foo?data=1 HTTP/1.0\" 400 ")); + _connector.getResponse("GET http://host:80/foo?data=1 HTTP/1.0\n\n"); log = _entries.poll(5, TimeUnit.SECONDS); assertThat(log, containsString("GET http://host:80/foo?data=1 HTTP/1.0\" 200 ")); - */ } @ParameterizedTest() From 3aec9635f9f0825278db052f2261fdec82a1ac29 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 25 May 2021 12:06:27 +1000 Subject: [PATCH 04/13] Add HttpUriTests for the empty segment as ambiguous Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/HttpURITest.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) 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 e52868b779fd..8757cca31385 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 @@ -420,4 +420,33 @@ public void testDecodedPath(String input, String decodedPath, EnumSet assertThat(decodedPath, nullValue()); } } + + public static Stream emptySegmentTests() + { + return Arrays.stream(new Object[][] + { + // Empty segment tests. + {"/", EnumSet.noneOf(Ambiguous.class)}, + {"/path", EnumSet.noneOf(Ambiguous.class)}, + {"/path/", EnumSet.noneOf(Ambiguous.class)}, + {"//", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo//", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo//bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"//foo/bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo?bar", EnumSet.noneOf(Ambiguous.class)}, + {"/foo/?bar", EnumSet.noneOf(Ambiguous.class)}, + }).map(Arguments::of); + } + + @ParameterizedTest + @MethodSource("emptySegmentTests") + public void testEmptySegment(String input, EnumSet expected) + { + HttpURI uri = HttpURI.from("GET", input); + assertThat(uri.isAmbiguous(), is(!expected.isEmpty())); + assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Ambiguous.SEGMENT))); + assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Ambiguous.SEPARATOR))); + assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Ambiguous.PARAM))); + assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Ambiguous.ENCODING))); + } } From 0cae7bff4a60f183f9135abdf7d6519ba4b230ed Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 25 May 2021 12:56:52 +1000 Subject: [PATCH 05/13] Issue #63002 - Only call checkSegment() for real empty segments. Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpURI.java | 30 +++++++------------ .../org/eclipse/jetty/http/HttpURITest.java | 5 ++++ 2 files changed, 15 insertions(+), 20 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 fb26bf04b921..13ee6c042fa9 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 @@ -1114,24 +1114,28 @@ else if (c == '/') switch (c) { case ';': - checkSegment(uri, segment, i, true); + if (segment != i) + checkSegment(uri, segment, i, true); mark = i + 1; state = State.PARAM; break; case '?': - checkSegment(uri, segment, i, false); + if (segment != i) + checkSegment(uri, segment, i, false); _path = uri.substring(pathMark, i); mark = i + 1; state = State.QUERY; break; case '#': - checkSegment(uri, segment, i, false); + if (segment != i) + checkSegment(uri, segment, i, false); _path = uri.substring(pathMark, i); mark = i + 1; state = State.FRAGMENT; break; case '/': - checkSegment(uri, segment, i, false); + if (segment - 1 >= 0 && uri.charAt(segment - 1) == '/') + checkSegment(uri, segment, i, false); segment = i + 1; break; case '.': @@ -1239,7 +1243,8 @@ else if (c == '/') _param = uri.substring(mark, end); break; case PATH: - checkSegment(uri, segment, end, false); + if (segment != end) + checkSegment(uri, segment, end, false); _path = uri.substring(pathMark, end); break; case QUERY: @@ -1282,21 +1287,6 @@ private void checkSegment(String uri, int segment, int end, boolean param) { if (!_ambiguous.contains(Ambiguous.SEGMENT)) { - if (segment == end) - { - // Falsely reported empty segment before the first slash. - if (segment == 0) - return; - - // If the segment starts after the URI string it means the string ended with a '/'. - if (segment > uri.length() - 1) - return; - - // It is only a true empty segment if it actually ends with a '/' character. - if (uri.charAt(end) != '/') - return; - } - Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); if (ambiguous == Boolean.TRUE) _ambiguous.add(Ambiguous.SEGMENT); 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 8757cca31385..9c9686184923 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 @@ -427,6 +427,7 @@ public static Stream emptySegmentTests() { // Empty segment tests. {"/", EnumSet.noneOf(Ambiguous.class)}, + {"/#", EnumSet.noneOf(Ambiguous.class)}, {"/path", EnumSet.noneOf(Ambiguous.class)}, {"/path/", EnumSet.noneOf(Ambiguous.class)}, {"//", EnumSet.of(Ambiguous.SEGMENT)}, @@ -434,7 +435,11 @@ public static Stream emptySegmentTests() {"/foo//bar", EnumSet.of(Ambiguous.SEGMENT)}, {"//foo/bar", EnumSet.of(Ambiguous.SEGMENT)}, {"/foo?bar", EnumSet.noneOf(Ambiguous.class)}, + {"/foo#bar", EnumSet.noneOf(Ambiguous.class)}, + {"/foo;bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo/?bar", EnumSet.noneOf(Ambiguous.class)}, + {"/foo/#bar", EnumSet.noneOf(Ambiguous.class)}, + {"/foo/;bar", EnumSet.noneOf(Ambiguous.class)}, }).map(Arguments::of); } From 26491825c656ad7bc1716dc15d736a4d3631d794 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 25 May 2021 17:03:56 +1000 Subject: [PATCH 06/13] Issue #63002 - Empty segments are only ambiguous if followed by a '#', '?' or end of string Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpURI.java | 34 ++++++++++++++----- .../org/eclipse/jetty/http/HttpURITest.java | 12 +++++-- 2 files changed, 35 insertions(+), 11 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 13ee6c042fa9..ee4b35e6b67d 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 @@ -920,16 +920,21 @@ private void parse(State state, final String uri) state = State.HOST_OR_PATH; break; case ';': + checkSegment(uri, segment, i, true); mark = i + 1; state = State.PARAM; break; case '?': // assume empty path (if seen at start) + checkSegment(uri, segment, i, false); _path = ""; mark = i + 1; state = State.QUERY; break; case '#': + // assume empty path (if seen at start) + checkSegment(uri, segment, i, false); + _path = ""; mark = i + 1; state = State.FRAGMENT; break; @@ -1114,27 +1119,25 @@ else if (c == '/') switch (c) { case ';': - if (segment != i) - checkSegment(uri, segment, i, true); + checkSegment(uri, segment, i, true); mark = i + 1; state = State.PARAM; break; case '?': - if (segment != i) - checkSegment(uri, segment, i, false); + checkSegment(uri, segment, i, false); _path = uri.substring(pathMark, i); mark = i + 1; state = State.QUERY; break; case '#': - if (segment != i) - checkSegment(uri, segment, i, false); + checkSegment(uri, segment, i, false); _path = uri.substring(pathMark, i); mark = i + 1; state = State.FRAGMENT; break; case '/': - if (segment - 1 >= 0 && uri.charAt(segment - 1) == '/') + // There is no leading segment when parsing only a path that starts with slash. + if (i != 0) checkSegment(uri, segment, i, false); segment = i + 1; break; @@ -1223,6 +1226,9 @@ else if (c == '/') switch (state) { case START: + _path = ""; + checkSegment(uri, segment, end, false); + break; case ASTERISK: break; case SCHEME_OR_PATH: @@ -1243,8 +1249,7 @@ else if (c == '/') _param = uri.substring(mark, end); break; case PATH: - if (segment != end) - checkSegment(uri, segment, end, false); + checkSegment(uri, segment, end, false); _path = uri.substring(pathMark, end); break; case QUERY: @@ -1287,11 +1292,22 @@ private void checkSegment(String uri, int segment, int end, boolean param) { if (!_ambiguous.contains(Ambiguous.SEGMENT)) { + // Empty segments are only ambiguous if followed by a '#', '?' or end of string. + if (end == segment && (end == uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0))) + return; + + // Look for segment in the ambiguous segment index. Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); if (ambiguous == Boolean.TRUE) + { + // The segment is always ambiguous. _ambiguous.add(Ambiguous.SEGMENT); + } else if (param && ambiguous == Boolean.FALSE) + { + // The segment is ambiguous only when followed by a parameter. _ambiguous.add(Ambiguous.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 9c9686184923..80bef77fb566 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 @@ -367,6 +367,14 @@ public static Stream decodePathTests() {"/foo///../../../bar", "/bar", EnumSet.of(Ambiguous.SEGMENT)}, {"/foo/./../bar", "/bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo//./bar", "/foo//bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"foo/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)}, + {"foo;/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)}, + {";/bar", "/bar", EnumSet.of(Ambiguous.SEGMENT)}, + {";?n=v", "", EnumSet.of(Ambiguous.SEGMENT)}, + {"?n=v", "", EnumSet.noneOf(Ambiguous.class)}, + {"#n=v", "", EnumSet.noneOf(Ambiguous.class)}, + {"", "", EnumSet.noneOf(Ambiguous.class)}, + {"http:/foo", "/foo", EnumSet.noneOf(Ambiguous.class)}, // ambiguous parameter inclusions {"/path/.;/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)}, @@ -439,8 +447,8 @@ public static Stream emptySegmentTests() {"/foo;bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo/?bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo/#bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo/;bar", EnumSet.noneOf(Ambiguous.class)}, - }).map(Arguments::of); + {"/foo/;bar", EnumSet.of(Ambiguous.SEGMENT)}, + }).map(Arguments::of); } @ParameterizedTest From 279221797c80eb3ed8eca865aa6914f38af842fb Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 27 May 2021 13:40:16 +1000 Subject: [PATCH 07/13] If only the last segment is empty it is not ambiguous. Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpURI.java | 59 +++++++++++++++---- .../org/eclipse/jetty/http/UriCompliance.java | 9 ++- .../org/eclipse/jetty/http/HttpURITest.java | 24 ++++---- .../org/eclipse/jetty/server/Request.java | 2 + 4 files changed, 70 insertions(+), 24 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 ee4b35e6b67d..35f0206565ae 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 @@ -51,6 +51,7 @@ public interface HttpURI enum Ambiguous { SEGMENT, + EMPTY, SEPARATOR, ENCODING, PARAM @@ -150,6 +151,11 @@ static Immutable from(String scheme, String host, int port, String pathQuery) */ boolean hasAmbiguousSegment(); + /** + * @return True if the URI empty segment that is ambiguous like '//' or '/;param/'. + */ + boolean hasAmbiguousEmptySegment(); + /** * @return True if the URI has a possibly ambiguous separator of %2f */ @@ -380,6 +386,12 @@ public boolean hasAmbiguousSegment() return _ambiguous.contains(Ambiguous.SEGMENT); } + @Override + public boolean hasAmbiguousEmptySegment() + { + return _ambiguous.contains(Ambiguous.EMPTY); + } + @Override public boolean hasAmbiguousSeparator() { @@ -453,7 +465,6 @@ private enum State .with("%2e%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. .with(".%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. .with("%2e.", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. - .with("", Boolean.TRUE) // An empty segment may be interpreted differently by file systems. .with("..", Boolean.FALSE) // If followed by a parameter is not removed by dot dot normalisation. .with(".", Boolean.FALSE) // If followed by a parameter is not removed by dot normalisation. .build(); @@ -469,6 +480,7 @@ private enum State private String _uri; private String _decodedPath; private final EnumSet _ambiguous = EnumSet.noneOf(Ambiguous.class); + private boolean _emptySegment; private Mutable() { @@ -728,6 +740,12 @@ public boolean hasAmbiguousSegment() return _ambiguous.contains(Mutable.Ambiguous.SEGMENT); } + @Override + public boolean hasAmbiguousEmptySegment() + { + return _ambiguous.contains(Ambiguous.EMPTY); + } + @Override public boolean hasAmbiguousSeparator() { @@ -905,6 +923,7 @@ private void parse(State state, final String uri) boolean dot = false; // set to true if the path containers . or .. segments int escapedTwo = 0; // state of parsing a %2 int end = uri.length(); + _emptySegment = false; for (int i = 0; i < end; i++) { char c = uri.charAt(i); @@ -1290,25 +1309,43 @@ else if (_path != null) */ private void checkSegment(String uri, int segment, int end, boolean param) { - if (!_ambiguous.contains(Ambiguous.SEGMENT)) + // We had a non last empty segment meaning it was ambiguous. + if (_emptySegment) + _ambiguous.add(Ambiguous.EMPTY); + + if (end == segment) { // Empty segments are only ambiguous if followed by a '#', '?' or end of string. - if (end == segment && (end == uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0))) + if (end >= uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0)) return; - // Look for segment in the ambiguous segment index. - Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); - if (ambiguous == Boolean.TRUE) + // If the first segment is empty then it is ambiguous. + if (segment == 0) { - // The segment is always ambiguous. - _ambiguous.add(Ambiguous.SEGMENT); + _ambiguous.add(Ambiguous.EMPTY); + return; } - else if (param && ambiguous == Boolean.FALSE) + + // Otherwise only a non last empty segment is ambiguous. + if (!_emptySegment) { - // The segment is ambiguous only when followed by a parameter. - _ambiguous.add(Ambiguous.PARAM); + _emptySegment = true; + return; } } + + // Look for segment in the ambiguous segment index. + Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); + if (ambiguous == Boolean.TRUE) + { + // The segment is always ambiguous. + _ambiguous.add(Ambiguous.SEGMENT); + } + else if (param && ambiguous == Boolean.FALSE) + { + // The segment is ambiguous only when followed by a parameter. + _ambiguous.add(Ambiguous.PARAM); + } } } } 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 cddb35dc3af3..29b51dc9bff1 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 @@ -50,6 +50,10 @@ public enum Violation implements ComplianceViolation * Allow ambiguous path segments e.g. /foo/%2e%2e/bar */ AMBIGUOUS_PATH_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path segment"), + /** + * Allow ambiguous empty segments e.g. // + */ + AMBIGUOUS_EMPTY_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI empty segment"), /** * Allow ambiguous path separator within a URI segment e.g. /foo/b%2fr */ @@ -104,9 +108,10 @@ public String getDescription() public static final UriCompliance DEFAULT = new UriCompliance("DEFAULT", of(Violation.AMBIGUOUS_PATH_SEPARATOR)); /** - * LEGACY compliance mode that models Jetty-9.4 behavior by allowing {@link Violation#AMBIGUOUS_PATH_SEGMENT}, {@link Violation#AMBIGUOUS_PATH_SEPARATOR} and {@link Violation#AMBIGUOUS_PATH_ENCODING}. + * LEGACY compliance mode that models Jetty-9.4 behavior by allowing {@link Violation#AMBIGUOUS_PATH_SEGMENT}, + * {@link Violation#AMBIGUOUS_EMPTY_SEGMENT}, {@link Violation#AMBIGUOUS_PATH_SEPARATOR} and {@link Violation#AMBIGUOUS_PATH_ENCODING}. */ - public static final UriCompliance LEGACY = new UriCompliance("LEGACY", of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_ENCODING)); + public static final UriCompliance LEGACY = new UriCompliance("LEGACY", of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_ENCODING, Violation.AMBIGUOUS_EMPTY_SEGMENT)); /** * Compliance mode that exactly follows RFC3986, including allowing all additional ambiguous URI Violations, 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 80bef77fb566..98de5a8e1db3 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 @@ -362,15 +362,15 @@ public static Stream decodePathTests() {"%2e%2e", "..", EnumSet.of(Ambiguous.SEGMENT)}, // empty segment treated as ambiguous - {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.SEGMENT)}, - {"/foo//../bar", "/foo/bar", EnumSet.of(Ambiguous.SEGMENT)}, - {"/foo///../../../bar", "/bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//../bar", "/foo/bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo///../../../bar", "/bar", EnumSet.of(Ambiguous.EMPTY)}, {"/foo/./../bar", "/bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo//./bar", "/foo//bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo//./bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, {"foo/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)}, {"foo;/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)}, - {";/bar", "/bar", EnumSet.of(Ambiguous.SEGMENT)}, - {";?n=v", "", EnumSet.of(Ambiguous.SEGMENT)}, + {";/bar", "/bar", EnumSet.of(Ambiguous.EMPTY)}, + {";?n=v", "", EnumSet.of(Ambiguous.EMPTY)}, {"?n=v", "", EnumSet.noneOf(Ambiguous.class)}, {"#n=v", "", EnumSet.noneOf(Ambiguous.class)}, {"", "", EnumSet.noneOf(Ambiguous.class)}, @@ -438,16 +438,17 @@ public static Stream emptySegmentTests() {"/#", EnumSet.noneOf(Ambiguous.class)}, {"/path", EnumSet.noneOf(Ambiguous.class)}, {"/path/", EnumSet.noneOf(Ambiguous.class)}, - {"//", EnumSet.of(Ambiguous.SEGMENT)}, - {"/foo//", EnumSet.of(Ambiguous.SEGMENT)}, - {"/foo//bar", EnumSet.of(Ambiguous.SEGMENT)}, - {"//foo/bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"//", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, + {"//foo/bar", EnumSet.of(Ambiguous.EMPTY)}, {"/foo?bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo#bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo;bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo/?bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo/#bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo/;bar", EnumSet.of(Ambiguous.SEGMENT)}, + {"/foo/;param", EnumSet.noneOf(Ambiguous.class)}, + {"/foo/;param/bar", EnumSet.of(Ambiguous.EMPTY)}, }).map(Arguments::of); } @@ -457,6 +458,7 @@ public void testEmptySegment(String input, EnumSet expected) { HttpURI uri = HttpURI.from("GET", input); assertThat(uri.isAmbiguous(), is(!expected.isEmpty())); + assertThat(uri.hasAmbiguousEmptySegment(), is(expected.contains(Ambiguous.EMPTY))); assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Ambiguous.SEGMENT))); assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Ambiguous.SEPARATOR))); assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Ambiguous.PARAM))); 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 48ac4cc13267..634638165cc0 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 @@ -1694,6 +1694,8 @@ public void setMetaData(MetaData.Request request) compliance = _channel == null || _channel.getHttpConfiguration() == null ? null : _channel.getHttpConfiguration().getUriCompliance(); 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))) From 3dd5460a076361213d9580ae3f05eeee6f967571 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 28 May 2021 11:48:37 +1000 Subject: [PATCH 08/13] improve testing for path HttpUri pathQuery() Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/HttpURITest.java | 85 +++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-) 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 98de5a8e1db3..4aeb6e0f1b88 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 @@ -28,6 +28,7 @@ import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -429,11 +430,41 @@ public void testDecodedPath(String input, String decodedPath, EnumSet } } - public static Stream emptySegmentTests() + public static Stream pathFromQueryTests() { return Arrays.stream(new Object[][] { - // Empty segment tests. + // Simple path example + {"/path/info", EnumSet.noneOf(Ambiguous.class)}, + + // legal non ambiguous relative paths + {"/path/../info", EnumSet.noneOf(Ambiguous.class)}, + {"/path/./info", EnumSet.noneOf(Ambiguous.class)}, + {"path/../info", EnumSet.noneOf(Ambiguous.class)}, + {"path/./info", EnumSet.noneOf(Ambiguous.class)}, + + // illegal paths + {"/../path/info", null}, + {"../path/info", null}, + {"/path/%XX/info", null}, + {"/path/%2/F/info", null}, + + // ambiguous dot encodings + {"/path/%2e/info", EnumSet.of(Ambiguous.SEGMENT)}, + {"path/%2e/info/", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e%2e/info", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e%2e;/info", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e%2e;param/info", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e%2e;param;other/info;other", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e/info", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e%2e/info", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e%2e;/info", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e.", EnumSet.of(Ambiguous.SEGMENT)}, + {".%2e", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e%2e", EnumSet.of(Ambiguous.SEGMENT)}, + + // empty segment treated as ambiguous {"/", EnumSet.noneOf(Ambiguous.class)}, {"/#", EnumSet.noneOf(Ambiguous.class)}, {"/path", EnumSet.noneOf(Ambiguous.class)}, @@ -449,14 +480,58 @@ public static Stream emptySegmentTests() {"/foo/#bar", EnumSet.noneOf(Ambiguous.class)}, {"/foo/;param", EnumSet.noneOf(Ambiguous.class)}, {"/foo/;param/bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//../bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo///../../../bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo/./../bar", EnumSet.noneOf(Ambiguous.class)}, + {"/foo//./bar", EnumSet.of(Ambiguous.EMPTY)}, + {"foo/bar", EnumSet.noneOf(Ambiguous.class)}, + {"foo;/bar", EnumSet.noneOf(Ambiguous.class)}, + {";/bar", EnumSet.of(Ambiguous.EMPTY)}, + {";?n=v", EnumSet.of(Ambiguous.EMPTY)}, + {"?n=v", EnumSet.noneOf(Ambiguous.class)}, + {"#n=v", EnumSet.noneOf(Ambiguous.class)}, + {"", EnumSet.noneOf(Ambiguous.class)}, + + // ambiguous parameter inclusions + {"/path/.;/info", EnumSet.of(Ambiguous.PARAM)}, + {"/path/.;param/info", EnumSet.of(Ambiguous.PARAM)}, + {"/path/..;/info", EnumSet.of(Ambiguous.PARAM)}, + {"/path/..;param/info", EnumSet.of(Ambiguous.PARAM)}, + {".;/info", EnumSet.of(Ambiguous.PARAM)}, + {".;param/info", EnumSet.of(Ambiguous.PARAM)}, + {"..;/info", EnumSet.of(Ambiguous.PARAM)}, + {"..;param/info", EnumSet.of(Ambiguous.PARAM)}, + + // ambiguous segment separators + {"/path/%2f/info", EnumSet.of(Ambiguous.SEPARATOR)}, + {"%2f/info", EnumSet.of(Ambiguous.SEPARATOR)}, + {"%2F/info", EnumSet.of(Ambiguous.SEPARATOR)}, + {"/path/%2f../info", EnumSet.of(Ambiguous.SEPARATOR)}, + + // ambiguous encoding + {"/path/%25/info", EnumSet.of(Ambiguous.ENCODING)}, + {"%25/info", EnumSet.of(Ambiguous.ENCODING)}, + {"/path/%25../info", EnumSet.of(Ambiguous.ENCODING)}, + + // combinations + {"/path/%2f/..;/info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM)}, + {"/path/%2f/..;/%2e/info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM, Ambiguous.SEGMENT)}, }).map(Arguments::of); } @ParameterizedTest - @MethodSource("emptySegmentTests") - public void testEmptySegment(String input, EnumSet expected) + @MethodSource("pathFromQueryTests") + public void testPathFromQuery(String input, EnumSet expected) { - HttpURI uri = HttpURI.from("GET", input); + // If expected is null then it is a bad URI and should throw. + if (expected == null) + { + assertThrows(Throwable.class, () -> HttpURI.build().pathQuery(input)); + return; + } + + HttpURI uri = HttpURI.build().pathQuery(input); assertThat(uri.isAmbiguous(), is(!expected.isEmpty())); assertThat(uri.hasAmbiguousEmptySegment(), is(expected.contains(Ambiguous.EMPTY))); assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Ambiguous.SEGMENT))); From 93eaffdcf489c7d627a71278d671e642f3dcf004 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 31 May 2021 11:57:29 +1000 Subject: [PATCH 09/13] Add javadoc for HttpURI.Ambiguous Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpURI.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) 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 35f0206565ae..b2a07380c1e6 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 @@ -50,10 +50,29 @@ public interface HttpURI { enum Ambiguous { + /** + * URI contains ambiguous path segments e.g. /foo/%2e%2e/bar + */ SEGMENT, + + /** + * URI contains ambiguous empty segments e.g. // + */ EMPTY, + + /** + * URI contains ambiguous path separator within a URI segment e.g. /foo/b%2fr + */ SEPARATOR, + + /** + * URI contains ambiguous path encoding within a URI segment e.g. /%2557EB-INF + */ ENCODING, + + /** + * URI contains ambiguous path parameters within a URI segment e.g. /foo/..;/bar + */ PARAM } From 918f3772eeebc9bd38e64f580d6d1b381a685ba1 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 31 May 2021 16:26:49 +1000 Subject: [PATCH 10/13] Add decodedPath to testPathQuery() expectations. Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/HttpURITest.java | 140 +++++++++--------- 1 file changed, 71 insertions(+), 69 deletions(-) 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 4aeb6e0f1b88..9bd4ab07d245 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 @@ -430,99 +430,100 @@ public void testDecodedPath(String input, String decodedPath, EnumSet } } - public static Stream pathFromQueryTests() + public static Stream testPathQueryTests() { return Arrays.stream(new Object[][] { // Simple path example - {"/path/info", EnumSet.noneOf(Ambiguous.class)}, + {"/path/info", "/path/info", EnumSet.noneOf(Ambiguous.class)}, // legal non ambiguous relative paths - {"/path/../info", EnumSet.noneOf(Ambiguous.class)}, - {"/path/./info", EnumSet.noneOf(Ambiguous.class)}, - {"path/../info", EnumSet.noneOf(Ambiguous.class)}, - {"path/./info", EnumSet.noneOf(Ambiguous.class)}, + {"/path/../info", "/info", EnumSet.noneOf(Ambiguous.class)}, + {"/path/./info", "/path/info", EnumSet.noneOf(Ambiguous.class)}, + {"path/../info", "info", EnumSet.noneOf(Ambiguous.class)}, + {"path/./info", "path/info", EnumSet.noneOf(Ambiguous.class)}, // illegal paths - {"/../path/info", null}, - {"../path/info", null}, - {"/path/%XX/info", null}, - {"/path/%2/F/info", null}, + {"/../path/info", null, null}, + {"../path/info", null, null}, + {"/path/%XX/info", null, null}, + {"/path/%2/F/info", null, null}, // ambiguous dot encodings - {"/path/%2e/info", EnumSet.of(Ambiguous.SEGMENT)}, - {"path/%2e/info/", EnumSet.of(Ambiguous.SEGMENT)}, - {"/path/%2e%2e/info", EnumSet.of(Ambiguous.SEGMENT)}, - {"/path/%2e%2e;/info", EnumSet.of(Ambiguous.SEGMENT)}, - {"/path/%2e%2e;param/info", EnumSet.of(Ambiguous.SEGMENT)}, - {"/path/%2e%2e;param;other/info;other", EnumSet.of(Ambiguous.SEGMENT)}, - {"%2e/info", EnumSet.of(Ambiguous.SEGMENT)}, - {"%2e%2e/info", EnumSet.of(Ambiguous.SEGMENT)}, - {"%2e%2e;/info", EnumSet.of(Ambiguous.SEGMENT)}, - {"%2e", EnumSet.of(Ambiguous.SEGMENT)}, - {"%2e.", EnumSet.of(Ambiguous.SEGMENT)}, - {".%2e", EnumSet.of(Ambiguous.SEGMENT)}, - {"%2e%2e", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e/info", "/path/./info", EnumSet.of(Ambiguous.SEGMENT)}, + {"path/%2e/info/", "path/./info/", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e%2e/info", "/path/../info", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e%2e;/info", "/path/../info", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e%2e;param/info", "/path/../info", EnumSet.of(Ambiguous.SEGMENT)}, + {"/path/%2e%2e;param;other/info;other", "/path/../info", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e/info", "./info", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e%2e/info", "../info", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e%2e;/info", "../info", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e", ".", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e.", "..", EnumSet.of(Ambiguous.SEGMENT)}, + {".%2e", "..", EnumSet.of(Ambiguous.SEGMENT)}, + {"%2e%2e", "..", EnumSet.of(Ambiguous.SEGMENT)}, // empty segment treated as ambiguous - {"/", EnumSet.noneOf(Ambiguous.class)}, - {"/#", EnumSet.noneOf(Ambiguous.class)}, - {"/path", EnumSet.noneOf(Ambiguous.class)}, - {"/path/", EnumSet.noneOf(Ambiguous.class)}, - {"//", EnumSet.of(Ambiguous.EMPTY)}, - {"/foo//", EnumSet.of(Ambiguous.EMPTY)}, - {"/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, - {"//foo/bar", EnumSet.of(Ambiguous.EMPTY)}, - {"/foo?bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo#bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo;bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo/?bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo/#bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo/;param", EnumSet.noneOf(Ambiguous.class)}, - {"/foo/;param/bar", EnumSet.of(Ambiguous.EMPTY)}, - {"/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, - {"/foo//../bar", EnumSet.of(Ambiguous.EMPTY)}, - {"/foo///../../../bar", EnumSet.of(Ambiguous.EMPTY)}, - {"/foo/./../bar", EnumSet.noneOf(Ambiguous.class)}, - {"/foo//./bar", EnumSet.of(Ambiguous.EMPTY)}, - {"foo/bar", EnumSet.noneOf(Ambiguous.class)}, - {"foo;/bar", EnumSet.noneOf(Ambiguous.class)}, - {";/bar", EnumSet.of(Ambiguous.EMPTY)}, - {";?n=v", EnumSet.of(Ambiguous.EMPTY)}, - {"?n=v", EnumSet.noneOf(Ambiguous.class)}, - {"#n=v", EnumSet.noneOf(Ambiguous.class)}, - {"", EnumSet.noneOf(Ambiguous.class)}, + {"/", "/", EnumSet.noneOf(Ambiguous.class)}, + {"/#", "/", EnumSet.noneOf(Ambiguous.class)}, + {"/path", "/path", EnumSet.noneOf(Ambiguous.class)}, + {"/path/", "/path/", EnumSet.noneOf(Ambiguous.class)}, + {"//", "//", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//", "/foo//", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, + {"//foo/bar", "//foo/bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo?bar", "/foo", EnumSet.noneOf(Ambiguous.class)}, + {"/foo#bar", "/foo", EnumSet.noneOf(Ambiguous.class)}, + {"/foo;bar", "/foo", EnumSet.noneOf(Ambiguous.class)}, + {"/foo/?bar", "/foo/", EnumSet.noneOf(Ambiguous.class)}, + {"/foo/#bar", "/foo/", EnumSet.noneOf(Ambiguous.class)}, + {"/foo/;param", "/foo/", EnumSet.noneOf(Ambiguous.class)}, + {"/foo/;param/bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//../bar", "/foo/bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo///../../../bar", "/bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo/./../bar", "/bar", EnumSet.noneOf(Ambiguous.class)}, + {"/foo//./bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, + {"foo/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)}, + {"foo;/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)}, + {";/bar", "/bar", EnumSet.of(Ambiguous.EMPTY)}, + {";?n=v", "", EnumSet.of(Ambiguous.EMPTY)}, + {"?n=v", "", EnumSet.noneOf(Ambiguous.class)}, + {"#n=v", "", EnumSet.noneOf(Ambiguous.class)}, + {"", "", EnumSet.noneOf(Ambiguous.class)}, // ambiguous parameter inclusions - {"/path/.;/info", EnumSet.of(Ambiguous.PARAM)}, - {"/path/.;param/info", EnumSet.of(Ambiguous.PARAM)}, - {"/path/..;/info", EnumSet.of(Ambiguous.PARAM)}, - {"/path/..;param/info", EnumSet.of(Ambiguous.PARAM)}, - {".;/info", EnumSet.of(Ambiguous.PARAM)}, - {".;param/info", EnumSet.of(Ambiguous.PARAM)}, - {"..;/info", EnumSet.of(Ambiguous.PARAM)}, - {"..;param/info", EnumSet.of(Ambiguous.PARAM)}, + {"/path/.;/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)}, + {"/path/.;param/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)}, + {"/path/..;/info", "/path/../info", EnumSet.of(Ambiguous.PARAM)}, + {"/path/..;param/info", "/path/../info", EnumSet.of(Ambiguous.PARAM)}, + {".;/info", "./info", EnumSet.of(Ambiguous.PARAM)}, + {".;param/info", "./info", EnumSet.of(Ambiguous.PARAM)}, + {"..;/info", "../info", EnumSet.of(Ambiguous.PARAM)}, + {"..;param/info", "../info", EnumSet.of(Ambiguous.PARAM)}, // ambiguous segment separators - {"/path/%2f/info", EnumSet.of(Ambiguous.SEPARATOR)}, - {"%2f/info", EnumSet.of(Ambiguous.SEPARATOR)}, - {"%2F/info", EnumSet.of(Ambiguous.SEPARATOR)}, - {"/path/%2f../info", EnumSet.of(Ambiguous.SEPARATOR)}, + {"/path/%2f/info", "/path///info", EnumSet.of(Ambiguous.SEPARATOR)}, + {"%2f/info", "//info", EnumSet.of(Ambiguous.SEPARATOR)}, + {"%2F/info", "//info", EnumSet.of(Ambiguous.SEPARATOR)}, + {"/path/%2f../info", "/path//../info", EnumSet.of(Ambiguous.SEPARATOR)}, // ambiguous encoding - {"/path/%25/info", EnumSet.of(Ambiguous.ENCODING)}, - {"%25/info", EnumSet.of(Ambiguous.ENCODING)}, - {"/path/%25../info", EnumSet.of(Ambiguous.ENCODING)}, + {"/path/%25/info", "/path/%/info", EnumSet.of(Ambiguous.ENCODING)}, + {"%25/info", "%/info", EnumSet.of(Ambiguous.ENCODING)}, + {"/path/%25../info", "/path/%../info", EnumSet.of(Ambiguous.ENCODING)}, // combinations - {"/path/%2f/..;/info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM)}, - {"/path/%2f/..;/%2e/info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM, Ambiguous.SEGMENT)}, + {"/path/%2f/..;/info", "/path///../info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM)}, + {"/path/%2f/..;/%2e/info", "/path///.././info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM, Ambiguous.SEGMENT)}, + {"/path/%2f/%25/..;/%2e//info", "/path///%/.././/info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM, Ambiguous.SEGMENT, Ambiguous.ENCODING, Ambiguous.EMPTY)}, }).map(Arguments::of); } @ParameterizedTest - @MethodSource("pathFromQueryTests") - public void testPathFromQuery(String input, EnumSet expected) + @MethodSource("testPathQueryTests") + public void testPathQuery(String input, String decodedPath, EnumSet expected) { // If expected is null then it is a bad URI and should throw. if (expected == null) @@ -532,6 +533,7 @@ public void testPathFromQuery(String input, EnumSet expected) } HttpURI uri = HttpURI.build().pathQuery(input); + assertThat(uri.getDecodedPath(), is(decodedPath)); assertThat(uri.isAmbiguous(), is(!expected.isEmpty())); assertThat(uri.hasAmbiguousEmptySegment(), is(expected.contains(Ambiguous.EMPTY))); assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Ambiguous.SEGMENT))); From 339e245a9c21427700816d4bb978596207edc5be Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 8 Jun 2021 17:49:01 +1000 Subject: [PATCH 11/13] Change comments, add some extra tests. Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpURI.java | 30 +++++++++---------- .../org/eclipse/jetty/http/HttpURITest.java | 2 ++ 2 files changed, 17 insertions(+), 15 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 b2a07380c1e6..54542599b5c7 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 @@ -51,27 +51,27 @@ public interface HttpURI enum Ambiguous { /** - * URI contains ambiguous path segments e.g. /foo/%2e%2e/bar + * URI contains ambiguous path segments e.g. {@code /foo/%2e%2e/bar} */ SEGMENT, /** - * URI contains ambiguous empty segments e.g. // + * URI contains ambiguous empty segments e.g. {@code //} */ EMPTY, /** - * URI contains ambiguous path separator within a URI segment e.g. /foo/b%2fr + * 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. /%2557EB-INF + * 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. /foo/..;/bar + * URI contains ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar} */ PARAM } @@ -480,12 +480,12 @@ private enum State */ private static final Index __ambiguousSegments = new Index.Builder() .caseSensitive(false) - .with("%2e", Boolean.TRUE) // Is real dot segment not removed by normalisation. - .with("%2e%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. - .with(".%2e", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. - .with("%2e.", Boolean.TRUE) // Is real dot dot segment not removed by normalisation. - .with("..", Boolean.FALSE) // If followed by a parameter is not removed by dot dot normalisation. - .with(".", Boolean.FALSE) // If followed by a parameter is not removed by dot normalisation. + .with("%2e", Boolean.TRUE) + .with("%2e%2e", Boolean.TRUE) + .with(".%2e", Boolean.TRUE) + .with("%2e.", Boolean.TRUE) + .with("..", Boolean.FALSE) + .with(".", Boolean.FALSE) .build(); private String _scheme; @@ -1328,24 +1328,24 @@ else if (_path != null) */ private void checkSegment(String uri, int segment, int end, boolean param) { - // We had a non last empty segment meaning it was ambiguous. + // If we have previously seen an empty segment, then it was not the last segment and was thus ambiguous. if (_emptySegment) _ambiguous.add(Ambiguous.EMPTY); if (end == segment) { - // Empty segments are only ambiguous if followed by a '#', '?' or end of string. + // Empty segments are not ambiguous if followed by a '#', '?' or end of string. if (end >= uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0)) return; - // If the first segment is empty then it is ambiguous. + // If this empty segment is the first segment then it is ambiguous. if (segment == 0) { _ambiguous.add(Ambiguous.EMPTY); return; } - // Otherwise only a non last empty segment is ambiguous. + // Otherwise remember we have seen an empty segment, which is check if we see a subsequent segment. if (!_emptySegment) { _emptySegment = true; 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 9bd4ab07d245..09fcc5ff19af 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 @@ -481,6 +481,8 @@ public static Stream testPathQueryTests() {"/foo/;param", "/foo/", EnumSet.noneOf(Ambiguous.class)}, {"/foo/;param/bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)}, + {"/foo//bar//", "/foo//bar//", EnumSet.of(Ambiguous.EMPTY)}, + {"//foo//bar//", "//foo//bar//", EnumSet.of(Ambiguous.EMPTY)}, {"/foo//../bar", "/foo/bar", EnumSet.of(Ambiguous.EMPTY)}, {"/foo///../../../bar", "/bar", EnumSet.of(Ambiguous.EMPTY)}, {"/foo/./../bar", "/bar", EnumSet.noneOf(Ambiguous.class)}, From 26d16461a590a362a05c710c9bc88414ac0150d7 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 10 Jun 2021 20:52:21 +1000 Subject: [PATCH 12/13] Update jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java --- jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 54542599b5c7..d8deb522fe8b 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 @@ -1328,7 +1328,10 @@ else if (_path != null) */ private void checkSegment(String uri, int segment, int end, boolean param) { - // If we have previously seen an empty segment, then it was not the last segment and was thus ambiguous. + // 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 if (_emptySegment) _ambiguous.add(Ambiguous.EMPTY); From ceccab934f5ab28d260fb402b9b52b4dac064f0b Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 10 Jun 2021 21:05:31 +1000 Subject: [PATCH 13/13] Update jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java better examples --- jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d8deb522fe8b..af2dcda012b2 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 @@ -56,7 +56,7 @@ enum Ambiguous SEGMENT, /** - * URI contains ambiguous empty segments e.g. {@code //} + * URI contains ambiguous empty segments e.g. {@code /foo//bar} or {@code /foo/;param/}, but not {@code /foo/} */ EMPTY,