Skip to content

Components: Add allowForms prop to SandBox component - #76471

Open
dhasilva wants to merge 4 commits into
WordPress:trunkfrom
dhasilva:fix/components-sandbox-allow-forms
Open

Components: Add allowForms prop to SandBox component#76471
dhasilva wants to merge 4 commits into
WordPress:trunkfrom
dhasilva:fix/components-sandbox-allow-forms

Conversation

@dhasilva

Copy link
Copy Markdown
Contributor

What?

Closes #48942

Why?

Currently there is no way to allow forms in the Sandbox component, so in cases where that is necessary, like submitting your date of birth to view age-restricted content, the submission is blocked.

How?

By adding an allowForms prop to the SandBox component that includes allow-forms to the iframe, making it possible to allow it selectively.

Testing Instructions

Storybook can be used for that.
1- Run storybook with npm run storybook:dev
2 - Go to Components > Utilities > SandBox > With Form
3 - Check that you can submit the form
4 - Disable allowForms and reload
5 - Check that you can't submit the form

Testing Instructions for Keyboard

Screenshots or screencast

Before After

Use of AI Tools

Claude Code was used to generate tests.

@dhasilva
dhasilva requested review from a team and ajitbohra as code owners March 12, 2026 22:49
@github-actions

Copy link
Copy Markdown

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Required label: Any label starting with [Type].
  • Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

@github-actions

github-actions Bot commented Mar 12, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: dhasilva <thehenridev@git.wordpress.org>
Co-authored-by: aduth <aduth@git.wordpress.org>
Co-authored-by: CGastrell <cgastrell@git.wordpress.org>
Co-authored-by: simison <simison@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions Bot added the [Package] Components /packages/components label Mar 12, 2026
@simison
simison requested review from ciampo and mirka March 13, 2026 06:21
@aduth

aduth commented Mar 13, 2026

Copy link
Copy Markdown
Member

I think I'd like to understand why the existing sandbox default values allow-scripts allow-same-origin allow-presentation were selected, and whether allow-forms is a reasonable inclusion in that default set aligned with the original implementation, rather than adding more options for individual sandbox properties. Arguably those (especially allow-scripts) are a lot more "risky" than allowing form submissions.

@dhasilva

Copy link
Copy Markdown
Contributor Author

I think I'd like to understand why the existing sandbox default values allow-scripts allow-same-origin allow-presentation were selected, and whether allow-forms is a reasonable inclusion in that default set aligned with the original implementation, rather than adding more options for individual sandbox properties. Arguably those (especially allow-scripts) are a lot more "risky" than allowing form submissions.

That is a good point. The prop idea came from this comment in the issue, though reading it again I'm not sure if by "it" @talldan meant the "allow-forms" specifically, or all values.

@CGastrell CGastrell added the [Type] Task Issues or PRs that have been broken down into an individual action to take label Mar 23, 2026
@CGastrell

Copy link
Copy Markdown
Contributor

Seems safe:

  1. Opt-in only — allowForms defaults to false, so no existing behavior changes. All current consumers of SandBox are unaffected.
  2. allow-forms is lower risk than what's already allowed — The sandbox already grants allow-scripts and allow-same-origin, which are significantly more powerful capabilities. allow-forms just lets form elements submit; it doesn't grant script execution or cross-origin access that isn't already present.
  3. Tests are solid — Both the positive and negative cases are covered.

Issues / suggestions

  1. Trailing space in sandbox attribute — When allowForms is false, the template literal produces a trailing space: allow-scripts allow-same-origin allow-presentation ${false ? 'allow-forms' : ''}
  2. yields "allow-scripts allow-same-origin allow-presentation ". This is functionally harmless (browsers trim it), but it's a bit sloppy. The test uses expect.not.stringContaining('allow-forms') which passes despite the trailing space, so it works — but a .trim() or building an array and joining would be cleaner.
  3. @aduth's raises a good point: given that allow-scripts and allow-same-origin are already in the default set (and are more dangerous), should allow-forms just be included by default rather than requiring a new prop? This would be simpler and avoid prop proliferation. The counter-argument is that the prop approach is more conservative and gives consumers explicit control. Both are reasonable, but this should be considered/addressed before merging.
  4. Minor: The story form's action="#" will cause a navigation inside the iframe on submit, which is fine for demo purposes but could be confusing. A would make the demo cleaner.

The implementation seems correct and safe and tests well — it's a small, well-scoped, backwards-compatible addition. The main open question is what @aduth raised: prop vs. default. That's a project-level API design decision, not an actual issue. The trailing whitespace is a minor nit.

@simison

simison commented Jul 28, 2026

Copy link
Copy Markdown
Member

Is this still needed/relevant?

We've recently added "allow popups" prop so there's pre-existing pattern.

@aduth

aduth commented Jul 28, 2026

Copy link
Copy Markdown
Member

Thanks for flagging @simison . It feels like there's parallel conversations happening between here and #69617, and that both pull requests are basically doing the same thing for different aspects of the sandbox.

I don't feel like it was ever really settled whether these make sense as defaults or if we should just open up the entire sandbox attribute, rather than do this piecemeal.

cc @Mamaduka @obenland @Imran92 @jsnajdr @mirka as having participated in that other pull request.

@Mamaduka

Copy link
Copy Markdown
Member

Thanks for the ping. This would require rebasing after #69617.

Now it's worth deciding on the format we plan to support new allow-* tokens for the Sandbox component. Are we going with the current direction and require a new prop for each case, or allow consumers to pass tokens as needed, while retaining the required ones?

@jsnajdr

jsnajdr commented Jul 29, 2026

Copy link
Copy Markdown
Member

Adding another allowForms prop looks fine to me. Simple and ergonomic.

Both allowForms and allowPopups are being added to fix the VideoPress block embed. Allow users to open links in a new tab, and submit an age-verification form.

That raises a question: should we enable these capabilities by default? The Sandbox component is used by the Cover and Embed blocks to display embed preview markup loaded from the generic /oembed/1.0/proxy endpoint. What if one of the registered embeds starts returning markup that is interactive enough to require these sandbox capabilities? Then we'll have to update the generic usages of Sandbox anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Components /packages/components [Type] Task Issues or PRs that have been broken down into an individual action to take

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support allow-forms in SandBox component

6 participants