Skip to content

Commit

Permalink
refactor[devtools]: forbid editing class instances in props (#26522)
Browse files Browse the repository at this point in the history
## Summary
Fixes #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
  • Loading branch information
hoxyq authored Apr 3, 2023
1 parent 7329ea8 commit b14f8da
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 5 deletions.
6 changes: 6 additions & 0 deletions fixtures/devtools/standalone/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,11 @@ <h1>List</h1>
},
});

class Foo {
flag = false;
object = {a: {b: {c: {d: 1}}}}
}

function UnserializableProps() {
return (
<ChildComponent
Expand All @@ -343,6 +348,7 @@ <h1>List</h1>
setOfSets={setOfSets}
typedArray={typedArray}
immutable={immutable}
classInstance={new Foo()}
/>
);
}
Expand Down
27 changes: 27 additions & 0 deletions packages/react-devtools-shared/src/__tests__/utils-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import {
getDisplayName,
getDisplayNameForReactElement,
isPlainObject,
} from 'react-devtools-shared/src/utils';
import {stackToComponentSources} from 'react-devtools-shared/src/devtools/utils';
import {
Expand Down Expand Up @@ -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);
});
});
});
41 changes: 36 additions & 5 deletions packages/react-devtools-shared/src/hydration.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
Expand Down
17 changes: 17 additions & 0 deletions packages/react-devtools-shared/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,7 @@ export type DataType =
| 'array_buffer'
| 'bigint'
| 'boolean'
| 'class_instance'
| 'data_view'
| 'date'
| 'function'
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -835,6 +841,8 @@ export function formatDataForPreview(
}
case 'date':
return data.toString();
case 'class_instance':
return data.constructor.name;

This comment has been minimized.

Copy link
@spenceryue

spenceryue Jul 27, 2023

Not everything that satisfies !isPlainObject(data) will have a .constructor property.

For instance, isPlainObject({ __proto__: { __proto__: { __proto__: null } } }) === false and yet it isn't constructed with new.

This causes

utils.js:758 Uncaught TypeError: Cannot read properties of undefined (reading 'name')
    at formatDataForPreview (utils.js:758:1)
    at createDehydrated (hydration.js:37:1)
    at dehydrate (hydration.js:236:1)
    at hydration.js:227:1
    at Set.forEach (<anonymous>)
    at dehydrate (hydration.js:225:1)
    at hydration.js:227:1
    at Set.forEach (<anonymous>)
    at dehydrate (hydration.js:225:1)
    at hydration.js:160:1
    at Array.map (<anonymous>)
    at dehydrate (hydration.js:160:1)
    at cleanForBridge (utils.js:26:1)
    at Object.inspectElement (renderer.js:3349:1)
    at agent.js:175:1
    at Bridge.emit (events.js:37:1)
    at bridge.js:136:1
    at listener (backendManager.js:4127:9)

Also, I think source maps might have broken (from webpack 5?), because clicking the link to utils.js:758:1 in Chrome devtools brings me to the Sources tab with an error:

Could not load content for webpack://react-devtools-extensions/react-devtools-shared/src/utils.js (Fetch through target failed: Unsupported URL scheme; Fallback: HTTP error: status code 404, net::ERR_UNKNOWN_URL_SCHEME)

Luckily, I was able to override the chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/react_devtools_backend_compact.js file with some console logs to find the line that triggered the error. (There are several places reading from .name in this formatDataForPreview() function. Breakpoints don't trigger.)

My specific use case is I have a data structure built as a chain of prototypes, constructed at each layer with the inline { __proto__: ... } syntax. (Not to be confused with the deprecated .__proto__ accessor.)

One possible fix is to change the last line of isPlainObject to

return !objectParentPrototype || object.constructor?.name == null;
case 'object':
if (showFormattedValue) {
const keys = Array.from(getAllEnumerableKeys(data)).sort(alphaSortKeys);
Expand Down Expand Up @@ -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;
};
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<ChildComponent
Expand All @@ -45,6 +52,7 @@ export default function UnserializableProps(): React.Node {
typedArray={typedArray}
immutable={immutable}
bigInt={bigInt}
classInstance={new Foo()}
/>
);
}
Expand Down

0 comments on commit b14f8da

Please sign in to comment.