fix(ses): tolerate frozen null arguments/caller pills on API functions

.changeset/frozen-null-pills.md

@@ -0,0 +1,8 @@
+---
+'ses': patch
+---
+
+`lockdown()` now tolerates an undeletable `arguments` or `caller` data property on an API function when it is a frozen `null` (non-writable, non-configurable), with a warning.
+This matches how it already tolerates an undeletable `prototype` that can be set to `undefined`.
+Chromium's V8 through at least 133, as shipped in the Android System WebView, puts such poison pills on every Web IDL function, and lockdown previously failed there.
+Any other value, or a writable or configurable pill, still fails lockdown.

packages/ses/src/cauterize-property.js

@@ -1,4 +1,4 @@
-import { hasOwn } from './commons.js';
+import { getOwnPropertyDescriptor, hasOwn } from './commons.js';
 
 /**
  * @import {Reporter} from './reporting-types.js'
@@ -61,6 +61,26 @@ export const cauterizeProperty = (
           return;
         }
       }
+      if (
+        typeof obj === 'function' &&
+        (prop === 'arguments' || prop === 'caller')
+      ) {
+        // Some engines (Chromium's V8 through at least 133, as shipped in the
+        // Android System WebView) give every API function own, frozen
+        // `arguments: null` and `caller: null` poison pills. They cannot be
+        // deleted, and they leak nothing: a frozen `null` is inert. Tolerate
+        // exactly that shape; anything else still fails lockdown.
+        const descriptor = getOwnPropertyDescriptor(obj, prop);
+        if (
+          descriptor !== undefined &&
+          descriptor.value === null &&
+          descriptor.writable === false &&
+          descriptor.configurable === false
+        ) {
+          warn(`Tolerating undeletable ${subPath} === null`);
+          return;
+        }
+      }
       error(`failed to delete ${subPath}`, reason);
     } else {
       error(`deleting ${subPath} threw`, reason);

packages/ses/test/error/_lockdown-with-frozen-null-pills.js

@@ -0,0 +1,3 @@
+import './_prepare-with-frozen-null-pills.js';
+
+lockdown();

packages/ses/test/error/_prepare-with-frozen-null-pills.js

@@ -0,0 +1,44 @@
+import '../../index.js';
+
+// Plant own, undeletable `arguments` and `caller` properties on an intrinsic
+// function that lockdown's property removal visits, imitating the poison
+// pills that Chromium's V8 (through at least 133, as shipped in the Android
+// System WebView) puts on every Web IDL function.
+//
+// The shape is chosen by `globalThis.pillShape`, or by the first command-line
+// argument when run as a child process:
+// - 'frozen-null': non-writable, non-configurable `null` (tolerated)
+// - 'frozen-zero': non-writable, non-configurable `0` (must still fail)
+// - 'writable-null': writable, non-configurable `null` (must still fail)
+
+const { defineProperties } = Object;
+const { apply } = Reflect;
+
+const pillShape =
+  globalThis.pillShape ||
+  (typeof process !== 'undefined' && process.argv[2]) ||
+  'frozen-null';
+
+const pillDescriptors = {
+  'frozen-null': { value: null, writable: false },
+  'frozen-zero': { value: 0, writable: false },
+  'writable-null': { value: null, writable: true },
+};
+const pillDescriptor = pillDescriptors[pillShape];
+if (pillDescriptor === undefined) {
+  throw Error(`unknown pill shape ${pillShape}`);
+}
+
+const originalIsArray = Array.isArray;
+
+// An arrow function has no own `arguments` or `caller`, so these
+// definitions add them, like the engine does.
+const isArrayWithPills = (...args) => apply(originalIsArray, Array, args);
+defineProperties(isArrayWithPills, {
+  arguments: { ...pillDescriptor, enumerable: false, configurable: false },
+  caller: { ...pillDescriptor, enumerable: false, configurable: false },
+});
+
+defineProperties(Array, {
+  isArray: { value: isArrayWithPills },
+});

packages/ses/test/error/frozen-null-pill-warnings-node.test.js

@@ -0,0 +1,59 @@
+// @ts-nocheck
+import test from 'ava';
+import url from 'url';
+import { spawn } from 'child_process';
+
+const cwd = url.fileURLToPath(new URL('./', import.meta.url));
+
+const stdio = ['ignore', 'pipe', 'pipe'];
+
+const runLockdown = shape =>
+  new Promise((resolve, reject) => {
+    const child = spawn(
+      'node',
+      ['_lockdown-with-frozen-null-pills.js', shape],
+      { cwd, stdio },
+    );
+    const stdoutChunks = [];
+    const stderrChunks = [];
+    child.stdout.on('data', chunk => stdoutChunks.push(chunk));
+    child.stderr.on('data', chunk => stderrChunks.push(chunk));
+    child.on('error', reject);
+    child.on('close', code => {
+      const decode = chunks => new TextDecoder().decode(Buffer.concat(chunks));
+      resolve({
+        code,
+        stdout: decode(stdoutChunks),
+        stderr: decode(stderrChunks),
+      });
+    });
+  });
+
+test('frozen null pills: lockdown succeeds and warns', async t => {
+  const { code, stdout, stderr } = await runLockdown('frozen-null');
+  t.is(code, 0, stderr);
+  t.is(stdout, '');
+  const lines = stderr.split('\n');
+  for (const expectedLine of [
+    '  Removing intrinsics.Array.isArray.arguments',
+    '  Tolerating undeletable intrinsics.Array.isArray.arguments === null',
+    '  Removing intrinsics.Array.isArray.caller',
+    '  Tolerating undeletable intrinsics.Array.isArray.caller === null',
+  ]) {
+    t.true(lines.includes(expectedLine), `missing ${expectedLine}`);
+  }
+});
+
+test('frozen non-null pill: lockdown still fails', async t => {
+  const { code, stderr } = await runLockdown('frozen-zero');
+  t.not(code, 0);
+  t.regex(stderr, /failed to delete intrinsics\.Array\.isArray\.arguments/);
+  t.notRegex(stderr, /Tolerating undeletable/);
+});
+
+test('writable null pill: lockdown still fails', async t => {
+  const { code, stderr } = await runLockdown('writable-null');
+  t.not(code, 0);
+  t.regex(stderr, /failed to delete intrinsics\.Array\.isArray\.arguments/);
+  t.notRegex(stderr, /Tolerating undeletable/);
+});

packages/ses/test/error/frozen-null-pill-warnings.test.js

@@ -0,0 +1,53 @@
+import test from 'ava';
+import './_prepare-with-frozen-null-pills.js';
+import { assertLogs } from './_throws-and-logs.js';
+
+/** @import { ExecutionContext } from 'ava' */
+
+/**
+ * Test that every golden record appears in the log, in order, allowing
+ * unrelated records in between, since engines may add other properties
+ * that lockdown removes with a warning.
+ * See https://github.com/endojs/endo/issues/1973
+ *
+ * @param {ExecutionContext} t
+ * @param {any[][]} log
+ * @param {any[][]} goldenLog
+ */
+const compareLogs = (t, log, goldenLog) => {
+  let i = 0;
+  for (const goldenRecord of goldenLog) {
+    const golden = JSON.stringify(goldenRecord);
+    while (i < log.length && JSON.stringify(log[i]) !== golden) {
+      i += 1;
+    }
+    t.true(i < log.length, `missing log record ${golden}`);
+    i += 1;
+  }
+};
+
+test('frozen null arguments/caller pills are tolerated with a warning', t => {
+  assertLogs(
+    t,
+    () => lockdown({ reporting: 'console' }),
+    [
+      ['groupCollapsed', 'SES Removing unpermitted intrinsics'],
+      ['warn', 'Removing intrinsics.Array.isArray.arguments'],
+      [
+        'warn',
+        'Tolerating undeletable intrinsics.Array.isArray.arguments === null',
+      ],
+      ['warn', 'Removing intrinsics.Array.isArray.caller'],
+      [
+        'warn',
+        'Tolerating undeletable intrinsics.Array.isArray.caller === null',
+      ],
+      ['groupEnd'],
+    ],
+    { compareLogs },
+  );
+  // The pills survive lockdown, still inert.
+  t.is(Array.isArray.arguments, null);
+  t.is(/** @type {unknown} */ (Array.isArray.caller), null);
+  t.true(Array.isArray([]));
+});