-
Notifications
You must be signed in to change notification settings - Fork 81
feat(compartment-mapper): add "preload" option to captureFromMap() #2915
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
base: master
Are you sure you want to change the base?
Conversation
c31a862 to
e808d60
Compare
| compartmentDescriptor.modules[compartmentDescriptor.name]; | ||
|
|
||
| if (!compartmentOwnModuleDescriptor?.module) { | ||
| throw new Error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a CompartmentDescriptor not having a ModuleDescriptor referencing itself considered malformed?
e808d60 to
986159f
Compare
| * @returns {Promise<CaptureResult>} | ||
| */ | ||
| export const captureFromMap = async (powers, compartmentMap, options = {}) => { | ||
| export const captureFromMap = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
besides the refactors, I renamed:
- parameter
powers➡️readPowers - internal name of option
Compartment➡️CompartmentOption - module-scoped
defaultCompartment➡️DefaultCompartment - internal entry Compartment
compartment➡️entryCompartment - internal
Record<string, Compartment>post-linkingcompartments➡️linkedCompartments
| * Attenuator, | ||
| * SomePolicy, | ||
| * PolicyEnforcementField, | ||
| * SomePackagePolicy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this change, the generated policy.d.ts has no reference to SomePackagePolicy and is thus broken. I would expect lint:types to report this error, but it doesn't and I don't know why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: #2921
3a8f22e to
8fe9e4d
Compare
8fe9e4d to
dc25949
Compare
dc25949 to
73bc30e
Compare
73bc30e to
876adf6
Compare
|
With outside-facing compartment map transforms the use of a marker in compartment map indicating what to load looks appealing again. As a result adding missing links and linking start nodes based on policy overrides from lavamoat could happen in one transform provided by lavamoat. I like the idea that compartment-mapper's linking and loading would be an engine to do whatever compartment map says, with as little configurability as possible. Configurability made more sense before transforms, but if we land transforms they could replace a lot of it. |
|
@naugtur Yes, I may have mentioned above that I considered a similar design. I'm happy to go that way if we feel it's a better fit. Still, there will need to be an option somewhere because how else is |
kriskowal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m sold on the need for the feature and mostly sold on the implementation.
I think forceLoad should be framed as a set of {compartment, module} duples, where module may default to ".".
I was confused during my review by the verb “load” being applied to compartments, since we capture compartments and load modules. This is indeed loading modules, as synecdoche for the main module of a compartment. That could be clearer in choices of names.
Consider some more names for forceLoad:
preload(I think “force” is dissatisfying because we’re not overriding a safety measure.)entries(If this is an array of additional entry modules with the same type as theentryincompartment-map.json)
Consider, in fact, instead of a option, a way to add an entries field to the compartment map input to the argument. That might be threaded from an option higher up.
I had envisioned a feature of extra entries in compartment-map.json such that app.import(name) would produce one of the extra modules. This would be useful for staged evaluation of modules in a bundle.
{
"entry": { "module": ".", "compartment": "x" },
"entries": {
"y": { "module": ".", "compartment": "y"}
}
}| const { stringify: q } = JSON; | ||
|
|
||
| const defaultCompartment = Compartment; | ||
| const DefaultCompartment = Compartment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to say globalThis.Compartment on the RHS so it’s not a reference error, in cases where it’s intended to be injected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globalThis being undefined per ESLint seems like a misconfiguration. SES also does not assign its globals to globalThis, which incurs a TS error. I just wanted to note this.
Regardless, your suggestion is correct, so I will just squelch the TS error for now.
This fixes a missing `SomePackagePolicy` type in `policy.js`.
876adf6 to
2ad8b12
Compare
2ad8b12 to
8b5ac3e
Compare
This adds option `forceLoad` to `captureFromMap()`. This is an optional `string[]` of keys of the `CompartmentMapDescriptor` also provided to `captureFromMap()`. After loading the entry Compartment and attenuators Compartment, the Compartments having names (practically speaking, these will be _locations_) mentioned in `forceLoad` will then be loaded _if they were not already_. This option can be used to support dynamic requires and imports which would otherwise be omitted from the captured `CompartmentMapDescriptor` via digestion. * * * - Refactored `capture-lite.js`; stuffed all of the Compartment-loading business into a function. - Added test and fixture that shows how a `Compartment` which would otherwise be omitted from the captured Compartment Map is included when used with `forceLoad`.
8b5ac3e to
38e98bf
Compare
| linkedCompartments, | ||
| entryCompartment, | ||
| attenuatorsCompartment, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used in capture-lite. Would an application work at runtime when its policy generation situation depended on this? I'm assuming it'd go and do a dynamic import for what's not been preloaded. But wanted to point at it and say I'm not sure if using it only in one place is enough.
Prior art:
Ref: #2856
Ref: #2864
Ref: #2876
Description
This adds option
preloadtocaptureFromMap(). This is an optionalstring[]of keys of theCompartmentMapDescriptoralso provided tocaptureFromMap(). It can also be anArray<{compartment: string, entry: string}>to preload a specific module other than the entry of that compartment.After loading the entry
Compartment(and attenuatorsCompartment, if applicable), theCompartments having names (practically speaking, these will be locations) mentioned inpreloadwill then be loaded if they were not already.This option can be used to support dynamic requires and imports which would otherwise be omitted from the captured
CompartmentMapDescriptorvia digestion.Alternatives Considered
Besides the above references, I considered adding a
booleanfieldloadtoCompartmentDescriptor. But I don't see how@endo/compartment-mapperwould want to set this flag for any reason other than incaptureFromMap()on the entry and attenuators compartments. Thus, it seemed specific tocaptureFromMap()and so shouldn't be inCompartmentDescriptor.Supplying an array of file URL strings is kind of a pain for the end-user, but they're right there in the
CompartmentMapDescriptor, so it seems workable.Open to naming suggestions.
Security Considerations
Just the usual disclaimers about being careful what you put in the compartment map.
Scaling Considerations
There's a tradeoff: use of
preloadhas performance implications, since it causes (potentially many) module sources to be read and analyzed which would not otherwise be.Testing Considerations
There are probably some conditionals I've left uncovered.
Compatibility Considerations
Backwards-compatible
Upgrade Considerations
This is a user-facing change which should be described in
NEWS.md.