From 52c7ed6bf33ed114ea281d6441368af76d4e8de2 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 --- 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 a9c69eda7b0e6c..08e83879e0a84d 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1215,7 +1215,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 794f84c12855d1..4945d14cd4a3e6 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -47,7 +47,6 @@ const { RegExpPrototypeTest, SafeMap, SafeWeakMap, - SafeSet, String, StringPrototypeIndexOf, StringPrototypeMatch, @@ -76,6 +75,7 @@ const { makeRequireFunction, normalizeReferrerURL, stripBOM, + cjsConditions, loadNativeModule } = require('internal/modules/cjs/helpers'); const { getOptionValue } = require('internal/options'); @@ -85,7 +85,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. @@ -1002,7 +1001,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 7cf3552948194d..643248707e8d00 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -31,6 +31,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'); @@ -42,6 +45,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, @@ -764,6 +768,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 72e06754210cc4..f198bc6cf529d7 100644 --- a/node.gyp +++ b/node.gyp @@ -214,6 +214,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 + ); + } + } + } + } + } +}