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

FollowRedirects clearing authorization header on redirect by default breaks integration #187

Open
wyantb opened this issue Mar 15, 2019 · 3 comments
Labels

Comments

@wyantb
Copy link

wyantb commented Mar 15, 2019

In #183 , the FollowRedirects middleware behavior changed from keeping auth headers by default to clearing auth headers by default. I appreciate that the option is useful, but I don't think that a minor version bump (from 0.12.2 to 0.13.1) should introduce such a notable change without warning.

How about making the default false? If others like me have a Gemfile.lock that fixes their faraday_middleware version to an older version, they may not detect this issue for some time in their projects. Having the default be backwards compatible would avoid some sticker shock.

@wyantb wyantb changed the title Clearing authorization header on redirect by default breaks integration FollowRedirects clearing authorization header on redirect by default breaks integration Mar 15, 2019
@iMacTia
Copy link
Member

iMacTia commented Mar 18, 2019

@wyantb I'm really sorry you had issues because of this change.
The reason we went for releasing it even with a minor release is because we've addressed it as a security bug.

You're perfectly right about backwards-compatibility, however this change should only take place when you're redirected to a different hostname, so there should be no impact on normal applications.

We've decided to address this as a security bug because sending the same auth header to different hostnames by mistake is wrong in most cases.

I'm actually curious about your specific case and how this has affected you.
Would you be willing to share more details about your scenario?
Maybe we can actually do some improvement that won't affect people in your same position.

@wyantb
Copy link
Author

wyantb commented Mar 18, 2019

In my particular case I'm calling Nest's API, https://developer-api.nest.com/devices, which does a status 307 redirect to some firebase API https://firebase-blah-blah.dapi.production.nest.com. Catching that both were nest.com in my case and preserving the header would've kept me in good shape, though I'm not sure it's safe to assume that subdomains should always be given the same auth headers. https://developers.nest.com/guides/api/how-to-handle-redirects and their docs have some more info.

@iMacTia
Copy link
Member

iMacTia commented Mar 18, 2019

Thanks @wyantb, that's exactly the scenario I was thinking about.
Sounds reasonable to me that sub-domains should be considered safe when forwarding the header, and makes sense in the context of a microservices-oriented infrastructure.
In this case, rather than changing the default to an insecure one, I'd gladly accept a PR that makes the check smarter and considers sub-domains safe, rather than simply matching the whole domain

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

2 participants