fix(ses): fix #2951 stronger sniffing for v8 #2952

erights · 2025-09-12T00:25:12Z

Closes: #2951
Refs: #1798 , https://github.com/tc39/proposal-error-capturestacktrace , https://github.com/tc39/proposal-error-stacks , https://github.com/tc39/proposal-error-stack-accessor

Description

(Description mostly from Copilot's overview below, which is quite good!)

This PR's error taming implements stronger platform detection for v8, addressing issue #2951. The change improves the reliability of v8 detection by testing the prepareStackTrace mechanism, in addition to checking for the presence of captureStackTrace.

This is needed because some non-v8 platforms have implemented captureStackTrace without prepareStackTrace. The https://github.com/tc39/proposal-error-capturestacktrace proposal would make it standard, eventually causing all conformant JS engines to implement it.

A verbally reported bug motivated this, even though that bug itself may be a false alarm. The report raised the possibility that our error-taming of treating the Hermes platform as if it was v8, and then misbehaving because it does not act like v8. Whether or not this actually happens on Hermes, it is plausible and would happen elsewhere with ses prior to this PR.

Security Considerations

If non-v8 engines start implementing the prepareStackTrace mechanism, we may need to tighten the sniff test once again. Such is the nature of sniff tests. Since we do not yet know the specifics of such possible additions, it does not yet make sense to tighten the sniff test, unless I'm missing some we should sniff instead.

During such a transition period, it is possible that the falsly-triggered v8 error taming could misbehave, including in ways that may be exploitable.

Scaling Considerations

none

Documentation Considerations

This is a bug fix and does not need documentation beyond the issue, this PR, and the inline comments added by this PR.

Testing Considerations

I did not add automated tests, because I am unclear on how automated tests would test this. Reviewers: suggestions welcome.
What I did do is manually step through the logic in the vscode debugger while running on Node. The sniff test code worked as expected there. If we do rely on such manual testing, I should at least test on some non-v8 platform that does implement captureStackTrace.

Compatibility Considerations

See "Security Considerations" above.

Upgrade Considerations

none

Copilot

Pull Request Overview

This PR implements stronger platform detection specifically for v8 JavaScript engine in the SES error constructor taming functionality, addressing issue #2951. The change improves the reliability of v8 detection by testing the prepareStackTrace mechanism rather than just checking for the presence of captureStackTrace.

Key changes:

Enhanced v8 detection logic using prepareStackTrace functionality testing
Added import for isArray utility function
Replaced simple function existence check with comprehensive platform sniffing

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

packages/ses/src/error/tame-error-constructor.js

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

naugtur · 2025-09-15T12:19:37Z

Get ready, this will hurt.

Hermes does implement Error.prepareStackTrace and calls it with error and CallSite array

CallSite implementation is incomplete, mainly because Hermes doesn't have a concept of source files.

(...) have been added, full paths are present generally

Result of calling all V8 CallSite methods:

method	return type	return value
getColumnNumber	number	3
getEnclosingColumnNumber	number	1
getEnclosingLineNumber	number	41
getEvalOrigin	undefined
getFileName	string	file:///(...)endo/packages/ses/test/hermes/prepare-stack.js
getFunction	undefined
getFunctionName	string	myfunction
getLineNumber	number	42
getMethodName	object
getPosition	number	965
getPromiseIndex	object
getScriptNameOrSourceURL	string	file:///(...)endo/packages/ses/test/hermes/prepare-stack.js
getScriptHash	string	8ce5e9bc697fd7bb9acae06a235f27efab97fdb82378433b06a63ffe38c8303b
getThis	undefined
getTypeName	object
isAsync	boolean	false
isConstructor	boolean	false
isEval	boolean	false
isNative	boolean	false
isPromiseAll	boolean	false
isToplevel	boolean	true
toString	string	myfunction (file:///(...)endo/packages/ses/test/hermes/prepare-stack.js:42:3)

Result of calling all the same methods in Hermes

method	return type	return value
getColumnNumber	number	8
getEnclosingColumnNumber	Error	undefined is not a function
getEnclosingLineNumber	Error	undefined is not a function
getEvalOrigin	object
getFileName	string	/(...)endo/packages/ses/test/hermes/prepare-stack.js
getFunction	undefined
getFunctionName	string	myfunction
getLineNumber	number	42
getMethodName	object
getPosition	Error	undefined is not a function
getPromiseIndex	object
getScriptNameOrSourceURL	Error	undefined is not a function
getScriptHash	Error	undefined is not a function
getThis	undefined
getTypeName	object
isAsync	boolean	false
isConstructor	object
isEval	object
isNative	boolean	false
isPromiseAll	boolean	false
isToplevel	object
toString	string	[object CallSite]

We could either make sniffing even stronger by testing for usefulness of toString
or we could keep the V8 detection in a state that also covers hermes (might need to rename some things?) and add a fallback for broken toString

naugtur · 2025-09-15T13:19:27Z

I've pushed two commits:

test runner for hermes with some tests covering the smoke test and known issues
a potential fallback for repareStackTrace CallSite toString method being broken

naugtur · 2025-09-15T14:12:39Z

packages/ses/scripts/hermes-test.js

+;;;;
+
+var TEST;
+function test(name, fn) {


Thanks to the test going in a function I can start the concatenation with it, so if errors are thrown, the line numbers will be correct. SES gets concatenated after but the test runs synchronously after SES still.

naugtur · 2025-09-15T14:13:23Z

packages/ses/src/error/tame-error-constructor.js

+            // like v8.
+            platform = 'v8';
+
+            if (`${sst[0]}` === '[object CallSite]') {


Here we detect Hermes providing a broken toString

naugtur · 2025-09-15T14:14:08Z

packages/ses/src/error/tame-error-constructor.js

+                uncurryThis(csProto.getColumnNumber),
+              ];
+
+              callSiteToStringFallback = callSite =>


Instead of creating it and passing around, we could instead repair the original, but I'm afraid someone else might be doing similar feature detection

naugtur · 2025-09-15T14:14:48Z

packages/ses/test/hermes/error-taming-unsafe.js

+// Error.captureStackTrace = null
+
+/* global test */
+test('error taming unsafe', () => {


This is the one that fails without my toStringFallback

naugtur · 2025-09-15T14:15:08Z

packages/ses/test/hermes/known-issue-callsite.js

@@ -0,0 +1,92 @@
+/* global test */
+test('known issue: CallSite implementation', () => {


full test documenting broken callsite implementation

naugtur · 2025-09-15T14:16:12Z

packages/ses/test/hermes/known-issue-promise.js

@@ -0,0 +1,36 @@
+/* global test */
+/* global repairIntrinsics */
+test('knonw issue: Hermes Promise is non-standard', () => {


This test documents the broken promise implementation and verifies that the fix we're applying in lavamoat still works.

Are we interested in adding the fix to ses-hermes? I think it'd make sense - Promise constructors will throw often if not fixed.

Where can I find this fix?

The fix is demonstrated here (capturing all truthy underscore-prefixed fields from Promise and preserving them till after repair to bring them back)
It's used in @lavamoat/react-native-lockdown but I think if we claim SES works under Hermes, we should consider adding this exception to the hermes build by default.

Another awful detail - the field that is exposed and must exist or the Promie implementation falls apart is containing a no-op function that the implementation reuses in a few places, which could have been a local scope function. Other fields, null by default, allow instrumenting all promises, so good riddance.

erights · 2025-09-15T23:25:39Z

Get ready, this will hurt.

Thanks for the warning. It did indeed hurt :/

mhofman · 2025-09-15T23:33:51Z

packages/ses/src/error/tame-error-constructor.js

+    if (typeof originalPrepareStackTrace === 'function') {
+      // This case should not occur on v8 or any other platform.
+      // But if it does, we assume we're on v8, or a platform whose
+      // error stack logic is close enough that we can treat it
+      // like v8.
+      platform = 'v8';


I believe it does happen in Node.js according to the realm init logic I'm seeing: https://github.com/nodejs/node/blob/b8870c4a61f9c4c4490e43472e98655b00055359/lib/internal/bootstrap/realm.js#L444-L470

FYI that logic shows that it's not v8 that calls Error.prepareStackTrace but the embedder that sets a callback (in this case setPrepareStackTraceCallback), which itself does the forwarding to Error.prepareStackTrace (or some other fallback)

Huh. Testing just now, it was added sometime between Node v18.19.0 exclusive and Node v20.18.3 inclusive.

erights · 2025-09-15T23:34:12Z

potential fallback for repareStackTrace CallSite toString method being broken

Btw, given the topic, I first read this as "repairStackTrace" and became unreasonably hopeful for a moment ;) .

mhofman

Given the new modular design of prepareStackTrace in Node.js introduced in nodejs/node#50827, I think we should at least restore (or not touch?) the original prepareStackTrace if the errorTaming mode is 'unsafe'.

Also, since this is so related, maybe we could attempt to leverage the original prepareStackTrace to build the stack string (see #1798), but happy to have this as a follow up PR.

(@mhofman , restoring your original. Somehow when I thought I was replying to it, I accidentally revised it instead. In any case, I found a better place for the reply, above.)

erights · 2025-10-07T23:44:19Z

@naugtur , since the problematic case is Hermes, and since you've contributed so much of this PR anyway, should you take it from here?

Github will allow me to assign you. But due to the fact that I started the PR, Github won't let me review. So if you do take it from here, just informally consider me a reviewer. Thanks!

naugtur · 2025-10-08T11:49:12Z

Happy to take over, but may need some help with specifying the desired scope / what's left to do.

@mhofman, care to elaborate on the comment about who calls prepareStackTrace? How does it affect what we should do here?

If Node is doing things, should I expect the behavior to differ between Node and Chrome?

mhofman · 2025-10-09T05:54:50Z

@mhofman, care to elaborate on the comment about who calls prepareStackTrace? How does it affect what we should do here?

If Node is doing things, should I expect the behavior to differ between Node and Chrome?

My understanding is that v8 itself only implements a mechanism so that the host can specify the callback to use to prepare stack traces, and Chrome and v8 both register and internal callback (setPrepareStackTraceCallback(prepareStackTraceCallback) in the case of Node.js) that ends up calling whatever function is at Error.prepareStackTrace if it exists.

Node.js further has an internal default behavior for prepareStackTrace that depends on whether source maps are enabled or not. That internal behavior is now exposed as a "proxy" function set as the initial value for Error.prepareStackTrace. That means that the comment currently saying "This case should not occur on v8 or any other platform" is in fact inaccurate.

Instead of bailing out and assume v8, we may want to still test behavior when Error.prepareStackTrace is called to check whether it's compliant, even if we find an existing Error.prepareStackTrace. That means we need to restore whatever we found there in the first place after the test if we don't end up applying our own (e.g. unsafe errors).

Furthermore I recommend that we go the extra mile, save the original Error.prepareStackTrace if it exists, and rely on it to generate the stack string (from the filtered stack) in our replacement, so that any source mapping that Node.js does is still applied.

erights self-assigned this Sep 12, 2025

erights requested a review from Copilot September 12, 2025 00:25

Copilot AI reviewed Sep 12, 2025

View reviewed changes

packages/ses/src/error/tame-error-constructor.js Show resolved Hide resolved

packages/ses/src/error/tame-error-constructor.js Show resolved Hide resolved

erights force-pushed the markm-zb-tighter-v8-error-sniff branch 2 times, most recently from 6c7e4b6 to 3630c30 Compare September 12, 2025 00:34

erights marked this pull request as ready for review September 12, 2025 00:57

erights requested review from kriskowal, mhofman and naugtur September 12, 2025 00:58

erights added debugging support taming compat tc39 tooling devex developer experience fidelity Pertaining to the fidelity of emulation of native Hardened JavaScript. metamask labels Sep 12, 2025

erights requested a review from Copilot September 12, 2025 01:04

Copilot AI reviewed Sep 12, 2025

View reviewed changes

naugtur force-pushed the markm-zb-tighter-v8-error-sniff branch 3 times, most recently from 41009a8 to 0fca5fc Compare September 15, 2025 14:09

naugtur reviewed Sep 15, 2025

View reviewed changes

mhofman reviewed Sep 15, 2025

View reviewed changes

erights force-pushed the markm-zb-tighter-v8-error-sniff branch from 0fca5fc to 6fb1c1c Compare September 16, 2025 06:56

erights requested a review from gibson042 September 16, 2025 08:16

erights force-pushed the markm-zb-tighter-v8-error-sniff branch 2 times, most recently from fb86608 to bd4fcbc Compare September 19, 2025 00:43

erights and others added 3 commits October 7, 2025 16:41

fix(ses): fix #2951 stronger sniffing for v8

d8e0b7f

test(ses): hermes test runner and multiple tests

909d91b

feat(ses): prepareStackTrace CallSite toString fallback for Hermes

bad2617

erights force-pushed the markm-zb-tighter-v8-error-sniff branch from bd4fcbc to bad2617 Compare October 7, 2025 23:41

erights requested a review from naugtur October 7, 2025 23:44

naugtur self-assigned this Oct 8, 2025

naugtur added lavamoat and removed metamask labels Oct 23, 2025

		@@ -0,0 +1,92 @@
		/* global test */
		test('known issue: CallSite implementation', () => {

fix(ses): fix #2951 stronger sniffing for v8 #2952

Are you sure you want to change the base?

fix(ses): fix #2951 stronger sniffing for v8 #2952

Uh oh!

Conversation

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

Uh oh!

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

naugtur commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naugtur commented Sep 15, 2025

Uh oh!

naugtur Sep 15, 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!

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!

naugtur Sep 17, 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!

erights commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erights commented Sep 15, 2025

Uh oh!

mhofman left a comment • edited by erights Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erights commented Oct 7, 2025

Uh oh!

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

Uh oh!

Uh oh!

mhofman commented Oct 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erights commented Sep 12, 2025 •

edited

Loading

naugtur commented Sep 15, 2025 •

edited

Loading

naugtur Sep 15, 2025 •

edited

Loading

naugtur Sep 17, 2025 •

edited

Loading

mhofman left a comment •

edited by erights

Loading

naugtur commented Oct 8, 2025 •

edited

Loading