fix(pass-style): Relax passable error criteria in hardenTaming unsafe mode. #2990

kriskowal · 2025-10-10T23:34:57Z

This change increases test coverage over lockdown with hardenTaming unsafe. The increase in test coverage revealed the need to relax certain cases, but moreof adjust passStyleOf to repair unfrozen V8 error objects in unsafe hardenTaming mode.

mhofman · 2025-10-10T23:46:36Z

packages/pass-style/src/error.js

+    // Error.prototype.stack is own accessor in V8, but nowhere else to our
+    // knowledge (2025).
+    // We relax validation for "stack" only when harden.isFake.
+    if (!(harden.isFake && propName === 'stack')) {


I fail to see how unsafe harden is related to error stacks. I think what we possibly need to do is allow known "platform own stack accessors" in pass-style. It would be nice to only allow these if there was no lockdown, or if lockdown didn't tame errors.

I’ll think more next week about sensing the effects of error taming variants. I’ll probe your mind next week about “platform own stack accessors” because my understanding is that expectations about the identity or uniqueness of error own get and set may be evolving in a way that will make it harder to sense whether they are the platform’s intrinsic get and set. I fear we may truly need to leave a pinhole for stack in pass-style, generally, and simply be careful to discriminate them with isError and time we destructure all properties of an object.

kriskowal · 2025-10-21T18:03:01Z

We’ve resolved to update this change such that passStyleOf will repair v8 error stack properties iff harden does not. This is a departure from the absolute purity of passStyleOf, but one necessary to sensibly continue supporting the noop mode of harden.

erights · 2025-10-30T23:10:41Z

packages/common/test/object-meta-map.test.js


 const { getOwnPropertyDescriptors, getPrototypeOf } = Object;

+// @ts-expect-error isFake is a secret.


WAT? If it is a secret, it is a very poorly kept secret!

(Please avoid ever seeming to make a security claim that we're not actually making.)

This is a pattern copied from elsewhere. It is not a secret in this sense. It is a secret in the sense that we do not make its existence evident in the type, in order to avoid burdening common users with undue knowledge of its existence. Recommendation welcome.

How about, “Existence of harden.isFake is not communicated by the types.”?

erights · 2025-10-30T23:11:39Z

packages/exo/test/heap-classes.test.js

  makeExo,
 } from '../index.js';

+// @ts-expect-error isFake is a secret property.


erights · 2025-10-30T23:15:03Z

packages/exo/test/heap-classes.test.js

      return obj;
    },
    tortuous(hardA, softB, hardC, optHardD, optSoftE = {}) {
+      // Recall that isFrozen lies with hardenTaming: unsafe


Aren't you fixing that? We should at least vehemently deprecate the existing unsafe-fast.

Yes, I expect this to change to accommodate the new mode. We may need to expose the real isFrozen somewhere to make this test work in every mode.

erights · 2025-10-31T00:14:11Z

packages/pass-style/src/error.js

+  // We do not trust isFrozen because lockdown with unsafe hardenTaming replaces
+  // isFrozen with a version that is in cahoots with fake harden.


This is the unsafe-fast case that we're at least deprecating and hopefully removing soon, yes?

erights · 2025-10-31T00:19:53Z

packages/pass-style/src/error.js

+  }
+
+  const er1StackDesc = getOwnPropertyDescriptor(Error('er1'), 'stack');
+  const er2StackDesc = getOwnPropertyDescriptor(TypeError('er2'), 'stack');


Just curious: Do you have a particular case in mind that you're defending against with the "redundant" check?

I don’t. This is copied from SES.

erights · 2025-10-31T00:21:37Z

packages/pass-style/src/error.js

