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

Headers on redirect #300

Closed
gavwhela opened this issue Sep 17, 2017 · 14 comments

Comments

Projects
None yet
4 participants
@gavwhela
Copy link

commented Sep 17, 2017

I ran into an issue with using httpSource to fetch a file from a website requiring basic authentication. What I found was that the website served the file by issuing a redirect to an Amazon service with separate authorization in the form of a X-Amz-Algorithm query parameter. The problem was that when responseOpen encountered the redirect it carried the Authorization header onto the redirect request, and the Amazon service responds with an error if it receives a request with more than one auth mechanism.

In my research I found that which headers are carried over for a redirected request is generally unspecified. It seems however that the behavior of browsers and curl is to strip the Authorization header on a redirect if the host of the Location response header is different than the host of the original request. Though curl has a option (--location-trusted) to disable this behavior if desired.

In general this seems to be done as a security feature (particularly for http basic authentication), but in my case the website depended on it. I'm unsure what, if any, action should be taken for the http-client library, but this seemed worth bringing up.

There's some decent discussion on header behavior with redirects in golang/go#4800 for their net/http library.

I'm including my workaround here in case anyone runs into the same issue. Gist here for syntax highlighting.

-- Need to see how this handles errors and probably fix infinite
-- redirect possibility. Also should probably not strip auth if the
-- host is the same.
redirectHttpBodySource :: Request -> Source LoaderIO ByteString
redirectHttpBodySource req = httpSource req doRedirects
    where removeAuthHeader req' =
              setRequestHeaders (filter ((/= hAuthorization) . fst) $ requestHeaders req') req'
          doRedirects res = do
            let code = getResponseStatusCode res
                respHeaders = getResponseHeaders res
                cookieJar = responseCookieJar res
                mRedirect = getRedirectedRequest req respHeaders cookieJar code
            case mRedirect of
              Nothing -> getResponseBody res
              Just req' -> redirectHttpBodySource $ removeAuthHeader req'

-- Make sure to set redirect count to 0
-- redirectHttpBodySource req { redirectCount = 0 }
@snoyberg

This comment has been minimized.

Copy link
Owner

commented Sep 18, 2017

@dzhus

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

I'm also interested in having this feature – should it be an extra

stripHeadersOnRedirect :: [HeaderName]

field in the Request?

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Feb 21, 2019

Another bikeshed color:

shouldStripHeaderOnRedirect :: HeaderName -> Bool

This may be more efficient than a linear list traversal.

@dzhus

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@gavwhela As of http-client-0.6.2 you should be able to do

req{ requestHeaders = [(hAuthorization, "mytoken")]
   , shouldStripHeaderOnRedirect = (== hAuthorization)
   }
@snoyberg

This comment has been minimized.

Copy link
Owner

commented Feb 27, 2019

Closing since #386, thanks @dzhus!

@snoyberg snoyberg closed this Feb 27, 2019

@hdgarrood

This comment has been minimized.

Copy link

commented Mar 22, 2019

I think it would be best to be able to say that the Authorization header should be stripped on redirect only in the case where the host you're being redirected to differs from that of the original request. Unless I'm missing something, that isn't currently possible with this API, as we don't get to look at the redirect location when determining whether to strip a header. Would you consider including the redirect location as a parameter to shouldStripHeaderOnRedirect?

Further, would you consider having the above be the default behaviour? I realise it's a breaking change but I think this ought to be considered a security issue: it's very easy to accidentally share your credentials with third parties this way.

@dzhus

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Maybe we should have a separate issue to discuss this? I'm personally in favor of Authorization being stripped as a sane default for the same reason as you. I don't have an opinion on the host matching thing but only because I don't need it yet :)

@hdgarrood

This comment has been minimized.

Copy link

commented Mar 22, 2019

I'm happy to make one if there's an appetite for it :)

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Mar 24, 2019

At some point we simply can't expect any library to address every different case people will come up with. Why not implement the redirect logic in your app instead?

@hdgarrood

This comment has been minimized.

Copy link

commented Mar 24, 2019

I can of course implement the redirect logic in my app (and I have now done so). But as I say, I think the current behaviour is really rather dangerous as it is very easy to accidentally give your credentials to third parties unintentionally. In particular, since other well-established libraries like libcurl, go's net/http, and python's urllib already remove headers like Authorization which are likely to contain sensitive information, I think it seems reasonable to expect that http-client will as well.

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Mar 26, 2019

@hdgarrood

This comment has been minimized.

Copy link

commented Mar 29, 2019

From curl’s manpage, under the -L, --location option:

When authentication is used, curl only sends its credentials to the initial host. If a redirect takes curl to a different host, it won't be able to intercept the user+password. See also --location-trusted on how to change this.

Python’s requests library did at one point include authorization headers in redirects but this was considered by its maintainers to be a security issue once it was pointed out. See kennethreitz/requests#1885 and the relevant CVEs CVE-2014-1829 and CVE-2014-1830.

There’s also the discussion in the go http client library which is linked in the original comment in this thread; they chose to implement the same behaviour as curl based on the docs I quoted above.

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Mar 31, 2019

Thanks for the references. I'd accept a PR to modify the behavior of http-client.

@hdgarrood

This comment has been minimized.

Copy link

commented Mar 31, 2019

Thanks! If nobody beats me to it I might get around to sending a PR eventually, but probably not any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.