[Fast Refresh] Fix crashes caused by rogue Proxies (#20030)

[kri]

Summary

This PR adds a safety mechanism to the heuristic that is currently being used to detect React components via duck typing (using $$typeof, see #20030).

In essence, this fixes crashes that could previously occur when processing rogue getters or Proxy instances.

[facebook-github-bot]

Hi @kri!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[gaearon]

Are there any other places where we access properties on arbitrary user code where it would be useful?

[codesandbox-ci]

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 e4112bf:

Sandbox	Source
React	Configuration

[sizebot]

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against e4112bf

[sizebot]

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against e4112bf

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[kri]

@gaearon:

Are there any other places where we access properties on arbitrary user code where it would be useful?

This might be yet another case:

react/packages/react-refresh/src/ReactFreshRuntime.js

Lines 146 to 148 in d95c493

	function isReactClass(type) {
	return type.prototype && type.prototype.isReactComponent;
	}

There's also a few more in isLikelyComponentType. The user-provided code would be the value of type:

react/packages/react-refresh/src/ReactFreshRuntime.js

Lines 665 to 674 in d95c493

	// eslint-disable-next-line no-proto
	if (type.prototype.__proto__ !== Object.prototype) {
	// It has a superclass.
	return false;
	}
	// Pass through.
	// This looks like a regular function with empty prototype.
	}
	// For plain functions and arrows, use name as a heuristic.
	const name = type.name || type.displayName;

If you approve, I'll add the guard there as well.

[kri]

Code snippet 1

This might be yet another case:

Actually, scratch that.

isReactClass is only called by canPreserveStateBetween, which in turn is called by performReactRefresh and only passes types that are already registered. I already guarded register, so guarding its dependents would be unnecessary.

Code snippet 2

The code I had referenced is part of a switch-block, and to be more precise, it's from the case 'function': case. typeof new Proxy({}, {}) is "object", so guarding this branch will not be necessary for Proxy instances.

So finally, let's think about when we enter the case 'function': case. Exporting getters isn't really a thing, since "getter" only makes sense in the context of a surrounding object or class in which the getter is encapsulated. You could still export the value of a getter like so:

const context = {};
Object.defineProperty(context, "example", {
  get() {
    throw new Error();
  }
});
export default context.example;

But then, the throw would occur before react-refresh even gets to touch it.

If instead, we were to export context or wrap a Proxy around a function, we'd enter the case 'object': case again; so I'm inclined to think that there is no way how a rogue getter could make it in the second code snippet.

So I suppose this is really all that needs to be done?

[gaearon]

It would be nice to add a regression test to ReactFreshIntegration-test or similar.

[kri]

I've added tests for the following cases:

• rogue Proxy
• benign Proxy wrapped around a functional component
• benign Proxy wrapped around a class component

Let me know if there's anything else I can do.

[gaearon]

Awesome. Thanks.