Cookie Store API: add branch for empty string path #49270

annevk · 2025-08-12T12:35:58Z

`2c5cf3d`

Cookie Store API: add branch for empty string path
https://bugs.webkit.org/show_bug.cgi?id=297268

Reviewed by Rupin Mittal.

Implement whatwg/cookiestore#283 with tests
upstreamed at web-platform-tests/wpt#54264

Canonical link: https://commits.webkit.org/298681@main

91f4b2b

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows	Apple Internal
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win	✅ 🛠 ios-apple
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	~~🧪 win-tests~~	🛠 mac-apple
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
✅ 🛠 🧪 unsafe-merge	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-08-12T12:36:04Z

EWS run on previous version of this PR (hash b060803)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows	Apple Internal
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win	✅ 🛠 ios-apple
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	⏳ 🧪 win-tests	✅ 🛠 mac-apple
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

annevk · 2025-08-12T14:13:56Z

This needs to use CookieUtil::defaultPathForURL instead.

webkit-early-warning-system · 2025-08-12T16:14:21Z

EWS run on previous version of this PR (hash 1fd68bc)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows	Apple Internal
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	🛠 win	✅ 🛠 ios-apple
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	🧪 win-tests	✅ 🛠 mac-apple
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	~~🛠 mac-safer-cpp~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-08-13T07:08:54Z

EWS run on previous version of this PR (hash 7b2a1a7)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows	Apple Internal
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win	✅ 🛠 ios-apple
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	💥 🧪 win-tests	✅ 🛠 mac-apple
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

RupinMittal · 2025-08-14T05:28:22Z

Source/WebCore/Modules/cookie-store/CookieStore.cpp

+        return;
    }

+    // FIXME: The specification does not perform this initialization of domain.


I think this code is correct and it's actually the spec that is wrong here. It says here https://cookiestore.spec.whatwg.org/#CookieStore-set (in the green box) that the domain defaults to "Domain: same as the domain of the current document or service worker’s location"

Yet the steps say "Let domain be null". I'm not sure how the spec expects the domain to be default to the same as that of the current document if it defaults to null in the steps.

I think this is a mistake in the spec maybe? Because last year, the default for domain was null (WebKit always defaulted to current document domain since CFNetwork won't let us set cookies with null domain). At some point the spec default was changed to current document domain but maybe they forgot to change in the steps?

(Contrast this with path, whose default is "/" and steps indeed start it off with "/"). Of course, the default is being changed in this patch here, but still the spec will be consistent in both places for path. It looks like a change needs to be made for domain to be consistent.

That's what the network layer ends up defaulting domain to. The domintro box is arguably a bit misleading, but it's also non-normative and not aimed at implementers.

Right, the comment is ok. I do remember that when implementing, I had to add this even though the spec didn't have it because setting cookies simply doesn't work without it (due to our network layer). Hence that one bug I filed. Then at some point I saw that the domintro box was changed, so I thought that the spec had in fact changed from default null to the current context's domain. And I thought great! Our quirk is correct now.

But I'm now realizing from your comment that's not fully true, and we're still figuring out that area. So yes, the comment is ok for now.

RupinMittal · 2025-08-14T05:36:51Z

Source/WebCore/Modules/cookie-store/CookieStore.cpp

-            promise->reject(Exception { ExceptionCode::TypeError, "If the cookie name begins with \"__Host-\", the path must either not be specified or be \"/\"."_s });
-            return;
-        }
+    // FIXME: This should be further down.


It seems like the spec should figure out the steps of where it wants to set the default domain (see my below comment), which will tell us where this should go.

It's a separate problem from what this PR is tackling though. I was hoping to tackle that next, though I think the changes will be mostly on our side.

Sounds good. Yeah since you're working on this area, I think the FIXME is ok for now then.

RupinMittal · 2025-08-14T05:43:01Z

Source/WebCore/Modules/cookie-store/CookieStore.cpp

-            promise->reject(Exception { ExceptionCode::TypeError, "The path must begin with a '/'"_s });
-            return;
-        }
+    ASSERT(!cookie.path.isNull());


It's not possible for this to happen. CookieInit::path is default initialized to "/". Perhaps we can remove this assertion.

Happy to remove it. The previous code checked for it being null so I thought I'd clarify that's not possible.

Actually yeah good point. It also doesn't hurt to have that there. Also a good safeguard just in case we change the default for path later too.

Let's keep it.

RupinMittal · 2025-08-14T06:04:29Z

Source/WebCore/Modules/cookie-store/CookieStore.cpp

