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

URI compliance modes for #6001 #6006

Merged
merged 8 commits into from Mar 2, 2021
Merged

URI compliance modes for #6001 #6006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f9b5974
Fix #4275 separate compliance modes for ambiguous URI segments and se…
gregw Feb 24, 2021
e68293e
Addressing copy/paste mistakes
joakime Feb 24, 2021
0603b13
Merge pull request #6005 from eclipse/jetty-9.4.x-6001-default-accept…
joakime Feb 24, 2021
ff3a9e7
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-10.0.x
gregw Feb 24, 2021
4d94851
Misc cleanups
gregw Feb 25, 2021
9dffc84
Misc cleanups
gregw Feb 25, 2021
e3c07d8
Misc cleanups
gregw Feb 25, 2021
7241479
Misc cleanups
gregw Feb 25, 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
26 changes: 23 additions & 3 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Open in desktop
Expand Up @@ -51,7 +51,8 @@ public interface HttpURI
enum Ambiguous
{
SEGMENT,
SEPARATOR
SEPARATOR,
PARAM
}

static Mutable build()
Expand Down Expand Up @@ -139,7 +140,7 @@ static Immutable from(String scheme, String host, int port, String pathQuery)
boolean isAbsolute();

/**
* @return True if the URI has either an {@link #hasAmbiguousSegment()} or {@link #hasAmbiguousSeparator()}.
* @return True if the URI has either an {@link #hasAmbiguousParameter()}, {@link #hasAmbiguousSegment()} or {@link #hasAmbiguousSeparator()}.
*/
boolean isAmbiguous();

Expand All @@ -153,6 +154,11 @@ static Immutable from(String scheme, String host, int port, String pathQuery)
*/
boolean hasAmbiguousSeparator();

/**
* @return True if the URI has a possibly ambiguous path parameter like '..;'
*/
boolean hasAmbiguousParameter();

gregw marked this conversation as resolved.
Show resolved Hide resolved
default URI toURI()
{
try
Expand Down Expand Up @@ -374,6 +380,12 @@ public boolean hasAmbiguousSeparator()
return _ambiguous.contains(Mutable.Ambiguous.SEPARATOR);
}

@Override
public boolean hasAmbiguousParameter()
{
return _ambiguous.contains(Ambiguous.PARAM);
}

@Override
public String toString()
{
Expand Down Expand Up @@ -709,6 +721,12 @@ public boolean hasAmbiguousSeparator()
return _ambiguous.contains(Mutable.Ambiguous.SEPARATOR);
}

@Override
public boolean hasAmbiguousParameter()
{
return _ambiguous.contains(Ambiguous.PARAM);
}

public Mutable normalize()
{
HttpScheme scheme = _scheme == null ? null : HttpScheme.CACHE.get(_scheme);
Expand Down Expand Up @@ -1241,8 +1259,10 @@ private void checkSegment(String uri, int segment, int end, boolean param)
if (!_ambiguous.contains(Ambiguous.SEGMENT))
{
Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment);
if (ambiguous == Boolean.TRUE || (param && ambiguous == Boolean.FALSE))
if (ambiguous == Boolean.TRUE)
_ambiguous.add(Ambiguous.SEGMENT);
else if (param && ambiguous == Boolean.FALSE)
_ambiguous.add(Ambiguous.PARAM);
}
}
}
Expand Down
96 changes: 68 additions & 28 deletions jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
View file
Open in desktop
Expand Up @@ -15,7 +15,6 @@

import java.util.Arrays;
import java.util.EnumSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
Expand All @@ -30,27 +29,39 @@

/**
* URI compliance modes for Jetty request handling.
* A Compliance mode consists of a set of {@link Violation}s which are applied
* A Compliance mode consists of a set of {@link Violation}s which are allowed
* when the mode is enabled.
*/
public final class UriCompliance implements ComplianceViolation.Mode
{
protected static final Logger LOG = LoggerFactory.getLogger(UriCompliance.class);

// These are compliance violations, which may optionally be allowed by the compliance mode, which mean that
// the relevant section of the RFC is not strictly adhered to.
/**
* These are URI compliance violations, which may be allowed by the compliance mode. Currently all these
* violations are for additional criteria in excess of the strict requirements of rfc3986.
*/
public enum Violation implements ComplianceViolation
{
/**
* 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"),
AMBIGUOUS_PATH_SEPARATOR("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path separator");
/**
* Ambiguous path separator within a URI segment e.g. <code>/foo/b%2fr</code>
*/
AMBIGUOUS_PATH_SEPARATOR("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path separator"),
/**
* Ambiguous path parameters within a URI segment e.g. <code>/foo/..;/bar</code>
*/
AMBIGUOUS_PATH_PARAMETER("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path parameter");

private final String url;
private final String description;
private final String _url;
private final String _description;

Violation(String url, String description)
{
this.url = url;
this.description = description;
_url = url;
_description = description;
}

@Override
Expand All @@ -62,24 +73,50 @@ public String getName()
@Override
public String getURL()
{
return url;
return _url;
}

@Override
public String getDescription()
{
return description;
return _description;
}
}

public static final UriCompliance SAFE = new UriCompliance("SAFE", noneOf(Violation.class));
public static final UriCompliance STRICT = new UriCompliance("STRICT", allOf(Violation.class));
private static final List<UriCompliance> KNOWN_MODES = Arrays.asList(SAFE, STRICT);
/**
* The default compliance mode that extends RFC3986 compliance with additional violations to avoid ambiguous URIs
*/
public static final UriCompliance DEFAULT = new UriCompliance("DEFAULT", noneOf(Violation.class));

/**
* LEGACY compliance mode that disallows only ambiguous path parameters as per Jetty-9.4
*/
public static final UriCompliance LEGACY = new UriCompliance("LEGACY", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR));

