-
Notifications
You must be signed in to change notification settings - Fork 81
feat(compartment-mapper)!: create Compartment Map Transforms #2929
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
feat(compartment-mapper)!: create Compartment Map Transforms #2929
Conversation
156a86f to
851147b
Compare
3a8f22e to
8fe9e4d
Compare
851147b to
65cfc31
Compare
8fe9e4d to
dc25949
Compare
a453dec to
d2e9f3f
Compare
|
Needs some snapshot fixes. |
851f9f3 to
22481cd
Compare
dc25949 to
73bc30e
Compare
22481cd to
3551c19
Compare
- Adds `include` to policy for force-loading compartments during generation - Upgrades `@endo/compartment-mapper` and `ses` to latest - Removes custom `Compartment` subclass for policy generation - Reorganize reporting functionality into its own modules - Use package descriptors from `mapNodeModules()`' `PackageCompartmentDescriptor`s to determine if missing modules are optional or not, and squelch warnings if they are - Rename `root` CLI arg to `project-root` (backwards compat) - Reorganize text formatting into its own module (`format.js`) - Overhaul options-bag types - Differentiate _merged_ policies from unmerged policies for safety - Fixed many bugs, including some additional work around reading policies - Update many tests, snapshots, fixtures
| ]), | ||
| ); | ||
| }; | ||
|
|
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.
(barely actionable) I'm a bit lost in the layers between import/archive/bundle top level APIs and then the -lite bits and then the digest. It's not intuitive to me what divides responsibility into those layers. That might be on me, I'll put some more effort into understanding it but I consider it signal that it could be made easier to reason about (or diagrammed?)
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.
Might be helpful to look at Kris' PR that split these up. It attempts to explain this in the docblocks at the top of the files, too, I think.
| ); | ||
| } | ||
| if (module.deferredError !== undefined) { | ||
| if (isErrorModuleDescriptorConfiguration(module)) { |
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.
these are definitely a step in the right direction. It'd be great to have a relationship diagram of all the descriptor related types handy at all times too.
| dev = false, | ||
| commonDependencies = {}, | ||
| policy, | ||
| policyOverride, |
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.
(should) I disagree with node-modules being aware of the concept of policyOverride. I'm not sure if I like Endo being aware of it at all. Might be just a naming issue, I'm not sure what it is yet. I couldn't find a single example of them being passed in too. IT seems like it's a dedicated pass-through for lavamoat to put things in the context of transform, which can be achieved by maketransform: ({policyOverride}) => (stuff) => {}
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 a carve-out, yes. I hesitated to add this and the "create" transform. These could both live in @lavamoat/node instead (with consequences; see below)
| const transforms = [ | ||
| ...defaultCompartmentMapTransforms, | ||
| ...compartmentMapTransforms, | ||
| ]; |
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.
transforms should be outside of node-modules. everything they rely on (and themselves) gets here by prop drilling. Might as well be applied on the outside.
One way to do that:
- add a compartmentMapBuilder
- make it call mapNodeModules
- give it the responsibility for validation
- give it the responsibility for transforms
- potentially also give it the responsibility for adding package policy (in a transform or not) so that node-modules becomes unaware of what policy even is.
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 see that point. Originally I had thought these transforms could be applicable to all manner of CompartmentMapDescriptors, which set me off on trying to determine what those are, exactly (and you see the result here). But at the time I believed (I am not sure any longer) that the CompartmentMapTransformContext API would necessarily differ based on the subtype of CompartmentMapDescriptor. And since mapNodeModules() is the only place that uses a PackageCompartmentMapDescriptor and presumably transforms only work on PackageCompartmentMapDescriptors, I found it appropriate to stuff in there.
Furthermore, the enforcePolicyTransform should be run by mapNodeModules() or we've introduced another breaking change. Might be worth it?
I will go back and validate my assumptions. I think since I added the canonical name as the label prop, it would be feasible to extract into its own module and possibly work on any CompartmentMapDescriptor.
EDIT: FWIW not including the "creation" transform in Endo breaks a bunch of stuff that used to work w/r/t dynamic requires. Might be fine, but it's just more ruin.
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.
There might be a misunderstanding here. I wanted transforms to happen outside node-modules.js but not much further away. The work of the module ends with the transforms, so moving them outside didn't seem that big of a deal.
the enforcePolicyTransform should be run by mapNodeModules() or we've introduced another breaking change
how?
| canonicalNameMap, | ||
| optionsForTransforms, | ||
| ); | ||
|
|
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.
(not sure about any of this, but posting anyway) each transform iterates over the whole compartmentMap on its own. They contain the unnecessary flexibility of running their own iteration. I know you said they have effects that require global coordination, but I'd like to challenge that. Let's expore the possibility that a transform consisting of init() or a constructor and transformCompartment where init creates a context object that's available to transformCompartment (either via scope or by being passed around as first arg) would be enough for all reasonable purposes. We could then iterate compartments once and for each apply the changes.
Both existing transforms are making changes in a sequence without depending on reading the entire map first, which leads me to a conclusion that they don't depend on owning their iteration.
Even if we don't apply them in one iteration, taking the concern of skipping ATTENUATORS_COMPARTMENT away from transforms and letting them have a separate method for transforming the entry compartment would simplify (IMHO) reading them.
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.
You're suggesting, instead, a CompartmentDescriptorTransform, right? Where iteration is owned by Endo, yes?
At any rate, such a transform must have access to the entire compartment map. There's no other way to create relationships between two arbitrary compartment descriptors without it. We'd need to supply the name of the entry compartment in this init context (otherwise it'd need to iterate looking for the compartment descriptor having the ENTRY_COMPARTMENT label) as well.
They don't need to touch ATTENUATORS_COMPARTMENT, no. But they do need to be able to monkey with the entry compartment.
Recall these can be async, which may negatively impact performance if we were to iterate this way; there would be at minimum transformer-count * compartment-count async frames added and then popped off the stack because these things should not run concurrently
Ultimately I am not sure that the damage that can be done to a CompartmentDescriptor is much less worse than the damage that could be done to a CompartmentMapDescriptor.
If we really wanted to constrain things and ensure referential integrity, then the CompartmentMapTransformContext should have methods for modifying the compartment map (how many, exactly?); it should otherwise be read-only. And all of that points to CompartmentMapDescriptor being a class and owning responsibility for maintaining its own integrity.
I'm not actually opposed to any of that, since I think managing a compartment map containing string references is a footgun & prone to errors. IMO, It'd be better to have a properly serializable/deserializable class maintaining its own object references.
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.
...but at some point this PR needs to stop expanding, I suppose.
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.
That would be too compelx. POJO representation makes debugging easier and leaves more flexibility for future decisions. A class would cement the design and be costly to change later. I was thinking of limiting the flexibility a little, not entirely; mostly in order to hide some complexity from transforms.
| typeof value === 'object' && | ||
| !isArray(value) && | ||
| values(value).every(item => typeof item === 'boolean'); | ||
|
|
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.
(nit) Split each transform into a separate file, aggregate builtin transforms in index.js
packages/compartment-mapper/src/compartment-map-transforms/transforms.js
Outdated
Show resolved
Hide resolved
|
|
||
| const packagePolicy = getPackagePolicy( | ||
| compartmentDescriptor, | ||
| policy ?? policyOverride, |
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 seems to be the only use of this varaible that gets drilled down from afar. I really don't see the point of it being passed through all those layers and the name makes this treatment here incorrect unless the only purpose of it is to add missing stuff to compartment map, which I know from wider context is the case. That being said, this approach seems incorrect to me. if we're using policyOveride for the lavamoat usecase here, it should either already be merged into policy before it becomes endo policy or passed into a custom transform that lavamoat provides. SO either this gets extracted and goes into lavamoat to serve only lavamoat purposes or we make sure it only relies on endo policy and not on a concept that doesn't really exist in endo. (I accept this might be a misunderstanding stemming form policyOverride meaning something else)
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 needed a way to affect the "create" transform w/o affecting the "enforce" transform. This would be for the policy gen case. We'd use the translated lavamoat policy override to tell the "create" transform to make our associations from it. The policy here will be undefined (which also causes the "enforce" transform to bail). No packagepolicy is applied to CompartmentDescriptor.policy in this case.
We can absolutely remove policyOverride if we rip out the "create" transform. Removing it does have a negative impact on the functionality of captureFromMap w/r/t dynamic imports/requires—which might not necessarily be bad, since the intent of captureFromMap is kind of fuzzy nowadays.
At runtime, policy will be preferred over policyOverride if both are provided (there are tests for the interaction between the two options; you must have missed them).
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.
Something is not right with the design here and I don't think there has to be impact on functionality to fix the design.
Maybe the transforms are not independent afterall and need to go together (eliminating the need for a separate policyOverride)
Maybe the logic of applying transforms needs to change to acomodate the create transform working with dynamic requires even if passed in by lavamoat.
At this point I'm convinced a better design exists and we are close, no need to turn everything upside down to get to it.
packages/compartment-mapper/src/types/compartment-map-schema.ts
Outdated
Show resolved
Hide resolved
- Adds `include` to policy for force-loading compartments during generation - Upgrades `@endo/compartment-mapper` and `ses` to latest - Removes custom `Compartment` subclass for policy generation - Reorganize reporting functionality into its own modules - Use package descriptors from `mapNodeModules()`' `PackageCompartmentDescriptor`s to determine if missing modules are optional or not, and squelch warnings if they are - Rename `root` CLI arg to `project-root` (backwards compat) - Reorganize text formatting into its own module (`format.js`) - Overhaul options-bag types - Differentiate _merged_ policies from unmerged policies for safety - Fixed many bugs, including some additional work around reading policies - Update many tests, snapshots, fixtures
This fixes a missing `SomePackagePolicy` type in `policy.js`.
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`.
The `PolicyItem` type was having compatibility issues with `SomePackagePolicy`; this changes `PolicyItem` to a non-distributive conditional such that a type argument of `void` defaults to the "base" type. End-users can still provide type arguments for custom `PolicyItem`s.
BREAKING CHANGE: The `CompartmentDescriptor.label` property now represents a _canonical name_. `CompartmentDescriptor.path` has been removed. Filenames in created archives are no longer percent-encoded. Compartments no longer have implicit access to ancestors during dynamic requires. Access to the root compartment via policy is now supported using the canonical name `$root$`. Many type changes.
…mentMapDescriptor
6df0cc6 to
d21364a
Compare
73bc30e to
876adf6
Compare
- Adds `include` to policy for force-loading compartments during generation - Upgrades `@endo/compartment-mapper` and `ses` to latest - Removes custom `Compartment` subclass for policy generation - Reorganize reporting functionality into its own modules - Use package descriptors from `mapNodeModules()`' `PackageCompartmentDescriptor`s to determine if missing modules are optional or not, and squelch warnings if they are - Rename `root` CLI arg to `project-root` (backwards compat) - Reorganize text formatting into its own module (`format.js`) - Overhaul options-bag types - Differentiate _merged_ policies from unmerged policies for safety - Fixed many bugs, including some additional work around reading policies - Update many tests, snapshots, fixtures
…stedCompartmentMapDescriptor
876adf6 to
2ad8b12
Compare
|
Ref: #2988 |
- Adds `include` to policy for force-loading compartments during generation - Upgrades `@endo/compartment-mapper` and `ses` to latest - Removes custom `Compartment` subclass for policy generation - Reorganize reporting functionality into its own modules - Use package descriptors from `mapNodeModules()`' `PackageCompartmentDescriptor`s to determine if missing modules are optional or not, and squelch warnings if they are - Rename `root` CLI arg to `project-root` (backwards compat) - Reorganize text formatting into its own module (`format.js`) - Overhaul options-bag types - Differentiate _merged_ policies from unmerged policies for safety - Fixed many bugs, including some additional work around reading policies - Update many tests, snapshots, fixtures
Closes: #2894
Description
BREAKING CHANGE: The
CompartmentDescriptor.labelproperty now represents a canonical name.CompartmentDescriptor.pathhas been removed. Filenames in created archives are no longer percent-encoded. Compartments no longer have implicit access to ancestors during dynamic requires. Access to the root compartment via policy is now supported using the canonical name$root$. Many type changes.This is a whole-ass heap of changes because the Compartment Map Transform implementation is based on the breaking changes to compartment maps. 😢 It may be possible to split this into two PRs (the Compartment Map Transform implementation following one containing the breaking changes), but it would involve reverting changes to
mapNodeModules()and many tests.At a high level, this provides an API for consumers to make arbitrary changes to
CompartmentMapDescriptors via plugin-like Compartment Map Transform functions.Compartment Map Transforms
These transforms are applied by
mapNodeModules()immediately after creation of the initialCompartmentMapDescriptor. Two transforms are built-in, and are always executed:enforcePolicyTransform- when a policy is provided, this transform is responsible for removingModuleDescriptors from theCompartmentDescriptors in the map which are not explicitly allowed by policy. This used to bemapNodeModules()' responsibility.createReferencesByPolicyTransform- This is new behavior and can be thought of as the inverse ofenforcePolicyTransform; this transform addsModuleDescriptors andScopeDescriptors toCompartmentDescriptors based on policy. This solves quite a bit of our policy-related problems at once and means we can revert some of my hacks for dynamic requires (no more implicit access to "related" compartments; no moreCompartmentDescriptor.compartmentsproperty).Each
CompartmentMapTransformFnreceives arguments containing aCompartmentMapTransformContextobject. This is a public API containing helper functions. See the type definitions for more information on what's included.Type Overhaul
This overhauls the types for
CompartmentMapDescriptorsuch that theCompartmentMapDescriptorreturned bycaptureFromMap()is not the same type as theCompartmentMapDescriptorreturned bymapNodeModules()(which has always been true, but was not previously expressed). In addition, theModuleDescriptorandModuleSourcetypes have been narrowed. BecauseModuleDescriptorconflicts withses'ModuleDescriptor, this type has been renamed toModuleDescriptorConfigurationbecauseModuleDescriptorDescriptorsmells like Java and @kriskowal is allergic to abbreviations.I am open to any suggestions for naming these new types; there are a lot of them and others probably better understand the intent behind them.
Label is now a Canonical Name
This changes the
CompartmentDescriptor.labelproperty to be a canonical name which matches that found in policy. Since the canonical name is now owned by theCompartmentDescriptorobject itself, we can drop thepathproperty and simplify several other policy-related utility functions.This had a specific impact on filenames in archives; while
foo-v1.2.3would be used verbatim as a "directory" name,bar>foobecamebar%3Efoowhich ultimately meant it could not be successfully matched to aCompartment. To solve this, I changed how the filepath is computed to match how the filepath is retrieved. Previously, computation involved passing a name and module specifier thruURL, which caused the percent-encoding. Now, we just make a/sandwich from the two--which happens to be the same strategy as how filenames were computed for retrieval.More Logging
The
logoption is now handled by other public APIs. When running tests, you can set env varSCAFFOLD_LOGGING=1to cause log messages to be echoed thru AVA'st.log(). I added a dev dep on@endo/env-optionsto do this, but I probably didn't need to.Using
assert.quote()Replaced
JSON.stringifywithassert.quotewhere appropriate (which is most places).Common Type Guards
Many public APIs want to check what kind of
ModuleDescriptorConfigurationorModuleSourcethey are working with. These checks are now type-safe via guards found insrc/guards.js.Security Considerations
If you use policy, the
CompartmentMapDescriptorreturned bymapNodeModules()will more accurately reflect what you meant. Access to the entry compartment by other compartments may be bad; use$root$with caution.Scaling Considerations
mapNodeModules()is likely slower, but not an order of magnitude slower.Documentation Considerations
Lots of user-facing changes need documentation
Testing Considerations
Tests were harmed during the conception of this PR.
Compatibility Considerations
Improves ecosystem compatibility for running tools such as Webpack and ESLint which often want to touch files in entry Compartment.
Upgrade Considerations
This PR includes BrEaKiNg ChAnGeS. I am not sure if archives created previously will still be loadable; if policy is involved, things will probably break.