Fix the mismatch between specifier and resource identifier in attenuators policy AND allow skiping powerless packages #1838
Conversation
naugtur
force-pushed
the
naugtur-fix-attenuator-nested-export
branch
from
8ad2406 to
2cd6a72
Compare
| if (module.compartment !== undefined) { | ||
| if ( | ||
| module.compartment !== undefined && | ||
| compartmentRenames[module.compartment] !== undefined |
There was a problem hiding this comment.
We need to evaluate more rigorously throughout compartment-mapper how to handle cases like module.compartment being toString or __proto__. The undefined check is fine for a constant name, but we should use an own property check here.
The fix for attenuators is to allow importing all into the attenuators compartment instead of trying to resolve a specifier into a policy resource identifier, which is a lot of work and implemented far away :) The fix in archive is pretty much independent, but the switch to I'm looking forward to discussing my assumptions and decisions. Don't mind keeping this open till Wed, but I'm not sure if I'm gonna be able to attend while at NodeConfEU. We'll see. |
|
@naugtur Good enough for me! |
naugtur
changed the title
| packages[specifier] = true; | ||
| return packages; | ||
| }, {}), | ||
| packages: 'any', |
There was a problem hiding this comment.
this removed section had the bug where we mistook specifier for a resource id. and it was all unnecessary.
There was a problem hiding this comment.
I trust that https://www.npmjs.com/package/any is not an issue with this property’s range of expressible values?
There was a problem hiding this comment.
packages is normally a key/value map. there's only one string value allowed and that's the "any". There's no room for a mistake here
naugtur
force-pushed
the
naugtur-fix-attenuator-nested-export
branch
from
5c1bc06 to
bd08203
Compare
… in policy attenuators
…attenuators compartment
…cy enforcement test
naugtur
force-pushed
the
naugtur-fix-attenuator-nested-export
branch
from
7fbb651 to
db2545b
Compare
| packages[specifier] = true; | ||
| return packages; | ||
| }, {}), | ||
| packages: 'any', |
There was a problem hiding this comment.
I trust that https://www.npmjs.com/package/any is not an issue with this property’s range of expressible values?
| /** | ||
| * 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] |
There was a problem hiding this comment.
Can this info type be made more specific?
| 'policy - nested export in attenuator', | ||
| test, | ||
| fixture, | ||
| combineAssertions( |
There was a problem hiding this comment.
I missed when we got an assertions combinator. Neat.
There was a problem hiding this comment.
Counting the expected number of assertions remains annoying tho.
Update:
The problem identified when fixing attenuators policy turns out to not exist when we allow skipping setting empty values for policy resources for packages we want to endow no powers to.
I decided to solve that right away.
The current state of the PR is a work in progress, but it's only missing some test coverage for error hints and some hint improvements.
The issue is a mismatch between a specifier and a resource id AKA cannonical name in:
https://github.com/endojs/endo/blob/master/packages/compartment-mapper/src/policy.js#L158
the right policy value for
@a/b/custom-exportis@a/band I need to add a proper conversion there.