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

Importable version of reflect-metadata that does not shim globals. #130

Closed
rbuckton opened this issue Apr 13, 2021 · 6 comments · Fixed by #144
Closed

Importable version of reflect-metadata that does not shim globals. #130

rbuckton opened this issue Apr 13, 2021 · 6 comments · Fixed by #144
Assignees

Comments

@rbuckton
Copy link
Owner

Related:

The various APIs added to Reflect by reflect-metadata are incompatible with SES. One option is to introduce an "importable" version that can still work with TypeScript's --emitDecoratorMetadata option:

// --module esnext
// --module es2020
// --module es2015
import { Reflect } from "reflect-metadata/no-conflict";

// --module system
// --module commonjs
// --module amd
// --module umd
import { Reflect as _Reflect } from "reflect-metadata/no-conflict";
const Reflect = _Reflect; // unfortunately necessary due to how TypeScript emits import aliases to preserve live bindings.

This version of Reflect would wrap the built-in global Reflect object so that existing TypeScript emit for the __metadata helper can continue to work while allowing access to global Reflect functionality.

@erights: Unfortunately, I cannot change the default behavior of the main module without it being a major breaking change. Is it possible to detect whether code is running in an SES environment so as not to attempt mutation of the global Reflect? If that's the case, I could investigate a hybrid approach that still performed shimming when non in an SES environment, but also provided exports for the various methods.

@rbuckton rbuckton self-assigned this Apr 13, 2021
@erights
Copy link

erights commented Apr 13, 2021

Easiest and most obviously relevant test, not at all SES specific, would be

Object.isExtensible(Reflect)

if the answer is false then you know that you can't monkey patch Reflect anyway and need to do something else. Under SES, and in the tc53 standard environment for embedded JavaScript, the answer will always be false.

@rbuckton
Copy link
Owner Author

Wasn't the problem that errors were reported after reflect-metadata had already patched globals when lockdown gets called?

Checking extensibility is easy enough, but I'm not sure it would help. I'm also not sure if just checking for the presence of lockdown and harden from SES would justify not patching by default, or if that's even feasible since that would be dependent on import order.

My biggest concern with how reflect-metadata works in an SES environment is that it allows side-channel communications due to the use of a WeakMap (i.e., you can attach metadata to a global). I wonder whether forbidding metadata mutation if the object is not extensible would be a valid mitigation, and if so, whether there is a way for an SES user to whitelist the patched API.

I'm wary that changing the current behavior to one that prevents metadata mutation on frozen objects could be a breaking change, but perhaps there is a way I could lock down the metadata API in response to a call to SES's lockdown.

I've started on a no-conflict import that avoids global mutation in any case.

@erights
Copy link

erights commented Apr 13, 2021

without it being a major breaking change

Major breaking change to what, precisely? After you introduce no-conflict and change appropriate things to use it, what would remain broken? Would those things just be broken under SES or do you have some other workaround in mind?

@rbuckton
Copy link
Owner Author

I was considering whether it would be feasible to fix the main module in some fashion rather than adding a no-conflict export (which would essentially be just a copy of the sources with some minor changes).

@erights
Copy link

erights commented Jun 9, 2021

Hi @rbuckton any progress on this?

@conartist6
Copy link

I need a metadata reflection package that works today with current versions of TS and without introducing unsupported features into the language. I'll try my hand at building and publishing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants