fix(ses): Distinguish intrinsics in start and non-start compartments #372

erights · 2020-07-02T22:18:20Z

Whitelists no longer painfully redundant.
Placeholder for multiple Compartment constructors, including one which only throws.

Start compartment has initial authority. Others do not. Independent of *Taming options.
{ *Taming: 'unsafe' } is no longer about initial authority in start compartment, as long as it obeys ocap rules.
'unsafe' is now about enabling violations of ocap rules, such as primordials that are no longer powerless.
Lockdown *Taming options have much milder meaning.

We still check that they are only set to 'safe' or 'unsafe', with default 'safe'.
dateTaming is ignored. Maybe we should just get rid of this flag. Transition problem?
mathTaming is ignored. Maybe we should just get rid of this flag. Transition problem?
regExpTaming is effectively ignored. But we may decide that 'unsafe' suppresses the deletion of RegExp.prototype.compile.
The new localeTaming, if set to 'unsafe', leaves the original *Locale* methods in place.

Finally we get to Error taming. Independent of the errorTaming option, the tamed powerful v8 API emulation remains on the start compartment's Error constructor. None of these are on the Error constructor of other compartments. However, this shows that we need a different knob: whether to expose engine-specific (e.g., v8 specific) APIs.

In a step towards https://tc39.es/proposal-error-stacks/ , we leave on the start compartment the powerful getStackString(error) function, which gives the stacktrace string associated with that error. Because this special power is only on the start compartment, we do not consider it a material violation of ocap rules, so it is there independent of errorTaming.

The violation of ocap rules which is pervasively available is the legacy behavior of error.stack, since it takes no privilege but violates information hiding. Accordingly, on v8 where we have control, under errorTaming: 'safe' (or default) this is truncated to the empty string. The 'unsafe' setting leaves the legacy behavior alone, providing the same string that the new getStackString(error) provides.

Fixes #317
Fixes #385 by getting rid of the realmRec altogether.

erights · 2020-07-19T06:55:53Z

@kriskowal @warner I hit the "ready for review" button, though I am aware that this needs to be divided into smaller reviewable PRs. Nevertheless, it works, is a big improvement of the status quo, and even seems to work. Comments appreciated, especially on strategy for breaking it up.

dckc · 2020-07-20T15:09:24Z

@erights writes:

In a step towards https://tc39.es/proposal-error-stacks/ , we leave on the start compartment the powerful getStackString(error) function, which gives the stacktrace string associated with that error. Because this special power is only on the start compartment, we do not consider it a material violation of ocap rules ...

