core: percent encode illegal chars when creating URI query #3003
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akka-ci
added
validating
|
Test FAILed. Pull request validation reportFailed Test SuitesTest result for 'akka-http2-support / Pr-validation / ./ executeTests' Mima Failures |
akka-ci
added
validating
|
Test FAILed. Pull request validation reportMima Failures |
Contributor
Author
|
(I'm confused why Jenkins is reporting mima failures, AFAICS those should be filtered out now..) |
jrudolph
reviewed
Mar 12, 2020
Contributor
jrudolph
left a comment
jrudolph
left a comment
There was a problem hiding this comment.
Looks reasonable. Thanks for digging into this rabbit hole. A few small things below
...ore/src/main/mima-filters/10.1.x.backwards.excludes/query-string-encoding.backwards.excludes
Outdated
Show resolved
Hide resolved
| uri.withQuery(Query(Map("param1" -> "value1"))) shouldEqual Uri("http://host/path?param1=value1#fragment") | ||
| uri.withRawQueryString("param1=value1") shouldEqual Uri("http://host/path?param1=value1#fragment") | ||
|
|
||
| uri.withQuery(Query("param1" -> "val\"ue1")) shouldEqual Uri("http://host/path?param1=val%22ue1#fragment") |
Contributor
There was a problem hiding this comment.
Yep, this API should definitely encode when converting to raw 👍
akka-http-core/src/test/scala/akka/http/scaladsl/model/UriSpec.scala
Outdated
Show resolved
Hide resolved
akka-http-tests/src/test/java/akka/http/javadsl/server/directives/ParameterDirectivesTest.java
Show resolved
Hide resolved
akka-ci
added
validating
akka-ci
added
needs-attention
|
Test FAILed. Pull request validation reportMima Failures |
akka-ci
added
validating
|
Test FAILed. Pull request validation reportMima Failures |
Co-Authored-By: Ignasi Marimon-Clos <ignasi@lightbend.com>
Co-Authored-By: Enno <458526+ennru@users.noreply.github.com>
akka-ci
added
validating
…http into encode-query-string-on-render
akka-ci
added
needs-attention
|
Test PASSed. |
jrudolph
approved these changes
Mar 18, 2020
Contributor
jrudolph
left a comment
jrudolph
left a comment
There was a problem hiding this comment.
LGTM, great work, thanks.
Do you think you have now closed all loopholes to create Uris with an invalid rawQueryString? AFAICS, it now looks fine. copy goes through the safe variant. From Java you could create an unexpected subclass of sealed scaladsl.model.Uri but let's not worry about that one.
akka-http-tests/src/test/java/akka/http/javadsl/server/directives/ParameterDirectivesTest.java
Show resolved
Hide resolved
Contributor
Author
Yes, I think so - of course there's a couple of situations where we parse the string again to make sure there are no invalid percent-encodings in there, but no longer in the 'hot path'. |
Contributor
Yeah, good point. If someone cares about those paths they can still be optimized later on. |
jrudolph
changed the title
jrudolph
pushed a commit
to jrudolph/akka-http
that referenced
this pull request
Mar 19, 2020
This makes sure 'strict' parsing of the Uri query string only allows characters that are actually allowed here by the RFC. It does not change the fact that Uri's rawQueryString is the percent-encoded representation of the query string, since this information is needed to parse it into a key-value structure. When parsing the Uri in 'relaxed' mode or constructing programmatically, we do accept characters that are not allowed, but percent-encode any would-be invalid characters. API's that can be used to programmatically add or modify the query string are adapted to not accept invalid percent-encoded values (e.g. %x2) and convert illegal characters. withRawQueryString is made available both in a 'strict' and a 'relaxed' variation. Fixes akka#3002 (cherry picked from commit e284816)
jrudolph
pushed a commit
to jrudolph/akka-http
that referenced
this pull request
Mar 19, 2020
release 10.1 backport of akka#3003 This makes sure 'strict' parsing of the Uri query string only allows characters that are actually allowed here by the RFC. It does not change the fact that Uri's rawQueryString is the percent-encoded representation of the query string, since this information is needed to parse it into a key-value structure. When parsing the Uri in 'relaxed' mode or constructing programmatically, we do accept characters that are not allowed, but percent-encode any would-be invalid characters. API's that can be used to programmatically add or modify the query string are adapted to not accept invalid percent-encoded values (e.g. %x2) and convert illegal characters. withRawQueryString is made available both in a 'strict' and a 'relaxed' variation. Fixes akka#3002 (cherry picked from commit e284816)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes sure 'strict' parsing of the Uri query string only allows characters that are actually allowed here by the RFC.
It does not change the fact that Uri's
rawQueryStringis the percent-encoded representation of the query string, since this information is needed to parse it into a key-value structure.When parsing the Uri in 'relaxed' mode or constructing programmatically, we do accept characters that are not allowed, but percent-encode any would-be invalid characters.
API's that can be used to programmatically add or modify the query string are adapted to not accept invalid percent-encoded values (e.g.
%x2) and convert illegal characters.withRawQueryStringis made available both in a 'strict' and a 'relaxed' variation.References #3002