ForwardedHeaderFilter could support X-Forwarded-Prefix as well [SPR-14270]

[spring-projects-issues]

Dave Syer opened SPR-14270 and commented

The ServletUriComponentsBuilder supports X-Forwarded-Prefix as a way to add a context path to the forwarded host and port. The new filter could adopt the same convention.

---

Affects: 4.3 RC2

Issue Links:

• Add support for x-forwarded- HttpServletRequest wrapping [SPR-13614] #18192 Add support for x-forwarded- HttpServletRequest wrapping
• Support for X-Forwarded-Prefix in ServletUriComponentsBuilder [SPR-12500] #17105 Support for X-Forwarded-Prefix in ServletUriComponentsBuilder
• ServletUriComponentsBuilder should replace context path when X-Forwarded-Prefix is present [SPR-16650] #21191 ServletUriComponentsBuilder should replace context path when X-Forwarded-Prefix is present
• ForwardedHeaderFilter should support cases where contextPath should not be replaced with X-Forwarded-Prefix [SPR-14376] #18949 ForwardedHeaderFilter should support cases where contextPath should not be replaced with X-Forwarded-Prefix

0 votes, 5 watchers

[spring-projects-issues]

Eddú Meléndez commented

PR submitted #1071

[spring-projects-issues]

Rossen Stoyanchev commented

Eddú Meléndez, thanks for the pull request as always!

At first I considered whether we should consolidate X-Forwarded-Header processing inside UriComponentsBuilder (or alternatively have ForwardedHeaderFilter delegate to ServletUriComponentsBuilder) but went along with your PR because of the contextPathOverride property in ForwardedHeaderFilter. Now I'm having second thoughts about how X-Forwarded-Header and the manual contextPathOverride relate to each other.

This the example for the contextPathOverride from #18192.

Proxy:
example.com/

Application:
192.168.1.1:8080/example/

Rob Winch do you have any insight what the X-Forwarded-Prefix is in that case or if there is one? Either way the desired end result seems to be removing rather than inserting a prefix.

Following #17105 through to DATAREST-620 and Spring Hateoas #367 the use case is like this:

Proxy:
http://localhost:8080/api/v1/

Application:
http://localhost:8541/

Here the desired end result is simply inserting a prefix based on the X-Forwarded-Header (and leaving the application contextPath as-is).

So perhaps X-Forwarded-Header and the contextPathOverride are independent concepts after all that would not be used at the same time? The X-Forwarded-Header is for inserting a prefix. The contextPathOverride is specifically for the use case where a proxy effectively hides the application's contextPath and it needs to be replaced (or rather removed). As far as I can see semantically X-Forwarded-Prefix can only express a prefix to be inserted, so I suspect it's not even present the latter case.

Well, if this is all correct then the current implementation is still good. I'll just update the documentation to be clear about it.

Thoughts?

[spring-projects-issues]

Rossen Stoyanchev commented

Re-opening to complete the discussion.

[spring-projects-issues]

Dave Syer commented

Rob Winch has pointed out that the prefix needs to be applied instead of the context path, not on top of it. A proxy has no knowledge of servlet context paths, so it only knows how to strip a prefix from the start of the path (after the host:port). In general it wouldn't help anyone to get the context path prepended onto links sent back from the app behind a proxy, so it should always (I believe) be omitted.

[spring-projects-issues]

Rossen Stoyanchev commented

Indeed a proxy has no knowledge of servlet context paths, which is why it feels odd to manipulate the context path in anyway (change or remove). I would also point out that this is not how ServletUriComponentsBuilder works today (right or wrong) and that hasn't been raised as an issue. If we make this change here then those two will not be consistent with each other.

[spring-projects-issues]

Dave Syer commented

I guess ServletUriComponentsBuilder will work for an app with no context path (the majority case for Spring Boot, so a large class of user is happy already). IMO both should be consistent (and not use the context path).

[spring-projects-issues]

Rossen Stoyanchev commented

I've updated ForwardedHeaderFilter to use X-Forwarded-Header (if present) instead of the contetxPath.

[spring-projects-issues]

Dave Syer commented

I think you mean X-Forwarded-Prefix (https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java#L139).