+  if (
+    er1StackDesc === undefined ||
+    er2StackDesc === undefined ||
+    er1StackDesc.get === undefined
+  ) {


The fact that you wrote || means I'm confused what the purpose of the "redundant" check is. I expected something more like

Suggested change

if (

er1StackDesc === undefined ||

er2StackDesc === undefined ||

er1StackDesc.get === undefined

) {

if (

(er1StackDesc === undefined &&

er2StackDesc === undefined) ||

er1StackDesc.get === undefined

) {

erights · 2025-10-31T00:22:41Z

packages/pass-style/src/error.js

+    return undefined;
+  }
+
+  // We should only encounter this case on v8 because of its problematic


And the JS engine MetaMask is using?

erights · 2025-10-31T00:26:01Z

packages/pass-style/src/error.js

+    // In the v8 case as we understand it, all errors have an own stack
+    // accessor property, but within the same realm, all these accessor
+    // properties have the same getter and have the same setter.
+    // This is therefore the case that we repair.


Should also mention somewhere that the upcoming Error.captureStackTrace proposal may result in more own stack accessors, on more platforms, that may or may not use the same getter and setter. If we don't think those are a problem, we should explain somewhere why not. But not necessarily here.

erights · 2025-10-31T00:26:48Z

packages/pass-style/src/error.js

+    // properties have the same getter and have the same setter.
+    // This is therefore the case that we repair.
+    typeof er1StackDesc.get !== 'function' ||
+    er1StackDesc.get !== er2StackDesc.get ||


Aha! Now I understand

erights · 2025-10-31T00:29:06Z

packages/pass-style/src/error.js

+  // Otherwise, we have own stack accessor properties that are outside
+  // our expectations, that therefore need to be understood better
+  // before we know how to repair them.


Should this comment be moved to immediately above the above throw?

erights

I did not review most of the tests. But I reviewed error.js carefully which LGTM.

Thanks!

erights · 2025-10-31T00:58:42Z

packages/pass-style/src/error.js

+  if (repairError !== undefined) {
+    repairError(candidate);
+  }


Comment here would be good about why we're ok with this special case side effect in what should be a pure query function.

mhofman

Looks good, but would love to see an explicit test for the can't patch error case (either by harden or by passStyleOf).

Also wondering about how lazy the makeRepairError should be.

mhofman · 2025-10-31T22:20:30Z

packages/exo/test/heap-classes.test.js

    },
    rawOut(obj) {
-      t.is(Object.isFrozen(obj), true);
+      t.true(hardenIsFake || Object.isFrozen(obj));


Curious why this is the only Object.isFrozen case where we have to condition on hardenIsFake

I count 9 total. 5 affirmative. 4 negative. Perhaps you mean something else?

I see some Object.freeze calls above in the same test that don't get the same treatment, and arguably I didn't look closely at what the test does, I just saw those lines modified, but not get an extra hardenIsFake condition, and was just wondering why.

Your intuition was onto something, anyway. I went through each of these cases again and found 4 that did not require the relaxation for harden.isFake, left over from when I relaxed these tests to accommodate the unsafe mode of @endo/harden where isFrozen is not rigged. Will have to revisit if we go so far.

packages/pass-style/src/error.js

mhofman · 2025-10-31T22:49:17Z

packages/pass-style/src/error.js

    );
  }
