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

Better support for implied ports on ForwardedRequestCustomizer #5419

Merged
merged 2 commits into from Oct 13, 2020

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 9, 2020

#5417
Implied ports were taken from the active connector's HttpConfiguration, this was a bad idea.

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

…tter

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime self-assigned this Oct 9, 2020
@joakime joakime requested a review from gregw October 9, 2020 14:55
@joakime joakime added the Bug For general bugs on Jetty side label Oct 9, 2020
@joakime joakime added this to In progress in Jetty 9.4.33 via automation Oct 9, 2020
@joakime joakime moved this from In progress to Review in progress in Jetty 9.4.33 Oct 9, 2020
@joakime
Copy link
Contributor Author

joakime commented Oct 9, 2020

@gregw I'm not 100% sure this is correct on this PR.

See issue #5419 and then look at this commit.
This PR might be 100% invalid and no change to ForwardedRequestCustomizer is necessary.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Looks good, but a few changes needed.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from gregw October 12, 2020 19:25
Jetty 9.4.33 automation moved this from Review in progress to Reviewer approved Oct 13, 2020
@joakime joakime merged commit c37c2c5 into jetty-9.4.x Oct 13, 2020
Jetty 9.4.33 automation moved this from Reviewer approved to Done Oct 13, 2020
@joakime joakime deleted the jetty-9.4.x-5417-forwarded-proto-port branch October 13, 2020 09:50
joakime referenced this pull request Oct 13, 2020
+ Merge ProxyPass tests from CheckReverseProxyHeadersTest into
  ForwardedRequestCustomizerTest
+ Deleted CheckReverseProxyHeadersTest.java
+ Add more tests for ForcedHost configuration
+ Updated ForwardedRequestCustomizer to conform to expectations

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime changed the title Issue #5417 - Honoring implied ports on ForwardedRequestCustomizer better Better support for implied ports on ForwardedRequestCustomizer Oct 13, 2020
olamy added a commit to jenkinsci/winstone that referenced this pull request Oct 26, 2020
…020 (#124)

* [JENKINS-63958] - Update Jetty from 9.4.32.v20200930 to 9.4.33.v20201019

Picks up https://github.com/eclipse/jetty.project/releases/tag/jetty-9.4.33.v20201019 which includes a fix for jetty/jetty.project#5417 fix is out (PR: jetty/jetty.project#5419 )

* Update parent POM

* jetty version is 9.4.33.v20201020

Signed-off-by: olivier lamy <olamy@apache.org>

* parent 1.59 is broken argLine

Signed-off-by: olivier lamy <olamy@apache.org>

Co-authored-by: olivier lamy <olamy@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Jetty 9.4.33
  
Done
Development

Successfully merging this pull request may close these issues.

Badly configured HttpConfiguration.securePort can lead to wrong port produced by ForwardedHeader
2 participants