feat(import-bundle): Support Endo zip hex bundle format #1983
Conversation
kriskowal
force-pushed
the
kris-endo-zip-hex
branch
2 times, most recently
from
a9f305e
to
50aa8c4
Compare
kriskowal
force-pushed
the
kris-endo-zip-hex
branch
3 times, most recently
from
f3af331
to
a12116b
Compare
| } | ||
|
|
||
| export function f8ReadGlobalSubmodule() { | ||
| export function f7ReadGlobalSubmodule() { |
There was a problem hiding this comment.
Hm, why remove this test? We should still check that globalThis cannot be used as a sneaky channel..
There was a problem hiding this comment.
The crux of the issue here is that freezing globalThis can be optional because freezing it only limits the vulnerability of programs in the same Compartment to one another. With the prior two bundle formats, there is only one compartment so it is necessary and sufficient to freeze a single globalThis to protect the application from its third-party dependencies. With this new bundle format, it is neither necessary nor sufficient to freeze the globalThis, and whether to freeze globalThis should be at the bundled application’s discretion, so that it can be informed by LavaMoat.
I could leave this test in place if I copied and modified the test for the new bundle format, but that seems like an unnecessary maintenance overhead. I could parameterize the test fixture so the constraint could vary. I could alternately go back into compartment mapper and thread a directive to freeze globalThis from package.json or a parallel policy JSON file. I had hoped to defer the last point until we had a better grasp of how LavaMoat will integrate.
| @@ -18,3 +18,8 @@ lockdown({ errorTaming: 'unsafe' }); | |||
| // Even on non-v8, we tame the start compartment's Error constructor so | |||
| // this assignment is not rejected, even if it does nothing. | |||
| Error.stackTraceLimit = Infinity; | |||
|
|
|||
| harden(TextEncoder.prototype); | |||
| harden(TextEncoder); | |||
There was a problem hiding this comment.
Huh, why are these not already hardened? These two are non-ECMA names, right? So they should be removed from the Compartment globals by SES..
There was a problem hiding this comment.
Ah, I see, we're changing the definition of the Compartment provided by import-bundle to include these two objects, and since they're endowments, they must be hardenable (thus .prototype must be hardened ahead of time).
We might want to update docs/vat-environment.md to reflect the availability of these names to vat code. OTOH we might want to prohibit their access (I can't think of any particularly good reason to withhold it, other than uniformity across platforms that might not provide it). (note: there are inbound changes to vat-environment.md from @tyg that I need to merge, so please make any changes to that file in a separate PR).
Is TextEncoder available in both Node.js and browser worlds? Just want to make sure this doesn't prevent swingset from working in a browser. (we haven't really tested that yet, apart from accidentally as part of the wallet, but I don't want to accidentally add a blocker)
There was a problem hiding this comment.
TextEncoder and TextDecoder arrived in browsers before Node.js. They’re the more webby API, which is the reason I’m leaning on them instead of Buffer for Compartment Map’s Zip implementation.
| const endowments = { ...optEndowments }; | ||
| const { source, sourceMap, moduleFormat } = bundle; | ||
| const endowments = { | ||
| TextEncoder, |
There was a problem hiding this comment.
@michaelfig will exposing these enable a metering break? If they're wrapped like all the other globals, probably not, but it's a new ability being handed to all Compartments so I wanted to double-check.
There was a problem hiding this comment.
@kriskowal I'm guessing that TextEncoder and TextDecoder will be available in the immediate Compartments produced by import-bundle, but they won't automatically be in any child compartments created therein. Is that correct? I guess that won't prohibit nested import-bundles, like the kernel does to load vats.
There was a problem hiding this comment.
encode and decode are O(string.length), so we may need to meter them.
There was a problem hiding this comment.
cc @michaelfig I think I need your reaction to the question “do we need metering to attenuate Text{En,De}coder?”, before I land this change.
There was a problem hiding this comment.
cc @michaelfig I think I need your reaction to the question “do we need metering to attenuate Text{En,De}coder?”, before I land this change.
Ideally, the TextEncoder and TextDecoder sources would be evaluated in a metered compartment (not endowed as a vetted shim), so that the metering transform would be applied to them as they are to user code.
There was a problem hiding this comment.
TextEncoder and TextDecoder are platform code.
sigh… We could get metering for free by slurping a UTF-8 dependency, but I’d wanted to avoid that because it’ll be unnecessary bloat on the web. The Text* utilities are available in both Node.js and Web.
There was a problem hiding this comment.
TextEncoder and TextDecoder are platform code.
Oh, in that case, they're covered by the blanket metering performed by @agoric/install-metering-and-ses. No need to special-case (I had thought they were shims written by you).
packages/install-ses/install-ses.js
Outdated
| harden(TextDecoder.prototype); | ||
| harden(TextDecoder); |
There was a problem hiding this comment.
Even with today's too-complicated harden lines 22 and 24 above are redundant. TextEncoder.prototype is reachable by own property traversal from TextEncoder and so will be included in the set hardened when starting with TextEncoder.
kriskowal
force-pushed
the
kris-endo-zip-hex
branch
from
ad08c79
to
c35cb03
Compare
This change introduces
endoZipHexas a supported bundle format forimportBundle. A corresponding change tobundleSourceis forthcoming. I may quickly amend this to a base64 format, given that we have a library for that in SwingSet that we can factor into a shared dependency package.