+    }
+
+    if (cookie.path != "/"_s && cookie.name.startsWithIgnoringASCIICase("__Host-"_s)) {
+        promise->reject(Exception { ExceptionCode::TypeError, "If the cookie name begins with \"__Host-\", the eventual path must be \"/\"."_s });


Nit: What's the point of "eventual" here? Perhaps it's most clear to simply say "... the path must be "/".

Actually I get it. You're trying to explain the whole "it should either be "/" or empty so it can default to that" in a nice way.

Maybe we can say ... the path must be either be specified as "/" or not be specified so it defaults to "/". to make it clear to the developer.

webkit-early-warning-system · 2025-08-14T07:08:30Z

EWS run on current version of this PR (hash 91f4b2b)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows	Apple Internal
🧪 style	⏳ 🛠 ios	🛠 mac	🛠 wpe	⏳ 🛠 win	⏳ 🛠 ios-apple
🧪 bindings	🛠 ios-sim	⏳ 🛠 mac-AS-debug	🧪 wpe-wk2	⏳ 🧪 win-tests	⏳ 🛠 mac-apple
⏳ 🧪 webkitperl	🧪 ios-wk2	🧪 api-mac	🧪 api-wpe
	🧪 ios-wk2-wpt	🧪 mac-wk1	⏳ 🛠 wpe-cairo
	🧪 api-ios	🧪 mac-wk2	⏳ 🛠 gtk
	⏳ 🛠 vision	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 gtk-wk2
	⏳ 🛠 vision-sim	🧪 mac-wk2-stress	⏳ 🧪 api-gtk
	⏳ 🧪 vision-wk2	🧪 mac-intel-wk2	⏳ 🛠 playstation
	⏳ 🛠 tv	⏳ 🛠 mac-safer-cpp	🛠 jsc-armv7
	⏳ 🛠 tv-sim		🧪 jsc-armv7-tests
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2025-08-14T07:08:30Z

EWS run on current version of this PR (hash 91f4b2b)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows	Apple Internal
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win	✅ 🛠 ios-apple
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	~~🧪 win-tests~~	🛠 mac-apple
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
✅ 🛠 🧪 unsafe-merge	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

RupinMittal

Looks great :)

https://bugs.webkit.org/show_bug.cgi?id=297268 Reviewed by Rupin Mittal. Implement whatwg/cookiestore#283 with tests upstreamed at web-platform-tests/wpt#54264 Canonical link: https://commits.webkit.org/298681@main

webkit-commit-queue · 2025-08-14T15:10:49Z

Committed 298681@main (2c5cf3d): https://commits.webkit.org/298681@main

Reviewed commits have been landed. Closing PR #49270 and removing active labels.

annevk self-assigned this Aug 12, 2025

annevk added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Aug 12, 2025

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 12, 2025

annevk marked this pull request as draft August 12, 2025 14:14

annevk removed the merging-blocked Applied to prevent a change from being merged label Aug 12, 2025

annevk force-pushed the eng/Cookie-Store-API-add-branch-for-empty-string-path branch from b060803 to 1fd68bc Compare August 12, 2025 16:14

annevk marked this pull request as ready for review August 13, 2025 05:56

annevk force-pushed the eng/Cookie-Store-API-add-branch-for-empty-string-path branch from 1fd68bc to 7b2a1a7 Compare August 13, 2025 07:08

annevk requested review from RupinMittal and achristensen07 and removed request for achristensen07 August 13, 2025 07:18

RupinMittal reviewed Aug 14, 2025

View reviewed changes

annevk force-pushed the eng/Cookie-Store-API-add-branch-for-empty-string-path branch from 7b2a1a7 to 91f4b2b Compare August 14, 2025 07:08

RupinMittal approved these changes Aug 14, 2025

View reviewed changes

annevk added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Aug 14, 2025

Cookie Store API: add branch for empty string path

2c5cf3d

https://bugs.webkit.org/show_bug.cgi?id=297268 Reviewed by Rupin Mittal. Implement whatwg/cookiestore#283 with tests upstreamed at web-platform-tests/wpt#54264 Canonical link: https://commits.webkit.org/298681@main

webkit-commit-queue force-pushed the eng/Cookie-Store-API-add-branch-for-empty-string-path branch from 91f4b2b to 2c5cf3d Compare August 14, 2025 15:10

webkit-commit-queue merged commit 2c5cf3d into WebKit:main Aug 14, 2025

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Aug 14, 2025

annevk deleted the eng/Cookie-Store-API-add-branch-for-empty-string-path branch August 14, 2025 16:03

Cookie Store API: add branch for empty string path #49270

Cookie Store API: add branch for empty string path #49270

Uh oh!

Conversation

annevk commented Aug 12, 2025 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

2c5cf3d

Uh oh!

webkit-early-warning-system commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk commented Aug 12, 2025

Uh oh!

webkit-early-warning-system commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RupinMittal Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

annevk Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RupinMittal Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RupinMittal Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

annevk Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

RupinMittal Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

RupinMittal Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

RupinMittal Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RupinMittal Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

RupinMittal Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RupinMittal left a comment

Choose a reason for hiding this comment

Uh oh!

webkit-commit-queue commented Aug 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

annevk commented Aug 12, 2025 •

edited by webkit-early-warning-system

Loading

`2c5cf3d`

webkit-early-warning-system commented Aug 12, 2025 •

edited

Loading

webkit-early-warning-system commented Aug 12, 2025 •

edited

Loading

webkit-early-warning-system commented Aug 13, 2025 •

edited

Loading

annevk Aug 14, 2025 •

edited

Loading

RupinMittal Aug 14, 2025 •

edited

Loading

RupinMittal Aug 14, 2025 •

edited

Loading

RupinMittal Aug 14, 2025 •

edited

Loading

RupinMittal Aug 14, 2025 •

edited

Loading

webkit-early-warning-system commented Aug 14, 2025 •

edited

Loading

webkit-early-warning-system commented Aug 14, 2025 •

edited

Loading