endo icon indicating copy to clipboard operation
endo copied to clipboard

feat: add reflect metadata taming

Open Jack-Works opened this issue 1 year ago • 8 comments

Refs: #1950

Description

reflect-metadata is a widely used package in the ecosystem. It is used to polyfill an outdated ECMAScript proposal about decorators.

It modifies the global Reflect namespace object to add several methods. Although in https://github.com/rbuckton/reflect-metadata/pull/131, @rbuckton added a /no-conflict version of this package, it is impossible to upgrade every package in the ecosystem.

This PR adds an option called reflectMetadataTaming. It has 3 valid options:

  • none (default): Current behavior, not try to fix it and cause reflect-metadata throw.
  • unsafe-keep-and-inherit: If the reflect-metadata methods are added before the lockdown is called, they will be preserved like they are safe primorials. They will also be inherited by any subcompartments.
  • mutable-per-global: The Reflect namespace is an exotic object and unique per-global. Reflect-metadata can set (prior 0.2.0) or define methods as it wishes.

Example:

unsafe-keep-and-inherit, install polyfill before lockdown

import './lockdown.umd.js'
import 'https://cdnjs.cloudflare.com/ajax/libs/reflect-metadata/0.2.2/Reflect.js'
lockdown({ reflectMetadataTaming: 'keep-and-inherit', errorTaming: 'unsafe', consoleTaming: 'unsafe' })
console.log(Reflect.metadata) // function

globalThis.c = new Compartment()
console.log(c.globalThis.Reflect === Reflect) // true

unsafe-keep-and-inherit, install polyfill after lockdown

import './lockdown.umd.js'
lockdown({ reflectMetadataTaming: 'keep-and-inherit', errorTaming: 'unsafe', consoleTaming: 'unsafe' })
await import('https://cdnjs.cloudflare.com/ajax/libs/reflect-metadata/0.2.2/Reflect.js')
// Cannot define property decorate, object is not extensible

mutable-per-global, install polyfill inside subcompartment, but not the initial one

import './lockdown.umd.js'
lockdown({ reflectMetadataTaming: 'mutable-per-global', errorTaming: 'unsafe', consoleTaming: 'unsafe' })

globalThis.c = new Compartment()
console.log(c.globalThis.Reflect === Reflect) // false

var data = await fetch('https://cdnjs.cloudflare.com/ajax/libs/reflect-metadata/0.2.2/Reflect.js').then(x => x.text())
c.globalThis.eval(data)
console.log(c.globalThis.Reflect.metadata) // function
console.log(Reflect.metadata) // undefined

Security Considerations

When using keep-and-inherit, all compartment and the initial compartment shares the Metadata Registry (which is viable by Reflect[Symbol.for('@reflect-metadata:registry')]). They may exchange objects via this registry and break the membrane if there is one.

Scaling Considerations

No problem.

Documentation Considerations

If you're hitting problems with reflect-metadata, you might need this.

Testing Considerations

I have not added any tests yet. I hope I can iterate the PR design with the endo team and write the tests after the design is stable.

Compatibility Considerations

No breaking changes.

Upgrade Considerations

No breaking changes.

Update NEWS.md for user-facing changes.

TODO.

Jack-Works avatar Jul 25 '24 16:07 Jack-Works

Please add the word 'unsafe' somewhere in the option currently named 'keep-and-inherit'

Done

Jack-Works avatar Jul 26 '24 03:07 Jack-Works

Reflect will likely house getIntrinsic as well, so i think a per compartment Reflect is already quite inevitable.

ljharb avatar Aug 06 '24 01:08 ljharb

@erights

For my use cases, unsafe-keep-and-inherit is enough,

permits.js remains as declarative as it is now. No imperative side effects to it based on lockdown options.

this is the best minimal change I can think of. If I patch this behavior in makeIntrinsicsCollector, I'll get more code changes than this.

If code in the start compartment wants to grant code in a constructed compartment access to its own unsafe Reflect, it does so by endowment, not by lockdown option. This recapitulates the transition we made with Math and Date, where we initially propagated the unsafe one by lockdown option, and then switched to propagation only by endowment

Can you give an example what this might be look like in the user land?

Jack-Works avatar Aug 07 '24 15:08 Jack-Works

For my use cases, unsafe-keep-and-inherit is enough,

Excellent!

Can you give an example what this might be look like in the user land?

const compartment = new Compartment({
    globals: { Reflect },
    __options__: true, // only needed during a compatibility transition
  });

Whatever Reflect is already bound to in this calling code, it passes as the Reflect endowment for the globals of the newly constructed compartment.

erights avatar Aug 10 '24 02:08 erights

@erights If I read this correctly, this feature can be implemented with very little behavior contingent on a lockdown option.

  1. Separating %Reflect% from %SharedReflect% is safe in general. We can visit a per-compartment Reflect when we realize a future need.
  2. Leaving %Reflect% between repairIntrinsics and hardenIntrinsics is the normal behavior.

Adding %Reflect%.metadata to the permits might introduce a hazard in the event the Reflect is later hardened and endowed. Do we need an option?

kriskowal avatar Aug 21 '24 21:08 kriskowal

Adding %Reflect%.metadata to the permits might introduce a hazard in the event the Reflect is later hardened and endowed. Do we need an option?

If the reflect-metadata package runs after repairIntrinsics as a vetted shim, then it runs after the permit processing, so we do not need to add anything to permits.js to accommodate all the stuff it monkey patches in to the start compartment's Reflect.

Right now, we don't support (well?) clients of ses running outside libraries as vetted shims between repairIntrinsics and hardenIntrinsics. But if we can somehow arrange that the flag leaves the start compartment's Reflect mutable even after hardenIntrinsics, then reflect-metadata can run after lockdown, still without needing to accommodate any added property in permits.js.

As you say, we do need permits.js to distinguish the start compartment's Reflect from the Reflect shared by constructed compartments.

erights avatar Aug 22 '24 02:08 erights

Hi @Jack-Works , What is the status of this?

erights avatar Nov 25 '24 15:11 erights

Hi @Jack-Works , What is the status of this?

Hi! I've been recently working on other projects, but this one is still on my TODO list. I'll keep it up to date after I come back to this.

Jack-Works avatar Nov 25 '24 17:11 Jack-Works