yay! unsealException restored to its former glory. (actually, I don't see it in erights.org nor https://github.com/kpreid/e-on-java ; did E have unsealException?)

erights · 2020-07-20T16:12:03Z

yay! unsealException restored to its former glory. (actually, I don't see it in erights.org nor https://github.com/kpreid/e-on-java ; did E have unsealException?)

@kpreid Did you implement something like that? (I remember us talking about it.)

kpreid · 2020-07-20T16:45:30Z

I implemented sealed exceptions in E-on-CL only.

Sealed exception primitives: https://github.com/kpreid/e-on-cl/blob/f93d188051c66db0ad4ff150bd73b838f7bc25ed/lisp/base.lisp#L75
The compiler implements sealing by transforming any exception (condition in CL terms) caught by a CatchExpr before passing it to the catch block code: https://github.com/kpreid/e-on-cl/blob/f93d188051c66db0ad4ff150bd73b838f7bc25ed/lisp/compiler-seq.lisp#L197
unsealingConditionGuard(class) makes guards which unseal exceptions and also match against the hidden type; this was intended to be the tool by which privileged E code accessed sealed exceptions: https://github.com/kpreid/e-on-cl/blob/f93d188051c66db0ad4ff150bd73b838f7bc25ed/lisp/knot.lisp#L76
The Lisp-side e-condition-unseal function is also used in the Updoc implementation and other diagnostic/logging things.

Let me know if any other pointers would be useful.

kriskowal

Reverse engineering of this change’s internal chronology:

Change StaticModuleRecord from class to function.
Create shared inert StaticModuleRecord.
Convert Compartment class to Compartment function and prototype
Rewrite createGlobalObject into initGlobalObject
a. Accept globalObject as an argument instead of creating one on behalf of caller
a. Thread intrinsics and sharedGlobalPropertyNames through initGlobalObject
a. Refactor switch to property loops
Create conventions for referring to distinct intrinsics for start and shared compartments.
a. Reframe taming functions not in terms of “start vs shared” but a single object bag that captures the intrinsics for both environments using %Names%
a. Reframe allow list keys %AllAllowListKeys%
nit: Fix misaligned JSDoc for performEval
Factor realmRec argument out of createScopeHandler and makeEvaluateFactory. The realmRec is cruft from before compartments and we only need the original, “feral” Function constructor from it. Better to just capture that. (Delete realm-rec.js)
nit: Rename XPrototypeConstructor to XOnlyThrows generally.
Add CompartmentOnlyThrows and StaticModuleRecordOnlyThrows to anonymous intrinsics
Delete get-named-intrinsic.js
Delete intrinsics-global.js
Reframe defineProperty as initProperty
Factor an “intrinsics manager” out of getGlobalIntrinsics. Reuse this part to initialize intrinsics on new global objects for compartments.
Factor repairIntrinsics and hardenIntrinsics out of lockdown using the intrinsics manager.
Rename the taming modules. (This obscures changes within these modules so needs to be effected in an isolated mechanical commit)
nit: Fix non-descriptive performEval x / source local variable.
nit: Fix unreported!? linting problem with scope-handler unused shadow argument.

packages/ses/src/compartment-shim.js

packages/ses/src/intrinsics.js

packages/ses/src/lockdown-shim.js

packages/ses/src/make-evaluate-factory.js

kriskowal · 2020-07-21T23:16:09Z

packages/ses/test/identity-continuity.test.js

@@ -33,8 +33,6 @@ test('identity Array', t => {

 // Compartment is a shared global
 test('identity Compartment', t => {
-  t.plan(7);


Should bring this back.

Sigh. I understand why we need it for async tests. But can't we omit it for purely sequential-synchronous tests?

erights · 2020-07-21T23:53:20Z

What is %AllAllowListKeys%? I don't think I have seen it before.

erights · 2020-07-21T23:59:30Z

Your extended comment of how to unpack this PR seems exactly right. Thanks!

kriskowal · 2020-07-23T17:36:07Z

I’ve decided to review this monolithically. That generally means more rounds of back-and-forth, but separating the mechanical changes from the semantic changes will in this case probably take longer than the review. So, I’m releasing the lock on this branch. I may instead push nit fix commits to the end in the process of a local holistic review, but will not force push.

kriskowal

I’ve left some comments that I resolved myself by pushing commits.

Looks great to me!

packages/ses/src/intrinsics.js

packages/ses/src/lockdown-shim.js

packages/ses/test/tame-regexp-unit.test.js

packages/ses/src/whitelist-intrinsics.js

packages/ses/test/get-intrinsics.test.js

kriskowal · 2020-07-23T18:27:32Z

packages/ses/test/tame-date.test.js

+  t.assert(Number.isInteger(Date.now()));
+  t.ok(isDate(new Date()));


Let’s use ok instead of assert throughout. They are aliases.

Addressed with a follow-up commit.

packages/ses/src/compartment-shim.js

packages/ses/src/lockdown-shim.js

erights

For some of your comments, github won't let me answer inline.

Answering #372 (comment)

Let’s carry this through or link an issue to track.

See #390

Answering #372 (comment)

Let’s use ok instead of assert throughout. They are aliases.
Addressed with a follow-up commit.

Ok. I made no further changes along these lines. But should we do this everywhere? I like strong "there are very few ways to do that" conventions, so I'd be happier if we did it everywhere.

packages/ses/src/compartment-shim.js

packages/ses/src/whitelist-intrinsics.js

erights · 2020-07-24T02:41:13Z

packages/ses/test/identity-continuity.test.js

@@ -33,8 +33,6 @@ test('identity Array', t => {

 // Compartment is a shared global
 test('identity Compartment', t => {
-  t.plan(7);


Sigh. I understand why we need it for async tests. But can't we omit it for purely sequential-synchronous tests?

erights force-pushed the default-prepareFn branch from 08ada7b to 6a3b37f Compare July 11, 2020 07:30

erights mentioned this pull request Jul 11, 2020

ses: Factor out makeCompartmentConstructor #363

Closed

erights self-assigned this Jul 11, 2020

erights mentioned this pull request Jul 13, 2020

[WIP] Intrinsic reform --- nothing works yet #334

Closed

erights force-pushed the default-prepareFn branch 3 times, most recently from 88cb07b to f5c2e1b Compare July 19, 2020 06:23

erights changed the title ~~fix: [WIP] preserve stack trace info~~ fix: Massive intrinsic reform. Start vs other compartments. Jul 19, 2020

erights marked this pull request as ready for review July 19, 2020 06:52

erights added the taming compat label Jul 19, 2020

kriskowal reviewed Jul 21, 2020

View reviewed changes

kriskowal approved these changes Jul 23, 2020

View reviewed changes

kriskowal changed the title ~~fix: Massive intrinsic reform. Start vs other compartments.~~ fix(ses): Distinguish intrinsics in start and non-start compartments Jul 23, 2020

kriskowal mentioned this pull request Jul 23, 2020

Factor out Compartment constructor maker #366

Closed

erights force-pushed the default-prepareFn branch from a071709 to 1885370 Compare July 24, 2020 01:36

erights mentioned this pull request Jul 24, 2020

Need new test that tests something like obsolete get-intrinsics.test.js #390

Open

fix: Massive intrinsic reform. Start vs other compartments.

03791e1

erights force-pushed the default-prepareFn branch from 8feeea1 to 03791e1 Compare July 24, 2020 03:07

erights commented Jul 24, 2020

View reviewed changes

erights merged commit 5cf2a20 into master Jul 24, 2020

erights deleted the default-prepareFn branch July 24, 2020 03:10

This was referenced Oct 11, 2024

fix(ses): reject unsupported lockdownOptions mathTaming + dateTaming #2583

Closed

fix(ses): warn on unsupported lockdownOptions mathTaming + dateTaming #2584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ses): Distinguish intrinsics in start and non-start compartments #372

fix(ses): Distinguish intrinsics in start and non-start compartments #372

erights commented Jul 2, 2020 •

edited

Loading

erights commented Jul 19, 2020

dckc commented Jul 20, 2020

erights commented Jul 20, 2020 •

edited

Loading

kpreid commented Jul 20, 2020 •

edited

Loading

kriskowal left a comment •

edited

Loading

kriskowal Jul 21, 2020

erights Jul 24, 2020

erights commented Jul 21, 2020

erights commented Jul 21, 2020

kriskowal commented Jul 23, 2020

kriskowal left a comment

kriskowal Jul 23, 2020

kriskowal Jul 23, 2020

erights left a comment

erights Jul 24, 2020

		t.assert(Number.isInteger(Date.now()));
		t.ok(isDate(new Date()));

fix(ses): Distinguish intrinsics in start and non-start compartments #372

fix(ses): Distinguish intrinsics in start and non-start compartments #372

Conversation

erights commented Jul 2, 2020 • edited Loading

erights commented Jul 19, 2020

dckc commented Jul 20, 2020

erights commented Jul 20, 2020 • edited Loading

kpreid commented Jul 20, 2020 • edited Loading

kriskowal left a comment • edited Loading

Choose a reason for hiding this comment

kriskowal Jul 21, 2020

Choose a reason for hiding this comment

erights Jul 24, 2020

Choose a reason for hiding this comment

erights commented Jul 21, 2020

erights commented Jul 21, 2020

kriskowal commented Jul 23, 2020

kriskowal left a comment

Choose a reason for hiding this comment

kriskowal Jul 23, 2020

Choose a reason for hiding this comment

kriskowal Jul 23, 2020

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

erights Jul 24, 2020

Choose a reason for hiding this comment

erights commented Jul 2, 2020 •

edited

Loading

erights commented Jul 20, 2020 •

edited

Loading

kpreid commented Jul 20, 2020 •

edited

Loading

kriskowal left a comment •

edited

Loading