Skip to content

Added X-Forwarded-Path to ControllerLinkBuilder #409

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

Conversation

nickgrealy
Copy link

I've added Added X-Forwarded-Path handling in the ControllerLinkBuilder, so that context paths can be handled.

I have signed and agree to the terms of the Spring Individual Contributor
License Agreement.

@gregturn
Copy link
Contributor

I'm trying to find a spec for x-forwarded-path to check things out and I can't. Do you have link documenting proper behavior?

@nickgrealy
Copy link
Author

@gregturn - no, unfortunately I don't have any supporting documentation.

I came across it (generally), while looking for documentation on X-Forwarded-Host - which I also couldn't find in w3c.org, yet exists in ControllerLinkBuilder.

Unless I'm mistaken, use of HTTP header prefix X- is generally discouraged.

Let me know what you want me to do with this...

@nickgrealy
Copy link
Author

The issue number would probably help too... #143

@nickgrealy
Copy link
Author

Hi all - just wondering, who can action the backlog of push requests? There's a few that have been sitting around for a while now. @gregturn @rgladwell @olivergierke

@odrotbohm
Copy link
Member

We can't just add some code for a header that's not specified anywhere. The headers we currently support are documented in this RFC.

@thebignet
Copy link

+1 Didn't see this PR before creating #431 .

@odrotbohm
Copy link
Member

As indicated above, nothing of this is going to make it unless we find a reliable source specifying the behavior of that header.

@thebignet
Copy link

Apparently, the RFC has no mention for X-Forwarded-Host nor X-Forwarded-Port.

X-Forwarded-Path is a made up header with no specification, but there seems to be a concensus as to how it should be used in proxies (see here, and here for examples).

Since it is a non invasive solution meaning that not using this header has no side effects, and that its definition is quite clear, it can be included in spring-hateoas.

@nickgrealy
Copy link
Author

+1 for precedent... ( 3f8b90e -> @olivergierke - is this where X-Forwarded-Host was introduced? )

Is X-Forwarded-Host and X-Forwarded-Port part of a spec? and if not, should they be removed?

Thanks, Nick

@gregturn
Copy link
Contributor

gregturn commented Feb 3, 2016

X-Forwarded-* appears governed by a draft spec here => http://tools.ietf.org/html/draft-ietf-appsawg-http-forwarded-10

Interestingly, I found RFC7239 here => http://tools.ietf.org/html/rfc7239, which appears designed to replace all the X-Forwarded headers.

If that's not enough fun, RFC6648 deprecates the "X-" prefix.

Not sure how to square non-standard, haphazard headers with a newly minted spec for the best approach on this project.

@nickgrealy
Copy link
Author

Ok, well I'm happy with whatever you guys decide.

Just a thought, what if the X-Forwarded-* header handling was a togglable feature - defaulting to disabled - which people could enable (somehow)? Give the choice to the people!

@sebster
Copy link

sebster commented Jun 22, 2016

I was wondering what the specification of this seems to be.

I've run into the same issue and really need a X-Forwarded-Path or equivalent header (to a reverse proxied rest service to avoid needing cross origin requests from a html5 client). RFC7239 seems to leave room for a Forwarded-Path header in section 5.5 (Extensions), specifically mentioning the reverse proxy situation.

The problem with ControllerLinkBuilder as it is now is that there is no way to alter or extend it's behavior: the constructor is package scoped, the getBuilder() as well as the linkTo methods are all static, and the "wiring" of the UriComponentsBuilder instance is fixed by the getBuilder() method implementation. The only option I see at the moment is to just copy the class and modify it and use the modified one instead; but it's not a very nice option.

@osvaldopina
Copy link

I developed a framework to render links in spring hateoas projects. In the current snapshot I'm working on clarifying the extension points. One of these extension points allows you to customize how url parts are obtained.

There is a chain of reponsability in which each processing object can contribute with url parts (scheme, host, port and context path).

This integration test using this configuration shows how it can integrate your own processing object into the chain.

@odrotbohm odrotbohm force-pushed the master branch 2 times, most recently from 4ebc1be to 266ad50 Compare July 25, 2016 18:32
@otrosien
Copy link

otrosien commented Dec 7, 2016

Should be obsolete now. Current master is delegating this to UriComponentsBuilder, which is base-path aware. (use X-Forwarded-Prefix header. see also https://jira.spring.io/browse/SPR-12500)

@odrotbohm
Copy link
Member

Sure? This one is about -Path the ticket you linked to is about -Prefix.

@otrosien
Copy link

otrosien commented Dec 7, 2016

Well, that should be the header to set if you want to add a base-path to your URLs.

@michael-simons
Copy link

Funny, how I end up finding a ticket and personally knowing everyone involved.

The project right here is using -Path as well… While making the switch to "pure" Spring HATOAS without custom wrapper, we found generated URLs not working anymore. I suggested a starter containing a custom filter that maps Prefix to Path and I might just share that one.

@pivotal-issuemaster
Copy link

@nickgrealy Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@nickgrealy Thank you for signing the Contributor License Agreement!

@nickgrealy
Copy link
Author

Fyi @olivergierke - I'm closing this PR due to inactivity. I can reopen again if requested.

@nickgrealy nickgrealy closed this Feb 28, 2018
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.

9 participants