Explicitly prevent sandboxed navigation in the history interface. #4787

dtapuska · 2019-07-18T14:35:50Z

The spec was a little unclear whether sandboxed navigation was prevented
if it causes a top-level navigation via the history API. The check for
the navigation was after the unload steps of the history traversal.

Fixes #880

/history.html ( diff )

The spec was a little unclear whether sandboxed navigation was prevented if it causes a top-level navigation via the history API. The check for the navigation was after the unload steps of the history traversal. Fixes whatwg#880

dtapuska · 2019-07-18T14:40:30Z

@mikewest @domenic @bzbarsky

For a data point Chrome has implemented metrics related to these changes.

Navigation that occurs within the same sandboxed subtree (this will still be allowed by the current pull request)
https://www.chromestatus.com/metrics/feature/popularity#SandboxBackForwardStaysWithinSubtree

Navigation that occurs outside of the sandboxed subtree (this will be prevented by the current pull request)
https://www.chromestatus.com/metrics/feature/popularity#SandboxBackForwardAffectsFramesOutsideSubtree

domenic · 2019-07-30T18:56:57Z

source

-  and forward buttons, the user agent must <span>traverse the history by a delta</span> equivalent
-  to the action specified by the user.</p>
+  and forward buttons, the user agent must <span>traverse the history by a delta</span> with a delta
+  equivalent to the action specified by the user and the <span>top-level browsing context</span>.</p>


I don't think it should necessarily be the top-level. Consider right-clicking inside an iframe and choosing back from that menu. I'll tweak it.

domenic

Thanks, great stuff! While cleaning up minor things (e.g. using <span data-x="concept-document-bc">browsing context</span> instead of <span>browsing context</span>) I took the liberty of also making these functions into algorithmic steps, for clarity.

It sounds like we need tests for this, and potentially we'll want to wait to hear about the compat fallout, right? Otherwise, #880 seems to contain interest from Gecko, so we can merge this as soon as there are tests and you are ready to confirm Chrome will implement this change.

…l doc Spec change whatwg/html#4787 This change adds tests (not currently run) and a content feature to be enable/disable the feature. A followup change will enable the feature and enable the tests. BUG=705583 Change-Id: I5169b54df63a30e252566398b139a7db187eaa42

Spec change whatwg/html#4787 BUG=705583 Change-Id: I6fc5fee627156c10c771b63b609d1d25c6fd439c

Spec change whatwg/html#4787 BUG=705583 Change-Id: I6fc5fee627156c10c771b63b609d1d25c6fd439c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1749444 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Domenic Denicola <[email protected]> Cr-Commit-Position: refs/heads/master@{#686032}

dtapuska · 2019-08-13T17:50:40Z

@domenic The tests have landed for this. Is there anything else blocking merging this?

domenic · 2019-08-13T17:58:57Z

Looks good. @bzbarsky @smaug---- I am going to take your comments in #880 (comment) as supportive of this change; please let us know if that was not formal enough of an indication.

bzbarsky

