Bug 189920 – [Curl] Fix priority issue with multiple cookies with different level of path.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

When multiple cookies are stored in the database for same site, the priority of multiple cookies which has matching path was not defined.

i.e)
cookie A: path = /
cookie B: path = /foo
cookie C: path = /buz
cookie D: path = /foo/bar

When accessing to /foo/bar/hello.html, cookie D should be return from the candidate of A, B and D.

[Basuke Suzuki]

Created attachment 350674 [details]
PATCH

[EWS Watchlist]

Comment on attachment 350674 [details]
PATCH

Attachment 350674 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9335035

New failing tests:
http/tests/cookies/cookie-with-path-multiple-level.html

[EWS Watchlist]

Created attachment 350680 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350674 [details]
PATCH

Attachment 350674 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9335347

New failing tests:
http/tests/cookies/cookie-with-path-multiple-level.html

[EWS Watchlist]

Created attachment 350694 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350674 [details]
PATCH

Attachment 350674 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9336254

New failing tests:
http/tests/cookies/cookie-with-path-multiple-level.html

[EWS Watchlist]

Created attachment 350702 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 350674 [details]
PATCH

Attachment 350674 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9335832

New failing tests:
http/tests/cookies/cookie-with-path-multiple-level.html

[EWS Watchlist]

Created attachment 350705 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Basuke Suzuki]

Based on https://tools.ietf.org/html/rfc6265#section-5.4:

> 2.  The user agent SHOULD sort the cookie-list in the following
>       order:
>
>       *  Cookies with longer paths are listed before cookies with
>          shorter paths.
>
>       *  Among cookies that have equal-length path fields, cookies with
>          earlier creation-times are listed before cookies with later
>          creation-times.
>
>       NOTE: Not all user agents sort the cookie-list in this order, but
>       this order reflects common practice when this document was
>       written, and, historically, there have been servers that
>       (erroneously) depended on this order.

[EWS Watchlist]

Comment on attachment 350674 [details]
PATCH

Attachment 350674 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9337179

New failing tests:
http/tests/cookies/cookie-with-path-multiple-level.html

[EWS Watchlist]

Created attachment 350719 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Basuke Suzuki]

Created attachment 350773 [details]
PATCH

Fix cookie path.

[Alex Christensen]

Comment on attachment 350773 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=350773&action=review

> LayoutTests/ChangeLog:10
> +        * http/tests/cookies/resources/cookie-with-multiple-level/foo/get.php: Added.

We already have many copies of echo-cookies.php, get-cookies.php, etc.

> LayoutTests/ChangeLog:11
> +        * http/tests/cookies/resources/cookie-with-multiple-level/foo/set.php: Added.

We already have many copies of set-cookie.php or set-a-cookie.php.

> LayoutTests/http/tests/cookies/cookie-with-path-multiple-level.html:9
> +                This tests flakiness of cookie query of Curl port.

This test is not specific to Curl.

[Basuke Suzuki]

Comment on attachment 350773 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=350773&action=review

>> LayoutTests/ChangeLog:10
>> +        * http/tests/cookies/resources/cookie-with-multiple-level/foo/get.php: Added.
> 
> We already have many copies of echo-cookies.php, get-cookies.php, etc.

It is required to be in that path. It returns the cookie which sent to that path.

>> LayoutTests/ChangeLog:11
>> +        * http/tests/cookies/resources/cookie-with-multiple-level/foo/set.php: Added.
> 
> We already have many copies of set-cookie.php or set-a-cookie.php.

ditto.

>> LayoutTests/http/tests/cookies/cookie-with-path-multiple-level.html:9
>> +                This tests flakiness of cookie query of Curl port.
> 
> This test is not specific to Curl.

Okay.

[Fujii Hironori]

Comment on attachment 350773 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=350773&action=review

> LayoutTests/ChangeLog:9
> +        * http/tests/cookies/cookie-with-path-multiple-level.html: Added.

This change has only a test case of path lenght of single cookie.
Accoring to the spec, multiple cookies should be sorted in longer path and older creation-time order.
https://tools.ietf.org/html/rfc6265#section-5.4
Can you create a test case to check them?
name1=val1; name2=val2; name3=val3

