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.

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

Fixes #8014 - Review HttpRequest URI construction. #8015

Merged
merged 5 commits into from
May 26, 2022

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented May 17, 2022

Now always adding a "/" before the path, if not already present.
Disabled flakey HTTP/3 test.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

Now always adding a "/" before the path, if not already present.
Disabled flakey HTTP/3 test.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from joakime May 17, 2022 17:17
joakime
joakime previously approved these changes May 17, 2022
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

LGTM

@gregw
Copy link
Contributor

gregw commented May 17, 2022

@simone, i think we should also fix the asString method in HttpURI. I know it's not used, but it makes the same mistake with non root paths.

Fixes after review in HttpURI.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
joakime
joakime previously approved these changes May 23, 2022
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

I like the IllegalArgumentExceptions on this

More fixes after review in HttpURI.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
joakime
joakime previously approved these changes May 23, 2022
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

The OPTIONS path of * makes this interesting for sure.

isPathValidForAuthority(String) method makes this much easier to understand now too.

@gregw
Copy link
Contributor

gregw commented May 23, 2022

Note, I'm working on fixing the test failure on this one.

@gregw gregw self-assigned this May 23, 2022
Parse CONNECT URIs as Authority
@gregw
Copy link
Contributor

gregw commented May 24, 2022

@sbordet @joakime I've made a change to treat the URI of a CONNECT request as the Authority rather than as a path (which I believe is more correct as per the RFC).
But it is kind of a significant change, so careful review needed.

@gregw gregw requested a review from joakime May 24, 2022 00:40
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable change.
Lets see what the CI build/test shows.

@gregw
Copy link
Contributor

gregw commented May 24, 2022

The CI failure was an unrelated problem with a repository. Retrying...

@gregw
Copy link
Contributor

gregw commented May 25, 2022

Still having CI issues... which I think are unrelated. If I get a clean build (again) locally, I will merge.

@gregw gregw merged commit d1e64f4 into jetty-10.0.x May 26, 2022
@gregw gregw deleted the jetty-10.0.x-8014-review-httprequest-uri branch May 26, 2022 08:13
HyukjinKwon pushed a commit to apache/spark that referenced this pull request Jul 9, 2022
…4.48.v20220622

### What changes were proposed in this pull request?
Upgrade `jetty-http` from 9.4.46.v20220331 to 9.4.48.v20220622

### Why are the changes needed?
[Release note](https://github.com/eclipse/jetty.project/releases)

[CVE-2022-2047](https://nvd.nist.gov/vuln/detail/CVE-2022-2047)

Info from Github dependabot

### Invalid URI parsing may produce invalid HttpURI.authority

### Description
URI use within Jetty's `HttpURI` class can parse invalid URIs such as `http://localhost;/path` as having an authority with a host of `localhost;`.

A URIs of the type `http://localhost;/path` should be interpreted to be either invalid or as `localhost;` to be the userinfo and no host.
However, `HttpURI.host` returns `localhost;` which is definitely wrong.

### Impact
This can lead to errors with Jetty's `HttpClient`, and Jetty's `ProxyServlet` / `AsyncProxyServlet` / `AsyncMiddleManServlet` wrongly interpreting an authority with no host as one with a host.

### Patches
Patched in PR jetty/jetty.project#8146 for Jetty version 9.4.47.
Patched in PR jetty/jetty.project#8015 for Jetty versions 10.0.10, and 11.0.10

### Workarounds
None.

### For more information
If you have any questions or comments about this advisory:

Email us at [securitywebtide.com](mailto:securitywebtide.com)."

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
Pass GA

Closes #37142 from bjornjorgensen/jetty-http-9.4.48.v20220622.

Lead-authored-by: Bjørn Jørgensen <bjornjorgensen@gmail.com>
Co-authored-by: Bjorn Jorgensen <bjornjorgensen@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants