Reject non-canonical trailing-dot tree paths in resolveChildPath (#76)

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.

Author: linkdata

jawstree/node.go

@@ -136,26 +136,35 @@ func (node *Node) JawsSetPath(elem *jaws.Element, jsPath string, value any) (err
 // nil-targeting segments are rejected, so a client can neither grow the slice nor
 // address a node that does not exist.
 //
-// Indices must be in canonical decimal form (no leading '+', '-' or zeros) so the
-// index a client sends round-trips to the same string [Node.Walk] emits as the
-// node ID. A non-canonical but in-range index would mutate the server node yet be
-// echoed verbatim as the [Node.JawsPathSet] broadcast "id", which no peer's
-// rendered node matches, diverging peer state from the server.
+// The whole path must be in canonical form so that it round-trips to the exact
+// string [Node.Walk] emits as the node ID: segments strictly alternate "children"
+// and a canonical decimal index (no leading '+', '-' or zeros), joined by single
+// dots with no empty segment from a leading, trailing or doubled '.'. A path that
+// resolves to a valid in-range node but is not canonical — for example a trailing
+// dot ("children.0.") or a non-canonical index ("children.+0") — would mutate the
+// server node yet be echoed verbatim as the [Node.JawsPathSet] broadcast "id",
+// which no peer's rendered node matches, diverging peer state from the server.
+//
+// Each index is checked against its canonical form (strconv.Itoa is allocation-free
+// for indices below 100). The trailing-dot case is the one a per-index check alone
+// misses: it leaves an empty final segment the pair-wise loop never visits, so we
+// reject when Cut reports a separator was consumed ('more') but nothing follows it.
 func (node *Node) resolveChildPath(nodePath string) (*Node, error) {
 	cur := node
-	for nodePath != "" {
+	for rest := nodePath; rest != ""; {
 		var seg, idxStr string
-		seg, nodePath, _ = strings.Cut(nodePath, ".")
+		var more bool
+		seg, rest, _ = strings.Cut(rest, ".")
 		if seg != "children" {
 			return nil, fmt.Errorf("%w: unexpected path segment %q", ErrPathRejected, seg)
 		}
-		idxStr, nodePath, _ = strings.Cut(nodePath, ".")
+		idxStr, rest, more = strings.Cut(rest, ".")
 		idx, err := strconv.Atoi(idxStr)
 		if err != nil || idx < 0 || idx >= len(cur.Children) || cur.Children[idx] == nil {
 			return nil, fmt.Errorf("%w: child index %q out of range", ErrPathRejected, idxStr)
 		}
-		if strconv.Itoa(idx) != idxStr {
-			return nil, fmt.Errorf("%w: non-canonical child index %q", ErrPathRejected, idxStr)
+		if strconv.Itoa(idx) != idxStr || (more && rest == "") {
+			return nil, fmt.Errorf("%w: non-canonical path %q", ErrPathRejected, nodePath)
 		}
 		cur = cur.Children[idx]
 	}

jawstree/node_benchmark_test.go

@@ -0,0 +1,41 @@
+package jawstree
+
+import "testing"
+
+var resolveChildPathBenchSink *Node
+
+// BenchmarkResolveChildPath guards the canonical child-path resolver against
+// regression across shallow, deep and rejected (trailing-dot) paths. The resolver
+// runs on every inbound tree-selection event, so it must stay allocation-free on
+// the accepted paths.
+func BenchmarkResolveChildPath(b *testing.B) {
+	// Build a 3-deep tree so the deep path resolves.
+	leaf := func(n string) *Node { return &Node{Name: n} }
+	root := &Node{Name: "root", Children: []*Node{
+		{Name: "a", Children: []*Node{
+			leaf("a0"),
+			{Name: "a1", Children: []*Node{leaf("a1.0"), leaf("a1.1"), leaf("a1.2")}},
+		}},
+		leaf("b"),
+	}}
+
+	cases := []struct {
+		name string
+		path string
+	}{
+		{"shallow", "children.1"},
+		{"deep", "children.0.children.1.children.2"},
+		{"rejected", "children.0.children.1.children.2."}, // trailing dot
+	}
+
+	for _, c := range cases {
+		b.Run(c.name, func(b *testing.B) {
+			b.ReportAllocs()
+			b.ResetTimer()
+			for i := 0; i < b.N; i++ {
+				n, _ := root.resolveChildPath(c.path)
+				resolveChildPathBenchSink = n
+			}
+		})
+	}
+}

jawstree/node_test.go

@@ -148,6 +148,49 @@ func TestNode_JawsSetPath_Gate(t *testing.T) {
 			}
 		}
 	})
+
+	// A non-canonical path that still resolves to a valid in-range node (an empty
+	// trailing/leading/embedded segment) must be rejected. Such a path otherwise
+	// mutates the server node while JawsPathSet echoes the non-canonical path
+	// verbatim as the broadcast id, which no peer's rendered (canonical) node id
+	// matches, silently desyncing peer selection. "children.0..selected" is the
+	// regression: it resolves to Children[0] yet is not the canonical
+	// "children.0.selected" that Node.Walk emits.
+	t.Run("rejects non-canonical paths resolving to a valid node", func(t *testing.T) {
+		for _, path := range []string{
+			"children.0..selected", // trailing dot -> nodePath "children.0."
+			".children.0.selected", // leading empty segment
+			"children..0.selected", // embedded empty segment
+		} {
+			root := newTree()
+			if err := root.JawsSetPath(nil, path, true); !errors.Is(err, jawstree.ErrPathRejected) {
+				t.Errorf("path %q: expected ErrPathRejected, got %v", path, err)
+			}
+			if root.Children[0].Selected {
+				t.Errorf("path %q: Children[0].Selected was mutated despite rejection", path)
+			}
+		}
+	})
+
+	// The fix must not reject legitimate canonical paths, including nested ones.
+	t.Run("accepts canonical paths including nested", func(t *testing.T) {
+		root := &jawstree.Node{Name: "root", Children: []*jawstree.Node{
+			{Name: "a", Children: []*jawstree.Node{{Name: "a0"}, {Name: "a1"}}},
+			{Name: "b"},
+		}}
+		if err := root.JawsSetPath(nil, "children.0.children.1.selected", true); err != nil {
+			t.Fatalf("nested canonical path: %v", err)
+		}
+		if !root.Children[0].Children[1].Selected {
+			t.Error("nested node was not selected")
+		}
+		if err := root.JawsSetPath(nil, "children.1.selected", true); err != nil {
+			t.Fatalf("canonical path: %v", err)
+		}
+		if !root.Children[1].Selected {
+			t.Error("node was not selected")
+		}
+	})
 }
 
 // TestNode_NilChildGuards exercises the defensive nil-child guards in marshalJSON