Skip to content

Commit

Permalink
refactor[devtools]: forbid editing class instances in props (facebook…
Browse files Browse the repository at this point in the history
…#26522)

## Summary
Fixes facebook#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 and kassens committed Apr 17, 2023
1 parent 2029852 commit 85bea51
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;
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 85bea51

Please sign in to comment.