Fix cookie jar/headers.cookie collision (#125) #161

Merged
merged 4 commits into from Feb 9, 2012

Projects

None yet

3 participants

@papandreou
Contributor

No description provided.

@papandreou
Contributor

Oh, I didn't realize there was an existing pull request (#139), sorry.

My fix suffered from the same redirect-related problem that mikeal pointed out, but I've just added two more commits that test and fix that by saving the original value of headers.cookie before any redirects have taken place.

The test part of it (20a00de) piggybacks on the existing redirect test, which might not be desired. If not, it can be left out.

@mikeal
Member
mikeal commented Jan 23, 2012

The cookie code is the code I'm least familiar with in request. Could I get some review help here from @afanasy @jhurliman @janjongboom @alessioalex

@afanasy
Contributor
afanasy commented Jan 23, 2012

dear @papandreou

#139 pull request has


if (self.headers && !self.headers.Cookie && self.headers.cookie) {
 self.headers.Cookie = self.headers.cookie; // be forgiving
}

and then uses uppercase Cookie, and this commit has mixed uppercase .Cookie and lowercase .cookie, may be it's a good idea to add this code and use .Cookie on the lines 169, 171 in main.js?

Also I'm not sure about semicolons-less formatting, but it looks cool :)

@papandreou
Contributor

@afanasy Per the conventions laid forth by node's http module, the lower-cased header names are canonical, so if anything we should normalize it as self.headers.cookie. Personally I wouldn't try to be forgiving on a case-by-case basis, because it's cumbersome (and very easy to forget) to check for the upper-case version each time.

Maybe there could be a single piece of code that normalized the headers once and for all, something like:

Object.keys(self.headers).forEach(function (headerName) {
    var headerNameLowerCased = headerName.toLowerCase()
    if (headerName !== headerNameLowerCased) {
        if (self.headers[headerNameLowerCased]) {
            console.error('You've specified both headers.' + headerNameLowerCased + ' and headers.' + headerName + ', ignoring ' + headerName)
        } else {
            console.warn('Please specify header name as lower case: ' + headerName)
            self.headers[headerNameLowerCased] = self.headers[headerName]
        }
        delete self.headers[headerName]
    }
})

Btw. I absolutely loathe semicolon-less formatting, I'm just trying to adhere to the existing coding style :)

@mikeal
Member
mikeal commented Jan 23, 2012

the coding style for request is more or less taken from npm :)

http://npmjs.org/doc/coding-style.html

@papandreou
Contributor

@mikeal: I know, it has kept me from contributing to npm. I'm a big fan of isaacs, but this particular battle was ill chosen :)

@mikeal
Member
mikeal commented Jan 23, 2012

@papandreou i actually decided to adopt the style after doing a lot of speaking for a year. I started to get really critical of my code samples in talks and when i would write about things.

As I continued to strip out "noise" I realized that semicolons add a lot of noise, and it's the worst kind of noise, it's noise that you figure out quickly how to ignore. It means that anything along the right side can be lost, it's mostly ignored, because you get used to ignoring the end of the line.

When I started stripping semicolons, all the code samples were clearer, and I decided it was a much better style and adopted it more widely.

@papandreou
Contributor

@mikeal Didn't work that way for me, although in all fairness that could be because my editor (emacs + js2-mode) underlines every line it thinks is missing a semicolon. But even if you're right I doubt the slightly higher S/N ratio is worth alienating 90% of the community from your code base :)

@mikeal
Member
mikeal commented Jan 24, 2012

request and npm have some of the largest contributor lists of node modules, so it doesn't seem to be too alienating :)

@afanasy
Contributor
afanasy commented Jan 24, 2012

ok, so lets use socket.headers.cookie
checked tests and they worked fine for me

@mikeal
Member
mikeal commented Feb 8, 2012

this no longer merges cleanly :(

papandreou added some commits Jan 22, 2012
@papandreou papandreou Added failing test for #125. a0ab977
@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
@papandreou papandreou Redirect test: Also assert that the request cookie doesn't get double…
…d in the request for the landing page.
1ac9e2d
@papandreou papandreou Cookie jar handling: Don't double the cookies on each redirect (see d…
…iscussion on #139).
c640eed
@papandreou
Contributor

@mikeal: Try now, I rebased and fixed the conflict.

@mikeal mikeal merged commit dcb6eea into request:master Feb 9, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment