Fix: ReactDevTool crashes if we have sandbox iframe in props #19994

omarsy · 2020-10-09T19:07:57Z

Summary

When we have an iframe in the props it causes a crash in reactdevtool.
You can see here a demo of the crash: https://codesandbox.io/s/modest-moser-jg9x9?file=/src/index.js.
Before:

After:

Maybe correct this issue #19854 (comment)

Test Plan

codesandbox-ci · 2020-10-09T19:13:42Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 77bac8c:

Sandbox	Source
React	Configuration
unruffled-microservice-jo8s2	PR

sizebot · 2020-10-09T19:15:29Z

	Warnings
⚠️	Could not find build artifacts for base commit: `4890779`

Size changes (stable)

Generated by 🚫 dangerJS against 77bac8c

sizebot · 2020-10-09T19:16:09Z

	Warnings
⚠️	Could not find build artifacts for base commit: `4890779`

Size changes (experimental)

Generated by 🚫 dangerJS against 77bac8c

bvaughn · 2020-10-16T13:46:14Z

Thanks for the PR @omarsy. I have a few questions though before moving forward.

bvaughn · 2020-10-16T13:40:34Z

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

+    case 'window':
+      try {
+        // eslint-disable-next-line no-unused-expressions
+        data.origin;
+      } catch {
+        cleaned.push(path);
+        return {
+          inspectable: false,
+          preview_short: 'Window',
+          preview_long: 'Window',
+          name: 'Window',
+          type,
+        };
+      }
+    // eslint-disable-next-line no-fallthrough


I don't think we should use the fall through here. Even with the explicit lint rule, it's hard to read and would be easy to break.

Why shouldn't we return the window object regardless of the try/catch?

If we do need the current behavior for a reason I'm not seeing, can we rewrite to make it more explicit, e.g.

