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

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
39c6237
Add support for saving original request URL to request attributes
jrhenderson1988 Jun 29, 2024
c33aeea
save original request URL and updated request URL following prefix strip
jrhenderson1988 Jun 29, 2024
2e4552d
use original request URL and updated request URL to calculate X-Forwa…
jrhenderson1988 Jun 29, 2024
279af98
Add support to HttpbinUriResolver for preserving the path of the reso…
jrhenderson1988 Jun 29, 2024
bad0448
First stab at getting some integration tests running to verify that t…
jrhenderson1988 Jun 29, 2024
713e57c
remove commented out tests and ensure that we preserve the path when …
jrhenderson1988 Jun 29, 2024
5bd5c61
remove failing deploy docs job temporarily
jrhenderson1988 Jun 30, 2024
d2b8c6b
add master to list of target branches for running Maven build
jrhenderson1988 Jun 30, 2024
2b2f1d5
move position of http bin uri resolver filter back to original position
jrhenderson1988 Jun 30, 2024
c682aa5
move strip httpbin uri resolver filter back to later position
jrhenderson1988 Jun 30, 2024
fbe943c
re-order position of httpbin uri resolver filter in setup for test
jrhenderson1988 Jun 30, 2024
9852de1
re-add deploy-docs.yml
jrhenderson1988 Jun 30, 2024
0747ad4
add some extra assertions
jrhenderson1988 Jun 30, 2024
2f8a33a
remove master from accept list of PR branches to run tests on
jrhenderson1988 Jun 30, 2024
9432657
Remove duplicated assertion
jrhenderson1988 Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions ...erver-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/common/MvcUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -72,6 +73,11 @@ public abstract class MvcUtils {
*/
public static final String GATEWAY_ATTRIBUTES_ATTR = qualify("gatewayAttributes");

/**
* Gateway original request URL attribute name.
*/
public static final String GATEWAY_ORIGINAL_REQUEST_URL_ATTR = qualify("gatewayOriginalRequestUrl");

/**
* Gateway request URL attribute name.
*/
Expand Down Expand Up @@ -242,6 +248,15 @@ public static void setRequestUrl(ServerRequest request, URI url) {
request.servletRequest().setAttribute(GATEWAY_REQUEST_URL_ATTR, url);
}

public static void addOriginalRequestUrl(ServerRequest request, URI url) {
LinkedHashSet<URI> urls = getAttribute(request, GATEWAY_ORIGINAL_REQUEST_URL_ATTR);
if (urls == null) {
urls = new LinkedHashSet<>();
}
urls.add(url);
putAttribute(request, GATEWAY_ORIGINAL_REQUEST_URL_ATTR, urls);
}

private record ByteArrayInputMessage(ServerRequest request, ByteArrayInputStream body) implements HttpInputMessage {

@Override
Expand Down
3 changes: 3 additions & 0 deletions .../main/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ public static Function<ServerRequest, ServerRequest> stripPrefix() {

public static Function<ServerRequest, ServerRequest> stripPrefix(int parts) {
return request -> {
MvcUtils.addOriginalRequestUrl(request, request.uri());
// TODO: gateway url attributes
String path = request.uri().getRawPath();
// TODO: begin duplicate code from StripPrefixGatewayFilterFactory
Expand All @@ -409,6 +410,8 @@ public static Function<ServerRequest, ServerRequest> stripPrefix(int parts) {
.replacePath(newPath.toString())
.build()
.toUri();
MvcUtils.setRequestUrl(request, prefixedUri);

return ServerRequest.from(request).uri(prefixedUri).build();
};
}
Expand Down
10 changes: 4 additions & 6 deletions ...a/org/springframework/cloud/gateway/server/mvc/filter/XForwardedRequestHeadersFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.DeprecatedConfigurationProperty;
import org.springframework.boot.context.properties.PropertyMapper;
import org.springframework.cloud.gateway.server.mvc.common.MvcUtils;
import org.springframework.core.Ordered;
import org.springframework.http.HttpHeaders;
import org.springframework.util.ObjectUtils;
Expand Down Expand Up @@ -397,18 +398,15 @@ public HttpHeaders apply(HttpHeaders input, ServerRequest request) {
// - see XForwardedHeadersFilterTests, so first get uris, then extract paths
// and remove one from another if it's the ending part.

LinkedHashSet<URI> originalUris = null; // TODO:
// exchange.getAttribute(GATEWAY_ORIGINAL_REQUEST_URL_ATTR);
URI requestUri = null; // TODO:
// exchange.getAttribute(GATEWAY_REQUEST_URL_ATTR);
LinkedHashSet<URI> originalUris = MvcUtils.getAttribute(request,
MvcUtils.GATEWAY_ORIGINAL_REQUEST_URL_ATTR);
URI requestUri = MvcUtils.getAttribute(request, MvcUtils.GATEWAY_REQUEST_URL_ATTR);

if (originalUris != null && requestUri != null) {

originalUris.forEach(originalUri -> {

if (originalUri != null && originalUri.getPath() != null) {
String prefix = originalUri.getPath();

// strip trailing slashes before checking if request path is end
// of original path
String originalUriPath = stripTrailingSlash(originalUri);
Expand Down
20 changes: 18 additions & 2 deletions ...src/test/java/org/springframework/cloud/gateway/server/mvc/ServerMvcIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,14 @@ public void stripPrefixWorks() {
.consumeWith(res -> {
Map<String, Object> map = res.getResponseBody();
Map<String, Object> headers = getMap(map, "headers");
assertThat(headers).containsKeys(
XForwardedRequestHeadersFilter.X_FORWARDED_PREFIX_HEADER,
XForwardedRequestHeadersFilter.X_FORWARDED_HOST_HEADER,
XForwardedRequestHeadersFilter.X_FORWARDED_PORT_HEADER,
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

assertThat(headers).containsEntry("X-Test", "stripPrefix");
});
}
Expand All @@ -266,6 +274,14 @@ public void stripPrefixPostWorks() {
Map<String, Object> map = res.getResponseBody();
assertThat(map).containsEntry("data", "hello");
Map<String, Object> headers = getMap(map, "headers");
assertThat(headers).containsKeys(
XForwardedRequestHeadersFilter.X_FORWARDED_PREFIX_HEADER,
XForwardedRequestHeadersFilter.X_FORWARDED_HOST_HEADER,
XForwardedRequestHeadersFilter.X_FORWARDED_PORT_HEADER,
XForwardedRequestHeadersFilter.X_FORWARDED_PROTO_HEADER,
XForwardedRequestHeadersFilter.X_FORWARDED_FOR_HEADER);
assertThat(headers).containsEntry(
XForwardedRequestHeadersFilter.X_FORWARDED_PREFIX_HEADER, "/long/path/to");
assertThat(headers).containsEntry("X-Test", "stripPrefixPost");
});
}
Expand Down Expand Up @@ -1080,9 +1096,9 @@ public RouterFunction<ServerResponse> gatewayRouterFunctionsSetPathPost() {
public RouterFunction<ServerResponse> gatewayRouterFunctionsStripPrefix() {
// @formatter:off
return route(GET("/long/path/to/get"), http())
.filter(new HttpbinUriResolver())
.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

.withAttribute(MvcUtils.GATEWAY_ROUTE_ID_ATTR, "teststripprefix");
// @formatter:on
}
Expand All @@ -1092,9 +1108,9 @@ public RouterFunction<ServerResponse> gatewayRouterFunctionsStripPrefixPost() {
// @formatter:off
return route("teststripprefixpost")
.route(POST("/long/path/to/post").and(host("**.stripprefixpost.org")), http())
.filter(new HttpbinUriResolver())
.filter(stripPrefix(3))
.filter(addRequestHeader("X-Test", "stripPrefixPost"))
.filter(new HttpbinUriResolver(true))
.build();
// @formatter:on
}
Expand Down
21 changes: 21 additions & 0 deletions ...c/src/test/java/org/springframework/cloud/gateway/server/mvc/test/HttpbinUriResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.springframework.cloud.gateway.server.mvc.test;

import java.net.URI;
import java.net.URISyntaxException;
import java.util.function.Function;

import org.springframework.cloud.gateway.server.mvc.common.MvcUtils;
Expand All @@ -30,12 +31,32 @@
public class HttpbinUriResolver
implements Function<ServerRequest, ServerRequest>, HandlerFilterFunction<ServerResponse, ServerResponse> {

private final boolean preservePath;

public HttpbinUriResolver(boolean preservePath) {
this.preservePath = preservePath;
}

public HttpbinUriResolver() {
this(false);
}

protected URI uri(ServerRequest request) {
ApplicationContext context = MvcUtils.getApplicationContext(request);
Integer port = context.getEnvironment().getProperty("httpbin.port", Integer.class);
String host = context.getEnvironment().getProperty("httpbin.host");
Assert.hasText(host, "httpbin.host is not set, did you initialize HttpbinTestcontainers?");
Assert.notNull(port, "httpbin.port is not set, did you initialize HttpbinTestcontainers?");
if (preservePath) {
URI original = request.uri();
try {
return new URI("http", original.getUserInfo(), host, port, original.getPath(),
original.getQuery(), original.getFragment());
} catch (URISyntaxException e) {
throw new IllegalArgumentException(e.getMessage(), e);
}
}

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

Expand Down
Loading