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

Ban prototype pollution and document it #219

Merged
merged 3 commits into from Mar 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
@@ -1,4 +1,6 @@
language: node_js
node_js: 8
before_script:
- npm install
script:
- npm test && npm run bench
22 changes: 15 additions & 7 deletions README.md
Expand Up @@ -182,42 +182,50 @@ else {

## API

#### `jsonpatch.applyPatch<T>(document: any, patch: Operation[], validateOperation: Boolean | Function = false): OperationResult<T>[]`
#### `function applyPatch<T>(document: T, patch: Operation[], validateOperation?: boolean | Validator<T>, mutateDocument: boolean = true, banPrototypeModifications: boolean = true): PatchResult<T>`

Applies `patch` array on `obj`.

- `document` The document to patch
- `patch` a JSON-Patch array of operations to apply
- `validateOperation` Boolean for whether to validate each operation with our default validator, or to pass a validator callback
- `mutateDocument` Whether to mutate the original document or clone it before applying
- `banPrototypeModifications` Whether to ban modifications to `__proto__`, defaults to `true`.

An invalid patch results in throwing an error (see `jsonpatch.validate` for more information about the error object).

It modifies the `document` object and `patch` - it gets the values by reference.
If you would like to avoid touching your values, clone them: `jsonpatch.applyPatch(document, jsonpatch.deepClone(patch))`.
If you would like to avoid touching your `patch` array values, clone them: `jsonpatch.applyPatch(document, jsonpatch.deepClone(patch))`.

Returns an array of [`OperationResult`](#operationresult-type) objects - one item for each item in `patches`, each item is an object `{newDocument: any, test?: boolean, removed?: any}`.

* `test` - boolean result of the test
* `remove`, `replace` and `move` - original object that has been removed
* `add` (only when adding to an array) - index at which item has been inserted (useful when using `-` alias)

** Note: It throws `TEST_OPERATION_FAILED` error if `test` operation fails. **

** Note II: the returned array has `newDocument` property that you can use as the final state of the patched document **.
- ** Note: It throws `TEST_OPERATION_FAILED` error if `test` operation fails. **
- ** Note II: the returned array has `newDocument` property that you can use as the final state of the patched document **.
- ** Note III: By default, when `banPrototypeModifications` is `true`, this method throws a `TypeError` when you attempt to modify an object's prototype.

- See [Validation notes](#validation-notes).

#### `applyOperation<T>(document: any, operation: Operation, validateOperation: <Boolean | Function> = false, mutateDocument = true): OperationResult<T>`
#### `function applyOperation<T>(document: T, operation: Operation, validateOperation: boolean | Validator<T> = false, mutateDocument: boolean = true, banPrototypeModifications: boolean = true): OperationResult<T>`

Applies single operation object `operation` on `document`.

- `document` The document to patch
- `operation` The operation to apply
- `validateOperation` Whether to validate the operation, or to pass a validator callback
- `mutateDocument` Whether to mutate the original document or clone it before applying
- `banPrototypeModifications` Whether to ban modifications to `__proto__`, defaults to `true`.

It modifies the `document` object and `operation` - it gets the values by reference.
If you would like to avoid touching your values, clone them: `jsonpatch.applyOperation(document, jsonpatch.deepClone(operation))`.

Returns an [`OperationResult`](#operationresult-type) object `{newDocument: any, test?: boolean, removed?: any}`.

** Note: It throws `TEST_OPERATION_FAILED` error if `test` operation fails. **
- ** Note: It throws `TEST_OPERATION_FAILED` error if `test` operation fails. **
- ** Note II: By default, when `banPrototypeModifications` is `true`, this method throws a `TypeError` when you attempt to modify an object's prototype.

- See [Validation notes](#validation-notes).

Expand Down
13 changes: 10 additions & 3 deletions dist/fast-json-patch.js
Expand Up @@ -347,11 +347,13 @@ exports.getValueByPointer = getValueByPointer;
* @param operation The operation to apply
* @param validateOperation `false` is without validation, `true` to use default jsonpatch's validation, or you can pass a `validateOperation` callback to be used for validation.
* @param mutateDocument Whether to mutate the original document or clone it before applying
* @param banPrototypeModifications Whether to ban modifications to `__proto__`, defaults to `true`.
* @return `{newDocument, result}` after the operation
*/
function applyOperation(document, operation, validateOperation, mutateDocument) {
function applyOperation(document, operation, validateOperation, mutateDocument, banPrototypeModifications) {
if (validateOperation === void 0) { validateOperation = false; }
if (mutateDocument === void 0) { mutateDocument = true; }
if (banPrototypeModifications === void 0) { banPrototypeModifications = true; }
if (validateOperation) {
if (typeof validateOperation == 'function') {
validateOperation(operation, 0, document, operation.path);
Expand Down Expand Up @@ -425,6 +427,9 @@ function applyOperation(document, operation, validateOperation, mutateDocument)
}
while (true) {
key = keys[t];
if (banPrototypeModifications && key == '__proto__') {
throw new TypeError('JSON-Patch: modifying `__proto_` prop is banned for security reasons, if this was on purpose, please set `banPrototypeModifications` flag false and pass it to this function. More info in fast-json-patch README');
}
if (validateOperation) {
if (existingPathFragment === undefined) {
if (obj[key] === undefined) {
Expand Down Expand Up @@ -490,10 +495,12 @@ exports.applyOperation = applyOperation;
* @param patch The patch to apply
* @param validateOperation `false` is without validation, `true` to use default jsonpatch's validation, or you can pass a `validateOperation` callback to be used for validation.
* @param mutateDocument Whether to mutate the original document or clone it before applying
* @param banPrototypeModifications Whether to ban modifications to `__proto__`, defaults to `true`.
* @return An array of `{newDocument, result}` after the patch
*/
function applyPatch(document, patch, validateOperation, mutateDocument) {
function applyPatch(document, patch, validateOperation, mutateDocument, banPrototypeModifications) {
if (mutateDocument === void 0) { mutateDocument = true; }
if (banPrototypeModifications === void 0) { banPrototypeModifications = true; }
if (validateOperation) {
if (!Array.isArray(patch)) {
throw new exports.JsonPatchError('Patch sequence must be an array', 'SEQUENCE_NOT_AN_ARRAY');
Expand All @@ -504,7 +511,7 @@ function applyPatch(document, patch, validateOperation, mutateDocument) {
}
var results = new Array(patch.length);
for (var i = 0, length_1 = patch.length; i < length_1; i++) {
results[i] = applyOperation(document, patch[i], validateOperation);
results[i] = applyOperation(document, patch[i], validateOperation, true, banPrototypeModifications);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cosmetics,
You can consider explaining why you hard-code one parameter and forward the others, as it caused me to think for a second.
Like, "mutateDocument was already covered for the entire sequence, we will apply operations on cloned document if applicable"

document = results[i].newDocument; // in case root was replaced
}
results.newDocument = document;
Expand Down
2 changes: 1 addition & 1 deletion dist/fast-json-patch.min.js

Large diffs are not rendered by default.

18 changes: 10 additions & 8 deletions src/core.ts
Expand Up @@ -15,11 +15,6 @@ import { PatchError, _deepClone, isInteger, unescapePathComponent, hasUndefined
export const JsonPatchError = PatchError;
export const deepClone = _deepClone;

interface HTMLElement {
attachEvent: Function;
detachEvent: Function;
}

export type Operation = AddOperation<any> | RemoveOperation | ReplaceOperation<any> | MoveOperation | CopyOperation | TestOperation<any> | GetOperation<any>;

export interface Validator<T> {
Expand Down Expand Up @@ -193,9 +188,10 @@ export function getValueByPointer(document: any, pointer: string): any {
* @param operation The operation to apply
* @param validateOperation `false` is without validation, `true` to use default jsonpatch's validation, or you can pass a `validateOperation` callback to be used for validation.
* @param mutateDocument Whether to mutate the original document or clone it before applying
* @param banPrototypeModifications Whether to ban modifications to `__proto__`, defaults to `true`.
* @return `{newDocument, result}` after the operation
*/
export function applyOperation<T>(document: T, operation: Operation, validateOperation: boolean | Validator<T> = false, mutateDocument: boolean = true): OperationResult<T> {
export function applyOperation<T>(document: T, operation: Operation, validateOperation: boolean | Validator<T> = false, mutateDocument: boolean = true, banPrototypeModifications: boolean = true): OperationResult<T> {
if (validateOperation) {
if (typeof validateOperation == 'function') {
validateOperation(operation, 0, document, operation.path);
Expand Down Expand Up @@ -264,6 +260,10 @@ export function applyOperation<T>(document: T, operation: Operation, validateOpe
while (true) {
key = keys[t];

if(banPrototypeModifications && key == '__proto__') {
throw new TypeError('JSON-Patch: modifying `__proto__` prop is banned for security reasons, if this was on purpose, please set `banPrototypeModifications` flag false and pass it to this function. More info in fast-json-patch README');
}

if (validateOperation) {
if (existingPathFragment === undefined) {
if (obj[key] === undefined) {
Expand Down Expand Up @@ -329,9 +329,10 @@ export function applyOperation<T>(document: T, operation: Operation, validateOpe
* @param patch The patch to apply
* @param validateOperation `false` is without validation, `true` to use default jsonpatch's validation, or you can pass a `validateOperation` callback to be used for validation.
* @param mutateDocument Whether to mutate the original document or clone it before applying
* @param banPrototypeModifications Whether to ban modifications to `__proto__`, defaults to `true`.
* @return An array of `{newDocument, result}` after the patch
*/
export function applyPatch<T>(document: T, patch: Operation[], validateOperation?: boolean | Validator<T>, mutateDocument: boolean = true): PatchResult<T> {
export function applyPatch<T>(document: T, patch: Operation[], validateOperation?: boolean | Validator<T>, mutateDocument: boolean = true, banPrototypeModifications: boolean = true): PatchResult<T> {
if(validateOperation) {
if(!Array.isArray(patch)) {
throw new JsonPatchError('Patch sequence must be an array', 'SEQUENCE_NOT_AN_ARRAY');
Expand All @@ -343,7 +344,8 @@ export function applyPatch<T>(document: T, patch: Operation[], validateOperation
const results = new Array(patch.length) as PatchResult<T>;

for (let i = 0, length = patch.length; i < length; i++) {
results[i] = applyOperation(document, patch[i], validateOperation);
// we don't need to pass mutateDocument argument because if it was true, we already deep cloned the object, we'll just pass `true`
results[i] = applyOperation(document, patch[i], validateOperation, true, banPrototypeModifications);
document = results[i].newDocument; // in case root was replaced
}
results.newDocument = document;
Expand Down
11 changes: 2 additions & 9 deletions src/duplex.ts
Expand Up @@ -3,15 +3,8 @@
* (c) 2017 Joachim Wester
* MIT license
*/
declare var require: any;

const equalsOptions = { strict: true };
const _equals = require('deep-equal');
const areEquals = (a: any, b: any): boolean => {
return _equals(a, b, equalsOptions)
}
import { PatchError, _deepClone, isInteger, _objectKeys, escapePathComponent, unescapePathComponent, hasUndefined, hasOwnProperty } from './helpers';
import { applyOperation, applyPatch, getValueByPointer, Operation } from './core';
import { _deepClone, _objectKeys, escapePathComponent, hasOwnProperty } from './helpers';
import { applyPatch, Operation } from './core';

/* export all core functions */
export { applyOperation, applyPatch, applyReducer, getValueByPointer, Operation, validate, validator, OperationResult } from './core';
Expand Down
114 changes: 81 additions & 33 deletions test/spec/coreSpec.js
Expand Up @@ -9,40 +9,44 @@ if (typeof _ === 'undefined') {
describe('jsonpatch.getValueByPointer', function() {
it('should retrieve values by JSON pointer from tree - deep object', function() {
var obj = {
person: {name: 'Marilyn'}
person: { name: 'Marilyn' }
};
var name = jsonpatch.getValueByPointer(obj, '/person/name')
var name = jsonpatch.getValueByPointer(obj, '/person/name');
expect(name).toEqual('Marilyn');
});

it('should retrieve values by JSON pointer from tree - deep array', function() {
var obj = {
people: [{name: 'Marilyn'}, {name: 'Monroe'}]
people: [{ name: 'Marilyn' }, { name: 'Monroe' }]
};
var name = jsonpatch.getValueByPointer(obj, '/people/1/name')
var name = jsonpatch.getValueByPointer(obj, '/people/1/name');
expect(name).toEqual('Monroe');
});

it('should retrieve values by JSON pointer from tree - root object', function() {
var obj = {
people: [{name: 'Marilyn'}, {name: 'Monroe'}]
people: [{ name: 'Marilyn' }, { name: 'Monroe' }]
};
var retrievedObject = jsonpatch.getValueByPointer(obj, '');

expect(retrievedObject).toEqual({
people: [{name: 'Marilyn'}, {name: 'Monroe'}]
people: [{ name: 'Marilyn' }, { name: 'Monroe' }]
});
});

it('should retrieve values by JSON pointer from tree - root array', function() {
var obj = [{
people: [{name: 'Marilyn'}, {name: 'Monroe'}]
}];
var obj = [
{
people: [{ name: 'Marilyn' }, { name: 'Monroe' }]
}
];
var retrievedObject = jsonpatch.getValueByPointer(obj, '');

expect(retrievedObject).toEqual([{
people: [{name: 'Marilyn'}, {name: 'Monroe'}]
}]);
expect(retrievedObject).toEqual([
{
people: [{ name: 'Marilyn' }, { name: 'Monroe' }]
}
]);
});
});
describe('jsonpatch.applyReducer - using with Array#reduce', function() {
Expand All @@ -62,26 +66,30 @@ describe('jsonpatch.applyReducer - using with Array#reduce', function() {
});
});
describe('root replacement with applyOperation', function() {
describe('_get operation', function () {
describe('_get operation', function() {
it('should get root value', function() {
var obj = [{
people: [{name: 'Marilyn'}, {name: 'Monroe'}]
}];
var obj = [
{
people: [{ name: 'Marilyn' }, { name: 'Monroe' }]
}
];

var patch = {op: '_get', path: ''};
var patch = { op: '_get', path: '' };

jsonpatch.applyOperation(obj, patch);

expect(patch.value).toEqual([{
people: [{name: 'Marilyn'}, {name: 'Monroe'}]
}]);
expect(patch.value).toEqual([
{
people: [{ name: 'Marilyn' }, { name: 'Monroe' }]
}
]);
});
it('should get deep value', function() {
var obj = {
people: [{name: 'Marilyn'}, {name: 'Monroe'}]
people: [{ name: 'Marilyn' }, { name: 'Monroe' }]
};

var patch = {op: '_get', path: '/people/1/name'};
var patch = { op: '_get', path: '/people/1/name' };

jsonpatch.applyOperation(obj, patch);

Expand Down Expand Up @@ -179,7 +187,7 @@ describe('root replacement with applyOperation', function() {
}
]);
});
it('should `add` an array prop', function() {
it('should `add` an array prop', function() {
var obj = [];

var newObj = jsonpatch.applyOperation(obj, {
Expand Down Expand Up @@ -1508,25 +1516,23 @@ describe('core', function() {
it('should apply copy, without leaving cross-reference between nodes', function() {
var obj = {};
var patchset = [
{op: 'add', path: '/foo', value: []},
{op: 'add', path: '/foo/-', value: 1},
{op: 'copy', from: '/foo', path: '/bar'},
{op: 'add', path: '/bar/-', value: 2}
{ op: 'add', path: '/foo', value: [] },
{ op: 'add', path: '/foo/-', value: 1 },
{ op: 'copy', from: '/foo', path: '/bar' },
{ op: 'add', path: '/bar/-', value: 2 }
];

jsonpatch.applyPatch(obj, patchset);

expect(obj).toEqual({
"foo": [1],
"bar": [1, 2],
foo: [1],
bar: [1, 2]
});
});

it('should use value object as a reference', function () {
it('should use value object as a reference', function() {
var obj1 = {};
var patch = [
{op: 'add', path: '/foo', value: []}
];
var patch = [{ op: 'add', path: '/foo', value: [] }];

jsonpatch.applyPatch(obj1, patch, false);

Expand Down Expand Up @@ -1910,5 +1916,47 @@ describe('undefined - JS to JSON projection / JSON to JS extension', function()
bar: null
});
});

it(`should allow __proto__ modifications when the banPrototypeModifications flag is set`, function() {
function SomeClass() {
this.foo = 'bar';
}

let doc = new SomeClass();
let otherDoc = new SomeClass();

const patch = [
{ op: 'replace', path: `/__proto__/x`, value: 'polluted' }
];

jsonpatch.applyPatch(doc, patch, false, true, false);

expect(otherDoc.x).toEqual('polluted');
});

it(`should not allow __proto__ modifications without unsetting the banPrototypeModifications flag and should throw an error`, function() {
const expectedErrorMessage =
'JSON-Patch: modifying `__proto__` prop is banned for security reasons, if this was on purpose, please set `banPrototypeModifications` flag false and pass it to this function. More info in fast-json-patch README';

function SomeClass() {
this.foo = 'bar';
}

let doc = new SomeClass();
let otherDoc = new SomeClass();

const patch = [
{ op: 'replace', path: `/__proto__/x`, value: 'polluted' }
];

expect(() => jsonpatch.applyPatch(doc, patch)).toThrow(new TypeError(expectedErrorMessage));

expect(otherDoc.x).toEqual(undefined);
expect(doc.x).toEqual(undefined);

let arr = [];
expect(() => jsonpatch.applyPatch(arr, patch)).toThrow(new TypeError(expectedErrorMessage));
expect(arr.x).toEqual(undefined);
});
});
});