fix(ses): work around #2348 linenumber bug

packages/ses/NEWS.md

@@ -4,6 +4,25 @@ User-visible changes in SES:
 
 - Adds support for module descriptors better aligned with XS.
 
+- When running Ava tests on Node written in TypeScript, the SES error taming
+  gives line-numbers into the generated JavaScript, in which all compiled code
+  is on line 1. 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
+  ```
+  In addition to the normal `'safe'` and `'unsafe'` settings, 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 Ava tests written in TypeScript on Node,
+  the stacktrace line-numbers should point back into the original
+  TypeScript source, as they would 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 TypeScript 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 TypeScript 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';