fix(compartment-mapper): specifier of module descriptor returned from findRedirect fixed #2765

boneskull · 2025-04-10T23:40:39Z

Description

findRedirect, which is part of importNowHook, is called when we encounter a dynamic require where the specifier is an absolute path.

Previous to this change, the RedirectStaticModuleInterface (a ModuleDescriptor) could contain an absolute path. Absolute paths are not generally assumed by ses or @endo/compartment-mapper. In the situation where the module referenced in the RedirectStaticModuleInterface itself imports another package (via mutual recursion & trampolining), these assumptions were violated and we made an attempt to call resolve() (from the node-module-specifier module) using its absolute path as the referrer, which is explicitly disallowed by resolve(); it throws an exception.

To that end, we need to ensure that whatever ModuleDescriptor we return from importNowHook with is one where its specifier is relative to its package location.

The fact that this nominally "worked" against our tests in both @endo/compartment-mapper and @lavamoat/node was due to a) chance, and b) lack of a test case against a fixture representing this configuration.

In addition, I removed the packageLocation parameter from findRedirect's options bucket since it was serving no practical purpose (as noticed by @naugtur).

Security Considerations

None that I am aware of

Scaling Considerations

no

Documentation Considerations

This is a bugfix

Testing Considerations

I need to add two more tests against fixtures before I publish this:

A fixture which is similar to the configuration mentioned above
A fixture where the dynamically-required file is in a different compartment (which should be subject to policy enforcement)

Compatibility Considerations

None that I am aware of

Upgrade Considerations

Not breaking. Probably not worth a note in NEWS.md

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

naugtur · 2025-04-11T08:18:01Z

packages/compartment-mapper/test/dynamic-require.test.js

@@ -418,3 +418,45 @@ test('sync module transforms work without dynamic require support', async t => {

  t.is(transformCount, 2);
 });