I'm sorry for the lag here. I've had a bunch of spec reviews dumped on me recently, and doing them carefully is taking more time than I have allocated to spec reviews. Especially when I have to do some of them multiple times, unfortunately... :(

There's a fundamental problem with the spec change that happened here: it might end up examining the wrong sandbox flags.

There's also a secondary question: should the history API use the sandbox flags of the History object's window's document, or the incumbent document? Or can we prove that they are the same in all the cases that matter?

bzbarsky · 2019-08-13T19:22:04Z

source

-  consisting of running the following steps:</p>
+  <p>To <dfn>traverse the history by a delta</dfn> given <var>delta</var> and <span>browsing
+  context</span> <var>source browsing context</var>, the user agent must append a <span
+  data-x="concept-task">task</span> to this <span>top-level browsing context</span>'s <span>session


Pre-existing, but "this top-level browsing context" doesn't make sense. Presumably this should be the top-level browsing context corresponding to the source browsing context, but there's no obvious short way to say that in the HTML spec that I can see.

Why can't it be source browsing context's top-level browsing context ?

Good catch, it can.

bzbarsky · 2019-08-13T19:29:34Z

source

@@ -81691,6 +81721,9 @@ interface <dfn>History</dfn> {
   <li><p>Let <var>specified browsing context</var> be the <span>browsing context</span> of
   the <var>specified entry</var>.</p></li>

+   <li><p>If <var>source browsing context</var> is not <span>allowed to navigate</span>


This looks wrong to me. This check is happening async from the original history.go() call, right? By the time this check runs, the source browsing context's document can be different from the document it had when the history.go() call happened, and can have different sandboxing flags.

There's the additional issue, inherent in the "allowed to navigate" setup, which is that we are now running in the event loop of the toplevel browsing context, which is possibly not-same-process with the source browsing context, but "allowed to navigate" reaches into the state of the "source browsing context document" and reads its sandbox flags. The Location API has this second issue too, but not the first one, since it navigates sync.

What should probably happen is that we should snapshot the sandbox flags at the point when we are about to go async and then have sandbox flags as input to "allowed to navigate" or something.

Yes I think we can probably put this as a few steps in the navigate by delta method. Load the sandbox flags from source browsing context and then queue. In Chrome we don't trust sandbox flags from the renderer so the browser process has them cloned (as set by the embedder) so this snapshot would be technically hard to implement for Chrome because the "post task" is essentially an IPC to the browser process in Chrome.

Right, the IPC bit is a pretty important bit here, not just in Chrome.

The case that worries me is the case when we have a sandboxed frame, then the parent removes the sandbox attr and starts a new navigation, and just before that navigation matures the script in the frame does a history operation. By the time we read the sandbox flags, would they have been updated with the new ("no sandbox") flags? Seems like they would.

@smaug---- do you know how we're planning on handling this in Gecko?

Chrome's sandbox flags on the browser side are only set on navigation (load of new document). So to force them to change you'd have to have to finish the navigate causing new flags while the goto offset IPC was inflight from the previous document.

Right, that is the worrisome case.

bzbarsky · 2019-08-13T19:32:10Z

source

-  and forward buttons, the user agent must <span>traverse the history by a delta</span> equivalent
-  to the action specified by the user.</p>
+  and forward buttons, the user agent must <span>traverse the history by a delta</span> with a delta
+  equivalent to the action specified by the user and the browsing context being operated on.</p>


What is "the browsing context being operated on" here, exactly? I don't know what the mental model here is for the way UA actions map to browsing contexts...

Well, this is about user agent UI, so that's really up to the UA. For example, when I click on the back button on the opt toolbar, in most browsers that navigates the current TLBC. Or when I right-click inside an iframe and choose the "Back" menu item, it navigates the iframe's BC.

I don't think specifying this kind of UI correspondence is really something we should be getting in to. This preexisting sentence is just saying that UA UI should use the same history traversal algorithm, and now we're updating it since that algorithm takes a BC.

I agree this is hand wavy 👋 but it seemed best at the time. I'm not sure there are other examples of this kind of operation.

I guess the whole paragraph would make a bit more sense to me if it said something like:

When the user navigates through history, e.g. using a browser's back and forward buttons, the user agent must determine the browsing context that the user wants to act on and traverse the history by a delta with a delta equivalent to the action specified by the user and that browsing context.

or something. I agree handwavy is the best we can do here.

bzbarsky · 2019-08-13T19:37:14Z

@domenic @dtapuska I assume we want a separate issue and PR for the problem here? This merged partway through me writing my review comments....

domenic · 2019-08-13T20:00:44Z

Eeek, OK. I'm sorry I didn't catch these.

I see two paths:

Discuss here. Open a tracking issue to ensure we don't lose the fact that there's unfinished work here.
Revert, then start a new pull request and have the discussions there.

Let's try (1) for now...

bzbarsky · 2019-08-13T20:05:10Z

(1) works for me.

domenic · 2019-08-13T20:09:01Z

There's also a secondary question: should the history API use the sandbox flags of the History object's window's document, or the incumbent document? Or can we prove that they are the same in all the cases that matter?

I would like to avoid any further uses of the incumbent concept...

As for whether they're the same, I believe that they won't necessarily be the same. I think you could loosen up enough sandboxing flags, except for allow navigation, so that you could set up an example similar to the a.html/b.html/c.html/d.html setup in the spec. But, they should be same-origin, at least. I'm unsure where that falls on the "matter" scale...

bzbarsky · 2019-08-13T20:48:13Z

So trying to reason through that stuff... We (the incumbent) are clearly running script and hence have allow-scripts. Let's assume we don't have allow-same-origin (if we do, we have already lost). If we're touching some other global's History, where did that other global come from?

If it's our subframe, then it would have the same sandbox flags as us.
If it's a popup we created, it would have the same sandbox flags as us, unless we have allow-popups-to-escape-sandbox. If we do, we could load a blob in that popup. However in that case we would be navigating the popup anyway, not our toplevel browsing context.
If it's some non-toplevel browsing context that has the same toplevel browsing context as us but is not our descendant, how did we get it to be same-origin with us? We have a unique origin, and we're not allowed to navigate such browsing contexts, so can't do the blob thing.
If it's our toplevel, that means we navigated it to a blob already and I'm not sure how we're getting to run script at all at this point....

…om using history APIs, a=testonly Automatic update from web-platform-tests Add tests to prevent a sandbox iframe from using history APIs Spec change whatwg/html#4787 BUG=705583 Change-Id: I6fc5fee627156c10c771b63b609d1d25c6fd439c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1749444 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Domenic Denicola <[email protected]> Cr-Commit-Position: refs/heads/master@{#686032} -- wpt-commits: 5d435e04f41adf7c891c575b3f8ab120923766fe wpt-pr: 18391

…l doc Spec change whatwg/html#4787 This change marks navigations from the renderer indicating if they are originating from script. If they are from script (as opposed to the back and forward button default event handlers) they will be subject to the top level navigation sandbox flag. Intent to ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/jOa27iZPJtg/2ArNlXIBBAAJ BUG=705583 Change-Id: I5169b54df63a30e252566398b139a7db187eaa42 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1729530 Reviewed-by: Daniel Cheng <[email protected]> Reviewed-by: Charlie Reis <[email protected]> Commit-Queue: Dave Tapuska <[email protected]> Cr-Commit-Position: refs/heads/master@{#690057}

Spec change whatwg/html#4787 BUG=705583 Change-Id: I6fc5fee627156c10c771b63b609d1d25c6fd439c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1749444 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Domenic Denicola <[email protected]> Cr-Commit-Position: refs/heads/master@{#686032}

…om using history APIs, a=testonly Automatic update from web-platform-tests Add tests to prevent a sandbox iframe from using history APIs Spec change whatwg/html#4787 BUG=705583 Change-Id: I6fc5fee627156c10c771b63b609d1d25c6fd439c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1749444 Reviewed-by: Domenic Denicola <domenicchromium.org> Commit-Queue: Domenic Denicola <domenicchromium.org> Cr-Commit-Position: refs/heads/master{#686032} -- wpt-commits: 5d435e04f41adf7c891c575b3f8ab120923766fe wpt-pr: 18391 UltraBlame original commit: 74a2012e3573c4725ed808e9dd18e29d0ecf2d8c

domenic reviewed Jul 30, 2019

View reviewed changes

Fix which BC; editorial tweaks

95992a6

domenic approved these changes Jul 30, 2019

View reviewed changes

domenic added needs tests Moving the issue forward requires someone to write tests normative change security/privacy There are security or privacy implications topic: history labels Jul 30, 2019

domenic self-assigned this Jul 30, 2019

chromium-wpt-export-bot mentioned this pull request Jul 31, 2019

Prevent a sandbox iframe from using history APIs to navigate top level doc web-platform-tests/wpt#18210

Closed

chromium-wpt-export-bot mentioned this pull request Aug 12, 2019

Add tests to prevent a sandbox iframe from using history APIs web-platform-tests/wpt#18391

Merged

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 12, 2019

Add tests to prevent a sandbox iframe from using history APIs

e5729f9

Spec change whatwg/html#4787 BUG=705583 Change-Id: I6fc5fee627156c10c771b63b609d1d25c6fd439c

domenic removed the needs tests Moving the issue forward requires someone to write tests label Aug 13, 2019

domenic merged commit 2dafc53 into whatwg:master Aug 13, 2019

bzbarsky reviewed Aug 13, 2019

View reviewed changes

domenic mentioned this pull request Aug 13, 2019

Followups for "Explicitly prevent sandboxed navigation in the history interface" #4838

Closed

siusin mentioned this pull request Apr 13, 2020

HTML Standard (Jan 2020): review tracker w3c/htmlwg#8

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicitly prevent sandboxed navigation in the history interface. #4787

Explicitly prevent sandboxed navigation in the history interface. #4787

dtapuska commented Jul 18, 2019 •

edited by pr-preview bot

Loading

dtapuska commented Jul 18, 2019

domenic Jul 30, 2019

domenic left a comment

dtapuska commented Aug 13, 2019

domenic commented Aug 13, 2019

bzbarsky left a comment

bzbarsky Aug 13, 2019

dtapuska Aug 13, 2019

bzbarsky Aug 13, 2019

bzbarsky Aug 13, 2019

dtapuska Aug 13, 2019

bzbarsky Aug 13, 2019

dtapuska Aug 13, 2019

bzbarsky Aug 13, 2019

bzbarsky Aug 13, 2019

domenic Aug 13, 2019

dtapuska Aug 13, 2019

bzbarsky Aug 13, 2019

bzbarsky commented Aug 13, 2019

domenic commented Aug 13, 2019

bzbarsky commented Aug 13, 2019

domenic commented Aug 13, 2019

bzbarsky commented Aug 13, 2019

Explicitly prevent sandboxed navigation in the history interface. #4787

Explicitly prevent sandboxed navigation in the history interface. #4787

Conversation

dtapuska commented Jul 18, 2019 • edited by pr-preview bot Loading

dtapuska commented Jul 18, 2019

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

dtapuska commented Aug 13, 2019

domenic commented Aug 13, 2019

bzbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky commented Aug 13, 2019

domenic commented Aug 13, 2019

bzbarsky commented Aug 13, 2019

domenic commented Aug 13, 2019

bzbarsky commented Aug 13, 2019

dtapuska commented Jul 18, 2019 •

edited by pr-preview bot

Loading