From 4653b7fd2a5dec90322c99e02566052191860d21 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Sun, 25 Jul 2021 12:06:23 -0700 Subject: [PATCH] fix: Enable enforcement that far must be declared --- .../src/helpers/environment-options.js | 131 ++++++++++++++++++ packages/marshal/src/helpers/remotable.js | 10 +- .../marshal/test/allow_implicit_remotables.js | 9 ++ .../test/default_implicit_remotables.js | 9 ++ .../test/disallow_implicit_remotables.js | 9 ++ .../test/test-allow-implicit-remotables.js | 11 ++ .../test/test-default-implicit-remotables.js | 20 +++ .../test/test-disallow-implicit-remotables.js | 13 ++ .../test/test-existing-implicit-remotables.js | 18 +++ packages/marshal/test/test-marshal-far-obj.js | 2 +- 10 files changed, 228 insertions(+), 4 deletions(-) create mode 100644 packages/marshal/src/helpers/environment-options.js create mode 100644 packages/marshal/test/allow_implicit_remotables.js create mode 100644 packages/marshal/test/default_implicit_remotables.js create mode 100644 packages/marshal/test/disallow_implicit_remotables.js create mode 100644 packages/marshal/test/test-allow-implicit-remotables.js create mode 100644 packages/marshal/test/test-default-implicit-remotables.js create mode 100644 packages/marshal/test/test-disallow-implicit-remotables.js create mode 100644 packages/marshal/test/test-existing-implicit-remotables.js diff --git a/packages/marshal/src/helpers/environment-options.js b/packages/marshal/src/helpers/environment-options.js new file mode 100644 index 00000000000..129cb49c786 --- /dev/null +++ b/packages/marshal/src/helpers/environment-options.js @@ -0,0 +1,131 @@ +// @ts-check +/* global globalThis */ + +// Note: Might need to be imported quite early, so this module should +// avoid importing other things. + +const { details: X, quote: q } = assert; + +/** + * JavaScript module semantics resists attempts to parameterize a module's + * initialization behavior. A module initializes in order according to + * the path by which it is first imported, and then the initialized module + * is reused by all the other times it is imported. Compartments give us + * the opportunity to bind the same import name to different imported + * modules, depending on the package/compartment doing the import. Compartments + * also address the difficulty of parameterizing a module's initialization + * logic, but not in a pleasant manner. + * + * A pleasant parameterization would be for a static module to be function-like + * with explicit parameters, and for the parameterization to be like + * calling the static module with parameters in order to derive from it a + * module instance. Compartments instead lets us parameterize the meaning + * of a module instance derived from a static module according to the + * three namespaces provided by the JavaScript semantics, effecting the + * meaning of a module instance. + * * The global variable namespaces. + * * The global scope, aliased to properties of the global object. + * This is necessarily compartment-wide, and therefore in our + * recommened usage pattern, packages-wide. + * * The global lexical scope. The SES-shim compartments supports + * these both compartment-wide as well as per-module. But it is + * not yet clear what we will propose in the Compartment proposal. + * * The import namespace. + * * The host hooks. + * + * This `environment-options.js` module looks for a setting of of an + * `optionName` parameter rooted in the global scope. If follows the Node + * precedent for finding Unix environment variable settings, looking for a + * global `process` object holding an `env` object, + * optionally holding a property named for the `optionName` whose value is the + * configuration setting of that option. For example, for the optionName + * `ALLOW_IMPLICIT_REMOTABLES` it would look in + * `globalThis.process.env.ALLOW_IMPLICIT_REMOTABLES`. + * + * If setting is either absent or `undefined`, that indicates that + * this configuration option should have its default behavior, whatever that is. + * Otherwise, reflecting Unix environment variables, the setting must be a + * string. This also helps ensure that this channel is used only to pass data, + * not authority beyond the ability to read this global state. + * + * The current placement of this `environment-options.js` module in the + * `@agoric/marshal` package is a stopgap measure. + * TODO the intention is to migrate it into Endo, and to migrate all our + * direct uses of `process.env` for configuration parameters to use it + * instead. + * + * Even after that migration, this module will still not be used for + * `LOCKDOWN_OPTIONS` itself, since that must happen before `lockdown`, + * whereas this module must initialize after `lockdown`. + * + * @param {string} optionName + * @param {string=} defaultSetting + */ +export const getEnvironmentOption = ( + optionName, + defaultSetting = undefined, +) => { + assert.typeof( + optionName, + 'string', + X`Environment option name ${q(optionName)} must be a string.`, + ); + assert( + defaultSetting === undefined || typeof defaultSetting === 'string', + X`Environment option default setting ${q( + defaultSetting, + )}, if present, must be a string.`, + ); + + let setting = defaultSetting; + const globalProcess = globalThis.process; + if (typeof process === 'object' && typeof globalProcess.env === 'object') { + if (optionName in globalProcess.env) { + console.log( + `Environment options sniffed and found an apparent ${q( + optionName, + )} environment variable.'\n`, + ); + setting = globalProcess.env[optionName]; + } + } + assert( + setting === undefined || typeof setting === 'string', + X`Environment option value ${q(setting)}, if present, must be a string.`, + ); + return setting; +}; +harden(getEnvironmentOption); + +/** + * Set `globalThis.process.env[optionName]` to `setting`. + * + * This function takes care of the complexity that `process` may or may not + * already exist, `process.env` may or may not already exist, and if both + * exist, this function should mutate only this one property setting, minimizing + * other damage to a shared `globalThis.process.env`. + * + * @param {string} optionName + * @param {string=} setting + */ +export const setEnvironmentOption = (optionName, setting) => { + assert.typeof(optionName, 'string'); + assert(setting === undefined || typeof setting === 'string'); + if (!('process' in globalThis)) { + // @ts-ignore TS assumes this is the Node process object. + globalThis.process = {}; + } + const globalProcess = globalThis.process; + assert.typeof(globalProcess, 'object'); + if (!('env' in globalProcess)) { + // @ts-ignore TS assumes this is the Node process object. + globalProcess.env = {}; + } + const env = globalProcess.env; + assert.typeof(env, 'object'); + if (optionName in env) { + console.log(`Overwriting apparent environment variable ${q(optionName)}`); + } + env[optionName] = setting; +}; +harden(setEnvironmentOption); diff --git a/packages/marshal/src/helpers/remotable.js b/packages/marshal/src/helpers/remotable.js index 4eb391b5bd4..a7bf5d1e9a4 100644 --- a/packages/marshal/src/helpers/remotable.js +++ b/packages/marshal/src/helpers/remotable.js @@ -19,6 +19,7 @@ import { PASS_STYLE, checkTagRecord, } from './passStyleHelpers.js'; +import { getEnvironmentOption } from './environment-options.js'; const { details: X, quote: q } = assert; const { ownKeys } = Reflect; @@ -39,8 +40,11 @@ const { // TODO: once the policy changes to force remotables to be explicit, remove this // flag entirely and fix code that uses it (as if it were always `false`). // -// Exported only for testing during the transition -export const ALLOW_IMPLICIT_REMOTABLES = true; +// Exported only for testing during the transition. The first step +// will be to change the default, the second argument to `getEnvironmentOption` +// below, from `'true'` to `'false'`. +export const ALLOW_IMPLICIT_REMOTABLES = + getEnvironmentOption('ALLOW_IMPLICIT_REMOTABLES', 'true') === 'true'; /** * @param {InterfaceSpec} iface @@ -94,7 +98,7 @@ const checkRemotableProtoOf = (original, check = x => x) => { // would always return true. // @ts-ignore TypeScript assumes what we're trying to check proto !== objectPrototype, - X`Remotables must now be explicitly declared: ${q(original)}`, + X`Remotables must be explicitly declared: ${q(original)}`, ) && checkTagRecord(proto, 'remotable', check) ) ) { diff --git a/packages/marshal/test/allow_implicit_remotables.js b/packages/marshal/test/allow_implicit_remotables.js new file mode 100644 index 00000000000..c4f8912c94c --- /dev/null +++ b/packages/marshal/test/allow_implicit_remotables.js @@ -0,0 +1,9 @@ +// @ts-check + +// This one is designed to be imported this early +import { setEnvironmentOption } from '../src/helpers/environment-options.js'; + +// Import this module early, so it initializes before any module whose +// initialization reads this option. + +setEnvironmentOption('ALLOW_IMPLICIT_REMOTABLES', 'true'); diff --git a/packages/marshal/test/default_implicit_remotables.js b/packages/marshal/test/default_implicit_remotables.js new file mode 100644 index 00000000000..2efe8789ffe --- /dev/null +++ b/packages/marshal/test/default_implicit_remotables.js @@ -0,0 +1,9 @@ +// @ts-check + +// This one is designed to be imported this early +import { setEnvironmentOption } from '../src/helpers/environment-options.js'; + +// Import this module early, so it initializes before any module whose +// initialization reads this option. + +setEnvironmentOption('ALLOW_IMPLICIT_REMOTABLES', undefined); diff --git a/packages/marshal/test/disallow_implicit_remotables.js b/packages/marshal/test/disallow_implicit_remotables.js new file mode 100644 index 00000000000..348c168a9ed --- /dev/null +++ b/packages/marshal/test/disallow_implicit_remotables.js @@ -0,0 +1,9 @@ +// @ts-check + +// This one is designed to be imported this early +import { setEnvironmentOption } from '../src/helpers/environment-options.js'; + +// Import this module early, so it initializes before any module whose +// initialization reads this option. + +setEnvironmentOption('ALLOW_IMPLICIT_REMOTABLES', 'false'); diff --git a/packages/marshal/test/test-allow-implicit-remotables.js b/packages/marshal/test/test-allow-implicit-remotables.js new file mode 100644 index 00000000000..e4bef858d9a --- /dev/null +++ b/packages/marshal/test/test-allow-implicit-remotables.js @@ -0,0 +1,11 @@ +// @ts-check + +import { test } from './prepare-test-env-ava.js'; +// Import early, before remotable.js might initialize. +import './allow_implicit_remotables.js'; + +import { passStyleOf } from '../src/passStyleOf.js'; + +test('environment options', t => { + t.notThrows(() => passStyleOf(harden({ toString: () => 'foo' }))); +}); diff --git a/packages/marshal/test/test-default-implicit-remotables.js b/packages/marshal/test/test-default-implicit-remotables.js new file mode 100644 index 00000000000..f3096d0d834 --- /dev/null +++ b/packages/marshal/test/test-default-implicit-remotables.js @@ -0,0 +1,20 @@ +// @ts-check + +import { test } from './prepare-test-env-ava.js'; +// Import early, before remotable.js might initialize. +import './default_implicit_remotables.js'; + +import { passStyleOf } from '../src/passStyleOf.js'; +import { ALLOW_IMPLICIT_REMOTABLES } from '../src/helpers/remotable.js'; + +// Whatever ALLOW_IMPLICIT_REMOTABLES defaults to, ensure that still works. + +test('environment options', t => { + if (ALLOW_IMPLICIT_REMOTABLES) { + t.notThrows(() => passStyleOf(harden({ toString: () => 'foo' }))); + } else { + t.throws(() => passStyleOf(harden({ toString: () => 'foo' })), { + message: /Remotables must be explicitly declared/, + }); + } +}); diff --git a/packages/marshal/test/test-disallow-implicit-remotables.js b/packages/marshal/test/test-disallow-implicit-remotables.js new file mode 100644 index 00000000000..05af480f5da --- /dev/null +++ b/packages/marshal/test/test-disallow-implicit-remotables.js @@ -0,0 +1,13 @@ +// @ts-check + +import { test } from './prepare-test-env-ava.js'; +// Import early, before remotable.js might initialize. +import './disallow_implicit_remotables.js'; + +import { passStyleOf } from '../src/passStyleOf.js'; + +test('environment options', t => { + t.throws(() => passStyleOf(harden({ toString: () => 'foo' })), { + message: /Remotables must be explicitly declared/, + }); +}); diff --git a/packages/marshal/test/test-existing-implicit-remotables.js b/packages/marshal/test/test-existing-implicit-remotables.js new file mode 100644 index 00000000000..eedea4d3842 --- /dev/null +++ b/packages/marshal/test/test-existing-implicit-remotables.js @@ -0,0 +1,18 @@ +// @ts-check + +import { test } from './prepare-test-env-ava.js'; +import { passStyleOf } from '../src/passStyleOf.js'; +import { ALLOW_IMPLICIT_REMOTABLES } from '../src/helpers/remotable.js'; + +// Whatever ALLOW_IMPLICIT_REMOTABLES is set to in the current test +// environment, ensure that still works. + +test('environment options', t => { + if (ALLOW_IMPLICIT_REMOTABLES) { + t.notThrows(() => passStyleOf(harden({ toString: () => 'foo' }))); + } else { + t.throws(() => passStyleOf(harden({ toString: () => 'foo' })), { + message: /Remotables must be explicitly declared/, + }); + } +}); diff --git a/packages/marshal/test/test-marshal-far-obj.js b/packages/marshal/test/test-marshal-far-obj.js index a38569e712c..0a9f9fe87f8 100644 --- a/packages/marshal/test/test-marshal-far-obj.js +++ b/packages/marshal/test/test-marshal-far-obj.js @@ -240,7 +240,7 @@ test('transitional remotables', t => { } const FAR_NOACC = /cannot serialize Remotables with accessors/; const FAR_ONLYMETH = /cannot serialize Remotables with non-methods/; - const FAR_EXPLICIT = /Remotables must now be explicitly declared/; + const FAR_EXPLICIT = /Remotables must be explicitly declared/; // Far('iface', {}) // all cases: pass-by-ref