feat: implement useIframeSandbox#9949
Conversation
🦋 Changeset detectedLatest commit: d390106 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
A-Project
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded a nursery lint rule Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_html_analyze/src/lint/nursery/use_iframe_sandbox.rs`:
- Around line 52-55: The diagnostic currently only triggers when a sandbox
attribute exists but has no value; update the logic in use_iframe_sandbox (where
is_iframe_element(...) and element.find_attribute_by_name("sandbox") are used)
to also handle the missing-sandbox case: treat
element.find_attribute_by_name("sandbox") returning None as a failing case and
emit the same diagnostic for <iframe></iframe> as for sandbox="" (i.e., if no
sandbox attribute OR sandbox_attribute.value().is_none(), create the
diagnostic). Ensure you adjust the conditional and any references to
sandbox_attribute accordingly so both missing and empty sandbox attributes are
reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65e63434-84e1-44c6-9502-84c9864aebec
⛔ Files ignored due to path filters (7)
crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/valid.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (9)
.changeset/fluffy-ways-punch.mdcrates/biome_html_analyze/src/lint/nursery/use_iframe_sandbox.rscrates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/valid.htmlcrates/biome_js_analyze/src/lint/nursery/use_iframe_sandbox.rscrates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/invalid.jsxcrates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/valid.jsxcrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_iframe_sandbox.rs
Merging this PR will not alter performance
Comparing 
Footnotes
|
| language: "html", | ||
| recommended: false, | ||
| severity: Severity::Warning, | ||
| sources: &[RuleSource::EslintReactDom("no-missing-iframe-sandbox").same(), RuleSource::EslintReactXyz("dom-no-missing-iframe-sandbox").same()], |
There was a problem hiding this comment.
kinda unrelated, but do you think we could have a unit test or something to flag rules that don't have the correct rule sources? specifically for this EslintReactXyz plugin.
basically, we should be able to flag in a unit test or something:
- rule has a EslintReactXyz source, but no corresponding "sub-source" like EslintReactDom (if applicable)
- vice versa, rule has the "sub-source" like EslintReactDom, but no corresponding EslintReactXyz (always applicable)
The reason I'm asking is because things that aren't enforced automatically degrade over time. Something to think about for another PR.
There was a problem hiding this comment.
Was thinking about that, if we can somehow lint that, could you point me into the right direction on where this kind of linting happens?
There was a problem hiding this comment.
I haven't looked at it too closely yet so not precisely. But we would make these assertions through unit tests, not via linting. Similar to how we have a bunch of unit tests that assert certain lists are sorted so binary searches work.
We would probably place these unit tests in the biome_js_analyze crate. Probably the biome_analyze::RegistryVisitor trait would be involved to iterate over rules? Again, not sure, haven't looked at it closely, but maybe that could help you look into it.
There was a problem hiding this comment.
Alright, will check that out in another PR
| rule_category!(), | ||
| node.range(), | ||
| markup! { | ||
| "Provide a "<Emphasis>"sandbox"</Emphasis>" attribute when using "<Emphasis>"iframe"</Emphasis>" elements." |
There was a problem hiding this comment.
Rule pillars. this message conflates the "what" with the "how to fix"
first message needs to describe what the error is. In this case, something like "this iframe is missing sandbox"
There was a problem hiding this comment.
Yup, wasn't really happy about it either. Hence the draft 😅
| /// - In component-based frameworks, only lowercase `iframe` is matched to avoid flagging custom components like `<Iframe>`. | ||
| fn is_iframe_element(element: &AnyHtmlElement, file_extension: &str) -> bool { | ||
| element.name().is_some_and(|token_text| { | ||
| let is_html_file = file_extension == "html"; |
There was a problem hiding this comment.
you should be using the HtmlFileSource to determine this
There was a problem hiding this comment.
Ahh okay, built this off of useIframeTitle
There was a problem hiding this comment.
oh, well that sucks, we should probably fix that.
There was a problem hiding this comment.
Tackled that one too, will extract that function into a more abstract way to also be used by other tags
Netail
force-pushed
the
feat/use-iframe-sandbox
branch
from
b187ba7 to
ff31004
Compare
| fn is_iframe_element(element: &AnyHtmlElement, ctx: &RuleContext<UseIframeTitle>) -> bool { | ||
| let Some(element_name) = element.name() else { | ||
| return false; | ||
| }; | ||
|
|
||
| let source_type = ctx.source_type::<HtmlFileSource>(); | ||
|
|
||
| // In HTML files: case-insensitive (IFRAME, Iframe, iframe all match) | ||
| // In component frameworks (Vue, Svelte, Astro): case-sensitive (only "iframe" matches) | ||
| // This means <Iframe> in Vue/Svelte is treated as a component and ignored | ||
| if source_type.is_html() { | ||
| element_name.text().eq_ignore_ascii_case("iframe") | ||
| } else { | ||
| element_name.text() == "iframe" | ||
| } | ||
| } |
There was a problem hiding this comment.
Will extract this function in a different PR, because it saw more components that could use this. But it will be a bit much for this PR
| fn is_iframe_element(element: &AnyHtmlElement, ctx: &RuleContext<UseIframeSandbox>) -> bool { | ||
| let Some(element_name) = element.name() else { | ||
| return false; | ||
| }; | ||
|
|
||
| let source_type = ctx.source_type::<HtmlFileSource>(); | ||
|
|
||
| // In HTML files: case-insensitive (IFRAME, Iframe, iframe all match) | ||
| // In component frameworks (Vue, Svelte, Astro): case-sensitive (only "iframe" matches) | ||
| // This means <Iframe> in Vue/Svelte is treated as a component and ignored | ||
| if source_type.is_html() { | ||
| element_name.text().eq_ignore_ascii_case("iframe") | ||
| } else { | ||
| element_name.text() == "iframe" | ||
| } | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/invalid.html`:
- Line 3: The rule implementation for dom-no-missing-iframe-sandbox is
incorrectly rejecting a bare sandbox attribute because it treats attributes with
value().is_none() as missing; update the rule check to treat a bare sandbox
(attribute present with no value) as equivalent to sandbox="" (i.e., accept when
the attribute exists even if value().is_none()), or alternatively change the
rule metadata/source to document/enforce stricter semantics—modify the condition
that currently rejects attributes with value().is_none() so that presence of the
sandbox attribute passes validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3975744-915c-4e44-9452-771e58b60359
⛔ Files ignored due to path filters (10)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (10)
.changeset/fluffy-ways-punch.mdcrates/biome_html_analyze/src/lint/a11y/use_iframe_title.rscrates/biome_html_analyze/src/lint/nursery/use_iframe_sandbox.rscrates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/useIframeSandbox/valid.htmlcrates/biome_js_analyze/src/lint/nursery/use_iframe_sandbox.rscrates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/invalid.jsxcrates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/valid.jsxcrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_iframe_sandbox.rs
✅ Files skipped from review due to trivial changes (5)
- .changeset/fluffy-ways-punch.md
- crates/biome_rule_options/src/lib.rs
- crates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/invalid.jsx
- crates/biome_js_analyze/tests/specs/nursery/useIframeSandbox/valid.jsx
- crates/biome_rule_options/src/use_iframe_sandbox.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_js_analyze/src/lint/nursery/use_iframe_sandbox.rs
- crates/biome_html_analyze/src/lint/nursery/use_iframe_sandbox.rs
Netail
force-pushed
the
feat/use-iframe-sandbox
branch
from
1b7b095 to
4af974e
Compare
Netail
force-pushed
the
feat/use-iframe-sandbox
branch
from
4af974e to
c056dad
Compare
3 participants
Summary
Port https://www.eslint-react.xyz/docs/rules/dom-no-missing-iframe-sandbox, which enforces setting the sandbox attribute on iframe tags
Test Plan
Unit tests
Docs