Ban prototype pollution and document it #219
Conversation
| 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); |
There was a problem hiding this comment.
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"
test/spec/coreSpec.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it(`should allow __proto__ modifications when the flag is set`, function() { |
There was a problem hiding this comment.
To me more descriptive
| it(`should allow __proto__ modifications when the flag is set`, function() { | |
| it(`should allow __proto__ modifications when the mutateDocument flag is set`, function() { |
test/spec/coreSpec.js
Outdated
| expect(otherDoc.x).toEqual('polluted'); | ||
| }); | ||
|
|
||
| it(`should not allow __proto__ modifications without setting the flag and should throw an error`, function() { |
There was a problem hiding this comment.
| it(`should not allow __proto__ modifications without setting the flag and should throw an error`, function() { | |
| it(`should not allow __proto__ modifications without setting the mutateDocument flag and should throw an error`, function() { |
test/spec/coreSpec.js
Outdated
| jsonpatch.applyPatch(doc, patch); | ||
| } catch (e) { | ||
| expect(e.message).toEqual(expectedErrorMessage); | ||
| } |
There was a problem hiding this comment.
Have you considered expect(()=>{jsonpatch.applyPatch(doc, patch)}).to.throw(TypeError, expectedErrorMessage); to make test easier to read, and check for the error type as well?
https://www.chaijs.com/api/bdd/#method_throw
I'm afraid the code above would pass the test if jsonpatch.applyPatch(doc, patch); does not throw at all, as then the expect function is not called either.
|
Please consider that PR #221 also proposes to change the signature of the method |
|
Addressed all and tests are passing. merging. |
fixes #216