Add Better Path Handling #12533
Merged
Add Better Path Handling #12533
Conversation
This file contains 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
Directly forward the uri when it's a path instead of trying to parse it. Also clean up isOriginForm/isAsteriskForm as their logic seems to be incorrect (for example, URI#getSchemeSpecificPart() never returns null so they both return false trivially). They should probably be edited themselves in a later commit (and possibly used again here when correct). Since this was always returning true, it's unclear if authority logic is still correct after this change.
|
I worked with @jkjk822 on this. The issue is that HTTP/1 request target is not a URI syntax. It is a bare path, or some other forms. A bare path is not the same as a URI. In HTTP/1, This may still be broken for authority-form, but it is no worse than it was previously and at least the code is easier to predict how it will behave. |
Fix test failures. This requires refactoring the authority pathway to reflect what it was in practice before (though not what the logic implied). Specifically, the logic before never set the HTTP2 Authority if the path was in origin or asterisk form (although due to those methods always returning false this never affected output).
|
Whoops, was running against master locally... Fixed all failures this time. Note the authority logic as mentioned in the first commit did end up having to be changed to pass tests. If the original logic is correct, the tests should be changed instead. |
normanmaurer
approved these changes
Jun 30, 2022
|
@jkjk822 can you please sign our icla: https://netty.io/s/icla ? /cc @ejona86 |
ejona86
approved these changes
Jun 30, 2022
|
CLA signed |
normanmaurer
pushed a commit
that referenced
this pull request
Jul 2, 2022
Motivation: Paths, especially those of the form "//path/to/resource" are handled as "/path/to/resource" by most browsers and servers, but Netty fails on these, parsing them as having an authority. Modification: Directly forward the uri when it's a path instead of trying to parse it. Also clean up isOriginForm/isAsteriskForm as their logic seems to be incorrect (for example, URI#getSchemeSpecificPart() never returns null so they both return false trivially). Result: Netty forwards uris unchanged when they look to be in origin or asterisk form.
raidyue
pushed a commit
to raidyue/netty
that referenced
this pull request
Jul 8, 2022
Motivation: Paths, especially those of the form "//path/to/resource" are handled as "/path/to/resource" by most browsers and servers, but Netty fails on these, parsing them as having an authority. Modification: Directly forward the uri when it's a path instead of trying to parse it. Also clean up isOriginForm/isAsteriskForm as their logic seems to be incorrect (for example, URI#getSchemeSpecificPart() never returns null so they both return false trivially). Result: Netty forwards uris unchanged when they look to be in origin or asterisk form.
franz1981
pushed a commit
to franz1981/netty
that referenced
this pull request
Aug 22, 2022
Motivation: Paths, especially those of the form "//path/to/resource" are handled as "/path/to/resource" by most browsers and servers, but Netty fails on these, parsing them as having an authority. Modification: Directly forward the uri when it's a path instead of trying to parse it. Also clean up isOriginForm/isAsteriskForm as their logic seems to be incorrect (for example, URI#getSchemeSpecificPart() never returns null so they both return false trivially). Result: Netty forwards uris unchanged when they look to be in origin or asterisk form.
DerGuteMoritz
added a commit
to clj-commons/aleph
that referenced
this pull request
Aug 22, 2022
4.1.77.Final shipped a fix for CVE-2022-24823. While that vulnerability only affects Java 6 and lower which Clojure doesn't even support, it's still reported by tools like `nvd-clojure`. Thus, bumping to the latest minor version relieves downstream users from having to suppress such false positives. Also included are various smaller bug fixes and improvements. A selection of particularly interesting ones for Aleph users: * netty/netty#12108 * netty/netty#12246 * netty/netty#12270 * netty/netty#12476 * netty/netty#12490 * netty/netty#12533
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.
Motivation:
Paths, especially those of the form "//path/to/resource" are handled as "/path/to/resource" by most browsers and servers, but Netty fails on these, parsing them as having an authority.
Modification:
Directly forward the uri when it's a path instead of trying to parse it.
Also clean up isOriginForm/isAsteriskForm as their logic seems to be incorrect (for example, URI#getSchemeSpecificPart() never returns null so they both return false trivially).
Result:
Netty forwards uris unchanged when they look to be in origin or asterisk form.