From 28999e8a2da209c28b217f096dc2fb8080898e9a Mon Sep 17 00:00:00 2001 From: Zbyszek Tenerowicz Date: Tue, 5 Nov 2024 01:31:30 +0100 Subject: [PATCH] =?UTF-8?q?fix(compartment-mapper):=20Defer=20all=20import?= =?UTF-8?q?Hook=20errors=20when=20importing=20a=E2=80=A6=20(#2610)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit … dependency if they're intended to be deferred ## Description When importHook is used to load dependencies, they might be coming from `imports` that were identified by a parser that uses heuristics to find them. (e.g. cjs parser implementation uses a lexer to find all `require` calls even if they're not _the_ require function but a local require function) In such cases we want to defer loading errors to runtime, so the error doesn't happen until actual `require` is _actually_ called to get it. That was true for loading known dependency, but exit modules (via exitModuleImportHook) were not covered. ### Security Considerations no changes security-wise ### Scaling Considerations The way this is currently implemented - one big try-catch instead of returning the deferred error without throwing and catching, might be less performant. Should we consider trading aesthetics for a tiny performance gain here? Note this only makes a difference for cases where non-dynamic `require` is called on a specifier that causes an error. ### Documentation Considerations It's a fix of unintended behavior. No changes to what needs to be documented. ### Testing Considerations Fix was added test-first ### Compatibility Considerations Compatibility is improved. Packages can now do: ```js try { require('my-dev-dependency') } catch(){} ``` to silently attempt to load something that is only useful in their own development process. (typescript does that) ### Upgrade Considerations While it's a fix, not a breaking change, it does change one specific behavior - an error resulting from calling exitModuleImportHook (the `importHook` passed to compartment-mapper, not the one in Compartment options) is now thrown at runtime, not during loading, which could be noteworthy for the Archive use-case. [(insert mandatory xkcd about breaking changes)](https://xkcd.com/1172/) --------- Co-authored-by: Christopher Hiller --- packages/compartment-mapper/NEWS.md | 5 + .../compartment-mapper/src/import-hook.js | 133 +++++++++--------- .../test/cjs-compat.test.js | 18 +++ 3 files changed, 89 insertions(+), 67 deletions(-) diff --git a/packages/compartment-mapper/NEWS.md b/packages/compartment-mapper/NEWS.md index 7a2aab4488..968df14ef1 100644 --- a/packages/compartment-mapper/NEWS.md +++ b/packages/compartment-mapper/NEWS.md @@ -4,6 +4,11 @@ User-visible changes to `@endo/compartment-mapper`: - Omits unused module descriptors from `compartment-map.json` in archived applications, potentially reducing file sizes. +- Fixes an issue where errors thrown from exit module hooks (`importHook`) would + be thrown at parse-time when the parser uses heuristic import analysis + _instead of_ at runtime. Such errors will now be thrown at runtime, as + originally intended. To those who expected the previous behavior: if you + exist, please exercise caution when upgrading. # v1.3.0 (2024-10-10) diff --git a/packages/compartment-mapper/src/import-hook.js b/packages/compartment-mapper/src/import-hook.js index cbc13e647b..0d478b3384 100644 --- a/packages/compartment-mapper/src/import-hook.js +++ b/packages/compartment-mapper/src/import-hook.js @@ -504,89 +504,88 @@ export const makeImportHookMaker = ( // for lint rule await null; - // per-module: - - // In Node.js, an absolute specifier always indicates a built-in or - // third-party dependency. - // The `moduleMapHook` captures all third-party dependencies, unless - // we allow importing any exit. - if (moduleSpecifier !== '.' && !moduleSpecifier.startsWith('./')) { - if (exitModuleImportHook) { - const record = await exitModuleImportHook(moduleSpecifier); - if (record) { - // It'd be nice to check the policy before importing it, but we can only throw a policy error if the - // hook returns something. Otherwise, we need to fall back to the 'cannot find' error below. - enforceModulePolicy(moduleSpecifier, compartmentDescriptor, { - exit: true, - errorHint: `Blocked in loading. ${q( + // All importHook errors must be deferred if coming from loading dependencies + // identified by a parser that discovers imports heuristically. + try { + // per-module: + + // In Node.js, an absolute specifier always indicates a built-in or + // third-party dependency. + // The `moduleMapHook` captures all third-party dependencies, unless + // we allow importing any exit. + if (moduleSpecifier !== '.' && !moduleSpecifier.startsWith('./')) { + if (exitModuleImportHook) { + const record = await exitModuleImportHook(moduleSpecifier); + if (record) { + // It'd be nice to check the policy before importing it, but we can only throw a policy error if the + // hook returns something. Otherwise, we need to fall back to the 'cannot find' error below. + enforceModulePolicy(moduleSpecifier, compartmentDescriptor, { + exit: true, + errorHint: `Blocked in loading. ${q( + moduleSpecifier, + )} was not in the compartment map and an attempt was made to load it as a builtin`, + }); + if (archiveOnly) { + // Return a place-holder. + // Archived compartments are not executed. + return freeze({ imports: [], exports: [], execute() {} }); + } + // note it's not being marked as exit in sources + // it could get marked and the second pass, when the archive is being executed, would have the data + // to enforce which exits can be dynamically imported + const attenuatedRecord = await attenuateModuleHook( moduleSpecifier, - )} was not in the compartment map and an attempt was made to load it as a builtin`, - }); - if (archiveOnly) { - // Return a place-holder. - // Archived compartments are not executed. - return freeze({ imports: [], exports: [], execute() {} }); + record, + compartmentDescriptor.policy, + attenuators, + ); + return attenuatedRecord; } - // note it's not being marked as exit in sources - // it could get marked and the second pass, when the archive is being executed, would have the data - // to enforce which exits can be dynamically imported - const attenuatedRecord = await attenuateModuleHook( - moduleSpecifier, - record, - compartmentDescriptor.policy, - attenuators, - ); - return attenuatedRecord; } - } - return deferError( - moduleSpecifier, - Error( + throw Error( `Cannot find external module ${q( moduleSpecifier, )} in package ${packageLocation}`, - ), - ); - } + ); + } - const { maybeRead } = unpackReadPowers(readPowers); + const { maybeRead } = unpackReadPowers(readPowers); - const candidates = nominateCandidates(moduleSpecifier, searchSuffixes); + const candidates = nominateCandidates(moduleSpecifier, searchSuffixes); - const record = await asyncTrampoline( - chooseModuleDescriptor, - { - candidates, - compartmentDescriptor, - compartmentDescriptors, - compartments, - computeSha512, - moduleDescriptors, - moduleSpecifier, - packageLocation, - packageSources, - readPowers, - sourceMapHook, - strictlyRequiredForCompartment, - }, - { maybeRead, parse, shouldDeferError }, - ); + const record = await asyncTrampoline( + chooseModuleDescriptor, + { + candidates, + compartmentDescriptor, + compartmentDescriptors, + compartments, + computeSha512, + moduleDescriptors, + moduleSpecifier, + packageLocation, + packageSources, + readPowers, + sourceMapHook, + strictlyRequiredForCompartment, + }, + { maybeRead, parse, shouldDeferError }, + ); - if (record) { - return record; - } + if (record) { + return record; + } - return deferError( - moduleSpecifier, - // TODO offer breadcrumbs in the error message, or how to construct breadcrumbs with another tool. - Error( + throw Error( `Cannot find file for internal module ${q( moduleSpecifier, )} (with candidates ${candidates .map(x => q(x)) .join(', ')}) in package ${packageLocation}`, - ), - ); + ); + } catch (error) { + return deferError(moduleSpecifier, error); + } }; return importHook; }; diff --git a/packages/compartment-mapper/test/cjs-compat.test.js b/packages/compartment-mapper/test/cjs-compat.test.js index c6f1af585d..891f37c818 100644 --- a/packages/compartment-mapper/test/cjs-compat.test.js +++ b/packages/compartment-mapper/test/cjs-compat.test.js @@ -15,6 +15,8 @@ const fixtureDirname = new URL( import.meta.url, ).toString(); +const q = JSON.stringify; + const assertFixture = (t, { namespace, testCategoryHint }) => { const { assertions, results } = namespace; @@ -66,6 +68,22 @@ scaffold( fixtureAssertionCount, ); +// Exit module errors are also deferred +scaffold( + 'fixtures-cjs-compat-exit-module', + test, + fixture, + assertFixture, + fixtureAssertionCount, + { + additionalOptions: { + importHook: async specifier => { + throw Error(`${q(specifier)} is NOT an exit module.`); + }, + }, + }, +); + scaffold( 'fixtures-cjs-compat-__dirname', test,