Skip to content

Commit e190e81

Browse files
committed
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.
1 parent 7dd4ca2 commit e190e81

File tree

4 files changed

+121
-11
lines changed

4 files changed

+121
-11
lines changed

packages/react-dom/src/__tests__/ReactDOMInput-test.js

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1433,6 +1433,40 @@ describe('ReactDOMInput', () => {
14331433
expect(node.getAttribute('value')).toBe('2');
14341434
});
14351435

1436+
it('does not set the value attribute on password inputs', () => {
1437+
const Input = getTestInput();
1438+
const stub = ReactTestUtils.renderIntoDocument(
1439+
<Input type="password" value="1" />,
1440+
);
1441+
const node = ReactDOM.findDOMNode(stub);
1442+
1443+
expect(node.hasAttribute('value')).toBe(false);
1444+
expect(node.value).toBe('1');
1445+
});
1446+
1447+
it('does not update the value attribute on password inputs', () => {
1448+
const Input = getTestInput();
1449+
const stub = ReactTestUtils.renderIntoDocument(
1450+
<Input type="password" value="1" />,
1451+
);
1452+
const node = ReactDOM.findDOMNode(stub);
1453+
1454+
ReactTestUtils.Simulate.change(node, {target: {value: '2'}});
1455+
1456+
expect(node.hasAttribute('value')).toBe(false);
1457+
expect(node.value).toBe('2');
1458+
});
1459+
1460+
it('does not set the defaultValue attribute on password inputs', () => {
1461+
const stub = ReactTestUtils.renderIntoDocument(
1462+
<input type="password" defaultValue="1" />,
1463+
);
1464+
const node = ReactDOM.findDOMNode(stub);
1465+
1466+
expect(node.hasAttribute('value')).toBe(false);
1467+
expect(node.value).toBe('1');
1468+
});
1469+
14361470
it('does not set the value attribute on number inputs if focused', () => {
14371471
const Input = getTestInput();
14381472
const stub = ReactTestUtils.renderIntoDocument(

packages/react-dom/src/__tests__/ReactDOMServerIntegrationForms-test.js

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,50 @@ describe('ReactDOMServerIntegration', () => {
9696
expect(e.getAttribute('defaultValue')).toBe(null);
9797
},
9898
);
99+
100+
itClientRenders(
101+
'a password input hydrates client-side with the value prop',
102+
async render => {
103+
const e = await render(
104+
<input type="password" value="foo" readOnly={true} />,
105+
0,
106+
);
107+
108+
expect(e.value).toBe('foo');
109+
expect(e.hasAttribute('value')).toBe(false);
110+
},
111+
);
112+
113+
itClientRenders(
114+
'a password input hydrates client-side with the defaultValue prop',
115+
async render => {
116+
const e = await render(
117+
<input type="password" defaultValue="foo" readOnly={true} />,
118+
0,
119+
);
120+
121+
expect(e.value).toBe('foo');
122+
expect(e.hasAttribute('value')).toBe(false);
123+
},
124+
);
125+
126+
it('will not render the value prop server-side', async () => {
127+
const e = await serverRender(
128+
<input type="password" value="foo" readOnly={true} />,
129+
);
130+
131+
expect(e.value).toBe('');
132+
expect(e.hasAttribute('value')).toBe(false);
133+
});
134+
135+
it('will not render the defaultValue prop server-side', async () => {
136+
const e = await serverRender(
137+
<input type="password" defaultValue="foo" readOnly={true} />,
138+
);
139+
140+
expect(e.value).toBe('');
141+
expect(e.hasAttribute('value')).toBe(false);
142+
});
99143
});
100144

101145
describe('checkboxes', function() {
@@ -540,6 +584,20 @@ describe('ReactDOMServerIntegration', () => {
540584
<input defaultValue="Hello" />,
541585
));
542586

587+
it('should not blow away user-entered text on successful reconnect to an uncontrolled password input', () =>
588+
testUserInteractionBeforeClientRender(
589+
<input type="password" defaultValue="Hello" />,
590+
'',
591+
'Hello',
592+
));
593+
594+
it('should not blow away user-entered text on successful reconnect to an controlled password input', () =>
595+
testUserInteractionBeforeClientRender(
596+
<input type="password" value="Hello" readOnly={true} />,
597+
'',
598+
'Hello',
599+
));
600+
543601
it('should not blow away user-entered text on successful reconnect to a controlled input', async () => {
544602
let changeCount = 0;
545603
await testUserInteractionBeforeClientRender(

packages/react-dom/src/client/ReactDOMFiberInput.js

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,9 @@ export function postMountWrapper(element: Element, props: Object) {
218218
// value must be assigned before defaultValue. This fixes an issue where the
219219
// visually displayed value of date inputs disappears on mobile Safari and Chrome:
220220
// https://github.com/facebook/react/issues/7233
221-
node.defaultValue = '' + node._wrapperState.initialValue;
221+
if (props.type !== 'password') {
222+
node.defaultValue = '' + node._wrapperState.initialValue;
223+
}
222224
}
223225

224226
// Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug
@@ -304,16 +306,24 @@ export function setDefaultValue(
304306
type: ?string,
305307
value: *,
306308
) {
307-
if (
308-
// Focused number inputs synchronize on blur. See ChangeEventPlugin.js
309-
type !== 'number' ||
310-
node.ownerDocument.activeElement !== node
311-
) {
312-
if (value == null) {
313-
node.defaultValue = '' + node._wrapperState.initialValue;
314-
} else if (node.defaultValue !== '' + value) {
315-
node.defaultValue = '' + value;
316-
}
309+
if (type === 'password') {
310+
// Do not synchronize password inputs to prevent password from
311+
// being exposed in markup
312+
// https://github.com/facebook/react/issues/11896
313+
return;
314+
}
315+
316+
// Only assign the value attribute on number inputs when they are not selected
317+
// This avoids edge cases in Chrome. Number inputs are synchronized on blur.
318+
// See ChangeEventPlugin.js
319+
if (type === 'number' && node.ownerDocument.activeElement === node) {
320+
return;
321+
}
322+
323+
if (value == null) {
324+
node.defaultValue = '' + node._wrapperState.initialValue;
325+
} else if (node.defaultValue !== '' + value) {
326+
node.defaultValue = '' + value;
317327
}
318328
}
319329

packages/react-dom/src/server/ReactPartialRenderer.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,7 @@ function createOpenTagMarkup(
326326
isRootElement: boolean,
327327
): string {
328328
let ret = '<' + tagVerbatim;
329+
const isPasswordInput = tagLowercase === 'input' && props.type === 'password';
329330

330331
for (const propKey in props) {
331332
if (!props.hasOwnProperty(propKey)) {
@@ -335,6 +336,13 @@ function createOpenTagMarkup(
335336
if (propValue == null) {
336337
continue;
337338
}
339+
// Do not render values for password inputs
340+
if (
341+
isPasswordInput &&
342+
(propKey === 'value' || propKey === 'defaultValue')
343+
) {
344+
continue;
345+
}
338346
if (propKey === STYLE) {
339347
propValue = createMarkupForStyles(propValue);
340348
}

0 commit comments

Comments
 (0)