Repository navigation
Conversation
546d519 to
ad0e6b8
Compare
annevk
left a comment
There was a problem hiding this comment.
This looks good to me. Since it's restricted to same-origin I don't think this really increases the risk of anything bad happening.
The one thing that gives me pause, but was apparently already the case, is that these values persist "forever". But maybe that's more of a comment to be had on bfcache, that expiring after a couple of days is probably a good idea.
|
Fixed nits. @mustaqahmed is working on web platform tests; it's been a bit tricky to test but I think we're getting close to a solution. I'll wait to merge until those are ready. I filed Gecko and MDN bugs, but https://bugs.webkit.org/ is down at the moment so I'll have to do that later. |
|
A colleague brought up some good points:
|
I agree with this.
I'm less sure about this. My instinct was to just do whatever was easiest to spec/implement, which in this case was to allow it to work in iframes. |
I'm no longer sure about this. It seems like most parts of the spec only compare the endpoint origins in A -> B -> A navigations today:
There's also one cases that is confusing:
The only case, in HTML at least, that unambiguously changes behavior for A -> B -> A cases, is unload timing info, which gets censored in those cases. Given this situation, I'd prefer sticky activation is carried over in A -> B -> A cases. Unless we have a compelling security story for a hole that carrying it over creates. Optionally, in the future, someone could investigate whether our choices in all the above-listed cases are coherent, and if we should move to a model that considers A -> B -> A "more cross-origin". (Although I suspect the compat implications might be bad.) |
|
I don't think that's correct? We call "enforce a response's opener policy" for each response we get, which includes redirect responses as navigate doesn't follow those automatically. The risk of exploitation seems minimal, but it's the standard confused deputy attack scenario. A navigates to B which redirects to A2. A2 doesn't think it's in a state where it can hold sticky activation, but it actually does, which results in something unfortunate. |
You're right, although we only do the final BCG swap checking at the end, the "COOP enforcement result" structure is modified each time through the loop in a cumulative way. So that leaves us at 6 endpoint-only checks, 2 all-legs checks, and 1 inconsistent-depending-on-bfcache check. Sticky activation feels more similar to things like navigation API state or |
Intent to Ship: https://groups.google.com/a/chromium.org/d/msgid/blink-dev/68b0b943.050a0220.270bc4.0090.GAE%40google.com Spec PR (already secured another browser's approval): whatwg/html#11454 Fixed: 433729626 Change-Id: Ib475166aeec709837c9d67f6936f51abdc38c6a5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6961179 Commit-Queue: Vladimir Levin <vmpstr@chromium.org> Auto-Submit: Mustaq Ahmed <mustaq@chromium.org> Reviewed-by: Vladimir Levin <vmpstr@chromium.org> Cr-Commit-Position: refs/heads/main@{#1523044}
…igation Original change's description: > Enable carrying sticky-activation state across same-origin navigation > > Intent to Ship: > https://groups.google.com/a/chromium.org/d/msgid/blink-dev/68b0b943.050a0220.270bc4.0090.GAE%40google.com > > Spec PR (already secured another browser's approval): > whatwg/html#11454 > > Fixed: 433729626 > Change-Id: Ib475166aeec709837c9d67f6936f51abdc38c6a5 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6961179 > Commit-Queue: Vladimir Levin <vmpstr@chromium.org> > Auto-Submit: Mustaq Ahmed <mustaq@chromium.org> > Reviewed-by: Vladimir Levin <vmpstr@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1523044} (cherry picked from commit 96f69eb) Bug: 448428855,433729626 Change-Id: Ib475166aeec709837c9d67f6936f51abdc38c6a5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7004556 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org> Auto-Submit: Chrome Cherry Picker <chrome-cherry-picker@chops-service-accounts.iam.gserviceaccount.com> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Cr-Commit-Position: refs/branch-heads/7444@{#80} Cr-Branched-From: 29907d3-refs/heads/main@{#1522585}
|
I'd like to finish this and have read through the discussion. @annevk raised two issues in #11454 (comment). On iframes, it seems simpler to just allow those navigations to carry over sticky activation. The remaining issue is A -> B -> A navigation where B is a redirect back to A. This PR currently does carry forward sticky activation in this case. @annevk what is your preference, to move forward with this as proposed, or ensure that A -> B -> A does not carry forward sticky activation? In either case we should file an issue about the lack of consistency and decide on what we want the default for new things to be. |
|
I would prefer not carrying it forward in those cases and testing that, to prevent potential attacks. If we can somehow find proof those attacks don't exist, maybe an exception can be made. |
|
Thanks @annevk. First step is a test to confirm without a doubt what the behavior implemented in Chromium is. I've asked @mustaqahmed about that. |
More AI review |
https://bugs.webkit.org/show_bug.cgi?id=313716 Reviewed by NOBODY (OOPS!). Implement sticky user activation carry-over per whatwg/html#11454, centralized in LocalDOMWindow::carryOverStickyActivationFromPreviousWindow and applied at two entry points: - DocumentWriter::begin carries sticky activation onto a same-origin destination, suppressed when the redirect chain crossed origins (guards A -> B -> A confused-deputy patterns). - FrameLoader::open(CachedFrameBase&) applies the same rule on bfcache reactivation; no network load, so the redirect suppressor is N/A. Builds on the explicit sticky/history-action activation booleans on LocalDOMWindow (bug 316317); carry-over sets m_hasStickyActivation on the destination window when the source had sticky activation and the navigation stayed same-origin. Only sticky activation is carried over, not transient or history-action. Gated behind StickyUserActivationAcrossSameOriginNavigationsEnabled (testable, off by default, auto-enabled in WebKitTestRunner). * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/html/user-activation/navigate-to-crossorigin-redirect-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/user-activation/navigate-to-sameorigin-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/user-activation/navigation-state-reset-sameorigin-expected.txt: * LayoutTests/platform/ios/TestExpectations: * Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml: * Source/WebCore/loader/DocumentWriter.cpp: * Source/WebCore/loader/FrameLoader.cpp: * Source/WebCore/page/LocalDOMWindow.cpp: * Source/WebCore/page/LocalDOMWindow.h:
This was previoiusly broken, "If navigable's container is not null" refered to variable that was never defined.
The condition looks a bit different in the two branches, but should amount to the same thing, with the information coming from the sourceDocument argument to the navigate algorithm.
|
@annevk I have addressed the blocker, those of the nits I thought were sensible and belong in this PR, and some additional minor things Claude found for me. |
AI feedbackReviewed at b71c8eb. Lint and build pass. Checked and fine: with a single decision point, the initiator check now applies the same way to push/replace, reload, and traversals with or without bfcache. That includes Suggestions
Nits
|
|
Of the remaining feedback, I think this is the one that could still lead to some normative change:
I think that would mean adding a boolean to https://html.spec.whatwg.org/#document-state-2 and copying it from the document. I'm a bit skeptical about propagating even more state, but it would be technically straightforward I think. @annevk happy to do whatever you think is going to be the pattern or principle in #13015. |
|
I checked what Chromium does and the "was created via cross-origin redirects" bit isn't implemented yet and navigate-to-crossorigin-redirect.html fails. So that doesn't really inform the decision, we can do whatever the best model is going forward. |
AI feedbackReviewed at fa92047. Lint and build pass. I skipped the commit message, which is about to be updated. Checked and fine: the reload exemption is safe. A reload can only come from the user, the document itself ( Blockers
Suggestions
Nits
|
|
I agree with the idea from the bot that we should store the bit on the document so that it remains correct in a bfcache situation. |
…e that for carryover decisions
|
OK, I've added another "was created via cross-origin redirects" bit. Note that it has to be on document state, not on a document. For the suggestions and nits, I've looked at them in previous rounds and also tried them out as local commits, but I didn't like the changes. Especially for notes I think the review skill's tendency is to ask for more precision, but notes that are more technically correct also become longer and less readable. @annevk feel free to flag changes that you agree with and I'll make them, though. |
AI feedbackReviewed at 7876a96. Lint and build pass. The new document-state bit closes the gap when the page has been evicted from bfcache, and it's correctly copied when the redirect loop rebuilds the document state. Blockers
|
annevk
left a comment
There was a problem hiding this comment.
Looks good! The commit message should be updated to describe the initiator, redirect, reload, and history fixes. And we should make sure to add tests for all those cases. (I guess it's somewhat fine if that happens as a follow-up, but let's make sure it actually happens.)
|
@annevk thanks! I made a small tweak to a note that I thought was an improvement, and also updated the commit message. Let me see about the test coverage before merging. |
See discussion in WICG/view-transitions#239
and #11328 (comment).
Details:
transient activation. This requires adding the "has sticky activation"
boolean to the user activation data model.
It is only for the navigated frame (which can be an iframe), and it
copies from its predecessor Window in that same frame.
and non-bfcached traversals, except for iframes recreated during
non-bfcache traversals.
navigation, so they only check whether the reload itself was redirected
cross-origin.
navigation or redirected to the destination. To make this hold after
bfcache eviction, a "was created via cross-origin redirects" bit is
added to document state.
instead of using the timestamp infrastructure.
/cc @mustaqahmed @nickcoury
(See WHATWG Working Mode: Changes for more details.)
/browsing-the-web.html ( diff )
/document-lifecycle.html ( diff )
/interaction.html ( diff )