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.

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

redirect messes up with https URIs #505

Closed
sukria opened this issue May 6, 2011 · 5 comments
Closed

redirect messes up with https URIs #505

sukria opened this issue May 6, 2011 · 5 comments
Labels

Comments

@sukria
Copy link
Member

sukria commented May 6, 2011

Hi,

At work we have an Apache frontend that listens on the 443 port. Behind that we have some Starman workers. When the application issues a relative redirect (eg: /login), Dancer tries to rebuild an absolute URI (because of RFC 2616, according to the source) and just ignores the original URI scheme.

This leads to the following unwanted behaviour:

 https://some.app.com/ -> redirect '/login'; -> Location: http://some.app.com/login # wich is bad

Removing the RFC2616 code bloc fixes the issue, but keeps the URI relative...

We need a way to fix that...

@bigpresh
Copy link
Member

bigpresh commented May 6, 2011

I think the fix is to look at the X-Forwarded-Protocol (OTTOMH) header to determine the protocol of the original request, and if it's set, use that. This should probably be configurable so that it only happens if you set a config setting to indicate that your app is behind a proxying front end which will set those headers, otherwise they shouldn't be trusted, as they could be from the client.

The request source IP should also be taken from the X-Forwarded-For header in that case also.

This was discussed on IRC recently, I think someone was starting work on that feature already; I'll need to search my logs to confirm.

@sukria
Copy link
Member Author

sukria commented May 6, 2011

Thanks for the update @bigpresh :)

Indeed your solution seems reasonable to me.

@sukria
Copy link
Member Author

sukria commented May 7, 2011

By the way, this bug is rather important to me (need a fix for it at work), just my two cents to vote for working on that one ASAP ;)

@ambs
Copy link
Member

ambs commented May 7, 2011

Check #512.

@ambs
Copy link
Member

ambs commented May 12, 2011

Fixed on #512, already merged.

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

No branches or pull requests

3 participants