Paths without a leading / are silently accepted

[liggitt]

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

json-patch/patch.go

Lines 338 to 344 in cb8f3b5

	split := strings.Split(path, "/")
	if len(split) < 2 {
	return nil, ""
	}
	parts := split[1 : len(split)-1]

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

Seen in kubernetes/kubernetes#81422

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

[liggitt]

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]

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]

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. kubernetes/kubernetes#81381 (comment)) :)

[evanphx]

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]

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):

• "" (root object)
• "/" (the "" property at the root)
• "/foo/" (the "" property in the "foo" property)

[evanphx]

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

[liggitt]

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

Thanks, sounds good

[matbhz]

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]

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

[matbhz]

@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]

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

[evantorrie]

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]

Same -- "" is a common case.

[evanphx]

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

[delaneyj]

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]

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

[kszafran]

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 #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]

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