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

policy: add dependencies map for redirect and whitelisting #28767

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 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
14 changes: 14 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1432,6 +1432,13 @@ An attempt was made to load a resource, but the resource did not match the
integrity defined by the policy manifest. See the documentation for [policy]
manifests for more information.

<a id="ERR_MANIFEST_DEPENDENCY_MISSING"></a>
### ERR_MANIFEST_DEPENDENCY_MISSING

An attempt was made to load a resource, but the resource was not listed as a
dependency from the location that attempted to load it. See the documentation
for [policy] manifests for more information.

<a id="ERR_MANIFEST_INTEGRITY_MISMATCH"></a>
### ERR_MANIFEST_INTEGRITY_MISMATCH

Expand All @@ -1440,6 +1447,13 @@ entries for a resource which did not match each other. Update the manifest
entries to match in order to resolve this error. See the documentation for
[policy] manifests for more information.

<a id="ERR_MANIFEST_INVALID_RESOURCE_FIELD"></a>
### ERR_MANIFEST_INVALID_RESOURCE_FIELD

A policy manifest resource had an invalid value for one of its fields. Update
the manifest entry to match in order to resolve this error. See the
bmeck marked this conversation as resolved.
Show resolved Hide resolved
documentation for [policy] manifests for more information.

<a id="ERR_MANIFEST_PARSE_POLICY"></a>
### ERR_MANIFEST_PARSE_POLICY

Expand Down
71 changes: 71 additions & 0 deletions doc/api/policy.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,5 +109,76 @@ In order to generate integrity strings, a script such as
`printf "sha384-$(cat checked.js | openssl dgst -sha384 -binary | base64)"`
can be used.

Integrity can be specified as the boolean value `true` in order to accept any
bmeck marked this conversation as resolved.
Show resolved Hide resolved
body for the resource which can be useful for local development. It is not
recommended in production since it would allow unexpected alteration of
resources to be considered valid.

### Dependency Redirection

An application may need to ship patched versions of software or to prevent
software from allowing all modules access to all other modules. In order to
Copy link
Contributor

@sam-github sam-github Jul 23, 2019

Choose a reason for hiding this comment

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

I'm not understanding this sentence, what does "software" refer to? How does this "software" allow modules access to other modules, and what does "access" mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

"software" is w/e the application seeks to run CJS / C++ modules / etc. For example, if you install a JS parser you would likely not want to allow it to access http as it that would be unexpected to have a string based operation need network access.

Copy link
Member

Choose a reason for hiding this comment

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

Optional nit:

Suggested change
software from allowing all modules access to all other modules. In order to
software from allowing all modules access to all other modules. To

Copy link
Member Author

Choose a reason for hiding this comment

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

I have rephrased this

do so redirection can be used.
bmeck marked this conversation as resolved.
Show resolved Hide resolved

```json
{
"builtins": [],
"resources": {
"./app/checked.js": {
"dependencies": {
"fs": true,
"os": "./app/node_modules/alt-os"
}
}
}
}
```

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 specifier string does not perform any searching and must match exactly
what is provided to the `require()`. Therefore, multiple specifiers may be
bmeck marked this conversation as resolved.
Show resolved Hide resolved
needed in the policy if `require()` 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
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 missing from the list of dependency will result in an error
bmeck marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

what does it mean for a specifier to be "missing"? do you mean "any string that is require()ed and not listed in the dependencies will result in an error"?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

i've put in your text but included the word specifier as it has a meaning that I feel is important.

according to the policy.

This will not prevent access to APIs through other means such as direct access
to `require.cache` and/or through `module.constructor`. Other means such as
attenuating variables are necessary to lock down that path of loading modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

what does "attenuation" mean wrt. JS variables?

Copy link
Member Author

Choose a reason for hiding this comment

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

Attenuation is a term used in various meetings we have about SES / Realms / etc. in TC39 meetings. Attenuation equates roughly to providing a specialized view of a variable/object/module etc. For example an attenuated fs would be a fs implementation different from the normal one. Attenuating a variable would be replacing the value with one that has been given a different implementation such as removing access to module.constructor or require.cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

i've rephrased this


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.

#### Example: Patched Dependency

Since a dependency can be redirected, you can provide attenuated or modified
forms of dependencies as fits your application. For example, you could log
data about timing of function durations by wrapping the original:

```js
const original = require('fn');
module.exports = function fn(...args) {
console.time();
try {
return new.target ?
Reflect.construct(original, args) :
Reflect.apply(original, this, args);
} finally {
console.timeEnd();
}
};
```


