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.

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

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

Merged
merged 13 commits into from Jun 10, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 3 additions & 2 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
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 @@ -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)
gregw marked this conversation as resolved.
Show resolved Hide resolved
.build();
Expand Down
13 changes: 13 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
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,6 +413,7 @@ 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)
{
Expand Down
Expand Up @@ -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);
gregw marked this conversation as resolved.
Show resolved Hide resolved

gregw marked this conversation as resolved.
Show resolved Hide resolved
if (path == null || _context == null)
Expand Down
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
Expand Up @@ -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" +
gregw marked this conversation as resolved.
Show resolved Hide resolved
"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()
Expand Down