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

fix: X-Forwarded-Prefix not working when using MVC #3443

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

jrhenderson1988
Copy link
Contributor

Fixes an issue whereby the X-Forwarded-Prefix header was not being added to the request when stripping a prefix. It does so by storing additional information in the request attributes and using that information to generate the prefix value later in the chain, similar to how it appears to be done in the reactive version of the gateway.

If this is not the right approach or there's something that can be improved, please do let me know.

Fixes gh-3354

@jrhenderson1988 jrhenderson1988 force-pushed the fix/x-forwarded-prefix-header-when-strip-prefix-used branch from 4ee7f91 to f934c26 Compare July 23, 2024 20:28
XForwardedRequestHeadersFilter.X_FORWARDED_PROTO_HEADER,
XForwardedRequestHeadersFilter.X_FORWARDED_FOR_HEADER);
assertThat(headers).containsEntry(
XForwardedRequestHeadersFilter.X_FORWARDED_PREFIX_HEADER, "/long/path/to");
Copy link
Member

Choose a reason for hiding this comment

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

What is the value here without the fix? /get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the fix, the header is missing entirely. I believe this is the bug as raised in the original issue. The X-Forwarded-Prefix header is missing when using MVC, but it is present when using Reactive/WebFlux

.filter(stripPrefix(3))
.filter(addRequestHeader("X-Test", "stripPrefix"))
.filter(new HttpbinUriResolver(true))
Copy link
Member

Choose a reason for hiding this comment

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

Help me understand why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I can't quite remember the details of why I had to do this as it's been a while since I worked on this. I was struggling getting tests to pass, despite believing that they should. The HttpbinUriResolver in its current state (without my changes) seems to return a new URI, without any path information being preserved:

return URI.create(String.format("http://%s:%d", host, port));

and since that was earlier in the filter chain than stripPrefix etc., that seemed to result in the path being chopped off before the stripPrefix filter was executed.

For the stripPrefixPostWorks test - If I undo all of my changes to HttpbinUriResolver, and put it back to the original position in the filter chain, then the HttpBin container seems to respond with a 500 internal server error, but with my code in place, it returns 200. Further, if I leave my modifications in place and just change the position in this test to move it further down the chain, I get the same outcome (500 response), but the requests sent to HttpBin appear to be identical to the one where it returns a 200.

I may be going completely down the wrong path here, but I'm convinced that either I'm doing something wrong or something is not quite right.

I'd appreciate any pointers for getting these two tests correct

…rded-Prefix header. Also removed unused variable.
…lved URL. This particular filter used in testing is currently throwing away the whole URL and building a new one, and it interferes with tests. This change allows the filter to be less destructive and do the bare minimum to ensure that requests are forwarded to HTTP Bin by changing only the hostname, port and scheme, but nothing else
…he X-Forwarded-Prefix header is added and what we expect.
@jrhenderson1988 jrhenderson1988 force-pushed the fix/x-forwarded-prefix-header-when-strip-prefix-used branch from 9c47a3b to 9432657 Compare September 28, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

X-Forwarded-Prefix not working when using MVC
3 participants