Skip to content

Commit

Permalink
Added safeguard against unsafe property selection
Browse files Browse the repository at this point in the history
  • Loading branch information
BadIdeaException committed Aug 26, 2024
1 parent fa7cb2a commit 3fccaf3
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 23 deletions.
9 changes: 5 additions & 4 deletions applicators.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ function _perform(select, fn, obj, options) {
const mode = { 'strict': 1, 'lenient': 2 }[options?.mode?.toLowerCase()];


let resolution = select(obj, options?.references, mode);
let resolution = select(obj, options?.references, mode, !options?.allowUnsafe);

if (options?.unique) {
console.warn('Using options.unique is deprecated. Use the :unique meta property instead.');
Expand Down Expand Up @@ -42,10 +42,9 @@ function _perform(select, fn, obj, options) {
resolution[k].selection = resolution[k].selection.filter(sel => sel !== undefined);
}
}

for (let item of resolution)
for (let property of item.selection) {
const value = fn(item.target[property], property, item.target);
const value = fn(item.target[property], property, item.target, resolution.root);
// Only assign new value if it is different
// This is important so that read operations will work even on read-only (e.g. frozen) objects
// In read-only mode, NEVER attempt to set a new value. This is important so that dynamically generated properties
Expand Down Expand Up @@ -137,7 +136,9 @@ export function compile(selector) {
* mimics the ordinary rules of selecting object properties in Javascript (where `{}['a'] === undefined`).
* In `strict` mode, any attempt to select a non-existent property immediately results in an error.
* In `lenient` mode, non-existent properties are silently dropped.
* The default mode is `normal`.
* @param {boolean} [options.allowUnsafe=false] Set this to `true` to allow accessing unsafe properties on the target. Unsafe properties are
* `constructor, prototype, __proto__, __defineGetter__, __lookupGetter__, __defineSetter__, __lookupSetter__`. Otherwise, trying to access these will result in an
* error being thrown. See the section on [security considerations](#security-considerations) for why these are considered unsafe.
* @param {Object} [options.references] The values for any references used in the selector.
* @return {*} The results of applying `fn` to all selected properties.
*/
Expand Down
23 changes: 21 additions & 2 deletions applicators.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ describe('perform', function() {
expect(fn).to.have.been.calledWith(obj.a.b2.c);
});

it('should pass value, property, context to the function', function() {
it('should pass value, property, context and root to the function', function() {
const fn = sinon.spy();
const c = obj.a.b1.c;

perform('a.b1.c', fn, obj);

expect(fn).to.have.been.calledWith(c, 'c', sinon.match.same(obj.a.b1));
expect(fn).to.have.been.calledWith(c, 'c', sinon.match.same(obj.a.b1), sinon.match.same(obj));
});

it('should set matching properties to the result value', function() {
Expand Down Expand Up @@ -146,6 +146,25 @@ describe('perform', function() {
expect(spy).to.have.been.calledOnce.and.calledWith(1);
});
});

describe('Selection safety', function() {
const fn = x => x;
const UNSAFE_KEYS = [ 'constructor', 'prototype', '__proto__', '__defineGetter__', '__lookupGetter__', '__defineSetter__', '__lookupSetter__' ];

it('should throw when trying to access a forbidden key', function() {
UNSAFE_KEYS.forEach(evil => expect(perform.bind(null, evil, fn, obj), evil).to.throw(/unsafe/i));
});

it('should allow to access a forbidden key when explicitly opting into unsafe selection', function() {
UNSAFE_KEYS.forEach(evil => {
const target = function() {}; // prototype property exists only on functions, and for the other ones it makes no difference
const op = perform.bind(null, evil, fn, target, { allowUnsafe: true });

expect(op, evil).to.not.throw();
expect(op(), evil).to.equal(target[evil]);
});
});
});
});

describe('get', function() {
Expand Down
23 changes: 14 additions & 9 deletions selector.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 13 additions & 8 deletions selector.peg
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@
const MODE_NORMAL = 0;
const MODE_STRICT = 1;
const MODE_LENIENT = 2;

const PROHIBITED_KEYS = [ 'constructor', 'prototype', '__proto__', '__defineGetter__', '__lookupGetter__', '__defineSetter__', '__lookupSetter__' ]
}

Union_Selector = selectors:Selector|1.., _* Union_Operator _*| {
return Object.assign(function union(obj, references, mode) {
return Object.assign(function union(obj, references, mode, safe) {

// For some reason, this is WAY faster than doing selectors.flatMap():
let result = selectors[0](obj, references, mode); // This is safe, because the grammar guarantees there is at least one selector
for (let i = 1; i < selectors.length; i++)
result = result.concat(selectors[i](obj, references, mode));
let result = selectors[0](obj, references, mode, safe); // This is safe, because the grammar guarantees there is at least one selector
for (let i = 1; i < selectors.length; i++)
result = result.concat(selectors[i](obj, references, mode, safe));
return result;
}, {
// The selector is ambiguous if it is a union of more than one selectors,
Expand All @@ -33,12 +36,12 @@ Selector
// Return value is the function select, but we will set some meta-properties of the selector on it:
// - Whether or not the selector is ambiguous
// - The raw source text of the selector
return Object.assign(function select(obj, references, mode) {
return Object.assign(function select(obj, references, mode, safe) {
const resolution = [
{ target: obj, selection: [] }
]
resolution.root = obj;
selector?.forEach(element => element(resolution, references, mode));
selector?.forEach(element => element(resolution, references, mode, safe));
return resolution;
}, {
// The selector as a whole is ambiguous if at least one of its constituents is ambiguous (multi-valued)
Expand Down Expand Up @@ -137,7 +140,7 @@ Property_descriptor
Property_name
= regex:Property_name_with_wildcard {
// Wildcard selectors are always ambiguous
return Object.assign(function selectByRegex(resolution, references, mode) {
return Object.assign(function selectByRegex(resolution, references, mode, safe) {
for (let item of resolution) {
if (mode === MODE_LENIENT && item.target == null)
item.selection = [];
Expand All @@ -152,7 +155,9 @@ Property_name
}, { ambiguous: true });
}
/ name:Identifier {
return function selectByName(resolution) {
return function selectByName(resolution, references, mode, safe) {
if (safe && PROHIBITED_KEYS.includes(name))
throw new Error(`Unsafe access on ${resolution.root}: tried to access prohibited key ${name}`);
resolution.forEach(item => item.selection = [ name ]);
}
}
Expand Down

0 comments on commit 3fccaf3

Please sign in to comment.