refactor[devtools]: forbid editing class instances in props #26522

hoxyq · 2023-03-30T17:35:51Z

Summary

Restricting from editing props, which are class instances, because their internals should be opaque.

Proposed changes:

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.

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

demo.mov

mondaychen

Overall looks good!

mondaychen · 2023-03-30T19:59:20Z

packages/react-devtools-shared/src/utils.js

+  if (!objectPrototype) return true;
+
+  const objectParentPrototype = Object.getPrototypeOf(objectPrototype);
+  return objectParentPrototype == null;


Is it the only prototype with null as its parent prototype?

Is it the only prototype with null as its parent prototype?

Sorry, didn't quite understand the question. I am using non-strict equals here, so essentially it will check just if objectParentPrototype is undefined or null

Might also just use !objectParentPrototype here

Sorry for the confusion. I was just curious whether the prototype of an object is the only one that has nothing up in the chain.
This check here is good.

packages/react-devtools-shared/src/utils.js

react-sizebot · 2023-03-31T13:59:11Z

Comparing: ca01f35...651a028

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	162.81 kB	162.81 kB	=	51.35 kB	51.35 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	164.35 kB	164.35 kB	=	51.84 kB	51.84 kB
facebook-www/ReactDOM-prod.classic.js	=	556.51 kB	556.51 kB	=	98.16 kB	98.16 kB
facebook-www/ReactDOM-prod.modern.js	=	540.21 kB	540.21 kB	=	95.78 kB	95.78 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 651a028

