Skip to content

Commit

Permalink
Fixed security vulnerability
Browse files Browse the repository at this point in the history
Thanks to Snyk.io for responsible disclosure
  • Loading branch information
alexindigo committed Jun 23, 2019
1 parent d9f59ee commit 6eccb2f
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 9 deletions.
1 change: 1 addition & 0 deletions .nvmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
10
41 changes: 34 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ and exposes hooks and custom adapters for more control and greater flexibility.

[![Readme](https://img.shields.io/badge/readme-tested-brightgreen.svg?style=flat)](https://www.npmjs.com/package/reamde)

| compression | size |
| :--------------- | ------: |
| deeply.js | 15.6 kB |
| deeply.min.js | 5.11 kB |
| deeply.min.js.gz | 1.53 kB |
| compression | size |
| :--------------- | -------: |
| deeply.js | 16.35 kB |
| deeply.min.js | 5.36 kB |
| deeply.min.js.gz | 1.63 kB |


## Table of Contents
Expand All @@ -27,6 +27,7 @@ and exposes hooks and custom adapters for more control and greater flexibility.
- [Install](#install)
- [Examples](#examples)
- [Merging](#merging)
- [Security concerns](#security-concerns)
- [Cloning](#cloning)
- [Arrays Custom Merging](#arrays-custom-merging)
- [Default Behavior](#default-behavior)
Expand All @@ -37,7 +38,8 @@ and exposes hooks and custom adapters for more control and greater flexibility.
- [Cloning Functions](#cloning-functions)
- [Cloning Prototype Chain](#cloning-prototype-chain)
- [Extend Original Function Prototype](#extend-original-function-prototype)
- [Custom hooks](#custom-hooks)
- [Custom flags and hooks](#custom-flags-and-hooks)
- [`allowDangerousObjectKeys`](#allowdangerousobjectkeys)
- [`useCustomAdapters`](#usecustomadapters)
- [`useCustomTypeOf`](#usecustomtypeof)
- [Mutable Operations](#mutable-operations)
Expand Down Expand Up @@ -71,6 +73,22 @@ var result = merge({a: {a1: 1}}, {a: {a2: 2}}, {b: {b3: 3}});
assert.equal(result, {a: {a1: 1, a2: 2}, b: {b3: 3}});
```

#### Security concerns

Due to Prototype Pollution security vulnerability concerns, default behavior of when merging objects is to skip unsafe keys, like `__proto__`, please refer to the [test/compatability.js](test/compatability.js) file for code examples.

If there is a use case where such behavior is desired, pass `allowDangerousObjectKeys` flag to the context to skip keys safety checks.

```javascript
var merge = require('deeply');
var result;

var context = { allowDangerousObjectKeys: merge.behaviors.allowDangerousObjectKeys };

result = merge.call(context, {}, JSON.parse('{"__proto__": {"a0": true}}'));
// end of the world, cats live with dogs...
```

### Cloning

As degenerated case of merging one object on itself, it's possible to use deeply as deep clone function.
Expand Down Expand Up @@ -360,7 +378,16 @@ assert.equal(s1 instanceof Subj, true);
assert.equal(s2 instanceof Subj, true);
```

### Custom hooks
### Custom flags and hooks

#### `allowDangerousObjectKeys`

As shown in (Security Concerns)[#security-concerns] section,
you can skip safety checks for unsafe object keys (e.g. `__proto__`) by passing `allowDangerousObjectKeys` flag to the context.

```js
merge.call({ allowDangerousObjectKeys: merge.behaviors.allowDangerousObjectKeys }, {}, JSON.parse('{"__proto__": {"a0": true}}'));
```

#### `useCustomAdapters`

Expand Down
3 changes: 2 additions & 1 deletion adapters/object.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ module.exports = objectAdapter;
function objectAdapter(to, from, merge)
{
// transfer source values
reduceObject(to, from, merge);
// pass context down the line, to allow behavior overrides
reduceObject.call(this, to, from, merge);

return to;
}
3 changes: 3 additions & 0 deletions flags.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// list of available flags
module.exports =
{
// allow (original) unsafe behavior of merge all properties, including ones like `__proto__`
allowDangerousObjectKeys: 'deeply:allowDangerousObjectKeys:' + Math.random(),

// to prevent (reduce chance of) accidental leaking of the global variables into runtime flags
useCustomAdapters: 'deeply:useCustomAdapters:' + Math.random(),
useCustomTypeOf: 'deeply:useCustomTypeOf:' + Math.random()
Expand Down
20 changes: 20 additions & 0 deletions lib/reduce_object.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
var behaviors = require('../flags.js');

// Public API
module.exports = reduceObject;

Expand All @@ -13,13 +15,31 @@ module.exports = reduceObject;
*/
function reduceObject(target, source, merge)
{
var context = this;

// clone exposed properties
Object.keys(source).reduce(function(acc, key)
{
if (context.allowDangerousObjectKeys !== behaviors.allowDangerousObjectKeys && isUnsafeKey(key))
{
return acc;
}

acc[key] = merge(acc[key], source[key]);

return acc;
}, target);

return target;
}


/**
* Checks if provide key is unsafe to use within object
*
* @param {string} key - object key to check against
* @returns {boolean} - `true` if key is unsafe to use (e.g. __proto__), `false` otherwise
*/
function isUnsafeKey(key) {
return ['__proto__'].indexOf(key) != -1;
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"description": "A toolkit for deep structure manipulations, provides deep merge/clone functionality out of the box, and exposes hooks and custom adapters for more control and greater flexibility.",
"main": "index.js",
"scripts": {
"clean": "rimraf coverage",
"clean": "rimraf .nyc_output coverage",
"lint": "eslint *.js adapters/*.js test/*.js",
"test": "nyc --reporter=json tape test/test-*.js | tap-spec",
"browser": "browserify -t browserify-istanbul test/test-compatibility.js | obake --coverage | tap-spec",
Expand Down
31 changes: 31 additions & 0 deletions test/test-compatibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,27 @@ var inout = [
, {in: ['ABC', 25, true], out: true}
, {in: [{a: 13}], out: {a: 13}}
, {in: [now], out: now}

// security
, {
in: [{}, JSON.parse('{"a": 1, "__proto__": {"a0": true}}')],
out: {a: 1},
// don't expect to see `a0` on object prototype
afterTest: function() { return !('a0' in {}); }
}
, {
in: [{}, JSON.parse('{"a": 1, "__proto__": {"a0": true}}')],
out: {a: 1},
// expect to see `a0` on object prototype
// with allowed dangerous keys flag
allowDangerousObjectKeys: true,
afterTest: function() {
var affected = ({})['a0'] === true;
// clean up proto pollution
delete ({}).__proto__['a0'];
return affected;
}
}
];

/**
Expand Down Expand Up @@ -213,6 +234,7 @@ test('merge', function test_deep_merge(t)

// extra sub tests
expectedTestsNum += inout.filter(function(pair){ return pair.in.length == 1 && typeof pair.modify == 'function'; }).length;
expectedTestsNum += inout.filter(function(pair){ return typeof pair.afterTest == 'function'; }).length;

t.plan(expectedTestsNum);

Expand All @@ -231,6 +253,11 @@ test('merge', function test_deep_merge(t)
context = pair.customTypeOf;
context['useCustomTypeOf'] = deeply.behaviors.useCustomTypeOf;
}
else if (pair.allowDangerousObjectKeys)
{
context = {};
context['allowDangerousObjectKeys'] = deeply.behaviors.allowDangerousObjectKeys;
}

// default - immutable

Expand All @@ -244,5 +271,9 @@ test('merge', function test_deep_merge(t)
// check that object's don't equal after modification
t.notDeepEqual(stringify(res), stringify(pair.in[0]), 'modified '+stringify(pair.in[0])+' should not be equal to '+stringify(res));
}

if (typeof pair.afterTest == 'function') {
t.ok(pair.afterTest(res), 'afterTest run should return truthy result');
}
});
});

0 comments on commit 6eccb2f

Please sign in to comment.