feat(ses): call lockdown before bundling SES shim

[leotm]

Description

Call lockdown before bundling the SES shim

Discussion: https://youtu.be/qGGeACz4cyM

Security Considerations

Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?

• bundle now fails (TypeError) to build if non-standard JS called after lockdown()
• yarn build now also logs to console (e.g. Removing unpermitted intrinsics) if non-standard JS called before lockdown()

Scaling Considerations

Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated?

Documentation Considerations

Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?

Testing Considerations

Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?

Compatibility Considerations

Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?

Upgrade Considerations

What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed?

Include *BREAKING*: in the commit message with migration instructions for any breaking change.

Update NEWS.md for user-facing changes.

Delete guidance from pull request description before merge (including this!)

[erights]

IIUC, this does not actually change the order in which things happen, at least in the absence of cyclic imports. All imported modules are evaluated/initialized before the code in this module starts evaluation/initialization. So either way, ../index.js initialization will happen before this call to lockdown.

Attn @kriskowal

[kriskowal]

This is true. To interleave behavior relative to other imports, we have to use a module like lockdown.js that just calls lockdown(). So,

import '../index.js'; // or maybe import 'ses' reflexively works now
import './lockdown.js'; // just lockdown()

This insures that ./index.js initializes before you call lockdown().

It’s unclear why the tests pass with this defect. It might be that we need to test yarn build under yarn test.

[leotm]

makes sense ^ reverted feat(ses): call lockdown earlier, looks good now

import './lockdown.js'; // just lockdown()

think we meant import '../lockdown.js'; right
if we prefer this vs import '../index.js';
seems either is fine, since they're both importing the same index file

ah so we should import both, done here 82a4bb4

then hopefully add/update a test to ensure ../lockdown.js is imported before calling lockdown()

[leotm]

It’s unclear why the tests pass with this defect. It might be that we need to test yarn build under yarn test.

yeah the tests still pass calling lockdown() too early before the imports 🤔

endo/packages/ses/package.json

Line 75 in 7a29550

"test": "tsd && ava",

even when doing a clean build before "test": "yarn prepare && tsd && ava", if that's what we meant

[leotm]

Suggested change

	import '../index.js';
	// eslint-disable-next-line import/no-extraneous-dependencies
	import 'ses';

also seems ok here (as mentioned earlier)

[erights]

I'm approving to clear my previous "request changes". FWIW LGTM, but on this I defer to @kriskowal . Please wait for an approval from him.

[kriskowal]

Posted the required change. Thank you for engaging!