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

Should twitcher allow redirects? #89

Open
davidcaron opened this issue Sep 20, 2019 · 4 comments
Open

Should twitcher allow redirects? #89

davidcaron opened this issue Sep 20, 2019 · 4 comments

Comments

@davidcaron
Copy link

@cehbrecht and @fmigneault I wanted to get your thoughts before submitting a PR.

If you look at this commit, I explain roughly why I think twitcher shouldn't allow redirects: 6e8203a

Our use case

Our problem is that we want to protect the web interface of geoserver behind twitcher. When a user logs in, a POST request is sent to twitcher and forwarded to geoserver. Geoserver responds with a series of 302 redirects. Twitcher sends back the last of these responses, which is the main geoserver page. But the user's browser still thinks it's at the POST url, and relative urls are broken. When I got to actually log in, all css, images and others were not loaded. With these changes, I could login successfully, because the redirects were sent to my browser directly, just as if twitcher wasn't there.

Where it could break existing applications

If application generates self-referential urls in the response content, this change should not break them if the app was configured correctly previously.

I believe the only place where this can cause a problem is when there is a redirect, and the app generates location header. If twitcher follows redirects, it will work. But if twitcher doesn't follow redirects, the browser will receive the protected url in the Location header and try to follow it. It will obviously break.

So... making this change would at least need to bump the minor 0.x version.

@davidcaron
Copy link
Author

Also, I found a project similar to twitcher, and it seems to also not follow redirects: https://github.com/TracyWebTech/django-revproxy/blob/b8d1d9e44eadbafbd16bc03f04d15560089d4472/revproxy/views.py#L162

One thing that would help is if twitcher tried to replace the protected url before sending back the location header, just like they do here: https://github.com/TracyWebTech/django-revproxy/blob/b8d1d9e44eadbafbd16bc03f04d15560089d4472/revproxy/views.py#L176

@fmigneault
Copy link
Contributor

fmigneault commented Sep 20, 2019

Only place I'm aware of Location being used in PAVICS is with Magpie login procedure, but since it's not behind Twitcher, I'm not too worried about it.
Weaver also uses Location in some cases, but more in a informative way than to follow it itself, so this also shouldn't be a problem.
Seems related to some display issues logged here: Ouranosinc/Magpie#76

Nothing else comes to mind for me.

@dbyrns @huard @tlvu tagging you for any additional use cases/inputs?

@tlvu
Copy link

tlvu commented Sep 23, 2019

I am not fully aware of all the capabilities/features/usecases of twitcher/magpie yet but I think I am okay with not following redirect.

I am not a web developper so what I am about to say might be too simple minded.

Putting myself in the shoes of a webapp dev, I would assume that my app is talking directly to the customer and if there are any proxies in the middle, they should try to be as much transparent as possible. So if I generate a Location redirect directive, it is intended for the customer so I would not expect the proxy in front of me to follow it and swallow it.

Worst case, we can make this "follow redirect behavior" configurable? Do not implement now, only when we need it of course.

@cehbrecht
Copy link
Member

@davidcaron I will follow your advice. It sounds like all the current use-cases are still supported.

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