From 81b86ac3c3d804c0542f63c1e50040ca468960d0 Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Thu, 24 Oct 2024 13:01:59 -0700 Subject: [PATCH 1/2] feat(ses): Lockdown reporting option Fixes #2608 --- packages/ses/NEWS.md | 18 +++ packages/ses/docs/lockdown.md | 49 ++++++++ packages/ses/src/enable-property-overrides.js | 7 +- packages/ses/src/lockdown.js | 26 ++++- packages/ses/src/permits-intrinsics.js | 41 +++---- packages/ses/src/reporting-types.d.ts | 13 +++ packages/ses/src/reporting.js | 106 ++++++++++++++++++ .../error/_lockdown-with-extra-intrinsics.js | 3 + .../error/_prepare-with-extra-intrinsics.js | 25 +++++ .../permit-removal-warnings-node.test.js | 56 +++++++++ .../error/permit-removal-warnings.test.js | 30 +---- packages/ses/types.d.ts | 1 + 12 files changed, 316 insertions(+), 59 deletions(-) create mode 100644 packages/ses/src/reporting-types.d.ts create mode 100644 packages/ses/src/reporting.js create mode 100644 packages/ses/test/error/_lockdown-with-extra-intrinsics.js create mode 100644 packages/ses/test/error/_prepare-with-extra-intrinsics.js create mode 100644 packages/ses/test/error/permit-removal-warnings-node.test.js diff --git a/packages/ses/NEWS.md b/packages/ses/NEWS.md index 5b08016f5b..705099c771 100644 --- a/packages/ses/NEWS.md +++ b/packages/ses/NEWS.md @@ -5,6 +5,24 @@ User-visible changes in `ses`: - Permit [Promise.try](https://github.com/tc39/proposal-promise-try), since it has reached Stage 4. +- Adds a `reporting` option to `lockdown` and `repairIntrinsics`. + + The default behavior is `"platform"` which will detect the platform and + report warnings according to whether a web `console`, Node.js `console`, or + `print` are available. + The web platform is distinguished by the existence of `window` or + `importScripts` (WebWorker). + The Node.js behavior is to report all warnings to `stderr` visually + consistent with use of a console group. + SES will use `print` in the absence of a `console`. + Captures the platform `console` at the time `lockdown` or `repairIntrinsics` + are called, not at the time `ses` initializes. + + The `"console"` option forces the web platform behavior. + On Node.js, this results in group labels being reported to `stdout`. + + The `"none"` option mutes warnings. + # v1.9.0 (2024-10-10) - On platforms without diff --git a/packages/ses/docs/lockdown.md b/packages/ses/docs/lockdown.md index a305f12a2f..94a53b03b2 100644 --- a/packages/ses/docs/lockdown.md +++ b/packages/ses/docs/lockdown.md @@ -27,6 +27,7 @@ Each option is explained in its own section below. | `consoleTaming` | `'safe'` | `'unsafe'` | deep stacks ([details](#consoletaming-options)) | | `errorTaming` | `'safe'` | `'unsafe'` `'unsafe-debug'` | `errorInstance.stack` ([details](#errortaming-options)) | | `errorTrapping` | `'platform'` | `'exit'` `'abort'` `'report'` `'none'` | handling of uncaught exceptions ([details](#errortrapping-options)) | +| `reporting` | `'platform'` | `'console'` `'none'` | where to report warnings ([details](#reporting-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)) | | `stackFiltering` | `'concise'` | `'verbose'` | deep stacks signal/noise ([details](#stackfiltering-options)) | @@ -47,6 +48,7 @@ for threading environment variables into a JavaScript program. | `consoleTaming` | `LOCKDOWN_CONSOLE_TAMING` | | | `errorTaming` | `LOCKDOWN_ERROR_TAMING` | | | `errorTrapping` | `LOCKDOWN_ERROR_TRAPPING` | | +| `reporting` | `LOCKDOWN_REPORTING` | | | `unhandledRejectionTrapping` | `LOCKDOWN_UNHANDLED_REJECTION_TRAPPING` | | | `evalTaming` | `LOCKDOWN_EVAL_TAMING` | | | `stackFiltering` | `LOCKDOWN_STACK_FILTERING` | | @@ -459,6 +461,53 @@ the container to exit explicitly, and we highly recommend setting - `'none'`: do not install traps for uncaught exceptions. Errors are likely to appear as `{}` when they are reported by the default trap. +## `reporting` Options + +**Background**: Lockdown and `repairIntrinsics` report warnings if they +encounter unexpected but repairable variations on the shared intrinsics, which +regularly occurs if the version of `ses` predates the introduction of new +language features. +With the `reporting` option, an application can mute or control the direction +of these warnings. + +```js +lockdown(); // reporting defaults to 'platform' +// or +lockdown({ reporting: 'platform' }); +// vs +lockdown({ reporting: 'console' }); +// vs +lockdown({ reporting: 'none' }); +``` + +If `lockdown` does not receive an `reporting` option, it will respect +`process.env.LOCKDOWN_REPORTING`. + +```console +LOCKDOWN_REPORTING=platform +LOCKDOWN_REPORTING=console +LOCKDOWN_REPORTING=none +``` + +- The default behavior is `'platform'` which will detect the platform and + report warnings according to whether a web `console`, Node.js `console`, or + `print` are available. + The web platform is distinguished by the existence of `window` or + `importScripts` (WebWorker). + The Node.js behavior is to report all warnings to `stderr` visually + consistent with use of a console group. + SES will use `print` in the absence of a `console`. + Captures the platform `console` at the time `lockdown` or `repairIntrinsics` + are called, not at the time `ses` initializes. +- The `'console'` option forces the web platform behavior. + On Node.js, this results in group labels being reported to `stdout`. + The global `console` can be replaced before `lockdown` so using this option + will drive use of `console.groupCollapsed`, `console.groupEnd`, + `console.warn`, and `console.error` assuming that console is suited for + reporting arbitrary diagnostics rather than also being suited to generate + machine-readable `stdout`. +- The `'none'` option mutes warnings. + ## `unhandledRejectionTrapping` Options **Background**: Same concerns as `errorTrapping`, but in addition, SES will diff --git a/packages/ses/src/enable-property-overrides.js b/packages/ses/src/enable-property-overrides.js index 06d6563186..454c1f64df 100644 --- a/packages/ses/src/enable-property-overrides.js +++ b/packages/ses/src/enable-property-overrides.js @@ -23,6 +23,8 @@ import { severeEnablements, } from './enablements.js'; +/** @import {Reporter} from './reporting-types.js' */ + /** * For a special set of properties defined in the `enablement` whitelist, * `enablePropertyOverrides` ensures that the effect of freezing does not @@ -75,11 +77,13 @@ import { * * @param {Record} intrinsics * @param {'min' | 'moderate' | 'severe'} overrideTaming + * @param {Reporter} reporter * @param {Iterable} [overrideDebug] */ export default function enablePropertyOverrides( intrinsics, overrideTaming, + { warn }, overrideDebug = [], ) { const debugProperties = new Set(overrideDebug); @@ -109,8 +113,7 @@ export default function enablePropertyOverrides( this[prop] = newValue; } else { if (isDebug) { - // eslint-disable-next-line @endo/no-polymorphic-call - console.error(TypeError(`Override property ${prop}`)); + warn(TypeError(`Override property ${prop}`)); } defineProperty(this, prop, { value: newValue, diff --git a/packages/ses/src/lockdown.js b/packages/ses/src/lockdown.js index 56f5bcad0d..1200000737 100644 --- a/packages/ses/src/lockdown.js +++ b/packages/ses/src/lockdown.js @@ -58,6 +58,7 @@ import { tameSymbolConstructor } from './tame-symbol-constructor.js'; import { tameFauxDataProperties } from './tame-faux-data-properties.js'; import { tameRegeneratorRuntime } from './tame-regenerator-runtime.js'; import { shimArrayBufferTransfer } from './shim-arraybuffer-transfer.js'; +import { reportInGroup, chooseReporter } from './reporting.js'; /** @import {LockdownOptions} from '../types.js' */ @@ -165,6 +166,9 @@ export const repairIntrinsics = (options = {}) => { errorTrapping = /** @type {"platform" | "none" | "report" | "abort" | "exit" | undefined} */ ( getenv('LOCKDOWN_ERROR_TRAPPING', 'platform') ), + reporting = /** @type {"platform" | "console" | "none"} */ ( + getenv('LOCKDOWN_REPORTING', 'platform') + ), unhandledRejectionTrapping = /** @type {"none" | "report" | undefined} */ ( getenv('LOCKDOWN_UNHANDLED_REJECTION_TRAPPING', 'report') ), @@ -208,6 +212,8 @@ export const repairIntrinsics = (options = {}) => { extraOptionsNames.length === 0 || Fail`lockdown(): non supported option ${q(extraOptionsNames)}`; + const reporter = chooseReporter(reporting); + priorRepairIntrinsics === undefined || // eslint-disable-next-line @endo/no-polymorphic-call assert.fail( @@ -363,7 +369,16 @@ export const repairIntrinsics = (options = {}) => { // Remove non-standard properties. // All remaining function encountered during whitelisting are // branded as honorary native functions. - whitelistIntrinsics(intrinsics, markVirtualizedNativeFunction); + reportInGroup( + 'SES Removing unpermitted intrinsics', + reporter, + groupReporter => + whitelistIntrinsics( + intrinsics, + markVirtualizedNativeFunction, + groupReporter, + ), + ); // Initialize the powerful initial global, i.e., the global of the // start compartment, from the intrinsics. @@ -425,7 +440,14 @@ export const repairIntrinsics = (options = {}) => { // therefore before vetted shims rather than afterwards. It is not // clear yet which is better. // @ts-ignore enablePropertyOverrides does its own input validation - enablePropertyOverrides(intrinsics, overrideTaming, overrideDebug); + reportInGroup('SES Enabling property overrides', reporter, groupReporter => + enablePropertyOverrides( + intrinsics, + overrideTaming, + groupReporter, + overrideDebug, + ), + ); if (legacyRegeneratorRuntimeTaming === 'unsafe-ignore') { tameRegeneratorRuntime(); } diff --git a/packages/ses/src/permits-intrinsics.js b/packages/ses/src/permits-intrinsics.js index fc13c61718..2c55f2434f 100644 --- a/packages/ses/src/permits-intrinsics.js +++ b/packages/ses/src/permits-intrinsics.js @@ -62,6 +62,10 @@ import { symbolKeyFor, } from './commons.js'; +/** + * @import {Reporter} from './reporting-types.js' + */ + /** * whitelistIntrinsics() * Removes all non-allowed properties found by recursively and @@ -69,22 +73,13 @@ import { * * @param {object} intrinsics * @param {(object) => void} markVirtualizedNativeFunction + * @param {Reporter} reporter */ export default function whitelistIntrinsics( intrinsics, markVirtualizedNativeFunction, + { warn, error }, ) { - let groupStarted = false; - const inConsoleGroup = (level, ...args) => { - if (!groupStarted) { - // eslint-disable-next-line @endo/no-polymorphic-call - console.groupCollapsed('Removing unpermitted intrinsics'); - groupStarted = true; - } - // eslint-disable-next-line @endo/no-polymorphic-call - return console[level](...args); - }; - // These primitives are allowed for permits. const primitives = ['undefined', 'boolean', 'number', 'string', 'symbol']; @@ -294,7 +289,7 @@ export default function whitelistIntrinsics( // that we are removing it so we know to look into it, as happens when // the language evolves new features to existing intrinsics. if (subPermit !== false) { - inConsoleGroup('warn', `Removing ${subPath}`); + warn(`Removing ${subPath}`); } try { delete obj[prop]; @@ -303,17 +298,14 @@ export default function whitelistIntrinsics( if (typeof obj === 'function' && prop === 'prototype') { obj.prototype = undefined; if (obj.prototype === undefined) { - inConsoleGroup( - 'warn', - `Tolerating undeletable ${subPath} === undefined`, - ); + warn(`Tolerating undeletable ${subPath} === undefined`); // eslint-disable-next-line no-continue continue; } } - inConsoleGroup('error', `failed to delete ${subPath}`, err); + error(`failed to delete ${subPath}`, err); } else { - inConsoleGroup('error', `deleting ${subPath} threw`, err); + error(`deleting ${subPath} threw`, err); } throw err; } @@ -321,14 +313,7 @@ export default function whitelistIntrinsics( } } - try { - // Start path with 'intrinsics' to clarify that properties are not - // removed from the global object by the whitelisting operation. - visitProperties('intrinsics', intrinsics, permitted); - } finally { - if (groupStarted) { - // eslint-disable-next-line @endo/no-polymorphic-call - console.groupEnd(); - } - } + // Start path with 'intrinsics' to clarify that properties are not + // removed from the global object by the whitelisting operation. + visitProperties('intrinsics', intrinsics, permitted); } diff --git a/packages/ses/src/reporting-types.d.ts b/packages/ses/src/reporting-types.d.ts new file mode 100644 index 0000000000..e1748ec521 --- /dev/null +++ b/packages/ses/src/reporting-types.d.ts @@ -0,0 +1,13 @@ +/* eslint-disable no-restricted-globals */ + +export type Reporter = { + warn: (...message: Array) => void; + error: (...message: Array) => void; +}; + +export type GroupReporter = Reporter & { + groupCollapsed: (label: string) => void; + groupEnd: () => void; +}; + +// Console implements GroupReporter diff --git a/packages/ses/src/reporting.js b/packages/ses/src/reporting.js new file mode 100644 index 0000000000..53626e8eef --- /dev/null +++ b/packages/ses/src/reporting.js @@ -0,0 +1,106 @@ +import { TypeError, functionBind, globalThis } from './commons.js'; +import { assert } from './error/assert.js'; + +/** + * @import {Reporter, GroupReporter} from './reporting-types.js' + */ + +/** + * Creates a suitable reporter for internal errors and warnings out of the + * Node.js console.error to ensure all messages to go stderr, including the + * group label. + * Accounts for the extra space introduced by console.error as a delimiter + * between the indent and subsequent arguments. + * + * @param {(...message: Array) => void} print + */ +const makeReportPrinter = print => { + let indent = false; + /** @param {Array} args */ + const printIndent = (...args) => { + if (indent) { + print(' ', ...args); + } else { + print(...args); + } + }; + return /** @type {GroupReporter} */ ({ + warn(...args) { + printIndent(...args); + }, + error(...args) { + printIndent(...args); + }, + groupCollapsed(...args) { + assert(!indent); + print(...args); + indent = true; + }, + groupEnd() { + indent = false; + }, + }); +}; + +const mute = () => {}; + +/** + * @param {"platform" | "console" | "none"} reporting + */ +export const chooseReporter = reporting => { + if (reporting === 'none') { + return makeReportPrinter(mute); + } + if (reporting !== 'platform' && reporting !== 'console') { + throw new TypeError(`Invalid lockdown reporting option: ${reporting}`); + } + if ( + reporting === 'console' || + globalThis.window === globalThis || + globalThis.importScripts !== undefined + ) { + return console; + } + if (globalThis.console !== undefined) { + // On Node.js, we send all feedback to stderr, regardless of purported level. + const console = globalThis.console; + const error = functionBind(console.error, console); + return makeReportPrinter(error); + } + if (globalThis.print !== undefined) { + return makeReportPrinter(globalThis.print); + } + return makeReportPrinter(mute); +}; + +/** + * @param {string} groupLabel + * @param {GroupReporter} console + * @param {(internalConsole: Reporter) => void} callback + */ +export const reportInGroup = (groupLabel, console, callback) => { + const { warn, error, groupCollapsed, groupEnd } = console; + let groupStarted = false; + try { + return callback({ + warn(...args) { + if (!groupStarted) { + groupCollapsed(groupLabel); + groupStarted = true; + } + warn(...args); + }, + error(...args) { + if (!groupStarted) { + groupCollapsed(groupLabel); + groupStarted = true; + } + error(...args); + }, + }); + } finally { + if (groupStarted) { + groupEnd(); + } + } +}; diff --git a/packages/ses/test/error/_lockdown-with-extra-intrinsics.js b/packages/ses/test/error/_lockdown-with-extra-intrinsics.js new file mode 100644 index 0000000000..1887664272 --- /dev/null +++ b/packages/ses/test/error/_lockdown-with-extra-intrinsics.js @@ -0,0 +1,3 @@ +import './_prepare-with-extra-intrinsics.js'; + +lockdown(); diff --git a/packages/ses/test/error/_prepare-with-extra-intrinsics.js b/packages/ses/test/error/_prepare-with-extra-intrinsics.js new file mode 100644 index 0000000000..e65a32c99b --- /dev/null +++ b/packages/ses/test/error/_prepare-with-extra-intrinsics.js @@ -0,0 +1,25 @@ +import '../../index.js'; + +const { defineProperties } = Object; +const { apply } = Reflect; + +const originalIsArray = Array.isArray; + +defineProperties(Array, { + extraRemovableDataProperty: { + value: 'extra removable data property', + configurable: true, + }, + isArray: { + value: function isArrayWithCleanablePrototype(...args) { + return apply(originalIsArray, this, args); + }, + }, + // To ensure that the test below remains tolerant of future engines + // adding unexpected properties, causing extra warnings on removal. + // See https://github.com/endojs/endo/issues/1973 + anotherOne: { + value: `another removable property`, + configurable: true, + }, +}); diff --git a/packages/ses/test/error/permit-removal-warnings-node.test.js b/packages/ses/test/error/permit-removal-warnings-node.test.js new file mode 100644 index 0000000000..b0901c8638 --- /dev/null +++ b/packages/ses/test/error/permit-removal-warnings-node.test.js @@ -0,0 +1,56 @@ +/* global Buffer */ +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']; + +test('node reporting to stderr with indented group', async t => { + const child = spawn('node', ['_lockdown-with-extra-intrinsics.js'], { + cwd, + stdio, + }); + const stdoutChunks = []; + const stderrChunks = []; + child.stdout.on('data', chunk => { + stdoutChunks.push(chunk); + }); + child.stderr.on('data', chunk => { + stderrChunks.push(chunk); + }); + await new Promise((resolve, reject) => { + child.on('close', actualCode => { + try { + t.is(actualCode, 0); + resolve(true); + } catch (error) { + reject(error); + } + }); + }); + + // Nothing written to stdout + t.deepEqual(Buffer.concat(stdoutChunks), Buffer.alloc(0)); + + const stderrBytes = Buffer.concat(stderrChunks); + const stderrText = new TextDecoder().decode(stderrBytes); + const stderrLines = stderrText.trim().split('\n'); + + // Group label for removing unpermitted intrinsics + t.is(stderrLines.shift(), 'SES Removing unpermitted intrinsics'); + // And all remaining lines have exactly a two space indent + t.assert(stderrLines.every(line => /^\s{2}\w/.test(line))); + + const expectedLines = [ + ' Removing intrinsics.Array.isArray.prototype', + ' Tolerating undeletable intrinsics.Array.isArray.prototype === undefined', + ' Removing intrinsics.Array.extraRemovableDataProperty', + ' Removing intrinsics.Array.anotherOne', + ]; + + for (const expectedLine of expectedLines) { + t.assert(stderrLines.some(line => line === expectedLine)); + } +}); diff --git a/packages/ses/test/error/permit-removal-warnings.test.js b/packages/ses/test/error/permit-removal-warnings.test.js index 16f1acc496..cfd0b5e691 100644 --- a/packages/ses/test/error/permit-removal-warnings.test.js +++ b/packages/ses/test/error/permit-removal-warnings.test.js @@ -1,31 +1,7 @@ import test from 'ava'; -import '../../index.js'; +import './_prepare-with-extra-intrinsics.js'; import { assertLogs } from './_throws-and-logs.js'; -const { defineProperties } = Object; -const { apply } = Reflect; - -const originalIsArray = Array.isArray; - -defineProperties(Array, { - extraRemovableDataProperty: { - value: 'extra removable data property', - configurable: true, - }, - isArray: { - value: function isArrayWithCleanablePrototype(...args) { - return apply(originalIsArray, this, args); - }, - }, - // To ensure that the test below remains tolerant of future engines - // adding unexpected properties, causing extra warnings on removal. - // See https://github.com/endojs/endo/issues/1973 - anotherOne: { - value: `another removable property`, - configurable: true, - }, -}); - const logRecordMatches = (logRecord, goldenRecord) => Array.isArray(logRecord) && Array.isArray(goldenRecord) && @@ -73,9 +49,9 @@ const compareLogs = (t, log, goldenLog) => { test('permit removal warnings', t => { assertLogs( t, - () => lockdown(), + () => lockdown({ reporting: 'console' }), [ - ['groupCollapsed', 'Removing unpermitted intrinsics'], + ['groupCollapsed', 'SES Removing unpermitted intrinsics'], ['warn', 'Removing intrinsics.Array.isArray.prototype'], [ 'warn', diff --git a/packages/ses/types.d.ts b/packages/ses/types.d.ts index 1cd2f57702..2486b3338b 100644 --- a/packages/ses/types.d.ts +++ b/packages/ses/types.d.ts @@ -24,6 +24,7 @@ export interface RepairOptions { localeTaming?: 'safe' | 'unsafe'; consoleTaming?: 'safe' | 'unsafe'; errorTrapping?: 'platform' | 'exit' | 'abort' | 'report' | 'none'; + reporting?: 'platform' | 'console' | 'none'; unhandledRejectionTrapping?: 'report' | 'none'; errorTaming?: 'safe' | 'unsafe' | 'unsafe-debug'; dateTaming?: 'safe' | 'unsafe'; // deprecated From cf7f299e4f9dc0ea2ed271ecb91b66cf2792b1ed Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Thu, 24 Oct 2024 13:15:04 -0700 Subject: [PATCH 2/2] fix(ses): Clarify lockdown types This got caught in type fixes for above, but generally, getenv with a specified default cannot produce undefined. --- packages/ses/src/lockdown.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/ses/src/lockdown.js b/packages/ses/src/lockdown.js index 1200000737..7c4a1b2d11 100644 --- a/packages/ses/src/lockdown.js +++ b/packages/ses/src/lockdown.js @@ -163,22 +163,24 @@ export const repairIntrinsics = (options = {}) => { const { errorTaming = getenv('LOCKDOWN_ERROR_TAMING', 'safe'), - errorTrapping = /** @type {"platform" | "none" | "report" | "abort" | "exit" | undefined} */ ( + errorTrapping = /** @type {"platform" | "none" | "report" | "abort" | "exit"} */ ( getenv('LOCKDOWN_ERROR_TRAPPING', 'platform') ), reporting = /** @type {"platform" | "console" | "none"} */ ( getenv('LOCKDOWN_REPORTING', 'platform') ), - unhandledRejectionTrapping = /** @type {"none" | "report" | undefined} */ ( + unhandledRejectionTrapping = /** @type {"none" | "report"} */ ( getenv('LOCKDOWN_UNHANDLED_REJECTION_TRAPPING', 'report') ), regExpTaming = getenv('LOCKDOWN_REGEXP_TAMING', 'safe'), localeTaming = getenv('LOCKDOWN_LOCALE_TAMING', 'safe'), - consoleTaming = /** @type {'unsafe' | 'safe' | undefined} */ ( + consoleTaming = /** @type {'unsafe' | 'safe'} */ ( getenv('LOCKDOWN_CONSOLE_TAMING', 'safe') ), - overrideTaming = getenv('LOCKDOWN_OVERRIDE_TAMING', 'moderate'), + overrideTaming = /** @type {'moderate' | 'min' | 'severe'} */ ( + getenv('LOCKDOWN_OVERRIDE_TAMING', 'moderate') + ), stackFiltering = getenv('LOCKDOWN_STACK_FILTERING', 'concise'), domainTaming = getenv('LOCKDOWN_DOMAIN_TAMING', 'safe'), evalTaming = getenv('LOCKDOWN_EVAL_TAMING', 'safeEval'),