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

Request.isSecure() returns false for https schemes in Jetty 10 #5448

Closed
joakime opened this issue Oct 13, 2020 · 1 comment · Fixed by #5450
Closed

Request.isSecure() returns false for https schemes in Jetty 10 #5448

joakime opened this issue Oct 13, 2020 · 1 comment · Fixed by #5450

Comments

@joakime
Copy link
Contributor

joakime commented Oct 13, 2020

Jetty version
10.0.x

Description
A request with https in it's scheme + authority will not return true for Request.isSecure().

The recent merge from jetty-9.4.x to jetty-10.0.x with ForwardingRequestCustomizerTest improvements highlighted this behavior.
See merge: 6856009#diff-6a150c3e05fb3d2c83189d7ded784eba8781a738a6018699bc082d46f176ee0eR697

That merge added a seemingly missing line in HttpChannel.onRequest().

But the change in Jetty 10 seems to prefer that to occur in Request.setMetadata() instead (line 1700).

https://github.com/eclipse/jetty.project/blob/68560090fe7332e28335335ed3ac43492b1d391d/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L1683-L1700

This change made during the merge should be reviewed.

@gregw
Copy link
Contributor

gregw commented Oct 14, 2020

Note that line 1700 is a more limited handling of https. It is only for when it is in the URI - ie for a proxied or HTTP/2 request
It will not trigger if a customiser has set the scheme... but then it may not need to....

let's review together before meeting tonight.

joakime added a commit that referenced this issue Oct 14, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
gregw pushed a commit that referenced this issue Oct 16, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
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 a pull request may close this issue.

2 participants