Full list of changes (not everything included in changelog): * refactor[devtools]: copy to clipboard only on frontend side ([hoxyq](https://github.com/hoxyq) in [#26604](#26604)) * Provide icon to edge devtools. ([harrygz889](https://github.com/harrygz889) in [#26543](#26543)) * [BE] move shared types & constants to consolidated locations ([mondaychen](https://github.com/mondaychen) in [#26572](#26572)) * remove backend dependency from the global hook ([mondaychen](https://github.com/mondaychen) in [#26563](#26563)) * Replace deprecated `new-window` with `webContents.setWindowOpenHandler()` ([Willie-Boy](https://github.com/Willie-Boy) in [#26559](#26559)) * DevTools: Inline references to fiber flags ([acdlite](https://github.com/acdlite) in [#26542](#26542)) * refactor[devtools]: forbid editing class instances in props ([hoxyq](https://github.com/hoxyq) in [#26522](#26522)) * Move update scheduling to microtask ([acdlite](https://github.com/acdlite) in [#26512](#26512)) * Remove unnecessary CIRCLE_CI_API_TOKEN checks ([mondaychen](https://github.com/mondaychen) in [#26499](#26499)) * browser extension: improve script injection logic ([mondaychen](https://github.com/mondaychen) in [#26492](#26492)) * [flow] make Flow suppressions explicit on the error ([kassens](https://github.com/kassens) in [#26487](#26487))

…#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

Full list of changes (not everything included in changelog): * refactor[devtools]: copy to clipboard only on frontend side ([hoxyq](https://github.com/hoxyq) in [facebook#26604](facebook#26604)) * Provide icon to edge devtools. ([harrygz889](https://github.com/harrygz889) in [facebook#26543](facebook#26543)) * [BE] move shared types & constants to consolidated locations ([mondaychen](https://github.com/mondaychen) in [facebook#26572](facebook#26572)) * remove backend dependency from the global hook ([mondaychen](https://github.com/mondaychen) in [facebook#26563](facebook#26563)) * Replace deprecated `new-window` with `webContents.setWindowOpenHandler()` ([Willie-Boy](https://github.com/Willie-Boy) in [facebook#26559](facebook#26559)) * DevTools: Inline references to fiber flags ([acdlite](https://github.com/acdlite) in [facebook#26542](facebook#26542)) * refactor[devtools]: forbid editing class instances in props ([hoxyq](https://github.com/hoxyq) in [facebook#26522](facebook#26522)) * Move update scheduling to microtask ([acdlite](https://github.com/acdlite) in [facebook#26512](facebook#26512)) * Remove unnecessary CIRCLE_CI_API_TOKEN checks ([mondaychen](https://github.com/mondaychen) in [facebook#26499](facebook#26499)) * browser extension: improve script injection logic ([mondaychen](https://github.com/mondaychen) in [facebook#26492](facebook#26492)) * [flow] make Flow suppressions explicit on the error ([kassens](https://github.com/kassens) in [facebook#26487](facebook#26487))

…#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

Full list of changes (not everything included in changelog): * refactor[devtools]: copy to clipboard only on frontend side ([hoxyq](https://github.com/hoxyq) in [facebook#26604](facebook#26604)) * Provide icon to edge devtools. ([harrygz889](https://github.com/harrygz889) in [facebook#26543](facebook#26543)) * [BE] move shared types & constants to consolidated locations ([mondaychen](https://github.com/mondaychen) in [facebook#26572](facebook#26572)) * remove backend dependency from the global hook ([mondaychen](https://github.com/mondaychen) in [facebook#26563](facebook#26563)) * Replace deprecated `new-window` with `webContents.setWindowOpenHandler()` ([Willie-Boy](https://github.com/Willie-Boy) in [facebook#26559](facebook#26559)) * DevTools: Inline references to fiber flags ([acdlite](https://github.com/acdlite) in [facebook#26542](facebook#26542)) * refactor[devtools]: forbid editing class instances in props ([hoxyq](https://github.com/hoxyq) in [facebook#26522](facebook#26522)) * Move update scheduling to microtask ([acdlite](https://github.com/acdlite) in [facebook#26512](facebook#26512)) * Remove unnecessary CIRCLE_CI_API_TOKEN checks ([mondaychen](https://github.com/mondaychen) in [facebook#26499](facebook#26499)) * browser extension: improve script injection logic ([mondaychen](https://github.com/mondaychen) in [facebook#26492](facebook#26492)) * [flow] make Flow suppressions explicit on the error ([kassens](https://github.com/kassens) in [facebook#26487](facebook#26487))

## 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 DiffTrain build for commit b14f8da.

hoxyq requested a review from mondaychen March 30, 2023 17:35

facebook-github-bot added the CLA Signed label Mar 30, 2023

hoxyq force-pushed the refactor/forbid-editing-class-instances-in-props branch 3 times, most recently from 967fecf to ab8fa95 Compare March 30, 2023 17:58

mondaychen added the React Core Team Opened by a member of the React Core Team label Mar 30, 2023

mondaychen approved these changes Mar 30, 2023

View reviewed changes

hoxyq force-pushed the refactor/forbid-editing-class-instances-in-props branch from ab8fa95 to e50a8ba Compare March 31, 2023 11:04

refactor[devtools]: forbid editing class instances in props

651a028

hoxyq force-pushed the refactor/forbid-editing-class-instances-in-props branch from e50a8ba to 651a028 Compare March 31, 2023 13:55

hoxyq marked this pull request as ready for review March 31, 2023 14:38

hoxyq merged commit b14f8da into facebook:main Apr 3, 2023

hoxyq deleted the refactor/forbid-editing-class-instances-in-props branch April 3, 2023 10:32

sammy-SC mentioned this pull request Apr 5, 2023

Use native scheduler if defined sammy-SC/react#1

Closed

hoxyq mentioned this pull request Apr 17, 2023

React DevTools 4.27.4 -> 4.27.5 #26637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor[devtools]: forbid editing class instances in props #26522

refactor[devtools]: forbid editing class instances in props #26522

hoxyq commented Mar 30, 2023 •

edited

Loading

mondaychen left a comment

mondaychen Mar 30, 2023

hoxyq Mar 31, 2023

mondaychen Mar 31, 2023

react-sizebot commented Mar 31, 2023

refactor[devtools]: forbid editing class instances in props #26522

refactor[devtools]: forbid editing class instances in props #26522

Conversation

hoxyq commented Mar 30, 2023 • edited Loading

Summary

Demo

mondaychen left a comment

Choose a reason for hiding this comment

mondaychen Mar 30, 2023

Choose a reason for hiding this comment

hoxyq Mar 31, 2023

Choose a reason for hiding this comment

mondaychen Mar 31, 2023

Choose a reason for hiding this comment

react-sizebot commented Mar 31, 2023

Critical size changes

Significant size changes

hoxyq commented Mar 30, 2023 •

edited

Loading