Skip to content

ControllerLinkBuilder and multiple X-Forwarded-* headers values. (issue #509) #519

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

Conversation

otrosien
Copy link

@otrosien otrosien commented Dec 7, 2016

Open for discussion: UriComponentsBuilder does not respect X-Forwarded-Ssl, and I didn't see any JIRA issue adding this support. So for now, it's a question if spring-hateoas should just be aligned with the feature set of spring-framework, or add its own customizations. This would basically revert the PR for issue #112. I would vote for aligning it with what UriComponentsBuilder supports, but I'm not to decide.
That's why I put a junit.Ignore on them until this is clarified.

@otrosien
Copy link
Author

otrosien commented Dec 7, 2016

Would also supersede #466.

@odrotbohm
Copy link
Member

Is there a way we can use UriComponentsBuilder for everything but that special handling, i.e. massaging the builder returned by Spring to match the setup X-Forwarded-Ssl defines?

@otrosien
Copy link
Author

otrosien commented Dec 7, 2016

I can give it a try. So you're saying we should care about X-Forwarded-Ssl, even if UriComponentsBuilder doesn't? Isn't it more appropriate to file an issue upstream?

@odrotbohm
Copy link
Member

I am about to file one right away, I just assume a bit of a longer cycle and am not sure it's gonna be introduced in 4.3 in the first place.

@odrotbohm odrotbohm closed this Dec 7, 2016
@odrotbohm odrotbohm reopened this Dec 7, 2016
@otrosien
Copy link
Author

otrosien commented Dec 7, 2016

Alright, so we'll implement X-Forwarded-Ssl as a work-around for the meantime.

@odrotbohm
Copy link
Member

I did a bit of research and can't really find a place where the intended behavior is really well defined. I guess I'll refrain from asking that to be added in Spring Framework then and rather just keep our current implementation around (i.e. the workaround) as we should be able to move quicker or even remove that support easier than core Spring Framework can.

if (hasText(proto)) {
builder.scheme(proto);
} else if (hasText(forwardedSsl) && forwardedSsl.equalsIgnoreCase("on")) {
UriComponentsBuilder builder = UriComponentsBuilder.fromHttpRequest(new ServletServerHttpRequest(request));
Copy link
Member

Choose a reason for hiding this comment

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

Don't we slightly change the logic here? Previously, if a protocol header was defined in any way, the SSL setting was disregarded. I think we'd override it here, wouldn't we?

Copy link
Author

Choose a reason for hiding this comment

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

This is correct. I'll add a test case that makes sure ambiguities are resolved in the same manner.

@otrosien
Copy link
Author

otrosien commented Dec 7, 2016

@olivergierke I think I'm done. On a side-note: ControllerLinkBuilder is one of five files using windows newlines...

@odrotbohm odrotbohm self-assigned this Dec 7, 2016
odrotbohm pushed a commit that referenced this pull request 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 pull request Dec 7, 2016
Formatting and author note.

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

That's polished and merged. I also added you to the team so that you should be able to push issue branches to the repo directly (i.e. /issues/${number}) and I can actually assign tickets to you. :)

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

Successfully merging this pull request may close these issues.

2 participants