-
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
UriCompliance mode improvements #6132 #6137
Conversation
jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
Outdated
Show resolved
Hide resolved
jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
Outdated
Show resolved
Hide resolved
jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
Outdated
Show resolved
Hide resolved
Updates from review
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.
Minor changes.
jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
Outdated
Show resolved
Hide resolved
jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
Outdated
Show resolved
Hide resolved
Updates from review
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.
Is there a reason that the private UriCompliance(String name, Set<Violation> violations)
is still private?
Can it be public
to allow embedded users a better API?
Never mind, the .from(Set)
API is sufficient.
jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
Outdated
Show resolved
Hide resolved
jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
Outdated
Show resolved
Hide resolved
jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
Outdated
Show resolved
Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
Outdated
Show resolved
Hide resolved
@joakime with regards to the |
Updates from review
@joakime nudge |
@joakime something strange is happening.... you've approved the changes, but I'm still seeing a red cross and requested changes from you??? What does this issue look like to you? |
Oh... as soon as I commented it went green! strange!!! |
Fixes #6132
This PR reverts the default behaviour to allows URIs that contain
%2F
. I'm only 80% convinced of this and we need to consider really carefully before changing the default again.The PR also contains a non-string based way to create custom
UriCompliance
instances.It also introduces a new
Violation
that will allow ambiguous Uri to be passed to the application without extra normalisation.