+  if (repairError !== undefined) {
+    // This point is unreachable unless the candidate is mutable and the


How do we know the object is mutable here? I mean it's possible that fake harden didn't freeze but that someone actually explicitly froze it.

packages/pass-style/test/errors.test.js

kriskowal

Pushed a commit to explicitly test frozen-not-hardened errors are unfixable on pathological-V8 (but fine on prior V8 versions we still test).

kriskowal · 2025-11-03T05:20:06Z

packages/exo/test/heap-classes.test.js

    },
    rawOut(obj) {
-      t.is(Object.isFrozen(obj), true);
+      t.true(hardenIsFake || Object.isFrozen(obj));


I count 9 total. 5 affirmative. 4 negative. Perhaps you mean something else?

kriskowal · 2025-11-03T23:00:01Z

I believe I’ve addressed all feedback. Commits added, none changed, for incremental review.

packages/pass-style/src/error.js

mhofman · 2025-11-03T23:32:15Z

packages/pass-style/src/passStyleOf.js

 const { ownKeys } = Reflect;
 const { isFrozen, getOwnPropertyDescriptors, values } = Object;

+const repairError = makeRepairError();


So this still causes makeRepairError to still run at import of @endo/pass-style, but from what I gather, for the use of the toPassableError export, which unlike passStyleOf, is always exported anew.

I think the pass-style package has grown into a weird monster of instantiable and instantiated exports, both exposed on the same entry-point. Because of this I'm tempted to ask to revert my previous request for a lazy repairError, as it's pointless with the current structure of this package. Sorry about the back and forth, I forgot how unwieldy this whole thing is.

packages/pass-style/NEWS.md

packages/pass-style/src/error.js

mhofman

LGTM

Feel free to further revert the repairError change so that it's fully contained within error.js and no changes are required of passStyleOf.js.

…coverage

kriskowal · 2025-11-04T05:42:39Z

packages/exo/test/heap-classes.test.js

  const mutable = {};
  t.is(greeter.sayHello(mutable), 'hello', `passableGreeter can sayHello`);
-  t.is(Object.isFrozen(mutable), true, `mutable is frozen`);
+  t.true(Object.isFrozen(mutable), `mutable is frozen`);


This case relies on harden and isFrozen to be in cahoots. This passes with unsafe harden taming because isFrozen is lying. The safe harden taming passes because harden, called as a side-effect of passing mutable to an Exo, is effective. This will continue to work without modification when we introduce surface hardening. If we obviate unsafe harden taming by making an unsafe mode of @endo/harden which doesn’t collude with isFrozen, this test will need to be adjusted to take that into account.

kriskowal · 2025-11-04T05:47:50Z

packages/exo/test/heap-classes.test.js

    },
    rawIn(obj) {
-      t.is(Object.isFrozen(obj), false);
+      t.false(Object.isFrozen(obj));


This test passes because the M.raw guard is bypassing the harden otherwise implied. It’s always passed an unfrozen/unhardened object in the tests.

kriskowal · 2025-11-04T06:06:48Z

packages/exo/test/heap-classes.test.js

    },
    rawOut(obj) {
-      t.is(Object.isFrozen(obj), true);
+      t.true(hardenIsFake || Object.isFrozen(obj));


Your intuition was onto something, anyway. I went through each of these cases again and found 4 that did not require the relaxation for harden.isFake, left over from when I relaxed these tests to accommodate the unsafe mode of @endo/harden where isFrozen is not rigged. Will have to revisit if we go so far.

kriskowal · 2025-11-04T06:07:20Z

packages/exo/test/heap-classes.test.js

+      t.false(Object.isFrozen(obj));
      return obj;
    },
    rawOut(obj) {


rawOut has an M.any() guard, so the argument is implicitly hardened, albeit fake hardened. The relaxation is unnecessary in this case and I will restore the case.

kriskowal · 2025-11-04T06:16:26Z

packages/exo/test/heap-classes.test.js

-      t.is(Object.isFrozen(optSoftE), false);
+      t.true(Object.isFrozen(hardC));
+      t.true(Object.isFrozen(optHardD));
+      t.false(!hardenIsFake && Object.isFrozen(optSoftE));


This case requires a relaxation because isFrozen is misreporting a soft object has having been frozen. It’s either a soft object that was passed through a raw optional arg, or defaulted to a soft object.

Refs: #2990 ## Description This change synchronizes the error repair mechanism in SES with changes made in #2990 to pass-style to gratutiously improve the resilience of that mechanism in the face of scripts that ran before SES. ### Security Considerations This reduces SES vulnerability to corruption from code that runs before SES. Replacing the TypeError constructor cannot confuse SES with regard to whether the platform produces type errors with own stack properties. ### Scaling Considerations None. ### Documentation Considerations None. ### Testing Considerations We cover versions of Node.js with and without own stack properties in CI and existing tests should be sufficient. ### Compatibility Considerations Programs that previously deceived SES may now fail when initializing SES. We consider such programs compromised and that incompatibility is a feature. ### Upgrade Considerations None.

kriskowal requested a review from erights October 10, 2025 23:35

mhofman reviewed Oct 10, 2025

View reviewed changes

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from 8f28d6a to cd71d7d Compare October 11, 2025 03:10

kriskowal force-pushed the kriskowal-ses-ava-multi-config branch from 87d00cd to 342ac49 Compare October 11, 2025 03:10

kriskowal marked this pull request as draft October 21, 2025 18:00

kriskowal force-pushed the kriskowal-ses-ava-multi-config branch 3 times, most recently from df3ba90 to 2b0119f Compare October 28, 2025 01:43

Base automatically changed from kriskowal-ses-ava-multi-config to master October 28, 2025 01:50

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from cd71d7d to afa9989 Compare October 30, 2025 22:06

kriskowal requested a review from mhofman October 30, 2025 22:08

kriskowal marked this pull request as ready for review October 30, 2025 22:08

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from afa9989 to 0fd4738 Compare October 30, 2025 22:15

erights reviewed Oct 30, 2025

View reviewed changes

erights reviewed Oct 31, 2025

View reviewed changes

erights approved these changes Oct 31, 2025

View reviewed changes

erights reviewed Oct 31, 2025

View reviewed changes

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from 0fd4738 to 9cbb853 Compare October 31, 2025 21:30

mhofman reviewed Oct 31, 2025

View reviewed changes

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from 9cbb853 to 392e030 Compare November 3, 2025 05:20

kriskowal commented Nov 3, 2025

View reviewed changes

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from 946b89b to 5221365 Compare November 3, 2025 22:58

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from 5221365 to de4795c Compare November 3, 2025 23:19

mhofman reviewed Nov 3, 2025

View reviewed changes

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch 5 times, most recently from 71c7d0f to 6feeca5 Compare November 4, 2025 01:50

mhofman reviewed Nov 4, 2025

View reviewed changes

packages/pass-style/src/error.js Outdated Show resolved Hide resolved

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from 6feeca5 to 95c8156 Compare November 4, 2025 02:05

mhofman approved these changes Nov 4, 2025

View reviewed changes

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from 95c8156 to 8c55a42 Compare November 4, 2025 05:22

kriskowal added 11 commits November 3, 2025 22:33

test(common): Cover unsafe mode

7ad4f55

test(exo): Cover hardenTaming unsafe mode

b862452

test(check-bundle): Cover unsafe mode

5f8dd42

test(marshal): Cover unsafe mode (reveals pass-style defect)

a43541b

test(ocapn): Cover unsafe mode

bf8e651

feat(pass-style): Repair V8 errors if harden is fake

db9bade

test(pass-style): Cover unsafe hardenTaming mode

5a996ec

test(pass-style): Relaxations necessary for unsafe hardenTaming test …

e6fc121

…coverage

docs: isFake is not advertised but also not a secret

674f686

test(pass-style): Test edge case of frozen, not hardened error on V8

5c19c25

docs(pass-style): News for error repair under unsafe harden taming

39f8386

kriskowal force-pushed the kriskowal-fix-harden-taming-unsafe branch from 8c55a42 to 39f8386 Compare November 4, 2025 06:33

kriskowal commented Nov 4, 2025

View reviewed changes

kriskowal merged commit a5a0f5c into master Nov 4, 2025
19 checks passed

kriskowal deleted the kriskowal-fix-harden-taming-unsafe branch November 4, 2025 06:39

kriskowal mentioned this pull request Nov 4, 2025

feat(ses): Synchronize error stack repair mechanism #3004

Merged


		const { getOwnPropertyDescriptors, getPrototypeOf } = Object;

		// @ts-expect-error isFake is a secret.

		// We do not trust isFrozen because lockdown with unsafe hardenTaming replaces
		// isFrozen with a version that is in cahoots with fake harden.

fix(pass-style): Relax passable error criteria in hardenTaming unsafe mode. #2990

fix(pass-style): Relax passable error criteria in hardenTaming unsafe mode. #2990

Uh oh!

Conversation

kriskowal commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kriskowal commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erights Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erights left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhofman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kriskowal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kriskowal commented Nov 3, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhofman left a comment

Choose a reason for hiding this comment

kriskowal commented Oct 10, 2025 •

edited

Loading

erights Oct 31, 2025 •

edited

Loading