ForwardedRequestCustomizer behavior should not be applied to requests without forwarding headers #5445
Conversation
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
There was a problem hiding this comment.
The proto and secure logic is spread out too much and too convoluted.
I think it should just be in one spot and should be something like:
if (forwarded._proto != null)
{
request.setScheme(forwarded._proto);
if (forwarded._proto.equalsIgnoreCase(config.getSecureScheme())
request.setSecure(true);
}
if (forwarded.isSecure())
{
request.setSecure(true);
if (forwarded._proto == null)
request.setScheme(config.getSecureScheme());
}that might not be exactly right... but you get the idea. Put it all in one spot and try to separate concerns as much as possible.
jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java
Outdated
Show resolved
Hide resolved
| // Set secure status | ||
| if (forwarded.isSecure() || | ||
| proto.equalsIgnoreCase(config.getSecureScheme()) || | ||
| port == getSecurePort(config)) |
There was a problem hiding this comment.
I'm don't think that just because it arrives on the secure port is sufficient to make it secure. Some ports might do both secure and insecure requests.
jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java
Show resolved
Hide resolved
|
Issue #5437 is about this kind of request ... A request with no forwarding headers, a fully qualified URL in the request-line, with https. Which is a (modified) test case in this PR ... This testcase is just to prevent regression in the future. But the fix was actually handled in PR #5419 The fix to not act if there are no forwarding headers, which is what this PR covers, also addresses this above example, not changing the request.metadata if there's no forwarding headers. (so in essence issue #5437 was fixed in two different ways). Issue #5443 is about an NPE when there are no forwarding headers and the Host is empty (example in issue is given in HTTP/1.0) Which is two test cases in this PR ... |
+ Simplify isSecure handling in customize. + Simplify handling of `Proxy-Ssl-Id` header. + Simplify handling of `Proxy-auth-cert` header. Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
+ Improve / document implied secure scheme behaviors for both `Proxy-Ssl-Id` or `Proxy-auth-cert` Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
+ Additional tests for HTTP/1.0 + Overly complex negative test cases for `X-Forwarded-Proto: http` and `X-Proxied-Https: off` + Failure testcase for `X-Proxied-Https: foo` Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Additional NPE safety checks. Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
The `X-Proxied-Https: off` case should have an implied port not a hardcoded port. Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime
changed the title
joakime
changed the title
Cleanup handling of forwarded.authority Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
|
Thank you! |
If the request has no Forwarding headers, the customization that ForwardingRequestCustomizer typically does should be skipped.
Closes #5443
Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com