From b14f8da15598cdc2253529a905421ac795d68ab1 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 3 Apr 2023 11:32:17 +0100 Subject: [PATCH] refactor[devtools]: forbid editing class instances in props (#26522) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes https://github.com/facebook/react/issues/24781 Restricting from editing props, which are class instances, because their internals should be opaque. Proposed changes: 1. Adding new data type `class_instance`: based on prototype chain of an object we will check if its plain or not. If not, then will be marked as `class_instance`. This should not affect `arrays`, ..., because we do this in the end of an `object` case in `getDataType` function. Important detail: this approach won't work for objects created with `Object.create`, because of the custom prototype. This can also be bypassed by manually deleting a prototype ¯\\\_(ツ)_/¯ I am not sure if there might be a better solution (which will cover all cases) to detect if object is a class instance. Initially I was trying to use `Object.getPrototypeOf(object) === Object.prototype`, but this won't work for cases when we are dealing with `iframe`. 2. Objects with a type `class_instance` will be marked as unserializable and read-only. ## Demo `person` is a class instance, `object` is a plain object https://user-images.githubusercontent.com/28902667/228914791-ebdc8ab0-eb5c-426d-8163-66d56b5e8790.mov --- fixtures/devtools/standalone/index.html | 6 +++ .../src/__tests__/utils-test.js | 27 ++++++++++++ .../react-devtools-shared/src/hydration.js | 41 ++++++++++++++++--- packages/react-devtools-shared/src/utils.js | 17 ++++++++ .../UnserializableProps.js | 8 ++++ 5 files changed, 94 insertions(+), 5 deletions(-) diff --git a/fixtures/devtools/standalone/index.html b/fixtures/devtools/standalone/index.html index 235ceeb16e..a7e9af3248 100644 --- a/fixtures/devtools/standalone/index.html +++ b/fixtures/devtools/standalone/index.html @@ -334,6 +334,11 @@ }, }); + class Foo { + flag = false; + object = {a: {b: {c: {d: 1}}}} + } + function UnserializableProps() { return ( ); } diff --git a/packages/react-devtools-shared/src/__tests__/utils-test.js b/packages/react-devtools-shared/src/__tests__/utils-test.js index 9c2c58f329..706fa99e09 100644 --- a/packages/react-devtools-shared/src/__tests__/utils-test.js +++ b/packages/react-devtools-shared/src/__tests__/utils-test.js @@ -10,6 +10,7 @@ import { getDisplayName, getDisplayNameForReactElement, + isPlainObject, } from 'react-devtools-shared/src/utils'; import {stackToComponentSources} from 'react-devtools-shared/src/devtools/utils'; import { @@ -270,4 +271,30 @@ describe('utils', () => { expect(gte('10.0.0', '9.0.0')).toBe(true); }); }); + + describe('isPlainObject', () => { + it('should return true for plain objects', () => { + expect(isPlainObject({})).toBe(true); + expect(isPlainObject({a: 1})).toBe(true); + expect(isPlainObject({a: {b: {c: 123}}})).toBe(true); + }); + + it('should return false if object is a class instance', () => { + expect(isPlainObject(new (class C {})())).toBe(false); + }); + + it('should retun false for objects, which have not only Object in its prototype chain', () => { + expect(isPlainObject([])).toBe(false); + expect(isPlainObject(Symbol())).toBe(false); + }); + + it('should retun false for primitives', () => { + expect(isPlainObject(5)).toBe(false); + expect(isPlainObject(true)).toBe(false); + }); + + it('should return true for objects with no prototype', () => { + expect(isPlainObject(Object.create(null))).toBe(true); + }); + }); }); diff --git a/packages/react-devtools-shared/src/hydration.js b/packages/react-devtools-shared/src/hydration.js index 610719c9db..431d360696 100644 --- a/packages/react-devtools-shared/src/hydration.js +++ b/packages/react-devtools-shared/src/hydration.js @@ -52,7 +52,7 @@ export type Unserializable = { size?: number, type: string, unserializable: boolean, - ... + [string | number]: any, }; // This threshold determines the depth at which the bridge "dehydrates" nested data. @@ -248,7 +248,6 @@ export function dehydrate( // Other types (e.g. typed arrays, Sets) will not spread correctly. Array.from(data).forEach( (item, i) => - // $FlowFixMe[prop-missing] Unserializable doesn't have an index signature (unserializableValue[i] = dehydrate( item, cleaned, @@ -296,6 +295,7 @@ export function dehydrate( case 'object': isPathAllowedCheck = isPathAllowed(path); + if (level >= LEVEL_THRESHOLD && !isPathAllowedCheck) { return createDehydrated(type, true, data, cleaned, path); } else { @@ -316,15 +316,46 @@ export function dehydrate( return object; } + case 'class_instance': + isPathAllowedCheck = isPathAllowed(path); + + if (level >= LEVEL_THRESHOLD && !isPathAllowedCheck) { + return createDehydrated(type, true, data, cleaned, path); + } + + const value: Unserializable = { + unserializable: true, + type, + readonly: true, + preview_short: formatDataForPreview(data, false), + preview_long: formatDataForPreview(data, true), + name: data.constructor.name, + }; + + getAllEnumerableKeys(data).forEach(key => { + const keyAsString = key.toString(); + + value[keyAsString] = dehydrate( + data[key], + cleaned, + unserializable, + path.concat([keyAsString]), + isPathAllowed, + isPathAllowedCheck ? 1 : level + 1, + ); + }); + + unserializable.push(path); + + return value; + case 'infinity': case 'nan': case 'undefined': // Some values are lossy when sent through a WebSocket. // We dehydrate+rehydrate them to preserve their type. cleaned.push(path); - return { - type, - }; + return {type}; default: return data; diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 0feea7399f..58845219d9 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -534,6 +534,7 @@ export type DataType = | 'array_buffer' | 'bigint' | 'boolean' + | 'class_instance' | 'data_view' | 'date' | 'function' @@ -620,6 +621,11 @@ export function getDataType(data: Object): DataType { return 'html_all_collection'; } } + + if (!isPlainObject(data)) { + return 'class_instance'; + } + return 'object'; case 'string': return 'string'; @@ -835,6 +841,8 @@ export function formatDataForPreview( } case 'date': return data.toString(); + case 'class_instance': + return data.constructor.name; case 'object': if (showFormattedValue) { const keys = Array.from(getAllEnumerableKeys(data)).sort(alphaSortKeys); @@ -873,3 +881,12 @@ export function formatDataForPreview( } } } + +// Basically checking that the object only has Object in its prototype chain +export const isPlainObject = (object: Object): boolean => { + const objectPrototype = Object.getPrototypeOf(object); + if (!objectPrototype) return true; + + const objectParentPrototype = Object.getPrototypeOf(objectPrototype); + return !objectParentPrototype; +}; diff --git a/packages/react-devtools-shell/src/app/InspectableElements/UnserializableProps.js b/packages/react-devtools-shell/src/app/InspectableElements/UnserializableProps.js index a5d2c33de8..d04783b6a7 100644 --- a/packages/react-devtools-shell/src/app/InspectableElements/UnserializableProps.js +++ b/packages/react-devtools-shell/src/app/InspectableElements/UnserializableProps.js @@ -33,6 +33,13 @@ const immutable = Immutable.fromJS({ }); const bigInt = BigInt(123); // eslint-disable-line no-undef +class Foo { + flag = false; + object: Object = { + a: {b: {c: {d: 1}}}, + }; +} + export default function UnserializableProps(): React.Node { return ( ); }