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.

Sign up for GitHub

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

Jump to bottom

ForwardingRequestCustomizer can change request.metadata even if there are no Forwarding headers present #5437

Closed
joakime opened this issue Oct 12, 2020 · 4 comments · Fixed by #5445
Closed

ForwardingRequestCustomizer can change request.metadata even if there are no Forwarding headers present #5437

joakime opened this issue Oct 12, 2020 · 4 comments · Fixed by #5445
Assignees
@joakime
Labels
Bug For general bugs on Jetty side

Comments

@joakime
Copy link
Contributor

joakime commented Oct 12, 2020

Jetty version
9.4.32.v20200930

Description

Originally spotted by @bentmann on commit ac42cbd#r43135647

The ForwardingRequestCustomizer can change request.metadata even if there are no Forwarding headers present.

The example given is a Request with ...

  • No Forwarding headers.
  • An absolute Request URI in the HTTP request line. GET https://localhost:12345
  • ForwardingRequestCustomizer impacts the proto / scheme of the request.metadata.
  • SecureRequestCustomizer runs and doesn't see the correct proto / scheme and fails to run also.
joakime referenced this issue Oct 12, 2020
@joakime
Issue #5247 - Improve testing of ForwardedRequestCustomizer
ac42cbd 
+ 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 <[email protected]>
@joakime joakime added the Bug For general bugs on Jetty side label Oct 12, 2020
@joakime joakime self-assigned this Oct 12, 2020
@joakime joakime mentioned this issue Oct 13, 2020
Closed
@joakime
Copy link
Contributor Author

joakime commented Oct 13, 2020

This was addressed in the changes from PR #5419 already.

@joakime joakime closed this as completed Oct 13, 2020
@bentmann
Copy link

bentmann commented Oct 13, 2020 •
edited
Loading

This was addressed in the changes from PR #5419 already.

Looking at said PR, I don't see how this would resolve the issue:

Nothing in the above flow is triggered by any Forwarded headers, no?

AFAICT, the additional tests in that PR are also all employing some Forwarded header, but no test cases matches the scenario from this issue.

@gregw
Copy link
Contributor

gregw commented Oct 13, 2020

@joakim I think Issues/PRs have got crossed.
This issue will be addressed by #5445 not #5419

@gregw gregw reopened this Oct 13, 2020
@gregw gregw mentioned this issue Oct 13, 2020
Merged
@joakime joakime linked a pull request Oct 13, 2020 that will close this issue
ForwardedRequestCustomizer behavior should not be applied to requests without forwarding headers #5445
Merged
@joakime
Copy link
Contributor Author

joakime commented Oct 13, 2020

PR #5445 merged (which fixes this)

@joakime joakime closed this as completed Oct 13, 2020
@joakime joakime mentioned this issue Oct 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joakime joakime

Labels
Bug For general bugs on Jetty side
Projects
None yet
Milestone
No milestone
3 participants
@gregw @bentmann @joakime