Prototype Pollution

Proof-of-concept (pre-fix vs post-fix demonstration): Pre-fix (vulnerable path simulation): // Vulnerable path simulation (before fix) function vulnerableClone(input) { const out = {}; for (const key in input) { if (Object.prototype.hasOwnProperty.call(input, key)) { // Potential prototype pollution if key === '__proto__' out[key] = input[key]; } } return out; } // Craft payload with an own __proto__ property const payload = {}; Object.defineProperty(payload, '__proto__', { value: { polluted: true }, enumerable: true, writable: true, configurable: true }); payload.a = 1; const res = vulnerableClone(payload); console.log('Pre-fix: does res.__proto__ point to the polluted object? ', res.__proto__ && res.__proto__.polluted === true); console.log('Pre-fix: is the prototype polluted for res? ', Object.getPrototypeOf(res).polluted === true); Post-fix (safe path after fix in this commit): // Safe path after fix (resolveModel + defineProperty for __proto__) function safeClone(input) { const out = {}; for (const key in input) { if (Object.prototype.hasOwnProperty.call(input, key)) { if (key === '__proto__') { Object.defineProperty(out, key, { value: input[key], enumerable: true, writable: true, configurable: true }); } else { out[key] = input[key]; } } } return out; } const payload2 = {}; Object.defineProperty(payload2, '__proto__', { value: { polluted: true }, enumerable: true, writable: true, configurable: true }); payload2.a = 1; const res2 = safeClone(payload2); console.log('Post-fix: own __proto__ present on res2? ', Object.prototype.hasOwnProperty.call(res2, '__proto__')); console.log('Post-fix: res2.__proto__ is own property value? ', res2.__proto__.polluted); console.log('Post-fix: Object.prototype polluted? ', Object.prototype.polluted !== undefined);

diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 95633283b2c7..473b86aad2a8 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -2085,6 +2085,55 @@ describe('ReactFlight', () => { ]); }); + it('should serialize an own __proto__ property nested among siblings without disturbing them', async () => { + // `__proto__` here is a real own enumerable data property (not the + // prototype). It sits between sibling keys and holds an object value, which + // is the case most likely to regress if the serializer used a plain + // `obj.__proto__ = value` assignment: that would hit the prototype setter, + // dropping the key and mutating the holder's prototype instead. + const value = {a: 1}; + Object.defineProperty(value, '__proto__', { + value: {nested: true}, + enumerable: true, + writable: true, + configurable: true, + }); + value.b = 2; + + const transport = ReactNoopFlightServer.render(value); + assertConsoleErrorDev([ + 'Expected not to serialize an object with own property `__proto__`. ' + + 'When parsed this property will be omitted.\n' + + ' {a: 1, __proto__: {nested: true}, b: 2}\n' + + ' ^^^^^^^^^^^^^^', + ]); + + const decoder = new TextDecoder(); + const payload = transport + .map(chunk => (typeof chunk === 'string' ? chunk : decoder.decode(chunk))) + .join(''); + // The legacy key is serialized as ordinary data, in source order, with its + // object value intact and without clobbering its sibling properties. + expect(payload).toContain('"a":1,"__proto__":{"nested":true},"b":2'); + + const model = await ReactNoopFlightClient.read(transport); + assertConsoleErrorDev([ + 'Expected not to serialize an object with own property `__proto__`. ' + + 'When parsed this property will be omitted.\n' + + ' {a: 1, __proto__: {nested: true}, b: 2}\n' + + ' ^^^^^^^^^^^^^^\n' + + ' in (at **)', + ]); + // On the client the legacy key is omitted, but its siblings survive intact + // and the holder's prototype is untouched. + expect(Object.prototype.hasOwnProperty.call(model, '__proto__')).toBe( + false, + ); + expect(Object.getPrototypeOf(model)).toBe(Object.prototype); + expect(model.a).toBe(1); + expect(model.b).toBe(2); + }); + it('should NOT warn in DEV for key getters', () => { const transport = ReactNoopFlightServer.render(<div key="a" />); ReactNoopFlightClient.read(transport); diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 9e46915fee44..189059f0b7c3 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -530,7 +530,6 @@ type Task = { status: 0 | 1 | 3 | 4 | 5, model: ReactClientValue, ping: () => void, - toJSON: (key: string, value: ReactClientValue) => ReactJSONValue, keyPath: ReactKey, // parent server component keys implicitSlot: boolean, // true if the root server component of this sequence had a null key formatContext: FormatContext, // an approximate parent context from host components @@ -2761,55 +2760,6 @@ function createTask( implicitSlot, formatContext: formatContext, ping: () => pingTask(request, task), - toJSON: function ( - this: - | {+[key: string | number]: ReactClientValue} - | $ReadOnlyArray<ReactClientValue>, - parentPropertyName: string, - value: ReactClientValue, - ): ReactJSONValue { - const parent = this; - // Make sure that `parent[parentPropertyName]` wasn't JSONified before `value` was passed to us - if (__DEV__) { - // $FlowFixMe[incompatible-use] - const originalValue = parent[parentPropertyName]; - if ( - typeof originalValue === 'object' && - originalValue !== value && - !(originalValue instanceof Date) - ) { - // Call with the server component as the currently rendering component - // for context. - callWithDebugContextInDEV(request, task, () => { - if (objectName(originalValue) !== 'Object') { - const jsxParentType = jsxChildrenParents.get(parent); - if (typeof jsxParentType === 'string') { - console.error( - '%s objects cannot be rendered as text children. Try formatting it using toString().%s', - objectName(originalValue), - describeObjectForErrorMessage(parent, parentPropertyName), - ); - } else { - console.error( - 'Only plain objects can be passed to Client Components from Server Components. ' + - '%s objects are not supported.%s', - objectName(originalValue), - describeObjectForErrorMessage(parent, parentPropertyName), - ); - } - } else { - console.error( - 'Only plain objects can be passed to Client Components from Server Components. ' + - 'Objects with toJSON methods are not supported. Convert it manually ' + - 'to a simple value before passing it to props.%s', - describeObjectForErrorMessage(parent, parentPropertyName), - ); - } - }); - } - } - return renderModel(request, task, parent, parentPropertyName, value); - }, thenableState: null, } as Omit< Task, @@ -2837,6 +2787,117 @@ function createTask( return task; } +function resolveModel( + request: Request, + task: Task, + parent: + | {+[key: string | number]: ReactClientValue} + | $ReadOnlyArray<ReactClientValue>, + parentPropertyName: string, + value: ReactClientValue, +): ReactJSONValue { + // Replicate JSON.stringify's toJSON semantics: if the value has a toJSON + // method, call it first. In practice this only matters for Date objects whose + // toJSON calls toISOString. Custom toJSON objects are not supported and will + // trigger a DEV warning below. + let jsonValue: ReactClientValue = value; + if ( + value !== null && + typeof value === 'object' && + // $FlowFixMe[method-unbinding] + typeof value.toJSON === 'function' + ) { + // $FlowFixMe[incompatible-use] + jsonValue = value.toJSON(parentPropertyName); + } + + if (__DEV__) { + // $FlowFixMe[incompatible-use] + const originalValue = parent[parentPropertyName]; + if ( + typeof originalValue === 'object' && + originalValue !== jsonValue && + !(originalValue instanceof Date) + ) { + // Call with the server component as the currently rendering component + // for context. + callWithDebugContextInDEV(request, task, () => { + if (objectName(originalValue) !== 'Object') { + const jsxParentType = jsxChildrenParents.get(parent); + if (typeof jsxParentType === 'string') { + console.error( + '%s objects cannot be rendered as text children. Try formatting it using toString().%s', + objectName(originalValue), + describeObjectForErrorMessage(parent, parentPropertyName), + ); + } else { + console.error( + 'Only plain objects can be passed to Client Components from Server Components. ' + + '%s objects are not supported.%s', + objectName(originalValue), + describeObjectForErrorMessage(parent, parentPropertyName), + ); + } + } else { + console.error( + 'Only plain objects can be passed to Client Components from Server Components. ' + + 'Objects with toJSON methods are not supported. Convert it manually ' + + 'to a simple value before passing it to props.%s', + describeObjectForErrorMessage(parent, parentPropertyName), + ); + } + }); + } + } + + const rendered = renderModel( + request, + task, + parent, + parentPropertyName, + jsonValue, + ); + + if (rendered === null || typeof rendered !== 'object') { + return rendered; + } + + if (isArray(rendered)) { + const resolved: Array<ReactJSONValue> = []; + for (let i = 0; i < rendered.length; i++) { + resolved[i] = resolveModel(request, task, rendered, '' + i, rendered[i]); + } + return resolved; + } + + // Use `{}` for fast properties; `__proto__` is handled below because simple + // assignment would hit Object.prototype's setter instead of creating a key. + const resolved: {[key: string]: ReactJSONValue} = {} as any; + for (const key in rendered) { + if (hasOwnProperty.call(rendered, key)) { + const resolvedValue = resolveModel( + request, + task, + rendered, + key, + rendered[key], + ); + if (key === __PROTO__) { + // Match JSON's ordinary data-property semantics for this legacy key. + Object.defineProperty(resolved, key, { + value: resolvedValue, + enumerable: true, + writable: true, + configurable: true, + }); + } else { + resolved[key] = resolvedValue; + } + } + } + return resolved; +} + function serializeByValueID(id: number): string { return '$' + id.toString(16); } @@ -3618,7 +3679,7 @@ function renderModelDestructive( // TODO: Pop this. Since we currently don't have a point where we can pop the stack // this debug information will be used for errors inside sibling properties that // are not elements. Leading to the wrong attribution on the server. We could fix - // that if we switch to a proper stack instead of JSON.stringify's trampoline. + // that if we switch to a proper stack instead of resolveModel's recursive walk. // Attribution on the client is still correct since it has a pop. } @@ -5816,8 +5877,11 @@ function emitChunk( return; } // For anything else we need to try to serialize it using JSON. + // We resolve the model tree first in pure JS to avoid the C++->JS boundary + // overhead of JSON.stringify's replacer callback. + const resolvedModel = resolveModel(request, task, {'': value}, '', value); // $FlowFixMe[incompatible-type] stringify can return null for undefined but we never do - const json: string = stringify(value, task.toJSON); + const json: string = stringify(resolvedModel); emitModelChunk(request, task.id, json); } @@ -5863,7 +5927,7 @@ function retryTask(request: Request, task: Task): void { try { // Track the root so we know that we have to emit this object even though it // already has an ID. This is needed because we might see this object twice - // in the same toJSON if it is cyclic. + // in the same resolveModel walk if it is cyclic. modelRoot = task.model; if (__DEV__) { @@ -5922,8 +5986,8 @@ function retryTask(request: Request, task: Task): void { // This is simulating what the JSON loop would do if this was part of it. emitChunk(request, task, resolvedModel); } else { - // If the value is a string, it means it's a terminal value and we already escaped it - // We don't need to escape it again so it's not passed the toJSON replacer. + // If the value is a string, it means it's a terminal value and we already escaped it. + // We don't need to escape it again so it's not passed through resolveModel. // $FlowFixMe[incompatible-type] stringify can return null for undefined but we never do const json: string = stringify(resolvedModel); emitModelChunk(request, task.id, json);