fix(runtime-dom): ensure iframe sandbox is handled as an attribute to prevent unintended behavior #13950
Conversation
… prevent unintended behavior
WalkthroughTreats iframe Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor V as Virtual DOM
participant P as patchProp
participant S as shouldSetAsProp
participant D as DOM Element
V->>P: patchProp(el, "sandbox", prev, next, isSVG=false)
P->>S: shouldSetAsProp(el, "sandbox", isSVG)
S-->>P: false (force attribute path)
alt next is null or undefined
P->>D: removeAttribute("sandbox")
else next is empty string
P->>D: setAttribute("sandbox", "")
else next is non-empty
P->>D: setAttribute("sandbox", next)
end
note over P,D: property setter is not invoked
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (1)
packages/runtime-dom/__tests__/patchAttrs.spec.ts (1)
92-115: Good coverage; add two quick assertions to lock behavior.
- Add a case for literal string
"null"to ensure it’s preserved as an attribute value (not special‑cased).- Optionally assert behavior when a non‑string (e.g.,
false) is passed so expectations are explicit.// Verify sandbox is treated as attribute, not property patchProp(iframe, 'sandbox', null, 'allow-scripts') expect(iframe.getAttribute('sandbox')).toBe('allow-scripts') + // Literal string "null" should be preserved as-is + patchProp(iframe, 'sandbox', 'allow-scripts', 'null') + expect(iframe.getAttribute('sandbox')).toBe('null') + // Setting to null should remove the attribute patchProp(iframe, 'sandbox', 'allow-scripts', null) expect(iframe.hasAttribute('sandbox')).toBe(false) expect(iframe.getAttribute('sandbox')).toBe(null) // Setting to undefined should also remove the attribute patchProp(iframe, 'sandbox', null, 'allow-forms') expect(iframe.getAttribute('sandbox')).toBe('allow-forms') patchProp(iframe, 'sandbox', 'allow-forms', undefined) expect(iframe.hasAttribute('sandbox')).toBe(false) // Empty string should set empty attribute (most restrictive sandbox) patchProp(iframe, 'sandbox', null, '') expect(iframe.getAttribute('sandbox')).toBe('') expect(iframe.hasAttribute('sandbox')).toBe(true) + + // Optional: non-string value clarifies current stringification behavior + patchProp(iframe, 'sandbox', '', false as any) + expect(iframe.getAttribute('sandbox')).toBe('false')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime-dom/__tests__/patchAttrs.spec.ts(1 hunks)packages/runtime-dom/src/patchProp.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/__tests__/patchAttrs.spec.ts (1)
packages/runtime-dom/src/patchProp.ts (1)
patchProp(25-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
packages/runtime-dom/src/patchProp.ts (1)
114-119: Scope sandbox exception to<iframe>only
Force attribute binding only for<iframe>; preserve custom element property semantics.- if (key === 'sandbox') { + // Only meaningful for <iframe>; keep CE prop semantics intact. + if (key === 'sandbox' && el.tagName === 'IFRAME') { return false }
| // #13946 iframe.sandbox should always be set as attribute since setting | ||
| // the property to null results in 'null' string, and setting to empty string | ||
| // enables the most restrictive sandbox mode instead of no sandboxing. | ||
| if (key === 'sandbox') { |
There was a problem hiding this comment.
I think we should be checking el.tagName too.
It isn't checked for spellcheck, draggable, translate and autocorrect because they are global and can apply to any element.
For other special key values we do usually check the tagName.
form is an outlier. It did originally check the tagName, but that check was removed because it applied to so many tags that the bloat wasn't worth it, see 180310c.
I'm not entirely sure why we check the tagName. Presumably it's to reduce the risk of the workarounds leaking out and having unintended consequences.
Fix: Handle iframe sandbox attribute correctly when bound to nullish values
Problem Description
The
sandboxattribute on<iframe>elements was not being removed when bound to nullish values (nullorundefined) in Vue 3, causing incorrect sandboxing behavior. Instead of removing the attribute entirely, Vue was setting it to the string"null", which applies restrictive sandbox policies.Issue Details
<iframe>withsandboxattribute:sandbox="null"or:sandbox="undefined", the attribute was set to"null"string instead of being removedsandboxattribute should be completely removed when bound to nullish valuessandboxattribute was set to"null"string, enabling restrictive sandboxingRoot Cause Analysis
The issue stems from Vue's property vs attribute handling logic in
patchProp. Vue determines whether to treat a binding as a DOM property or HTML attribute using theshouldSetAsPropfunction:The
<iframe>element has asandboxDOM property of typestring. When Vue setsiframe.sandbox = null, JavaScript's type coercion convertsnullto the string"null", which then becomes the attribute value.Why This Is Problematic
sandbox="null"applies sandbox restrictions instead of no sandboxingsandboxattribute to mean no sandboxingSolution
Added
sandboxto the list of attributes that should always be handled as attributes rather than properties in theshouldSetAsPropfunction.Code Changes
packages/runtime-dom/src/patchProp.ts:
This ensures that:
sandboxis always handled through the attribute patching path (patchAttr)null/undefined) properly remove the attribute viael.removeAttribute()Why This Approach
This follows the same pattern used for other problematic attributes like
spellcheck,draggable,translate, andautocorrectthat have property/attribute handling conflicts.Testing
Added comprehensive tests covering all scenarios:
nullandundefinedcompletely remove the attributefalseand0remove the attributeImpact Assessment
Positive Impact
Related Issues
spellcheck,draggable,translateattributes that had property/attribute conflictsMigration Guide
No migration needed. This fix corrects incorrect behavior:
Before (incorrect):
After (correct):
Developers who were working around this issue by using explicit conditional rendering can now use nullish values directly:
Summary by CodeRabbit