Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

failed to delete intrinsics #612

Closed
AidenRourke opened this issue Mar 15, 2021 · 25 comments
Closed

failed to delete intrinsics #612

AidenRourke opened this issue Mar 15, 2021 · 25 comments
Assignees
Labels
permits Issues pertaining to SES’s permits for properties of shared intrinsics taming compat

Comments

@AidenRourke
Copy link
Contributor

Everything was working fine and then all of a sudden I started to get this message:

Removing intrinsics.Array.@wry/context:Slot

Followed by this error:

failed to delete intrinsics.Array.@wry/context:Slot (TypeError#1)

The messages only appear if I call lockdown()

@erights
Copy link
Contributor

erights commented Mar 15, 2021

What version of what browser?

@erights erights self-assigned this Mar 15, 2021
@AidenRourke
Copy link
Contributor Author

Google Chrome, Version 89.0.4389.82 (Official Build) (x86_64)

@erights
Copy link
Contributor

erights commented Mar 15, 2021

Were you doing anything else in that window or frame before calling lockdown? Can you reproduce it in a fresh window?

@erights
Copy link
Contributor

erights commented Mar 15, 2021

Could you visit https://ses-demo.netlify.app/demos/console/
Open "More Tools" > "Developer Tools"
select the "Console" tab
refresh the page. What do you see on the console? On my Chrome I see

console-console

@AidenRourke
Copy link
Contributor Author

AidenRourke commented Mar 16, 2021

I don't see it in https://ses-demo.netlify.app/demos/console/

This is what my code looks like

import React from "react";
import ReactDOM from "react-dom";
import "./index.css";
import "ses";

window.lockdown();

ReactDOM.render(
    <div/>,
    document.getElementById("entry")
);

@AidenRourke
Copy link
Contributor Author

It's weird because it was working fine and then randomly started giving me the error. Like I didn't update my SES version or anything.

@erights
Copy link
Contributor

erights commented Mar 16, 2021

@erights
Copy link
Contributor

erights commented Mar 16, 2021

Attn @warner @kriskowal @dckc @michaelfig

@AidenRourke
Copy link
Contributor Author

Is there any hack I can use to get rid of this error? I use a locally downloaded version of ses.

@erights
Copy link
Contributor

erights commented Mar 16, 2021

What happens if you restart your browser?

How are you turning that code into something that runs in your browser? Are you using a bundler or packer or something?

@AidenRourke
Copy link
Contributor Author

It's basically a stock create-react-app so it uses webpack. I'll try restarting my browser.

@kriskowal
Copy link
Member

Looks like https://github.com/benjamn/wryware must be running first. Same author as https://github.com/benjamn/private, which is not SES compatible. They are probably employing similar tricks, which can now all be replaced with a WeakMap or private fields. @AidenRourke Can you trace your app’s dependency graph to see what’s using wryware?

@dckc
Copy link
Contributor

dckc commented Mar 16, 2021

I'm sort of able to reproduce it:

  1. npx create-react-app ses612
  2. cd ses612; yarn start
  3. all is well. React app with spinning log appears
  4. yarn add ses
  5. add 2 lines to index.js: import 'ses'; and window.lockdown()
  6. yarn start

expected behavior: all is well. React app works
actual behavior: blank screen. Error generating stack: Cannot redefine property: info in the console:

[HMR] Waiting for update signal from WDS...
scheduler.development.js:298 [Deprecation] SharedArrayBuffer will require cross-origin isolation as of M91, around May 2021. See https://developer.chrome.com/blog/enabling-shared-array-buffer/ for more details.
(anonymous) @ scheduler.development.js:298
ses.umd.js:4568 Removing intrinsics.Reflect.decorate
ses.umd.js:4568 Removing intrinsics.Reflect.metadata
ses.umd.js:4568 Removing intrinsics.Reflect.defineMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.hasMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.hasOwnMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.getMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.getOwnMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.getMetadataKeys
ses.umd.js:4568 Removing intrinsics.Reflect.getOwnMetadataKeys
ses.umd.js:4568 Removing intrinsics.Reflect.deleteMetadata
react-dom.development.js:789 Uncaught 
index.js:1 The above error occurred in the <App> component:

Error generating stack: Cannot redefine property: info


Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://reactjs.org/link/error-boundaries to learn more about error boundaries.
console.<computed> @ index.js:1
react-dom.development.js:23275 Uncaught 
DevTools failed to load SourceMap: Could not load content for chrome-extension://dmkamcknogkgcdfhhbddcghachkejeap/browser-polyfill.js.map: HTTP error: status code 404, net::ERR_UNKNOWN_URL_SCHEME

@erights
Copy link
Contributor

erights commented Mar 16, 2021

On the first Removing intrinsics.Array.@wry/context:Slot error, if it reappears:

Is there any hack I can use to get rid of this error? I use a locally downloaded version of ses.

If you don't care if you're still safe after doing that hack, in your download of ses, you can add it to the whitelist. But in order to whitelist it, we need to know what the type of its value is.

In whitelist-intrinsics.js you'll see

      try {
        delete obj[prop];
      } catch (err) {
        if (prop in obj) {
          console.error(`failed to delete ${subPath}`, err);
        } else {
          console.error(`deleting ${subPath} threw`, err);
        }
        throw err;
      }

Change that first call to console.log into

          console.error(`failed to delete ${subPath}: ${typeof obj[prop]}`, err);
          return; // to continue to see all other such errors

and let me know what the new error message says, or messages if these are more than one. Then we should be able to complete the following whitelisting instructions:

In whitelist.js you'll find an entry like

  Array: {
    // Properties of the Array Constructor
    '[[Proto]]': '%FunctionPrototype%',
    from: fn,
    isArray: fn,
    of: fn,
    prototype: '%ArrayPrototype%',
    '@@species': getter,
  },

Add a line at the end, changing it to

  Array: {
    // Properties of the Array Constructor
    '[[Proto]]': '%FunctionPrototype%',
    from: fn,
    isArray: fn,
    of: fn,
    prototype: '%ArrayPrototype%',
    '@@species': getter,
    '@wry/context:Slot': <something depending on what type its value is>
  },

@dckc
Copy link
Contributor

dckc commented Mar 16, 2021

Maybe "create-react-app and ses don't get along" should be a separate issue?

@AidenRourke
Copy link
Contributor Author

Restarting my browser didn't fix it. @dckc It's weird because I've been developing this for weeks now and it suddenly stopped working. That is to say, it WAS working with react until it suddenly stopped. I didn't update my ses version or add an npm module it just randomly started giving me the error.

@erights
Copy link
Contributor

erights commented Mar 16, 2021

On the error @dckc reports above , from the messages

ses.umd.js:4568 Removing intrinsics.Reflect.decorate
ses.umd.js:4568 Removing intrinsics.Reflect.metadata
ses.umd.js:4568 Removing intrinsics.Reflect.defineMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.hasMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.hasOwnMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.getMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.getOwnMetadata
ses.umd.js:4568 Removing intrinsics.Reflect.getMetadataKeys
ses.umd.js:4568 Removing intrinsics.Reflect.getOwnMetadataKeys
ses.umd.js:4568 Removing intrinsics.Reflect.deleteMetadata

a bit of searching reveals @rbuckton's https://github.com/rbuckton/reflect-metadata which looks like it does a bunch of primordial monkey patching (or shimming) of a future proposal he has in mind. None of these are on the ses whitelist, so ses lockdown removes them all. Since the ses whitelist doesn't even list them as things it expects to remove, it prints out this diagnostic to let us know that their appearance is a surprise. As it is! Since ses has no idea what they are, ses must remove them to preserve its own safety properties. However, whatever other code was counting on them being there is now broken.

I know @rbuckton and he knows me and ses, so perhaps we can get this resolved.

@rbuckton, does this indeed look like it is coming from your code? Do these additional properties introduce any primordial state or behavior that ses might find problematic? Whatever the functionality is, could you provide it in another manner, without monkey patching the primordials?

@erights
Copy link
Contributor

erights commented Mar 16, 2021

Restarting my browser didn't fix it. @dckc It's weird because I've been developing this for weeks now and it suddenly stopped working. That is to say, it WAS working with react until it suddenly stopped. I didn't update my ses version or add an npm module it just randomly started giving me the error.

Are you using yarn or npm? Do you have a yarn.lock or package-lock.json file from when it used to work? If you regenerate whichever file that is, is it different? This may be a clue that you upgraded one of your dependencies.

@erights
Copy link
Contributor

erights commented Mar 16, 2021

Did you upgrade your browser recently? (I doubt this could cause these symptoms anyway, but just checking)

@AidenRourke
Copy link
Contributor Author

AidenRourke commented Mar 16, 2021

@erights
I haven't upgraded my browser recently.
I don't have a package-lock or yarn.lock from when it was working but I didn't change any of the dependencies when it started giving me the error.
The output of console.error('failed to delete ${subPath}: ${typeof obj[prop]}', err); is that it is a function.
Would adding it to the whitelist open me up to a side channel attack through that property?

@erights
Copy link
Contributor

erights commented Mar 16, 2021

Would adding it to the whitelist open me up to a side channel attack through that property?

Possibly, or worse. Until we know what that function does, we must assume it is unsafe to leave it.

Could you place the call to lockdown into a separate file that you import first, so all your other imports happen after lockdown? They may fail, but their failure should give us interesting diagnostic info about how they are incompat with ses. For example, whatever code is adding that '@wry/context:Slot' property to the Array constructor will instead fail because the Array constructor is frozen first. Likewise with the additions to Reflect that @dckc reported.

@rbuckton
Copy link

rbuckton commented Mar 16, 2021

@rbuckton, does this indeed look like it is coming from your code?

Yes, that seems to be the case.

Do these additional properties introduce any primordial state or behavior that ses might find problematic?

I'm not certain on this count. The API essentially wraps a WeakMap that is used to store Metadata on an arbitrary JavaScript object or function in concert with decorators. I intended to propose this to TC39 once decorators landed, but that was some number of years ago and we are currently discussing Metadata amongst the decorator proposal stakeholders.

Whatever the functionality is, could you provide it in another manner, without monkey patching the primordials?

I do have an importable version, which is what https://esfx.js.org/esfx/api/metadata.html does. The problem is that TypeScript's __metadata helper/decorator checks for the existence of Reflect.metadata for use with --emitDecoratorMetadata. Changing that would require changes to the compiler and some design work around how to specify where to pull the decorator from, and wouldn't work for non-module scripts.

@AidenRourke
Copy link
Contributor Author

AidenRourke commented Mar 16, 2021

Updates:

  1. I tried making a brand new react app and importing ses/calling lockdown in the index.js file and I get the same error
  2. I tried adding a script tag with ses and calling lockdown in the public html file and I get the same error
  3. If I just have an html file that imports ses and calls lockdown there is no error

Note:
I don't get the same error as @dckc, I get failed to delete intrinsics.Array.@wry/context:Slot: function (TypeError#1)

@AidenRourke
Copy link
Contributor Author

  Array: {
    // Properties of the Array Constructor
    '[[Proto]]': '%FunctionPrototype%',
    from: fn,
    isArray: fn,
    of: fn,
    prototype: '%ArrayPrototype%',
    '@@species': getter,
    '@wry/context:Slot': <something depending on what type its value is>
  },

What should I add given that the type is "function"?

@erights
Copy link
Contributor

erights commented Mar 16, 2021

fn
as you see above with, for example, isArray: fn,

@erights erights added taming compat permits Issues pertaining to SES’s permits for properties of shared intrinsics labels Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
permits Issues pertaining to SES’s permits for properties of shared intrinsics taming compat
Projects
None yet
Development

No branches or pull requests

6 participants