Skip to content
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

SES vs rollup #9

Closed
warner opened this issue Feb 7, 2020 · 7 comments
Closed

SES vs rollup #9

warner opened this issue Feb 7, 2020 · 7 comments

Comments

@warner
Copy link
Contributor

warner commented Feb 7, 2020

I'm working on getting SwingSet to build under the new SES shim. I'm going to use this ticket to collect problems that I've run into so far. These aren't necessarily problems with SES, they may be more like ".. so don't do that" items, but we might need to figure out workarounds and include them in the docs for the benefit of programmers trying to use SES.

1: rollup uses Math.random()

I was surprised to see that rollup calling Math.random(), which of course fails (throws a disabled error). A function named assignChunkColouringHashes randomizes a list somehow. There are a pair of switches (--preserveModules or --inlineDynamicImports) which bypass this code, but have other effects that aren't trivial to deal with. The immediate workaround was to lockdown({ noTameMath: true }), but of course that's only temporary.

2: rollup uses for .. in instead of a Map, fails on toLocaleString

Rollup walks through a list of modules and adds each one to an object named chunkModules, using a hash of the entrypoints as the property name. It then attempts to iterate through those modules by calling for .. in on chunkModules. Search rollup's Graph.ts for chunkModulesOrdered to find the code.

This is choking on a surprising property named toLocaleString. I don't think it's an own-property, but I forget the rules for for .. in (because it's horrible and we don't use it, of course), and why it appears when none of the other built-ins show up. We modify toLocaleString unless lockdown() is called with the noTameDate: true option. Setting that is the quick+temporary workaround. But we need to figure out how to tame this property while still hiding it from foolish code that uses for .. in instead of a proper Map.

@erights
Copy link
Contributor

erights commented Feb 7, 2020

When can we stop using rollup, or any third party translator whose translation we don't control?

How soon can we shift to our own make-importer module system integrated with the SES-shim?

Given the make-importer integration, does our packer become, essentially, zip? (Note that Java's .jar files are a small wrapper around zip.)

@michaelfig
Copy link
Member

@erights we'll still need rollup until we can get rid of all CommonJS dependencies from the SwingSet code. Not sure how far away that is.

@warner
Copy link
Contributor Author

warner commented Feb 7, 2020

3: harden cannot (and should not) be evaluated under SES

harden is a vital utility that "deep freezes" the API surface of an object. Within the new SES world, the harden() function is available on the global object. In the old SES world, harden() was provided by SES through a require hook, so user code could say const harden = require('@agoric/harden') (or, if it was transformed somehow, it could say import harden from '@agoric/harden').

We chose that "module" name so that we could also publish a module to NPM that behaves similarly to the real harden: close enough for unit tests. This not-quite-harden freezes the object and recursively crawls its enumerable properties and prototype chain, just like the real one, but it stops short of freezing the primordials like Object.prototype. In a SES world, these are already frozen. This seemed good enough to let developers' unit tests discover direct mutations of a supposedly-frozen object that would obviously not work in the real SES environment, without "Ice-9" freezing the entire world in a very surprising way.

This let us recommend @agoric/harden as a general-purpose tool that should be used by all code, knowing that it gives educational error messages when run outside of SES, and actual protection when run inside SES.

In new-SES, we want to get rid of require, which means we not longer want to get harden with an import/require. Making it visible as a global is a fine way to deliver it to SES code, but I don't know how we should make it available to non-SES code. @michaelfig has an approach in mind where we continue to publish a module, but it checks to see if harden is present on the global before defining anything.

The specific problem here is that harden includes a call that looks enough like a direct eval that SES rejects it:

// this use of 'global' is why Harden is a "resource module", whereas
// MakeHardener is "pure".
const initialRoots = buildTable((0, eval)('this')); // eslint-disable-line no-eval

After removing the external: list from bundle-source's use of rollup (to avoid require() in the output bundle), then if anything we feed to bundle-source imports harden (regardless of whether we call it or not), this line gets included in the bundle, which makes SES choke.

I don't have a good answer yet, just trying to explain the problem well enough to have more discussion about it.

@erights
Copy link
Contributor

erights commented Feb 7, 2020

I am surprised that SES rejects

(0, eval)('this'))

Are you sure?

@warner
Copy link
Contributor Author

warner commented Feb 7, 2020

something is complaining about SyntaxError: possible direct eval expression rejected around line 3195. I have to do some more digging to be sure, but a previous archaeological expedition reported that line being the buildTable call. I'll keep investigating.

@warner
Copy link
Contributor Author

warner commented Feb 7, 2020

Oh, hah, SES is snagging the no-eval in the comment at the end of the line. Same issue as Agoric/agoric-sdk#43 . If I remove that comment, I hit some other problem (ReferenceError: _a6a is not defined, which I know has to do with esm).

@jfparadis
Copy link
Contributor

Closing, since all is resolved. @warner feel free to reopen!

1: rollup uses Math.random()

It was discovered that this issue was not related to the SES shim. Math.random() need to be allowed or tamed differently to allow normal rollup operation.

2: rollup uses for .. in instead of a Map, fails on toLocaleString

This is fixed by making toLocaleString non-enumerable.
See #10

3: harden cannot (and should not) be evaluated under SES

It was discovered that this issue is not related to the SES shim: rollup needs to remove the bundling of harden, and makeRequire, ported from the previous SES shim, needs to move to Swingset.

kriskowal pushed a commit that referenced this issue Jan 12, 2022
* fix(resolve): protect against reentrancy attack

Closes #9

* fix(resolve): harden argument to HandledPromise.resolve

The harden calls in eventual-send already need an SES-like
environment for proper security.  Make HandledPromise.resolve
simpler and prevent proxy trickery.

* fix(HandledPromise): set prototype to Promise

* fix(test-thenable): proper workaround of override mistake
kriskowal pushed a commit that referenced this issue Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants