Skip to content
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

Fixed cookies parsing #133

Merged
merged 1 commit into from Dec 28, 2011

Conversation

Projects
None yet
4 participants
@afanasy
Copy link
Contributor

commented Dec 12, 2011

  • correct handling of cookies values with equal sign (sid="c2lkDQo=")
  • improved default path parsing
  • updated tests
@mikeal

This comment has been minimized.

Copy link
Member

commented Dec 12, 2011

i'd like some eyes on this who've been in this code, I haven't spent enough time in in it.

@jhurliman @janjongboom @alessioalex

@afanasy

This comment has been minimized.

Copy link
Owner Author

commented on vendor/cookie/index.js in 4882e51 Dec 13, 2011

for cookies like
sid =12aazfffaaef
with space after cookie name

This comment has been minimized.

Copy link

replied Dec 13, 2011

Have you ever encountered such a cookie? Because this is clearly off spec, don't know how we should handle this.

This comment has been minimized.

Copy link
Owner Author

replied Dec 13, 2011

No, I've put that for consistency with split(/ *= */) below (and replaced it with trim() later)

@afanasy

This comment has been minimized.

Copy link
Owner Author

commented on vendor/cookie/index.js in 4882e51 Dec 13, 2011

split() truncates everything after = sign
sid="c2lkDQo=" becomes sid="c2lkDQo=

@afanasy

This comment has been minimized.

Copy link
Owner Author

commented on vendor/cookie/index.js in 4882e51 Dec 13, 2011

toLowerCase() makes tests/test-cookie.js httpOnly test fail
|| true sets cookies with empty string value to true
sid= becomes sid=true

@afanasy

This comment has been minimized.

@afanasy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2011

I've added comments

@pinakin-ezee

This comment has been minimized.

Copy link

commented Dec 20, 2011

Hi.

I'm not sure if this is related but the new version of request does not appear to handle cookies correctly. Or rather, not as well as it used to. After spending a morning trying to work out why some previously working code had stopped functioning I narrowed it down to the header information sent and saw that the cookie I was sending appeared to be undefined. I am now using an old version of request (not sure which version it is but I installed it 18/11/2011) and that works correctly.

var sessionId = "EFFCBC5DF5D12EEF44BFECB81B58C221.86922E30DDA32934988F2834DFC92B";
var cookieStr = "sessionID=" + sessionId;

var headerData = {"Accept":"text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8",
                  "Accept-Language":"en-us",
                  "Origin":"http://www.xxxxxxxx.com",
                  "Referer":"http://www.xxxxxxxx.com/home",
                  "User-Agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/534.51.22 (KHTML, like Gecko) Version/5.1.1 Safari/534.51.22",
                  "Cookie":cookieStr};

var options = {uri:headingUri, headers:headerData};

request.get(options, function (error, response, body) {
@mikeal

This comment has been minimized.

Copy link
Member

commented Dec 20, 2011

latest release or master?

@afanasy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2011

So what's up with this request? Its kind of fixes a major bug.

@pinakin-ezee

This comment has been minimized.

Copy link

commented Dec 21, 2011

The latest release (the version one gets with "npm install request"). The weird thing is I tested this version with a simple php file that reads header data and all appeared fine, including cookies. It's just that the response coming back for the code mentioned above indicates that I am send an "undefined" sessionid (a cookie) but with the older version of request the response shows the correct sessionid sent.

mikeal added a commit that referenced this pull request Dec 28, 2011

@mikeal mikeal merged commit faf36fc into request:master Dec 28, 2011

@mikeal

This comment has been minimized.

Copy link
Member

commented Dec 28, 2011

pushed new version with this fix in it.

@afanasy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2011

great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.