+
+test('dynamic require of missing module falls through to importNowHook', async t => {


could you point out which test is covering the situation where findRedirect finds the redirect?

any test which contains a fixture which is absolute-requiring something is. ... which is most of them.

specifically, this test covers the same sort of configuration as we encountered:

hooked-app statically requires package hooked

hooked dynamically requires package dynamic by absolute path

dynamic dynamically requires package is-ok by absolute path

is-ok statically requires package is-not-ok

naugtur · 2025-04-11T08:20:18Z

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

@@ -692,7 +701,6 @@ export function makeImportNowHookMaker(
          compartmentDescriptors,
          compartments,
          absoluteModuleSpecifier: moduleSpecifier,
-          packageLocation,


I still believe we should not continue with an absolute path if findRedirect returns undefined. Any new findings to counter that since we talked?

It looks like findRedirect fails to find absolute paths to compartment-relative modules which were not already mentioned. For example, if we try to require(require.resolve('./good.js'))—and there are no existing ModuleDescriptors for a ./good.js in the compartment descriptor—then findRedirect will return undefined. We need to let it bounce on the trampoline to find the file. If the file still can't be found, then we fall through to the hook-of-last-resort (exit hook).

That said, maybe it would be prudent to rewrite the path as relative to the compartment's location before we continue (though it "works" as-is).

That said, maybe it would be prudent to rewrite the path as relative to the compartment's location before we continue (though it "works" as-is).

I did this and it was wrong as pointed out by @naugtur. Reverted it

boneskull · 2025-04-15T00:49:55Z

OK, this is ready

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

boneskull · 2025-04-15T21:33:02Z

I've had to change the importNowHook logic somewhat considerably due to #2755. There are now two paths to a potential call to an ExitModuleImportNowHook:

If the module specifier was absolute and not found via guesswork (chooseModuleDescriptor())
If the module specifier is likely a builtin (or something else weird)

This preserves support for node:-prefixed builtins while still enabling a fallback to the ExitModuleImportNowHook if a module specifier w/ an absolute path could not be found otherwise.

This test covers the situation where we have a dynamic require of a module requested by absolute path which is found successfully via chooseModuleDescriptor (good.js), and another which cannot be found this way (missing.js) because it is missing; this invokes the ExitModuleImportNowHook.

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

naugtur · 2025-04-17T11:53:59Z

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

      }

+      // we might have an absolute path here, but it might be within the compartment, so
+      // we will try to find it.
      const candidates = nominateCandidates(moduleSpecifier, searchSuffixes);

      const record = syncTrampoline(


AFAIR going into chooseModuleDescriptor with an absolute path can result in an error that got us to find this issue in the first place. Some of the nested functionality is not ready to handle absolute paths. If the path is within the current compartment, we should be able to convert it to relative.

so your recommendation would be to convert it to relative if the path is in the current compartment?

@naugtur

… findRedirect fixed `findRedirect`, which is part of `importNowHook`, is called when we encounter a dynamic require where the specifier is an absolute path. Previous to this change, the `RedirectStaticModuleInterface` (a `ModuleDescriptor`) could contain an absolute path. Absolute paths are not generally assumed by `ses` or `@endo/compartment-mapper`. In the situation where the module referenced in the `RedirectStaticModuleInterface` itself imports _another_ package (via mutual recursion & trampolining), these assumptions were violated and we made an attempt to call `resolve()` (from the `node-module-specifier` module) using its absolute path as the referrer, which is explicitly disallowed by `resolve()`; it throws an exception. To that end, we need to ensure that whatever `ModuleDescriptor` we return from `importNowHook` with is one where its `specifier` is relative to its package location. The fact that this nominally "worked" against our tests in both `@endo/compartment-mapper` _and_ `@lavamoat/node` was due to a) chance, and b) lack of a test case against a fixture representing this configuration. In addition, I removed the `packageLocation` parameter from `findRedirect`'s options bucket since it was serving no practical purpose (as noticed by @naugtur). I need to add two more tests against fixtures: 1. A fixture which is similar to the configuration mentioned above 2. A fixture where the dynamically-required file is in a different compartment (which should be subject to policy enforcement)

This ensures the case that we found where a package which was required dynamically via an absolute path attempts to require yet another package

Co-authored-by: Zbyszek Tenerowicz <[email protected]>

boneskull self-assigned this Apr 10, 2025

naugtur reviewed Apr 11, 2025

View reviewed changes

packages/compartment-mapper/src/import-hook.js Outdated Show resolved Hide resolved

naugtur reviewed Apr 11, 2025

View reviewed changes

boneskull force-pushed the boneskull/findredirect-fix branch 2 times, most recently from ca8d0ef to c051e7c Compare April 15, 2025 00:49

boneskull marked this pull request as ready for review April 15, 2025 00:49

boneskull added the bug Something isn't working label Apr 15, 2025

naugtur reviewed Apr 15, 2025

View reviewed changes

packages/compartment-mapper/src/import-hook.js Outdated Show resolved Hide resolved

boneskull force-pushed the boneskull/findredirect-fix branch from c051e7c to 41c4494 Compare April 15, 2025 20:02

boneskull requested a review from kriskowal April 15, 2025 20:03

boneskull force-pushed the boneskull/findredirect-fix branch 2 times, most recently from 8c74957 to 0744b25 Compare April 15, 2025 20:52

boneskull force-pushed the boneskull/findredirect-fix branch from d51b21c to 9c86393 Compare April 15, 2025 22:48

naugtur reviewed Apr 17, 2025

View reviewed changes

packages/compartment-mapper/src/import-hook.js Outdated Show resolved Hide resolved

naugtur reviewed Apr 17, 2025

View reviewed changes

packages/compartment-mapper/src/import-hook.js Outdated Show resolved Hide resolved

naugtur reviewed Apr 17, 2025

View reviewed changes

packages/compartment-mapper/src/import-hook.js Outdated Show resolved Hide resolved

naugtur reviewed Apr 17, 2025

View reviewed changes

boneskull force-pushed the boneskull/findredirect-fix branch 2 times, most recently from f08e0d9 to 11c99a9 Compare April 25, 2025 20:07

boneskull and others added 6 commits April 25, 2025 15:08

chore(compartment-mapper): expand dynamic require tests

3e44bcf

This ensures the case that we found where a package which was required dynamically via an absolute path attempts to require yet another package

chore(compartment-mapper): remove unused type imports

9869d96

chore(compartment-mapper): post-rebase fixes

42a11dd

chore(compartment-mapper): better relativeSpecifier implementation

142b693

Co-authored-by: Zbyszek Tenerowicz <[email protected]>

chore(compartment-mapper): peer review updates

c1b8bb5

boneskull force-pushed the boneskull/findredirect-fix branch from 11c99a9 to c1b8bb5 Compare April 25, 2025 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compartment-mapper): specifier of module descriptor returned from findRedirect fixed #2765

fix(compartment-mapper): specifier of module descriptor returned from findRedirect fixed #2765

boneskull commented Apr 10, 2025 •

edited

Loading

naugtur Apr 11, 2025

boneskull Apr 11, 2025

boneskull Apr 15, 2025

naugtur Apr 11, 2025

boneskull Apr 12, 2025

boneskull Apr 15, 2025

boneskull commented Apr 15, 2025

boneskull commented Apr 15, 2025

naugtur Apr 17, 2025

boneskull Apr 24, 2025

fix(compartment-mapper): specifier of module descriptor returned from findRedirect fixed #2765

Are you sure you want to change the base?

fix(compartment-mapper): specifier of module descriptor returned from findRedirect fixed #2765

Conversation

boneskull commented Apr 10, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Apr 15, 2025

boneskull commented Apr 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Apr 10, 2025 •

edited

Loading