feat(compartment-mapper): support mapping of undiscoverable packages #2856

boneskull · 2025-06-12T21:36:38Z

Description

This adds the additionalPackageLocations and additionalPackageDetails options to mapNodeModules(). The former is a mapping of module location to one or more additional module locations. The latter is a data structure mapNodeModules will populate, which can then be provided to captureFromMap (which is in turn provided to makeImportHookMaker().

This is intended to be used when dynamic requires prevent mapNodeModules & captureFromMap from otherwise creating a complete CompartmentMapDescriptor due to its ignorance of what would be dynamically required. Archival is not a use-case.

Also:

Fixed signature of chooseModuleDescriptor
Tweaked ESLint config to ignore my new fixture
Add options type for translateGraph
Fixed deprecation notice for compartmentMapForNodeModules; it was previously on the options object, but it should only be on the export.
graphPackages now accepts an optional Graph object to be able to re-use the same graph between runs

Questions for Reviewers

Naming is hard and I was not sure what to name the new options. The AdditionalPackageDetails type looks like the existing PackageDetails type (with a new field) thus influencing the naming—but they are otherwise unrelated and because of this I did not extend PackageDetails.
I am monkeying with imports in a RecordModuleDescriptor object in order to direct SES' module loading. Is there a better way? The things I stuff in imports are also extremely dubious.

Security Considerations

n/a

These changes do not meddle with systems loading untrusted code, afaik.

Scaling Considerations

Use of the new option is not free.

Documentation Considerations

n/a

Testing Considerations

I did add some tests, but could probably use more.

Compatibility Considerations

No.

Upgrade Considerations

This warrants an entry in NEWS.md since it is part of a public API.

This adds the `additionalPackageLocations` and `additionalPackageDetails` options to `mapNodeModules`. The former is a mapping of module location to one or more additional module locations. The latter is a data structure `mapNodeModules` will populate, which can then be provided to `captureFromMap` (which is in turn provided to `makeImportHookMaker()`. This is intended to be used when dynamic requires prevent `mapNodeModules` & `captureFromMap` from otherwise creating a complete `CompartmentMapDescriptor` due to its ignorance of what would be dynamically required. Archival is not a use-case. Also: - Fixed signature of `chooseModuleDescriptor` - Tweaked ESLint config to ignore my new fixture - Add options type for `translateGraph` - Fixed deprecation notice for `compartmentMapForNodeModules`; it was previously on the options object, but it should only be on the export. - `graphPackages` now accepts an optional `Graph` object to be able to re-use the same graph between runs

boneskull · 2025-06-12T21:39:43Z

packages/compartment-mapper/src/import-hook.js

+                // XXX: this "works" but that's probably a conincidence!
+                record.record?.imports?.push(
+                  `${compartmentDescriptors[additionalPackageLocation].name}${additionalPackageModuleSpecifier.substring(1)}`,
+                );


⬆️ 🚨 ⬆️

I'm expecting an explosion of edge cases here, starting with there being a bunch of record types and only one (or two?) have the structure of record.record so it's a little specific to what you've been testing with.
Unless no other record shape needs to be affected by this.

I don't like the concept of stuffing imports on the records. It's obviously working if added early enough, but there might be a more gentle way.

I'd rather include the information necessary for this to work in the compartmentMap itself. What's preventing that?

const compartmentMap = await mapNodeModules(readPowers, entryModuleLocation, { additionalModuleLocations, // ~~additionalPackageDetails~~ });

If we managed to do that, the step to manually add imports here could end up being unnecessary.

A single map of referrer-target should be enough to feed graphPackages with information to add missing links to the list it gets from packageDescriptor
Calling load on each of the targets in the same place where we call load on attenuators would suffice to keep them retained.

I'm not 100% sure the links are even necessary. If we had multiple entries, made graphPackages accept multiple (or call it many times if that works) and then proceed to load each of them, it should be enough to reach them through importNow flow.

Here's a more precise wording of what I mean in a language that works better than English:
#2864

boneskull · 2025-06-12T21:48:40Z

packages/compartment-mapper/src/import-hook.js

+                  `${compartmentDescriptors[additionalPackageLocation].name}${additionalPackageModuleSpecifier.substring(1)}`,
+                );
+              } else {
+                throw new Error(


the only way for this to happen would be to provide a broken CompartmentMapDescriptor in the first place. IDK if it's worth worrying about.

boneskull · 2025-06-12T21:50:20Z

packages/compartment-mapper/src/node-modules.js

+      packageLocation,
+      packageDescriptor: { name },
+    } of moduleDetails) {
+      node.dependencyLocations[name] ??= packageLocation;


this seems to be sufficient for associating a Node with another Node, but maybe there's a safer way to do so?

boneskull · 2025-06-12T21:51:07Z

packages/compartment-mapper/src/node-modules.js

+      moduleDescriptor.retained = true;
+      compartmentMap.compartments[packageLocation].retained = true;


I'm not sure these are actually needed.

@boneskull: check assumptions

manually setting retained should not be necessary and if it is, it suggests a flaw in the design.
Causing them to be retained should make other things easier too.

boneskull · 2025-06-12T21:51:54Z

packages/compartment-mapper/src/node-modules.js

@@ -1075,12 +1142,54 @@ export const mapNodeModules = async (
    parseLocatedJson(packageDescriptorText, packageDescriptorLocation)
  );

-  return compartmentMapForNodeModules(
+  await Promise.all(


afaict there is no memoization happening here that would make it so I shouldn't just spray promises everywhere

boneskull · 2025-06-12T21:53:12Z

packages/compartment-mapper/test/capture-lite.test.js

+  const entryModuleLocation = new URL(
+    'fixtures-additional-modules/node_modules/goofy/index.js',
+    import.meta.url,
+  ).href;
+
+  const additionalModuleLocations = {
+    [entryModuleLocation]: [
+      new URL('fixtures-additional-modules/config.js', import.meta.url).href,
+    ],
+  };


the fixture here mimics something like webpack. we have a tool (goofy) and that tool dynamically loads a config file (config.js) from our application root.

naugtur

Review in progress, but I think there might be a less (opinion) invasive way to serve the same need.

That being said, I want to encourage others to review because this exists and my hypothetical other way doesn't (at least until I try)

naugtur · 2025-06-17T12:52:01Z

packages/compartment-mapper/src/import-hook.js

+                // XXX: this "works" but that's probably a conincidence!
+                record.record?.imports?.push(
+                  `${compartmentDescriptors[additionalPackageLocation].name}${additionalPackageModuleSpecifier.substring(1)}`,
+                );


I'm expecting an explosion of edge cases here, starting with there being a bunch of record types and only one (or two?) have the structure of record.record so it's a little specific to what you've been testing with.
Unless no other record shape needs to be affected by this.

I don't like the concept of stuffing imports on the records. It's obviously working if added early enough, but there might be a more gentle way.

I'd rather include the information necessary for this to work in the compartmentMap itself. What's preventing that?

const compartmentMap = await mapNodeModules(readPowers, entryModuleLocation, { additionalModuleLocations, // ~~additionalPackageDetails~~ });

If we managed to do that, the step to manually add imports here could end up being unnecessary.

A single map of referrer-target should be enough to feed graphPackages with information to add missing links to the list it gets from packageDescriptor
Calling load on each of the targets in the same place where we call load on attenuators would suffice to keep them retained.

I'm not 100% sure the links are even necessary. If we had multiple entries, made graphPackages accept multiple (or call it many times if that works) and then proceed to load each of them, it should be enough to reach them through importNow flow.

naugtur · 2025-06-17T13:30:53Z

packages/compartment-mapper/src/node-modules.js

+      moduleDescriptor.retained = true;
+      compartmentMap.compartments[packageLocation].retained = true;


manually setting retained should not be necessary and if it is, it suggests a flaw in the design.
Causing them to be retained should make other things easier too.

boneskull self-assigned this Jun 12, 2025

boneskull added the enhancement New feature or request label Jun 12, 2025

boneskull requested review from kriskowal and naugtur June 12, 2025 21:36

boneskull commented Jun 12, 2025

View reviewed changes

boneskull mentioned this pull request Jun 13, 2025

feat(node): hints LavaMoat/LavaMoat#1683

Draft

naugtur reviewed Jun 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(compartment-mapper): support mapping of undiscoverable packages #2856

feat(compartment-mapper): support mapping of undiscoverable packages #2856

Uh oh!

boneskull commented Jun 12, 2025 •

edited

Loading

Uh oh!

boneskull Jun 12, 2025

Uh oh!

naugtur Jun 17, 2025

Uh oh!

naugtur Jun 18, 2025

Uh oh!

boneskull Jun 12, 2025

Uh oh!

boneskull Jun 12, 2025

Uh oh!

boneskull Jun 12, 2025

Uh oh!

naugtur Jun 17, 2025

Uh oh!

boneskull Jun 12, 2025

Uh oh!

boneskull Jun 12, 2025

Uh oh!

naugtur left a comment

Uh oh!

naugtur Jun 17, 2025

Uh oh!

naugtur Jun 17, 2025

Uh oh!

Uh oh!

		moduleDescriptor.retained = true;
		compartmentMap.compartments[packageLocation].retained = true;

feat(compartment-mapper): support mapping of undiscoverable packages #2856

Are you sure you want to change the base?

feat(compartment-mapper): support mapping of undiscoverable packages #2856

Uh oh!

Conversation

boneskull commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Questions for Reviewers

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naugtur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

boneskull commented Jun 12, 2025 •

edited

Loading