From 84031183bcedd613c067e2f58a02adad23441e87 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 17 Jul 2020 12:39:02 -0500 Subject: [PATCH] policy: support conditions for redirects PR-URL: https://github.com/nodejs/node/pull/34414 Reviewed-By: Jan Krems Reviewed-By: Guy Bedford --- doc/api/policy.md | 34 +++-- lib/internal/errors.js | 3 +- lib/internal/modules/cjs/helpers.js | 24 +++- lib/internal/modules/cjs/loader.js | 4 +- lib/internal/modules/esm/resolve.js | 25 ++++ lib/internal/policy/manifest.js | 98 +++++++++++--- lib/internal/util/compositekey.js | 114 ++++++++++++++++ node.gyp | 1 + .../test-policy-dependency-conditions.js | 122 ++++++++++++++++++ 9 files changed, 387 insertions(+), 38 deletions(-) create mode 100644 lib/internal/util/compositekey.js create mode 100644 test/parallel/test-policy-dependency-conditions.js diff --git a/doc/api/policy.md b/doc/api/policy.md index 05918500fcac21..e5387f49c0306d 100644 --- a/doc/api/policy.md +++ b/doc/api/policy.md @@ -124,24 +124,25 @@ replaced. ```json { - "builtins": [], "resources": { "./app/checked.js": { "dependencies": { "fs": true, - "os": "./app/node_modules/alt-os" + "os": "./app/node_modules/alt-os", + "http": { "import": true } } } } } ``` -The dependencies are keyed by the requested string specifier and have values -of either `true` or a string pointing to a module that will be resolved. +The dependencies are keyed by the requested specifier string and have values +of either `true`, `null`, a string pointing to a module that will be resolved, +or a conditions object. The specifier string does not perform any searching and must match exactly -what is provided to the `require()`. Therefore, multiple specifiers may be -needed in the policy if `require()` uses multiple different strings to point +what is provided to the `require()` or `import`. Therefore, multiple specifiers +may be needed in the policy if it uses multiple different strings to point to the same module (such as excluding the extension). If the value of the redirection is `true` the default searching algorithms will @@ -150,20 +151,31 @@ be used to find the module. If the value of the redirection is a string, it will be resolved relative to the manifest and then immediately be used without searching. -Any specifier string that is `require()`ed and not listed in the dependencies -will result in an error according to the policy. +Any specifier string that is attempted to resolved and not listed in the +dependencies will result in an error according to the policy. Redirection will not prevent access to APIs through means such as direct access to `require.cache` and/or through `module.constructor` which allow access to -loading modules. Policy redirection only affect specifiers to `require()`. -Other means such as to prevent undesired access to APIs through variables are -necessary to lock down that path of loading modules. +loading modules. Policy redirection only affect specifiers to `require()` and +`import`. Other means such as to prevent undesired access to APIs through +variables are necessary to lock down that path of loading modules. A boolean value of `true` for the dependencies map can be specified to allow a module to load any specifier without redirection. This can be useful for local development and may have some valid usage in production, but should be used only with care after auditing a module to ensure its behavior is valid. +Similar to `"exports"` in `package.json` dependencies can also be specified to +be objects containing conditions which branch how dependencies are loaded. In +the above example `"http"` will be allowed when the `"import"` condition is +part of loading it. + +A value of `null` for the resolved value will cause the resolution to fail. +This can be used to ensure some kinds dynamic access are explicitly prevented. + +Unknown values for the resolved module location will cause failure, but are +not guaranteed to be forwards compatible. + #### Example: Patched dependency Redirected dependencies can provide attenuated or modified functionality as fits diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 759025496801ba..c8d681e5356387 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1205,7 +1205,8 @@ E('ERR_MANIFEST_ASSERT_INTEGRITY', return msg; }, Error); E('ERR_MANIFEST_DEPENDENCY_MISSING', - 'Manifest resource %s does not list %s as a dependency specifier', + 'Manifest resource %s does not list %s as a dependency specifier for ' + + 'conditions: %s', Error); E('ERR_MANIFEST_INTEGRITY_MISMATCH', 'Manifest resource %s has multiple entries but integrity lists do not match', diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 3d224fb5142bca..9b6944c59390fe 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -1,9 +1,11 @@ 'use strict'; const { + ArrayPrototypeJoin, ObjectDefineProperty, ObjectPrototypeHasOwnProperty, SafeMap, + SafeSet, } = primordials; const { ERR_MANIFEST_DEPENDENCY_MISSING, @@ -16,10 +18,16 @@ const path = require('path'); const { pathToFileURL, fileURLToPath } = require('internal/url'); const { URL } = require('url'); +const { getOptionValue } = require('internal/options'); +const userConditions = getOptionValue('--conditions'); + let debug = require('internal/util/debuglog').debuglog('module', (fn) => { debug = fn; }); +// TODO: Use this set when resolving pkg#exports conditions in loader.js. +const cjsConditions = new SafeSet(['require', 'node', ...userConditions]); + function loadNativeModule(filename, request) { const mod = NativeModule.map.get(filename); if (mod) { @@ -38,11 +46,12 @@ function makeRequireFunction(mod, redirects) { let require; if (redirects) { - const { resolve, reaction } = redirects; const id = mod.filename || mod.id; - require = function require(path) { + const conditions = cjsConditions; + const { resolve, reaction } = redirects; + require = function require(specifier) { let missing = true; - const destination = resolve(path); + const destination = resolve(specifier, conditions); if (destination === true) { missing = false; } else if (destination) { @@ -66,9 +75,13 @@ function makeRequireFunction(mod, redirects) { } } if (missing) { - reaction(new ERR_MANIFEST_DEPENDENCY_MISSING(id, path)); + reaction(new ERR_MANIFEST_DEPENDENCY_MISSING( + id, + specifier, + ArrayPrototypeJoin([...conditions], ', ') + )); } - return mod.require(path); + return mod.require(specifier); }; } else { require = function require(path) { @@ -168,6 +181,7 @@ function normalizeReferrerURL(referrer) { module.exports = { addBuiltinLibsToObject, + cjsConditions, loadNativeModule, makeRequireFunction, normalizeReferrerURL, diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 7fe5f4ce8f9b45..2f62265952208c 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -44,7 +44,6 @@ const { ReflectSet, RegExpPrototypeTest, SafeMap, - SafeSet, String, StringPrototypeMatch, StringPrototypeSlice, @@ -71,6 +70,7 @@ const { makeRequireFunction, normalizeReferrerURL, stripBOM, + cjsConditions, loadNativeModule } = require('internal/modules/cjs/helpers'); const { getOptionValue } = require('internal/options'); @@ -81,7 +81,6 @@ const manifest = getOptionValue('--experimental-policy') ? require('internal/process/policy').manifest : null; const { compileFunction } = internalBinding('contextify'); -const userConditions = getOptionValue('--conditions'); // Whether any user-provided CJS modules had been loaded (executed). // Used for internal assertions. @@ -803,7 +802,6 @@ Module._load = function(request, parent, isMain) { return module.exports; }; -const cjsConditions = new SafeSet(['require', 'node', ...userConditions]); Module._resolveFilename = function(request, parent, isMain, options) { if (NativeModule.canBeRequiredByUsers(request)) { return request; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index f6879465451c83..04a17c908ad91b 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -30,6 +30,9 @@ const { Stats, } = require('fs'); const { getOptionValue } = require('internal/options'); +const manifest = getOptionValue('--experimental-policy') ? + require('internal/process/policy').manifest : + null; const { sep, relative } = require('path'); const preserveSymlinks = getOptionValue('--preserve-symlinks'); const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); @@ -41,6 +44,7 @@ const { ERR_INVALID_MODULE_SPECIFIER, ERR_INVALID_PACKAGE_CONFIG, ERR_INVALID_PACKAGE_TARGET, + ERR_MANIFEST_DEPENDENCY_MISSING, ERR_MODULE_NOT_FOUND, ERR_PACKAGE_IMPORT_NOT_DEFINED, ERR_PACKAGE_PATH_NOT_EXPORTED, @@ -710,6 +714,27 @@ function resolveAsCommonJS(specifier, parentURL) { function defaultResolve(specifier, context = {}, defaultResolveUnused) { let { parentURL, conditions } = context; + if (manifest) { + const redirects = manifest.getRedirector(parentURL); + if (redirects) { + const { resolve, reaction } = redirects; + const destination = resolve(specifier, new SafeSet(conditions)); + let missing = true; + if (destination === true) { + missing = false; + } else if (destination) { + const href = destination.href; + return { url: href }; + } + if (missing) { + reaction(new ERR_MANIFEST_DEPENDENCY_MISSING( + parentURL, + specifier, + ArrayPrototypeJoin([...conditions], ', ')) + ); + } + } + } let parsed; try { parsed = new URL(specifier); diff --git a/lib/internal/policy/manifest.js b/lib/internal/policy/manifest.js index 0531f198f21971..920790a22ed46e 100644 --- a/lib/internal/policy/manifest.js +++ b/lib/internal/policy/manifest.js @@ -7,11 +7,15 @@ const { ObjectCreate, ObjectEntries, ObjectFreeze, + ObjectKeys, ObjectSetPrototypeOf, RegExpPrototypeTest, SafeMap, uncurryThis, } = primordials; +const { + compositeKey +} = require('internal/util/compositekey'); const { canBeRequiredByUsers } = require('internal/bootstrap/loaders').NativeModule; @@ -70,13 +74,21 @@ class Manifest { */ #integrities = new SafeMap(); /** - * @type {Map true | URL>} + * @type { + Map< + string, + (specifier: string, conditions: Set) => true | null | URL + > + } * * Used to find where a dependency is located. * * This stores functions to lazily calculate locations as needed. * `true` is used to signify that the location is not specified * by the manifest and default resolution should be allowed. + * + * The functions return `null` to signify that a dependency is + * not found */ #dependencies = new SafeMap(); /** @@ -158,36 +170,83 @@ class Manifest { dependencyMap = ObjectCreate(null); } if (typeof dependencyMap === 'object' && !ArrayIsArray(dependencyMap)) { + function searchDependencies(target, conditions) { + if ( + target && + typeof target === 'object' && + !ArrayIsArray(target) + ) { + const keys = ObjectKeys(target); + for (let i = 0; i < keys.length; i++) { + const key = keys[i]; + if (conditions.has(key)) { + const ret = searchDependencies(target[key], conditions); + if (ret != null) { + return ret; + } + } + } + } else if (typeof target === 'string') { + return target; + } else if (target === true) { + return target; + } else { + throw new ERR_MANIFEST_INVALID_RESOURCE_FIELD( + resourceHREF, + 'dependencies'); + } + return null; + } + // This is used so we don't traverse this every time + // in theory we can delete parts of the dep map once this is populated + const localMappings = new SafeMap(); /** - * @returns {true | URL} + * @returns {true | null | URL} */ - const dependencyRedirectList = (toSpecifier) => { - if (toSpecifier in dependencyMap !== true) { + const dependencyRedirectList = (specifier, conditions) => { + const key = compositeKey([localMappings, specifier, ...conditions]); + if (localMappings.has(key)) { + return localMappings.get(key); + } + if (specifier in dependencyMap !== true) { + localMappings.set(key, null); return null; } - const to = dependencyMap[toSpecifier]; - if (to === true) { + const target = searchDependencies( + dependencyMap[specifier], + conditions); + if (target === true) { + localMappings.set(key, true); return true; } - if (parsedURLs.has(to)) { - return parsedURLs.get(to); - } else if (canBeRequiredByUsers(to)) { - const href = `node:${to}`; + if (typeof target !== 'string') { + localMappings.set(key, null); + return null; + } + if (parsedURLs.has(target)) { + const parsed = parsedURLs.get(target); + localMappings.set(key, parsed); + return parsed; + } else if (canBeRequiredByUsers(target)) { + const href = `node:${target}`; const resolvedURL = new URL(href); - parsedURLs.set(to, resolvedURL); + parsedURLs.set(target, resolvedURL); parsedURLs.set(href, resolvedURL); + localMappings.set(key, resolvedURL); return resolvedURL; - } else if (RegExpPrototypeTest(kRelativeURLStringPattern, to)) { - const resolvedURL = new URL(to, manifestURL); + } else if (RegExpPrototypeTest(kRelativeURLStringPattern, target)) { + const resolvedURL = new URL(target, manifestURL); const href = resourceURL.href; - parsedURLs.set(to, resolvedURL); + parsedURLs.set(target, resolvedURL); parsedURLs.set(href, resolvedURL); + localMappings.set(key, resolvedURL); return resolvedURL; } - const resolvedURL = new URL(to); - const href = resourceURL.href; - parsedURLs.set(to, resolvedURL); + const resolvedURL = new URL(target); + const href = resolvedURL.href; + parsedURLs.set(target, resolvedURL); parsedURLs.set(href, resolvedURL); + localMappings.set(key, resolvedURL); return resolvedURL; }; dependencies.set(resourceHREF, dependencyRedirectList); @@ -208,7 +267,10 @@ class Manifest { const dependencies = this.#dependencies; if (dependencies.has(requester)) { return { - resolve: (to) => dependencies.get(requester)(`${to}`), + resolve: (specifier, conditions) => dependencies.get(requester)( + `${specifier}`, + conditions + ), reaction: this.#reaction }; } diff --git a/lib/internal/util/compositekey.js b/lib/internal/util/compositekey.js new file mode 100644 index 00000000000000..9b7c460231f876 --- /dev/null +++ b/lib/internal/util/compositekey.js @@ -0,0 +1,114 @@ +'use strict'; +const { + codes: { + ERR_INVALID_ARG_VALUE, + }, +} = require('internal/errors'); +const { + ObjectCreate, + ObjectFreeze, + SafeMap, + SafeWeakMap, +} = primordials; +/** + * @param {any} value + * @returns {boolean} + */ +const hasLifetime = (value) => { + return value !== null && ( + typeof value === 'object' || + typeof value === 'function' + ); +}; +class CompositeNode { + /** + * @type {WeakMap>} + */ + lifetimeNodes; + /** + * @type {Map>} + */ + primitiveNodes; + /** + * @type {null | Readonly<{}>} + */ + value; + constructor() { + this.value = null; + } + get() { + if (this.value === null) { + return this.value = ObjectFreeze(ObjectCreate(null)); + } + return this.value; + } + /** + * @param {any} value + * @param {number} position + */ + emplacePrimitive(value, position) { + if (!this.primitiveNodes) { + this.primitiveNodes = new SafeMap(); + } + if (!this.primitiveNodes.has(value)) { + this.primitiveNodes.set(value, new SafeMap()); + } + const positions = this.primitiveNodes.get(value); + if (!positions.has(position)) { + positions.set(position, new CompositeNode()); + } + return positions.get(position); + } + /** + * @param {object} value + * @param {number} position + */ + emplaceLifetime(value, position) { + if (!this.lifetimeNodes) { + this.lifetimeNodes = new SafeWeakMap(); + } + if (!this.lifetimeNodes.has(value)) { + this.lifetimeNodes.set(value, new SafeMap()); + } + const positions = this.lifetimeNodes.get(value); + if (!positions.has(position)) { + positions.set(position, new CompositeNode()); + } + return positions.get(position); + } +} +const compoundStore = new CompositeNode(); +// Accepts objects as a key and does identity on the parts of the iterable +/** + * @param {any[]} parts + */ +const compositeKey = (parts) => { + /** + * @type {CompositeNode} + */ + let node = compoundStore; + for (let i = 0; i < parts.length; i++) { + const value = parts[i]; + if (hasLifetime(value)) { + node = node.emplaceLifetime(value, i); + parts[i] = hasLifetime; + } + } + // Does not leak WeakMap paths since there are none added + if (node === compoundStore) { + throw new ERR_INVALID_ARG_VALUE( + 'parts', + parts, + 'must contain a non-primitive element'); + } + for (let i = 0; i < parts.length; i++) { + const value = parts[i]; + if (value !== hasLifetime) { + node = node.emplacePrimitive(value, i); + } + } + return node.get(); +}; +module.exports = { + compositeKey +}; diff --git a/node.gyp b/node.gyp index bc5e4a11aecde1..d11d6703f647de 100644 --- a/node.gyp +++ b/node.gyp @@ -204,6 +204,7 @@ 'lib/internal/url.js', 'lib/internal/util.js', 'lib/internal/util/comparisons.js', + 'lib/internal/util/compositekey.js', 'lib/internal/util/debuglog.js', 'lib/internal/util/inspect.js', 'lib/internal/util/inspector.js', diff --git a/test/parallel/test-policy-dependency-conditions.js b/test/parallel/test-policy-dependency-conditions.js new file mode 100644 index 00000000000000..af865538ce7ebd --- /dev/null +++ b/test/parallel/test-policy-dependency-conditions.js @@ -0,0 +1,122 @@ +'use strict'; +// Flags: --expose-internals + +const common = require('../common'); + +if (!common.hasCrypto) common.skip('missing crypto'); + +const Manifest = require('internal/policy/manifest').Manifest; + + +const assert = require('assert'); + +const { debuglog } = require('util'); +const debug = debuglog('test'); + +const conditionTreePermutations = [ + ['default'], + ['import'], + ['node'], + ['require'], + ['require', 'import'], + ['import', 'require'], + ['default', 'require'], + ['require', 'default'], + ['node', 'require'], + ['require', 'node'], +]; +for (const totalDepth of [1, 2, 3]) { + const conditionTrees = []; + function calc(depthLeft = 0, path = []) { + if (depthLeft) { + for (const conditions of conditionTreePermutations) { + calc(depthLeft - 1, [...path, conditions]); + } + } else { + conditionTrees.push(path); + } + } + calc(totalDepth, []); + let nextURLId = 1; + function getUniqueHREF() { + const id = nextURLId++; + return `test:${id}`; + } + for (const tree of conditionTrees) { + const root = {}; + const targets = [root]; + const offsets = [-1]; + const order = []; + while (offsets.length) { + const depth = offsets.length - 1; + offsets[depth]++; + const conditionOffset = offsets[depth]; + const conditionsForDepth = tree[depth]; + if (conditionOffset >= conditionsForDepth.length) { + offsets.pop(); + continue; + } + let target; + if (depth === tree.length - 1) { + target = getUniqueHREF(); + order.push({ + target, + conditions: new Set( + offsets.map( + (conditionOffset, depth) => tree[depth][conditionOffset] + ) + ) + }); + } else { + target = {}; + targets[depth + 1] = target; + offsets.push(-1); + } + const condition = tree[depth][conditionOffset]; + targets[depth][condition] = target; + } + const manifest = new Manifest({ + resources: { + 'test:_': { + dependencies: { + _: root + } + } + } + }); + const redirector = manifest.getRedirector('test:_'); + for (const { target, conditions } of order) { + const result = redirector.resolve('_', conditions).href; + if (result !== target) { + // If we didn't hit the target, make sure a target prior to this one + // matched, including conditions + searchPriorTargets: + for (const { target: preTarget, conditions: preConditions } of order) { + if (result === preTarget) { + // Ensure that the current conditions are a super set of the + // prior target + for (const preCondition of preConditions) { + if (conditions.has(preCondition) !== true) { + continue searchPriorTargets; + } + } + break searchPriorTargets; + } + if (preTarget === target) { + debug( + 'dependencies %O expected ordering %O and trying for %j ' + + 'no prior targets matched', + root, + order, + target + ); + // THIS WILL ALWAYS FAIL, but we want that error message + assert.strictEqual( + result, target + ); + } + } + } + } + } +}