Skip to content

Commit

Permalink
Issue #6302 - Treat empty path segments as ambiguous. (#6304)
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@lachlan-roberts @gregw
lachlan-roberts and gregw committed Jun 10, 2021
1 parent 900c719 commit b4d7e51
Show file tree
Hide file tree
Showing 7 changed files with 260 additions and 23 deletions.
99 changes: 90 additions & 9 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Expand Up @@ -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
}

Expand Down Expand Up @@ -150,6 +170,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 +402,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 @@ -468,6 +499,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 +759,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 +942,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 +958,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 +1174,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 +1264,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 +1328,45 @@ else if (_path != null)
*/
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)
{
// 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)
{
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
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
136 changes: 136 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -361,6 +362,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)},

// ambiguous parameter inclusions
{"/path/.;/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
{"/path/.;param/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
Expand All @@ -377,6 +393,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 +422,125 @@ 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> 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)));
}
}

0 comments on commit b4d7e51

Please sign in to comment.