Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions packages/runtime-dom/__tests__/patchAttrs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,38 @@ describe('runtime-dom: attrs patching', () => {
expect(el2.dataset.test).toBe(undefined)
expect(testvalue).toBe(obj)
})

// #13946
test('sandbox should be handled as attribute even if property exists', () => {
const iframe = document.createElement('iframe') as any
let propSetCount = 0
// simulate sandbox property in jsdom environment
Object.defineProperty(iframe, 'sandbox', {
configurable: true,
enumerable: true,
get() {
return this._sandbox
},
set(v) {
propSetCount++
this._sandbox = v
},
})

patchProp(iframe, 'sandbox', null, 'allow-scripts')
expect(iframe.getAttribute('sandbox')).toBe('allow-scripts')
expect(propSetCount).toBe(0)

patchProp(iframe, 'sandbox', 'allow-scripts', null)
expect(iframe.hasAttribute('sandbox')).toBe(false)
expect(iframe.getAttribute('sandbox')).toBe(null)
expect(propSetCount).toBe(0)

patchProp(iframe, 'sandbox', null, '')
expect(iframe.getAttribute('sandbox')).toBe('')
expect(iframe.hasAttribute('sandbox')).toBe(true)
expect(propSetCount).toBe(0)

delete iframe.sandbox
})
})
7 changes: 7 additions & 0 deletions packages/runtime-dom/src/patchProp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ function shouldSetAsProp(
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') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@dejour dejour Sep 30, 2025

Choose a reason for hiding this comment

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

agree

return false
}

// #1787, #2840 form property on form elements is readonly and must be set as
// attribute.
if (key === 'form') {
Expand Down