Adopt IETF's test suite, fix spec and browser compliance errors #30

inikulin · 2015-03-12T12:58:50Z

Hello!

We've decided to adopt tough-cookie for the jsdom (jsdom/jsdom#1027). Therefore I've adopted IETF's test suite and fixed issues that arose.

1)Doubles quotes should be a part of the value (RFC6265 S4.1.1) 2)"Relaxed" mode should use \r \n\ and \0 as terminators according to Chromium source (https://chromium.googlesource.com/chromium/src.git/+/master/net/cookies/parsed_cookie.cc)

…0029 test)

… correct way). According to S5.2.4 we should fallback to the default-path only if the path attribute-value is empty or if the first character is not "/". Meanwhile, then we are query cookies from the store we should use path-match algorithm from S5.1.4 instead of path permutation based algorithm.

… correct order for the cookies which are set synchronously. So we are using high-resolution time now where possible.

…in browsers (see: https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/parsed_cookie.cc#L64)

…values is '/' we should fallback to the default-path. Previously code doesn't considered the case then we have two path attributes. Now we always assign null explicitly if the path attribute is empty or '/' (Fixes PATH0030 IETF test)

… meanwhile previously used regexp doesn't allowed years that starts with zero (e.g. 07). Also, it allowed 3-digits year, which is incorrect

According to RFC6265: time = hms-time ( non-digit *OCTET ) hms-time = time-field ":" time-field ":" time-field time-field = 1*2DIGIT Previous regexp doesn't allowed 1 digit minute/second time fields.

According to RFC6265 S5.1.1: month = ( "jan" / "feb" / "mar" / "apr" / "may" / "jun" / "jul" / "aug" / "sep" / "oct" / "nov" / "dec" ) *OCTET So, spec allows suffix for the month.

Use the exact grammar productions from the RFC for the time and day-of-month. Then perform correct validation (e.g. we should fail on "Sat, 15-Apr-17 91:22:33 21:01:22" because first met time token was incorrect. Previous approach just skipped first token, which is incorrect according to the RFC and doesn't matches browser's behavior (see: https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/cookie_util.cc#L189)

domenic · 2015-03-18T15:33:51Z

+1, would love to be able to have jsdom depend on something with a cute cookie-turtle logo.

apsavin · 2015-03-23T19:09:40Z

Where is somebody who can merge it?)

JSBizon · 2015-03-23T21:03:51Z

looks this important repository isn't supported anymore, maybe make sense create a fork? I did one for myself: https://github.com/JSBizon/tough-cookie

inikulin · 2015-03-23T21:16:16Z

This is a plan B =)

JSBizon · 2015-03-23T22:03:36Z

right, but owner of this repo is silent for a long time

domenic · 2015-03-23T22:46:29Z

Someone should email https://www.npmjs.com/~goinstant and https://www.npmjs.com/~jstash to see if they'd be willing to transfer the npm package ownership at least

stash · 2015-03-30T22:31:39Z

Hey folks. I'm so, so sorry to have taken so long to respond. Long story short (I can explain if you want) is that I lost control of this repo for legal reasons, but now I can maintain it again!

I will look over the PR. Salesforce is currently requiring that contributors sign a CLA. If the PR looks good, I will need to get one from @inikulin and then I can publish to NPM.

stash · 2015-03-30T22:42:24Z

lib/cookie.js

-var STRICT_TIME = /^(0?[0-9]|1[0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9])$/;
+var DAY_OF_MONTH = /^(\d{1,2})[^\d]*$/;
+var TIME = /^(\d{1,2})[^\d]*:(\d{1,2})[^\d]*:(\d{1,2})[^\d]*$/;
+var MONTH = /^(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)/i;


Is the removal of the $ anchor here intentional?

You mean in month regexp? If so, yes, according to RFC:

month = ( "jan" / "feb" / "mar" / "apr" /
"may" / "jun" / "jul" / "aug" /
"sep" / "oct" / "nov" / "dec" ) *OCTET

*OCTET - means that we can have any suffix. So, thing like January and even JanWhatever are acceptable. In Chromium source: https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/cookie_util.cc#L125

stash · 2015-03-30T22:58:42Z

lib/memstore.js

-      });
-    };
+       //NOTE: we should use path-match algorithm from S5.1.4 here
+       //(see : https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/canonical_cookie.cc#L299)


Could you please explain here why we should use that path-match implementation?

Tactical: if this is a TODO, then maybe that goes as a comment on pathMatch? Or are you saying that matchRFC doesn't sync up with how Chromium does it?

This is according to the S5.4.1:

The user agent MUST use an algorithm equivalent to the following
algorithm to compute the "cookie-string" from a cookie store and a
request-uri:
....

