search

evanphx / json-patch

A Go library to apply RFC6902 patches and create and apply RFC7386 patches
BSD 3-Clause "New" or "Revised" License
1.06k stars 182 forks source link

Paths without a leading `/` are silently accepted #86

Closed liggitt closed 9 months ago

liggitt commented 5 years ago

The JSONPatch library splits the path by / and discards the first segment, assuming it is empty:

https://github.com/evanphx/json-patch/blob/cb8f3b5f9db3c390f2cdf6e67f7e6ea3b339a197/patch.go#L338-L344

This means that an operation with a patch like bogus/path/to/item will successfully apply to /path/to/item

Seen in https://github.com/kubernetes/kubernetes/issues/81422

Before fixing this, we should consider the compatibility implications of rejecting patches that were previously accepted.

liggitt commented 5 years ago

Per https://tools.ietf.org/html/rfc6901#section-3:

A JSON Pointer is a Unicode string (see [RFC4627], Section 3) containing a sequence of zero or more reference tokens, each prefixed by a '/' (%x2F) character.

Per http://jsonpatch.com/#json-pointer:

To point to the root of the document use an empty string for the pointer. The pointer / doesn’t point to the root, it points to a key of "" on the root (which is totally valid in JSON).

In addition to incorrectly accepting "bogus/...", looking briefly at the testcases, it doesn't look like "" or "/" are handled properly either.

Again, any changes should consider the behavior change impact on currently accepted patches

evanphx commented 5 years ago

Good catch! So because the current behavior is pretty weird and probably not useful, it's pretty safe to assume we won't be breaking compatibility by just rejecting paths that don't have a leading /.

liggitt commented 5 years ago

it's pretty safe to assume we won't be breaking compatibility by just rejecting paths that don't have a leading /.

you underestimate how much people thrash to get a patch applying (c.f. https://github.com/kubernetes/kubernetes/pull/81381#discussion_r313780676) :)

evanphx commented 5 years ago

Ooof. I guess it makes sense that they'd trash around and find out you can put garbage on the beginning. I don't want to make life harder for package users, but do want to just fix this obvious bug. I'm open to all options, including a configuration flag to configure the proper behavior so folks can opt in safely.

For kubernetes, if you want to retain the current, buggy behavior, either you could use a version of jsonpatch that allows for configuration of this new, proper behavior. Or we could make it a hard error and you could detect and prepend /blah to the beginning of patches that don't include a /.

I'm really open to all options here.

liggitt commented 5 years ago

I looked really hard for examples of mistaken patches like foo/bar used against the kubernetes API and couldn't find any. I think erroring on that case is reasonable.

Tests should probably also be added for these special cases as well (which are valid and should not error):

evanphx commented 5 years ago

Totally agree. I'll get this coded up into a PR for you to review shortly.

liggitt commented 4 years ago

Totally agree. I'll get this coded up into a PR for you to review shortly.

Thanks, sounds good

matbhz commented 4 years ago

Azure AD will send PATCH requests for the SCIM protocol in this format:

{"schemas":["urn:ietf:params:scim:api:messages:2.0:PatchOp"],"Operations":[{"op":"Replace","path":"active","value":"True"},{"op":"Add","path":"displayName","value":"Test 2 Smith"},{"op":"Replace","path":"externalId","value":"Test2"}]}

(without the leading /)

How can we support this case, which seems to contradict what's expected from Kubernetes API? Right now I get an error that says active does not exist in the document

liggitt commented 4 years ago

That's not a valid patch path, is it?

matbhz commented 4 years ago

@liggitt after reading and learning more about the RFC6902 it definitely looks like the case. I don't think I have an alternative on Azure side of things though. It looks very odd to me that they're sending wrong requests that do not respect the standard... I'll keep researching for alternatives 😢

liggitt commented 4 years ago

Yeah, it's definitely invalid. The path is a jsonpointer, which is defined as "zero or more segments each prefixed with /"

evantorrie commented 4 years ago

I've run into this too.. particularly the rejection of "" as a valid path (i.e. the whole document).

Any progress on this?

jbsolomon-fw commented 4 years ago

Same -- "" is a common case.

evanphx commented 4 years ago

Dropped the ball on this one, need to get back to it.

delaneyj commented 3 years ago

Good catch! So because the current behavior is pretty weird and probably not useful, it's pretty safe to assume we won't be breaking compatibility by just rejecting paths that don't have a leading /.

That assumption is breaking valid JSONPatches #142

graineri commented 3 years ago

Would it make sense to create a new version clearly stating the breaking compatibility change?

kszafran commented 2 years ago

So, do we agree that paths like some/field are invalid? It seems that with the current implementation, such a patch would fail saying that some/field is missing (so we wouldn't break the aforementioned Azure AD patch, because it already does not work). So except for "", all paths should start with /.

If https://github.com/evanphx/json-patch/issues/134 is implemented first, this validation could be be as simple as using a custom type for Path and From with an UnmarshalText method that checks for the slash.

evanphx commented 9 months ago

I guess I really dropped the ball on this, gonna leave the current behavior.