/**
* Compliance mode that exactly follows RFC3986, including allowing all additional ambiguous URI Violations
*/
public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", allOf(Violation.class));

/**
* @deprecated equivalent to DEFAULT
*/
@SuppressWarnings("DeprecatedIsStillUsed")
gregw marked this conversation as resolved.
Show resolved Hide resolved
@Deprecated
public static final UriCompliance SAFE = new UriCompliance("SAFE", DEFAULT.getAllowed());

/**
* @deprecated equivalent to RFC3986
*/
@SuppressWarnings("DeprecatedIsStillUsed")
gregw marked this conversation as resolved.
Show resolved Hide resolved
@Deprecated
public static final UriCompliance STRICT = new UriCompliance("STRICT", RFC3986.getAllowed());

private static final AtomicInteger __custom = new AtomicInteger();

public static UriCompliance valueOf(String name)
{
for (UriCompliance compliance : KNOWN_MODES)
for (UriCompliance compliance : Arrays.asList(DEFAULT, LEGACY, RFC3986, STRICT, SAFE))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using EnumSet.allOf() or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are not enums! They are just static final fields.
They were enums in 9, but that meant users could not define their own modes, so we have the CUSTOM[0-4] hack there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh! Ship it.

{
if (compliance.getName().equals(name))
return compliance;
Expand All @@ -91,20 +128,23 @@ public static UriCompliance valueOf(String name)
/**
* Create compliance set from string.
* <p>
* Format:
* Format: &lt;BASE&gt;[,[-]&lt;violation&gt;]...
* </p>
* <p>BASE is one of:</p>
* <dl>
* <dt>0</dt><dd>No {@link Violation}s</dd>
* <dt>*</dt><dd>All {@link Violation}s</dd>
* <dt>RFC2616</dt><dd>The set of {@link Violation}s application to https://tools.ietf.org/html/rfc2616,
* but not https://tools.ietf.org/html/rfc7230</dd>
* <dt>RFC7230</dt><dd>The set of {@link Violation}s application to https://tools.ietf.org/html/rfc7230</dd>
* <dt>name</dt><dd>Any of the known modes defined in {@link UriCompliance#KNOWN_MODES}</dd>
* <dt>&lt;name&gt;</dt><dd>The name of a static instance of {@link UriCompliance} (e.g. {@link UriCompliance#RFC3986}).
* </dl>
* <p>
* The remainder of the list can contain then names of {@link Violation}s to include them in the mode, or prefixed
* with a '-' to exclude thm from the mode.
* with a '-' to exclude thm from the mode. Examples are:
* </p>
* <dl>
* <dt><code>0,AMBIGUOUS_PATH_PARAMETER</code></dt><dd>Only allow {@link Violation#AMBIGUOUS_PATH_PARAMETER}</dd>
* <dt><code>*,-AMBIGUOUS_PATH_PARAMETER</code></dt><dd>Only all except {@link Violation#AMBIGUOUS_PATH_PARAMETER}</dd>
* <dt><code>RFC3986,AMBIGUOUS_PATH_PARAMETER</code></dt><dd>Same as RFC3986 plus {@link Violation#AMBIGUOUS_PATH_PARAMETER}</dd>
* </dl>
*
* @param spec A string in the format of a comma separated list starting with one of the following strings:
* @return the compliance from the string spec
Expand Down Expand Up @@ -150,19 +190,19 @@ public static UriCompliance from(String spec)
}

private final String _name;
private final Set<Violation> _violations;
private final Set<Violation> _allowed;

private UriCompliance(String name, Set<Violation> violations)
{
Objects.requireNonNull(violations);
_name = name;
_violations = unmodifiableSet(violations.isEmpty() ? noneOf(Violation.class) : copyOf(violations));
_allowed = unmodifiableSet(violations.isEmpty() ? noneOf(Violation.class) : copyOf(violations));
}

@Override
public boolean allows(ComplianceViolation violation)
{
return violation instanceof Violation && _violations.contains(violation);
return violation instanceof Violation && _allowed.contains(violation);
}

@Override
Expand All @@ -179,7 +219,7 @@ public String getName()
@Override
public Set<Violation> getAllowed()
{
return _violations;
return _allowed;
}

@Override
Expand All @@ -197,7 +237,7 @@ public Set<Violation> getKnown()
*/
public UriCompliance with(String name, Violation... violations)
{
Set<Violation> union = _violations.isEmpty() ? EnumSet.noneOf(Violation.class) : copyOf(_violations);
Set<Violation> union = _allowed.isEmpty() ? EnumSet.noneOf(Violation.class) : copyOf(_allowed);
union.addAll(copyOf(violations));
return new UriCompliance(name, union);
}
Expand All @@ -211,15 +251,15 @@ public UriCompliance with(String name, Violation... violations)
*/
public UriCompliance without(String name, Violation... violations)
{
Set<Violation> remainder = _violations.isEmpty() ? EnumSet.noneOf(Violation.class) : copyOf(_violations);
Set<Violation> remainder = _allowed.isEmpty() ? EnumSet.noneOf(Violation.class) : copyOf(_allowed);
remainder.removeAll(copyOf(violations));
return new UriCompliance(name, remainder);
}

@Override
public String toString()
{
return String.format("%s%s", _name, _violations);
return String.format("%s%s", _name, _allowed);
}

private static Set<Violation> copyOf(Violation[] violations)
Expand Down