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

net/http: copy headers when following redirects #4800

Closed
rsc opened this Issue Feb 13, 2013 · 29 comments

Comments

Projects
None yet
9 participants
@rsc
Copy link
Contributor

rsc commented Feb 13, 2013

http changed recently to follow a redirect on POST. However, the followup request does
not include the headers from the original request. This caused a problem in a Selenium
setting because the Accept: app/json header was not propagated. Is there a standard for
which headers should be copied when following a redirect? Should we, uh, follow it?
@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Feb 13, 2013

Comment 1:

I see nothing either way in:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3
On one hand, it seems sane to copy all the headers when following a redirect.
On the other hand, what if one of those headers is "Authorization" and it contains
credentials you don't want to pass along to another party?
I don't know the answer.  I'm curious what other implementations do.
@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Feb 13, 2013

Comment 2:

Copy/paste of an internal G+ comment I got about this:
-----------
As per these threads, it's not totally specified by HTTP itself:
http://lists.w3.org/Archives/Public/ietf-http-wg/2011AprJun/0489.html
http://lists.w3.org/Archives/Public/ietf-http-wg/2011JulSep/0014.html
See HTML which specifies some part of it:
http://www.whatwg.org/specs/web-apps/current-work/multipage/fetching-resources.html
"""
First, apply any relevant requirements for redirects (such as showing any appropriate
prompts). Then, redo main step, but using the target of the redirect as the resource to
fetch, rather than the original resource. For HTTP requests, the new request must
include the same headers as the original request, except for headers for which other
requirements are specified (such as the Host header). [HTTP]
"""
As far as Chrome's HTTP stack, we only preserve embedder headers. Here's the algorithm:
* Embedder (Chrome/WebKit) specifies "extra" request headers in the URL request. A URL
request follows HTTP redirects, so since these are tied to the URL request, they are
sent on all HTTP requests.
* All other headers are generated internally during HTTP processing:
  - At the HTTP transaction level (actually, in URLRequestHttpJob), we add some headers, like cookies.
  - At the network transaction level, we generate the Host header and others. This is also the level at which we do authorization. We re-run the authorization algorithm there, which includes querying our auth cache and re-applying a token if applicable.
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 12, 2013

Comment 3 by stephane.travostino:

+1, in a Go crawler I've written recently, I had to create a custom http.Client function
which, upon redirect, kept the "Range" and "User-Agent" headers I set.
Looking at Python's urllib2 implementation, they have a map of "unredirected headers"
such as "Authorization" and other internally generated ones, while headers set by the
user are kept upon redirection.
What if a user sets an Authorization header manually and the URL redirects to a
different domain? 
I think the implementation should keep it, so the idea of having a "custom headers"
array and "unredirected headers" one is the way to go, IMHO.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Mar 12, 2013

Comment 4:

Labels changed: added go1.1maybe, removed go1.1.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented May 15, 2013

Comment 5 by alberto.garcia.hierro:

I've checked what libcurl does (which I'd consider a pretty solid implementation) and it
forwards any headers but authentication info. From the curl manual:
"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."
 --location-trusted
              (HTTP/HTTPS) Like -L, --location, but will allow sending the name + password to all hosts that the site may redirect to. This may or may not introduce a security breach if the site redirects you to a site to which you'll send your authentication info (which  is  plain-
              text in the case of HTTP Basic authentication).
I think this is a pretty sensible behavior and I already have a patch which implements
this is the net/http package. By default, all headers but WWW-Authenticate are copied
and the User property in the initial URL is not copied. If the property TrustRedirects
in the request it set, it will copy everything, including the User property of the
initial URL. So, should I just submit this patch as a CL or do you have any objections?
@robpike

This comment has been minimized.

Copy link
Contributor

robpike commented May 18, 2013

Comment 6:

Labels changed: added go1.2maybe, removed go1.1maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jun 26, 2013

Comment 7:

Issue #5782 has been merged into this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jul 30, 2013

Comment 8:

Labels changed: added feature.

@robpike

This comment has been minimized.

Copy link
Contributor

robpike commented Aug 30, 2013

Comment 9:

Not for 1.2.

Labels changed: removed go1.2maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Nov 27, 2013

