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
59 changes: 48 additions & 11 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
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 @@ -380,6 +386,12 @@ public boolean hasAmbiguousSegment()
return _ambiguous.contains(Ambiguous.SEGMENT);
}

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

@Override
public boolean hasAmbiguousSeparator()
{
Expand Down Expand Up @@ -453,7 +465,6 @@ private enum State
.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();
Expand All @@ -469,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 @@ -728,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 @@ -905,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 Down Expand Up @@ -1290,25 +1309,43 @@ 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 == segment && (end == uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0)))
if (end >= uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0))
return;

// Look for segment in the ambiguous segment index.
Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment);
if (ambiguous == Boolean.TRUE)
// If the first segment is empty then it is ambiguous.
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
if (segment == 0)
{
// The segment is always ambiguous.
_ambiguous.add(Ambiguous.SEGMENT);
_ambiguous.add(Ambiguous.EMPTY);
return;
}
else if (param && ambiguous == Boolean.FALSE)

// Otherwise only a non last empty segment is ambiguous.
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
if (!_emptySegment)
{
// The segment is ambiguous only when followed by a parameter.
_ambiguous.add(Ambiguous.PARAM);
_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)
{
// The segment is ambiguous only when followed by a parameter.
_ambiguous.add(Ambiguous.PARAM);
}
}
}
}
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
24 changes: 13 additions & 11 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
Expand Up @@ -362,15 +362,15 @@ public static Stream<Arguments> decodePathTests()
{"%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", "/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.SEGMENT)},
{"/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.SEGMENT)},
{";?n=v", "", EnumSet.of(Ambiguous.SEGMENT)},
{";/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)},
Expand Down Expand Up @@ -438,16 +438,17 @@ public static Stream<Arguments> emptySegmentTests()
{"/#", 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)},
{"//", 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/;bar", EnumSet.of(Ambiguous.SEGMENT)},
{"/foo/;param", EnumSet.noneOf(Ambiguous.class)},
{"/foo/;param/bar", EnumSet.of(Ambiguous.EMPTY)},
}).map(Arguments::of);
}

Expand All @@ -457,6 +458,7 @@ public void testEmptySegment(String input, EnumSet<Ambiguous> expected)
{
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)));
Expand Down
Expand Up @@ -1694,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