Conversation
546d519 to
ad0e6b8
Compare
| interacted in <var>W</var>. It starts false, then changes to true (and never changes back to | ||
| false) when <var>W</var> gets the very first <span>activation notification</span>.</p> | ||
| false) when <var>W</var> gets the very first <span>activation notification</span>. It is also | ||
| carried over between windows for same-origin navigations and traversals.</p> |
There was a problem hiding this comment.
Hmm, is this quite right? If there is a bfcached page without sticky activation, I don't think the algorithms will set the flag on those window objects if another same origin window gets sticky activation. And I'm not sure what behavior we want there.
There was a problem hiding this comment.
You're right, that's a great catch! I think we should carry it over to same-origin bfcached documents too, to minimize differences between cases where the bfcache is hit vs. missed.
I'll add a line to the "reactivate" algorithm similar to the one I added to the "create and initialize a new Document" algorithm.
There was a problem hiding this comment.
This ended up being more annoying than I'd prefer, as threading things from the predecessor document to the new document seems to be surprisingly unusual. (In particular, if the browser plans to unload and destroy the previous document, we need to grab the state before it does that.)
I have a half-finished local branch with an alternate option, which, at the time we set sticky activation for one window, immediately tries to propagate it to all contiguous same-origin bfcached windows in the same navigable. But I realized that keeping track of "contiguous" would add a good amount of complexity (albeit only locally), and this probably would not be how implementations do it, so I stashed that.
Of course, there's a separate issue here where the whole user activation framework ignores the complexities of propagating the bit across processes, instead just letting people access the Window object from anywhere. That is fairly pervasive in the spec ecosystem though. (That is, although specs these days are relatively good about separating out processes, the rarer cases like this one where we need to propagate state so that it lives in multiple processes are all hand-waved. See w3c/ServiceWorker#1755 (comment) for more rambling.)
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. |
https://bugs.webkit.org/show_bug.cgi?id=316059 Reviewed by Anne van Kesteren. Update html/user-activation/navigation-state-reset-sameorigin.html to the latest upstream version, which adds a check that a same-origin child frame retains its sticky user-activation state after it completes a same-origin navigation, matching whatwg/html#11454. WebKit does not yet implement this behavior, so the new subtest fails and is baselined into the -expected.txt accordingly. Upstream commit: web-platform-tests/wpt@8187b0a8e3 * LayoutTests/imported/w3c/web-platform-tests/html/user-activation/navigation-state-reset-sameorigin-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/user-activation/navigation-state-reset-sameorigin.html: Canonical link: https://commits.webkit.org/314406@main
… flags https://bugs.webkit.org/show_bug.cgi?id=316317 Reviewed by Youenn Fablet. Per the user-activation data model change in the proposed whatwg/html#11454, track sticky activation and history-action activation as explicit booleans on LocalDOMWindow instead of deriving them from the last activation timestamp. m_lastActivationTimestamp continues to drive transient activation only. notifyActivated and its ancestor/descendant propagation, consume-history- action, and WebPage::updateUserActivationTimestamps now update the booleans alongside the timestamp. This is not gated behind a preference. Behavior is preserved, except that consuming transient activation no longer resurrects an already-consumed history-action activation, which matches the proposed explicit boolean model. * Source/WebCore/page/LocalDOMWindow.cpp: (WebCore::LocalDOMWindow::hasStickyActivation const): (WebCore::LocalDOMWindow::hasHistoryActionActivation const): (WebCore::LocalDOMWindow::consumeHistoryActionUserActivation): (WebCore::LocalDOMWindow::notifyActivated): * Source/WebCore/page/LocalDOMWindow.h: (WebCore::LocalDOMWindow::setHasStickyActivation): (WebCore::LocalDOMWindow::setHasHistoryActionActivation): * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::didNotifyUserActivation): * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::updateUserActivationState): * Source/WebKit/WebProcess/WebPage/WebPage.h: * Source/WebKit/WebProcess/WebPage/WebPage.messages.in: Canonical link: https://commits.webkit.org/315598@main
|
I've now updated this PR to not carry forward sticky activation through a A -> B -> A navigation. @annevk can you give this another look? |
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:
|
Apart from https://gist.github.com/annevk/8fef6335f0442b9124114bbb54f17b0f there's two other issues worth discussing:
|
|
What would be the process for feature detection here? In the instance of, say, a video player that carries playback through from a thumbnail to fullscreen presentation it would be preferable to still use a same-page transition rather then a multi-page that won't preserve playback state. But I'm not clear how you'd be able to know ahead of time whether it will or not. |
The blockers there were addressed in 2e8536a and an update to the PR description. I also took some of the suggestions in 6329610 and f4a1274, but not all of it. In particular the claimed null deref that would be fixed by moving a step into the "Otherwise" branch, I really can't tell from looking at the algorithm that it might do a null deref, the condition is "If changingNavigableContinuation's update-only is true, or targetEntry's document is displayedDocument" and I don't understand what's implied by that being false.
Fixed in c71dd74.
I have a change prepared by AI for this that looks like this: --- a/source
+++ b/source
@@ -112989,11 +112989,14 @@ <h4>Shared document creation infrastructure</h4>
<li>
<p>If <var>browsingContext</var>'s <span>active document</span>'s <span>is initial
- <code>about:blank</code></span> is true, and <var>browsingContext</var>'s <span>active
+ <code>about:blank</code></span> is true, <var>browsingContext</var>'s <span>active
document</span>'s <span data-x="concept-document-origin">origin</span> is <span>same
origin-domain</span> with <var>navigationParams</var>'s <span
- data-x="navigation-params-origin">origin</span>, then set <var>window</var> to
- <var>browsingContext</var>'s <span>active window</span>.</p>
+ data-x="navigation-params-origin">origin</span>, and <var>navigationParams</var>'s <span
+ data-x="navigation-params-response">response</span>'s <span
+ data-x="concept-response-has-cross-origin-redirects">has cross-origin redirects</span> is
+ false, then set <var>window</var> to <var>browsingContext</var>'s <span>active
+ window</span>.</p>
<p class="note">This means that both the <span data-x="is initial about:blank">initial
<code>about:blank</code></span> <code>Document</code>, and the new <code>Document</code> thatI'm not very confident about this though. It has the appearance of a correct fix, but I don't know this infrastructure. I can create a separate PR for it if you like. |
@alastaircoote I'm afraid it just won't be possible to feature detect this since it doesn't introduce any new API surface area. |
See discussion in WICG/view-transitions#239 and #11328 (comment).
This also redefines history-action user activation as a simple boolean, instead of using the timestamp infrastructure.
Details:
/cc @mustaqahmed @nickcoury
(See WHATWG Working Mode: Changes for more details.)
/browsing-the-web.html ( diff )
/document-lifecycle.html ( diff )
/interaction.html ( diff )