fix(ses): `harden` hacks v8 `stack` own accessor problem

erights commented 2 months ago

closes: #2198 refs: #2230 #2200 https://chromium-review.googlesource.com/c/v8/v8/+/4459251 #2229 #2231 https://issues.chromium.org/issues/40279506

Description

Alternative to #2229 that just hacks harden to directly repair a problematic error own stack accessor property, replacing it with a data property.

Security Considerations

Both before and after this PR, passStyleOf will reject errors with the v8 problematic error own stack accessor property, preventing the unsafety at stake here. However, this would mean that much existing code that used to be correct will break when run on a v8 with this problem.

Scaling Considerations

Avoids any extra overhead on platforms without this problem, including all platforms other than v8.

Documentation Considerations

probably none. This PR essentially avoids the need to document the v8 problem that it masks.

Testing Considerations

Only needed to repair one test to use harden rather than freeze, in a case where harden was more natural anyway.

Compatibility Considerations

This PR enables more errors to pass that check without further changes to user code. #2229 had similar goals, but would still require more changes to user code than this PR. This is demonstrated by all the test code in #2229 that needed to be fixed that does not need to be fixed in this PR.

Upgrade Considerations

none

~[ ] Includes *BREAKING*: in the commit message with migration instructions for any breaking change.~
~[ ] Updates NEWS.md for user-facing changes.~

erights commented 2 months ago

CI for #2233 fails on Node 21 due to the v8 problems that this PR hacks around. CI for #2234 succeeds, validating that for at least these occurrences of the problem, this PR fixes them.

erights commented 2 months ago

Since #2231 was already merged, causing CI errors for the CI cases that this PR is trying to address, I added to this PR the change from #2233 / #2234 that switches from trying to use 22.x, which fails because it is not found, to using 21.x, which is found. I will thus also close #2233 and #2234 in favor of this PR.

mhofman commented 2 months ago

I want to hear more from @mhofman about how this change would preclude a separable harden shim.

This is a repair that harden does for the benefit other layers (pass style). IMO, it's conceptually hard to justify its presence in the ses shim itself. It wouldn't be justifiable at all in a standalone harden shim.

erights commented 2 months ago

I want to hear more from @mhofman about how this change would preclude a separable harden shim.

This is a repair that harden does for the benefit other layers (pass style). IMO, it's conceptually hard to justify its presence in the ses shim itself. It wouldn't be justifiable at all in a standalone harden shim.

It could be exposed as a configuration option in a standalone harden shim. I agree that it is unpleasant, but the underlying problem caused by v8 is the source of the unpleasantness. "Fixing" it in harden is, I think, the fix that's least unpleasant for users, as evidenced by all the test case changes that I no longer needed to do.

erights commented 2 months ago

endojs / endo