Skip to content

ControllerLinkBuilder does not handle multiple X-Forwarded-* headers values #509

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

Closed
smilasek opened this issue Nov 4, 2016 · 6 comments
Closed
Assignees
Milestone

Comments

@smilasek
Copy link

smilasek commented Nov 4, 2016

Hi,

Spring Cloud has released Camden.SR2 this week. It includes following bug fix from Spring Cloud Netflix project: spring-cloud/spring-cloud-netflix#959.
particular commit: spring-cloud/spring-cloud-netflix@a38b7b7

Based on that X-Forwarded-Host, X-Forwarded-Port and X-Forwarded-Proto could have multiple values separated by comma.

X-Forwarded-Host is already handled properly by ControllerLinkBuilder (

String host = forwarded.getHost();
host = hasText(host) ? host : request.getHeader("X-Forwarded-Host");
if (!hasText(host)) {
return builder;
}
String[] hosts = commaDelimitedListToStringArray(host);
String hostToUse = hosts[0];
if (hostToUse.contains(":")) {
String[] hostAndPort = split(hostToUse, ":");
builder.host(hostAndPort[0]);
builder.port(Integer.parseInt(hostAndPort[1]));
} else {
builder.host(hostToUse);
builder.port(-1); // reset port if it was forwarded from default port
}
).

X-Forwarded-Port and X-Forwarded-Proto are not.

Additionally for multiple values of X-Forwarded-Port header application is throwing NumberFormatExceptionbecause of input values like 8080,443 etc.

@smilasek smilasek changed the title ControllerLinkBuilder do not handle multiple X-Forwarded-* headers values ControllerLinkBuilder does not handle multiple X-Forwarded-* headers values Nov 4, 2016
@otrosien
Copy link

otrosien commented Nov 4, 2016

@dsyer could you point to the RFC specifying the X-Forwarded-Port behaviour you implemented in spring-cloud/spring-cloud-netflix@a38b7b7 ?

@otrosien
Copy link

otrosien commented Nov 4, 2016

... found it, it's probably https://tools.ietf.org/html/rfc7239#section-4 -- but in this RFC only "by", "for", "host" and "proto" are defined. The spec allows extensions (#section-5.5), but basically, with regards to "port" it means, that it is implementation-specific. On the other hand, UriComponentsBuilder from spring-framework deals with comma-separated ports, since 4.2.0.RELEASE:
https://github.com/spring-projects/spring-framework/blob/1f6f0dc101fb425bd63786ef669b0626a6591543/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java#L674 / spring-projects/spring-framework@88a1448#diff-73d13991926d9c9b00031a62c7e39c95

@dsyer
Copy link
Member

dsyer commented Nov 4, 2016

There's no RFC for X-Forwarded-Port as far as I know. But it seems to be a common enough convention in proxies that we support it in Spring (https://github.com/spring-projects/spring-framework/blob/1f6f0dc101fb425bd63786ef669b0626a6591543/spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java#L674).

@otrosien
Copy link

The ControllerLinkBuilder should just delegate this logic to UriComponentsBuilder, which as stated above, is already capable of correctly dealing with the X-Forwarded headers.

@odrotbohm
Copy link
Member

Wouldn't mind to see a PR, @otrosien!

otrosien pushed a commit to otrosien/spring-hateoas that referenced this issue Dec 7, 2016
otrosien pushed a commit to otrosien/spring-hateoas that referenced this issue Dec 7, 2016
odrotbohm pushed a commit that referenced this issue Dec 7, 2016
…Framework.

Removed the custom handling of proxy headers in ControllerLinkBuilder in favor of the behavior of UriComponentsBuilder in Spring Framework. The only thing that we keep custom is the handling of X-Forwarded-Ssl as it's rather non-standard.

Original pull request: #519.
odrotbohm added a commit that referenced this issue Dec 7, 2016
Formatting and author note.

Original pull request: #519.
@odrotbohm
Copy link
Member

Thanks to @otrosien, that should be in place now.

@odrotbohm odrotbohm added this to the 0.22 milestone Dec 7, 2016
@odrotbohm odrotbohm self-assigned this Dec 7, 2016
odrotbohm added a commit that referenced this issue Dec 22, 2016
…ilder API.

Our migration to the header handling for #509 (47cefe) unfortunately moved from starting with the servlet mapping for URI creation to using the full URI from the start.

This is now fixed by switching back by creating a UriComponentsBuilder from the current servlet mapping.

Related ticket: #509.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants