Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

headers.cookie shouldn't be replaced #125

Closed
papandreou opened this Issue Dec 5, 2011 · 5 comments

Comments

Projects
None yet
5 participants
Contributor

papandreou commented Dec 5, 2011

I had some old code that pre-dates request's cookie jar support:

request({
    headers: {
        cookie: "foo=bar"
    },
    // ...
});

However, after I updated request it stopped working because now headers.cookie is always overwritten unless jar: false is specified:

if (self.jar === false) {
  // disable cookies
  var cookies = false;
  self._disableCookies = true;
} else if (self.jar) {
  // fetch cookie from the user defined cookie jar
  var cookies = self.jar.get({ url: self.uri.href })
} else {
  // fetch cookie from the global cookie jar
  var cookies = cookieJar.get({ url: self.uri.href })
}
if (cookies) {
    var cookieString = cookies.map(function (c) {
        return c.name + "=" + c.value;
    }).join("; ");

    self.headers.Cookie = cookieString;
}

The fix was trivial, but I'm reporting it anyway because I think it's a wtf. In my opinion there should be a check for whether self.headers.cookie already exists, and if so, append to it. Or at least it shouldn't do anything if the global cookie jar is empty.

My guess is that the current behavior breaks a bunch of proxying use cases where cookies are involved.

I can whip up a pull request, but it would be nice to hear if everyone else is on board with this?

sujal commented Dec 19, 2011

+1

I just got burned by this as well, was very confusing

dscape commented Jan 21, 2012

This was a problem for nano, reported by dale

@papandreou papandreou added a commit to papandreou/request that referenced this issue Feb 8, 2012

@papandreou papandreou Added failing test for #125. a0ab977

@papandreou papandreou added a commit to papandreou/request that referenced this issue Feb 8, 2012

@papandreou papandreou Fix cookie jar/headers.cookie collision. Closes #125.
If both a cookie jar and headers.cookie are specified, include both in the request, separated by '; '. Also fixed uppercase header name: self.headers.Cookie => self.headers.cookie.
c80800a
Owner

mikeal commented Feb 8, 2012

did this get merged? can we close?

@mikeal mikeal added a commit that referenced this issue Feb 9, 2012

@mikeal mikeal Merge pull request #161 from papandreou/master
Fix cookie jar/headers.cookie collision (#125)
dcb6eea
Owner

mikeal commented Feb 18, 2012

fixed.

@mikeal mikeal closed this Feb 18, 2012

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