Skip to content

compartment-mapper: dynamic require of "node:"-prefixed builtins fails #2754

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

Closed
boneskull opened this issue Apr 2, 2025 · 1 comment · Fixed by #2755
Closed

compartment-mapper: dynamic require of "node:"-prefixed builtins fails #2754

boneskull opened this issue Apr 2, 2025 · 1 comment · Fixed by #2755
Assignees
Labels
bug Something isn't working

Comments

@boneskull
Copy link
Member

Describe the bug

While I was careful to add a test case for "dynamic require of a builtin module", that test case only covers a bare module specifier (cluster).

Changing cluster to node:cluster fails the test:

  Rejected promise returned by test. Reason:

  TypeError {
    message: 'Failed to load module "node:cluster" in package "file:///path/to/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/" (1 underlying failures: The URL must be of scheme file',
  }

  TypeError: Failed to load module "node:cluster" in package "file:///path/to/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/" (1 underlying failures: The URL must be of scheme file
      at throwAggregateError (file:///path/to/ses/src/module-load.js:557:11)
      at loadNow (file:///path/to/ses/src/module-load.js:654:3)
      at Compartment.importNow (file:///path/to/ses/src/compartment.js:187:5)
      at require (file:///path/to/compartment-mapper/src/parse-cjs-shared-export-wrapper.js:149:33)
      at Proxy.eval (eval at <anonymous> (eval at makeEvaluate (file:///path/to/ses/src/make-evaluate.js:92:27)), <anonymous>:5:1)
      at Object.execute (file:///path/to/compartment-mapper/src/parse-cjs.js:56:13)
      at execute (file:///path/to/ses/src/module-instance.js:101:24)
      at compartmentImportNow (file:///path/to/ses/src/compartment.js:103:3)
      at file:///path/to/ses/src/compartment.js:160:27
      at async file:///path/to/compartment-mapper/test/dynamic-require.test.js:303:25

This is reproducible outside of the @endo/compartment-mapper's test environment (I found it via @lavamoat/node's testing environment; lol).

Steps to reproduce

  1. Modify compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js:

    diff --git a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js
    index f64a83d77acdc2d1b5b5b9ffe254108d2c0fa567..972b76820199e55ef0c0624417708c4a432a9cb4 100644
    --- a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js
    +++ b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js
    @@ -1,5 +1,5 @@
    exports.isOk = require('hooked').isOk;
    
    -const builtin = 'cluster';
    +const builtin = 'node:cluster';
    
    require(builtin);
    \ No newline at end of file
  2. Execute test (from packages/compartment-mapper):

    npx ava test/dynamic-require.test.js -c 1 -m 'inter-package and exit module dynamic require works'
  3. Observe failure akin to the trace pasted above.

Expected behavior

It should be the same behavior as exhibited sans-prefix.

Additional context

This is likely my fault, so I will fix it.

@boneskull boneskull added the bug Something isn't working label Apr 2, 2025
@boneskull boneskull self-assigned this Apr 2, 2025
@boneskull
Copy link
Member Author

Also: I would love a proper stack trace, but I'm not sure how to get any better; we're already using errorTaming: 'unsafe'

boneskull added a commit that referenced this issue Apr 3, 2025
closes #2754

Dynamically-required builtins were going through the entire candidate-specifier-guess-'n'-check loop, which shouldn't happen.

Specifically, the call to a `MaybeReadNowFn` in this loop caused the original error as reported in #2754 because it uses a `FileUrlToPathFn`; the default implementation (Node.js' `url.fileURLToPath`) throws when provided `node:foo` _but does not throw_ when provided `foo`.

Also exports `WILDCARD_POLICY_VALUE` from `policy-format` so we can use it instead of magic strings all over the place.

Fixes #2754
boneskull added a commit that referenced this issue Apr 3, 2025
closes #2754

Dynamically-required builtins were going through the entire candidate-specifier-guess-'n'-check loop, which shouldn't happen.

Specifically, the call to a `MaybeReadNowFn` in this loop caused the original error as reported in #2754 because it uses a `FileUrlToPathFn`; the default implementation (Node.js' `url.fileURLToPath`) throws when provided `node:foo` _but does not throw_ when provided `foo`.

Also exports `WILDCARD_POLICY_VALUE` from `policy-format` so we can use it instead of magic strings all over the place.

Fixes #2754
boneskull added a commit that referenced this issue Apr 5, 2025
closes #2754

Dynamically-required builtins were going through the entire candidate-specifier-guess-'n'-check loop, which shouldn't happen.

Specifically, the call to a `MaybeReadNowFn` in this loop caused the original error as reported in #2754 because it uses a `FileUrlToPathFn`; the default implementation (Node.js' `url.fileURLToPath`) throws when provided `node:foo` _but does not throw_ when provided `foo`.

Also exports `WILDCARD_POLICY_VALUE` from `policy-format` so we can use it instead of magic strings all over the place.

Fixes #2754
boneskull added a commit that referenced this issue Apr 14, 2025
closes #2754

Dynamically-required builtins were going through the entire candidate-specifier-guess-'n'-check loop, which shouldn't happen.

Specifically, the call to a `MaybeReadNowFn` in this loop caused the original error as reported in #2754 because it uses a `FileUrlToPathFn`; the default implementation (Node.js' `url.fileURLToPath`) throws when provided `node:foo` _but does not throw_ when provided `foo`.

Also exports `WILDCARD_POLICY_VALUE` from `policy-format` so we can use it instead of magic strings all over the place.

Fixes #2754
boneskull added a commit that referenced this issue Apr 15, 2025
closes #2754

Dynamically-required builtins were going through the entire candidate-specifier-guess-'n'-check loop, which shouldn't happen.

Specifically, the call to a `MaybeReadNowFn` in this loop caused the original error as reported in #2754 because it uses a `FileUrlToPathFn`; the default implementation (Node.js' `url.fileURLToPath`) throws when provided `node:foo` _but does not throw_ when provided `foo`.

Also exports `WILDCARD_POLICY_VALUE` from `policy-format` so we can use it instead of magic strings all over the place.

Fixes #2754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant