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

Jetty 12.0.x 9444 servlet paths fully decoded #9479

Merged
merged 11 commits into from
Mar 11, 2023
Merged

Jetty 12.0.x 9444 servlet paths fully decoded #9479

merged 11 commits into from
Mar 11, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Mar 8, 2023 •
edited
Loading

Alternative to #9465
This is an alternative to #9462 & #9465 as a fix for #9444, replacing changes made by #9455

gregw added 4 commits March 8, 2023 17:27
@gregw
Fully decode #9444
60fd78a 
getServletPath and getPathInfo will never return an encoded path segment. Instead, they will throw an IllegalArgumentException if they are called when there is a URI with violations.
@gregw
Fully decode #9444
a4c7fc3 
getServletPath and getPathInfo will never return an encoded path segment. Instead, they will throw an IllegalArgumentException if they are called when there is a URI with violations.
@gregw
WIP
dc918db 
Signed-off-by: gregw <[email protected]>
@gregw
Fully decode #9444
9fc64a7 
WIP
@gregw gregw mentioned this pull request Mar 9, 2023
Closed
@gregw
Copy link
Contributor Author

gregw commented Mar 9, 2023

@joakime do you think there are any tests from #9462 that should be kept even without safeDecode in this PR?

@gregw
Fully decode #9444
3407336 
Fixed ee9 tests
@gregw gregw marked this pull request as ready for review March 9, 2023 11:34
@gregw gregw requested review from joakime, sbordet and janbartel March 9, 2023 11:36
@gregw
Copy link
Contributor Author

gregw commented Mar 9, 2023 •
edited
Loading

@sbordet @janbartel @joakime this PR is now building and so is ready for some review, however I think there is still some more work that could be done:

  • Need to merge the BadMessage/HttpException PR so we can correctly throw an IAE rather than a RTE
  • We could potentially make throwing the IAE configurable, so an app could both allow the violation and allow the ambiguous decoding. The question is, where should that configuration be?
  • probably more renaming could be done to distinguish between encodedPathInContext and decodedPathInContext. Any ideas for better names for those?
  • there are some related unit tests that are disabled. They should be re-enabled and debugged (edit: turned out they were cross context dispatch)

@gregw
Copy link
Contributor Author

gregw commented Mar 9, 2023

How about we use canonicalPathInContext instead of encodedPathInContext and then just used pathInContext for the decodedPathInContext?

@gregw
Fully decode #9444
6751f1e 
Optionally throw on decode of ambiguous URIs
@joakime
Copy link
Contributor

joakime commented Mar 9, 2023

How about we use canonicalPathInContext instead of encodedPathInContext and then just used pathInContext for the decodedPathInContext?

I'd prefer that we keep the "encoded" and "decoded" portions in the method names.

gregw added 2 commits March 10, 2023 09:59
@gregw
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
e19b961 
…9444-servletPaths-fully-decoded
@gregw
Fully decode #9444
e759b9e 
Optionally throw on decode of ambiguous URIs
sbordet
sbordet requested changes Mar 10, 2023
View reviewed changes
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Context.java Outdated Show resolved Hide resolved
...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java Outdated Show resolved Hide resolved
...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java Outdated Show resolved Hide resolved
jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java Outdated Show resolved Hide resolved
...y-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncContextTest.java Outdated Show resolved Hide resolved
@joakime
Copy link
Contributor

joakime commented Mar 10, 2023 •
edited
Loading

We'll get rid of the ISO-8859-1 fallback during decode in a different PR?

@joakime
Copy link
Contributor

joakime commented Mar 10, 2023

@gregw I added some more URIUtilTest cases, and a block of tests that are commented out for a future PR.
I'm onboard with this PR once the concerns from @sbordet are addressed.

@gregw
Fully decode #9444
4c71870 
updates from review. Extra testing.
@gregw gregw requested a review from sbordet March 10, 2023 17:49
sbordet
sbordet approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Just nits, LGTM.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Context.java Outdated Show resolved Hide resolved
...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java Outdated Show resolved Hide resolved
...0/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java Show resolved Hide resolved
@gregw
Fully decode #9444
92c0504 
updates from review.
@gregw gregw merged commit bd0186c into jetty-12.0.x Mar 11, 2023
@gregw gregw deleted the jetty-12.0.x-9444-servletPaths-fully-decoded branch March 11, 2023 13:13
@joakime joakime mentioned this pull request Mar 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime approved these changes

@sbordet sbordet sbordet approved these changes

@janbartel janbartel Awaiting requested review from janbartel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gregw @joakime @sbordet