JSONPointer should not process reverse solidus or double-quote chars in tokens by fossterer · Pull Request #588 · stleary/JSON-java · GitHub

As never defined in https://tools.ietf.org/html/rfc6901#section-3 and
as discussed in JSONPointer: Encoding quotes not required? #563,

backslashes(\) and quotes(") are not to be treated any special by JSONPointer

…kslashes (\) and quotes(") as anything special

What problem does this code solve?
This PR claims that the JSONPointer implementation does not follow the spec RFC-6901

Risks
High This code has been in place for several years

Changes to the API?
API behavior is changed

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
Yes

Was any code refactored in this commit?
No

Review status
APPROVED
Following a truncated 7 day window for comments, with concurrence of original author.

@erosb @jimblackler @johnjaylward Please comment if you have any thoughts or concerns.

Section 5 of the RFC states:

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

JSON String Representation

A JSON Pointer can be represented in a JSON string value. Per
[RFC4627], Section 2.5, all instances of quotation mark '"' (%x22),
reverse solidus '' (%x5C), and control (%x00-1F) characters MUST be
escaped.

Note that before processing a JSON string as a JSON Pointer,
backslash escape sequences must be unescaped.

So, I take this to mean that the current implementation may be correct.

See the continuation of section 5 for a test document and test cases.

@fossterer , can you add an explicit test case that matches the ones in the RFC?

Given the document here: https://github.com/stleary/JSON-java/blob/master/src/test/resources/jsonpointer-testdoc.json (which I believe matches the RFC at a glance)

Test JSON Pointer	Expected Result
""	// the whole document
"/foo"	["bar", "baz"]
"/foo/0"	"bar"
"/"	0
"/a~1b"	1
"/c%d"	2
"/e^f"	3
"/i\\j"	5
"/k\"l"	6
"/ "	7
"/m~0n"	8

And this one (because GitHub doesn't like the | in table values)

"/g|h" -> 4

Test JSON Pointer (URI Fragment)	Expected Result
#	// the whole document
#/foo	["bar", "baz"]
#/foo/0	"bar"
#/	0
#/a~1b	1
#/c%25d	2
#/e%5Ef	3
#/g%7Ch	4
#/i%5Cj	5
#/k%22l	6
#/%20	7
#/m~0n	8

The paths are expressed as JSON strings, not Java strings. so "/k\"l". However, going by the test example for the URI fragments, I believe you can leave them as-is. i.e. The key for the value 5 should be the actual string i\j and the key for the value 6 should be the exact string k"l

Update my previous comment.

Also @fossterer, if some of these test cases already exist, can you confirm that they are working as expected?

linking original implementation discussion: #218, #222 and #292

OK, after reading through the comments by @jimblackler in #563 and re-reading the RFC section 5 a few times, I'm reaching the conclusion that the escape and unescape functions in JSONPointer are wrong, and this PR corrects them.

Test cases like this one:

@Test
    public void quotationEscaping() {
        assertSame(document.get("k\"l"), query("/k\\\\\\\"l"));
    }

are essentially double escaping the "query" string. The RFC states that the query only needs to be escaped when stored as JSON. For the purpose of the test, the query is not a JSON String and is instead a Java String, and should not have the escaping applied.

The above test should be changed to (as provided in this PR):

@Test
    public void quotationEscaping() {
        assertSame(document.get("k\"l"), query("/k\"l"));
    }

As such, I'm giving my stamp of approval for this PR and I believe it matches the "the bug is worse than the fix" requirement.

@stleary can we try to get this in the next release too? @jimblackler has been waiting on it for a while now.

We can include it in the release at the end of this week. I would still like to give @erosb a few more days to respond, as he is the original JSONPointer author.

Agreed, based on section 3 and 5 of RFC 6901, this is the correct behavior. Thanks for the fix.

Merging the code but someone needs to complete the unit tests per the comments in this PR, before the next release.

@stleary All the tests indicated by @johnjaylward exist already in the test suite!

Reading from the highlighted line you can spot all the m~, k\"l, foo etc. cases

Thanks for taking the time to double check.

@stleary All the tests indicated by @johnjaylward exist already in the test suite!

Reading from the highlighted line you can spot all the m~, k\"l, foo etc. cases

Thanks for pointing this out, I will confirm.

Some of the tests exist, some do not. Most of the tests do not clearly call out the expected result. For example, if both document.get() and query() return the same incorrect result, the test will not catch it. Will open an issue to track this to completion.

Closes 563: As never defined in RFC 6901 Section 3, do not handle bac…

d6ccc64

…kslashes (\) and quotes(") as anything special

fossterer changed the title Closes 563: JSONPointer: Encoding quotes not required? Closes #563: JSONPointer: Encoding quotes not required? Feb 28, 2021

fossterer marked this pull request as ready for review February 28, 2021 21:14

fossterer mentioned this pull request Feb 28, 2021

JSONPointer: Encoding quotes not required? #563

Closed

stleary changed the title Closes #563: JSONPointer: Encoding quotes not required? JSONPointer should not process reverse solidus or double-quote chars in tokens Mar 1, 2021

stleary added the 7 day window due to breaking change label Mar 1, 2021

johnjaylward approved these changes Mar 1, 2021

View reviewed changes

johnjaylward mentioned this pull request Mar 1, 2021

Queries involving keys containing four backslashes ("\\\\") failing. #561

Closed

erosb approved these changes Mar 6, 2021

View reviewed changes

stleary added the Approved label Mar 6, 2021

stleary merged commit c43e21a into stleary:master Mar 6, 2021

stleary mentioned this pull request Mar 6, 2021

Need to complete the unit tests for JSONPointer #590

Closed

katldewitt mentioned this pull request Oct 28, 2021

Unit tests for multiple backslashes in JSONPointer #640

Merged

JSONPointer should not process reverse solidus or double-quote chars in tokens by fossterer · Pull Request #588 · stleary/JSON-java · GitHub

JSONPointer should not process reverse solidus or double-quote chars in tokens - #588

JSONPointer should not process reverse solidus or double-quote chars in tokens#588stleary merged 1 commit intostleary:masterstleary/JSON-java:masterfrom fossterer:563-jsonpointer-do-not-encode-quotesfossterer/JSON-java:563-jsonpointer-do-not-encode-quotesCopy head branch name to clipboard

Conversation

fossterer commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stleary commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stleary commented Mar 1, 2021

Uh oh!

johnjaylward commented Mar 1, 2021

Uh oh!

johnjaylward commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnjaylward commented Mar 1, 2021

Uh oh!

johnjaylward commented Mar 1, 2021

Uh oh!

johnjaylward left a comment

Choose a reason for hiding this comment

Uh oh!

johnjaylward commented Mar 2, 2021

Uh oh!

stleary commented Mar 2, 2021

Uh oh!

erosb left a comment

Choose a reason for hiding this comment

Uh oh!

stleary commented Mar 6, 2021

Uh oh!

fossterer commented Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnjaylward commented Mar 6, 2021

Uh oh!

stleary commented Mar 6, 2021

Uh oh!

stleary commented Mar 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JSONPointer should not process reverse solidus or double-quote chars in tokens#588
stleary merged 1 commit into
stleary:masterfrom
fossterer:563-jsonpointer-do-not-encode-quotes

fossterer commented Feb 28, 2021 •
edited
Loading

stleary commented Mar 1, 2021 •
edited
Loading

johnjaylward commented Mar 1, 2021 •
edited
Loading

fossterer commented Mar 6, 2021 •
edited
Loading