The request-uri's path path-matches the cookie's path.

BTW, seems like we can remove other matcher. I'll try it and will add another commit in case if it works.

OK. A separate PR sounds good just to keep this big one reasonable.

stash · 2015-03-30T23:01:30Z

@inikulin I'm in the middle of looking at the changes and, overall, I'm impressed. I'm confident we can work through the minor issues. Most importantly: THANK YOU for all of the hard work! This is great!

stash · 2015-03-30T23:12:31Z

test/cookie_sorting_test.js

+        // some may have been created at now + 1ms.
+        var i = cookies.length;
+        cookies.forEach(function (cookie) {
+          cookie.creation = new Date(now - 100 * (i--));


If .hrCreation gets left in, then this doesn't do anything since .hrCreation takes precedence over .creation. The end result is coincidentally the same, making this forEach loop redundant.

Not saying it should be left in; we should still discuss it (and probably have some unit tests that cover it).

Previous algorithm failed at the last day of the current month. This was caused by the fact that setUTCMonth method was used. According to MDN(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/setUTCMonth) it accepts two parameters. If second parameter (dayValue) is not specified then V8 uses current day value. So if you are setting April as month, and current system date is March 31 then if will be resolved as May 1 which doesn't works for us. Therefore, we will parse tokens separately and then create new Date() object using Date.UTC()

inikulin · 2015-03-31T13:34:46Z

Weird, BSD date tests starts to fail after March 30th (https://travis-ci.org/inikulin/tough-cookie/jobs/56567365). Need to take a look at it. (Upd: Fixed via inikulin/tough-cookie@553d20b)

Previous algorithm failed at the last day of the current month. This was caused by the fact that setUTCMonth method was used. According to MDN(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/setUTCMonth) it accepts two parameters. If second parameter (dayValue) is not specified then V8 uses current day value. So if you are setting April as month, and current system date is March 31 then if will be resolved as May 1 which doesn't works for us. Therefore, we will parse tokens separately and then create new Date() object using Date.UTC()

inikulin · 2015-03-31T14:50:53Z

Ok, now we need to decide what to do with the hrCreation

Use runtime creation index instead of hrTime. Track cookie.creation modifications by storing original UTC timestamp and comparing it with the actual cookie.creation.getTime(). If creation date was modified we fallback to the plane dates comparison.

inikulin · 2015-03-31T16:48:09Z

Ok, I think some tests required for the latest commit to be sure that we are not breaking cookieCompare if cookie.creation was modified by the user. But, I need to go now, so I'll add them tomorrow.

…n (fallback from runtimeIdx)

stash · 2015-04-01T22:58:39Z

There's another issue open for the date thing. I have a fix waiting in the wings that I'll merge in after this one.

stash · 2015-04-01T23:07:23Z

re-reviewed; looking great still.

domenic · 2015-04-15T12:49:12Z

Anything left before this can be merged?

@inikulin

Adopt IETF's test suite, fix spec and browser compliance errors Amazing thanks to @inikulin !

stash · 2015-04-17T02:04:51Z

I will cut a release for this tomorrow. I'm still sorting out NPM ownership so please bear with me.

stash · 2015-04-20T22:38:06Z

Still working on this, stay tuned.

stash · 2015-04-22T01:27:43Z

Published as v0.13.0

inikulin added 15 commits February 25, 2015 17:29

Refactor tests

b8aa8e8

Add IETF's http state tests

1af799c

Fix IETF's value parsing tests:

a0ea9d5

1)Doubles quotes should be a part of the value (RFC6265 S4.1.1) 2)"Relaxed" mode should use \r \n\ and \0 as terminators according to Chromium source (https://chromium.googlesource.com/chromium/src.git/+/master/net/cookies/parsed_cookie.cc)

Fix previously incorrect value parsing test

b4fbe73

Fix path tests and add url decoding of the context url (DISABLED_PATH…

7b3bbf2

…0029 test)

Fix request URL in IETF tests, update test data

d0fbac6

Fix ordering tests: JS new Date() precision was not enough to provide…

d8a02c1

… correct order for the cookies which are set synchronously. So we are using high-resolution time now where possible.

Allow whitespaces in attribute (aka token) in "relaxed" mode as done …

4be6807

…in browsers (see: https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/parsed_cookie.cc#L64)

Fix date parsing: from RFC6265: year = 2*4DIGIT ( non-digit *OCTET ),…

8c65a0b

… meanwhile previously used regexp doesn't allowed years that starts with zero (e.g. 07). Also, it allowed 3-digits year, which is incorrect

Fix date parsing:

d18d6cd

According to RFC6265: time = hms-time ( non-digit *OCTET ) hms-time = time-field ":" time-field ":" time-field time-field = 1*2DIGIT Previous regexp doesn't allowed 1 digit minute/second time fields.

Fix date parsing:

e58487f

According to RFC6265 S5.1.1: month = ( "jan" / "feb" / "mar" / "apr" / "may" / "jun" / "jul" / "aug" / "sep" / "oct" / "nov" / "dec" ) *OCTET So, spec allows suffix for the month.

Return original build status badge

ecc093b

inikulin mentioned this pull request Mar 12, 2015

Expired cookie is still present in document.cookie jsdom/jsdom#1027

Closed

inikulin added 2 commits March 20, 2015 16:52

Fix cookie order in getCookieString to be consistent with the browsers

0ecd088

Merge branch 'master' of https://github.com/inikulin/tough-cookie

f9015c7

stash self-assigned this Mar 30, 2015

stash reviewed Mar 30, 2015
View reviewed changes

inikulin added 2 commits March 31, 2015 16:31

Remove duplicate assignment, get rid of the unnecessary path matcher

3b11467

inikulin closed this Mar 31, 2015

inikulin reopened this Mar 31, 2015

inikulin added 2 commits March 31, 2015 19:41

Fix comment typos

9408b95

Add test for the cookieCompare and modification of the cookie.creatio…

55b1ffb

…n (fallback from runtimeIdx)

stash added a commit that referenced this pull request Apr 17, 2015

Merge pull request #30 from inikulin/master

16625cb

Adopt IETF's test suite, fix spec and browser compliance errors Amazing thanks to @inikulin !

stash merged commit 16625cb into salesforce:master Apr 17, 2015

inikulin unassigned stash Apr 23, 2015

This was referenced Apr 24, 2015

Incorrect cookie date handling (expires dates) - maybe? #19

Closed

Do a 1.0.0 release and start following semver #31

Closed

Bug inside the RFC6265 S5.1.1 date parser parseDate() on days 29, 30, 31 #28

Closed

lalitkapoor mentioned this pull request Sep 17, 2015

Incorrect cookie date handling with CookieJar (mangled expires dates) request/request#948

Closed

wjhsf pushed a commit that referenced this pull request Feb 8, 2024

remove custom namespace logic in resolver (#30)

cd38feb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt IETF's test suite, fix spec and browser compliance errors #30

Adopt IETF's test suite, fix spec and browser compliance errors #30

inikulin commented Mar 12, 2015

domenic commented Mar 18, 2015

apsavin commented Mar 23, 2015

JSBizon commented Mar 23, 2015

inikulin commented Mar 23, 2015

JSBizon commented Mar 23, 2015

domenic commented Mar 23, 2015

stash commented Mar 30, 2015

stash Mar 30, 2015

inikulin Mar 31, 2015

stash Mar 30, 2015

inikulin Mar 31, 2015

stash Apr 1, 2015

stash commented Mar 30, 2015

stash Mar 30, 2015

stash Mar 30, 2015

inikulin commented Mar 31, 2015

inikulin commented Mar 31, 2015

inikulin commented Mar 31, 2015

stash commented Apr 1, 2015

stash commented Apr 1, 2015

domenic commented Apr 15, 2015

stash commented Apr 17, 2015

stash commented Apr 20, 2015

stash commented Apr 22, 2015

Adopt IETF's test suite, fix spec and browser compliance errors #30

Adopt IETF's test suite, fix spec and browser compliance errors #30

Conversation

inikulin commented Mar 12, 2015

domenic commented Mar 18, 2015

apsavin commented Mar 23, 2015

JSBizon commented Mar 23, 2015

inikulin commented Mar 23, 2015

JSBizon commented Mar 23, 2015

domenic commented Mar 23, 2015

stash commented Mar 30, 2015

stash Mar 30, 2015

Choose a reason for hiding this comment

inikulin Mar 31, 2015

Choose a reason for hiding this comment

stash Mar 30, 2015

Choose a reason for hiding this comment

inikulin Mar 31, 2015

Choose a reason for hiding this comment

stash Apr 1, 2015

Choose a reason for hiding this comment

stash commented Mar 30, 2015

stash Mar 30, 2015

Choose a reason for hiding this comment

stash Mar 30, 2015

Choose a reason for hiding this comment

inikulin commented Mar 31, 2015

inikulin commented Mar 31, 2015

inikulin commented Mar 31, 2015

stash commented Apr 1, 2015

stash commented Apr 1, 2015

domenic commented Apr 15, 2015

stash commented Apr 17, 2015

stash commented Apr 20, 2015

stash commented Apr 22, 2015