fix: escape member names in normalized paths - #123
Open
gaoflow wants to merge 1 commit into
Open
Conversation
`Display for NormalizedPath` destructured `PathElement::Name` and wrote the `&str` directly, bypassing the RFC 9535 section 2.7 escaper that hiltontj#113 added to `Display for PathElement`. Format each element through its own `Display`, and fix two defects that only that delegation exposes: U+005C was written as a single backslash instead of `\\`, and the hex-escape arm hard-coded a `\u000` prefix, so U+0010-U+001F fell through unescaped. Also deserialize the compliance suite's `result_paths`/`results_paths`, which the runner had never read, and check that every normalized path re-parses and re-selects the node it names.
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.
Display for NormalizedPathdestructuresPathElement::Nameand writes the&strwithwrite!(f, "['{name}']"). That formats the raw string, not thePathElement, so it never reaches the RFC 9535 §2.7 escaper that #113 added toDisplay for PathElement. The escaper has been unreachable from the public API since it landed.The example from #113's own description still reproduces on
main:Serialize for NormalizedPathgoes throughto_string(), so serialized locations were affected identically.Why it went unnoticed
On #113 you wrote:
The CTS added exactly that 11 days later (jsonpath-standard/jsonpath-compliance-test-suite@aaa53f2, "Test normalized paths"). The pinned submodule carries
result_pathson 447 cases andresults_pathson 9.tests/compliance.rsdeserializes onlyresult/results/invalid_selector, and its located branch callsq.nodes()and dropsq.locations(), so none of those 456 expectations were ever read.That is measurable rather than inferred. Replacing the
result_paths/results_pathsvalue of all 456 cases with["$['TOTALLY_BOGUS_PATH']"]:maincompliance_test_suite ... okThe class
Delegating to
PathElement'sDisplayexposes two further defects in that escaper, both in the samematch:'\u{005C}' => write!(f, r#"\"#)emits one backslash. §2.7 lists"\"undernormal-escapable, i.e.ESC+\. The visible effect is the worst case in the set, because it fails silently rather than loudly: member namea\bproduced$['a\b'], which re-parses fine and selects 0 nodes, the\bhaving been re-read as a backspace.\u000prefix (write!(f, "\\u000{:x}", c as i32)), so it structurally could not emit\u001xand U+0010–U+001F fell through to_ => write!(f, "{c}")raw. §2.7 allows exactly this range:Replaced with
c if c < '\u{0020}' => write!(f, "\\u{:04x}", c as u32), which covers the whole range uniformly and drops the prefix hazard.normal-unescapedomits%x0-1Fentirely, so every remaining control code must be escaped, andnormal-HEXDIGisDIGIT / %x61-66, so the digits are lowercase.I audited the rest of the path-formatting surface and found nothing else:
to_json_pointerimplements RFC 6901, not §2.7, and its~-then-/ordering is correct.Extent
Every code point U+0000–U+02FF as a member name, alone and as
a<c>b, plus quote/backslash cases — 1551 names, checked against three oracles: a §2.7 ABNF validator over the emitted string, a round-trip (re-parse the path, must re-select that node), andSerializevsDisplay.Serializedisagreeing withDisplayresult_pathsTests
tests/compliance.rs:result_paths/results_pathsare now required fields on theTestResultvariants and asserted againstlocations(), so the detector cannot silently disarm again. Also a round-trip assertion per located node — a normalized path is a singular query, so re-running it must return exactly the node it names.tests/normalized_paths.rs: round-trip over U+0000–U+001F and the quote/backslash cases. The corpus cannot cover these; itsresult_pathscontain only\b \f \n \r \t \' \\and no\uXXXXat all.path.rs: addednormalized_path_fmt, and extendednormalized_element_fmtto U+0010–U+001F.One existing assertion had to change.
normalized_element_fmt's"escapes"case fedr#"'\b\f\n\r\t\\'"#— a raw string, so the input is literal backslash-letter pairs, not the control characters the case name implies. Under the lone-backslash bug each\passed through unchanged and the pairs came out looking like the intended escapes, so the case passed by coincidence and would have passed with no escaping of\whatsoever. Correcting\to\\makes the expected value\'\\b\\f\\n\\r\\t\\\\\'. I split it in two rather than just updating the string:"escapes"now uses the actual control characters, and"literal_backslashes"keeps the original input with the RFC-correct expectation.Mutation-tested; every mutant was confirmed to compile before its verdict was taken.
NormalizedPath::Displayto the raw-name write\→ one backslash (pre-fix)\→ four backslashes (over-fix){:04X}uppercase hex{:03x}hex widthUppercase hex is pinned by the unit tests alone — the corpus has no
\uXXXXexpectation to catch it with.cargo test --workspace --all-features,cargo clippy --all-targets -- -D warnings,cargo fmt --all -- --checkandcargo doc --no-depsall pass.