From e190e81931c7293cc5302d33bcbe4effb2bf6e94 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 30 Apr 2018 19:52:23 -0400 Subject: [PATCH] Password inputs do not synchronize the value attribute In order to prevent passwords from showing up in the markup React generates, this commit adds exceptions for password inputs such that defaultValue synchronization is omitted. When rendered server-side, password inputs will not send markup down from the server, however the value attribute is restored upon hydration. This is probably a design decision that we should clamp down. --- .../src/__tests__/ReactDOMInput-test.js | 34 +++++++++++ .../ReactDOMServerIntegrationForms-test.js | 58 +++++++++++++++++++ .../src/client/ReactDOMFiberInput.js | 32 ++++++---- .../src/server/ReactPartialRenderer.js | 8 +++ 4 files changed, 121 insertions(+), 11 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 03eed7bc1ec65..30ca0f5e8bdaa 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1433,6 +1433,40 @@ describe('ReactDOMInput', () => { expect(node.getAttribute('value')).toBe('2'); }); + it('does not set the value attribute on password inputs', () => { + const Input = getTestInput(); + const stub = ReactTestUtils.renderIntoDocument( + , + ); + const node = ReactDOM.findDOMNode(stub); + + expect(node.hasAttribute('value')).toBe(false); + expect(node.value).toBe('1'); + }); + + it('does not update the value attribute on password inputs', () => { + const Input = getTestInput(); + const stub = ReactTestUtils.renderIntoDocument( + , + ); + const node = ReactDOM.findDOMNode(stub); + + ReactTestUtils.Simulate.change(node, {target: {value: '2'}}); + + expect(node.hasAttribute('value')).toBe(false); + expect(node.value).toBe('2'); + }); + + it('does not set the defaultValue attribute on password inputs', () => { + const stub = ReactTestUtils.renderIntoDocument( + , + ); + const node = ReactDOM.findDOMNode(stub); + + expect(node.hasAttribute('value')).toBe(false); + expect(node.value).toBe('1'); + }); + it('does not set the value attribute on number inputs if focused', () => { const Input = getTestInput(); const stub = ReactTestUtils.renderIntoDocument( diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationForms-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationForms-test.js index fc4594261eba2..83a008e0f04f3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationForms-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationForms-test.js @@ -96,6 +96,50 @@ describe('ReactDOMServerIntegration', () => { expect(e.getAttribute('defaultValue')).toBe(null); }, ); + + itClientRenders( + 'a password input hydrates client-side with the value prop', + async render => { + const e = await render( + , + 0, + ); + + expect(e.value).toBe('foo'); + expect(e.hasAttribute('value')).toBe(false); + }, + ); + + itClientRenders( + 'a password input hydrates client-side with the defaultValue prop', + async render => { + const e = await render( + , + 0, + ); + + expect(e.value).toBe('foo'); + expect(e.hasAttribute('value')).toBe(false); + }, + ); + + it('will not render the value prop server-side', async () => { + const e = await serverRender( + , + ); + + expect(e.value).toBe(''); + expect(e.hasAttribute('value')).toBe(false); + }); + + it('will not render the defaultValue prop server-side', async () => { + const e = await serverRender( + , + ); + + expect(e.value).toBe(''); + expect(e.hasAttribute('value')).toBe(false); + }); }); describe('checkboxes', function() { @@ -540,6 +584,20 @@ describe('ReactDOMServerIntegration', () => { , )); + it('should not blow away user-entered text on successful reconnect to an uncontrolled password input', () => + testUserInteractionBeforeClientRender( + , + '', + 'Hello', + )); + + it('should not blow away user-entered text on successful reconnect to an controlled password input', () => + testUserInteractionBeforeClientRender( + , + '', + 'Hello', + )); + it('should not blow away user-entered text on successful reconnect to a controlled input', async () => { let changeCount = 0; await testUserInteractionBeforeClientRender( diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index fd5a574fec377..28c5d7e87db62 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -218,7 +218,9 @@ export function postMountWrapper(element: Element, props: Object) { // value must be assigned before defaultValue. This fixes an issue where the // visually displayed value of date inputs disappears on mobile Safari and Chrome: // https://github.com/facebook/react/issues/7233 - node.defaultValue = '' + node._wrapperState.initialValue; + if (props.type !== 'password') { + node.defaultValue = '' + node._wrapperState.initialValue; + } } // Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug @@ -304,16 +306,24 @@ export function setDefaultValue( type: ?string, value: *, ) { - if ( - // Focused number inputs synchronize on blur. See ChangeEventPlugin.js - type !== 'number' || - node.ownerDocument.activeElement !== node - ) { - if (value == null) { - node.defaultValue = '' + node._wrapperState.initialValue; - } else if (node.defaultValue !== '' + value) { - node.defaultValue = '' + value; - } + if (type === 'password') { + // Do not synchronize password inputs to prevent password from + // being exposed in markup + // https://github.com/facebook/react/issues/11896 + return; + } + + // Only assign the value attribute on number inputs when they are not selected + // This avoids edge cases in Chrome. Number inputs are synchronized on blur. + // See ChangeEventPlugin.js + if (type === 'number' && node.ownerDocument.activeElement === node) { + return; + } + + if (value == null) { + node.defaultValue = '' + node._wrapperState.initialValue; + } else if (node.defaultValue !== '' + value) { + node.defaultValue = '' + value; } } diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index ec26eba2c1d27..062a23691e698 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -326,6 +326,7 @@ function createOpenTagMarkup( isRootElement: boolean, ): string { let ret = '<' + tagVerbatim; + const isPasswordInput = tagLowercase === 'input' && props.type === 'password'; for (const propKey in props) { if (!props.hasOwnProperty(propKey)) { @@ -335,6 +336,13 @@ function createOpenTagMarkup( if (propValue == null) { continue; } + // Do not render values for password inputs + if ( + isPasswordInput && + (propKey === 'value' || propKey === 'defaultValue') + ) { + continue; + } if (propKey === STYLE) { propValue = createMarkupForStyles(propValue); }