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 7 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
89 changes: 74 additions & 15 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Open in desktop
Expand Up @@ -51,6 +51,7 @@ public interface HttpURI
enum Ambiguous
{
SEGMENT,
EMPTY,
SEPARATOR,
ENCODING,
PARAM
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -377,13 +383,19 @@ public boolean isAmbiguous()
@Override
public boolean hasAmbiguousSegment()
{
return _ambiguous.contains(Mutable.Ambiguous.SEGMENT);
return _ambiguous.contains(Ambiguous.SEGMENT);
}

@Override
public boolean hasAmbiguousEmptySegment()
{
return _ambiguous.contains(Ambiguous.EMPTY);
}

@Override
public boolean hasAmbiguousSeparator()
{
return _ambiguous.contains(Mutable.Ambiguous.SEPARATOR);
return _ambiguous.contains(Ambiguous.SEPARATOR);
}

@Override
Expand Down Expand Up @@ -449,12 +461,12 @@ 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.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 All @@ -468,6 +480,7 @@ private enum State
private String _uri;
private String _decodedPath;
private final EnumSet<Ambiguous> _ambiguous = EnumSet.noneOf(Ambiguous.class);
private boolean _emptySegment;

private Mutable()
{
Expand Down Expand Up @@ -727,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()
{
Expand Down Expand Up @@ -904,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<x>
int end = uri.length();
_emptySegment = false;
for (int i = 0; i < end; i++)
{
char c = uri.charAt(i);
Expand All @@ -919,16 +939,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;
Expand Down Expand Up @@ -1130,7 +1155,9 @@ else if (c == '/')
state = State.FRAGMENT;
break;
case '/':
checkSegment(uri, segment, i, false);
// 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;
case '.':
Expand Down Expand Up @@ -1218,6 +1245,9 @@ else if (c == '/')
switch (state)
{
case START:
_path = "";
checkSegment(uri, segment, end, false);
break;
case ASTERISK:
break;
case SCHEME_OR_PATH:
Expand Down Expand Up @@ -1279,13 +1309,42 @@ 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.
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
if (_emptySegment)
_ambiguous.add(Ambiguous.EMPTY);

if (end == segment)
{
// Empty segments are only ambiguous if followed by a '#', '?' or end of string.
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
if (end >= uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0))
return;

// If the first segment is empty then it is ambiguous.
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
if (segment == 0)
{
_ambiguous.add(Ambiguous.EMPTY);
return;
}

// Otherwise only a non last empty segment is ambiguous.
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
if (!_emptySegment)
{
_emptySegment = true;
sbordet marked this conversation as resolved.
Show resolved Hide resolved
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)
{
Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment);
if (ambiguous == Boolean.TRUE)
_ambiguous.add(Ambiguous.SEGMENT);
else if (param && ambiguous == Boolean.FALSE)
_ambiguous.add(Ambiguous.PARAM);
// The segment is ambiguous only when followed by a parameter.
_ambiguous.add(Ambiguous.PARAM);
}
}
}
Expand Down
9 changes: 7 additions & 2 deletions jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
View file
Open in desktop
Expand Up @@ -50,6 +50,10 @@ public enum Violation implements ComplianceViolation
* Allow ambiguous path segments e.g. <code>/foo/%2e%2e/bar</code>
*/
AMBIGUOUS_PATH_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path segment"),
/**
* Allow ambiguous empty segments e.g. <code>//</code>
*/
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. <code>/foo/b%2fr</code>
*/
Expand Down Expand Up @@ -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,
Expand Down
57 changes: 57 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Open in desktop
Expand Up @@ -361,6 +361,21 @@ 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.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)},
{"http:/foo", "/foo", EnumSet.noneOf(Ambiguous.class)},

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 +392,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 +421,47 @@ 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.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)},
}).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.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)));
assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Ambiguous.ENCODING)));
}
}
6 changes: 2 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 Expand Up @@ -1698,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)))
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