Skip to content

Update platform guards and add notImplemented() call in shouldTreatAsSameSite #44751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update platform guards and add notImplemented() call in shouldTreatAsSameSite #44751

wants to merge 1 commit into from

Conversation

mcatanzaro
Copy link
Contributor

@mcatanzaro mcatanzaro commented Apr 30, 2025 •
edited by webkit-early-warning-system
Loading

e07a2d9

Update platform guards and add notImplemented() call in shouldTreatAsSameSite
https://bugs.webkit.org/show_bug.cgi?id=292334

Reviewed by NOBODY (OOPS!).

We can use the new implementation on any platform that implements the
cookie change listener API. Add a notImplemented() warning everywhere
else.

* Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::shouldTreatAsSameSite):

e07a2d9

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🛠 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

@mcatanzaro
Update platform guards and add notImplemented() call in shouldTreatAs…
e07a2d9 
…SameSite

https://bugs.webkit.org/show_bug.cgi?id=292334

Reviewed by NOBODY (OOPS!).

We can use the new implementation on any platform that implements the
cookie change listener API. Add a notImplemented() warning everywhere
else.

* Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::shouldTreatAsSameSite):
@mcatanzaro mcatanzaro requested a review from cdumez as a code owner April 30, 2025 16:41
@mcatanzaro mcatanzaro self-assigned this Apr 30, 2025
@mcatanzaro mcatanzaro added the WebKit2 Bugs relating to the WebKit2 API layer label Apr 30, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Apr 30, 2025 •
edited
Loading

EWS run on current version of this PR (hash e07a2d9)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🛠 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

@mcatanzaro mcatanzaro requested a review from fujii April 30, 2025 16:41
@fujii fujii requested review from donny-dont, foopoiuyt, rkirsling and ykimot and removed request for fujii April 30, 2025 20:44
@fujii
Copy link
Contributor

fujii commented May 2, 2025

I'm no longer a maintainer of the ports. I hope the right person reviews this. I don't know who.
IMHO, "security bug" is not a good comment. You should add a comment of how to fix.
Hostly speeking, I don't understand the bug and don't know how to exploit the bug.

@mcatanzaro
Copy link
Contributor Author

Ping reviewers

1 similar comment
@mcatanzaro
Copy link
Contributor Author

Ping reviewers

@mcatanzaro mcatanzaro requested a review from a team June 24, 2025 15:36
cdumez
cdumez reviewed Jun 24, 2025
View reviewed changes
Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
@@ -781,12 +781,14 @@ void NetworkConnectionToWebProcess::registerURLSchemesAsCORSEnabled(Vector<Strin

static bool shouldTreatAsSameSite(const URL& firstParty, const URL& url)
{
#if PLATFORM(COCOA) || USE(SOUP)
#if HAVE(COOKIE_CHANGE_LISTENER_API)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using COOKIE_CHANGE_LISTENER_API here is super confusing to me as I don't see what this has to do with cookie changes listening. Can you clarify why we need a compile guard at all? The code below doesn't looks COCOA / SOUP specific at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All I know is it "breaks tests," see: https://bugs.webkit.org/show_bug.cgi?id=285969#c2

We were able to add the || USE(SOUP) condition after implementing cookie change listener API in https://commits.webkit.org/289693@main so it's definitely tied to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could try removing the guards and see what breaks on Windows EWS. (But this will go onto my to-do list rather than for today.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez left review comments

@ykimot ykimot ykimot left review comments

@donny-dont donny-dont Awaiting requested review from donny-dont

@foopoiuyt foopoiuyt Awaiting requested review from foopoiuyt

@rkirsling rkirsling Awaiting requested review from rkirsling

Assignees

@mcatanzaro mcatanzaro

Labels
WebKit2 Bugs relating to the WebKit2 API layer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mcatanzaro @webkit-early-warning-system @fujii @cdumez @ykimot