Bug 232722 – [Curl] Improve curl's cookie conformance in WPT

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

[Christopher Reid]

.

[Christopher Reid]

Created attachment 443719 [details]
patch

[Fujii Hironori]

Comment on attachment 443719 [details]
patch

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

> LayoutTests/platform/wincairo/imported/w3c/web-platform-tests/cookies/ordering/ordering.sub-expected.txt:4
> +FAIL Cookies with longer path attribute values are ordered before shorter ones (2) assert_equals: The cookie was set as expected. expected "testG=2; testB=2; testF=2; testH=2; testC=2" but got "testB=2; testH=2; testC=2"

Don't add FAIL result as a expected result. Mark the test as Failure.

[Fujii Hironori]

Comment on attachment 443719 [details]
patch

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

> LayoutTests/platform/wincairo/imported/w3c/web-platform-tests/cookies/navigated-away-expected.txt:1
> +

Why do we have to add WinCairo specific navigated-away-expected.txt? Why can't WinCairo use the common navigated-away-expected.txt?

[Fujii Hironori]

Comment on attachment 443719 [details]
patch

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

> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:71
>              cookies.append(cookie.value);

Do you need to set the value even if the name is empty?

[Fujii Hironori]

Comment on attachment 443719 [details]
patch

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

> Source/WebCore/platform/network/curl/CookieUtil.cpp:137
> +            result.path = emptyString();

Only path? What about other parameters? For example, if the second expires parameter has a invalid value, should it override the first one?

[Radar WebKit Bug Importer]

<rdar://problem/85310326>

[Christopher Reid]

Comment on attachment 443719 [details]
patch

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

>> LayoutTests/platform/wincairo/imported/w3c/web-platform-tests/cookies/navigated-away-expected.txt:1
>> +
> 
> Why do we have to add WinCairo specific navigated-away-expected.txt? Why can't WinCairo use the common navigated-away-expected.txt?

Yeah I can clean it up, it was a bit tricky because I couldn't find a way to easily generate baseline files only for tests that don't match expected results. Also top level WPT baseline has FAIL results in the baseline so the expected results get a bit messy.
I'll try to clean up the expected results some more.

>> Source/WebCore/platform/network/curl/CookieUtil.cpp:137
>> +            result.path = emptyString();
> 
> Only path? What about other parameters? For example, if the second expires parameter has a invalid value, should it override the first one?

According to the spec for expires "If the attribute-value failed to parse as a cookie date, ignore the cookie-av."
However checking both chrome and firefox, setting invalid or empty string expires/max-age will override previous attribute-values and set a session cookie.

For domain we match chrome's behavior.
Testing on trac.webkit.org with chrome:
document.cookie = "foo=test; Domain=webkit.org; Domain=" => Sets Cookie with domain=.webkit.org
document.cookie = "foo=test; Domain=webkit.org; Domain=google.com" => Doesn't set cookie

I'll update the domain and max-age parsing to override previous attribute-values.

>> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:71
>>              cookies.append(cookie.value);
> 
> Do you need to set the value even if the name is empty?

Yeah, checking chrome you can set a cookie value to the empty string name. Doing `document.cookie = "=test"` would add `test` to the list of cookies with an empty name. Doing `document.cookie = "=test2"` would override the first cookie since they both are setting cookies on the empty string.

[Christopher Reid]

Created attachment 447060 [details]
patch

Updated patch, only adding expectations files for ones that should now all pass or have more passes than the w3c baseline.

[Fujii Hironori]

Comment on attachment 447060 [details]
patch

LGTM

[EWS]

Committed r287000 (245212@main): <https://commits.webkit.org/245212@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447060 [details].