From d4ddc8d4c20c8a7dbdd02b8b62fdce0df3bf5a58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 11 Oct 2023 12:18:49 -0400 Subject: [PATCH] [Flight] Enforce "simple object" rule in production (#27502) We only allow plain objects that can be faithfully serialized and deserialized through JSON to pass through the serialization boundary. It's a bit too expensive to do all the possible checks in production so we do most checks in DEV, so it's still possible to pass an object in production by mistake. This is currently exaggerated by frameworks because the logs on the server aren't visible enough. Even so, it's possible to do a mistake without testing it in DEV or just testing a conditional branch. That might have security implications if that object wasn't supposed to be passed. We can't rely on only checking if the prototype is `Object.prototype` because that wouldn't work with cross-realm objects which is unfortunate. However, if it isn't, we can check wether it has exactly one prototype on the chain which would catch the common error of passing a class instance. --- .../src/ReactFlightReplyClient.js | 95 +++++++++++-------- .../src/__tests__/ReactFlight-test.js | 22 +++-- .../react-server/src/ReactFlightServer.js | 66 ++++++++----- packages/react/src/ReactTaint.js | 6 +- packages/shared/ReactSerializationErrors.js | 5 +- packages/shared/getPrototypeOf.js | 12 +++ scripts/error-codes/codes.json | 4 +- 7 files changed, 127 insertions(+), 83 deletions(-) create mode 100644 packages/shared/getPrototypeOf.js diff --git a/packages/react-client/src/ReactFlightReplyClient.js b/packages/react-client/src/ReactFlightReplyClient.js index 57989237481ed..dc20eb55d2c1b 100644 --- a/packages/react-client/src/ReactFlightReplyClient.js +++ b/packages/react-client/src/ReactFlightReplyClient.js @@ -29,6 +29,9 @@ import { } from 'shared/ReactSerializationErrors'; import isArray from 'shared/isArray'; +import getPrototypeOf from 'shared/getPrototypeOf'; + +const ObjectPrototype = Object.prototype; import {usedWithSSR} from './ReactFlightClientConfig'; @@ -227,6 +230,10 @@ export function processReply( ); return serializePromiseID(promiseId); } + if (isArray(value)) { + // $FlowFixMe[incompatible-return] + return value; + } // TODO: Should we the Object.prototype.toString.call() to test for cross-realm objects? if (value instanceof FormData) { if (formData === null) { @@ -263,54 +270,60 @@ export function processReply( formData.append(formFieldPrefix + setId, partJSON); return serializeSetID(setId); } - if (!isArray(value)) { - const iteratorFn = getIteratorFn(value); - if (iteratorFn) { - return Array.from((value: any)); - } + const iteratorFn = getIteratorFn(value); + if (iteratorFn) { + return Array.from((value: any)); } + // Verify that this is a simple plain object. + const proto = getPrototypeOf(value); + if ( + proto !== ObjectPrototype && + (proto === null || getPrototypeOf(proto) !== null) + ) { + throw new Error( + 'Only plain objects, and a few built-ins, can be passed to Server Actions. ' + + 'Classes or null prototypes are not supported.', + ); + } if (__DEV__) { - if (value !== null && !isArray(value)) { - // Verify that this is a simple plain object. - if ((value: any).$$typeof === REACT_ELEMENT_TYPE) { - console.error( - 'React Element cannot be passed to Server Functions from the Client.%s', - describeObjectForErrorMessage(parent, key), - ); - } else if ((value: any).$$typeof === REACT_LAZY_TYPE) { - console.error( - 'React Lazy cannot be passed to Server Functions from the Client.%s', - describeObjectForErrorMessage(parent, key), - ); - } else if ((value: any).$$typeof === REACT_PROVIDER_TYPE) { - console.error( - 'React Context Providers cannot be passed to Server Functions from the Client.%s', - describeObjectForErrorMessage(parent, key), - ); - } else if (objectName(value) !== 'Object') { - console.error( - 'Only plain objects can be passed to Client Components from Server Components. ' + - '%s objects are not supported.%s', - objectName(value), - describeObjectForErrorMessage(parent, key), - ); - } else if (!isSimpleObject(value)) { + if ((value: any).$$typeof === REACT_ELEMENT_TYPE) { + console.error( + 'React Element cannot be passed to Server Functions from the Client.%s', + describeObjectForErrorMessage(parent, key), + ); + } else if ((value: any).$$typeof === REACT_LAZY_TYPE) { + console.error( + 'React Lazy cannot be passed to Server Functions from the Client.%s', + describeObjectForErrorMessage(parent, key), + ); + } else if ((value: any).$$typeof === REACT_PROVIDER_TYPE) { + console.error( + 'React Context Providers cannot be passed to Server Functions from the Client.%s', + describeObjectForErrorMessage(parent, key), + ); + } else if (objectName(value) !== 'Object') { + console.error( + 'Only plain objects can be passed to Client Components from Server Components. ' + + '%s objects are not supported.%s', + objectName(value), + describeObjectForErrorMessage(parent, key), + ); + } else if (!isSimpleObject(value)) { + console.error( + 'Only plain objects can be passed to Client Components from Server Components. ' + + 'Classes or other objects with methods are not supported.%s', + describeObjectForErrorMessage(parent, key), + ); + } else if (Object.getOwnPropertySymbols) { + const symbols = Object.getOwnPropertySymbols(value); + if (symbols.length > 0) { console.error( 'Only plain objects can be passed to Client Components from Server Components. ' + - 'Classes or other objects with methods are not supported.%s', + 'Objects with symbol properties like %s are not supported.%s', + symbols[0].description, describeObjectForErrorMessage(parent, key), ); - } else if (Object.getOwnPropertySymbols) { - const symbols = Object.getOwnPropertySymbols(value); - if (symbols.length > 0) { - console.error( - 'Only plain objects can be passed to Client Components from Server Components. ' + - 'Objects with symbol properties like %s are not supported.%s', - symbols[0].description, - describeObjectForErrorMessage(parent, key), - ); - } } } } diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index c0f3d2bbb0b04..c68630e5b23e5 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -988,19 +988,21 @@ describe('ReactFlight', () => { ReactNoopFlightClient.read(transport); }); - it('should warn in DEV if a class instance is passed to a host component', () => { + it('should error if a class instance is passed to a host component', () => { class Foo { method() {} } - expect(() => { - const transport = ReactNoopFlightServer.render( - , - ); - ReactNoopFlightClient.read(transport); - }).toErrorDev( - 'Only plain objects can be passed to Client Components from Server Components. ', - {withoutStack: true}, - ); + const errors = []; + ReactNoopFlightServer.render(, { + onError(x) { + errors.push(x.message); + }, + }); + + expect(errors).toEqual([ + 'Only plain objects, and a few built-ins, can be passed to Client Components ' + + 'from Server Components. Classes or null prototypes are not supported.', + ]); }); it('should warn in DEV if a a client reference is passed to useContext()', () => { diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index e8d120b952f1e..6c08dd9948838 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -111,10 +111,13 @@ import {getOrCreateServerContext} from 'shared/ReactServerContextRegistry'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import ReactServerSharedInternals from './ReactServerSharedInternals'; import isArray from 'shared/isArray'; +import getPrototypeOf from 'shared/getPrototypeOf'; import binaryToComparableString from 'shared/binaryToComparableString'; import {SuspenseException, getSuspendedThenable} from './ReactFlightThenable'; +const ObjectPrototype = Object.prototype; + type JSONValue = | string | boolean @@ -1046,6 +1049,11 @@ function resolveModelToJSON( return (undefined: any); } + if (isArray(value)) { + // $FlowFixMe[incompatible-return] + return value; + } + if (value instanceof Map) { return serializeMap(request, value); } @@ -1107,39 +1115,45 @@ function resolveModelToJSON( } } - if (!isArray(value)) { - const iteratorFn = getIteratorFn(value); - if (iteratorFn) { - return Array.from((value: any)); - } + const iteratorFn = getIteratorFn(value); + if (iteratorFn) { + return Array.from((value: any)); } + // Verify that this is a simple plain object. + const proto = getPrototypeOf(value); + if ( + proto !== ObjectPrototype && + (proto === null || getPrototypeOf(proto) !== null) + ) { + throw new Error( + 'Only plain objects, and a few built-ins, can be passed to Client Components ' + + 'from Server Components. Classes or null prototypes are not supported.', + ); + } if (__DEV__) { - if (value !== null && !isArray(value)) { - // Verify that this is a simple plain object. - if (objectName(value) !== 'Object') { - console.error( - 'Only plain objects can be passed to Client Components from Server Components. ' + - '%s objects are not supported.%s', - objectName(value), - describeObjectForErrorMessage(parent, key), - ); - } else if (!isSimpleObject(value)) { + if (objectName(value) !== 'Object') { + console.error( + 'Only plain objects can be passed to Client Components from Server Components. ' + + '%s objects are not supported.%s', + objectName(value), + describeObjectForErrorMessage(parent, key), + ); + } else if (!isSimpleObject(value)) { + console.error( + 'Only plain objects can be passed to Client Components from Server Components. ' + + 'Classes or other objects with methods are not supported.%s', + describeObjectForErrorMessage(parent, key), + ); + } else if (Object.getOwnPropertySymbols) { + const symbols = Object.getOwnPropertySymbols(value); + if (symbols.length > 0) { console.error( 'Only plain objects can be passed to Client Components from Server Components. ' + - 'Classes or other objects with methods are not supported.%s', + 'Objects with symbol properties like %s are not supported.%s', + symbols[0].description, describeObjectForErrorMessage(parent, key), ); - } else if (Object.getOwnPropertySymbols) { - const symbols = Object.getOwnPropertySymbols(value); - if (symbols.length > 0) { - console.error( - 'Only plain objects can be passed to Client Components from Server Components. ' + - 'Objects with symbol properties like %s are not supported.%s', - symbols[0].description, - describeObjectForErrorMessage(parent, key), - ); - } } } } diff --git a/packages/react/src/ReactTaint.js b/packages/react/src/ReactTaint.js index d9e532737be6e..399acb8f01431 100644 --- a/packages/react/src/ReactTaint.js +++ b/packages/react/src/ReactTaint.js @@ -9,6 +9,8 @@ import {enableTaint, enableBinaryFlight} from 'shared/ReactFeatureFlags'; +import getPrototypeOf from 'shared/getPrototypeOf'; + import binaryToComparableString from 'shared/binaryToComparableString'; import ReactServerSharedInternals from './ReactServerSharedInternals'; @@ -22,9 +24,7 @@ const { interface Reference {} // This is the shared constructor of all typed arrays. -const TypedArrayConstructor = Object.getPrototypeOf( - Uint32Array.prototype, -).constructor; +const TypedArrayConstructor = getPrototypeOf(Uint32Array.prototype).constructor; const defaultMessage = 'A tainted value was attempted to be serialized to a Client Component or Action closure. ' + diff --git a/packages/shared/ReactSerializationErrors.js b/packages/shared/ReactSerializationErrors.js index a4875007791f5..e0def3e1c657d 100644 --- a/packages/shared/ReactSerializationErrors.js +++ b/packages/shared/ReactSerializationErrors.js @@ -19,6 +19,7 @@ import { import type {LazyComponent} from 'react/src/ReactLazy'; import isArray from 'shared/isArray'; +import getPrototypeOf from 'shared/getPrototypeOf'; // Used for DEV messages to keep track of which parent rendered some props, // in case they error. @@ -35,7 +36,7 @@ function isObjectPrototype(object: any): boolean { } // It might be an object from a different Realm which is // still just a plain simple object. - if (Object.getPrototypeOf(object)) { + if (getPrototypeOf(object)) { return false; } const names = Object.getOwnPropertyNames(object); @@ -48,7 +49,7 @@ function isObjectPrototype(object: any): boolean { } export function isSimpleObject(object: any): boolean { - if (!isObjectPrototype(Object.getPrototypeOf(object))) { + if (!isObjectPrototype(getPrototypeOf(object))) { return false; } const names = Object.getOwnPropertyNames(object); diff --git a/packages/shared/getPrototypeOf.js b/packages/shared/getPrototypeOf.js new file mode 100644 index 0000000000000..2137bdc1c3947 --- /dev/null +++ b/packages/shared/getPrototypeOf.js @@ -0,0 +1,12 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +const getPrototypeOf = Object.getPrototypeOf; + +export default getPrototypeOf; diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 851c058cf1b2d..b01d1150b2287 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -482,5 +482,7 @@ "494": "taintUniqueValue cannot taint objects or functions. Try taintObjectReference instead.", "495": "Cannot taint a %s because the value is too general and not unique enough to block globally.", "496": "Only objects or functions can be passed to taintObjectReference. Try taintUniqueValue instead.", - "497": "Only objects or functions can be passed to taintObjectReference." + "497": "Only objects or functions can be passed to taintObjectReference.", + "498": "Only plain objects, and a few built-ins, can be passed to Client Components from Server Components. Classes or null prototypes are not supported.", + "499": "Only plain objects, and a few built-ins, can be passed to Server Actions. Classes or null prototypes are not supported." } \ No newline at end of file