Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applying an
add,replaceortestoperation with an emptypathand novaluemember dereferences a nil*lazyNodeand panics. This affects both v4 and v5:[{"op":"replace","path":""}]DecodePatch, panic if thePatchis built directly[{"op":"add","path":""}]DecodePatch, panic if thePatchis built directly[{"op":"test","path":""}]Reproducer:
The
replacecase was reported in #168, which was closed, but it still reproduces on v4.13.0. #114 / #158 fixed the same panic fortestwith a non-empty path, but not for the whole document.This matters for servers that apply patches received from clients. For example, kube-apiserver applies user-supplied JSON Patch documents with v4, so any user allowed to patch a resource can trigger the panic with a single request.
Changes
add(v5) andreplace(v4, v5) with an empty path and no value return an error wrappingErrMissing.testwith an empty path and no value fails withErrTestFailed. This keeps the existing semantics oftest, where a missing value is treated asnull(see thetestcases without a value inTestCases), and the whole document is nevernull.The v4 and v5 changes are in separate commits, so either can be taken on its own.
Tests
BadCasesandTestCasesfor v4 and v5.TestMissingValueOnWholeDocumentin v5 builds thePatchwithoutDecodePatch, becauseDecodePatchalready rejectsaddandreplacewithout a value.go testruns. v4 has nogo.modand is not run in CI, so I ran its tests with a temporarygo.mod(module gopkg.in/evanphx/json-patch.v4).