-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
srcdoc and sandbox interaction with session history #6809
Comments
Something I haven't looked at yet: When going |
/cc @domfarolino @antosart for the CSP rules question. I am pretty sure that has a canonical answer as to how it should be, given recent policy container work. |
I believe the answer for CSP rules depends on the answer to the main question (whether the If we store the content of I believe the currently specced behaviour is to store the policies in history for srcdoc. This is also implemented in chrome. (This is not tested for CSP at the moment I believe, although it is for referrer-policy, see the file referrer-policy/generic/inheritance/iframe-inheritance-history-about-srcdoc.html) |
Agreed.
The spec doesn't seem to cover restoring srcdoc pages at all, unless I'm missing something.
I don't think so. See the tests above – it seems like Chrome goes back to the srcdoc defined in the page. I tested Firefox, and it does seem to store the CSP rules along with the srcdoc src, so it's like it stores the whole response. That's what I'll spec for now, and we can look at it again later if needed. |
Sorry, I just meant w.r.t. policy inheritance (which might never be called because the spec does not cover navigating back to srcdoc). And I actually was mistaken: we explicitly excluded srcdoc when storing policies in history
Also here I was referring to policies. Chrome reloads the srcdoc content from the Back to the original question: if we store the content of the |
I think it comes down to what you'd expect to happen here:
Should '/a' be sandboxed? |
Another good question! I am not sure. For history navigations to srcdoc it seemed to be slightly more clear to me, in order to avoid the asymmetry of getting one attribute (srcdoc) back from history while re-parsing another one (sandbox) from the <iframe> element, hence creating something potentially inconsistent. |
Yeah it's a tough one, and I don't know enough about sandboxing. How about:
It seems weird to load a non-sandboxed page into an iframe that was born with a sandbox attribute. Maybe reconnection just shouldn't happen in that case. |
That's a tough one. On the one hand, I would expect '/a' not to be sandboxed since it was originally not sandboxed when loaded. On the other hand, I worry that that could allow some sort of sandbox escape. E.g. maybe the outer page expects that once it puts the sandbox attribute on the iframe, it's safe from its contents.
This is my instinct. |
So, to summarize:
I think that sounds pretty reasonable. cc @smaug---- |
I think that's still a bit weird in this case:
If we're saying that policies are stored with the history entry, that means loading page1 without sandboxing, into an iframe with the sandbox attribute set. Isn't that… confusing? If we were starting again, I think I'd want iframe history to be cleared if safety features changed. |
Yeah it is. I wonder if clearing history upon such an action would break anything in practice. Failing that, using the sandboxing rules at the time navigate is invoked is prolly best for that case. Is this something we should consider doing better on with new features such as https://github.com/camillelamy/explainers/blob/master/anonymous_iframes.md? No breakage there yet. cc @camillelamy |
I think we're envisioning to look at the attribute every time we navigate, whether history or not. I thought this is what sandbox was doing? I would find it weird that an attribute set by the parent of the iframe for security purposes could be overwritten by the iframe's action of going back. I reckon that this looks weird from the iframe point-of-view, however in this case the iframe parent's choice of security attribute should be respected. Otherwise, we might risk a sandbox escape, or loading a regular document in a crossOriginIsolated context by error for the anonymous iframe case. |
@camillelamy yeah, I think that's a good fallback. The question is whether setting Although I'm not entirely sure how that would work. In the example from #6809 (comment) it would be reset in step 2, but presumably you still need a current history entry so would it really solve anything? @jakearchibald what was the model you had in mind? |
Doesn't that leave you with the other problem?
This results in the risky page being reparented in an iframe without sandboxing. Or:
This results in the risky page loading in an unsandboxed iframe. |
I was thinking the history clearing would happen in step 3, as that's the moment new sandboxing rules would be applied to the iframe. The spec would go "wait, the policy has changed!" and cycle the navigable before continuing (as if the iframe had been removed and readded in the same place). |
If there is a top-level navigation, then back, we would reload the top-level document. It might then recreate or not recreate the iframe, with whatever sandbox flag it wants, and then load whatever it wants inside. I don't see how this necessarily leads to reparenting the risky page.
I would argue that it is probably not good security practice to set sandbox flags on iframe and then remove them... For the history clearing part, would this apply to the history list the browser maintains (not really specced AFAIU) or just the history API available to the document? Because if the later, wouldn't there still be a problem when the user clicks on the back button? |
Unfortunately that's what happens. To reproduce:
"one" will be reparented into the second iframe, but it's no longer sandboxed. Reparenting logic is different across browsers, and unspec'd. In Firefox it seems to just use index in the source to reconnect iframes, so it's fairly possible to end up with content reloading in the wrong iframe, which may have a weaker policy. At a minimum, I think we need to avoid reparenting in cases like this, although I'm not 100% sure what the logic would be.
Yeah, I think you're right. Maybe solving the reparenting case is enough. Although, the "clearing history" fix seems easy enough, and if it protects folks, it might be worth it?
It'd be the browser history list (I'm working on a better spec for that in #6315). But, it'd work the same as removing and readding the iframe, since that flushes iframe history. |
Treating it as if the iframe was removed an re-added when we change security properties of the frame seems reasonable to me. |
cc @iVanlIsh |
Firefox behavior of storing srcdoc into the history entry is interesting. At some point, we are going to isolate sandboxed document in their own process. Since we are going to make srcdoc content to move across processes, we might benefits from copying Firefox behavior here along the way.
Actually, by reading this thread, I realize the breaking change would actually mean converging with Firefox here. That might turns out to be a nice outcome. |
Some more findings: Test:
Chrome, Firefox: add replace It seems like Chrome & Firefox treat it as a replacement since the url is There's a good case to be made for each srcdoc change being an 'add', but I guess spec'ing what Chrome & Firefox do is fine. Test:
Chrome, Firefox: add add Chrome and Firefox are consistent with the previous finding. In this case it's an 'add' because the url is changing from Chrome: The back button doesn't work as I'd expect, but it's consistent with findings in the OP. I'll spec it so the back button changes the document, like we see from Firefox apart from the error case. |
PSA:
It seems we might have reached the conclusion to move toward Firefox behavior. If yes, it means restoring the PolicyContainer from history also makes some sense. Anyway, it seems this is already what Chrome and Firefox are doing: I initially wanted to align Chrome with the spec, but I am now believing I should align the spec/WPT with Chrome and Firefox. |
This is the current behavior of Chrome and Firefox: See WPT. https://wpt.fyi/results/content-security-policy/inheritance/iframe-srcdoc-history-inheritance.html?label=experimental&label=master&aligned I would like to update the specification and the WPT test to align with the current browsers behaviors. See: whatwg#6809
This is the current behavior of Chrome, and is that of Firefox for CSP. Part of #6809.
So what are all the behaviors this bug is covering?
I will work on some WPTs. If someone could help clarify the desired resolution for sandboxing that'd be great as well. |
See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e
Summary of the sandbox options: Option 1: The rules from the sandbox attr are used when navigating an iframePros: It's what browsers do now! Risk: If a developer loads an unsafe page in a sandboxed iframe, then removes sandbox and loads a safe page, it's possible through traversal that the unsafe page loads without a sandbox. Risk: If a page loads with a safe page in an iframe, then adds a sandbox to load an unsafe page, it's possible through iframe reparenting that the unsafe page loads without a sandbox. Risk: A page loads with an unsafe page in an iframe. It's possible through iframe reparenting, that the heuristics don't quite work, and the unsafe page ends up reparenting into a 'different' iframe without a sandbox. Risk: Inconsistent results if we start pulling iframe documents from bfcache. Option 2: The sandbox rules are stored in the history entryPros: Follows what we've decided to do with srcdoc and other policy state. Risk: It's a change in behaviour, so could be a compat issue. Risk: The developer sets a Option 3: History clearing and & reparenting failingThis solution has two parts:
Pros: Avoids pages loading with unexpected sandbox rules. Risk: It's a change in behaviour, so could be a compat issue. Maybe option 2 is the right balance of safe & simple? |
See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e
See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e
See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e
See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759061 Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Commit-Queue: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/main@{#1028593}
See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759061 Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Commit-Queue: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/main@{#1028593}
See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759061 Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Commit-Queue: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/main@{#1028593}
…actions, a=testonly Automatic update from web-platform-tests WPT: test srcdoc + session history interactions See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759061 Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Commit-Queue: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/main@{#1028593} -- wpt-commits: c78f07a33c46e6cf2f12183f47241f0f6275fe24 wpt-pr: 34843
See discussion in whatwg/html#6809. Bug: 1344417 Change-Id: Iec66ca607fab98d37fbaddba26c2428ab65bd70e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759061 Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Commit-Queue: Domenic Denicola <domenic@chromium.org> Cr-Commit-Position: refs/heads/main@{#1028593} NOKEYCHECK=True GitOrigin-RevId: 1d65f97bd467f8c82b2ad7db89e4694a9bab1e7f
This monster completely rewrites everything to do with navigation and traversal. It introduces the "navigable" and "traversable navigable" concepts, which take on many of the roles that browsing contexts previously did, but better. A navigable can present a sequence of browsing contexts, which to the user seem to all be the same, but due to browsing context group switches, have different WindowProxys and are allocated in different agent clusters. A traversable navigable manages the session history for itself and all its descendant navigables, providing a synchronization point and source of truth. The general flow of navigation and traversal is now geared toward creating a session history entry, populated with the appropriate document, before finally applying the history "step". The step concept for session history, managed by the traversable, replaces the previous idea of joint session history, which was a sort of deduplicated union of individual session histories for each browsing context within a top-level browsing context. Notable things we won't tackle this round, but are much easier to tackle in the future: - Iframe restoration on (non-bfcache) history traversal is not yet specified. - Overlapping navigations and traversals (see #6927) are not perfect yet, although this makes them better. - Browsing context names (see #313) are not perfect yet, although this makes them better. - Base URL inheritance and storage in session history (see #421, #2883, and #3989) is not yet specified. - Sandbox flag storage in session history (see #6809) is not yet specified. - Task queuing when creating agents/realms/windows/documents (see #8443) remains sketchy. - Window object reuse is not yet rationalized (see #3267). Closes #854 by clarifying the javascript: URL origin and origin-checking setup. Closes #1073 by properly resetting active-ness of documents when they are removed. Closes #1130 by removing the source browsing context concept, using a sourceDocument argument instead, and taking source snapshot params at the appropriate early time. Closes #1191 by properly sharing document state across documents, as well as overlapping same-document navigations plus cross-document traversals. Closes #1336 by properly handling child browsing contexts. Closes #1382 by only unloading after we are sure we have a new document (i.e., not a 204 or download). Closes #1454 by rewriting session history closer to what implementations do, with the nested history concept in particular taking care of the issues discussed there. Closes #1524 by introducing the POST data concept and storing it in the document state. Closes #2436 by rewriting the spec for history.go() to be clear about the results. Tests: web-platform-tests/wpt#36366. Closes #2566 by introducing an explicit "history object" definition. Tests: web-platform-tests/wpt#36367. Closes #2649 through clear creation of srcdoc documents, including during history traversal. Closes #3215 by preserving POST data and reusing it on reloads. Closes #3447 by specifying a precise mechanism (the ongoing navigation) for canceling navigations, and the points at which that mechanism is consulted. It also stops queuing a task for hyperlink navigations. Closes #3497 by posting appropriate tasks for cross-event-loop navigations. Closes #3615 by rewriting traverse a history by a delta, which eventually calls into apply the history step, to navigate all relevant navigables. Closes #3625 by storing information in the document state (not just the URL), so that future traversals can reconstruct the request appropriately. Closes #3730 by doing proper task queuing for navigation, including one for javascript: URLs but not including one for normal same-frame navigations. Tests: web-platform-tests/wpt#36358. Closes #3734 by rewriting the definition of script-closable to use well-defined concepts. Closes #3812 by removing all uses of "active document" as a predicate instead of a property. Closes #4054 by introducing the session history traversal queue and renaming the previous "history traversal task source" to "navigation and traversal task source". Closes #4121 by doing the "allowed to navigate" check at the top of apply the history step. Closes #4428 by keeping a strong reference from documents (including bfcached documents) to their containing browsing context. Closes #4782 by introducing the top-level traversable and navigable concepts. Closes #4838 by doing sandbox checking in a much more precise manner, in particular snapshotting the relevant flags early in any traversals. Closes #4852 by using document state (in particular history policy container, request referrer, and request referrer policy) in reloads. Closes #5103 by properly restoring scroll positions for everything that is traversed, as part of properly traversing more than one navigable. Closes #5350 by properly restoring window names across browsing context group switches, and going back to the same browsing context as was previously there when traversing back across a BCG switch boundary. (Implementations could create new browsing contexts, as long as they restore the WindowProxy scripting relationships and other browsing context features; the result is observably equivalent.) Closes #5597 by rewriting "allowed to download" to just take booleans, derived from the appropriate snapshotted or computed sandboxing flags. Closes #5767, modulo bugs and oversights we made, by rewriting everything :). Closes #5877 by re-specifying "fully active" in terms of navigables, instead of browsing contexts. Closes #6446 by properly firing beforeunload to all descendant navigables, although whether or not they actually prompt still allows implementation leeway. Closes #6483 by introducing the distinction between current session history entry and active session history entry. Closes #6514 by settling on using a single origin for these checks. Closes #6628 by storing window.name values in the document state, so even in strange splitting situations like described there, they remain. Closes #6652 by no longer changing history.state when reactivating a document from bfcache ("restore the history object state" is called only when documentsEntryChanged is true). Tests: web-platform-tests/wpt#36368. Closes #6773 by having careful handling of synchronous navigations during traversals. Test updates: web-platform-tests/wpt#36364. Closes #6798 by treating javascript: URL navigations as replacements. Works towards #6809 by storing srcdoc resources in the document state. Closes #6813 by storing referrer in the document state. Tests for the repopulation case: web-platform-tests/wpt#36352. (No tests yet for the reload case.) Closes #6947 by rolling its contents into this change: PDF documents are put in the same category as other inaccessible, no-DOM documents. Closes #7107 by clearing history state on redirects and when origin changes by other means, such as CSP. Closes #7441 by making window.blur() a no-op because that was simpler than updating it to operate on navigables. Closes #7722 by incorporating its contents into the rewritten version. Closes #8295 by refactoring the iframe/frame load event specs to avoid the bug. Helps with #8395 by at least ensuring the javascript: case does not fire beforeunload. Tests: web-platform-tests/wpt#36488. (The other cases remain open for investigation and testing.) Closes #8449 by exporting "create a fresh top-level traversable" which is designed for the use case in question. Co-authored-by: Domenic Denicola <d@domenic.me> Co-authored-by: Dominic Farolino <domfarolino@gmail.com>
I and @petervanderbeken happened to discuss about this and also prefer option 2 of those three options (and the way Firefox stores srcdoc). |
First up is a simple one: Does setting
srcdoc
replace the current history entry or add a new one?Chrome & Firefox: Replace if the current document url is
about:srcdoc
, otherwise add.Safari: Always replace.
I'm pretty sure Chrome & Firefox are doing the right thing here and it's somewhat spec'd.
How is a
srcdoc
entry stored in session history? It's currently unspec'd.Test:
This creates history entries
[srcdoc1, 'one', srcdoc2]
in Chrome/Firefox and[srcdoc1, srcdoc2]
in Safari.Each browser behaves differently when traversing to the srcdoc entries.
Chrome: Recreates the document using the
srcdoc
of the iframe. As insrcdoc1
andsrcdoc2
use the same source, which is different to the sourcesrcdoc1
used when it was first created.Firefox: Seems to store the
srcdoc
source along with the history entry, sosrcdoc1
andsrcdoc2
use different source, which is each the source they were created with.Safari: Restores the original history entries in these places, as if the
srcdoc
stuff never happened.Another test:
back()
Chrome: iframe empty. I guess this is because it looks at the
srcdoc
of the iframe which is empty, since the page has reloaded.Firefox: Restores the iframe to its previous content.
Safari: Restores the iframe as if setting
srcdoc
never happened.This seems consistent with the findings above. Firefox's model seems to make the most sense to me. Chrome's behaviour is ok I guess. Safari's behaviour is plain weird.
The text was updated successfully, but these errors were encountered: