-
-
Notifications
You must be signed in to change notification settings - Fork 15.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
Add Better Path Handling #12533
Add Better Path Handling #12533
Conversation
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. |
@jkjk822 can you please sign our icla: https://netty.io/s/icla ? /cc @ejona86 |
CLA signed |
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.
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.
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.
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
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.