Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ses): Support an incomplete shimmed globalThis.process #1923

Merged
merged 4 commits into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 15 additions & 16 deletions packages/env-options/src/env-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,22 +61,21 @@ export const makeEnvironmentCaptor = aGlobal => {

/** @type {string} */
let setting = defaultSetting;
const globalProcess = aGlobal.process;
if (globalProcess && typeof globalProcess === 'object') {
const globalEnv = globalProcess.env;
if (globalEnv && typeof globalEnv === 'object') {
if (optionName in globalEnv) {
arrayPush(capturedEnvironmentOptionNames, optionName);
const optionValue = globalEnv[optionName];
// eslint-disable-next-line @endo/no-polymorphic-call
typeof optionValue === 'string' ||
Fail`Environment option named ${q(
optionName,
)}, if present, must have a corresponding string value, got ${q(
optionValue,
)}`;
setting = optionValue;
}
const globalProcess = aGlobal.process || undefined;
const globalEnv =
(typeof globalProcess === 'object' && globalProcess.env) || undefined;
if (typeof globalEnv === 'object') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this regresses if env === null. Unlikely, I’m sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't given the || undefined

if (optionName in globalEnv) {
arrayPush(capturedEnvironmentOptionNames, optionName);
const optionValue = globalEnv[optionName];
// eslint-disable-next-line @endo/no-polymorphic-call
typeof optionValue === 'string' ||
Fail`Environment option named ${q(
optionName,
)}, if present, must have a corresponding string value, got ${q(
optionValue,
)}`;
setting = optionValue;
}
}
return setting;
Expand Down
2 changes: 1 addition & 1 deletion packages/ses/src/error/fatal-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ let abandon;
// below). Currently it only checks for the `process.abort` or `process.exit`
// found on Node. It should also sniff for a vat terminating function expected
// to be found within the start compartment of SwingSet vats. What else?
if (typeof process === 'object') {
if (typeof process === 'object' && process) {
abandon = process.abort || process.exit;
}
let raise;
Expand Down
68 changes: 44 additions & 24 deletions packages/ses/src/error/tame-console.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// @ts-check

import {
// Using TypeError minimizes risk of exposing the feral Error constructor
TypeError,
apply,
defineProperty,
Expand All @@ -13,6 +14,10 @@ import { makeRejectionHandlers } from './unhandled-rejection.js';
import './types.js';
import './internal-types.js';

const failFast = message => {
throw TypeError(message);
};

const wrapLogger = (logger, thisArg) =>
freeze((...args) => apply(logger, thisArg, args));

Expand Down Expand Up @@ -60,9 +65,9 @@ export const tameConsole = (
unhandledRejectionTrapping = 'report',
optGetStackString = undefined,
) => {
if (consoleTaming !== 'safe' && consoleTaming !== 'unsafe') {
throw TypeError(`unrecognized consoleTaming ${consoleTaming}`);
}
consoleTaming === 'safe' ||
consoleTaming === 'unsafe' ||
failFast(`unrecognized consoleTaming ${consoleTaming}`);

let loggedErrorHandler;
if (optGetStackString === undefined) {
Expand Down Expand Up @@ -95,21 +100,36 @@ export const tameConsole = (
/* eslint-disable @endo/no-polymorphic-call */

// Node.js
if (errorTrapping !== 'none' && globalThis.process !== undefined) {
globalThis.process.on('uncaughtException', error => {
const globalProcess = globalThis.process || undefined;
if (
errorTrapping !== 'none' &&
typeof globalProcess === 'object' &&
typeof globalProcess.on === 'function'
mhofman marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should test for existence of abort and exit as well.

) {
let terminate;
if (errorTrapping === 'platform' || errorTrapping === 'exit') {
const { exit } = globalProcess;
// If there is a function-valued process.on but no function-valued process.exit,
// fail early without caring whether errorTrapping is "platform" only by default.
typeof exit === 'function' || failFast('missing process.exit');
terminate = () => exit(globalProcess.exitCode || -1);
} else if (errorTrapping === 'abort') {
terminate = globalProcess.abort;
typeof terminate === 'function' || failFast('missing process.abort');
}

globalProcess.on('uncaughtException', error => {
// causalConsole is born frozen so not vulnerable to method tampering.
ourConsole.error(error);
if (errorTrapping === 'platform' || errorTrapping === 'exit') {
globalThis.process.exit(globalThis.process.exitCode || -1);
} else if (errorTrapping === 'abort') {
globalThis.process.abort();
if (terminate) {
terminate();
}
});
}

if (
unhandledRejectionTrapping !== 'none' &&
globalThis.process !== undefined
typeof globalProcess === 'object' &&
typeof globalProcess.on === 'function'
) {
const handleRejection = reason => {
// 'platform' and 'report' just log the reason.
Expand All @@ -119,32 +139,32 @@ export const tameConsole = (
const h = makeRejectionHandlers(handleRejection);
if (h) {
// Rejection handlers are supported.
globalThis.process.on('unhandledRejection', h.unhandledRejectionHandler);
globalThis.process.on('rejectionHandled', h.rejectionHandledHandler);
globalThis.process.on('exit', h.processTerminationHandler);
globalProcess.on('unhandledRejection', h.unhandledRejectionHandler);
globalProcess.on('rejectionHandled', h.rejectionHandledHandler);
globalProcess.on('exit', h.processTerminationHandler);
}
}

// Browser
const globalWindow = globalThis.window || undefined;
if (
errorTrapping !== 'none' &&
globalThis.window !== undefined &&
globalThis.window.addEventListener !== undefined
typeof globalWindow === 'object' &&
typeof globalWindow.addEventListener === 'function'
) {
globalThis.window.addEventListener('error', event => {
globalWindow.addEventListener('error', event => {
event.preventDefault();
// 'platform' and 'report' just log the reason.
ourConsole.error(event.error);
if (errorTrapping === 'exit' || errorTrapping === 'abort') {
globalThis.window.location.href = `about:blank`;
globalWindow.location.href = `about:blank`;
}
});
}

if (
unhandledRejectionTrapping !== 'none' &&
globalThis.window !== undefined &&
globalThis.window.addEventListener !== undefined
typeof globalWindow === 'object' &&
typeof globalWindow.addEventListener === 'function'
) {
const handleRejection = reason => {
ourConsole.error('SES_UNHANDLED_REJECTION:', reason);
Expand All @@ -153,17 +173,17 @@ export const tameConsole = (
const h = makeRejectionHandlers(handleRejection);
if (h) {
// Rejection handlers are supported.
globalThis.window.addEventListener('unhandledrejection', event => {
globalWindow.addEventListener('unhandledrejection', event => {
event.preventDefault();
h.unhandledRejectionHandler(event.reason, event.promise);
});

globalThis.window.addEventListener('rejectionhandled', event => {
globalWindow.addEventListener('rejectionhandled', event => {
event.preventDefault();
h.rejectionHandledHandler(event.promise);
});

globalThis.window.addEventListener('beforeunload', _event => {
globalWindow.addEventListener('beforeunload', _event => {
h.processTerminationHandler();
});
}
Expand Down
10 changes: 4 additions & 6 deletions packages/ses/src/tame-domains.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@ export function tameDomains(domainTaming = 'safe') {
}

// Protect against the hazard presented by Node.js domains.
if (typeof globalThis.process === 'object' && globalThis.process !== null) {
const globalProcess = globalThis.process || undefined;
if (typeof globalProcess === 'object') {
// Check whether domains were initialized.
const domainDescriptor = getOwnPropertyDescriptor(
globalThis.process,
'domain',
);
const domainDescriptor = getOwnPropertyDescriptor(globalProcess, 'domain');
if (domainDescriptor !== undefined && domainDescriptor.get !== undefined) {
// The domain descriptor on Node.js initially has value: null, which
// becomes a get, set pair after domains initialize.
Expand All @@ -37,7 +35,7 @@ export function tameDomains(domainTaming = 'safe') {
// The domain module merely throws an exception when it attempts to define
// the domain property of the process global during its initialization.
// We have no better recourse because Node.js uses defineProperty too.
defineProperty(globalThis.process, 'domain', {
defineProperty(globalProcess, 'domain', {
value: null,
configurable: false,
writable: false,
Expand Down
22 changes: 22 additions & 0 deletions packages/ses/test/test-lockdown-shimmed-process.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/* global globalThis */

import test from 'ava';
import '../index.js';

test('shimmed globalThis.process', t => {
const process = {};
Object.defineProperty(globalThis, 'process', {
value: process,
configurable: false,
writable: false,
});
t.is(globalThis.process, process);
t.is(globalThis.process.on, undefined);
lockdown({
consoleTaming: 'safe',
errorTrapping: 'platform',
unhandledRejectionTrapping: 'report',
});
t.is(globalThis.process, process);
t.is(globalThis.process.on, undefined);
});