Skip to content

Commit

Permalink
Password inputs do not synchronize the value attribute
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nhunzaker committed May 1, 2018
1 parent 7dd4ca2 commit 9619e81
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 11 deletions.
34 changes: 34 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<Input type="password" value="1" />,
);
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(
<Input type="password" value="1" />,
);
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(
<input type="password" defaultValue="1" />,
);
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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<input type="password" value="foo" readOnly={true} />,
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(
<input type="password" defaultValue="foo" readOnly={true} />,
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(
<input type="password" value="foo" readOnly={true} />,
);

expect(e.value).toBe('');
expect(e.hasAttribute('value')).toBe(false);
});

it('will not render the defaultValue prop server-side', async () => {
const e = await serverRender(
<input type="password" defaultValue="foo" readOnly={true} />,
);

expect(e.value).toBe('');
expect(e.hasAttribute('value')).toBe(false);
});
});

describe('checkboxes', function() {
Expand Down
32 changes: 21 additions & 11 deletions packages/react-dom/src/client/ReactDOMFiberInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}

Expand Down
8 changes: 8 additions & 0 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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);
}
Expand Down

0 comments on commit 9619e81

Please sign in to comment.