fix(ses): work around #2348 linenumber bug (#2355)

Closes: #2348 

Refs: Agoric/agoric-sdk#9711
#1798
#1799
Agoric/agoric-sdk#8662
Agoric/agoric-sdk#9700

## Description

Prior to this PR, when you ran on Ava on Node a test written in
TypeScript, you'd see something like the following in your stack traces.

```
  boot/test/bootstrapTests/stack-linenumbers.test.ts:1:104
```

This is because the TypeScript compiler compiles a TypeScripy file into
one line
of JavaScript with a sourceMap that should map back into original
source positions. Node specifically makes use of that sourceMap
to produce original line-numbers. However, Node does this in a way
that resists virtualization, so the normal SES error taming cannot use
this sourceMap info.

By default, this PR does not change this behavior. However it recognizes
a new `SUPPRESS_NODE_ERROR_TAMING` environment variable.

With the `SUPPRESS_NODE_ERROR_TAMING` environment variable absent
or set to `'disabled'`, you should still see stack traces as shown above

However, if you also set  the `SUPPRESS_NODE_ERROR_TAMING` environment
variable `'enabled'`, for example by doing

```sh
$ export SUPPRESS_NODE_ERROR_TAMING=enabled
```
at a bash shell, then when you run this test you should instead see
something like
```
boot/test/bootstrapTests/stack-linenumbers.test.ts:40:32
```

At Agoric/agoric-sdk#9711 I both 
- turn this PR into an agoric-sdk patch of endo, in order to emulate
this fix until the next endo-release-agoric-sdk-sync cycle, and
- add a test case that emits an error stack trace from an Ava test case
written in TypeScript, to test that it works.

### Security Considerations

This new behavior only applies when `errorTaming: 'unsafe'`, on v8, and
with this new environment variable enabled.

Setting `errorTaming: 'unsafe'` already flags to sacrifice some security
for a better debugging experience. But the loss of security is moderate
enough --- mostly confidentiality rather than integrity --- that some
may chose this setting for some production purposes.

The new behavior is a more severe loss of security that really should be
used ***only during development***, not production, when even a severe
loss of security is usually not an issue.

### Scaling Considerations

none
### Documentation Considerations

The behavior prior to this PR or without this environment variable
enabled is an unpleasant debugging experience. However, developers won't
know how to repair it, or even that it can be repaired, without
explanation. Even then, the difficultly of discovery in a problem.

The names `SUPPRESS_NODE_ERROR_TAMING` and the settings `'enabled'` and
`'disabled'` are by no means clear expressions of what this does.
Reviewers, ***better names would be appreciated!***

### Testing Considerations

The point. As developers write and run tests written in TypeScript, they
need to iterate with problems revealed by the tests, for which they need
good line numbers, including into the test code.

When the environment variable is enabled, the new behavior broke some
SES tests written specifically to test the old behavior. This would not
happen under CI because the environment variable is not set by default,
and so may not have been noticed. But it was revealed in local testing.
To repair this, this PR also sets those tests up to set
`process.env.SUPPRESS_NODE_ERROR_TAMING` to `'disabled'` before
lockdown, protecting those tests from the external environment variable
setting.

Awkwardly, at the moment Agoric/agoric-sdk#9711
serves as the only test of this PR. This is because I failed to figure
out how to configure things so I can run TypeScript tests under Ava,
like Agoric/agoric-sdk#9711 does. I tried cargo
culting the configs that seemed relevant, but it didn't work.

Reviewers, if you let me know how to do this, I'll duplicate the test
case from Agoric/agoric-sdk#9711 here, which
would be good.

### Compatibility Considerations

With the environment variable absent or disabled, there should be zero
difference in behavior, so none.

In a development environment where this environment variable is enabled,
some stack traces will be different. But outside of SES itself, nothing
should depend on the contents of stack traces, so again none.

### Upgrade Considerations

No upgrade considerations.

Nothing BREAKING.

- [x] Update `NEWS.md` for user-facing changes.

