Skip to content
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

Adds clarifying note to using sandbox with iframe tag #31164

Merged
merged 5 commits into from
Jan 2, 2024

Conversation

Tenkir
Copy link
Contributor

@Tenkir Tenkir commented Dec 19, 2023

Description

Adds a note to explain the behavior of the iframe sandbox attribute when opening auxiliary windows.

Motivation

We encountered an issue where we where not able to submit forms in a page linked to from an iframe. We only discovered this behavior with the sandbox attribute after finding a gist from Google demoing the changes adding allow-popups-to-escape-sandbox flag.

Additional details

whatwg proposal

@Tenkir Tenkir requested a review from a team as a code owner December 19, 2023 22:20
@Tenkir Tenkir requested review from chrisdavidmills and removed request for a team December 19, 2023 22:20
@github-actions github-actions bot added the Content:HTML Hypertext Markup Language docs label Dec 19, 2023
Copy link
Contributor

github-actions bot commented Dec 19, 2023

Preview URLs

(comment last updated: 2024-01-02 17:17:22)

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Hi @Tenkir, thanks a lot for your contribution to MDN, much appreciated.

I think this is a good point to draw attention to, although I did have some thoughts on how to present the information:

  1. More a cosmetic detail — since this note only contains one point, you can put it all on the same line, i.e.

    Note: When opening a link from an embedded page with the sandbox attribute, the Auxiliary Window is restricted to the same sandbox values unless allow-popups-to-escape-sandbox is included.

  2. To be clear, we are specifically talking about the sandboxed behavior of new tabs or windows spawned from inside an iframe, right? Either created by following a basic link, or by completing an action that invokes an open() call that does the same thing via JS? I'd probably rephrase in that case, as "opening a link" is a bit ambiguous.
  3. "Auxiliary Window" sounds a bit spec-language and potentially confusing. Maybe just "popup window or new tab", assuming my statement above is correct?
  4. I also thought about the placement of your note. I think it would make sense to move the note to the "Scripting" section of the page, make it specifically about disallowing scripting being an issue, and give your form problem as an example. I'm sure most of the issues people have with this are due to scripting being disallowed in the newly spawned window, and then wondering why, so the "Scripting" section would be a good focal point for people to find the information.
  5. Then you could also update the "allow-popups-to-escape-sandbox" description earlier in the page to something a bit more useful, as currently it is a bit value and doesn't really give a good use case. How about:

"Allows a sandboxed document to open new windows or tabs without forcing the same sandboxing restrictions upon them. This will allow, for example, a third-party advertisement to be safely sandboxed without blocking functionality in the page the ad links to, or form contained in a popup window spawned from a sandboxed document that disallows scripting to be submitted via JavaScript."

Let me know what you think of my suggestions.

@Tenkir
Copy link
Contributor Author

Tenkir commented Dec 20, 2023

Hi @chrisdavidmills , thanks for the quick response!

To be clear, we are specifically talking about the sandboxed behavior of new tabs or windows spawned from inside an iframe, right? Either created by following a basic link, or by completing an action that invokes an open() call that does the same thing via JS? I'd probably rephrase in that case, as "opening a link" is a bit ambiguous.

Yes, though I believe it will also apply if the current page is redirected within the same tab. Perhaps "redirecting the user, opening a popup window, or opening a new tab"

"Auxiliary Window" sounds a bit spec-language and potentially confusing. Maybe just "popup window or new tab", assuming my statement above is correct?

Agreed.

I also thought about the placement of your note. I think it would make sense to move the note to the "Scripting" section of the page, make it specifically about disallowing scripting being an issue, and give your form problem as an example. I'm sure most of the issues people have with this are due to scripting being disallowed in the newly spawned window, and then wondering why, so the "Scripting" section would be a good focal point for people to find the information.

I would hesitate to locate this note here, as it can pertain to more than just scripting. The restriction applies to all sandbox flags, not just those pertaining to scripting. For example, the allow-forms flag will block the submission of pure HTML forms as well.

My thought of locating it with the sandbox section is that, if a developer is utilizing the sandbox attribute, they would be most likely to be in this section, where the note is most relevant. Though I can see the value of locating it somewhere where a developer of a destination page who is not also developing the source iframe would find it.

Then you could also update the "allow-popups-to-escape-sandbox" description earlier in the page to something a bit more useful, as currently it is a bit value and doesn't really give a good use case. How about:

This makes sense, I can update that language.

@chrisdavidmills
Copy link
Contributor

Hi @chrisdavidmills , thanks for the quick response!

