From d36386f304310f7207f91d25ac6bf44db1c0191f Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Sat, 15 Aug 2026 14:42:08 +0200 Subject: [PATCH] fix(ui): align JsVar array paths with JavaScript --- .agents/skills/jaws/SKILL.md | 5 ++ SECURITY.md | 27 ++++--- go.mod | 2 +- go.sum | 4 +- lib/assets/AI.md | 12 ++-- lib/assets/js_test.go | 50 +++++++++++++ lib/ui/AI.md | 8 ++- lib/ui/jsvar.go | 28 ++++++-- lib/ui/jsvar_check_test.go | 134 ++++++++++++++++++++++++++++++++++- 9 files changed, 238 insertions(+), 32 deletions(-) diff --git a/.agents/skills/jaws/SKILL.md b/.agents/skills/jaws/SKILL.md index 2ec4728d..c728bcf9 100644 --- a/.agents/skills/jaws/SKILL.md +++ b/.agents/skills/jaws/SKILL.md @@ -313,6 +313,11 @@ The handler candidate is asked via `JawsClick` / `JawsContextMenu` / `JawsInput` ## JsVar JSON representation limits +- Generic jq paths require JavaScript array-index names at Go arrays and slices: + `"0"` or a nonzero decimal without a leading zero, at most `4294967294` and + representable as Go `int`. Other components produce an error matching + `jq.ErrPathNotFound` at an array or slice. String-keyed map entries and JSON + field names are exact; empty components are ignored. - Browser JSON numbers use JavaScript `Number` values. Integers outside `-9007199254740991` through `9007199254740991` may round, and a browser write may commit the rounded value to Go. diff --git a/SECURITY.md b/SECURITY.md index 611d5f37..db00e797 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -274,17 +274,22 @@ state-size policy. Applications can set `JsVar.ClientCheck` to inspect the complete tentative value and the browser-supplied jq path before a generic browser write commits. -The path is passed through unchanged, jq accepts equivalent noncanonical -spellings with empty components, and both `""` and `"."` address the root. Treat -it as an inspection hint, not an authorization key; use `ui.PathSetter` to -allow-list paths. A returned error rolls the write back without broadcasting -it. If the error matches `ui.ErrJsVarTooLarge`, -`JawsInput` returns that sentinel and, during normal framework dispatch, -cancels the associated request after releasing application locks; other check -errors do not cancel the request. The check validates tentative Go state, not -the decoded browser value used in an accepted peer broadcast. jq conversions -and ignored map-to-struct entries can make those values differ; use -`ui.PathSetter` when peer-visible input also needs validation. +The path is passed through unchanged. Empty components are ignored, so +`.value.` aliases `value`, and both `""` and `"."` address the root. Generic +array and slice writes require canonical JavaScript array-index names +representable as Go `int`; string-keyed map entries and JSON field names are +exact. Invalid array or slice paths are rejected before `ClientCheck` runs. +Treat the raw path as an inspection hint, not an authorization key; use +`ui.PathSetter` to allow-list paths. A returned error +rolls the write back without broadcasting it. If the error matches +`ui.ErrJsVarTooLarge`, `JawsInput` returns that sentinel and, during normal +framework dispatch, cancels the associated request after releasing application +locks; other check errors do not cancel the request. The check validates +tentative Go state, not the decoded browser value used in an accepted peer +broadcast. jq conversions and ignored map-to-struct entries can make those +values differ; use `ui.PathSetter` when peer-visible input also needs +validation. + `ui.JSONSizeCheck[T](maxBytes)` supplies an exact serialized-size check. A value above the limit or one that cannot be marshaled makes the check match the size sentinel. A non-positive limit disables the check. The helper marshals the diff --git a/go.mod b/go.mod index ce43b262..cb474ff1 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.25.0 require ( github.com/coder/websocket v1.8.15 github.com/linkdata/deadlock v0.5.5 - github.com/linkdata/jq v0.2.0 + github.com/linkdata/jq v0.3.0 github.com/linkdata/secureheaders v1.5.0 github.com/linkdata/staticserve v1.1.8 golang.org/x/net v0.58.0 diff --git a/go.sum b/go.sum index d9f04bd7..a9ee4b55 100644 --- a/go.sum +++ b/go.sum @@ -2,8 +2,8 @@ github.com/coder/websocket v1.8.15 h1:6B2JPeOGlpff2Uz6vOEH1Vzpi0iUz20A+lPVhPHtNU github.com/coder/websocket v1.8.15/go.mod h1:NX3SzP+inril6yawo5CQXx8+fk145lPDC6pumgx0mVg= github.com/linkdata/deadlock v0.5.5 h1:d6O+rzEqasSfamGDA8u7bjtaq7hOX8Ha4Zn36Wxrkvo= github.com/linkdata/deadlock v0.5.5/go.mod h1:tXb28stzAD3trzEEK0UJWC+rZKuobCoPktPYzebb1u0= -github.com/linkdata/jq v0.2.0 h1:0R5w5SG8xcpA1U3mTW79M15pR90kI0iwP5KCWmiof2o= -github.com/linkdata/jq v0.2.0/go.mod h1:b76MMuWybyXrVEKSHJkv+r5IEattnghPf1bNRJ5PNlE= +github.com/linkdata/jq v0.3.0 h1:GKDcydShVH63mLUvZJwoEvSFO9HS7M2iYxp7HOEsEfw= +github.com/linkdata/jq v0.3.0/go.mod h1:b76MMuWybyXrVEKSHJkv+r5IEattnghPf1bNRJ5PNlE= github.com/linkdata/secureheaders v1.5.0 h1:4kI290hrVUYqluxF8pQfRYpbZuorSd9+8GlMgrLcrK4= github.com/linkdata/secureheaders v1.5.0/go.mod h1:50TiKmPaWki8gIZXmBxrLUinyJst12YuN3EbsXZMO+4= github.com/linkdata/staticserve v1.1.8 h1:qpm0dAaAsik1YrETSiMoS7ciBL9qPVNUFcfdio/ol84= diff --git a/lib/assets/AI.md b/lib/assets/AI.md index fb5298f4..5d76e6be 100644 --- a/lib/assets/AI.md +++ b/lib/assets/AI.md @@ -44,11 +44,13 @@ Keep reconnect constants and behavior covered by the JavaScript runtime tests. ## Browser helpers `jawsVar` reads or writes an application-owned global and sends a `Set` frame -for every live binding registered under its top-level name. Dotted components -after the top-level name form the JSON path. Exact `__proto__` path components -are rejected, and the routing table remains a `Map` so names cannot mutate an -object prototype. Full `JsVar` authority, validation, and synchronization rules -belong to `lib/ui/AI.md`. +for every live binding registered under its top-level name. Non-empty dotted +components are verbatim JavaScript property names: on an array, `"1"` addresses +an element, while `"01"` creates a side property omitted by JSON serialization +and rejected by generic Go array and slice bindings. Exact `__proto__` path +components are rejected, and the routing table remains a `Map` so names cannot +mutate an object prototype. Full `JsVar` authority, validation, and +synchronization rules belong to `lib/ui/AI.md`. Value updates avoid writes when possible and preserve text selection when a textual value changes by insertion or removal. Managed native form reset is not diff --git a/lib/assets/js_test.go b/lib/assets/js_test.go index af3effb5..f093c6cb 100644 --- a/lib/assets/js_test.go +++ b/lib/assets/js_test.go @@ -649,6 +649,56 @@ process.stdout.write(jaws.sent[0] || ""); } } +func TestJawsJS_JsVarArrayPathsUseExactPropertyNames(t *testing.T) { + raw := runJawsJSSnippet(t, ` +function FakeSocket() { this.readyState = 1; this.sent = []; } +FakeSocket.prototype.send = function(msg) { this.sent.push(msg); }; +WebSocket = FakeSocket; + +window.app = { items: [10, 20] }; +window.jawsNames.set("app", ["Jid.9"]); +jaws = new FakeSocket(); + +jawsVar("app.items.1", 21); +jawsVar("app.items.01", 99); + +process.stdout.write(JSON.stringify({ + canonical: window.app.items[1], + sideProperty: window.app.items["01"], + hasSideProperty: Object.hasOwn(window.app.items, "01"), + serialized: JSON.stringify(window.app.items), + frames: jaws.sent, +})); +`) + + var got struct { + Canonical int `json:"canonical"` + SideProperty int `json:"sideProperty"` + HasSideProperty bool `json:"hasSideProperty"` + Serialized string `json:"serialized"` + Frames []string `json:"frames"` + } + if err := json.Unmarshal([]byte(raw), &got); err != nil { + t.Fatalf("unexpected JSON output %q: %v", raw, err) + } + if got.Canonical != 21 || got.SideProperty != 99 || !got.HasSideProperty { + t.Fatalf("array property state = %+v, want canonical index 21 and exact side property 99", got) + } + if got.Serialized != "[10,21]" { + t.Fatalf("serialized array = %q, want side property omitted", got.Serialized) + } + wantData := []string{"items.1=21", "items.01=99"} + if len(got.Frames) != len(wantData) { + t.Fatalf("frames = %q, want %d", got.Frames, len(wantData)) + } + for i, rawFrame := range got.Frames { + msg, ok := wire.Parse([]byte(rawFrame)) + if !ok || msg.What != what.Set || msg.Jid != 9 || msg.Data != wantData[i] { + t.Fatalf("frame %d = %+v, parseable %t; want Jid.9 Set %q", i, msg, ok, wantData[i]) + } + } +} + func TestJawsJS_JsVarRejectsProtoPathComponents(t *testing.T) { raw := runJawsJSSnippet(t, ` function FakeSocket() { this.readyState = 1; this.sent = []; } diff --git a/lib/ui/AI.md b/lib/ui/AI.md index edab9f1b..42096eb5 100644 --- a/lib/ui/AI.md +++ b/lib/ui/AI.md @@ -317,9 +317,11 @@ as `time.Time`, `[]byte`, and maps with non-string keys. The generic setter can update exported JSON fields and grow slices and has no default accumulated-state limit. `ClientCheck` validates the complete tentative value before an actual generic write commits. It receives the browser-supplied -jq path unchanged; equivalent noncanonical paths exist, so treat it as an -inspection hint rather than an authorization key. Use `PathSetter` to allow-list -paths and operations. +jq path unchanged. Empty components are ignored, so `.value.` aliases `value`; +treat the raw path as an inspection hint, not an authorization key. Array and +slice components must be canonical JavaScript array-index names representable +as Go `int`; string-keyed map entries and JSON field names are exact. Use +`PathSetter` to allow-list paths and operations. A check runs while the application locker is held. It must inspect only: do not mutate or retain tentative state, re-enter the JsVar, call a path setter, acquire diff --git a/lib/ui/jsvar.go b/lib/ui/jsvar.go index 08cd0c2e..27fe3551 100644 --- a/lib/ui/jsvar.go +++ b/lib/ui/jsvar.go @@ -52,6 +52,11 @@ type PathSetter interface { // the raw JSON bytes. Programmatic [JsVar.JawsSetPath] calls pass the // caller-supplied value unchanged. // + // A [JsVar] returns [ErrIllegalJsVarPath] before calling a PathSetter when + // jsPath contains a protocol byte. Otherwise it passes jsPath unchanged and + // does not apply generic jq validation. Successful broadcasts preserve jsPath + // and the requested value. + // // If the member is already the given value, it should return [jaws.ErrValueUnchanged]. // // When a [JsVar]'s bound value (Ptr) implements PathSetter, the JsVar @@ -109,11 +114,11 @@ var ( // JsVarCheck is a type alias so a value of a defined function type with this // signature can be assigned to [JsVar.ClientCheck] without explicit conversion. // -// The value contains the complete tentative state, and jsPath is the -// browser-supplied jq path used for the update. The path is passed through -// unchanged: jq accepts equivalent noncanonical spellings, including empty -// components, and both "" and "." address the root. Use jsPath as an inspection -// hint, not as an authorization key; implement [PathSetter] to allow-list paths. +// The value contains the complete tentative state, and jsPath is the original +// browser-supplied jq path. The generic setter ignores empty components, so use +// jsPath only as an inspection hint; implement [PathSetter] to allow-list paths. +// See [JsVar] for generic path rules. +// // A nil error accepts the update. A non-nil error rejects it atomically and is // returned unchanged, except that an error matching [ErrJsVarTooLarge] is // returned as that sentinel after cancelling the associated request, when one @@ -195,6 +200,13 @@ func JSONSizeCheck[T any](maxBytes int) (check JsVarCheck[T]) { // Use a browser-facing DTO compatible with the generic setter, or implement // [PathSetter] to parse and validate the decoded value. // +// Generic array and slice path components must be canonical JavaScript array +// indices representable as int: "0" or ASCII decimal digits without a leading +// zero, at most 4294967294. A component that violates these rules produces an +// error matching [github.com/linkdata/jq.ErrPathNotFound] when traversal reaches +// an array or slice. String-keyed map entries and JSON field names are matched +// exactly. Empty components are ignored, and both "" and "." address the root. +// // While the WebSocket is open, jawsVar sends one complete message per matching // live binding, subject to [jaws.Request.ServeHTTP]'s inbound limit. // [JsVar.ClientCheck] runs only after receipt and cannot enforce that limit. Use @@ -363,8 +375,8 @@ func (jsvar *JsVar[T]) setPathLock(elem *jaws.Element, jsPath string, value any, // The broadcast carries the caller's requested value, not the value actually // stored. If a PathSetter transforms the input (e.g. clamps a number), the // stored Go value and the value seen by peers can differ; the stored value is what - // JawsGet returns. Reject noncanonical input or arrange reconciliation after the - // PathSetter callback if peers must observe the transformed value. + // JawsGet returns. If peers must observe a transformed stored value, the + // application must reject the input or reconcile after the callback. if err == nil && changed && elem != nil && dirtyTag != nil { elem.Jaws.Broadcast(wire.Message{ Dest: dirtyTag, @@ -412,6 +424,8 @@ func (jsvar *JsVar[T]) setPath(elem *jaws.Element, jsPath string, value any, cli // It is a programmatic server-side write, so it does not invoke // [JsVar.ClientCheck]. // +// See [JsVar] for generic path rules. +// // A nil elem changes the bound value without broadcasting. A set before this // JsVar has acquired a dirty tag from rendering also produces no broadcast; its // initial render seeds the value via the data-jawsdata attribute. diff --git a/lib/ui/jsvar_check_test.go b/lib/ui/jsvar_check_test.go index e8efcf56..886378df 100644 --- a/lib/ui/jsvar_check_test.go +++ b/lib/ui/jsvar_check_test.go @@ -31,6 +31,18 @@ type jsVarSliceData struct { Items []string `json:"items"` } +type jsVarArrayIndexState struct { + Items []int `json:"items"` + ByName map[string]string `json:"byName"` + pathSetCalls int + lastPath string +} + +func (state *jsVarArrayIndexState) JawsPathSet(_ *jaws.Element, jsPath string, _ any) { + state.pathSetCalls++ + state.lastPath = jsPath +} + type jsVarConcurrentData struct { Left string `json:"left"` Right string `json:"right"` @@ -68,10 +80,12 @@ func (*jsVarSizeErrorPathSetter) JawsSetPath(*jaws.Element, string, any) error { } type jsVarErrorPathSetter struct { - err error + err error + lastPath string } -func (data *jsVarErrorPathSetter) JawsSetPath(*jaws.Element, string, any) error { +func (data *jsVarErrorPathSetter) JawsSetPath(_ *jaws.Element, jsPath string, _ any) error { + data.lastPath = jsPath return data.err } @@ -257,6 +271,103 @@ func TestJsVarClientCheckContract(t *testing.T) { } } +func TestJsVarGenericPathsUseJavaScriptArrayIndices(t *testing.T) { + jw, err := jaws.New() + if err != nil { + t.Fatal(err) + } + t.Cleanup(jw.Close) + go jw.Serve() + + tr := jawstest.NewTestRequest(jw, nil) + if tr == nil { + t.Fatal("nil test request") + } + cleanupJsVarTestRequest(t, tr) + <-tr.ReadyCh + + state := jsVarArrayIndexState{ + Items: []int{1, 2}, + ByName: map[string]string{"01": "old"}, + } + var mu sync.Mutex + jsvar := NewJsVar(&mu, &state) + checkPaths := make([]string, 0, 2) + jsvar.ClientCheck = func(_ *jsVarArrayIndexState, jsPath string) error { + checkPaths = append(checkPaths, jsPath) + return nil + } + + rw := RequestWriter{Request: tr.Request, Writer: io.Discard} + if err = rw.JsVar("indices", jsvar); err != nil { + t.Fatal(err) + } + elements := tr.GetElements(&state) + if len(elements) != 1 { + t.Fatalf("rendered elements = %d, want 1", len(elements)) + } + elem := elements[0] + + requireNoBroadcast := func(stage string) { + t.Helper() + select { + case msg := <-tr.OutCh: + t.Fatalf("%s broadcast %#v", stage, msg) + default: + } + } + requireUnchanged := func(stage string) { + t.Helper() + if state.Items[1] != 2 || state.ByName["01"] != "old" { + t.Fatalf("%s changed state to %#v", stage, state) + } + if len(checkPaths) != 0 || state.pathSetCalls != 0 { + t.Fatalf("%s made %d checks and %d callbacks", stage, len(checkPaths), state.pathSetCalls) + } + } + + if got := jsvar.JawsGetPath(nil, "items.01"); got != nil { + t.Fatalf("noncanonical array Get = %#v, want nil", got) + } + if got := jsvar.JawsGetPath(nil, "byName.01"); got != "old" { + t.Fatalf("exact map-key Get = %#v, want old", got) + } + + if err = jsvar.JawsSetPath(elem, "items.01", 9); !errors.Is(err, jq.ErrPathNotFound) { + t.Fatalf("programmatic noncanonical array write error = %v, want ErrPathNotFound", err) + } + requireUnchanged("programmatic noncanonical array write") + requireNoBroadcast("programmatic noncanonical array write") + + if err = clientSetFrame(t, jsvar, elem, "items.01", 9); !errors.Is(err, jq.ErrPathNotFound) { + t.Fatalf("browser noncanonical array write error = %v, want ErrPathNotFound", err) + } + requireUnchanged("browser noncanonical array write") + requireNoBroadcast("browser noncanonical array write") + + if err = clientSetFrame(t, jsvar, elem, "items.1", 9); err != nil { + t.Fatal(err) + } + if state.Items[1] != 9 || len(checkPaths) != 1 || checkPaths[0] != "items.1" || state.pathSetCalls != 1 || state.lastPath != "items.1" { + t.Fatalf("canonical array write left state %#v, check paths %q", state, checkPaths) + } + msg := awaitJsVarOperation(t, "canonical array-index broadcast", tr.OutCh) + if msg.What != what.Set || msg.Data != "items.1=9" { + t.Fatalf("canonical array-index broadcast = %#v, want items.1=9 Set", msg) + } + + if err = clientSetFrame(t, jsvar, elem, "byName.01", "new"); err != nil { + t.Fatal(err) + } + if state.ByName["01"] != "new" || len(checkPaths) != 2 || checkPaths[1] != "byName.01" || state.pathSetCalls != 2 || state.lastPath != "byName.01" { + t.Fatalf("exact map-key write left state %#v, check paths %q", state, checkPaths) + } + msg = awaitJsVarOperation(t, "exact map-key broadcast", tr.OutCh) + if msg.What != what.Set || msg.Data != `byName.01="new"` { + t.Fatalf(`exact map-key broadcast = %#v, want byName.01="new" Set`, msg) + } +} + func TestJsVarClientCheckRejectRestoresZeroValueAppendAliases(t *testing.T) { backing := []string{"sentinel"} state := jsVarSliceData{Items: backing[:0]} @@ -306,7 +417,7 @@ func TestJsVarClientCheckRootPath(t *testing.T) { } } -func TestJsVarClientCheckReceivesNoncanonicalPathUnchanged(t *testing.T) { +func TestJsVarClientCheckReceivesEmptyComponentsUnchanged(t *testing.T) { state := jsVarCheckedState{Value: "old"} jsvar := NewJsVar(new(sync.Mutex), &state) gotPath := "not called" @@ -639,6 +750,23 @@ func TestJsVarClientCheckBypasses(t *testing.T) { } }) + t.Run("PathSetter owns path syntax", func(t *testing.T) { + errRejected := errors.New("custom path rejection") + state := jsVarErrorPathSetter{err: errRejected} + jsvar := NewJsVar(new(sync.Mutex), &state) + calls := 0 + jsvar.ClientCheck = func(*jsVarErrorPathSetter, string) error { + calls++ + return nil + } + if err := jsvar.JawsInput(nil, `items.01="x"`); err != errRejected { + t.Fatalf("PathSetter error = %v, want exact %v", err, errRejected) + } + if calls != 0 || state.lastPath != "items.01" { + t.Fatalf("PathSetter path=%q calls=%d, want items.01 and no ClientCheck", state.lastPath, calls) + } + }) + t.Run("unchanged PathSetter", func(t *testing.T) { var mu sync.Mutex state := jsVarPathHooks{Value: "same"}