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: minor perf opts and cleanup #29322

Closed
wants to merge 1 commit into from
Closed
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
2 changes: 1 addition & 1 deletion lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1129,7 +1129,7 @@ E('ERR_SOCKET_DGRAM_IS_CONNECTED', 'Already connected', Error);
E('ERR_SOCKET_DGRAM_NOT_CONNECTED', 'Not connected', Error);
E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running', Error);
E('ERR_SRI_PARSE',
'Subresource Integrity string %s had an unexpected at %d',
'Subresource Integrity string %j had an unexpected %j at position %d',
SyntaxError);
E('ERR_STREAM_ALREADY_FINISHED',
'Cannot call %s after a stream was finished',
Expand Down
44 changes: 25 additions & 19 deletions lib/internal/modules/cjs/helpers.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

const { Object } = primordials;
const { Object, SafeMap } = primordials;
const {
ERR_MANIFEST_DEPENDENCY_MISSING,
ERR_UNKNOWN_BUILTIN_MODULE
Expand Down Expand Up @@ -28,34 +28,40 @@ function loadNativeModule(filename, request, experimentalModules) {
// Invoke with makeRequireFunction(module) where |module| is the Module object
// to use as the context for the require() function.
// Use redirects to set up a mapping from a policy and restrict dependencies
const urlToFileCache = new SafeMap();
function makeRequireFunction(mod, redirects) {
const Module = mod.constructor;

let require;
if (redirects) {
const { map, reaction } = redirects;
const { resolve, reaction } = redirects;
const id = mod.filename || mod.id;
require = function require(path) {
let missing = true;
if (map === true) {
const destination = resolve(path);
if (destination === 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));
} else if (destination) {
const href = destination.href;
if (destination.protocol === 'node:') {
const specifier = destination.pathname;
const mod = loadNativeModule(
specifier,
href,
experimentalModules);
if (mod && mod.canBeRequiredByUsers) {
return mod.exports;
}
throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier);
} else if (destination.protocol === 'file:') {
let filepath;
if (urlToFileCache.has(href)) {
filepath = urlToFileCache.get(href);
} else {
filepath = fileURLToPath(destination);
urlToFileCache.set(href, filepath);
}
return mod.require(filepath);
}
}
if (missing) {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ Module.prototype._compile = function(content, filename) {
let redirects;
if (manifest) {
moduleURL = pathToFileURL(filename);
redirects = manifest.getRedirects(moduleURL);
redirects = manifest.getRedirector(moduleURL);
manifest.assertIntegrity(moduleURL, content);
}

Expand Down
14 changes: 11 additions & 3 deletions lib/internal/per_context/primordials.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// by the native module compiler.

const ReflectApply = Reflect.apply;
const ReflectConstruct = Reflect.construct;

// This function is borrowed from the function with the same name on V8 Extras'
// `utils` object. V8 implements Reflect.apply very efficiently in conjunction
Expand Down Expand Up @@ -102,10 +103,17 @@ primordials.SafePromise = makeSafe(
'String',
'Symbol',
].forEach((name) => {
const target = primordials[name] = Object.create(null);
copyProps(global[name], target);
const original = global[name];
const target = primordials[name] = Object.setPrototypeOf({
[name]: function(...args) {
return new.target ?
ReflectConstruct(original, args, new.target) :
ReflectApply(original, this, args);
}
}[name], null);
copyProps(original, target);
const proto = primordials[name + 'Prototype'] = Object.create(null);
copyPrototype(global[name].prototype, proto);
copyPrototype(original.prototype, proto);
});

Object.setPrototypeOf(primordials, null);
Expand Down
137 changes: 86 additions & 51 deletions lib/internal/policy/manifest.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
'use strict';

const {
SafeMap,
SafeWeakMap,
Map,
MapPrototype,
Object,
RegExpPrototype,
SafeMap,
uncurryThis
} = primordials;
const {
Expand All @@ -21,16 +22,13 @@ const debug = require('internal/util/debuglog').debuglog('policy');
const SRI = require('internal/policy/sri');
const crypto = require('crypto');
const { Buffer } = require('buffer');
const { URL } = require('url');
const { URL } = require('internal/url');
const { createHash, timingSafeEqual } = crypto;
const HashUpdate = uncurryThis(crypto.Hash.prototype.update);
const HashDigest = uncurryThis(crypto.Hash.prototype.digest);
const BufferEquals = uncurryThis(Buffer.prototype.equals);
const BufferToString = uncurryThis(Buffer.prototype.toString);
const { entries } = Object;
const kIntegrities = new SafeWeakMap();
const kDependencies = new SafeWeakMap();
const kReactions = new SafeWeakMap();
const kRelativeURLStringPattern = /^\.{0,2}\//;
const { getOptionValue } = require('internal/options');
const shouldAbortOnUncaughtException =
Expand All @@ -54,13 +52,12 @@ function REACTION_LOG(error) {
}

class Manifest {
#integrities = new SafeMap();
#dependencies = new SafeMap();
#reaction = null;
constructor(obj, manifestURL) {
const integrities = {
__proto__: null,
};
const dependencies = {
__proto__: null,
};
const integrities = this.#integrities;
const dependencies = this.#dependencies;
let reaction = REACTION_THROW;

if (obj.onerror) {
Expand All @@ -75,23 +72,33 @@ class Manifest {
}
}

kReactions.set(this, reaction);
this.#reaction = reaction;
const manifestEntries = entries(obj.resources);

const parsedURLs = new SafeMap();
for (var i = 0; i < manifestEntries.length; i++) {
let url = manifestEntries[i][0];
const originalURL = url;
if (RegExpPrototype.test(kRelativeURLStringPattern, url)) {
url = new URL(url, manifestURL).href;
let resourceHREF = manifestEntries[i][0];
const originalHREF = resourceHREF;
let resourceURL;
if (parsedURLs.has(resourceHREF)) {
resourceURL = parsedURLs.get(resourceHREF);
resourceHREF = resourceURL.href;
} else if (
RegExpPrototype.test(kRelativeURLStringPattern, resourceHREF)
) {
resourceURL = new URL(resourceHREF, manifestURL);
resourceHREF = resourceURL.href;
parsedURLs.set(originalHREF, resourceURL);
parsedURLs.set(resourceHREF, resourceURL);
}
let integrity = manifestEntries[i][1].integrity;
if (!integrity) integrity = null;
if (integrity != null) {
debug(`Manifest contains integrity for url ${originalURL}`);
debug(`Manifest contains integrity for url ${originalHREF}`);
if (typeof integrity === 'string') {
const sri = Object.freeze(SRI.parse(integrity));
if (url in integrities) {
const old = integrities[url];
if (integrities.has(resourceHREF)) {
const old = integrities.get(resourceHREF);
let mismatch = false;

if (old.length !== sri.length) {
Expand All @@ -112,14 +119,16 @@ class Manifest {
}

if (mismatch) {
throw new ERR_MANIFEST_INTEGRITY_MISMATCH(url);
throw new ERR_MANIFEST_INTEGRITY_MISMATCH(resourceURL);
}
}
integrities[url] = sri;
integrities.set(resourceHREF, sri);
} else if (integrity === true) {
integrities[url] = true;
integrities.set(resourceHREF, true);
} else {
throw new ERR_MANIFEST_INVALID_RESOURCE_FIELD(url, 'integrity');
throw new ERR_MANIFEST_INVALID_RESOURCE_FIELD(
resourceHREF,
'integrity');
}
}

Expand All @@ -128,50 +137,72 @@ class Manifest {
dependencyMap = {};
}
if (typeof dependencyMap === 'object' && !Array.isArray(dependencyMap)) {
dependencies[url] = new SafeMap(Object.entries(dependencyMap).map(
([ from, to ]) => {
/**
* @returns {true | URL}
*/
const dependencyRedirectList = (toSpecifier) => {
if (toSpecifier in dependencyMap !== true) {
return null;
} else {
const to = dependencyMap[toSpecifier];
if (to === true) {
return [from, to];
return true;
}
if (canBeRequiredByUsers(to)) {
return [from, `node:${to}`];
if (parsedURLs.has(to)) {
return parsedURLs.get(to);
} else if (canBeRequiredByUsers(to)) {
const href = `node:${to}`;
const resolvedURL = new URL(href);
parsedURLs.set(to, resolvedURL);
parsedURLs.set(href, resolvedURL);
return resolvedURL;
} else if (RegExpPrototype.test(kRelativeURLStringPattern, to)) {
return [from, new URL(to, manifestURL).href];
const resolvedURL = new URL(to, manifestURL);
const href = resourceURL.href;
parsedURLs.set(to, resolvedURL);
parsedURLs.set(href, resolvedURL);
return resolvedURL;
}
return [from, new URL(to).href];
})
);
const resolvedURL = new URL(to);
const href = resourceURL.href;
parsedURLs.set(to, resolvedURL);
parsedURLs.set(href, resolvedURL);
return resolvedURL;
}
};
dependencies.set(resourceHREF, dependencyRedirectList);
} else if (dependencyMap === true) {
dependencies[url] = true;
const arbitraryDependencies = () => true;
dependencies.set(resourceHREF, arbitraryDependencies);
} else {
throw new ERR_MANIFEST_INVALID_RESOURCE_FIELD(url, 'dependencies');
throw new ERR_MANIFEST_INVALID_RESOURCE_FIELD(
resourceHREF,
'dependencies');
}
}
Object.freeze(integrities);
kIntegrities.set(this, integrities);
Object.freeze(dependencies);
kDependencies.set(this, dependencies);
Object.freeze(this);
}

getRedirects(requester) {
const dependencies = kDependencies.get(this);
if (dependencies && requester in dependencies) {
getRedirector(requester) {
requester = `${requester}`;
const dependencies = this.#dependencies;
if (dependencies.has(requester)) {
return {
map: dependencies[requester],
reaction: kReactions.get(this)
resolve: (to) => dependencies.get(requester)(`${to}`),
reaction: this.#reaction
};
}
return null;
}

assertIntegrity(url, content) {
debug(`Checking integrity of ${url}`);
const integrities = kIntegrities.get(this);
const realIntegrities = new SafeMap();
const href = `${url}`;
debug(`Checking integrity of ${href}`);
const integrities = this.#integrities;
const realIntegrities = new Map();

if (integrities && url in integrities) {
const integrityEntries = integrities[url];
if (integrities.has(href)) {
const integrityEntries = integrities.get(href);
if (integrityEntries === true) return true;
// Avoid clobbered Symbol.iterator
for (var i = 0; i < integrityEntries.length; i++) {
Expand All @@ -186,11 +217,15 @@ class Manifest {
timingSafeEqual(digest, expected)) {
return true;
}
realIntegrities.set(algorithm, BufferToString(digest, 'base64'));
MapPrototype.set(
realIntegrities,
algorithm,
BufferToString(digest, 'base64')
);
}
}
const error = new ERR_MANIFEST_ASSERT_INTEGRITY(url, realIntegrities);
kReactions.get(this)(error);
this.#reaction(error);
}
}

Expand Down
12 changes: 6 additions & 6 deletions lib/internal/policy/sri.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,16 @@ const {
} = require('internal/errors').codes;
const kWSP = '[\\x20\\x09]';
const kVCHAR = '[\\x21-\\x7E]';
const kHASH_ALGO = 'sha256|sha384|sha512';
const kHASH_ALGO = 'sha(?:256|384|512)';
// Base64
const kHASH_VALUE = '[A-Za-z0-9+/]+[=]{0,2}';
const kHASH_EXPRESSION = `(${kHASH_ALGO})-(${kHASH_VALUE})`;
const kOPTION_EXPRESSION = `(${kVCHAR}*)`;
const kHASH_WITH_OPTIONS = `${kHASH_EXPRESSION}(?:[?](${kOPTION_EXPRESSION}))?`;
const kSRIPattern = new RegExp(`(${kWSP}*)(?:${kHASH_WITH_OPTIONS})`, 'g');
const kSRIPattern = RegExp(`(${kWSP}*)(?:${kHASH_WITH_OPTIONS})`, 'g');
const { freeze } = Object;
Object.seal(kSRIPattern);
const kAllWSP = new RegExp(`^${kWSP}*$`);
const kAllWSP = RegExp(`^${kWSP}*$`);
Object.seal(kAllWSP);

const BufferFrom = require('buffer').Buffer.from;
Expand All @@ -34,10 +34,10 @@ const parse = (str) => {
const entries = [];
while (match = RegExpPrototype.exec(kSRIPattern, str)) {
if (match.index !== prevIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be one whole condition with below ?

Copy link
Member Author

Choose a reason for hiding this comment

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

could be yea, i think readability is better as is but if requested to change I could

throw new ERR_SRI_PARSE(str, prevIndex);
throw new ERR_SRI_PARSE(str, str.charAt(prevIndex), prevIndex);
}
if (entries.length > 0 && match[1] === '') {
throw new ERR_SRI_PARSE(str, prevIndex);
throw new ERR_SRI_PARSE(str, str.charAt(prevIndex), prevIndex);
}

// Avoid setters being fired
Expand All @@ -56,7 +56,7 @@ const parse = (str) => {

if (prevIndex !== str.length) {
if (!RegExpPrototype.test(kAllWSP, StringPrototype.slice(str, prevIndex))) {
throw new ERR_SRI_PARSE(str, prevIndex);
throw new ERR_SRI_PARSE(str, str.charAt(prevIndex), prevIndex);
}
}
return entries;
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/process/policy.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,6 @@ module.exports = Object.freeze({
},

assertIntegrity(moduleURL, content) {
this.manifest.matchesIntegrity(moduleURL, content);
this.manifest.assertIntegrity(moduleURL, content);
}
});