-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat(compartment-mapper): Validate compartment maps #1085
feat(compartment-mapper): Validate compartment maps #1085
Conversation
dd5588f
to
a7702d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
@@ -164,12 +165,11 @@ export const parseArchive = async ( | |||
} | |||
|
|||
const compartmentMapText = textDecoder.decode(compartmentMapBytes); | |||
const compartmentMap = /** @type {CompartmentMapDescriptor} */ (parseLocatedJson( | |||
const compartmentMap = parseLocatedJson( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have asked this before, but do our specs say anything about repeated keys in JSON? That would be a way for two different readers to interpret the same compartment map in divergent ways. If TC39 doesn't have a stance, we might want to add a note to the docs around this hashing/security stuff, that correctness depends upon consistent behavior of everybody's JSON.parse
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON (ECMA 404) has no position on repeated keys, but JS (ECMA 262) takes the position that JSON gets evaluated as a JS expression if it parses according to 404, such that the last property defined with the one key wins. So, as long as everyone uses JS, they’ll arrive at the same interpretation.
|
||
const { compartment, ...extra } = scope; | ||
assert( | ||
Object.keys(extra).length === 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tempted to suggest this assertNoExtra(extra, explanation)
pattern be factored out so the calls fit on a single line, but I think I'm just feeling grumpy towards prettier
's verbosity today. The work this file is doing is completely straightforward.
a7702d8
to
a46fb2c
Compare
a46fb2c
to
4204058
Compare
Fixes: #601
Refs: Agoric/agoric-sdk#3859
This change adds validation for compartment map input from archives.
This design sacrifices schema evolution for a tight correspondence between the content of a compartment map and its consistent hash. The validator allows for nothing extra. This may become a hindrance, in which case we would need to relax the extra property assertions and closed ranges for some types. For now, this is conservative in order to provide the maximum assurance to the authors.