>>> LayoutTests/ChangeLog:11
>>> +        * http/tests/cookies/resources/cookie-with-multiple-level/foo/set.php: Added.
>> 
>> We already have many copies of set-cookie.php or set-a-cookie.php.
> 
> ditto.

I think set.php doesn't need to placed in the nested directory.
LayoutTests/http/tests/cookies/resources/setCookies.cgi
can be used to set a arbitrary cookeis. I think you can use this with " path=/...".

>>> LayoutTests/http/tests/cookies/cookie-with-path-multiple-level.html:9
>>> +                This tests flakiness of cookie query of Curl port.
>> 
>> This test is not specific to Curl.
> 
> Okay.

I think you should follow the style of other HTML files in this directory.
I think 'description' should be placed immediately after <script>.

[Basuke Suzuki]

Created attachment 351024 [details]
PATCH

[Basuke Suzuki]

- Added created date test.
- Use setCookies.cgi

[EWS Watchlist]

Comment on attachment 351024 [details]
PATCH

Attachment 351024 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9374977

New failing tests:
http/tests/cookies/cookie-with-multiple-level-path.html

[EWS Watchlist]

Created attachment 351033 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351024 [details]
PATCH

Attachment 351024 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9375173

New failing tests:
http/tests/cookies/cookie-with-multiple-level-path.html

[EWS Watchlist]

Created attachment 351037 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351024 [details]
PATCH

Attachment 351024 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9374954

New failing tests:
http/tests/cookies/cookie-with-multiple-level-path.html

[EWS Watchlist]

Created attachment 351039 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Fujii Hironori]

You greatly improved the test case. Thank you very much for working on it. 

Some Mac port EWS bots are red.

> FAIL () => result should be foo=buz; bingo=bongo; foo=bar; secret=42. Was bingo=bongo; foo=buz; secret=42; foo=bar.

It seems that it sorts cookies in newer order.

Could you add a comment about other major browsers results?

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #25)
> You greatly improved the test case. Thank you very much for working on it. 
> 
> Some Mac port EWS bots are red.
> 
> > FAIL () => result should be foo=buz; bingo=bongo; foo=bar; secret=42. Was bingo=bongo; foo=buz; secret=42; foo=bar.
> 
> It seems that it sorts cookies in newer order.
> 
> Could you add a comment about other major browsers results?

Chrome PASS. I'll try on Firefox.

But as RFC mentions, some browsers are not following that behavior and that is acceptable because the order of different cookie name is not critical. It differs from multi level path issues that even if the order of sort result caused by creation date is deffer, the parsed result in the context is almost always same in practical.

In reality, I don't think we need this test.

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #26)
> But as RFC mentions, some browsers are not following that behavior and that
> is acceptable because the order of different cookie name is not critical. It
> differs from multi level path issues that even if the order of sort result
> caused by creation date is deffer, the parsed result in the context is
> almost always same in practical.
> 
> In reality, I don't think we need this test.

Do you mean you think test cases of both sort order conditions (path lenght and creation-time) are usesless?
Or, creation-time test is usesless?

If the latter case, I can agree on you.
If the former case, I don't understand you. Could you elaborate?

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #27)
> (In reply to Basuke Suzuki from comment #26)
> > But as RFC mentions, some browsers are not following that behavior and that
> > is acceptable because the order of different cookie name is not critical. It
> > differs from multi level path issues that even if the order of sort result
> > caused by creation date is deffer, the parsed result in the context is
> > almost always same in practical.
> > 
> > In reality, I don't think we need this test.
> 
> Do you mean you think test cases of both sort order conditions (path lenght
> and creation-time) are usesless?
> Or, creation-time test is usesless?
> 
> If the latter case, I can agree on you.
> If the former case, I don't understand you. Could you elaborate?

Of course latter. The path length sort order is the main issue this patch fixes on curl port.

[EWS Watchlist]

Comment on attachment 351024 [details]
PATCH

Attachment 351024 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9376985

New failing tests:
http/tests/cookies/cookie-with-multiple-level-path.html

[EWS Watchlist]

Created attachment 351054 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Basuke Suzuki]

Created attachment 351055 [details]
PATCH

Remove timestamp tests

[WebKit Commit Bot]

Comment on attachment 351055 [details]
PATCH

Clearing flags on attachment: 351055

Committed r236593: <https://trac.webkit.org/changeset/236593>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44856431>