From d19afd2f9a194f554c7218f7190b0070d3e9278a Mon Sep 17 00:00:00 2001 From: naugtur Date: Fri, 24 Nov 2023 16:03:56 +0100 Subject: [PATCH] fix(compartment-mapper): correct error interpretations, negative policy enforcement test --- .../compartment-mapper/src/import-archive.js | 4 +- .../compartment-mapper/src/import-hook.js | 4 +- packages/compartment-mapper/src/link.js | 13 ++-- packages/compartment-mapper/src/policy.js | 26 ++++---- .../test/snapshots/test-policy.js.md | 60 ++++++++++++++---- .../test/snapshots/test-policy.js.snap | Bin 624 -> 788 bytes .../compartment-mapper/test/test-policy.js | 40 +++++++++--- 7 files changed, 108 insertions(+), 39 deletions(-) diff --git a/packages/compartment-mapper/src/import-archive.js b/packages/compartment-mapper/src/import-archive.js index cc51a9b34b..c3248cfdad 100644 --- a/packages/compartment-mapper/src/import-archive.js +++ b/packages/compartment-mapper/src/import-archive.js @@ -95,7 +95,9 @@ const makeArchiveImportHookMaker = ( // module is a "builtin" module and the policy needs to be enforced. enforceModulePolicy(moduleSpecifier, compartmentDescriptor, { exit: true, - errorHint: 'The module was not in the archive and an attempt was made to load it as a builtin', + errorHint: `Blocked in loading. ${q( + moduleSpecifier, + )} was not in the archive and an attempt was made to load it as a builtin`, }); const record = await exitModuleImportHook(moduleSpecifier); if (record) { diff --git a/packages/compartment-mapper/src/import-hook.js b/packages/compartment-mapper/src/import-hook.js index 6bba911e77..af58c33b70 100644 --- a/packages/compartment-mapper/src/import-hook.js +++ b/packages/compartment-mapper/src/import-hook.js @@ -220,7 +220,9 @@ export const makeImportHookMaker = ( // hook returns something. Otherwise, we need to fall back to the 'cannot find' error below. enforceModulePolicy(moduleSpecifier, compartmentDescriptor, { exit: true, - errorHint: 'The module was not in the compartment map and an attempt was made to load it as a builtin', + 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. diff --git a/packages/compartment-mapper/src/link.js b/packages/compartment-mapper/src/link.js index 8decfda31b..302e439379 100644 --- a/packages/compartment-mapper/src/link.js +++ b/packages/compartment-mapper/src/link.js @@ -231,11 +231,14 @@ const makeModuleMapHook = ( return undefined; // fall through to import hook } if (foreignModuleSpecifier !== undefined) { + // archive goes through foreignModuleSpecifier for local modules too if (!moduleSpecifier.startsWith('./')) { - // archive goes through foreignModuleSpecifier for local modules too + // This code path seems to only be reached on subsequent imports of the same specifier in the same compartment. + // The check should be redundant and is only left here out of abundance of caution. enforceModulePolicy(moduleSpecifier, compartmentDescriptor, { exit: false, - errorHint: `TODO`, // TODO: add a testcase for this and provide a helpful hint + errorHint: + 'This check should not be reachable. If you see this error, please file an issue.', }); } @@ -280,11 +283,9 @@ const makeModuleMapHook = ( enforceModulePolicy(scopePrefix, compartmentDescriptor, { exit: false, - errorHint: ` There is an alias defined that causes another package to be imported as ${q( + errorHint: `Blocked in linking. ${q( moduleSpecifier, - )}. Package ${q(moduleSpecifier)} resolves to ${q( - foreignModuleSpecifier, - )} in ${q( + )} is part of the compartment map and resolves to ${q( foreignCompartmentName, )}.`, }); diff --git a/packages/compartment-mapper/src/policy.js b/packages/compartment-mapper/src/policy.js index 5392c079f9..e0eb1488ae 100644 --- a/packages/compartment-mapper/src/policy.js +++ b/packages/compartment-mapper/src/policy.js @@ -328,31 +328,35 @@ export const attenuateGlobals = ( freezeGlobalThisUnlessOptedOut(); }; - -const diagnoseModulePolicy = (errorHint) => { - if(!errorHint) { +const diagnoseModulePolicy = errorHint => { + if (!errorHint) { return ''; } - return ` (${errorHint})`; -} - + return ` (info: ${errorHint})`; +}; /** * Throws if importing of the specifier is not allowed by the policy * * @param {string} specifier - * @param {object} compartmentDescriptor + * @param {import('./types.js').CompartmentDescriptor} compartmentDescriptor * @param {object} [info] */ -export const enforceModulePolicy = (specifier, compartmentDescriptor, info = {}) => { - const { policy, modules } = compartmentDescriptor; +export const enforceModulePolicy = ( + specifier, + compartmentDescriptor, + info = {}, +) => { + const { policy, modules, label } = compartmentDescriptor; if (!policy) { return; } - + if (!info.exit) { if (!modules[specifier]) { throw Error( - `Importing ${q(specifier)} was not allowed by policy packages:${q( + `Importing ${q(specifier)} in ${q( + label, + )} was not allowed by packages policy ${q( policy.packages, )}${diagnoseModulePolicy(info.errorHint)}`, ); diff --git a/packages/compartment-mapper/test/snapshots/test-policy.js.md b/packages/compartment-mapper/test/snapshots/test-policy.js.md index ffbb95e400..a9c594bb84 100644 --- a/packages/compartment-mapper/test/snapshots/test-policy.js.md +++ b/packages/compartment-mapper/test/snapshots/test-policy.js.md @@ -4,41 +4,77 @@ The actual snapshot is saved in `test-policy.js.snap`. Generated by [AVA](https://avajs.dev). -## policy - attack - browser alias / loadLocation +## policy - attack - browser alias - with alias hint / loadLocation > Snapshot 1 - 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' + 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' -## policy - attack - browser alias / importLocation +## policy - attack - browser alias - with alias hint / importLocation > Snapshot 1 - 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' + 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' -## policy - attack - browser alias / makeArchive / parseArchive +## policy - attack - browser alias - with alias hint / makeArchive / parseArchive > Snapshot 1 - 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' + 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' -## policy - attack - browser alias / makeArchive / parseArchive with a prefix +## policy - attack - browser alias - with alias hint / makeArchive / parseArchive with a prefix > Snapshot 1 - 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' + 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' -## policy - attack - browser alias / writeArchive / loadArchive +## policy - attack - browser alias - with alias hint / writeArchive / loadArchive > Snapshot 1 - 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' + 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' -## policy - attack - browser alias / writeArchive / importArchive +## policy - attack - browser alias - with alias hint / writeArchive / importArchive > Snapshot 1 - 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' + 'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)' + +## policy - disallowed package with error hint / loadLocation + +> Snapshot 1 + + 'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)' + +## policy - disallowed package with error hint / importLocation + +> Snapshot 1 + + 'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)' + +## policy - disallowed package with error hint / makeArchive / parseArchive + +> Snapshot 1 + + 'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)' + +## policy - disallowed package with error hint / makeArchive / parseArchive with a prefix + +> Snapshot 1 + + 'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)' + +## policy - disallowed package with error hint / writeArchive / loadArchive + +> Snapshot 1 + + 'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)' + +## policy - disallowed package with error hint / writeArchive / importArchive + +> Snapshot 1 + + 'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)' ## policy - attenuator error aggregation / loadLocation diff --git a/packages/compartment-mapper/test/snapshots/test-policy.js.snap b/packages/compartment-mapper/test/snapshots/test-policy.js.snap index ba57a1335e5a5dedf48b9dd5cd2c5c339ef9e6e2..2848cf1ebcbe596d2dc05c8b1ee51aa5bb4ec330 100644 GIT binary patch literal 788 zcmV+v1MB=jRzVNdv4+vZSQ+?QPT&D;shpbvwkXuef zBsI}y=O2p*00000000B+na_*UKorNf74htU@W??{*e1K89)gG#f31jk5(Gh}$)p)O znF%w~rd2$6^y+`GUi=UI`+P~7ZVQz(X)6S#r%W>Q=DqiMU((6Dibh-p6aDiSHOz22 zf09bhf&~nK7(;>yUW}B?G*v)2BO1pUGx2f|Ghx63E=l-C2E;Ham=+|}u{8SoRTvUO zKJ?{Zty(9?tU|IK64pcxZ)n|&CwGznL zS=D!k`MU!!FJJJMdUvS%E9xQBRbW-V3*J;IsdoFH-v6_~_jmuFd>Q6{ek{EAYVig{ zNnQfRat<0IE|xqlZTJ!4x`uVN+kd8AxVd?&@Fd57X0x%DjY)GG z*d~Wp&E(MJ)Mus9m13~c*d~qlhmuC!;n)hLE2UwDa)5V8lWz!C8eJ&{D~)Z^c)gc2 zYCg4Cadf2|tT?ubahX^5o_@f{WJ@mo}P3*C~ zWW8(cuIogIL+^b9PUT5Ewv)7+oYd;Ul5>dc?98{{%x_nj>tv}+v~pMf$V;zi9TY~% zXah4q?=f2O!_u0{kp)x|9fY6^#@|hZUK%Z(#=>RByUXi1Mvp(He;-etPU_F}$>iCi ztLYb%iem7lNrc?Qr6LeP*lrN~ass4=f}b(vg@nWBVHgBK7@534>vPh6mSa(n4Lvy@ zCUWgdOK#Sh4Yi5sXDjSNjtd2xyn<54WYtFM6cYAOC(L1y7smSfHxZ*2P;p7xcrH$v zift}8uQW8SfzdCKSPo9p#tZlv}GK=RtFR*UdS{6@9Q# zCNGHJcvrW39hnc3`L{mIp^`oWgu-&&>+u8(@&p`k>nkh$og+}Q8*lxPnZG}pdFyi~wUjo)TldWV=0DcYr(cy>qH=rHTBMQX&VO#?tBk9;U1tvI zHYed#!3~W_wErd|-`u(ra}bMt9-YF1+sy|x5T}PJM`(?D5M}0YobHao+e0aI&eS7^ zJ{&$C%HgP)cR);z?%cz%-Z44r&!lrE9SQXzbnvY?YGxdn^kMSpo=m!Cx{=C#sN4Wp K2qMWP6#xJTkuxp; diff --git a/packages/compartment-mapper/test/test-policy.js b/packages/compartment-mapper/test/test-policy.js index e9b6515fa1..0203b620c0 100644 --- a/packages/compartment-mapper/test/test-policy.js +++ b/packages/compartment-mapper/test/test-policy.js @@ -113,11 +113,14 @@ const anyExpectations = { const powerlessCarolExpectations = { namespace: { ...defaultExpectations.namespace, - carol: { bluePill: 'undefined', redPill: 'undefined', purplePill: 'undefined' }, + carol: { + bluePill: 'undefined', + redPill: 'undefined', + purplePill: 'undefined', + }, }, }; - const makeResultAssertions = expectations => async (t, { namespace }) => { @@ -167,7 +170,7 @@ scaffold( ); scaffold( - 'policy - attack - browser alias', + 'policy - attack - browser alias - with alias hint', test, fixtureAttack, assertTestAlwaysThrows, @@ -175,7 +178,8 @@ scaffold( { shouldFailBeforeArchiveOperations: true, onError: (t, { error }) => { - t.regex(error.message, /dan.*alias.*hackity/); + t.regex(error.message, /dan.*resolves.*hackity/); + // see the snapshot for the error hint in the message t.snapshot(sanitizePaths(error.message)); }, addGlobals: globals, @@ -236,14 +240,18 @@ const recursiveEdit = editor => originalPolicy => { }; const mutationEdit = editor => originalPolicy => { const policyToAlter = JSON.parse(JSON.stringify(originalPolicy)); - editor(policyToAlter) + editor(policyToAlter); return policyToAlter; -} +}; -const skipCarol = mutationEdit((policyToAlter) => { +const skipCarol = mutationEdit(policyToAlter => { policyToAlter.resources['alice>carol'] = undefined; }); +const disallowCarol = mutationEdit(policyToAlter => { + policyToAlter.resources.alice.packages['alice>carol'] = false; +}); + const addAttenuatorForAllGlobals = recursiveEdit((key, obj) => { if (key === 'globals') { obj[key] = { @@ -280,7 +288,6 @@ const nestedAttenuator = recursiveEdit((key, obj) => { } }); - scaffold( 'policy - allow skipping policy entries for powerless compartments', test, @@ -293,6 +300,23 @@ scaffold( }, ); +scaffold( + 'policy - disallowed package with error hint', + test, + fixture, + assertTestAlwaysThrows, + 2, // expected number of assertions + { + shouldFailBeforeArchiveOperations: true, + onError: (t, { error }) => { + t.regex(error.message, /Importing.*carol.*in.*alice.*not allowed/i); + t.snapshot(sanitizePaths(error.message)); + }, + addGlobals: globals, + policy: disallowCarol(policy), + }, +); + scaffold( 'policy - globals attenuator', test,