From 32c10568f8250f024a6df67bb2475aaa602f888b Mon Sep 17 00:00:00 2001 From: Bruno Carvalho Date: Sat, 13 Jun 2026 12:16:02 -0300 Subject: [PATCH] fix(stores): close persistence fallbacks flagged in PR review Three same-class silent-fallback issues where persisted page-store behavior depended on navigation history or skipped a check: 1. Mismatched persist scopes (validate_stores.go): two pages persisting the same store name with the same shape but different scopes (local vs session) share one storage key. The runtime keeps whichever scope initialized first, so the effective backend depended on nav order. Now warns with page_store_persist_scope_conflict (registered + explain entry). 2. Dropped persist config on re-hydration (runtime_assets.go): when a store was first declared without persistence and a later route declared it persisted, registry.init early-returned and silently discarded the persist config, so writes never reached storage. init now adopts the persist config for an existing-but-unpersisted store and restores the saved value, independent of navigation order. 3. Nested secret fields (validate_page.go): the secret-field check only scanned top-level fields, so a persisted store nesting e.g. Profile.Token wrote a credential-shaped value to browser storage with no warning. It now scans nested field paths from FieldTypes. Tests: scope-conflict + nested-secret compiler tests, a node store-runtime step asserting persistence adoption across SPA navigation, and a nested-secret fixture type. CHANGELOG, docs (syntax/components), and explain updated. Full non-example suite green. --- CHANGELOG.md | 12 +++-- docs/language/components.md | 14 ++++-- docs/language/syntax.md | 4 +- internal/buildgen/runtime_assets.go | 19 ++++++- .../buildgen/store_persist_runtime_test.go | 31 ++++++++++++ internal/compiler/validate_page.go | 37 ++++++++++++-- .../compiler/validate_page_persist_test.go | 47 ++++++++++++++++++ internal/compiler/validate_stores.go | 49 +++++++++++++------ internal/diagnostics/explain.go | 15 ++++++ internal/diagnostics/registry.go | 1 + testfixture/islands/islands.go | 19 +++++++ 11 files changed, 220 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99ece59..d6907de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,14 +12,20 @@ packages, and tooling contracts may change before a stable release. (`store cart ui.CartState = ui.NewCartState() persist "local"`). The generated store runtime hydrates from localStorage/sessionStorage on load, re-hydrates on SPA navigation (stores first declared on a later route are picked up on content - swap), writes the store's declared fields on change, mirrors cross-tab writes + swap, and a store first declared without persistence adopts a later route's + persist config and restores its saved value, so persistence never depends on + navigation order), writes the store's declared fields on change, mirrors cross-tab writes through the `storage` event, exposes `window.__gowdkStores.clear(name)` to drop a persisted store, and discards persisted state whose embedded schema hash no longer matches the store's shape (so a struct change never restores stale data). Only the store's own fields persist — never component state, props, or computed values. New diagnostics: `page_store_persist_scope_invalid` (error), - `page_store_persist_secret_field` (warning), and - `page_store_persist_key_conflict` (warning). Persistence is a JS-island/store + `page_store_persist_secret_field` (warning, raised for nested secret-resembling + fields such as `Profile.Token`, not only top-level fields), + `page_store_persist_key_conflict` (warning), and + `page_store_persist_scope_conflict` (warning, when the same store name is + persisted under different `local`/`session` scopes across pages and would + otherwise let navigation order decide the backend). Persistence is a JS-island/store runtime feature; WASM islands do not yet participate in page stores. - M4 Go interop is complete for the current 0.x surface: a user can see why a Go function or type did or did not bind. `gowdk inspect go-bindings` emits a diff --git a/docs/language/components.md b/docs/language/components.md index 8c1d3f8..7f2439e 100644 --- a/docs/language/components.md +++ b/docs/language/components.md @@ -293,9 +293,11 @@ component state, props, or computed values. The compiler embeds a hash of the store's struct shape; when the shape changes, stale persisted state is discarded rather than restored, so a struct change never crashes on old data. Because browser storage is readable by any script on the origin, persisting a field whose -name resembles a secret (`token`, `password`, `secret`, `auth`, …) is a warning: -keep credentials and trusted authorization state server-side. An unknown scope is -rejected — see `gowdk explain page_store_persist_scope_invalid`. +name resembles a secret (`token`, `password`, `secret`, `auth`, …) is a warning — +including a nested field such as `Profile.Token`, because persistence writes the +whole value of each top-level field: keep credentials and trusted authorization +state server-side. An unknown scope is rejected — see +`gowdk explain page_store_persist_scope_invalid`. Persisted stores also sync across tabs: when one tab writes, other tabs on the origin mirror the value through the browser `storage` event. To drop a persisted @@ -304,7 +306,11 @@ store (for example after checkout or logout), call the store to its build-time init value. If two pages persist a store with the same name but different shapes, they share one storage key and discard each other's data on navigation; the compiler warns with -`page_store_persist_key_conflict`. +`page_store_persist_key_conflict`. If they share the same shape but declare +different `local`/`session` scopes, the runtime keeps whichever scope initialized +first and the compiler warns with `page_store_persist_scope_conflict`. A store +first reached on a route that does not persist it still adopts persistence when a +later route declares it, restoring the saved value regardless of navigation order. Persistence survives SPA navigation: when the client runtime swaps page content it re-scans store seeds, so a store first declared on a later client-side route diff --git a/docs/language/syntax.md b/docs/language/syntax.md index 684bacb..1989b81 100644 --- a/docs/language/syntax.md +++ b/docs/language/syntax.md @@ -499,7 +499,9 @@ The scope must be `"local"` (localStorage) or `"session"` (sessionStorage); any other value is rejected with `page_store_persist_scope_invalid`. Persisted store state is keyed by store name, restores over the build-time init value on load, is discarded when the store's struct shape changes, and warns when a persisted -field name resembles a secret. +field name resembles a secret (nested fields included). Declaring the same store +name with a different `local`/`session` scope across pages warns with +`page_store_persist_scope_conflict`. Client blocks can declare limited DOM refs for safe methods: diff --git a/internal/buildgen/runtime_assets.go b/internal/buildgen/runtime_assets.go index fff663c..2adb4c9 100644 --- a/internal/buildgen/runtime_assets.go +++ b/internal/buildgen/runtime_assets.go @@ -138,7 +138,24 @@ func storeRuntimeSource() string { }; registry.init = (name, state, persist) => { - if (!name || registry.stores[name]) return; + if (!name) return; + if (registry.stores[name]) { + // The store already exists (for example SPA navigation reached a later + // route that declares the same store). If this declaration adds + // persistence and we are not persisting yet, adopt it and restore the + // saved value so persistence does not depend on which route loaded first. + // A conflicting scope is kept first-wins and reported at build time by + // page_store_persist_scope_conflict, so navigation cannot thrash storage. + if (persist && persist.scope && persist.key && persist.version && !registry.persist[name]) { + registry.persist[name] = persist; + const restored = readPersisted(persist, registry.fields[name]); + if (restored) { + registry.stores[name] = Object.assign({}, registry.stores[name] || {}, restored); + notify(name); + } + } + return; + } const seed = Object.assign(Object.create(null), state || {}); registry.fields[name] = Object.keys(seed); registry.seeds[name] = Object.assign(Object.create(null), seed); diff --git a/internal/buildgen/store_persist_runtime_test.go b/internal/buildgen/store_persist_runtime_test.go index de542d1..de8bf69 100644 --- a/internal/buildgen/store_persist_runtime_test.go +++ b/internal/buildgen/store_persist_runtime_test.go @@ -163,6 +163,37 @@ r.hydrate(); assert.equal(r.get("prefs").Theme, "dark", "hydrate() picks up a store first seen on a later route"); assert.equal(r.get("cart").Count, 0, "re-hydrate leaves an existing store untouched"); +// 9. Adopting persistence across SPA navigation: a store first declared WITHOUT +// persistence must pick up a later route's persist config (and restore the saved +// value) instead of silently staying unpersisted, so persistence does not depend +// on which route loaded first. +localStorage.setItem("gowdk:store:wishlist", JSON.stringify({ v: "w1", s: { Items: 2 } })); +// First navigation declares wishlist without persistence (no data-gowdk-persist). +global.document.querySelectorAll = () => [{ + getAttribute(name) { return name === "data-gowdk-store" ? "wishlist" : null; }, + get textContent() { return '{"Items":0}'; } +}]; +r.hydrate(); +assert.equal(r.get("wishlist").Items, 0, "unpersisted first declaration uses the seed, not storage"); +// Second navigation declares the same store as persist "local". +let adopted = null; +r.subscribe("wishlist", (next) => { adopted = next; }); +global.document.querySelectorAll = () => [{ + getAttribute(name) { + if (name === "data-gowdk-store") return "wishlist"; + if (name === "data-gowdk-persist") return "local"; + if (name === "data-gowdk-persist-key") return "gowdk:store:wishlist"; + if (name === "data-gowdk-persist-version") return "w1"; + return null; + }, + get textContent() { return '{"Items":0}'; } +}]; +r.hydrate(); +assert.equal(r.get("wishlist").Items, 2, "re-hydrate adopts persistence and restores the saved value"); +assert.ok(adopted && adopted.Items === 2, "adopting persistence notifies subscribers"); +r.set("wishlist", { Items: 7 }); +assert.equal(JSON.parse(localStorage.getItem("gowdk:store:wishlist")).s.Items, 7, "after adoption, set() writes through to storage"); + console.log("OK"); ` } diff --git a/internal/compiler/validate_page.go b/internal/compiler/validate_page.go index 151393b..3146402 100644 --- a/internal/compiler/validate_page.go +++ b/internal/compiler/validate_page.go @@ -3,6 +3,7 @@ package compiler import ( "fmt" "os" + "sort" "strings" "github.com/cssbruno/gowdk" @@ -419,22 +420,48 @@ func validateStorePersist(page gwdkir.Page, store gwdkir.Store, resolved gotypes }} } var diagnostics []ValidationError - for _, field := range resolved.Fields { - if !looksLikeSecretFieldName(field.Name) { - continue - } + // Persistence writes the whole value of each top-level field, so a nested + // field such as Profile.Token reaches browser storage too. Scan every field + // path the resolver recorded (top-level and nested), not just the top level. + for _, path := range secretResemblingFieldPaths(resolved) { diagnostics = append(diagnostics, ValidationError{ Code: "page_store_persist_secret_field", PageID: page.ID, Source: page.Source, Span: firstSpan(store.Span, page.Spans.Page), Severity: SeverityWarning, - Message: fmt.Sprintf("page %s store %q persists field %q, which resembles a secret; %s browser storage is readable by any script on this origin", page.ID, store.Name, field.Name, store.Persist), + Message: fmt.Sprintf("page %s store %q persists field %q, which resembles a secret; %s browser storage is readable by any script on this origin", page.ID, store.Name, path, store.Persist), }) } return diagnostics } +// secretResemblingFieldPaths returns the resolved struct's field paths (top-level +// and nested) whose leaf name resembles a secret, deduplicated and sorted for a +// stable diagnostic order. Slice/array markers ("[]") are stripped so a path +// reads like Tags.Token rather than Tags[].Token. +func secretResemblingFieldPaths(resolved gotypes.Struct) []string { + seen := map[string]bool{} + var paths []string + for raw := range resolved.FieldTypes { + path := strings.ReplaceAll(raw, "[]", "") + leaf := path + if index := strings.LastIndex(path, "."); index >= 0 { + leaf = path[index+1:] + } + if leaf == "" || !looksLikeSecretFieldName(leaf) { + continue + } + if seen[path] { + continue + } + seen[path] = true + paths = append(paths, path) + } + sort.Strings(paths) + return paths +} + // looksLikeSecretFieldName flags field names that commonly hold credentials or // trusted authorization state, which the store contract already forbids from // browser-visible state and which persistence would write to disk. diff --git a/internal/compiler/validate_page_persist_test.go b/internal/compiler/validate_page_persist_test.go index 8ac089c..29eee9f 100644 --- a/internal/compiler/validate_page_persist_test.go +++ b/internal/compiler/validate_page_persist_test.go @@ -1,6 +1,7 @@ package compiler import ( + "strings" "testing" "github.com/cssbruno/gowdk" @@ -88,6 +89,52 @@ func TestValidatePageWarnsOnPersistedSecretField(t *testing.T) { } } +func TestValidatePageWarnsOnNestedPersistedSecretField(t *testing.T) { + // ProfileState has no top-level secret-like field, but nests Account.Token. + // Persistence writes the whole Account value, so the nested secret must be + // flagged even though the top-level scan alone would miss it. + page := persistedStorePage("profile", "ProfileState", "NewProfileState", "local") + diagnostics := ValidatePage(gowdk.Config{}, irPage(page)) + diagnostic := firstDiagnostic(diagnostics, "page_store_persist_secret_field") + if diagnostic == nil { + t.Fatalf("missing nested page_store_persist_secret_field diagnostic: %#v", diagnostics) + } + if !strings.Contains(diagnostic.Message, "Account.Token") { + t.Fatalf("expected the nested field path Account.Token in the message, got %q", diagnostic.Message) + } + if diagnostic.Severity != SeverityWarning { + t.Fatalf("nested secret field should be a warning, got severity %v", diagnostic.Severity) + } + if ValidationErrors(diagnostics).HasErrors() { + t.Fatalf("nested secret field warning should not fail the build: %#v", diagnostics) + } +} + +func TestValidateWarnsOnPersistedStoreScopeConflict(t *testing.T) { + // Same store name and shape but different persist scopes share one storage + // key; the effective scope then depends on navigation order. + app := appFixture{Pages: []gwdkir.Page{ + persistedStorePageNamed("shop", "/shop", "cart", "CounterState", "NewCounterState", "local"), + persistedStorePageNamed("checkout", "/checkout", "cart", "CounterState", "NewCounterState", "session"), + }} + diagnostics := ValidateProgramReport(gowdk.Config{}, app.program(gowdk.Config{})) + d := firstDiagnostic(diagnostics, "page_store_persist_scope_conflict") + if d == nil { + t.Fatalf("missing page_store_persist_scope_conflict: %#v", diagnostics) + } + if d.Severity != SeverityWarning { + t.Fatalf("scope conflict should be a warning, got %v", d.Severity) + } + // A mismatched-shape conflict is reported separately; same shape must not + // also raise a key conflict. + if other := firstDiagnostic(diagnostics, "page_store_persist_key_conflict"); other != nil { + t.Fatalf("same-shape scope conflict must not also raise a key conflict: %#v", other) + } + if ValidationErrors(diagnostics).HasErrors() { + t.Fatalf("scope conflict alone must not fail the build: %#v", diagnostics) + } +} + func TestValidatePageDoesNotPersistCheckUnpersistedStore(t *testing.T) { // A non-persisted store with a secret-looking field must stay silent. page := persistedStorePage("session", "SessionState", "NewSessionState", "") diff --git a/internal/compiler/validate_stores.go b/internal/compiler/validate_stores.go index 6b0172e..c30b711 100644 --- a/internal/compiler/validate_stores.go +++ b/internal/compiler/validate_stores.go @@ -36,21 +36,42 @@ func validatePersistedStoreConflicts(pages []gwdkir.Page) []ValidationError { seen[store.Name] = seenStore{page: page, store: store, signature: signature} continue } - if prior.signature == signature { - continue // same shape sharing one key across routes is intended + if prior.signature != signature { + diagnostics = append(diagnostics, ValidationError{ + Code: "page_store_persist_key_conflict", + PageID: page.ID, + Source: page.Source, + Span: firstSpan(store.Span, page.Spans.Page), + Severity: SeverityWarning, + Related: relatedSpan(prior.page.Source, prior.store.Span, fmt.Sprintf("store %q first persisted here on page %s", prior.store.Name, prior.page.ID)), + Message: fmt.Sprintf( + "persisted store %q has different shapes on pages %s and %s but shares browser storage key %q; navigating between them discards each other's saved data. Rename one store or give them matching shapes", + store.Name, prior.page.ID, page.ID, "gowdk:store:"+store.Name, + ), + }) + continue } - diagnostics = append(diagnostics, ValidationError{ - Code: "page_store_persist_key_conflict", - PageID: page.ID, - Source: page.Source, - Span: firstSpan(store.Span, page.Spans.Page), - Severity: SeverityWarning, - Related: relatedSpan(prior.page.Source, prior.store.Span, fmt.Sprintf("store %q first persisted here on page %s", prior.store.Name, prior.page.ID)), - Message: fmt.Sprintf( - "persisted store %q has different shapes on pages %s and %s but shares browser storage key %q; navigating between them discards each other's saved data. Rename one store or give them matching shapes", - store.Name, prior.page.ID, page.ID, "gowdk:store:"+store.Name, - ), - }) + if prior.store.Persist != store.Persist { + // Same name and shape but different persist scopes (local vs + // session) still share one browser storage key. The runtime + // registry keeps the scope of whichever route initialized first, + // so the effective backend depends on navigation history. Flag + // it instead of silently letting nav order decide. + diagnostics = append(diagnostics, ValidationError{ + Code: "page_store_persist_scope_conflict", + PageID: page.ID, + Source: page.Source, + Span: firstSpan(store.Span, page.Spans.Page), + Severity: SeverityWarning, + Related: relatedSpan(prior.page.Source, prior.store.Span, fmt.Sprintf("store %q first persisted with scope %q here on page %s", prior.store.Name, prior.store.Persist, prior.page.ID)), + Message: fmt.Sprintf( + "persisted store %q uses scope %q on page %s but scope %q on page %s while sharing browser storage key %q; only one scope wins and which one depends on navigation order. Use the same persist scope on both pages or rename one store", + store.Name, prior.store.Persist, prior.page.ID, store.Persist, page.ID, "gowdk:store:"+store.Name, + ), + }) + continue + } + // Same name, shape, and scope is intentional cross-route sharing. } } return diagnostics diff --git a/internal/diagnostics/explain.go b/internal/diagnostics/explain.go index e195632..ffcd186 100644 --- a/internal/diagnostics/explain.go +++ b/internal/diagnostics/explain.go @@ -41,6 +41,21 @@ store cart admin.AuditState = admin.NewAuditState() persist "local"`, store cart ui.CartState = ui.NewCartState() persist "local" // pages/admin.page.gwdk store audit admin.AuditState = admin.NewAuditState() persist "local"`, + }, + "page_store_persist_scope_conflict": { + Details: "Two pages declare a persisted store with the same name and the same struct shape but different persist scopes (one local, one session). Persistence is keyed by store name (gowdk:store:), so they share one storage slot, but local uses window.localStorage and session uses window.sessionStorage. The runtime keeps whichever scope initialized first, so the effective backend — and whether the value survives a browser restart — depends on which route the user visited first. Use the same scope on both pages, or rename one store.", + NextSteps: []string{ + "Declare the same persist scope (both \"local\" or both \"session\") wherever the store is persisted.", + "Rename one of the stores so each persisted store owns its own scope and storage key.", + }, + Invalid: `// pages/shop.page.gwdk +store cart ui.CartState = ui.NewCartState() persist "local" +// pages/checkout.page.gwdk (same shape, different scope) +store cart ui.CartState = ui.NewCartState() persist "session"`, + Fixed: `// pages/shop.page.gwdk +store cart ui.CartState = ui.NewCartState() persist "local" +// pages/checkout.page.gwdk +store cart ui.CartState = ui.NewCartState() persist "local"`, }, "page_store_persist_scope_invalid": { Details: "A page store may opt into browser persistence with persist \"local\" or persist \"session\". local uses window.localStorage (survives a browser restart); session uses window.sessionStorage (survives reload and SPA navigation, cleared when the tab closes). No other scope is supported.", diff --git a/internal/diagnostics/registry.go b/internal/diagnostics/registry.go index b3d3518..0a725e0 100644 --- a/internal/diagnostics/registry.go +++ b/internal/diagnostics/registry.go @@ -131,6 +131,7 @@ var Registry = []Code{ {Code: "package_must_be_first", Area: "packages", Stability: StabilityStable, Severity: SeverityError, Summary: "package declaration is not the first non-comment declaration"}, {Code: "page_store_error", Area: "stores", Stability: StabilityExperimental, Severity: SeverityError, Summary: "page store type or initializer is invalid"}, {Code: "page_store_persist_key_conflict", Area: "stores", Stability: StabilityExperimental, Severity: SeverityWarning, Summary: "persisted stores share a storage key but have different shapes"}, + {Code: "page_store_persist_scope_conflict", Area: "stores", Stability: StabilityExperimental, Severity: SeverityWarning, Summary: "persisted stores share a storage key but use different scopes"}, {Code: "page_store_persist_scope_invalid", Area: "stores", Stability: StabilityExperimental, Severity: SeverityError, Summary: "page store persist scope is not \"local\" or \"session\""}, {Code: "page_store_persist_secret_field", Area: "stores", Stability: StabilityExperimental, Severity: SeverityWarning, Summary: "persisted page store field resembles a secret"}, {Code: "parse_error", Area: "parser", Stability: StabilityStable, Severity: SeverityError, Summary: "parser rejected source without a more specific code"}, diff --git a/testfixture/islands/islands.go b/testfixture/islands/islands.go index 6d1cf25..ba4e1f7 100644 --- a/testfixture/islands/islands.go +++ b/testfixture/islands/islands.go @@ -51,6 +51,21 @@ type SessionState struct { Open bool } +// Credentials nests a secret-resembling field. It exercises the nested +// persisted-store secret-field warning and should not be persisted in practice. +type Credentials struct { + Token string + Label string +} + +// ProfileState carries a nested Credentials value, so persisting it would write +// the nested Token to browser storage even though no top-level field looks like +// a secret. +type ProfileState struct { + Name string + Account Credentials +} + func NewCounterState() CounterState { return CounterState{Count: 1, Open: false} } @@ -59,6 +74,10 @@ func NewSessionState() SessionState { return SessionState{} } +func NewProfileState() ProfileState { + return ProfileState{} +} + func NewOtherState() OtherState { return OtherState{Name: "other"} }