Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #6302 - Treat empty path segments as ambiguous. #6304

Merged
merged 13 commits into from Jun 10, 2021
Merged

Issue #6302 - Treat empty path segments as ambiguous. #6304

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
36332ab
Issue #6302 - Treat empty path segments are ambiguous.
lachlan-roberts May 20, 2021
c30875e
Fix false empty segments being reported + changes from review.
lachlan-roberts May 21, 2021
3206dab
Fix broken test + improve comments.
lachlan-roberts May 23, 2021
3aec963
Add HttpUriTests for the empty segment as ambiguous
lachlan-roberts May 25, 2021
0cae7bf
Issue #63002 - Only call checkSegment() for real empty segments.
lachlan-roberts May 25, 2021
2649182
Issue #63002 - Empty segments are only ambiguous if followed by a '#'…
lachlan-roberts May 25, 2021
2792217
If only the last segment is empty it is not ambiguous.
lachlan-roberts May 27, 2021
3dd5460
improve testing for path HttpUri pathQuery()
lachlan-roberts May 28, 2021
93eaffd
Add javadoc for HttpURI.Ambiguous
lachlan-roberts May 31, 2021
918f377
Add decodedPath to testPathQuery() expectations.
lachlan-roberts May 31, 2021
339e245
Change comments, add some extra tests.
lachlan-roberts Jun 8, 2021
26d1646
Update jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
gregw Jun 10, 2021
ceccab9
Update jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
gregw Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 19 additions & 13 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -449,12 +449,13 @@ private enum State
*/
private static final Index<Boolean> __ambiguousSegments = new Index.Builder<Boolean>()
.caseSensitive(false)
.with("%2e", Boolean.TRUE)
.with("%2e%2e", Boolean.TRUE)
.with(".%2e", Boolean.TRUE)
.with("%2e.", Boolean.TRUE)
.with("..", Boolean.FALSE)
.with(".", Boolean.FALSE)
.with("%2e", Boolean.TRUE) // Is real dot segment not removed by normalisation.
gregw marked this conversation as resolved.
Show resolved Hide resolved
.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;
Expand Down Expand Up @@ -1113,24 +1114,28 @@ else if (c == '/')
switch (c)
{
case ';':
checkSegment(uri, segment, i, true);
if (segment != i)
checkSegment(uri, segment, i, true);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
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 '.':
Expand Down Expand Up @@ -1238,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:
Expand Down
47 changes: 47 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Open in desktop
Expand Up @@ -361,6 +361,13 @@ public static Stream<Arguments> 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)},

lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
// ambiguous parameter inclusions
{"/path/.;/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
{"/path/.;param/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
Expand All @@ -377,6 +384,11 @@ public static Stream<Arguments> 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)},
Expand All @@ -401,10 +413,45 @@ public void testDecodedPath(String input, String decodedPath, EnumSet<Ambiguous>
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)
{
assertThat(decodedPath, nullValue());
}
}

public static Stream<Arguments> emptySegmentTests()
{
return Arrays.stream(new Object[][]
{
// 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)},
{"/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)},
{"/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);
}

@ParameterizedTest
@MethodSource("emptySegmentTests")
public void testEmptySegment(String input, EnumSet<Ambiguous> expected)
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
{
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)));
}
}
4 changes: 0 additions & 4 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Open in desktop
Expand Up @@ -1251,10 +1251,6 @@ public String getRemoteUser()
@Override
public RequestDispatcher getRequestDispatcher(String path)
{
// path is encoded, potentially with query

path = URIUtil.compactPath(path);

gregw marked this conversation as resolved.
Show resolved Hide resolved
if (path == null || _context == null)
return null;

Expand Down
4 changes: 4 additions & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java
View file
Open in desktop
Expand Up @@ -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))
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
17 changes: 17 additions & 0 deletions jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Open in desktop
Expand Up @@ -1790,6 +1790,23 @@ 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 testAmbiguousDoubleSlash() throws Exception
{
_handler._checker = (request, response) -> true;
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"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add other combinations?

//ambiguous/initial
/ambiguous//middle
/double//ambiguous//
//triple//ambiguous//

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would be the right place for these extra tests. This is just testing how the compliance modes handle and empty segment.

What combinations cause an empty segment is tested by HttpUriTest. We already cover these cases, but I have also added these ones exactly to be sure.

}

@Test
public void testPushBuilder()
Expand Down
12 changes: 4 additions & 8 deletions jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java
View file
Open in desktop
Expand Up @@ -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()
Expand Down