Bug 189920 - [Curl] Fix priority issue with multiple cookies with different level of path.
Summary: [Curl] Fix priority issue with multiple cookies with different level of path.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-24 11:12 PDT by Basuke Suzuki
Modified: 2018-09-27 23:56 PDT (History)
10 users (show)

See Also:


Attachments
PATCH (6.84 KB, patch)
2018-09-24 13:19 PDT, Basuke Suzuki
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.49 MB, application/zip)
2018-09-24 14:06 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.30 MB, application/zip)
2018-09-24 14:50 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews200 for win-future (12.89 MB, application/zip)
2018-09-24 15:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.17 MB, application/zip)
2018-09-24 15:50 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.82 MB, application/zip)
2018-09-24 17:38 PDT, EWS Watchlist
no flags Details
PATCH (6.84 KB, patch)
2018-09-25 11:44 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
PATCH (6.84 KB, patch)
2018-09-27 17:08 PDT, Basuke Suzuki
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.33 MB, application/zip)
2018-09-27 18:27 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.40 MB, application/zip)
2018-09-27 18:59 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.19 MB, application/zip)
2018-09-27 19:06 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews201 for win-future (13.18 MB, application/zip)
2018-09-27 21:59 PDT, EWS Watchlist
no flags Details
PATCH (6.46 KB, patch)
2018-09-27 22:05 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2018-09-24 11:12:13 PDT
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.
Comment 1 Basuke Suzuki 2018-09-24 13:19:59 PDT
Created attachment 350674 [details]
PATCH
Comment 2 EWS Watchlist 2018-09-24 14:06:48 PDT
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
Comment 3 EWS Watchlist 2018-09-24 14:06:50 PDT
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
Comment 4 EWS Watchlist 2018-09-24 14:50:07 PDT
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
Comment 5 EWS Watchlist 2018-09-24 14:50:09 PDT
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
Comment 6 EWS Watchlist 2018-09-24 15:40:36 PDT
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
Comment 7 EWS Watchlist 2018-09-24 15:40:48 PDT
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
Comment 8 EWS Watchlist 2018-09-24 15:50:51 PDT
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
Comment 9 EWS Watchlist 2018-09-24 15:50:53 PDT
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
Comment 10 Basuke Suzuki 2018-09-24 16:21:23 PDT
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.
Comment 11 EWS Watchlist 2018-09-24 17:38:48 PDT
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
Comment 12 EWS Watchlist 2018-09-24 17:38:50 PDT
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
Comment 13 Basuke Suzuki 2018-09-25 11:44:51 PDT
Created attachment 350773 [details]
PATCH

Fix cookie path.
Comment 14 Alex Christensen 2018-09-25 14:00:24 PDT
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.
Comment 15 Basuke Suzuki 2018-09-25 14:41:16 PDT
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.
Comment 16 Fujii Hironori 2018-09-26 21:58:59 PDT
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>.
Comment 17 Basuke Suzuki 2018-09-27 17:08:08 PDT
Created attachment 351024 [details]
PATCH
Comment 18 Basuke Suzuki 2018-09-27 17:09:05 PDT
- Added created date test.
- Use setCookies.cgi
Comment 19 EWS Watchlist 2018-09-27 18:27:47 PDT
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
Comment 20 EWS Watchlist 2018-09-27 18:27:49 PDT
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
Comment 21 EWS Watchlist 2018-09-27 18:59:04 PDT
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
Comment 22 EWS Watchlist 2018-09-27 18:59:06 PDT
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
Comment 23 EWS Watchlist 2018-09-27 19:05:59 PDT
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
Comment 24 EWS Watchlist 2018-09-27 19:06:01 PDT
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
Comment 25 Fujii Hironori 2018-09-27 20:03:20 PDT
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?
Comment 26 Basuke Suzuki 2018-09-27 20:17:09 PDT
(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.
Comment 27 Fujii Hironori 2018-09-27 20:36:57 PDT
(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?
Comment 28 Basuke Suzuki 2018-09-27 21:25:43 PDT
(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.
Comment 29 EWS Watchlist 2018-09-27 21:59:24 PDT
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
Comment 30 EWS Watchlist 2018-09-27 21:59:36 PDT
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
Comment 31 Basuke Suzuki 2018-09-27 22:05:00 PDT
Created attachment 351055 [details]
PATCH

Remove timestamp tests
Comment 32 WebKit Commit Bot 2018-09-27 23:55:11 PDT
Comment on attachment 351055 [details]
PATCH

Clearing flags on attachment: 351055

Committed r236593: <https://trac.webkit.org/changeset/236593>
Comment 33 WebKit Commit Bot 2018-09-27 23:55:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Radar WebKit Bug Importer 2018-09-27 23:56:37 PDT
<rdar://problem/44856431>