-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
HttpURI.Mutable path changes should clear out any existing path violations before parsing the new path. #12306
HttpURI.Mutable path changes should clear out any existing path violations before parsing the new path. #12306
Conversation
…tions before parsing the new path. Fixes: #11298
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good... except to make it a little less fragile it would be good to move the test to a method in UriViolation
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry .. one more change
*/ | ||
public static boolean isPathViolation(UriCompliance.Violation violation) | ||
{ | ||
return (violation == Violation.AMBIGUOUS_PATH_PARAMETER) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an enumset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes the removal of individual items on a collection WAY more complicated and slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind that the usage is _violations.removeIf(UriCompliance::isPathViolation);
As an EnumSet
, this would mean a EnumSet.contains()
call for every entry in the _violations
collection.
Why go to all that complication for a simple comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnumSet's are highly optimized for enums with less than 32 values. A contains operation will be a single bitwise operation and null check. OK in this case, it is probably hardly different from 4 == and 3 || operations, but I think the code is also more readable. Also we do other EnumSet comparisons on Violations (e.g. org.eclipse.jetty.http.UriCompliance#AMBIGUOUS_VIOLATIONS) so best to keep the style the same.
private static final EnumSet<Violation> PATH_VIOLATIONS = Collections.unmodifiableSet(EnumSet.of(
Violation.AMBIGUOUS_PATH_SEGMENT,
Violation.AMBIGUOUS_PATH_SEPARATOR,
Violation.AMBIGUOUS_PATH_ENCODING,
Violation.AMBIGUOUS_EMPTY_SEGMENT));
// ...
public static boolean isPathViolation(UriCompliance.Violation violation)
{
return PATH_VIOLATIONS.contains(violation);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to EnumSet pushed.
The use of HttpURI.Mutable methods that change the path (eg
.path(String)
and.pathQuery(String)
) should clear out any existing path violations before parsing the newly provided path.This helps CompactPathRule to also clear out the path violations that the CompactPathRule is capable of addressing.
Updating CompactPathRuleTest testcase to show these new behaviors.
Fixes: #11298