case 'object': case 'window': if (type === 'window') { try { // eslint-disable-next-line no-unused-expressions data.origin; } catch { cleaned.push(path); return { inspectable: false, preview_short: 'Window', preview_long: 'Window', name: 'Window', type, }; } } isPathAllowedCheck = isPathAllowed(path); if (level >= LEVEL_THRESHOLD && !isPathAllowedCheck) { return createDehydrated(type, true, data, cleaned, path); } else { const object = {}; getAllEnumerableKeys(data).forEach(key => { const name = key.toString(); object[name] = dehydrate( data[key], cleaned, unserializable, path.concat([name]), isPathAllowed, isPathAllowedCheck ? 1 : level + 1, ); }); return object; }

Why shouldn't we return the window object regardless of the try/catch?

I want to keep the behaviour we had before. We used to display all the elements of the Window object when we could access it.

bvaughn · 2020-10-16T13:41:13Z

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

@@ -325,7 +339,6 @@ export function dehydrate(
      return {
        type,
      };
-


nit: Keep the empty line on 298 and 328 for consistency with the rest of the file 😉

:-D Reverted

bvaughn · 2020-10-16T13:43:21Z

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

+  if (data.self === data && data.parent && data.top) {
+    return 'window';
+  }


This seems like an convoluted way to check for window objects. Why wouldn't something like data instanceof Window work?

I'm also a little confused about why this fix (for window elements) is required to fix the linked bug about iframes. Wouldn't we need to be adding a check for data instanceof HTMLIFrameElement here instead of window?

Hello @bvaughn

This seems like an convoluted way to check for window objects. Why wouldn't something like data instanceof Window work?

It doesn't always work for Iframe it considers that sometimes it is an Object.
I've been looking at how the others are dealing with it:
JQuery: https://api.jquery.com/jquery.iswindow/
Anglular: https://github.com/angular/angular.js/blob/master/src/Angular.js#L677
I added some check to make it more robust but i flow the same behaviour.

I'm also a little confused about why this fix (for window elements) is required to fix the linked bug about iframes. Wouldn't we need to be adding a check for data instanceof HTMLIFrameElement here instead of window?

I tested with the iframe element. It works

I'm not sure I understand. This check doesn't really look like the two you linked to (JQuery and Angular). It also looks like it could hit a false positive, e.g.

// This would be mistake as type "window" const fake = { parent: 1, top: 1, // ... }; fake.self = fake;

But it wouldn't if the check was:

if (data instanceof Window || data instanceof HTMLIFrameElement) { return 'window'; }

So I'm curious why this doesn't work?

The JQuery function you linked to has been deprecated since v3.3 which is almost 3 years old, and the Angular check you linked to looks like it's in Angular v1 which is also deprecated.

Okay I think I've come around to the necessity of this "looks like a window" check, although I think we need an inline comment here explaining the reason we check this way (aka the iframe.contentWindow case).

Maybe we could also handle the more common case explicitly? I dunno, maybe something like this?

// We fallback to duck typing to handle an edge case: // An HTMLIFrameElement's "contentWindow" is not an instance of Window, // but rather an instance of the iframe's own Window (which we can't reliably access). if ( data instanceof Window || data instanceof HTMLIFrameElement || (data.self === data && data.parent != null && data.top != null) ) { return "window"; }

Also, maybe this whole conditional should be moved down into the case 'object': block.

omarsy · 2020-11-13T19:27:47Z

Sorry @bvaughn for the delay.

bvaughn · 2020-11-13T22:13:10Z

packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js

+    // Todo: Uncomment if this feature is implemented https://github.com/jsdom/jsdom/issues/1892
+    // const iframe = document.createElement('iframe');
+    // iframe.setAttribute('sandbox', '');
+    // window.document.body.appendChild(iframe);


Can we add something like this as a fixture test in the meanwhile? (It doesn't seem too likely that JSDom will implement this.)

Ok I will add the test :)

bvaughn · 2020-11-13T22:15:31Z

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

+  if (data.self === data && data.parent && data.top) {
+    return 'window';
+  }


I'm not sure I understand. This check doesn't really look like the two you linked to (JQuery and Angular). It also looks like it could hit a false positive, e.g.

// This would be mistake as type "window" const fake = { parent: 1, top: 1, // ... }; fake.self = fake;

But it wouldn't if the check was:

if (data instanceof Window || data instanceof HTMLIFrameElement) { return 'window'; }

So I'm curious why this doesn't work?

The JQuery function you linked to has been deprecated since v3.3 which is almost 3 years old, and the Angular check you linked to looks like it's in Angular v1 which is also deprecated.

omarsy · 2020-11-13T22:56:55Z

I'm not sure I understand. This check doesn't really look like the two you linked to (JQuery and Angular). It also looks like it could hit a false positive, e.g.

Yes, I changed it on purpose. To avoid as many false positives as possible. But it is the same idea

But it wouldn't if the check was:

if (data instanceof Window || data instanceof HTMLIFrameElement) {
return 'window';
}
So I'm curious why this doesn't work?

I don't know why browsers only consider the main window as a window type and the window of iframes are considered as objects. That's why I've been looking at how other libs do it.
You can find an example of what it will return here: https://jsfiddle.net/fx3hmu2n/

bvaughn

I see! Thanks for explaining. I was checking the main window or the iframe itself, not the contentWindow.

Ping me once the fixture is ready 😄

bvaughn · 2020-11-16T13:41:46Z

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

+      if (type === 'window') {
+        try {
+          // eslint-disable-next-line no-unused-expressions
+          data.origin;


This could use an inline comment explaining the case(s) it's intended to catch.

bvaughn · 2020-11-16T13:43:24Z

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

@@ -78,7 +78,7 @@ export function getAllEnumerableKeys(
    const descriptors = Object.getOwnPropertyDescriptors(current);
    currentKeys.forEach(key => {
      // $FlowFixMe: key can be a Symbol https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyDescriptor
-      if (descriptors[key].enumerable) {
+      if (descriptors[key] && descriptors[key].enumerable) {


Do you have an example of where this would have thrown before? (Can we add a test for it to ensure that we don't regress?)

bvaughn · 2020-11-16T13:50:56Z

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

+  if (data.self === data && data.parent && data.top) {
+    return 'window';
+  }


Okay I think I've come around to the necessity of this "looks like a window" check, although I think we need an inline comment here explaining the reason we check this way (aka the iframe.contentWindow case).

Maybe we could also handle the more common case explicitly? I dunno, maybe something like this?

// We fallback to duck typing to handle an edge case: // An HTMLIFrameElement's "contentWindow" is not an instance of Window, // but rather an instance of the iframe's own Window (which we can't reliably access). if ( data instanceof Window || data instanceof HTMLIFrameElement || (data.self === data && data.parent != null && data.top != null) ) { return "window"; }

Also, maybe this whole conditional should be moved down into the case 'object': block.

stale · 2022-01-09T07:41:36Z

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

omarsy · 2022-01-09T12:15:41Z

bump

Fix: ReactDevTool crashes if we have sandbox iframe in props

9c05cd1

facebook-github-bot added the CLA Signed label Oct 9, 2020

remove unused case

00d4209

bvaughn reviewed Oct 16, 2020

View reviewed changes

refactor window case: not use fallthrough

77bac8c

omarsy requested a review from bvaughn November 13, 2020 19:47

bvaughn reviewed Nov 13, 2020

View reviewed changes

bvaughn reviewed Nov 16, 2020

View reviewed changes

stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2022

stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2022

omarsy mentioned this pull request Jul 26, 2022

DevTools: Improve browser extension iframe support #18945

Closed

sebmarkbage deleted the branch facebook:master October 20, 2022 20:41

sebmarkbage closed this Oct 20, 2022

Fix: ReactDevTool crashes if we have sandbox iframe in props #19994

Fix: ReactDevTool crashes if we have sandbox iframe in props #19994

Uh oh!

Conversation

omarsy commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

codesandbox-ci bot commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sizebot commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size changes (stable)

Uh oh!

sizebot commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size changes (experimental)

Uh oh!

bvaughn commented Oct 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omarsy commented Nov 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omarsy commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jan 9, 2022

Uh oh!

omarsy commented Jan 9, 2022

Uh oh!

Uh oh!

omarsy commented Oct 9, 2020 •

edited

Loading

codesandbox-ci bot commented Oct 9, 2020 •

edited

Loading

sizebot commented Oct 9, 2020 •

edited

Loading

sizebot commented Oct 9, 2020 •

edited

Loading

omarsy commented Nov 13, 2020 •

edited

Loading