Reject non-canonical trailing-dot tree paths in resolveChildPath#76
Merged
Conversation
A crafted WebSocket frame `children.0..selected=true` resolved to a valid
in-range node: JawsSetPath's CutSuffix(".selected") left nodePath "children.0.",
and resolveChildPath consumed "children"/"0" in one iteration and exited with the
trailing empty segment unvisited, returning Children[0] with no error. The server
then mutated the node while JawsPathSet broadcast the non-canonical "children.0."
verbatim as the jawstreeSetPath id. No peer's rendered node uses that id (Node.Walk
emits the canonical "children.0"), so the visual selection was silently dropped on
every peer, diverging their rendered tree from server state with no error.
The per-index canonical check added earlier only inspects index segments and never
sees an empty final segment, so it could not catch this. Reject a consumed-but-empty
trailing segment as well (Cut reports the separator via its third return value),
which closes the trailing-dot gap while keeping the cheap alloc-free per-index check.
Add the failing rejection cases (and canonical-path regressions) to the gate test
table, plus BenchmarkResolveChildPath as a regression guard. The resolver was
chosen by benchmarking three candidates (this incremental check vs a canonical
round-trip via strings.Builder vs a regex validator); the incremental check was
fastest and the only one that stays allocation-free on accepted paths. Accepted
paths are unchanged from before (zero allocations, within a few ns); the added cost
is only on rejected adversarial input, where an error is now correctly returned.
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.
Defect (per-package code review)
A crafted WebSocket frame
children.0..selected=trueresolved to a valid in-range tree node when it should have been rejected.JawsSetPath'sCutSuffix(".selected")leftnodePath=children.0., andresolveChildPathconsumed thechildren/0pair in one loop iteration and exited with the empty trailing segment unvisited — returningChildren[0]with no error.The server then mutated that node while
JawsPathSetbroadcast the non-canonicalchildren.0.verbatim as thejawstreeSetPathid. No peer's rendered node uses that id (Node.Walkemits the canonicalchildren.0), so the visual selection was silently dropped on every peer, diverging their rendered tree from server state with no error surfaced. This is the peer-divergence class the earlier per-index canonical check was meant to close, but that check only inspects index segments and never sees an empty final segment.Reachable only via a hand-crafted frame (the trusted client always builds canonical paths), but hardening against crafted client input is the documented contract of this path ("server holds the truth").
Fix
Reject a consumed-but-empty trailing segment as well:
strings.Cutreports via its third return value whether a separator was consumed, so an index followed by.with nothing after it (more && rest == "") is now rejected. This closes the trailing-dot gap while keeping the cheap, allocation-free per-index canonical check.How the implementation was chosen (benchmarked)
Three candidates were benchmarked (
benchstat -col /impl, count=10): this incremental check, a canonical round-trip viastrings.Builder, and aregexpvalidator. The incremental check was fastest and the only one allocation-free on accepted paths:BenchmarkResolveChildPathis kept as a regression guard. Before/after vsmainon accepted paths: unchanged and zero-alloc (+3–7 ns from one bool check); the only added cost is on rejected adversarial input, where anErrPathRejectederror is now correctly built instead of the path being wrongly accepted.Tests
TestNode_JawsSetPath_Gate(children.0..selected,.children.0.selected,children..0.selected) assertErrPathRejectedand that no node'sSelectedflips. The regression case (children.0..selected) fails on the unpatched code.Verification
go vet,gofumpt -l,staticcheck,go build,go test -race ./..., andgo test -tags debug -race ./...all pass.