To be clear, we are specifically talking about the sandboxed behavior of new tabs or windows spawned from inside an iframe, right? Either created by following a basic link, or by completing an action that invokes an open() call that does the same thing via JS? I'd probably rephrase in that case, as "opening a link" is a bit ambiguous.

Yes, though I believe it will also apply if the current page is redirected within the same tab. Perhaps "redirecting the user, opening a popup window, or opening a new tab"

@Tenkir yup, makes perfect sense.

I also thought about the placement of your note. I think it would make sense to move the note to the "Scripting" section of the page, make it specifically about disallowing scripting being an issue, and give your form problem as an example. I'm sure most of the issues people have with this are due to scripting being disallowed in the newly spawned window, and then wondering why, so the "Scripting" section would be a good focal point for people to find the information.

I would hesitate to locate this note here, as it can pertain to more than just scripting. The restriction applies to all sandbox flags, not just those pertaining to scripting. For example, the allow-forms flag will block the submission of pure HTML forms as well.

OK, fair point. In that case, maybe expand your content a little bit to talk about the real-world effects on popup/redirect/new tab content. I'm thinking that we want to make sure that it doesn't just basically repeat the contents of the allow-popups-to-escape-sandbox value description. Maybe something along the lines of:

Note: When a popup or new tab is opened from content embedded in a <iframe>, or that content redirects the user, that popup/new location will have the same sandbox privileges as the original <iframe>. This can create problems such as ...(description). For example (give form example). To get around these problems, include the allow-popups-to-escape-sandbox value in your list of sandbox values.

Hastily written, with placeholders, but you get the idea.

My thought of locating it with the sandbox section is that, if a developer is utilizing the sandbox attribute, they would be most likely to be in this section, where the note is most relevant. Though I can see the value of locating it somewhere where a developer of a destination page who is not also developing the source iframe would find it.

Then you could also update the "allow-popups-to-escape-sandbox" description earlier in the page to something a bit more useful, as currently it is a bit value and doesn't really give a good use case. How about:

This makes sense, I can update that language.

Cool

@Tenkir Tenkir requested review from mdn-bot and a team as code owners December 22, 2023 22:07
@Tenkir Tenkir requested a review from a team December 22, 2023 22:07
@Tenkir Tenkir requested review from a team as code owners December 22, 2023 22:07
@Tenkir Tenkir requested review from Ryuno-Ki, sideshowbarker, teoli2003 and scottaohara and removed request for a team December 22, 2023 22:07
@github-actions github-actions bot removed Content:SVG SVG docs Content:Media Media docs Content:Glossary Glossary entries Content:Games Games docs Content:Performance Web performance docs Content:Guide Guide docs Content:Security Security docs Content:Learn:Client-side Content under “Client-side JavaScript frameworks” (Svelte, React, Angular, Vue) and related subtrees Content:Firefox Content in the Mozilla/Firefox subtree Content:Learn:Cross-Browser-Testing Learning area Cross-Browser-Testing docs system [PR only] Infrastructure and configuration for the project labels Dec 22, 2023
@Tenkir
Copy link
Contributor Author

Tenkir commented Dec 22, 2023

@chrisdavidmills Had a bit of a kerfuffle with your merging the upstream (as I typically rebase my commits for a cleaner git history). But it should be resolved now and ready for review.

I don't seem to be able to remove the automatically added extraneous reviewers however.

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Hi there @Tenkir, thanks for the most recent updates you made, and sorry it took me so long to respond to this. I think the placement of the new content is great; I just had some new wording suggestions for you to go over. Nearly there now!

