search

lostisland / faraday_middleware

Various Faraday middlewares for Faraday-based API wrappers
MIT License
556 stars 205 forks source link

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

Open wyantb opened 5 years ago

wyantb commented 5 years ago

In https://github.com/lostisland/faraday_middleware/pull/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.

iMacTia commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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