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.

Sign up for GitHub

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

Jump to bottom

fix: remove spread opts and toString of integrity #71

Closed
wants to merge 4 commits into from
Closed

fix: remove spread opts and toString of integrity #71

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3743043
perf: remove spread of defaultOpts
H4ad Apr 1, 2023
00378da
perf: faster toString for integrity
H4ad Apr 1, 2023
d54207b
perf: faster stream verification
H4ad Apr 1, 2023
694f7ed
refactor: use more optional chaining instead &&
H4ad Apr 1, 2023
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
119 changes: 79 additions & 40 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
const crypto = require('crypto')
const MiniPass = require('minipass')

const SPEC_ALGORITHMS = ['sha256', 'sha384', 'sha512']
const SPEC_ALGORITHMS = {
'sha256': true,
'sha384': true,
'sha512': true
};

// TODO: this should really be a hardcoded list of algorithms we support,
// rather than [a-z0-9].
Expand All @@ -22,8 +26,6 @@ const defaultOpts = {
strict: false,
}

const ssriOpts = (opts = {}) => ({ ...defaultOpts, ...opts })

const getOptString = options => !options || !options.length
? ''
: `?${options.join('?')}`
Expand All @@ -44,7 +46,7 @@ class IntegrityStream extends MiniPass {
this[_getOptions]()

// options used for calculating stream. can't be changed.
const { algorithms = defaultOpts.algorithms } = opts
const algorithms = opts && opts.algorithms || defaultOpts.algorithms
this.algorithms = Array.from(
new Set(algorithms.concat(this.algorithm ? [this.algorithm] : []))
)
Expand Down Expand Up @@ -141,8 +143,7 @@ class Hash {
}

constructor (hash, opts) {
opts = ssriOpts(opts)
const strict = !!opts.strict
const strict = opts && opts.strict
this.source = hash.trim()

// set default values so that we make V8 happy to
Expand All @@ -161,7 +162,7 @@ class Hash {
if (!match) {
return
}
if (strict && !SPEC_ALGORITHMS.some(a => a === match[1])) {
if (strict && SPEC_ALGORITHMS[match[1]] !== true) {
Copy link
Member

Choose a reason for hiding this comment

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

We can keep SPEC_ALGORITHMS as an array and use .includes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using includes is little bit slower than just the property index, do you have some reason to keep it as an array?

Copy link
Member

Choose a reason for hiding this comment

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

It's a balance against performance and developer experience. I think "a little bit" slower is ok here given that the vast majority of an npm install is disk and io bound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a mistake, using includes is faster if we don't know the value:

const Benchmark = require('benchmark');
const suite = new Benchmark.Suite();

const SPEC_ALGORITHMS = {
  sha256: true,
  sha384: true,
  sha512: true,
};

const SPEC_ALGORITHMS_ARRAY = Object.keys(SPEC_ALGORITHMS);
const randomAndUnkown = [...SPEC_ALGORITHMS_ARRAY, 'test'];

suite
  .add('includes', function () {
    const random = randomAndUnkown[Math.floor(Math.random() * randomAndUnkown.length)];

    const r = SPEC_ALGORITHMS_ARRAY.includes(random);
  })
  .add('index access', function () {
    const random = randomAndUnkown[Math.floor(Math.random() * randomAndUnkown.length)];

    const r = SPEC_ALGORITHMS[random] === true;
  })
  .on('cycle', function (event) {
    console.log(String(event.target));
  })
  .run({ async: false });

Perf:

includes x 83,638,547 ops/sec ±1.96% (92 runs sampled)
index access x 28,349,129 ops/sec ±2.07% (90 runs sampled)

My assumptions not always are good, I forgot that random access for an object is slower.

I will turn back to includes.

return
}
this.algorithm = match[1]
Expand All @@ -182,14 +183,13 @@ class Hash {
}

toString (opts) {
opts = ssriOpts(opts)
if (opts.strict) {
if (opts && opts.strict) {
// Strict mode enforces the standard as close to the foot of the
// letter as it can.
if (!(
// The spec has very restricted productions for algorithms.
// https://www.w3.org/TR/CSP2/#source-list-syntax
SPEC_ALGORITHMS.some(x => x === this.algorithm) &&
SPEC_ALGORITHMS[this.algorithm] === true &&
// Usually, if someone insists on using a "different" base64, we
// leave it as-is, since there's multiple standards, and the
// specified is not a URL-safe variant.
Expand All @@ -210,6 +210,41 @@ class Hash {
}
}

function integrityHashToString(toString, sep, opts, hashes) {
if (!hashes || hashes.length === 0)
return toString;

const toStringIsNotEmpty = toString !== '';

let shouldAddFirstSep = false;
let complement = '';

const lastIndex = hashes.length - 1

for (let i = 0; i < lastIndex; i++) {
const hashString = Hash.prototype.toString.call(hashes[i], opts)

if (hashString) {
shouldAddFirstSep = true;

complement += hashString
complement += sep
}
}

const finalHashString = Hash.prototype.toString.call(hashes[lastIndex], opts)

if (finalHashString) {
shouldAddFirstSep = true;
complement += finalHashString
}

if (toStringIsNotEmpty && shouldAddFirstSep)
return toString + sep + complement;

return toString + complement
}

class Integrity {
get isIntegrity () {
return true
Expand All @@ -224,21 +259,34 @@ class Integrity {
}

toString (opts) {
opts = ssriOpts(opts)
let sep = opts.sep || ' '
if (opts.strict) {
let sep = opts && opts.sep || ' '
let toString = '';

if (opts && opts.strict) {
// Entries must be separated by whitespace, according to spec.
sep = sep.replace(/\S+/g, ' ')

if (this.sha512 && this.sha512.length > 0) {
toString = integrityHashToString(toString, sep,opts, this['sha512'])
}

if (this.sha384 && this.sha384.length > 0) {
toString = integrityHashToString(toString, sep,opts, this['sha384'])
}

if (this.sha256 && this.sha256.length > 0) {
toString = integrityHashToString(toString,sep, opts, this['sha256'])
}
} else {
for (const hash of Object.keys(this)) {
toString = integrityHashToString(toString, sep,opts, this[hash])
}
}
return Object.keys(this).map(k => {
return this[k].map(hash => {
return Hash.prototype.toString.call(hash, opts)
}).filter(x => x.length).join(sep)
}).filter(x => x.length).join(sep)

return toString;
}

concat (integrity, opts) {
opts = ssriOpts(opts)
const other = typeof integrity === 'string'
? integrity
: stringify(integrity, opts)
Expand All @@ -252,7 +300,6 @@ class Integrity {
// add additional hashes to an integrity value, but prevent
// *changing* an existing integrity hash.
merge (integrity, opts) {
opts = ssriOpts(opts)
const other = parse(integrity, opts)
for (const algo in other) {
if (this[algo]) {
Expand All @@ -268,7 +315,6 @@ class Integrity {
}

match (integrity, opts) {
opts = ssriOpts(opts)
const other = parse(integrity, opts)
if (!other) {
return false
Expand All @@ -286,8 +332,7 @@ class Integrity {
}

pickAlgorithm (opts) {
opts = ssriOpts(opts)
const pickAlgorithm = opts.pickAlgorithm
const pickAlgorithm = opts && opts.pickAlgorithm || defaultOpts.pickAlgorithm;
const keys = Object.keys(this)
return keys.reduce((acc, algo) => {
return pickAlgorithm(acc, algo) || acc
Expand All @@ -300,7 +345,6 @@ function parse (sri, opts) {
if (!sri) {
return null
}
opts = ssriOpts(opts)
if (typeof sri === 'string') {
return _parse(sri, opts)
} else if (sri.algorithm && sri.digest) {
Expand All @@ -315,7 +359,7 @@ function parse (sri, opts) {
function _parse (integrity, opts) {
// 3.4.3. Parse metadata
// https://w3c.github.io/webappsec-subresource-integrity/#parse-metadata
if (opts.single) {
if (opts && opts.single) {
return new Hash(integrity, opts)
}
const hashes = integrity.trim().split(/\s+/).reduce((acc, string) => {
Expand All @@ -334,7 +378,6 @@ function _parse (integrity, opts) {

module.exports.stringify = stringify
function stringify (obj, opts) {
opts = ssriOpts(opts)
if (obj.algorithm && obj.digest) {
return Hash.prototype.toString.call(obj, opts)
} else if (typeof obj === 'string') {
Expand All @@ -346,8 +389,7 @@ function stringify (obj, opts) {

module.exports.fromHex = fromHex
function fromHex (hexDigest, algorithm, opts) {
opts = ssriOpts(opts)
const optString = getOptString(opts.options)
const optString = getOptString(opts && opts.options)
return parse(
`${algorithm}-${
Buffer.from(hexDigest, 'hex').toString('base64')
Expand All @@ -357,9 +399,8 @@ function fromHex (hexDigest, algorithm, opts) {

module.exports.fromData = fromData
function fromData (data, opts) {
opts = ssriOpts(opts)
const algorithms = opts.algorithms
const optString = getOptString(opts.options)
const algorithms = opts && opts.algorithms || defaultOpts.algorithms
const optString = getOptString(opts && opts.options)
return algorithms.reduce((acc, algo) => {
const digest = crypto.createHash(algo).update(data).digest('base64')
const hash = new Hash(
Expand All @@ -382,7 +423,6 @@ function fromData (data, opts) {

module.exports.fromStream = fromStream
function fromStream (stream, opts) {
opts = ssriOpts(opts)
const istream = integrityStream(opts)
return new Promise((resolve, reject) => {
stream.pipe(istream)
Expand All @@ -399,10 +439,9 @@ function fromStream (stream, opts) {

module.exports.checkData = checkData
function checkData (data, sri, opts) {
opts = ssriOpts(opts)
sri = parse(sri, opts)
if (!sri || !Object.keys(sri).length) {
if (opts.error) {
if (opts && opts.error) {
throw Object.assign(
new Error('No valid integrity hashes to check against'), {
code: 'EINTEGRITY',
Expand All @@ -416,7 +455,8 @@ function checkData (data, sri, opts) {
const digest = crypto.createHash(algorithm).update(data).digest('base64')
const newSri = parse({ algorithm, digest })
const match = newSri.match(sri, opts)
if (match || !opts.error) {
opts = opts || Object.create(null)
if (match || !(opts && opts.error)) {
return match
} else if (typeof opts.size === 'number' && (data.length !== opts.size)) {
/* eslint-disable-next-line max-len */
Expand All @@ -440,7 +480,7 @@ function checkData (data, sri, opts) {

module.exports.checkStream = checkStream
function checkStream (stream, sri, opts) {
opts = ssriOpts(opts)
opts = opts || Object.create(null)
opts.integrity = sri
sri = parse(sri, opts)
if (!sri || !Object.keys(sri).length) {
Expand All @@ -465,15 +505,14 @@ function checkStream (stream, sri, opts) {
}

module.exports.integrityStream = integrityStream
function integrityStream (opts = {}) {
function integrityStream (opts = Object.create(null)) {
return new IntegrityStream(opts)
}

module.exports.create = createIntegrity
function createIntegrity (opts) {
opts = ssriOpts(opts)
const algorithms = opts.algorithms
const optString = getOptString(opts.options)
const algorithms = opts && opts.algorithms || defaultOpts.algorithms
const optString = getOptString(opts && opts.options)

const hashes = algorithms.map(crypto.createHash)

Expand Down