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,