Comment 10:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Nov 27, 2013

Comment 11:

Labels changed: removed feature.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 12:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 13:

Labels changed: added repo-main.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 9, 2014

Comment 14:

Issue #7801 has been merged into this issue.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 19, 2014

Comment 15:

Issue #8027 has been merged into this issue.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jul 29, 2014

Comment 16 by steven.hartland@multiplay.co.uk:

Just hit this with our code, specifically not preserving the range request headers which
means it prevents any sort of attempt to process ranged downloads unless a custom 
transport / client is written :(
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jul 29, 2014

Comment 17 by steven.hartland@multiplay.co.uk:

@ alberto from #5 do you have your proposed patch hosted somewhere we can look at?
@rogpeppe

This comment has been minimized.

Copy link
Contributor

rogpeppe commented Sep 21, 2014

Comment 18:

I've just hit this issue when using cookies against http.ServeMux.
The cookies are lost when the ServeMux redirects.
Here's an example of it happening:
http://play.golang.org/p/sMzbiLDm2i
and here's a simple workaround:
http://play.golang.org/p/JiGekzeAAY
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 19, 2014

Comment 19 by gyuho.cs:

I just hit this issue when using proxy to redirects. Any updates?
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 28, 2014

Comment 20 by sulochan:

I am using go version go1.3.3, and i am still seeing this issue. When
doFollowingRedirects is followed the new request is not copying the headers. Any updates
?
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 28, 2014

Comment 21 by gyuho.cs:

I don't think we need to enforce the headers to redirects. It's totally doable with
CheckRedirect... I use this for my personal API calls and http request. Works well.
https://github.com/gyuho/gon/blob/master/httpclient/httpclient.go#L72
@karalabe

This comment has been minimized.

Copy link
Contributor

karalabe commented Dec 19, 2014

Just got bitten by this one. I was implementing an REST client to eventbrite and if I don't specify a trailing slash after a resource, eventbrite will redirect to the slashed version. However, this means that all the auth headers get dropped, and the redirected GET fails.

Wouldn't it be possible/worthwhile to check if the redirect refers to the same host, and copy the headers in such cases?

pdf added a commit to pdf/gomusicbrainz that referenced this issue Jan 5, 2015

Preserve HTTP headers on redirect
This ensures things like our custom UserAgent get preserved if the
server does a redirect (my mirror enforces SSL, for example).

See golang/go#4800

Code cribbed from gyuho/gon

mislav added a commit to github/hub that referenced this issue Aug 9, 2016

mislav added a commit to github/hub that referenced this issue Aug 10, 2016

@buro9

This comment has been minimized.

Copy link

buro9 commented Aug 16, 2016

Also just been bitten by this, calling an API with an Authorization header, and the API does a redirect. I can't avoid the redirect, the API implements a /whoami endpoint that looks at the identity of the owner of the Authorization token and then redirects to the given /profile/%d that is the person. The issue is that without the Authorization header being present on that subsequent call I get back a guest view of the profile in question, rather than the profile owners view of that profile.

The suggestion of implementing a solution similar to cURL wherein headers are retained for redirects on the same host (and scheme would be wise too) is a good one.

This is go1.7 btw.

@opennota

This comment has been minimized.

Copy link

opennota commented Aug 29, 2016

Any updates? This bug can confuse Go newbies. For example, if one uses Reddit API and, having set a unique User-Agent, issues GET requests to, say, https://reddit.com/r/golang/top.json, one will get 429 Too Many Requests most of the time, because reddit.com redirects to www.reddit.com and User-Agent is quietly replaced upon redirect by Go-http-client/1.1, which is, being too common, is heavily rate limited. Meanwhile, in Python the same code just works. This isn't exactly what would attract beginners.

@campoy

This comment has been minimized.

Copy link
Contributor

campoy commented Sep 7, 2016

I ran on an issue related to this with talks.godoc.org

Details: golang/gddo#440

@bradfitz bradfitz modified the milestones: Go1.8, Unplanned Sep 9, 2016

@bradfitz bradfitz self-assigned this Sep 9, 2016

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Sep 9, 2016

CL https://golang.org/cl/28930 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Oct 18, 2016

CL https://golang.org/cl/31435 mentions this issue.

gopherbot pushed a commit that referenced this issue Oct 25, 2016

net/http: fix redirect logic to handle mutations of cookies
In the situation where the Client.Jar is set and the Request.Header
has cookies manually inserted, the redirect logic needs to be
able to apply changes to cookies from "Set-Cookie" headers to both
the Jar and the manually inserted Header cookies.

Since Header cookies lack information about the original domain
and path, the logic in this CL simply removes cookies from the
initial Header if any subsequent "Set-Cookie" matches. Thus,
in the event of cookie conflicts, the logic preserves the behavior
prior to change made in golang.org/cl/28930.

Fixes #17494
Updates #4800

Change-Id: I645194d9f97ff4d95bd07ca36de1d6cdf2f32429
Reviewed-on: https://go-review.googlesource.com/31435
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

kyhavlov added a commit to hashicorp/consul that referenced this issue Nov 4, 2016

kyhavlov added a commit to hashicorp/consul that referenced this issue Nov 4, 2016

slackpad added a commit to hashicorp/consul that referenced this issue Nov 5, 2016

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Nov 14, 2016

fhajny
Update sysutils/consul to 0.7.1.
BACKWARDS INCOMPATIBILITIES:

- Child process reaping support has been removed, along with the reap
  configuration option.
- The default for max_stale has been increased to a near-indefinite
  threshold (10 years) to allow DNS queries to continue to be served in
  the event of a long outage with no leader.
- The api package's PreparedQuery.Delete() method now takes WriteOptions
  instead of QueryOptions.


FEATURES:

- Key/Value Store Command Line Interface: New consul kv commands were
  added for easy access to all basic key/value store operations.
- Snapshot/Restore: A new /v1/snapshot HTTP endpoint and corresponding
  set of consul snapshot commands were added for easy point-in-time
  snapshots for disaster recovery.
- AWS auto-discovery: New -retry-join-ec2 configuration options added to
  allow bootstrapping by automatically discovering AWS instances with a
  given tag key/value at startup.


IMPROVEMENTS:

- api: All session options can now be set when using api.Lock().
- agent: Added the ability to bind Serf WAN and LAN to different
  interfaces than the general bind address.
- agent: Added a new tls_skip_verify configuration option for HTTP
  checks.
- agent: Consul is now built with Go 1.7.3.


BUG FIXES:

- agent: Fixed a Go race issue with log buffering at startup.
- agent: Fixed a panic during anti-entropy sync for services and checks.
- agent: Fixed an issue on Windows where "wsarecv" errors were logged
  when CLI commands accessed the RPC interface.
- agent: Syslog initialization will now retry on errors for up to 60
  seconds to avoid a race condition at system startup.
- agent: Fixed a panic when both -dev and -bootstrap-expect flags were
  provided.
- agent: Added a retry with backoff when a session fails to invalidate
  after expiring.
- agent: Fixed an issue where Consul would fail to start because of
  leftover malformed check/service state files.
- agent: Fixed agent crashes on macOS Sierra by upgrading Go.
- agent: Log a warning instead of success when attempting to deregister
  a nonexistent service.
- api: Trim leading slashes from keys/prefixes when querying KV
  endpoints to avoid a bug with redirects in Go 1.7 (golang/go#4800).
- dns: Fixed external services that pointed to consul addresses (CNAME
  records) not resolving to A-records.
- dns: Fixed an issue with SRV lookups where the service address was
  different from the node's.
- dns: Fixed an issue where truncated records from a recursor query were
  improperly reported as errors.
- server: Fixed the port numbers in the sample JSON inside peers.info.
- server: Squashes ACL datacenter name to lower case and checks for
  proper formatting at startup.
- ui: Fixed an XSS issue with the display of sessions and ACLs in the
  web UI.

delicb added a commit to delicb/gwc that referenced this issue Jul 1, 2017

Remove copy of headers on redirect.
Since original issue was fixed with GoLang release 1.8
(golang/go#4800), GWC will no longer fix
broken behaviour from earlier versions of GoLang.

If needed, users of GWC might implement similar solution on their own
and provide custom instance of http.Client to GWC upon creation.

@golang golang locked and limited conversation to collaborators Oct 18, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.