Updated: Remove 'Content Type' and 'Content Length' from headers on redirect when previous HTTP method was either a POST or PUT #302

Merged
merged 1 commit into from Apr 25, 2012

Projects

None yet

4 participants

@thestoics
Contributor

Removes extra commit

This pull request ensures that we delete the the 'Content Length' and 'Content Type' from the headers on a redirect when the previous (request) HTTP method was either a POST or a PUT.

Rationale:

  • The redirect is always a GET which means form content types (application/x-www-form-url-encoded, multipart) are invalid
  • This was causing Google's OAuth authentication to fail after the user had been authenticated and redirected to the authorization page

Potential pitfalls

  • I couldn't find test suite that tested redirects -- I'm inclined to delegate this for now
  • The headers object is being modified and passed to other methods. Optimally, we should treat this like an immutable data structure and copy then modify
  • My placement of the deletion may not be necessary in the nextTick callback
@adrienbrault

I had a redirect that failed (i had the following content <html><body></body></html>), and this patch fixed the bug.

I think this also fixes #298

@adrienbrault

I'm in a case where a host sends me a redirect to another host. Zombie sends the cookies from the first host to the second, and i get unexpected things.

I had to solve it by adding this in the process.nextTick => function

if redirect.host != url.host
  delete headers['cookie']

Maybe too much things are passed through when a redirect occurs.

@thestoics
Contributor

Indeed,

It may be wiser to rebuild the headers given a certain set of rules (which should be well defined). Passing the same headers when a 302 is encountered is not wise.

@adrienbrault

What about passing null to the headers parameters when a redirect occurs ? Headers will be created as if this is a new request.

@_makeRequest "GET", redirect, null, null, resource, callback
@assaf
Owner
assaf commented Apr 6, 2012

Any ideas which headers should pass after redirect?

Clearly cookies need to be reconstructed, there's no document to pass. It is just Accept, User-Agent and Referer? How would we find out?

@thestoics
Contributor

This probably merits some investigation with regard to common practice on redirects. There may be an established standard or convention.

@assaf assaf merged commit d3f6e6c into assaf:master Apr 25, 2012
@jkingyens

Did this fix get lost somewhere? Its not in master branch anymore and I am hitting the bug caused by this.

@assaf
Owner
assaf commented May 27, 2012

Resource loading got rewritten twice since that commit. There was no test for it, so naturally it got lost.

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