@@ -104,7 +104,7 @@ This element includes the [global attributes](/en-US/docs/Web/HTML/Global_attrib
- `allow-popups`
- : Allows popups (like from {{domxref("Window.open()")}}, `target="_blank"`, {{domxref("Window.showModalDialog()")}}). If this keyword is not used, that functionality will silently fail.
- `allow-popups-to-escape-sandbox`
- : Allows a sandboxed document to open new windows without forcing the sandboxing flags upon them. This will allow, for example, a third-party advertisement to be safely sandboxed without forcing the same restrictions upon the page the ad links to.
- : Allows a sandboxed document to open new windows without forcing the sandboxing flags upon them. This will allow, for example, a third-party advertisement to be safely sandboxed without forcing the same restrictions upon the page the ad links to. If this flag is not included, a redirected page, popup window, or new tab will be limited to the same sandbox restrictions as the originating `<iframe>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- : Allows a sandboxed document to open new windows without forcing the sandboxing flags upon them. This will allow, for example, a third-party advertisement to be safely sandboxed without forcing the same restrictions upon the page the ad links to. If this flag is not included, a redirected page, popup window, or new tab will be limited to the same sandbox restrictions as the originating `<iframe>`.
- : Allows a sandboxed document to open a new browsing context without forcing the sandboxing flags upon it. This will allow, for example, a third-party advertisement to be safely sandboxed without forcing the same restrictions upon the page the ad links to. If this flag is not included, a redirected page, popup window, or new tab will be subject to the same sandbox restrictions as the originating `<iframe>`.

@@ -125,6 +125,8 @@ This element includes the [global attributes](/en-US/docs/Web/HTML/Global_attrib
> - When the embedded document has the same origin as the embedding page, it is **strongly discouraged** to use both `allow-scripts` and `allow-same-origin`, as that lets the embedded document remove the `sandbox` attribute — making it no more secure than not using the `sandbox` attribute at all.
> - Sandboxing is useless if the attacker can display content outside a sandboxed `iframe` — such as if the viewer opens the frame in a new tab. Such content should be also served from a _separate origin_ to limit potential damage.

> **Note:** When redirecting the user, opening a popup window, or opening a new tab from an embedded page within an `<iframe>` with the `sandbox` attribute, the new page, popup window or new tab is restricted to the same `sandbox` values. This can create issues, for example, if an embedded page within an `<iframe>` with attribute `sandbox="allow-scripts"` opens a new site in a seperate tab, form submission in that new tab will silently fail. To get around these problems, either include the `allow-popups-to-escape-sandbox` value, or the applicable `sandbox` value to enable the desired behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> **Note:** When redirecting the user, opening a popup window, or opening a new tab from an embedded page within an `<iframe>` with the `sandbox` attribute, the new page, popup window or new tab is restricted to the same `sandbox` values. This can create issues, for example, if an embedded page within an `<iframe>` with attribute `sandbox="allow-scripts"` opens a new site in a seperate tab, form submission in that new tab will silently fail. To get around these problems, either include the `allow-popups-to-escape-sandbox` value, or the applicable `sandbox` value to enable the desired behavior.
> **Note:** When redirecting the user, opening a popup window, or opening a new tab from an embedded page within an `<iframe>` with the `sandbox` attribute, the new browsing context is subject to the same sandbox restrictions. This can create issuesfor example, if a page embedded within an `<iframe>` with a `sandbox="allow-scripts"` attribute set on it opens a new site in a separate tab, script-based functionality such as form submission will silently fail in that new tab. To get around these problems, include the `allow-popups-to-escape-sandbox` flag in the `<iframe>`'s `sandbox` attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the second option you included in the workarounds sentence — "or include the applicable sandbox value to enable the desired behavior" — as I wasn't sure where you'd include this in the new browsing context content. Do you mean include it as part of a content security policy, or something else? Feel free to add it back in, provided you clarify it.

Copy link
Contributor Author

@Tenkir Tenkir Jan 2, 2024

Choose a reason for hiding this comment

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

To clarify, I mean add the appropriate flag to the sandbox attribute to enable the desired behavior.

script-based functionality such as form submission will silently fail in that new tab.

More accurately, form-based functionality will silently fail in that new tab.

This doesn't necessarily have anything to do with scripts. I elected to use the form example, since not including the allow-forms flag will cause all HTML form elements to be blocked, whether they use JS in their submit handler or not.

For example, let's say I have an iframe with allow-scripts. This links to a new page where I have a pure HTML form element that does not use javascript. This form will be blocked. Additionally, using javascript in the submit handler will still be blocked, since allow-forms was not set, all form submission is blocked. It's entirely separate from the script-based qualifier.

I can update some language here to clarify 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.

Proposed language:

This can create issues — for example, if a page embedded within an <iframe> without a sandbox="allow-forms" or sandbox="allow-popups-to-escape-sandbox" attribute set on it opens a new site in a separate tab, form submission in that new browsing context will silently fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I understand now — yup, the proposed language looks fine.

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Great, thanks @Tenkir! Let's get this merged.

@chrisdavidmills chrisdavidmills merged commit 3dac1a1 into mdn:main Jan 2, 2024
7 checks passed
dipikabh pushed a commit to dipikabh/content that referenced this pull request Jan 17, 2024
* Add clarifying note to using sandbox with iframe tag

* Clarifying language around sandbox attribute pitfalls

---------

Co-authored-by: Chris Mills <chrisdavidmills@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:HTML Hypertext Markup Language docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants