From dcedbbdd885a0031dc417337594daa6e50d35a87 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:04:31 +0000 Subject: [PATCH] fix(settings): close the review findings in the validator and the theme cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects the existing tests did not reach. The web theme resolver compared the server's value against the appearance cache and fell back when they agreed, but the mirroring effect writes the server's value into that same cache — so the comparison held on the first render and stopped holding on the second, reverting an explicitly chosen theme to the default. The server's value is this account's own stored choice, so it now simply wins. The regression test re-renders rather than asserting on the first paint, which is why the original one passed. golangci-lint's exclusions.paths is a path regex, not a directory list, so a bare `web` also excluded internal/jellycompat/web_component.go, internal/webhooksync/, internal/notifications/webhook*.go and eleven other non-test files that were being linted before. Anchored. json.Number is a string kind, so `"1.5"` unmarshalled into it happily and Float64 parsed the quoted digits: a numeric setting validated as a JSON string and NormalizeValue stored the quoted form into jsonb. Rejected. The lone-surrogate check ran only on the object branch, so a lone surrogate in ui.custom_css decoded to U+FFFD on SQLite and was refused outright by Postgres jsonb — the two backends disagreeing about whether the same value could be stored. Hoisted to cover every type. The strict language-tag validation this branch added is correct, but it rejects what the shipped Android client sends; the companion fix is silo-android 4aeb78b4. Co-Authored-By: Claude Opus 5 (1M context) --- .golangci.yml | 9 ++- internal/settingscontract/contract_test.go | 79 +++++++++++++++++++ internal/settingscontract/strictjson.go | 15 ++++ internal/settingscontract/validate.go | 17 +++- .../hooks/appearanceCacheOwnership.test.tsx | 25 ++++++ web/src/hooks/useTheme.tsx | 19 ++--- 6 files changed, 146 insertions(+), 18 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 4af9fbe2..bef4244f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -60,9 +60,14 @@ linters: locale: US exclusions: + # Anchored regexes, not directory names: `paths` matches anywhere in the + # path, so a bare `web` also excluded internal/jellycompat/web_component.go, + # internal/webhooksync/, internal/notifications/webhook*.go and every other + # non-test file with "web" in its name — 14 files that were being linted + # before. paths: - - web - - migrations + - ^web/ + - ^migrations/ rules: # Allow repeated strings in test files diff --git a/internal/settingscontract/contract_test.go b/internal/settingscontract/contract_test.go index 31f8d531..4cf2bfd5 100644 --- a/internal/settingscontract/contract_test.go +++ b/internal/settingscontract/contract_test.go @@ -1173,3 +1173,82 @@ func TestNormalizeValueCanonicalizesLanguageTags(t *testing.T) { t.Error(`NormalizeValue("") was accepted; unset must be null, not the empty string`) } } + +// TestQuotedNumbersAreRejected covers a type confusion encoding/json will not +// catch. json.Number is a string kind, so `"1.5"` unmarshals into it happily +// and Float64 then parses the quoted digits — the value validates, and +// NormalizeValue stores the quoted form into jsonb where every consumer reading +// it as a number disagrees with the row. +func TestQuotedNumbersAreRejected(t *testing.T) { + for name, tc := range map[string]struct { + schema *ValueSchema + raw string + }{ + "number": {&ValueSchema{Type: TypeNumber}, `"1.5"`}, + "integer": {&ValueSchema{Type: TypeInteger}, `"3"`}, + "integer with sign": {&ValueSchema{Type: TypeInteger}, `"-3"`}, + } { + t.Run(name, func(t *testing.T) { + if err := tc.schema.ValidateValue(json.RawMessage(tc.raw), nil); err == nil { + t.Fatalf("%s was accepted for a %s setting", tc.raw, tc.schema.Type) + } + if _, err := tc.schema.NormalizeValue(json.RawMessage(tc.raw), nil); err == nil { + t.Fatalf("%s was stored for a %s setting", tc.raw, tc.schema.Type) + } + }) + } + + // The unquoted forms still validate, so the check is not rejecting numbers + // outright. + if err := (&ValueSchema{Type: TypeNumber}).ValidateValue( + json.RawMessage(`1.5`), nil); err != nil { + t.Errorf("1.5 was rejected for a number setting: %v", err) + } + if err := (&ValueSchema{Type: TypeInteger}).ValidateValue( + json.RawMessage(`3`), nil); err != nil { + t.Errorf("3 was rejected for an integer setting: %v", err) + } +} + +// TestLoneSurrogatesAreRejectedForEveryType pins the check to the whole +// validation path rather than the object branch it started in. A lone surrogate +// decodes to U+FFFD on SQLite and is refused outright by Postgres jsonb, so a +// string setting that skipped the check made the two backends disagree about +// whether the same value could be stored at all. +func TestLoneSurrogatesAreRejectedForEveryType(t *testing.T) { + for name, schema := range map[string]*ValueSchema{ + "string": {Type: TypeString}, + "language tag": {Type: TypeLanguageTag}, + "enum": {Type: TypeEnum, Values: []EnumMember{{Value: "a"}}}, + } { + t.Run(name, func(t *testing.T) { + raw := json.RawMessage(`"\ud800"`) + if err := schema.ValidateValue(raw, nil); err == nil { + t.Fatal("a lone surrogate was accepted") + } + if _, err := schema.NormalizeValue(raw, nil); err == nil { + t.Fatal("a lone surrogate was stored") + } + }) + } + + // ui.custom_css is the setting that actually carries free text, so pin it + // against the real manifest definition too. + manifest, err := Load() + if err != nil { + t.Fatalf("loading manifest: %v", err) + } + def, ok := manifest.Lookup("ui.custom_css") + if !ok { + t.Fatal("ui.custom_css is not registered") + } + if err := def.ValueSchema.ValidateValue( + json.RawMessage(`"body { content: \"\ud800\"; }"`), objSchemas); err == nil { + t.Error("ui.custom_css accepted a lone surrogate") + } + // A well-formed pair is an ordinary character and must still store. + if err := def.ValueSchema.ValidateValue( + json.RawMessage(`"body::after { content: \"😀\"; }"`), objSchemas); err != nil { + t.Errorf("ui.custom_css rejected a valid surrogate pair: %v", err) + } +} diff --git a/internal/settingscontract/strictjson.go b/internal/settingscontract/strictjson.go index 8d23c9e3..889f8c2b 100644 --- a/internal/settingscontract/strictjson.go +++ b/internal/settingscontract/strictjson.go @@ -13,6 +13,21 @@ import ( // input that Go accepts by silently changing it, which is the dangerous shape // for a contract whose whole promise is that every peer agrees on the bytes. +// rejectQuotedNumber reports an error if a value declared numeric arrived as a +// JSON string. +// +// json.Number is a string kind, so encoding/json unmarshals `"1.5"` into it +// without complaint and Float64/Int64 then parse the quoted digits happily. The +// value would validate, and NormalizeValue would store the quoted form into +// jsonb — where every consumer reading it as a number, and every client +// comparing canonical bytes, disagrees with the row. +func rejectQuotedNumber(raw []byte) error { + if len(raw) > 0 && raw[0] == '"' { + return errors.New("a numeric setting must not be sent as a JSON string") + } + return nil +} + // maxJSONDepth bounds the recursion in the duplicate-key scan. Setting values // arrive from the network, and the deepest schema the contract declares nests // three levels, so this is far above anything legitimate and still cannot be diff --git a/internal/settingscontract/validate.go b/internal/settingscontract/validate.go index 685738b0..e6bc76be 100644 --- a/internal/settingscontract/validate.go +++ b/internal/settingscontract/validate.go @@ -389,6 +389,14 @@ func (v *ValueSchema) ValidateValue(raw json.RawMessage, objectSchemas map[strin return errors.New("null is not allowed for this setting") } + // Every type that can carry a string, not just the object branch: a lone + // surrogate in a plain string setting (ui.custom_css) decodes to U+FFFD on + // SQLite and is rejected outright by Postgres jsonb, so leaving this to the + // object case alone let the two backends disagree about the same value. + if err := rejectLoneSurrogates(trimmed); err != nil { + return err + } + switch v.Type { case TypeBoolean: var value bool @@ -401,6 +409,9 @@ func (v *ValueSchema) ValidateValue(raw json.RawMessage, objectSchemas map[strin if err := strictUnmarshal(trimmed, &value); err != nil { return fmt.Errorf("expected an integer: %w", err) } + if err := rejectQuotedNumber(trimmed); err != nil { + return fmt.Errorf("expected an integer: %w", err) + } parsed, err := value.Int64() if err != nil { return fmt.Errorf("expected an integer, got %s", value) @@ -412,6 +423,9 @@ func (v *ValueSchema) ValidateValue(raw json.RawMessage, objectSchemas map[strin if err := strictUnmarshal(trimmed, &value); err != nil { return fmt.Errorf("expected a number: %w", err) } + if err := rejectQuotedNumber(trimmed); err != nil { + return fmt.Errorf("expected a number: %w", err) + } parsed, err := value.Float64() if err != nil { return fmt.Errorf("expected a number, got %s", value) @@ -457,9 +471,6 @@ func (v *ValueSchema) ValidateValue(raw json.RawMessage, objectSchemas map[strin if err := rejectDuplicateKeys(trimmed); err != nil { return err } - if err := rejectLoneSurrogates(trimmed); err != nil { - return err - } doc, err := jsonschema.UnmarshalJSON(bytes.NewReader(trimmed)) if err != nil { return fmt.Errorf("expected an object: %w", err) diff --git a/web/src/hooks/appearanceCacheOwnership.test.tsx b/web/src/hooks/appearanceCacheOwnership.test.tsx index 486055ee..8d3b47bd 100644 --- a/web/src/hooks/appearanceCacheOwnership.test.tsx +++ b/web/src/hooks/appearanceCacheOwnership.test.tsx @@ -186,6 +186,31 @@ describe("appearance cache ownership", () => { expect(captured.custom.customCss).toBe("body { color: blue; }"); }); + it("keeps applying the server's theme once the mirror has written it back", () => { + signedInAs(2); + mocks.useSettings.mockReturnValue({ + data: { ui_theme: "oxblood-noir", ui_text_scale: "x-large" }, + }); + + const view = renderAppearance(); + expect(view.captured.theme.theme).toBe("oxblood-noir"); + + // The mirror effect writes the server's theme into the same namespace the + // resolver reads. A resolver that compared the two would see them agree + // here and fall back to the default from the second render on, so this + // re-renders rather than trusting the first paint. + act(() => { + view.rerender(); + }); + act(() => { + view.rerender(); + }); + + expect(view.captured.theme.theme).toBe("oxblood-noir"); + expect(view.captured.theme.textScale).toBe("x-large"); + expect(document.documentElement.getAttribute("data-theme")).toBe("oxblood-noir"); + }); + it("mirrors the server's appearance so the next cold start paints it", () => { signedInAs(2); mocks.useSettings.mockReturnValue({ diff --git a/web/src/hooks/useTheme.tsx b/web/src/hooks/useTheme.tsx index cdb8eb9a..c415296d 100644 --- a/web/src/hooks/useTheme.tsx +++ b/web/src/hooks/useTheme.tsx @@ -110,10 +110,12 @@ export function ThemeProvider({ children }: { children: ReactNode }) { const fallbackTheme: ThemeId = !hasStoredThemeChoice && isValidTheme(adminDefaultTheme) ? adminDefaultTheme : localTheme; - const theme = - loadApiTheme && apiTheme - ? getInitialThemeFromApi(apiTheme, fallbackTheme, cacheOwner) - : fallbackTheme; + // The server's value is this account's own stored choice, so it wins outright + // whenever it is present and valid. It is deliberately not compared against + // the local cache: the effect below mirrors the server's value into that very + // cache, so any such comparison stops holding after the first render and the + // theme silently reverts to the default on the second. + const theme = loadApiTheme && isValidTheme(apiTheme) ? apiTheme : fallbackTheme; const textScale = loadApiTheme ? parseTextScale(apiTextScale ?? localTextScale) : localTextScale; const textWeight = loadApiTheme ? parseTextWeight(apiTextWeight ?? localTextWeight) @@ -243,12 +245,3 @@ export function useTheme(): ThemeContextValue { if (!ctx) throw new Error("useTheme must be used within ThemeProvider"); return ctx; } - -function getInitialThemeFromApi( - apiTheme: string | null, - fallback: ThemeId, - cacheOwner: string | null, -): ThemeId { - if (!apiTheme || !isValidTheme(apiTheme)) return fallback; - return appearanceCache.get(storage.KEYS.THEME, cacheOwner) !== apiTheme ? apiTheme : fallback; -}