[relative url string]: https://url.spec.whatwg.org/#relative-url-with-fragment-string
6 changes: 6 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1033,9 +1033,15 @@ E('ERR_MANIFEST_ASSERT_INTEGRITY',
}
return msg;
}, Error);
E('ERR_MANIFEST_DEPENDENCY_MISSING',
'Manifest resource %s does not list %s as a dependency specifier',
Error);
E('ERR_MANIFEST_INTEGRITY_MISMATCH',
'Manifest resource %s has multiple entries but integrity lists do not match',
SyntaxError);
E('ERR_MANIFEST_INVALID_RESOURCE_FIELD',
'Manifest resource %s has invalid property value for %s',
TypeError);
E('ERR_MANIFEST_TDZ', 'Manifest initialization has not yet run', Error);
E('ERR_MANIFEST_UNKNOWN_ONERROR',
'Manifest specified unknown error behavior "%s".',
Expand Down
62 changes: 58 additions & 4 deletions lib/internal/modules/cjs/helpers.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,72 @@
'use strict';

const { Object } = primordials;
const {
ERR_MANIFEST_DEPENDENCY_MISSING,
ERR_UNKNOWN_BUILTIN_MODULE
} = require('internal/errors').codes;
const { NativeModule } = require('internal/bootstrap/loaders');
const { getOptionValue } = require('internal/options');
const experimentalModules = getOptionValue('--experimental-modules');

const { validateString } = require('internal/validators');
const path = require('path');
const { pathToFileURL } = require('internal/url');
const { pathToFileURL, fileURLToPath } = require('internal/url');
const { URL } = require('url');

const debug = require('internal/util/debuglog').debuglog('module');

function loadNativeModule(filename, request, experimentalModules) {
const mod = NativeModule.map.get(filename);
if (mod) {
debug('load native module %s', request);
mod.compileForPublicLoader(experimentalModules);
return mod;
}
}

// Invoke with makeRequireFunction(module) where |module| is the Module object
// to use as the context for the require() function.
function makeRequireFunction(mod) {
// Use redirects to set up a mapping from a policy and restrict dependencies
function makeRequireFunction(mod, redirects) {
const Module = mod.constructor;

function require(path) {
return mod.require(path);
let require;
if (redirects) {
const { map, reaction } = redirects;
const id = mod.filename || mod.id;
require = function require(path) {
let missing = true;
if (map === true) {
missing = false;
} else if (map.has(path)) {
const redirect = map.get(path);
if (redirect === true) {
missing = false;
} else if (typeof redirect === 'string') {
const parsed = new URL(redirect);
if (parsed.protocol === 'node:') {
const specifier = parsed.pathname;
const mod = loadNativeModule(
specifier,
redirect,
experimentalModules);
if (mod && mod.canBeRequiredByUsers) return mod.exports;
throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier);
} else if (parsed.protocol === 'file:') {
return mod.require(fileURLToPath(parsed));
}
}
}
if (missing) {
reaction(new ERR_MANIFEST_DEPENDENCY_MISSING(id, path));
}
return mod.require(path);
};
} else {
require = function require(path) {
return mod.require(path);
};
}

function resolve(request, options) {
Expand Down Expand Up @@ -114,6 +167,7 @@ function normalizeReferrerURL(referrer) {
module.exports = {
addBuiltinLibsToObject,
builtinLibs,
loadNativeModule,
makeRequireFunction,
normalizeReferrerURL,
stripBOM,
Expand Down
17 changes: 9 additions & 8 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const {
makeRequireFunction,
normalizeReferrerURL,
stripBOM,
loadNativeModule
} = require('internal/modules/cjs/helpers');
const { getOptionValue } = require('internal/options');
const preserveSymlinks = getOptionValue('--preserve-symlinks');
Expand Down Expand Up @@ -531,11 +532,8 @@ Module._load = function(request, parent, isMain) {
return cachedModule.exports;
}

const mod = NativeModule.map.get(filename);
if (mod && mod.canBeRequiredByUsers) {
debug('load native module %s', request);
return mod.compileForPublicLoader(experimentalModules);
}
const mod = loadNativeModule(filename, request, experimentalModules);
if (mod && mod.canBeRequiredByUsers) return mod.exports;

// Don't call updateChildren(), Module constructor already does.
const module = new Module(filename, parent);
Expand Down Expand Up @@ -741,8 +739,11 @@ function wrapSafe(filename, content) {
// the file.
// Returns exception, if any.
Module.prototype._compile = function(content, filename) {
let moduleURL;
let redirects;
if (manifest) {
const moduleURL = pathToFileURL(filename);
moduleURL = pathToFileURL(filename);
redirects = manifest.getRedirects(moduleURL);
manifest.assertIntegrity(moduleURL, content);
}

Expand All @@ -766,7 +767,7 @@ Module.prototype._compile = function(content, filename) {
}
}
const dirname = path.dirname(filename);
const require = makeRequireFunction(this);
const require = makeRequireFunction(this, redirects);
var result;
const exports = this.exports;
const thisValue = exports;
Expand Down Expand Up @@ -855,7 +856,7 @@ function createRequireFromPath(filename) {
m.filename = proxyPath;

m.paths = Module._nodeModulePaths(m.path);
return makeRequireFunction(m);
return makeRequireFunction(m, null);
}

Module.createRequireFromPath = deprecate(
Expand Down
7 changes: 3 additions & 4 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ const {
StringPrototype
} = primordials;

const { NativeModule } = require('internal/bootstrap/loaders');
const {
stripBOM
stripBOM,
loadNativeModule
} = require('internal/modules/cjs/helpers');
const CJSModule = require('internal/modules/cjs/loader').Module;
const internalURLModule = require('internal/url');
Expand Down Expand Up @@ -93,11 +93,10 @@ translators.set('builtin', async function builtinStrategy(url) {
debug(`Translating BuiltinModule ${url}`);
// Slice 'node:' scheme
const id = url.slice(5);
const module = NativeModule.map.get(id);
const module = loadNativeModule(id, url, true);
if (!module) {
throw new ERR_UNKNOWN_BUILTIN_MODULE(id);
}
module.compileForPublicLoader(true);
return createDynamicModule(
[], [...module.exportKeys, 'default'], url, (reflect) => {
debug(`Loading BuiltinModule ${url}`);
Expand Down
Loading