packages/ses/NEWS.md

@@ -4,6 +4,27 @@ User-visible changes in SES:
 
 - Adds support for module descriptors better aligned with XS.
 
+- When running transpiled code on Node, the SES error taming
+  gives line-numbers into the generated JavaScript, which often don't match the
+  the original lines. This happens even with the normal development-time
+  lockdown options setting,
+  ```js
+  errorTaming: 'unsafe'
+  ```
+  or setting the environment variable
+  ```sh
+  $ export LOCKDOWN_ERROR_TAMING=unsafe
+  ```
+  To get the original line numbers, this release
+  adds `'unsafe-debug'`. This `errorTaming: 'unsafe-debug'` setting
+  should be used ***during development only*** when you can
+  sacrifice more security for a better debugging experience, as explained at
+  [`errorTaming` Options](https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md#errortaming-options).
+  With this setting, when running transpiled code on Node (e.g. tests written
+  in TypeScript),
+  the stacktrace line-numbers point back into the original
+  source, as they do on Node without SES.
+
 # v1.5.0 (2024-05-06)
 
 - Adds `importNowHook` to the `Compartment` options. The compartment will invoke the hook whenever it encounters a missing dependency while running `compartmentInstance.importNow(specifier)`, which cannot use an asynchronous `importHook`.

packages/ses/docs/lockdown.md

@@ -25,7 +25,7 @@ Each option is explained in its own section below.
 | `regExpTaming`               | `'safe'`         | `'unsafe'`                             | `RegExp.prototype.compile` ([details](#regexptaming-options)) |
 | `localeTaming`               | `'safe'`         | `'unsafe'`                             | `toLocaleString`           ([details](#localetaming-options)) |
 | `consoleTaming`              | `'safe'`         | `'unsafe'`                             | deep stacks                ([details](#consoletaming-options)) |
-| `errorTaming`                | `'safe'`         | `'unsafe'`                             | `errorInstance.stack`      ([details](#errortaming-options)) |
+| `errorTaming`                | `'safe'`         | `'unsafe'` `'unsafe-debug'`            | `errorInstance.stack`      ([details](#errortaming-options)) |
 | `errorTrapping`              | `'platform'`     | `'exit'` `'abort'` `'report'` `'none'` | handling of uncaught exceptions ([details](#errortrapping-options)) |
 | `unhandledRejectionTrapping` | `'report'`       | `'none'`                               | handling of finalized unhandled rejections ([details](#unhandledrejectiontrapping-options)) |
 | `evalTaming`                 | `'safeEval'`     | `'unsafeEval'` `'noEval'`              | `eval` and `Function` of the start compartment ([details](#evaltaming-options)) |
@@ -293,27 +293,48 @@ default error behavior is much more dangerous. The v8 `Error` constructor
 provides a set of
 [static methods for accessing the raw stack
 information](https://v8.dev/docs/stack-trace-api) that are used to create
-error stack string. Some of this information is consistent with the level
+error stack strings. Some of this information is consistent with the level
 of disclosure provided by the proposed `getStack` special power above.
 Some go well beyond it.
 
-The `errorTaming` option of `lockdown` do not affect the safety of the `Error`
-constructor. In all cases, after calling `lockdown`, the tamed `Error`
+Neither the `'safe'` or `'unsafe'` settings of the `errorTaming`
+affect the safety of the `Error`
+constructor. In those cases, after calling `lockdown`, the tamed `Error`
 constructor in the start compartment follows ocap rules.
 Under v8 it emulates most of the
 magic powers of the v8 `Error` constructor—those consistent with the
 level of disclosure of the proposed `getStack`. In all cases, the `Error`
 constructor shared by all other compartments is both safe and powerless.
 
-See the [error README](../src/error/README.md) for an in depth explanation of the
-relationship between errors, `assert` and the virtual `console`.
+See the [error README](../src/error/README.md) for an in depth explanation of
+the relationship between errors, `assert` and the virtual `console`.
+
+When running TypeScript tests on Node without SES,
+you'll see accurate line numbers into the original TypeScript source.
+However, with SES with the `'safe'` or `'unsafe'` settings of
+`errorTaming` the stacks will show all TypeScript positions as line 1,
+which is the one line of JavaScript the TypeScript compiled to.
+We would like to fix this while preserving safety, but have not yet done so.
+
+Instead, we introduce the `'unsafe-debug'` setting, which sarifices
+more security to restore this pleasant Node behavior.
+Where `'safe'` and `'unsafe'` endangers only confidentiality, `'unsafe-debug'` also
+endangers intergrity. For development and debugging purposes ***only***,
+this is usually the right tradeoff. But please don't use this setting in a
+production environment.
+
+On non-v8 platforms, `'unsafe'` and `'unsafe-debug'` do the same thing, since
+[the problem](https://github.com/endojs/endo/issues/1798) is specific to
+Node on v8.
 
 ```js
 lockdown(); // errorTaming defaults to 'safe'
 // or
 lockdown({ errorTaming: 'safe' }); // Deny unprivileged access to stacks, if possible
 // vs
 lockdown({ errorTaming: 'unsafe' }); // stacks also available by errorInstance.stack
+// vs
+lockdown({ errorTaming: 'unsafe-debug' }); // sacrifice more safety for source-mapped line numbers.
 ```
 
 If `lockdown` does not receive an `errorTaming` option, it will respect

packages/ses/docs/reference.md

@@ -122,11 +122,15 @@ after `lockdown()`was called.
 
 ### Default `'safe'` settings
 
-All four of these safety-relevant options default to `'safe'` if omitted
+All three of these safety-relevant options default to `'safe'` if omitted
 from a call to `lockdown()`. Their other possible value is `'unsafe'`.
 - `regExpTaming`
 - `localeTaming`
 - `consoleTaming`
+
+In addition, `errorTaming` defaults to `'safe'` but can be set to `'unsafe'`
+or `'unsafe-debug'`, as explained at
+[`errorTaming` Options](https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md#errortaming-options).
 - `errorTaming`
 
 The tradeoff is safety vs compatibility with existing code. However, much legacy
@@ -167,9 +171,10 @@ below.
   </tr>
   <tr>
     <td><code>errorTaming</code></td>
-    <td><code>'safe'</code> (default) or <code>'unsafe'</code></td>
+    <td><code>'safe'</code> (default) or <code>'unsafe'</code> or <code>'unsafe-debug'</code></td>
     <td><code>'safe'</code> denies unprivileged stacks access,<br>
-        <code>'unsafe'</code> makes stacks also available by <code>errorInstance.stackkeeps()</code>.</td>
+        <code>'unsafe'</code> makes stacks also available by <code>errorInstance.stack</code>,<br>
+        <code>'unsafe-debug'</code> sacrifices more safety for better TypeScript line-numbers.</td>
   </tr>
   <tr>
     <td><code>stackFiltering</code></td>
@@ -179,9 +184,10 @@ below.
   </tr>
   <tr>
     <td><code>overrideTaming</code></td>
-    <td><code>'moderate'</code> (default) or <code>'min'</code></td>
+    <td><code>'moderate'</code> (default) or <code>'min'</code> or <code>'severe'</code></td>
     <td><code>'moderate'</code> moderates mitigations for legacy compatibility,<br>
-        <code>'min'</code> minimal mitigations for purely modern code</td>
+        <code>'min'</code> minimal mitigations for purely modern code, <br>
+        <code>'severe'</code> when moderate mitigations are inadequate</td>
   </tr>
   </tbody>
 </table>
@@ -258,18 +264,30 @@ In JavaScript the stack is only available via `err.stack`, so some
 development tools assume it is there. When the information leak is tolerable,
 the `'unsafe'` setting preserves `err.stack`'s filtered stack information.
 
-`errorTaming` does not affect the `Error` constructor's safety.
+The `'safe'` or `'unsafe'` settings of `errorTaming` do not affect the `Error`
+constructor's safety, beyond the confidentiality hazards mentioned above.
 After calling `lockdown`, the tamed `Error` constructor in the
 start compartment follows OCap rules. Under v8 it emulates most of the
 magic powers of the v8 `Error` constructor&mdash;those consistent with the
 discourse level of the proposed `getStack`. In all cases, the `Error`
 constructor shared by all other compartments is both safe and powerless.
+
+However, with the `'safe'` and `'unsafe'` settings, you'll often see
+line-numbers into TypeScript sources are always 1, since the TypeScript compiler
+compiles into a single line of JavaScript. For TypeScript on Node on v8,
+the setting `'unsafe-debug'` sacrifices more security to restore the
+normal Node behavior of providing accurate positions into the TypeScript source.
+The `'unsafe-debug'` setting should be used ***for development only***, when
+this is usually the right tradeoff. Please do not use it in production.
+
 ```js
 lockdown(); // errorTaming defaults to 'safe'
 // or
 lockdown({ errorTaming: 'safe' }); // Deny unprivileged access to stacks, if possible
 // vs
 lockdown({ errorTaming: 'unsafe' }); // Stacks also available by errorInstance.stack
+// vs
+lockdown({ errorTaming: 'unsafe-debug' }); // sacrifice more safety for source-mapped line numbers.
 ```
 
 ### `stackFiltering` Options
@@ -329,4 +347,3 @@ lockdown({ overrideTaming: 'moderate' }); // Moderate mitigations for legacy com
 // vs
 lockdown({ overrideTaming: 'min' }); // Minimal mitigations for purely modern code
 ```
-

packages/ses/src/error/README.md

@@ -58,7 +58,7 @@ All the above forms of diagnostic information—error stacks, detailed error
 
 To minimize visual noise, none of the following directly produces any logging output: throwing errors, assertion failure, or error annotation. They record information silently, only to be displayed if reached from an explicit `console` logging action. These logging actions are the root of a graph of this additional accumulated information. The logging action arguments include errors. These errors have both a detailed message that include errors, and detailed message annotations that include errors. Those errors likewise... The console logs this extra information once, with a unique tag per error. Any further occurrences of that error output that unique tag, from which to look up such previous log output.
 
-Before repair or `lockdown`, we assume there is some prior "system" console bound to the global `console` in the start compartment. `{ consoleTaming: 'unsafe' }` leaves this unsafe system console in place. This system console is completely ignorant of the extra information in these side tables, which is therefore never seen. This includes the hidden data in the detailed messages. Instead, the system console shows the abbreviated `message` text produced by the `details` template literal that omits censored data. When combined with the default `{ errorTaming: 'safe' }`, the system console may not see error stack information. The `{ errorTaming: 'unsafe' }` setting does not remove error stacks from error objects, in which case the system console will find it as usual.
+Before repair or `lockdown`, we assume there is some prior "system" console bound to the global `console` in the start compartment. `{ consoleTaming: 'unsafe' }` leaves this unsafe system console in place. This system console is completely ignorant of the extra information in these side tables, which is therefore never seen. This includes the hidden data in the detailed messages. Instead, the system console shows the abbreviated `message` text produced by the `details` template literal that omits censored data. When combined with the default `{ errorTaming: 'safe' }`, the system console may not see error stack information. The `{ errorTaming: 'unsafe' }` or `{ errorTaming: 'unsafe-details' }` setting does not remove error stacks from error objects, in which case the system console will find it as usual.
 
 The default `{ consoleTaming: 'safe' }` setting replaces the system console with a root console that does use all these side tables to generate a more informative log. This root console wraps the prior system console. This root console outputs its log information only by invoking this wrapped system console, which therefore determines how this log information is made available to the external world. To support determinism, we will also need to support a no-op console setting, as explained at [deterministic handling of adversarial code calling console.log with a Proxy #1852](https://github.com/Agoric/agoric-sdk/issues/1852) and [Need no-op console setting for determinism #487](https://github.com/Agoric/SES-shim/issues/487).
 

packages/ses/src/error/assert.js

@@ -150,7 +150,8 @@ freeze(DetailsTokenProto.toString);
 
 /**
  * Normally this is the function exported as `assert.details` and often
- * spelled `X`. However, if the `{errorTaming: 'unsafe'}` option is
+ * spelled `X`. However, if the `{errorTaming: 'unsafe'}` or
+ * `{errorTaming: 'unsafe-debug'}` option is
  * given to `lockdown`, then `unredactedDetails` is used instead.
  *
  * There are some unconditional uses of `redactedDetails` in this module. All
@@ -174,7 +175,8 @@ freeze(redactedDetails);
  * `unredactedDetails` is like `details` except that it does not redact
  * anything. It acts like `details` would act if all substitution values
  * were wrapped with the `quote` function above (the function normally
- * spelled `q`). If the `{errorTaming: 'unsafe'}` option is given to
+ * spelled `q`). If the `{errorTaming: 'unsafe'}`
+ * or `{errorTaming: 'unsafe-debug'}` option is given to
  * `lockdown`, then the lockdown-shim arranges for the global `assert` to be
  * one whose `details` property is `unredactedDetails`.
  * This setting optimizes the debugging and testing experience at the price

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

@@ -7,6 +7,7 @@ import {
   setPrototypeOf,
   getOwnPropertyDescriptor,
   defineProperty,
+  getOwnPropertyDescriptors,
 } from '../commons.js';
 import { NativeErrors } from '../permits.js';
 import { tameV8ErrorConstructor } from './tame-v8-error-constructor.js';
@@ -29,22 +30,27 @@ const tamedMethods = {
     return '';
   },
 };
+let initialGetStackString = tamedMethods.getStackString;
 
 export default function tameErrorConstructor(
   errorTaming = 'safe',
   stackFiltering = 'concise',
 ) {
-  if (errorTaming !== 'safe' && errorTaming !== 'unsafe') {
+  if (
+    errorTaming !== 'safe' &&
+    errorTaming !== 'unsafe' &&
+    errorTaming !== 'unsafe-debug'
+  ) {
     throw TypeError(`unrecognized errorTaming ${errorTaming}`);
   }
   if (stackFiltering !== 'concise' && stackFiltering !== 'verbose') {
     throw TypeError(`unrecognized stackFiltering ${stackFiltering}`);
   }
   const ErrorPrototype = FERAL_ERROR.prototype;
 
-  const platform =
-    typeof FERAL_ERROR.captureStackTrace === 'function' ? 'v8' : 'unknown';
   const { captureStackTrace: originalCaptureStackTrace } = FERAL_ERROR;
+  const platform =
+    typeof originalCaptureStackTrace === 'function' ? 'v8' : 'unknown';
 
   const makeErrorConstructor = (_ = {}) => {
     // eslint-disable-next-line no-shadow
@@ -122,6 +128,45 @@ export default function tameErrorConstructor(
     },
   });
 
+  if (errorTaming === 'unsafe-debug' && platform === 'v8') {
+    // This case is a kludge to work around
+    // https://github.com/endojs/endo/issues/1798
+    // https://github.com/endojs/endo/issues/2348
+    // https://github.com/Agoric/agoric-sdk/issues/8662
+
+    defineProperties(InitialError, {
+      prepareStackTrace: {
+        get() {
+          return FERAL_ERROR.prepareStackTrace;
+        },
+        set(newPrepareStackTrace) {
+          FERAL_ERROR.prepareStackTrace = newPrepareStackTrace;
+        },
+        enumerable: false,
+        configurable: true,
+      },
+      captureStackTrace: {
+        value: FERAL_ERROR.captureStackTrace,
+        writable: true,
+        enumerable: false,
+        configurable: true,
+      },
+    });
+
+    const descs = getOwnPropertyDescriptors(InitialError);
+    defineProperties(SharedError, {
+      stackTraceLimit: descs.stackTraceLimit,
+      prepareStackTrace: descs.prepareStackTrace,
+      captureStackTrace: descs.captureStackTrace,
+    });
+
+    return {
+      '%InitialGetStackString%': initialGetStackString,
+      '%InitialError%': InitialError,
+      '%SharedError%': SharedError,
+    };
+  }
+
   // The default SharedError much be completely powerless even on v8,
   // so the lenient `stackTraceLimit` accessor does nothing on all
   // platforms.
@@ -171,15 +216,14 @@ export default function tameErrorConstructor(
     });
   }
 
-  let initialGetStackString = tamedMethods.getStackString;
   if (platform === 'v8') {
     initialGetStackString = tameV8ErrorConstructor(
       FERAL_ERROR,
       InitialError,
       errorTaming,
       stackFiltering,
     );
-  } else if (errorTaming === 'unsafe') {
+  } else if (errorTaming === 'unsafe' || errorTaming === 'unsafe-debug') {
     // v8 has too much magic around their 'stack' own property for it to
     // coexist cleanly with this accessor. So only install it on non-v8
 

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

@@ -16,6 +16,7 @@ import {
   weakmapSet,
   weaksetAdd,
   weaksetHas,
+  TypeError,
 } from '../commons.js';
 
 // Whitelist names from https://v8.dev/docs/stack-trace-api
@@ -164,6 +165,11 @@ export const tameV8ErrorConstructor = (
   errorTaming,
   stackFiltering,
 ) => {
+  if (errorTaming === 'unsafe-debug') {
+    throw TypeError(
+      'internal: v8+unsafe-debug special case should already be done',
+    );
+  }
   // TODO: Proper CallSite types
   /** @typedef {{}} CallSite */
 

packages/ses/src/lockdown.js

@@ -298,7 +298,7 @@ export const repairIntrinsics = (options = {}) => {
    * @type {((error: any) => string | undefined) | undefined}
    */
   let optGetStackString;
-  if (errorTaming !== 'unsafe') {
+  if (errorTaming === 'safe') {
     optGetStackString = intrinsics['%InitialGetStackString%'];
   }
   const consoleRecord = tameConsole(
@@ -323,8 +323,12 @@ export const repairIntrinsics = (options = {}) => {
   }
 
   // @ts-ignore assert is absent on globalThis type def.
-  if (errorTaming === 'unsafe' && globalThis.assert === assert) {
-    // If errorTaming is 'unsafe' we replace the global assert with
+  if (
+    (errorTaming === 'unsafe' || errorTaming === 'unsafe-debug') &&
+    globalThis.assert === assert
+  ) {
+    // If errorTaming is 'unsafe' or 'unsafe-debug' we replace the
+    // global assert with
     // one whose `details` template literal tag does not redact
     // unmarked substitution values. IOW, it blabs information that
     // was supposed to be secret from callers, as an aid to debugging

packages/ses/test/error/tame-v8-error.test.js

@@ -1,7 +1,7 @@
 import test from 'ava';
 import '../../index.js';
 
-lockdown();
+lockdown({ errorTaming: 'safe' });
 
 test('lockdown Error is safe', t => {
   t.is(typeof Error.captureStackTrace, 'function');

packages/ses/types.d.ts

@@ -25,7 +25,7 @@ export interface RepairOptions {
   consoleTaming?: 'safe' | 'unsafe';
   errorTrapping?: 'platform' | 'exit' | 'abort' | 'report' | 'none';
   unhandledRejectionTrapping?: 'report' | 'none';
-  errorTaming?: 'safe' | 'unsafe';
+  errorTaming?: 'safe' | 'unsafe' | 'unsafe-debug';
   dateTaming?: 'safe' | 'unsafe'; // deprecated
   mathTaming?: 'safe' | 'unsafe'; // deprecated
   evalTaming?: 'safeEval' | 'unsafeEval' | 'noEval';