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

Issue #6302 - Treat empty path segments are ambiguous.

* Fix false empty segments being reported.
* Add HttpUriTests for the empty segment as ambiguous

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Co-authored-by: Greg Wilkins <gregw@webtide.com>
This commit is contained in:
Lachlan 2021-06-10 23:12:59 +10:00 committed by GitHub
parent 900c71902c
commit b4d7e5117d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 260 additions and 23 deletions

View File

@ -50,9 +50,29 @@ public interface HttpURI
{
enum Ambiguous
{
/**
* URI contains ambiguous path segments e.g. {@code /foo/%2e%2e/bar}
*/
SEGMENT,
/**
* URI contains ambiguous empty segments e.g. {@code /foo//bar} or {@code /foo/;param/}, but not {@code /foo/}
*/
EMPTY,
/**
* 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. {@code /%2557EB-INF}
*/
ENCODING,
/**
* URI contains ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar}
*/
PARAM
}
@ -150,6 +170,11 @@ public interface HttpURI
*/
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
*/
@ -377,13 +402,19 @@ public interface HttpURI
@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
@ -468,6 +499,7 @@ public interface HttpURI
private String _uri;
private String _decodedPath;
private final EnumSet<Ambiguous> _ambiguous = EnumSet.noneOf(Ambiguous.class);
private boolean _emptySegment;
private Mutable()
{
@ -727,6 +759,12 @@ public interface HttpURI
return _ambiguous.contains(Mutable.Ambiguous.SEGMENT);
}
@Override
public boolean hasAmbiguousEmptySegment()
{
return _ambiguous.contains(Ambiguous.EMPTY);
}
@Override
public boolean hasAmbiguousSeparator()
{
@ -904,6 +942,7 @@ public interface HttpURI
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);
@ -919,16 +958,21 @@ public interface HttpURI
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;
@ -1130,7 +1174,9 @@ public interface HttpURI
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 '.':
@ -1218,6 +1264,9 @@ public interface HttpURI
switch (state)
{
case START:
_path = "";
checkSegment(uri, segment, end, false);
break;
case ASTERISK:
break;
case SCHEME_OR_PATH:
@ -1279,13 +1328,45 @@ public interface HttpURI
*/
private void checkSegment(String uri, int segment, int end, boolean param)
{
if (!_ambiguous.contains(Ambiguous.SEGMENT))
// 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);
if (end == segment)
{
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);
// Empty segments are not ambiguous if followed by a '#', '?' or end of string.
if (end >= uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0))
return;
// If this empty segment is the first segment then it is ambiguous.
if (segment == 0)
{
_ambiguous.add(Ambiguous.EMPTY);
return;
}
// Otherwise remember we have seen an empty segment, which is check if we see a subsequent segment.
if (!_emptySegment)
{
_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);
}
}
}

View File

@ -50,6 +50,10 @@ public final class UriCompliance implements ComplianceViolation.Mode
* 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>
*/
@ -104,9 +108,10 @@ public final class UriCompliance implements ComplianceViolation.Mode
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,

View File

@ -28,6 +28,7 @@ import static org.hamcrest.Matchers.is;
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;
@ -361,6 +362,21 @@ public class HttpURITest
{".%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)},
// ambiguous parameter inclusions
{"/path/.;/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
{"/path/.;param/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
@ -377,6 +393,11 @@ public class HttpURITest
{"%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,10 +422,125 @@ public class HttpURITest
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> testPathQueryTests()
{
return Arrays.stream(new Object[][]
{
// Simple path example
{"/path/info", "/path/info", EnumSet.noneOf(Ambiguous.class)},
// legal non ambiguous relative paths
{"/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, null},
{"../path/info", null, null},
{"/path/%XX/info", null, null},
{"/path/%2/F/info", null, null},
// ambiguous dot encodings
{"/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", "/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//", "//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", "/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", "/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", "/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)},
{"/path/%2f/%25/..;/%2e//info", "/path///%/.././/info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM, Ambiguous.SEGMENT, Ambiguous.ENCODING, Ambiguous.EMPTY)},
}).map(Arguments::of);
}
@ParameterizedTest
@MethodSource("testPathQueryTests")
public void testPathQuery(String input, String decodedPath, EnumSet<Ambiguous> expected)
{
// 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.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)));
assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Ambiguous.SEPARATOR)));
assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Ambiguous.PARAM)));
assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Ambiguous.ENCODING)));
}
}

View File

@ -1252,10 +1252,6 @@ public class Request implements HttpServletRequest
@Override
public RequestDispatcher getRequestDispatcher(String path)
{
// path is encoded, potentially with query
path = URIUtil.compactPath(path);
if (path == null || _context == null)
return null;
@ -1699,6 +1695,8 @@ public class Request implements HttpServletRequest
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)))

View File

@ -1249,6 +1249,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
// 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))
@ -1807,7 +1808,9 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
/**
* @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;
@ -1816,6 +1819,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
/**
* @param compactPath True if URLs are compacted to replace multiple '/'s with a single '/'
*/
@Deprecated
public void setCompactPath(boolean compactPath)
{
_compactPath = compactPath;

View File

@ -1732,6 +1732,23 @@ public class RequestTest
_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"));
}
@Test
public void testPushBuilder()

View File

@ -136,21 +136,17 @@ public class NcsaRequestLogTest
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()