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; -}