Cookie handling contains bugs #104

Merged
merged 5 commits into from Nov 30, 2011

Projects

None yet

4 participants

@janjongboom
Contributor

@mikeal See my inline comments on this pull request.

@janjongboom janjongboom commented on the diff Nov 15, 2011
@@ -151,7 +151,13 @@ Request.prototype.request = function () {
// fetch cookie from the global cookie jar
var cookies = cookieJar.get({ url: self.uri.href })
}
- if (cookies) {self.headers.Cookie = cookies}
+ if (cookies) {
@janjongboom
janjongboom Nov 15, 2011 Contributor

Cookies has to be a string, this way an array is put into cookies; resulting in multiple 'Cookie' headers. This isn't on spec.

@janjongboom janjongboom commented on the diff Nov 15, 2011
@@ -422,7 +428,7 @@ Request.prototype.request = function () {
response.body = JSON.parse(response.body)
} catch (e) {}
}
- if (response.statusCode == 200 && response.headers['set-cookie'] && (!self._disableCookies)) {
@janjongboom
janjongboom Nov 15, 2011 Contributor

Every response can set cookies, not just 200 OK ones. For example after authenticating on GitHub the 302 redirect also sets your auth cookie.

@indexzero

+1 Seeing this error:

{ stack: 
   [ 'TypeError: Cannot read property \'url\' of undefined',
     '    at new Cookie (/some-project/node_modules/request/vendor/cookie/index.js:45:20)',
     '    at /some-project/node_modules/request/main.js:432:33',
     '    at Array.forEach (native)',
     '    at Request.<anonymous> (/some-project/node_modules/request/main.js:426:46)',
     '    at Request.emit (events.js:64:17)',
     '    at IncomingMessage.<anonymous> (/some-project/node_modules/request/main.js:391:16)',
     '    at IncomingMessage.emit (events.js:81:20)',
     '    at HTTPParser.onMessageComplete (http.js:133:23)',
     '    at Socket.ondata (http.js:1231:22)',
     '    at Socket._onReadable (net.js:677:27)' ] }
@mikeal
Member
mikeal commented Nov 29, 2011

what's the status on this pull request? there are some comments which I don't believe are resolved and there is also a change that has already been merged which i believe addresses some of the problems.

@janjongboom
Contributor

This pull request is still active. The other pull request about cookies was about the way it handled the Path header. This pull request covers:

  • Sending cookies from the jar in the correct syntax
  • Storing cookies that are set by requests with other response codes than 200
@mikeal
Member
mikeal commented Nov 30, 2011

sorry, i mistook the inline comments as unresolved issues, didn't realize at first that you were using them to describe the changes :)

@mikeal mikeal merged commit 5b8359c into request:master Nov 30, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment