From b853b18a24dd2d1c9408705b821cc11146ae1186 Mon Sep 17 00:00:00 2001 From: Dan Bjorge Date: Thu, 1 Feb 2024 09:24:00 -0500 Subject: [PATCH] fix: avoid reading element-specific node properties of non-element node types (#4317) Updates VirtualNode's `props` initialization path to avoid reading properties that we know based on `nodeType` won't be present anyway. This should mitigate #4316 by avoiding reading the problematic `value` prop present on certain text nodes. I also cleaned up some missing test coverage in the impacted code. Closes: #4316 --- lib/core/base/virtual-node/virtual-node.js | 29 ++++++------- test/core/base/virtual-node/virtual-node.js | 48 +++++++++++++++++---- 2 files changed, 51 insertions(+), 26 deletions(-) diff --git a/lib/core/base/virtual-node/virtual-node.js b/lib/core/base/virtual-node/virtual-node.js index 589b61237f..4a19c8b021 100644 --- a/lib/core/base/virtual-node/virtual-node.js +++ b/lib/core/base/virtual-node/virtual-node.js @@ -51,30 +51,25 @@ class VirtualNode extends AbstractVirtualNode { // add to the prototype so memory is shared across all virtual nodes get props() { if (!this._cache.hasOwnProperty('props')) { - const { - nodeType, - nodeName, - id, - multiple, - nodeValue, - value, - selected, - checked, - indeterminate - } = this.actualNode; + const { nodeType, nodeName, id, nodeValue } = this.actualNode; this._cache.props = { nodeType, nodeName: this._isXHTML ? nodeName : nodeName.toLowerCase(), id, type: this._type, - multiple, - nodeValue, - value, - selected, - checked, - indeterminate + nodeValue }; + + // We avoid reading these on node types where they won't be relevant + // to work around issues like #4316. + if (nodeType === 1) { + this._cache.props.multiple = this.actualNode.multiple; + this._cache.props.value = this.actualNode.value; + this._cache.props.selected = this.actualNode.selected; + this._cache.props.checked = this.actualNode.checked; + this._cache.props.indeterminate = this.actualNode.indeterminate; + } } return this._cache.props; diff --git a/test/core/base/virtual-node/virtual-node.js b/test/core/base/virtual-node/virtual-node.js index 771d3693b1..338442d75e 100644 --- a/test/core/base/virtual-node/virtual-node.js +++ b/test/core/base/virtual-node/virtual-node.js @@ -37,15 +37,45 @@ describe('VirtualNode', () => { assert.equal(vNode.props.type, 'text'); }); - it('should reflect selected property', () => { - node = document.createElement('option'); - let vNode = new VirtualNode(node); - assert.equal(vNode.props.selected, false); - - node.selected = true; - vNode = new VirtualNode(node); - assert.equal(vNode.props.selected, true); - }); + for (const [prop, tagName, examplePropValue] of [ + ['value', 'input', 'test value'], + ['selected', 'option', true], + ['checked', 'input', true], + ['indeterminate', 'input', true], + ['multiple', 'select', true] + ]) { + describe(`props.${prop}`, () => { + it(`should reflect a ${tagName} element's ${prop} property`, () => { + node = document.createElement(tagName); + let vNode = new VirtualNode(node); + assert.equal(vNode.props[prop], ''); + + node[prop] = examplePropValue; + vNode = new VirtualNode(node); + assert.equal(vNode.props[prop], examplePropValue); + }); + + it('should be undefined for a text node', () => { + node = document.createTextNode('text content'); + let vNode = new VirtualNode(node); + assert.equal(vNode.props[prop], undefined); + }); + + // Regression test for #4316 + it(`should be resilient to text node with un-gettable ${prop} property`, () => { + node = document.createTextNode('text content'); + Object.defineProperty(node, prop, { + get() { + throw new Error('Unqueryable value'); + } + }); + let vNode = new VirtualNode(node); + assert.throws(() => node[prop]); + assert.doesNotThrow(() => vNode.props[prop]); + assert.equal(vNode.props[prop], undefined); + }); + }); + } it('should lowercase type', () => { node = document.createElement('input');