From c57093544de9171e1e3e2ae3488d450a20181298 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 11:53:38 +0000 Subject: [PATCH 1/9] fix: restore user settings displaced by collection forced settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A collection's `forced` settings from a remote settings.json were written straight into the user's persisted global settings with no backup, and the only teardown action (clearCollectionForcedSettings) had zero callers. Viewing one hostile allowlisted-CDN source once — reachable via a single unconfirmed /gh// link that auto-adds and activates the source — permanently overwrote the visitor's display config (title mapping, badges, card face) across reloads and source switches, with several keys unrecoverable except a full settings reset. Snapshot the user's own value for each key a source forces (source-scoped, so a refetch of the same source cannot clobber the backup with already-forced values), restore it when the active source changes, and roll it back on rehydration for crash recovery — mirroring the _mechanicOverridesBackup pattern. The restore runs from a dedicated effect on source change because a source with no settings.json never reaches applyCollectionSettings. The dead clearCollectionForcedSettings action is repurposed into restoreCollectionForcedSettings. Assisted-by: Claude:claude-fable-5 --- src/context/CollectionDataContext.tsx | 14 ++ src/stores/settingsStore.ts | 166 +++++++++++--- ...lectionDataContext.forcedSettings.test.tsx | 97 ++++++++ .../settingsStore.collectionForced.test.ts | 212 ++++++++++++++++++ 4 files changed, 463 insertions(+), 26 deletions(-) create mode 100644 tests/context/CollectionDataContext.forcedSettings.test.tsx create mode 100644 tests/stores/settingsStore.collectionForced.test.ts diff --git a/src/context/CollectionDataContext.tsx b/src/context/CollectionDataContext.tsx index 195f038..81f692c 100644 --- a/src/context/CollectionDataContext.tsx +++ b/src/context/CollectionDataContext.tsx @@ -70,6 +70,10 @@ export function CollectionDataProvider({ children }: CollectionDataProviderProps ); const applyCollectionDefaults = useSettingsStore((s) => s.applyCollectionDefaults); const applyCollectionSettings = useSettingsStore((s) => s.applyCollectionSettings); + const restoreCollectionForcedSettings = useSettingsStore( + (s) => s.restoreCollectionForcedSettings + ); + const forcedSourceId = useSettingsStore((s) => s.collectionForcedSourceId); const applySmartSelectionDefault = useSettingsStore((s) => s.applySmartSelectionDefault); const hasAppliedDefaults = useSettingsStore((s) => s.hasAppliedCollectionDefaults); const edits = useEditsStore((s) => s.edits); @@ -81,6 +85,16 @@ export function CollectionDataProvider({ children }: CollectionDataProviderProps } }, [data?.config, hasAppliedDefaults, applyCollectionDefaults]); + // Revert a previous collection's forced settings as soon as the active + // source changes. This must run before the apply effect below and cannot be + // folded into it: a source with no settings.json never reaches + // applyCollectionSettings, so nothing else would restore the user's settings. + useEffect(() => { + if (forcedSourceId && forcedSourceId !== sourceUrl) { + restoreCollectionForcedSettings(); + } + }, [sourceUrl, forcedSourceId, restoreCollectionForcedSettings]); + // Apply collection-specific settings from settings.json (every load) // Uses sourceUrl as the sourceId for tracking which collection's defaults have been applied useEffect(() => { diff --git a/src/stores/settingsStore.ts b/src/stores/settingsStore.ts index 7485e59..d9c2821 100644 --- a/src/stores/settingsStore.ts +++ b/src/stores/settingsStore.ts @@ -352,6 +352,23 @@ type DraftSettings = { [K in DraftableSettingsKeys]?: SettingsState[K]; }; +/** + * The settings keys a collection's `forced` settings can displace. The backup + * snapshots the user's own value for each key a source forces, so it can be + * restored when the source changes. + */ +type CollectionForcedKey = + | "fieldMapping" + | "defaultCardFace" + | "cardBackDisplay" + | "cardBackStyle" + | "titleDisplayMode" + | "showRankBadge" + | "showDeviceBadge" + | "rankPlaceholderText"; + +type CollectionForcedBackup = Partial>; + /** * Settings store state. */ @@ -515,6 +532,16 @@ interface SettingsState { /** Collection forced settings (applied on every load, user cannot override) */ collectionForcedSettings: ForcedSettings | null; + /** + * Backup of the user's own settings displaced by a collection's `forced` + * settings, so they can be restored when the source changes. Persisted for + * crash recovery (mirrors _mechanicOverridesBackup). + */ + _collectionForcedBackup: CollectionForcedBackup | null; + + /** Source ID whose forced settings are currently applied (null = none) */ + collectionForcedSourceId: string | null; + /** Source ID of the collection whose defaults were applied (prevents re-applying) */ appliedCollectionDefaultsSourceId: string | null; @@ -610,7 +637,12 @@ interface SettingsState { setShowViewButton: (show: boolean) => void; setUsePlaceholderImages: (use: boolean) => void; applyCollectionSettings: (sourceId: string, settings: CollectionSettings) => void; - clearCollectionForcedSettings: () => void; + /** + * Restore the user's own settings displaced by a collection's forced + * settings. Called when the active source changes away from the one whose + * forced settings are applied. + */ + restoreCollectionForcedSettings: () => void; // v0.14.0: Draft State Actions (F-090) /** Start editing - creates draft from current committed state */ @@ -718,6 +750,8 @@ const DEFAULT_SETTINGS = { showViewButton: true, usePlaceholderImages: true, collectionForcedSettings: null as ForcedSettings | null, + _collectionForcedBackup: null as CollectionForcedBackup | null, + collectionForcedSourceId: null as string | null, appliedCollectionDefaultsSourceId: null as string | null, // v0.14.0: Draft State defaults (F-090) _draft: null as DraftSettings | null, @@ -1050,37 +1084,87 @@ export const useSettingsStore = create()( // Always apply forced settings (user cannot override) if (settings.forced) { - updates.collectionForcedSettings = settings.forced; + const forced = settings.forced; + const isNewSource = state.collectionForcedSourceId !== sourceId; + + // When switching to a different forced source, restore the previous + // source's displaced values first so the new backup captures the + // user's own settings, never the previous source's forced ones. + const base: SettingsState = + isNewSource && state._collectionForcedBackup + ? { ...state, ...state._collectionForcedBackup } + : state; + + if (isNewSource) { + // Snapshot the user's own value for each key THIS source forces, + // so unrelated user settings are never rolled back. Snapshot once + // per source (guarded by isNewSource) so a refetch of the same + // source cannot clobber the backup with already-forced values. + const backup: CollectionForcedBackup = {}; + if (forced.fieldMapping) backup.fieldMapping = base.fieldMapping; + if (forced.defaultCardFace !== undefined) { + backup.defaultCardFace = base.defaultCardFace; + } + if (forced.cardBackDisplay !== undefined) { + backup.cardBackDisplay = base.cardBackDisplay; + } + if (forced.cardBackStyle !== undefined) { + backup.cardBackStyle = base.cardBackStyle; + } + if (forced.titleDisplayMode !== undefined) { + backup.titleDisplayMode = base.titleDisplayMode; + } + if (forced.showRankBadge !== undefined) { + backup.showRankBadge = base.showRankBadge; + } + if (forced.showDeviceBadge !== undefined) { + backup.showDeviceBadge = base.showDeviceBadge; + } + if (forced.rankPlaceholderText !== undefined) { + backup.rankPlaceholderText = base.rankPlaceholderText; + } + + updates._collectionForcedBackup = + Object.keys(backup).length > 0 ? backup : null; + updates.collectionForcedSourceId = sourceId; + + // Carry the previous source's restored values through for keys the + // new source does NOT force (the forced assignments below win for + // keys it does force). + Object.assign(updates, state._collectionForcedBackup ?? {}); + } + + updates.collectionForcedSettings = forced; - // Apply forced field mapping - if (settings.forced.fieldMapping) { + // Apply forced field mapping (merged onto the user's own mapping) + if (forced.fieldMapping) { updates.fieldMapping = { - ...state.fieldMapping, - ...settings.forced.fieldMapping, + ...base.fieldMapping, + ...forced.fieldMapping, } as FieldMappingConfig; } // Apply forced card settings - if (settings.forced.defaultCardFace !== undefined) { - updates.defaultCardFace = settings.forced.defaultCardFace; + if (forced.defaultCardFace !== undefined) { + updates.defaultCardFace = forced.defaultCardFace; } - if (settings.forced.cardBackDisplay !== undefined) { - updates.cardBackDisplay = settings.forced.cardBackDisplay; + if (forced.cardBackDisplay !== undefined) { + updates.cardBackDisplay = forced.cardBackDisplay; } - if (settings.forced.cardBackStyle !== undefined) { - updates.cardBackStyle = settings.forced.cardBackStyle; + if (forced.cardBackStyle !== undefined) { + updates.cardBackStyle = forced.cardBackStyle; } - if (settings.forced.titleDisplayMode !== undefined) { - updates.titleDisplayMode = settings.forced.titleDisplayMode; + if (forced.titleDisplayMode !== undefined) { + updates.titleDisplayMode = forced.titleDisplayMode; } - if (settings.forced.showRankBadge !== undefined) { - updates.showRankBadge = settings.forced.showRankBadge; + if (forced.showRankBadge !== undefined) { + updates.showRankBadge = forced.showRankBadge; } - if (settings.forced.showDeviceBadge !== undefined) { - updates.showDeviceBadge = settings.forced.showDeviceBadge; + if (forced.showDeviceBadge !== undefined) { + updates.showDeviceBadge = forced.showDeviceBadge; } - if (settings.forced.rankPlaceholderText !== undefined) { - updates.rankPlaceholderText = settings.forced.rankPlaceholderText; + if (forced.rankPlaceholderText !== undefined) { + updates.rankPlaceholderText = forced.rankPlaceholderText; } } @@ -1131,10 +1215,23 @@ export const useSettingsStore = create()( }); }, - clearCollectionForcedSettings: () => { - set({ - collectionForcedSettings: null, - appliedCollectionDefaultsSourceId: null, + restoreCollectionForcedSettings: () => { + set((state) => { + if ( + !state.collectionForcedSourceId && + !state._collectionForcedBackup + ) { + return state; + } + return { + ...(state._collectionForcedBackup ?? {}), + collectionForcedSettings: null, + _collectionForcedBackup: null, + collectionForcedSourceId: null, + // appliedCollectionDefaultsSourceId is intentionally untouched: + // clearing it would let the same source re-apply its one-shot + // `defaults` over the user's later choices. + }; }); }, @@ -1360,6 +1457,18 @@ export const useSettingsStore = create()( state._mechanicOverridesBackup = null; state.mechanicOverridesActive = false; } + + // A persisted forced-settings backup means the previous session applied + // a collection's forced settings and never reverted them (crash or + // tab-kill). Roll them back; if the same source is still active, the + // load effect re-applies and re-snapshots from this clean baseline. + const forcedBackup = state?._collectionForcedBackup; + if (state && forcedBackup) { + Object.assign(state, forcedBackup); + state._collectionForcedBackup = null; + state.collectionForcedSourceId = null; + state.collectionForcedSettings = null; + } }, partialize: (state) => ({ layout: state.layout, @@ -1408,8 +1517,13 @@ export const useSettingsStore = create()( cacheConsentDenied: state.cacheConsentDenied, // v0.11.5: Collection Settings showViewButton: state.showViewButton, - // Note: collectionForcedSettings is intentionally NOT persisted - // Forced settings are applied fresh from collection on each load + // Note: collectionForcedSettings (the transient marker) is intentionally + // NOT persisted. The backup and source id ARE persisted so a crash or + // tab-kill while a collection's forced settings are applied cannot + // permanently overwrite the user's own settings; onRehydrateStorage + // rolls them back from the backup. + _collectionForcedBackup: state._collectionForcedBackup, + collectionForcedSourceId: state.collectionForcedSourceId, appliedCollectionDefaultsSourceId: state.appliedCollectionDefaultsSourceId, // v0.14.0: Draft state is intentionally NOT persisted // _draft and isDirty are excluded - editing session is transient diff --git a/tests/context/CollectionDataContext.forcedSettings.test.tsx b/tests/context/CollectionDataContext.forcedSettings.test.tsx new file mode 100644 index 0000000..027914c --- /dev/null +++ b/tests/context/CollectionDataContext.forcedSettings.test.tsx @@ -0,0 +1,97 @@ +/** + * Integration test for forced-settings restore on source change. + * + * A collection's `forced` settings overwrite the user's persisted global + * settings. When the user switches to a different source — especially one with + * no settings.json, which never reaches applyCollectionSettings — the + * provider must restore the user's own settings via the dedicated restore + * effect. This is the exact end-to-end failure the store backup guards. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render } from "@testing-library/react"; +import { CollectionDataProvider } from "@/context/CollectionDataContext"; +import { useSettingsStore } from "@/stores/settingsStore"; +import { useSourceStore, type Source } from "@/stores/sourceStore"; +import type { CollectionSettings } from "@/types/collectionSettings"; + +const SOURCE_A_ID = "src_a"; +const SOURCE_B_ID = "src_b"; +const SOURCE_A_URL = "https://cdn.jsdelivr.net/gh/a/MyPlausibleMe@main/x"; +const SOURCE_B_URL = "https://cdn.jsdelivr.net/gh/b/MyPlausibleMe@main/y"; + +// Source A forces cardBackDisplay; source B has no settings.json at all. +const forcedForA: CollectionSettings = { + forced: { cardBackDisplay: "none" }, +}; + +// useLocalCollection returns settings based on the active source URL so the +// provider's effects see A's forced settings, then nothing for B. +vi.mock("@/hooks/useCollection", () => ({ + useLocalCollection: ({ basePath }: { basePath: string }) => ({ + data: { + cards: [{ id: "c1", title: "One" }], + collection: undefined, + displayConfig: undefined, + config: undefined, + settings: basePath === SOURCE_A_URL ? forcedForA : undefined, + }, + isLoading: false, + error: null, + isSuccess: true, + }), +})); + +function makeSource(id: string, url: string): Source { + return { + id, + url, + name: id, + addedAt: new Date(), + sourceType: "myplausibleme", + }; +} + +describe("CollectionDataProvider - forced settings restore on source change", () => { + beforeEach(() => { + useSettingsStore.getState().resetToDefaults(); + useSourceStore.setState({ + sources: [ + makeSource(SOURCE_A_ID, SOURCE_A_URL), + makeSource(SOURCE_B_ID, SOURCE_B_URL), + ], + activeSourceId: SOURCE_A_ID, + defaultSourceId: SOURCE_A_ID, + }); + }); + + it("restores the user's own settings when switching to a source with no settings.json", () => { + // The user's own value. + useSettingsStore.setState({ cardBackDisplay: "logo" }); + + const { rerender } = render( + +
child
+ + ); + + // Source A's forced value is now applied and backed up. + expect(useSettingsStore.getState().cardBackDisplay).toBe("none"); + expect(useSettingsStore.getState().collectionForcedSourceId).toBe( + SOURCE_A_URL + ); + + // Switch the active source to B (which serves no settings.json). + useSourceStore.setState({ activeSourceId: SOURCE_B_ID }); + rerender( + +
child
+ + ); + + // The user's own setting is restored; the forced state is cleared. + expect(useSettingsStore.getState().cardBackDisplay).toBe("logo"); + expect(useSettingsStore.getState().collectionForcedSourceId).toBeNull(); + expect(useSettingsStore.getState()._collectionForcedBackup).toBeNull(); + }); +}); diff --git a/tests/stores/settingsStore.collectionForced.test.ts b/tests/stores/settingsStore.collectionForced.test.ts new file mode 100644 index 0000000..03353c5 --- /dev/null +++ b/tests/stores/settingsStore.collectionForced.test.ts @@ -0,0 +1,212 @@ +/** + * Tests for collection forced-settings backup/restore. + * + * A collection's `forced` settings are written into the user's persisted + * global settings. Without a backup, viewing one hostile source once + * permanently overwrites the user's own display config, with no revert path. + * The store must snapshot the displaced values per source, restore them when + * the source changes, and survive a crash via rehydration — mirroring the + * mechanic-overrides backup. + */ + +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { useSettingsStore } from "@/stores/settingsStore"; +import type { CollectionSettings } from "@/types/collectionSettings"; + +const STORAGE_KEY = "itemdeck-settings"; + +function getSetItemMock() { + return vi.mocked(window.localStorage.setItem); +} + +function getGetItemMock() { + return vi.mocked(window.localStorage.getItem); +} + +function lastPersistedState(): Record { + const writes = getSetItemMock().mock.calls.filter( + ([key]) => key === STORAGE_KEY + ); + const lastWrite = writes[writes.length - 1]; + if (!lastWrite) throw new Error("expected a persisted write"); + const parsed = JSON.parse(lastWrite[1]) as { + state: Record; + }; + return parsed.state; +} + +const SOURCE_A = "https://cdn.jsdelivr.net/gh/a/MyPlausibleMe@main/x"; +const SOURCE_B = "https://cdn.jsdelivr.net/gh/b/MyPlausibleMe@main/y"; + +describe("settingsStore - collection forced-settings backup/restore", () => { + beforeEach(() => { + useSettingsStore.getState().resetToDefaults(); + getSetItemMock().mockClear(); + getGetItemMock().mockReset(); + }); + + it("snapshots only the user's own values for keys the source forces", () => { + // User's own settings. + useSettingsStore.setState({ + cardBackDisplay: "logo", + showRankBadge: true, + showDeviceBadge: true, + }); + + const settings: CollectionSettings = { + forced: { cardBackDisplay: "none", showRankBadge: false }, + }; + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, settings); + + const state = useSettingsStore.getState(); + // Forced values are applied. + expect(state.cardBackDisplay).toBe("none"); + expect(state.showRankBadge).toBe(false); + // The backup holds the user's originals for the forced keys only. + expect(state._collectionForcedBackup).toEqual({ + cardBackDisplay: "logo", + showRankBadge: true, + }); + expect(state.collectionForcedSourceId).toBe(SOURCE_A); + // showDeviceBadge was not forced, so it is not in the backup. + expect(state._collectionForcedBackup).not.toHaveProperty("showDeviceBadge"); + }); + + it("does not re-snapshot when the same source re-applies (idempotent)", () => { + useSettingsStore.setState({ cardBackDisplay: "logo" }); + const settings: CollectionSettings = { + forced: { cardBackDisplay: "none" }, + }; + + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, settings); + // A refetch re-applies the same forced settings; the backup must still hold + // the ORIGINAL user value, not the now-forced "none". + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, settings); + + expect(useSettingsStore.getState()._collectionForcedBackup).toEqual({ + cardBackDisplay: "logo", + }); + }); + + it("restores the user's own values and clears backup state on restore", () => { + useSettingsStore.setState({ + cardBackDisplay: "logo", + appliedCollectionDefaultsSourceId: SOURCE_A, + }); + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, { + forced: { cardBackDisplay: "none" }, + }); + + useSettingsStore.getState().restoreCollectionForcedSettings(); + + const state = useSettingsStore.getState(); + expect(state.cardBackDisplay).toBe("logo"); + expect(state.collectionForcedSettings).toBeNull(); + expect(state._collectionForcedBackup).toBeNull(); + expect(state.collectionForcedSourceId).toBeNull(); + // appliedCollectionDefaultsSourceId must NOT be cleared by a forced restore. + expect(state.appliedCollectionDefaultsSourceId).toBe(SOURCE_A); + }); + + it("captures the user's originals (not source A's forced values) when chaining A → B", () => { + useSettingsStore.setState({ + cardBackDisplay: "logo", + showRankBadge: true, + }); + + // Source A forces cardBackDisplay. + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, { + forced: { cardBackDisplay: "none" }, + }); + // Source B forces a different key plus the overlapping one, with no + // explicit restore between (e.g. a direct source switch). + useSettingsStore.getState().applyCollectionSettings(SOURCE_B, { + forced: { cardBackDisplay: "both", showRankBadge: false }, + }); + + const state = useSettingsStore.getState(); + expect(state.collectionForcedSourceId).toBe(SOURCE_B); + // B's backup must hold the user's true originals, not A's forced "none". + expect(state._collectionForcedBackup).toEqual({ + cardBackDisplay: "logo", + showRankBadge: true, + }); + + // Restoring now returns the user's real settings. + useSettingsStore.getState().restoreCollectionForcedSettings(); + const restored = useSettingsStore.getState(); + expect(restored.cardBackDisplay).toBe("logo"); + expect(restored.showRankBadge).toBe(true); + }); + + it("does not roll back a key the source never forced", () => { + useSettingsStore.setState({ + cardBackDisplay: "logo", + showDeviceBadge: true, + }); + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, { + forced: { cardBackDisplay: "none" }, + }); + // User changes a non-forced key while the forced source is active. + useSettingsStore.setState({ showDeviceBadge: false }); + + useSettingsStore.getState().restoreCollectionForcedSettings(); + + // The forced key is restored; the untouched key keeps the user's new value. + expect(useSettingsStore.getState().cardBackDisplay).toBe("logo"); + expect(useSettingsStore.getState().showDeviceBadge).toBe(false); + }); + + it("persists the backup and source id", () => { + useSettingsStore.setState({ cardBackDisplay: "logo" }); + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, { + forced: { cardBackDisplay: "none" }, + }); + + const persisted = lastPersistedState(); + expect(persisted._collectionForcedBackup).toEqual({ + cardBackDisplay: "logo", + }); + expect(persisted.collectionForcedSourceId).toBe(SOURCE_A); + // The transient marker is not persisted. + expect(persisted.collectionForcedSettings).toBeUndefined(); + }); + + it("rolls back forced settings from a persisted backup on rehydration (crash recovery)", async () => { + const persisted = JSON.stringify({ + state: { + cardBackDisplay: "none", + showRankBadge: false, + _collectionForcedBackup: { + cardBackDisplay: "logo", + showRankBadge: true, + }, + collectionForcedSourceId: SOURCE_A, + }, + version: 27, + }); + getGetItemMock().mockReturnValue(persisted); + + await useSettingsStore.persist.rehydrate(); + + const state = useSettingsStore.getState(); + expect(state.cardBackDisplay).toBe("logo"); + expect(state.showRankBadge).toBe(true); + expect(state._collectionForcedBackup).toBeNull(); + expect(state.collectionForcedSourceId).toBeNull(); + expect(state.collectionForcedSettings).toBeNull(); + }); + + it("leaves settings untouched on rehydration when no forced backup is stored", async () => { + const persisted = JSON.stringify({ + state: { cardBackDisplay: "both" }, + version: 27, + }); + getGetItemMock().mockReturnValue(persisted); + + await useSettingsStore.persist.rehydrate(); + + expect(useSettingsStore.getState().cardBackDisplay).toBe("both"); + expect(useSettingsStore.getState()._collectionForcedBackup).toBeNull(); + }); +}); From d0bfec0b68fd71c4b7be43df321518c453f3066b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 11:53:48 +0000 Subject: [PATCH 2/9] fix: coerce untrusted entity text fields and cap media per card MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two untrusted-input hardenings in the entity-to-DisplayCard transform: - summary and the resolved platform title/shortTitle/summary/year were cast to string instead of coerced. The v2 entity schema is .loose(), so an object-typed value (e.g. an i18n {en: "..."} field) passed through truthy and reached JSX as a child, where React throws "Objects are not valid as a React child" and the whole collection view fails — the device badge sink fires on first paint in the default grid. Coerce these fields with a shared helper, matching the existing title/year/videos guards. - card.imageUrls (images + videos) was unbounded. It feeds the gallery (one dot button per entry) and the load-time preloader (one cache probe + fetch per entry), so a hostile entity listing a huge media array could freeze the tab or flood the CDN. Cap the combined list. Assisted-by: Claude:claude-fable-5 --- src/hooks/useCollection.ts | 56 ++++++++--- tests/hooks/useCollection.untrusted.test.ts | 106 +++++++++++++++++++- 2 files changed, 148 insertions(+), 14 deletions(-) diff --git a/src/hooks/useCollection.ts b/src/hooks/useCollection.ts index 18935d8..8cb7ebe 100644 --- a/src/hooks/useCollection.ts +++ b/src/hooks/useCollection.ts @@ -150,6 +150,32 @@ interface CollectionResult { isStale?: boolean; } +/** + * Upper bound on media items (images + videos) kept per card. + * + * The v2 `images`/`videos` arrays are untrusted and uncapped in the schema. + * `card.imageUrls` feeds the gallery (one dot button each) and the load-time + * preloader (one cache probe + fetch each), so an entity listing a huge array + * would freeze the tab / flood the CDN. Real cards carry a few media items. + */ +const MAX_MEDIA_PER_CARD = 100; + +/** + * Coerce an untrusted entity field to a display string. + * + * The v2 entity schema is `.loose()`, so text fields (summary, platform + * title/shortTitle/summary/year, …) may hold arbitrary JSON. An object left + * as-is is truthy and reaches JSX as a child, where React throws "Objects are + * not valid as a React child" during render — a single such field can deny the + * whole collection view. Mirror the inline coercion already used for + * `title`/`year`: keep strings, stringify numbers, drop everything else. + */ +function toDisplayString(value: unknown): string | undefined { + if (typeof value === "string") return value; + if (typeof value === "number") return String(value); + return undefined; +} + /** * Format attribution from structured Image objects. * Collects all unique attributions from all images. @@ -317,8 +343,15 @@ async function loadFreshCollection( .filter((u): u is string => u !== undefined) ?? []), ]; - // Combine image URLs with video URLs for the gallery - const allMediaUrls = [...imageUrls, ...videoUrls]; + // Combine image URLs with video URLs for the gallery. The images/videos + // arrays are untrusted and uncapped in the schema, and this list feeds + // both the gallery (one dot button per entry) and the load-time image + // preloader (one cache probe + fetch per entry), so a hostile entity + // could otherwise mount tens of thousands of nodes or requests. Cap it. + const allMediaUrls = [...imageUrls, ...videoUrls].slice( + 0, + MAX_MEDIA_PER_CARD + ); // Get resolved platform const platform = entity._resolved?.platform as ResolvedEntity | undefined; @@ -365,10 +398,10 @@ async function loadFreshCollection( | undefined ); - // v2: Use generic terminology - const categoryShort = (platform?.shortTitle ?? platform?.title) as - | string - | undefined; + // v2: Use generic terminology. `shortTitle`/`title` are untrusted (loose + // schema) and this feeds the device badge, rendered as a JSX child. + const categoryShort = + toDisplayString(platform?.shortTitle) ?? toDisplayString(platform?.title); const order = rank; // Build DisplayCard with all entity fields for field path resolution @@ -377,13 +410,13 @@ async function loadFreshCollection( id: entity.id, title, year, - summary: entity.summary as string | undefined, + summary: toDisplayString(entity.summary), detailUrl: entity.detailUrl as string | undefined, imageUrl: primaryImageUrl, imageUrls: allMediaUrls.length > 0 ? allMediaUrls : [placeholder(entity.id)], // v2 terminology - categoryTitle: platform?.title as string | undefined, + categoryTitle: toDisplayString(platform?.title), categoryShort, order, imageAttribution: formatAttribution(images), @@ -481,11 +514,8 @@ async function loadFreshCollection( return { id: platform.id, title: platformTitle, - year: - typeof platform.year === "number" - ? String(platform.year) - : (platform.year as string | undefined), - summary: platform.summary as string | undefined, + year: toDisplayString(platform.year), + summary: toDisplayString(platform.summary), detailUrls: platformDetailUrls.length > 0 ? platformDetailUrls diff --git a/tests/hooks/useCollection.untrusted.test.ts b/tests/hooks/useCollection.untrusted.test.ts index f4817c7..c3ab290 100644 --- a/tests/hooks/useCollection.untrusted.test.ts +++ b/tests/hooks/useCollection.untrusted.test.ts @@ -36,7 +36,7 @@ vi.mock("@/lib/cardCache", () => ({ })); import { useLocalCollection } from "@/hooks/useCollection"; -import { loadCollection } from "@/loaders"; +import { loadCollection, getImageUrls } from "@/loaders"; import { useSourceStore, type Source } from "@/stores/sourceStore"; import { useSettingsStore } from "@/stores/settingsStore"; @@ -137,4 +137,108 @@ describe("useCollection — malformed untrusted entities do not crash the load", expect(result.current.isError).toBe(false); expect(result.current.data?.cards).toHaveLength(1); }); + + it("coerces an object-typed summary to undefined (not a React-child object)", async () => { + setResolvedEntities([ + { id: "g1", title: "Game One", summary: { en: "hello" } }, + ]); + + const { result } = renderHook( + () => useLocalCollection({ basePath: SOURCE_URL }), + { wrapper: createWrapper() } + ); + + await waitFor(() => { + expect(result.current.isSuccess).toBe(true); + }); + const card = result.current.data?.cards[0]; + // An object left as-is is truthy and would throw "Objects are not valid as + // a React child" when rendered; it must be dropped to undefined. + expect(typeof card?.summary).not.toBe("object"); + expect(card?.summary).toBeUndefined(); + }); + + it("coerces object-typed platform shortTitle/title so the device badge cannot be an object", async () => { + setResolvedEntities([ + { + id: "g1", + title: "Game One", + _resolved: { + platform: { + id: "p1", + title: { nested: "x" }, + shortTitle: { nested: "y" }, + }, + }, + }, + ]); + + const { result } = renderHook( + () => useLocalCollection({ basePath: SOURCE_URL }), + { wrapper: createWrapper() } + ); + + await waitFor(() => { + expect(result.current.isSuccess).toBe(true); + }); + const card = result.current.data?.cards[0]; + // `device` (categoryShort) and categoryTitle feed JSX children directly. + expect(typeof card?.device).not.toBe("object"); + expect(card?.device).toBeUndefined(); + expect(typeof card?.categoryTitle).not.toBe("object"); + expect(card?.categoryTitle).toBeUndefined(); + }); + + it("caps card.imageUrls for an entity listing a huge number of images", async () => { + const hugeUrlList = Array.from( + { length: 5000 }, + (_, i) => `https://cdn.jsdelivr.net/img/${String(i)}.png` + ); + vi.mocked(getImageUrls).mockReturnValueOnce(hugeUrlList); + setResolvedEntities([{ id: "g1", title: "Game One" }]); + + const { result } = renderHook( + () => useLocalCollection({ basePath: SOURCE_URL }), + { wrapper: createWrapper() } + ); + + await waitFor(() => { + expect(result.current.isSuccess).toBe(true); + }); + const card = result.current.data?.cards[0]; + // The gallery mounts one node per media item and the preloader fetches + // each, so the combined media list must be bounded well below 5000. + expect(card?.imageUrls.length).toBeLessThanOrEqual(100); + }); + + it("coerces object-typed platform summary/year in categoryInfo", async () => { + setResolvedEntities([ + { + id: "g1", + title: "Game One", + _resolved: { + platform: { + id: "p1", + title: "Platform", + summary: { en: "obj" }, + year: { not: "a-year" }, + }, + }, + }, + ]); + + const { result } = renderHook( + () => useLocalCollection({ basePath: SOURCE_URL }), + { wrapper: createWrapper() } + ); + + await waitFor(() => { + expect(result.current.isSuccess).toBe(true); + }); + const categoryInfo = result.current.data?.cards[0]?.categoryInfo; + expect(typeof categoryInfo?.summary).not.toBe("object"); + expect(categoryInfo?.summary).toBeUndefined(); + expect(typeof categoryInfo?.year).not.toBe("object"); + expect(categoryInfo?.year).toBeUndefined(); + }); }); From ed9fe85053036d5ab65899f7cdd1f328167c2ace Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 11:53:59 +0000 Subject: [PATCH 3/9] fix: compute collection stats min/max in a single pass computeCollectionStats spread a per-card value array into Math.min(...values) and Math.max(...values). The array length scales with the untrusted card count, so a large collection throws RangeError past the engine's argument limit (~125k on V8, lower on JSC). CollectionToast renders these stats outside the collection error boundary, so the throw unmounts the app and blanks the page. Replace the spread with a single-pass loop. Assisted-by: Claude:claude-fable-5 --- src/utils/collectionStats.ts | 16 +++++++++++++--- tests/utils/collectionStats.test.ts | 28 +++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/utils/collectionStats.ts b/src/utils/collectionStats.ts index 2d082ce..959cf69 100644 --- a/src/utils/collectionStats.ts +++ b/src/utils/collectionStats.ts @@ -130,9 +130,19 @@ export function computeCollectionStats( for (const [field, acc] of numericAccumulators) { if (acc.values.length > 0) { const values = acc.values; - const sum = values.reduce((a, b) => a + b, 0); - const min = Math.min(...values); - const max = Math.max(...values); + // Single-pass min/max/sum. `values` holds one entry per card, so its + // length scales with the untrusted card count; spreading it into + // Math.min(...values)/Math.max(...values) throws RangeError past the + // engine's argument limit (~125k on V8, far lower on JSC), and these + // stats render outside the collection error boundary, blanking the app. + let sum = 0; + let min = values[0] ?? 0; + let max = values[0] ?? 0; + for (const value of values) { + sum += value; + if (value < min) min = value; + if (value > max) max = value; + } const avg = sum / values.length; stats.numericFields.set(field, { diff --git a/tests/utils/collectionStats.test.ts b/tests/utils/collectionStats.test.ts index 4576427..fd0c961 100644 --- a/tests/utils/collectionStats.test.ts +++ b/tests/utils/collectionStats.test.ts @@ -216,6 +216,30 @@ describe("computeCollectionStats", () => { expect(stats.numericFields.get("year")).toBeUndefined(); }); }); + + describe("large untrusted collections", () => { + // A hostile collection can carry one numeric value per card. Computing + // min/max via Math.min(...values) throws RangeError past the engine's + // argument limit; because these stats render outside the collection error + // boundary, that blanks the whole app. The computation must not spread. + it("computes min/max without throwing on a very large value set", () => { + const count = 200_000; + const items: DisplayCard[] = new Array(count); + for (let i = 0; i < count; i += 1) { + // Deterministic values with a known min (1900) and max (1900 + count - 1) + items[i] = createMockCard({ year: 1900 + i }); + } + + const stats = computeCollectionStats(items); + const yearStats = stats.numericFields.get("year"); + + expect(yearStats).toBeDefined(); + expect(yearStats?.min).toBe(1900); + expect(yearStats?.max).toBe(1900 + count - 1); + expect(yearStats?.count).toBe(count); + expect(stats.yearRange).toEqual({ min: 1900, max: 1900 + count - 1 }); + }); + }); }); describe("formatStatsSummary", () => { @@ -311,6 +335,8 @@ describe("formatStatsSummary", () => { const summary = formatStatsSummary(stats); - expect(summary).toBe("100 items | Years: 1990-2020 | Platforms: 15 | Avg Rating: 8.2"); + expect(summary).toBe( + "100 items | Years: 1990-2020 | Platforms: 15 | Avg Rating: 8.2" + ); }); }); From a4413671194c83cfd42b9e2b3e2f6cb5dc98106b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 11:53:59 +0000 Subject: [PATCH 4/9] fix: cap entity-type fan-out when loading a collection loadCollection mapped the untrusted entityTypes record straight into a Promise.all, and each type fans out into several probe fetches (index files, GitHub discovery, single-file fallbacks). A collection.json listing tens of thousands of empty entity types amplified one load into ~80k CDN requests plus GitHub-quota exhaustion. Cap the type count and load the types through the same fixed-size pool used for entity ids, always retaining the primary type. Assisted-by: Claude:claude-fable-5 --- src/loaders/collectionLoader.ts | 63 +++++++++++++++++++++----- tests/loaders/collectionLoader.test.ts | 38 ++++++++++++++++ 2 files changed, 89 insertions(+), 12 deletions(-) diff --git a/src/loaders/collectionLoader.ts b/src/loaders/collectionLoader.ts index 69330fa..9a79299 100644 --- a/src/loaders/collectionLoader.ts +++ b/src/loaders/collectionLoader.ts @@ -296,6 +296,25 @@ const ENTITY_FETCH_CONCURRENCY = 8; */ const MAX_ENTITY_IDS = 10000; +/** + * Upper bound on the number of entity types loaded from one collection. + * + * `collection.json` is untrusted and its `entityTypes` record is unbounded. + * Each type triggers several fetches (index probes, GitHub discovery, single- + * file fallbacks) before resolving, so an entity-type count scaled by an + * attacker multiplies one collection load into a request flood against the + * CDN and the visitor's GitHub API quota. Cap the count and load the types + * through the same fixed-size pool used for entity ids. Real collections use + * a handful of types. + */ +const MAX_ENTITY_TYPES = 50; + +/** + * Number of entity types loaded concurrently. Bounds the per-type fetch + * fan-out (each type issues its own probes) to a fixed width. + */ +const ENTITY_TYPE_CONCURRENCY = 4; + /** * Load individual entity files from a directory. * @@ -377,20 +396,40 @@ export async function loadCollection( throw new Error("Collection has no entity types defined"); } - // Load all entity types in parallel - const entityTypes = Object.keys(definition.entityTypes); - const entityPromises = entityTypes.map(async (type) => { - const entities = await loadEntities(basePath, type); - return { type, entities }; - }); - - const entityResults = await Promise.all(entityPromises); + // Load entity types through a fixed-size pool. Each type fans out into + // several fetches, and the type list comes from untrusted collection.json, + // so cap the count and bound concurrency rather than firing every type at + // once (mirrors loadEntitiesFromDirectory's pool for entity ids). + let entityTypes = Object.keys(definition.entityTypes); + if (entityTypes.length > MAX_ENTITY_TYPES) { + console.warn( + `Collection defines ${String(entityTypes.length)} entity types; loading the first ${String(MAX_ENTITY_TYPES)}.` + ); + // Always keep the primary type even if it sorts past the cap. + const capped = entityTypes.slice(0, MAX_ENTITY_TYPES); + if (!capped.includes(primaryType)) { + capped[capped.length - 1] = primaryType; + } + entityTypes = capped; + } - // Build entities map const entities: Record = {}; - for (const { type, entities: typeEntities } of entityResults) { - entities[type] = typeEntities; - } + + let typeCursor = 0; + const typeWorker = async (): Promise => { + while (typeCursor < entityTypes.length) { + const index = typeCursor; + typeCursor += 1; + const type = entityTypes[index]; + if (type === undefined) continue; + entities[type] = await loadEntities(basePath, type); + } + }; + + const typeWorkerCount = Math.min(ENTITY_TYPE_CONCURRENCY, entityTypes.length); + await Promise.all( + Array.from({ length: typeWorkerCount }, () => typeWorker()) + ); return { definition, diff --git a/tests/loaders/collectionLoader.test.ts b/tests/loaders/collectionLoader.test.ts index 306cfc2..980137b 100644 --- a/tests/loaders/collectionLoader.test.ts +++ b/tests/loaders/collectionLoader.test.ts @@ -207,6 +207,44 @@ describe("entity fetch fan-out is bounded", () => { }); }); +describe("entity-type fan-out is bounded", () => { + const base = + "https://cdn.jsdelivr.net/gh/REPPL/MyPlausibleMe@main/data/collections/demo"; + + it("caps a hostile collection that defines too many entity types", async () => { + // A collection.json with a huge entityTypes record: each type would fan + // out into several probe fetches. The loader must cap the type count. + const entityTypes: Record = { + advert: { primary: true, fields: {} }, + }; + for (let i = 0; i < 500; i += 1) { + entityTypes[`type${String(i)}`] = { fields: {} }; + } + const hostileDefinition = { id: "demo", name: "Demo", entityTypes }; + + const probedTypeDirs = new Set(); + const mock = vi.fn((input: string) => { + if (input.endsWith("/collection.json")) { + return Promise.resolve(jsonResponse(hostileDefinition)); + } + // Record which entity-type directory each probe targets. + const match = /\/data\/collections\/demo\/([^/]+)\//.exec(input); + if (match?.[1]) probedTypeDirs.add(match[1]); + return Promise.resolve(notFound()); + }); + vi.stubGlobal("fetch", mock); + + const collection = await loadCollection(base); + + // The primary type is always loaded even though it would otherwise be + // among 500 keys; the total probed directories stay bounded. + expect(collection.primaryType).toBe("advert"); + expect(probedTypeDirs.size).toBeLessThanOrEqual(50); + expect(probedTypeDirs.has("adverts")).toBe(true); + expect(console.warn).toHaveBeenCalledWith(expect.stringContaining("501")); + }); +}); + describe("tolerant entity validation", () => { it("skips an invalid entity from a plural file and keeps the valid one", async () => { const base = From 957e8052fdd4cf59b48845749df45bb671209e58 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 11:54:09 +0000 Subject: [PATCH 5/9] fix: cap MyPlausibleMe collection-discovery fan-out useMyPlausibleMeDiscovery ran a Promise.all over every collection.json in the GitHub tree, one metadata fetch each. The username is attacker-controlled and discovery auto-runs on the startup picker with no click, so a /gh// link to a repository listing thousands of collection.json files turned one page load into a CDN request flood. Cap the discovered-collection count and fetch metadata through a fixed-size pool. Assisted-by: Claude:claude-fable-5 --- src/hooks/useMyPlausibleMeDiscovery.ts | 55 +++++++++++++-- tests/hooks/useMyPlausibleMeDiscovery.test.ts | 67 +++++++++++++++++++ 2 files changed, 116 insertions(+), 6 deletions(-) diff --git a/src/hooks/useMyPlausibleMeDiscovery.ts b/src/hooks/useMyPlausibleMeDiscovery.ts index b3f1356..7b721aa 100644 --- a/src/hooks/useMyPlausibleMeDiscovery.ts +++ b/src/hooks/useMyPlausibleMeDiscovery.ts @@ -83,6 +83,22 @@ function buildCdnUrl(username: string, path: string, branch = "main"): string { return `https://cdn.jsdelivr.net/gh/${username}/MyPlausibleMe@${branch}/${path}`; } +/** + * Upper bound on the number of collections discovered from one repository. + * + * The username is attacker-controlled (it comes from the `/gh//` URL and + * auto-runs on the picker with no click), and the GitHub tree can list tens of + * thousands of `collection.json` files. Firing a metadata fetch for each would + * amplify one page load into a CDN request flood. Cap the count; a real user + * repository holds a handful of collections. + */ +const MAX_DISCOVERED_COLLECTIONS = 200; + +/** + * Number of metadata fetches kept in flight at once. + */ +const METADATA_FETCH_CONCURRENCY = 8; + /** * Fetch collection metadata from collection.json. * @@ -189,29 +205,48 @@ export function useMyPlausibleMeDiscovery( // Step 2: Find all collection.json files under data/collections/ // Pattern: data/collections/{path}/collection.json - const collectionJsonFiles = tree.filter( + const allCollectionJsonFiles = tree.filter( (entry) => entry.type === "blob" && entry.path.startsWith("data/collections/") && entry.path.endsWith("/collection.json") ); - if (collectionJsonFiles.length === 0) { + if (allCollectionJsonFiles.length === 0) { setError("No collections found in repository"); setCollections([]); setIsLoading(false); return; } + // The username is untrusted, so a hostile repository can list a huge + // number of collection.json files. Cap before fanning out a metadata + // fetch per file. + const collectionJsonFiles = allCollectionJsonFiles.slice( + 0, + MAX_DISCOVERED_COLLECTIONS + ); + if (allCollectionJsonFiles.length > MAX_DISCOVERED_COLLECTIONS) { + console.warn( + `Repository lists ${String(allCollectionJsonFiles.length)} collections; showing the first ${String(MAX_DISCOVERED_COLLECTIONS)}.` + ); + } + // Step 3: Extract collection paths and fetch metadata // e.g., "data/collections/retro/games/collection.json" -> "retro/games" const validCollections: CollectionEntry[] = []; - await Promise.all( - collectionJsonFiles.map(async (file) => { + // Fetch metadata through a fixed-size pool rather than all at once. + let fileCursor = 0; + const metadataWorker = async (): Promise => { + while (fileCursor < collectionJsonFiles.length) { + const index = fileCursor; + fileCursor += 1; + const file = collectionJsonFiles[index]; + if (!file) continue; // Extract the collection path (everything between data/collections/ and /collection.json) const match = /^data\/collections\/(.+)\/collection\.json$/.exec(file.path); - if (!match?.[1]) return; + if (!match?.[1]) continue; const collectionPath = match[1]; const metadata = await fetchCollectionMetadata(trimmedUsername, collectionPath); @@ -251,7 +286,15 @@ export function useMyPlausibleMeDiscovery( isCached: cached, }); } - }) + } + }; + + const metadataWorkerCount = Math.min( + METADATA_FETCH_CONCURRENCY, + collectionJsonFiles.length + ); + await Promise.all( + Array.from({ length: metadataWorkerCount }, () => metadataWorker()) ); // Sort: cached collections first, then alphabetically by name diff --git a/tests/hooks/useMyPlausibleMeDiscovery.test.ts b/tests/hooks/useMyPlausibleMeDiscovery.test.ts index 584056a..b7cda0e 100644 --- a/tests/hooks/useMyPlausibleMeDiscovery.test.ts +++ b/tests/hooks/useMyPlausibleMeDiscovery.test.ts @@ -129,3 +129,70 @@ describe("useMyPlausibleMeDiscovery cache status", () => { expect(unregistered?.isCached).toBe(false); }); }); + +describe("useMyPlausibleMeDiscovery fan-out is bounded", () => { + beforeEach(() => { + vi.clearAllMocks(); + useSourceStore.setState({ + sources: [], + activeSourceId: null, + defaultSourceId: null, + }); + vi.spyOn(console, "warn").mockImplementation(() => undefined); + vi.mocked(isCollectionCached).mockResolvedValue(false); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it("caps metadata fetches for a hostile repository listing thousands of collections", async () => { + const hostileTree = { + sha: "abc", + truncated: false, + tree: Array.from({ length: 5000 }, (_, i) => ({ + path: `data/collections/c${String(i)}/collection.json`, + type: "blob" as const, + sha: `sha${String(i)}`, + })), + }; + + let metadataFetches = 0; + vi.stubGlobal( + "fetch", + vi.fn((input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("api.github.com")) { + return Promise.resolve( + new Response(JSON.stringify(hostileTree), { status: 200 }) + ); + } + if (url.endsWith("/collection.json")) { + metadataFetches += 1; + return Promise.resolve( + new Response(JSON.stringify({ name: "C" }), { status: 200 }) + ); + } + return Promise.resolve(new Response("not found", { status: 404 })); + }) + ); + + const { result } = renderHook(() => useMyPlausibleMeDiscovery("EVIL")); + + // Wait until discovery finishes populating (isLoading starts false before + // the debounce fires, so wait on the collections instead). + await waitFor( + () => { + expect(result.current.collections.length).toBeGreaterThan(0); + }, + { timeout: 5000 } + ); + + // Only the first MAX_DISCOVERED_COLLECTIONS (200) metadata files are + // fetched, not all 5000. + expect(metadataFetches).toBeLessThanOrEqual(200); + expect(result.current.collections.length).toBeLessThanOrEqual(200); + expect(console.warn).toHaveBeenCalledWith(expect.stringContaining("5000")); + }); +}); From f791bc9bf5fd236d060e229b054d7217cee61e13 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 11:54:09 +0000 Subject: [PATCH 6/9] fix: make the hard reset tolerant of a blocked app-DB delete clearAllPersistedData awaited deleteDB first and sequentially, and deleteDB had no onblocked handler. A blocked or failed app-DB delete therefore either hung the reset dialog or aborted the remaining steps, silently leaving the cached-collection store and the plugin cache DB on disk while the UI reported a complete "delete everything". Give deleteDB an onblocked handler so it settles, and run the three IndexedDB cleanups independently via Promise.allSettled. Assisted-by: Claude:claude-fable-5 --- src/db/index.ts | 7 +++ src/lib/clearPersistedData.ts | 14 +++--- tests/db/index.test.ts | 65 ++++++++++++++++++++++++++++ tests/lib/clearPersistedData.test.ts | 13 ++++++ 4 files changed, 94 insertions(+), 5 deletions(-) create mode 100644 tests/db/index.test.ts diff --git a/src/db/index.ts b/src/db/index.ts index 6c664fd..c99d366 100644 --- a/src/db/index.ts +++ b/src/db/index.ts @@ -185,6 +185,13 @@ export function deleteDB(): Promise { const request = indexedDB.deleteDatabase(DB_NAME); request.onsuccess = () => { resolve(); }; request.onerror = () => { reject(new Error(request.error?.message ?? "Failed to delete database")); }; + // Without this, a delete blocked by another open connection never settles, + // hanging any caller that awaits it (e.g. the hard reset). Resolve so the + // reset proceeds; the delete completes once the blocking connection closes. + request.onblocked = () => { + console.warn("[itemdeck] Database deletion blocked by another connection"); + resolve(); + }; }); } diff --git a/src/lib/clearPersistedData.ts b/src/lib/clearPersistedData.ts index 194fed7..49ec2e6 100644 --- a/src/lib/clearPersistedData.ts +++ b/src/lib/clearPersistedData.ts @@ -55,9 +55,13 @@ export async function clearAllPersistedData(): Promise { } } - await deleteDB(); // the app database ("itemdeck") - await clearAllCollectionCaches(); // cached collections (idb-keyval store) - for (const name of SATELLITE_DATABASES) { - await deleteIndexedDb(name); - } + // The three IndexedDB cleanups are independent. Run them so a failure or + // block in one never aborts the others — previously a rejected/blocked app-DB + // delete (the first awaited step) silently left the cached-collection store + // and the plugin cache on disk while the UI reported a complete reset. + await Promise.allSettled([ + deleteDB(), // the app database ("itemdeck") + clearAllCollectionCaches(), // cached collections (idb-keyval store) + ...SATELLITE_DATABASES.map((name) => deleteIndexedDb(name)), + ]); } diff --git a/tests/db/index.test.ts b/tests/db/index.test.ts new file mode 100644 index 0000000..8c785a8 --- /dev/null +++ b/tests/db/index.test.ts @@ -0,0 +1,65 @@ +/** + * Tests for the app IndexedDB helpers. + * + * Regression: deleteDB registered no `onblocked` handler, so a delete blocked + * by another open connection never settled — hanging any caller that awaited + * it (notably the hard reset). It must resolve on both success and block. + */ + +import { describe, it, expect, vi, afterEach } from "vitest"; + +interface FakeRequest { + onsuccess: (() => void) | null; + onerror: (() => void) | null; + onblocked: (() => void) | null; + error: { message: string } | null; +} + +let lastRequest: FakeRequest | null; + +function stubIndexedDB(): void { + lastRequest = null; + vi.stubGlobal("indexedDB", { + deleteDatabase: vi.fn(() => { + const request: FakeRequest = { + onsuccess: null, + onerror: null, + onblocked: null, + error: null, + }; + lastRequest = request; + return request; + }), + }); +} + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe("deleteDB", () => { + it("resolves when the delete succeeds", async () => { + stubIndexedDB(); + const { deleteDB } = await import("@/db"); + + const promise = deleteDB(); + lastRequest?.onsuccess?.(); + + await expect(promise).resolves.toBeUndefined(); + }); + + it("resolves (does not hang) when the delete is blocked by another connection", async () => { + stubIndexedDB(); + vi.spyOn(console, "warn").mockImplementation(() => undefined); + const { deleteDB } = await import("@/db"); + + const promise = deleteDB(); + // Simulate another open connection blocking the delete: only onblocked + // fires, never onsuccess. Without an onblocked handler this would never + // settle. + lastRequest?.onblocked?.(); + + await expect(promise).resolves.toBeUndefined(); + }); +}); diff --git a/tests/lib/clearPersistedData.test.ts b/tests/lib/clearPersistedData.test.ts index f5e541f..8de04f1 100644 --- a/tests/lib/clearPersistedData.test.ts +++ b/tests/lib/clearPersistedData.test.ts @@ -126,4 +126,17 @@ describe("clearAllPersistedData", () => { await expect(clearAllPersistedData()).resolves.toBeUndefined(); }); + + it("still clears the cache and satellite DBs when the app-DB delete rejects", async () => { + vi.stubGlobal("localStorage", makeLocalStorage({})); + // The app DB delete rejects (e.g. an IndexedDB error). The remaining + // cleanups must still run — a partial reset presented as complete would + // leave the user's cached collections on disk. + deleteDBMock.mockRejectedValueOnce(new Error("delete failed")); + + await expect(clearAllPersistedData()).resolves.toBeUndefined(); + + expect(clearAllCollectionCachesMock).toHaveBeenCalledTimes(1); + expect(deletedDatabases).toContain("itemdeck-plugins"); + }); }); From aafb11ac5698fe5f777ca45da9ed5b40783a145f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 11:54:09 +0000 Subject: [PATCH 7/9] docs: record round 6 in the security decisions log Assisted-by: Claude:claude-fable-5 --- .abcd/work/DECISIONS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index b0d6d3a..76a3ccd 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -7,3 +7,4 @@ Dated, one-line summaries of each autonomous security-hardening round. - 2026-07-30 — Round 3: fixed 4 confirmed defects — (1) imported theme colours now validated as hex, closing an external-beacon vector where a loose `z.string()` colour in a shared settings file resolved to a CSS `url(...)` background that bypassed the `connect-src` allowlist via `img-src`; (2) `detectNumericFields` computes min/max in a single pass instead of spreading a per-card array into `Math.min`/`Math.max`, so a large untrusted collection no longer crashes the Competing mechanic with a `RangeError`; (3) forced `fieldMapping` values from a remote `settings.json` are validated as strings (plus a defensive guard in `resolveFieldPath`), closing a persistent-DoS where a non-string path threw on every card render and survived reloads via localStorage; (4) `editsStore` `getEdit`/`hasEdits` use own-property guards so an untrusted entity id such as `"toString"` no longer resolves to an inherited function and crashes the edit form. 7 reproduction tests added. Refuted: provider `..` path traversal (escapes only a non-security UX template into a capability the app already grants over a public CDN; schema-validated output), `CollectionDataContext` edits lookup (inert), and assorted nitpicks (CSP tightening, `migrate-collection` argv path, `fillTheBlank` `$&` replace). - 2026-08-05 — Round 4: fixed 7 confirmed defects — (1) an entity `rating`/`averageRating` of `null` no longer crashes the whole collection load (`isStructuredRating` is null- and type-safe, `normaliseRating`/`formatRating` coerce malformed values, and the load treats `null` as "no rating"); (2) a relationship `target` or entity field named after an `Object.prototype` member (e.g. `"toString"`) no longer resolves to an inherited function and throws during load (`resolveReference` and the implicit-relationship path use `Object.hasOwn` guards); (3) a non-array `videos` field no longer throws via `.map()`; (4) a non-string resolved platform `title` no longer throws via `.replace()`; (5) `detectNumericFields` runs a single pass over each card's own fields instead of a keys×cards nested scan, so a hostile collection with many uniquely-named fields can no longer freeze the main thread for tens of seconds; (6) `SourceIcon` detects known sources by URL hostname (anchored on the dot boundary) rather than a substring of the whole URL, so an attacker link can no longer be branded with a trusted source's icon/name via a path like `evil.example/en.wikipedia.org/x`; (7) the pre-commit PII and British-English gates read staged paths NUL-terminated with `core.quotePath=false` and fail closed on unreadable blobs, closing a silent bypass for files whose names contain non-ASCII/backslash/quote/newline characters. 24 reproduction tests added. Deferred (confirmed but out of scope for a smallest-diff round): collection `forced` settings persist into global state and are never reverted — the correct fix is a collection-scoped overlay plus a persisted-store migration, carrying moderate/high regression risk. Refuted: stale `dist/` analyser artefact leak (publish root is a clean Cloudflare Pages checkout with `ANALYZE` off). - 2026-08-05 — Round 5: fixed 4 confirmed defects — (1) `normaliseDetailUrls` guards each array element with `isDetailLink` before reading `.url`, so an entity `detailUrls: [null]` (an unvalidated `.loose()` passthrough) no longer throws and rejects the whole collection load; (2) entity files load through a fixed-size concurrency pool with a generous id cap (`ENTITY_FETCH_CONCURRENCY`/`MAX_ENTITY_IDS`) instead of a `Promise.all` over the whole untrusted `index.json`, closing a main-thread stall / CDN request-amplification DoS from a hostile index listing tens of thousands of ids; (3) Snap Ranking's `initGame` refuses to start above `MAX_UNIQUE_VALUES` distinct values (one guess button is rendered per value; the default per-card `order` field on a large collection otherwise renders tens of thousands of DOM buttons and freezes the tab); (4) the "Hard Reset" clears every `itemdeck-`-prefixed localStorage key and all three IndexedDB databases (app DB, the idb-keyval cached-collection store, and the plugin cache DB) via `clearAllPersistedData`, honouring the dialog's "delete everything" promise instead of leaving cached remote collections, imported data, config and game state behind. 11 reproduction tests added. Deferred (confirmed, own dedicated round): collection `forced` settings still persist into global state without a revert path — the smallest safe fix is a symmetric backup/restore mirroring `_mechanicOverridesBackup` (snapshot the touched keys on apply, restore on source change, without clearing `appliedCollectionDefaultsSourceId`). Nitpicks/defence-in-depth noted (not fixed): `fontUrl`/`cardBackBackgroundImage` import fields use bare `z.url()` (unreachable — no consumer), `validateForcedSettings` enum allow-lists are stale, `fieldDiscovery` bracket lookups lack `Object.hasOwn` (latent — provider unmounted), image fetch has no size/timeout pre-check, and `cardBackBackground`/`usePlaceholderImages` are absent from `partialize`. +- 2026-08-06 — Round 6: fixed 7 confirmed defects — (1) collection `forced` settings now snapshot the user's own displaced values per source and restore them when the active source changes, with crash recovery via `onRehydrateStorage` (mirroring `_mechanicOverridesBackup`); previously viewing one hostile allowlisted-CDN source once, reachable via a single unconfirmed `/gh//` link, permanently overwrote the visitor's global display config with no revert path (`clearCollectionForcedSettings` had zero callers) — the dead action is repurposed into `restoreCollectionForcedSettings`; (2) untrusted entity text fields (`summary`, resolved platform `title`/`shortTitle`/`summary`/`year`) are coerced to strings at the `useCollection` source instead of cast, so an object-typed field (e.g. an i18n `{en: "…"}` value) can no longer reach JSX as a child and crash the whole collection view — the device badge sink fires on first paint in the default grid; (3) `computeCollectionStats` computes min/max in a single pass instead of spreading a per-card array into `Math.min`/`Math.max`, so a large untrusted collection no longer throws `RangeError` in `CollectionToast`, which renders outside the collection error boundary and would blank the app; (4) `loadCollection` caps the untrusted `entityTypes` count (`MAX_ENTITY_TYPES`) and loads types through a fixed-size pool, closing a per-type fetch multiplier that amplified one collection load into ~80k CDN requests plus GitHub-quota exhaustion; (5) `useMyPlausibleMeDiscovery` caps discovered collections (`MAX_DISCOVERED_COLLECTIONS`) and pools metadata fetches, closing a zero-click CDN request flood triggerable via an attacker `/gh//` URL on the startup picker; (6) `card.imageUrls` (images + videos) is capped per card (`MAX_MEDIA_PER_CARD`), bounding the gallery dot buttons and the load-time image preloader against an entity listing a huge media array; (7) the hard reset runs its three IndexedDB cleanups independently via `Promise.allSettled` and `deleteDB` gained an `onblocked` handler, so a blocked or failed app-DB delete no longer hangs the dialog or silently leaves cached collections and the plugin DB on disk while reporting a complete "delete everything". 19 reproduction tests added. Refuted: `applyMechanicOverrides` second-activation backup clobber (unreachable — the Start Game overlay is gated on `!activeMechanic`, so overrides only apply when no mechanic is active and every deactivation restores first) and the write-only `collectionForcedSettings` mid-session revert (TanStack Query structural sharing keeps `data.settings` identity stable, so the apply effect does not re-fire; folded into the S1 fix). Nitpicks noted (not fixed): TruffleHog pre-commit `--since-commit HEAD` empty-range scan, gitleaks checksum not provenance-anchored, `build:analyse` writing `stats.html` into the deploy root, forced `cardBackStyle`/`titleDisplayMode` enum mismatch vs the store types, and the `getDB()` in-flight dedup that makes the delete-blocked path reachable. From fa4efd7853043a265113dcbac10657c36d23e47d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 12:06:55 +0000 Subject: [PATCH 8/9] fix: back up forced settings per key, not per source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The forced-settings snapshot was guarded solely by a source-id change, so a later settings.json revision of the SAME source that began forcing an additional key never captured that key's original user value — permanently losing it on restore, the exact defect the backup is meant to prevent (a routine forced-key addition by a benign author clobbers it for returning users too). Snapshot per forced key instead: start a fresh backup on a genuine source change, extend the existing backup on a same-source refetch, and back up only keys not already held so an already-forced value never overwrites the user's own original. Assisted-by: Claude:claude-fable-5 --- src/stores/settingsStore.ts | 64 +++++++++---------- .../settingsStore.collectionForced.test.ts | 46 +++++++++++++ 2 files changed, 76 insertions(+), 34 deletions(-) diff --git a/src/stores/settingsStore.ts b/src/stores/settingsStore.ts index d9c2821..69bf4ff 100644 --- a/src/stores/settingsStore.ts +++ b/src/stores/settingsStore.ts @@ -1095,42 +1095,38 @@ export const useSettingsStore = create()( ? { ...state, ...state._collectionForcedBackup } : state; - if (isNewSource) { - // Snapshot the user's own value for each key THIS source forces, - // so unrelated user settings are never rolled back. Snapshot once - // per source (guarded by isNewSource) so a refetch of the same - // source cannot clobber the backup with already-forced values. - const backup: CollectionForcedBackup = {}; - if (forced.fieldMapping) backup.fieldMapping = base.fieldMapping; - if (forced.defaultCardFace !== undefined) { - backup.defaultCardFace = base.defaultCardFace; - } - if (forced.cardBackDisplay !== undefined) { - backup.cardBackDisplay = base.cardBackDisplay; - } - if (forced.cardBackStyle !== undefined) { - backup.cardBackStyle = base.cardBackStyle; - } - if (forced.titleDisplayMode !== undefined) { - backup.titleDisplayMode = base.titleDisplayMode; - } - if (forced.showRankBadge !== undefined) { - backup.showRankBadge = base.showRankBadge; - } - if (forced.showDeviceBadge !== undefined) { - backup.showDeviceBadge = base.showDeviceBadge; - } - if (forced.rankPlaceholderText !== undefined) { - backup.rankPlaceholderText = base.rankPlaceholderText; - } + // Snapshot per forced KEY, not per source: start a fresh backup on + // a genuine source change, but on a same-source refetch extend the + // existing backup so a key a later settings.json revision begins + // forcing is still captured. Back up only keys not already held, so + // an already-forced value never clobbers the user's own original. + // (Guarding solely on the source id would permanently lose the + // user's value for any key a refetch newly forces.) + const backup: CollectionForcedBackup = isNewSource + ? {} + : { ...(state._collectionForcedBackup ?? {}) }; + const snapshot = (key: K): void => { + if (!(key in backup)) backup[key] = base[key]; + }; + if (forced.fieldMapping) snapshot("fieldMapping"); + if (forced.defaultCardFace !== undefined) snapshot("defaultCardFace"); + if (forced.cardBackDisplay !== undefined) snapshot("cardBackDisplay"); + if (forced.cardBackStyle !== undefined) snapshot("cardBackStyle"); + if (forced.titleDisplayMode !== undefined) snapshot("titleDisplayMode"); + if (forced.showRankBadge !== undefined) snapshot("showRankBadge"); + if (forced.showDeviceBadge !== undefined) snapshot("showDeviceBadge"); + if (forced.rankPlaceholderText !== undefined) { + snapshot("rankPlaceholderText"); + } - updates._collectionForcedBackup = - Object.keys(backup).length > 0 ? backup : null; - updates.collectionForcedSourceId = sourceId; + updates._collectionForcedBackup = + Object.keys(backup).length > 0 ? backup : null; + updates.collectionForcedSourceId = sourceId; - // Carry the previous source's restored values through for keys the - // new source does NOT force (the forced assignments below win for - // keys it does force). + // On a genuine source change, carry the previous source's restored + // values through for keys the new source does NOT force (the forced + // assignments below win for keys it does force). + if (isNewSource) { Object.assign(updates, state._collectionForcedBackup ?? {}); } diff --git a/tests/stores/settingsStore.collectionForced.test.ts b/tests/stores/settingsStore.collectionForced.test.ts index 03353c5..abbcd1d 100644 --- a/tests/stores/settingsStore.collectionForced.test.ts +++ b/tests/stores/settingsStore.collectionForced.test.ts @@ -88,6 +88,52 @@ describe("settingsStore - collection forced-settings backup/restore", () => { }); }); + it("backs up a key a later refetch of the SAME source begins forcing", () => { + // User's own values. + useSettingsStore.setState({ + cardBackDisplay: "logo", + showRankBadge: true, + }); + + // v1 of the same source forces only cardBackDisplay. + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, { + forced: { cardBackDisplay: "none" }, + }); + // v2 (a refetch of the SAME source — settings.json is not pinned) now forces + // an ADDITIONAL key. Its original user value must still be captured. + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, { + forced: { cardBackDisplay: "none", showRankBadge: false }, + }); + + expect(useSettingsStore.getState()._collectionForcedBackup).toEqual({ + cardBackDisplay: "logo", + showRankBadge: true, + }); + + // Restoring returns BOTH keys to the user's originals. + useSettingsStore.getState().restoreCollectionForcedSettings(); + expect(useSettingsStore.getState().cardBackDisplay).toBe("logo"); + expect(useSettingsStore.getState().showRankBadge).toBe(true); + }); + + it("backs up keys forced only after an initial empty forced set (same source)", () => { + useSettingsStore.setState({ showRankBadge: true }); + + // The source first serves an empty forced set (claims the source id), then + // a real forced set on refetch. The later-forced key must be backed up. + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, { + forced: {}, + }); + useSettingsStore.getState().applyCollectionSettings(SOURCE_A, { + forced: { showRankBadge: false }, + }); + + expect(useSettingsStore.getState().showRankBadge).toBe(false); + + useSettingsStore.getState().restoreCollectionForcedSettings(); + expect(useSettingsStore.getState().showRankBadge).toBe(true); + }); + it("restores the user's own values and clears backup state on restore", () => { useSettingsStore.setState({ cardBackDisplay: "logo", From cdc295baab2648c4d05846c28db92b81bf7b531c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 12:17:58 +0000 Subject: [PATCH 9/9] fix: drop unnecessary generic in forced-settings snapshot The per-key snapshot helper used a single-use type parameter, which @typescript-eslint/no-unnecessary-type-parameters rejects as an error and failed the lint gate. Replace it with explicit per-key literal assignments, which keep each backup value type correlated with its key without a generic. Behaviour is unchanged; all 944 tests pass. Assisted-by: Claude:claude-fable-5 --- src/stores/settingsStore.ts | 66 ++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 16 deletions(-) diff --git a/src/stores/settingsStore.ts b/src/stores/settingsStore.ts index 69bf4ff..ddfbd6d 100644 --- a/src/stores/settingsStore.ts +++ b/src/stores/settingsStore.ts @@ -1098,25 +1098,59 @@ export const useSettingsStore = create()( // Snapshot per forced KEY, not per source: start a fresh backup on // a genuine source change, but on a same-source refetch extend the // existing backup so a key a later settings.json revision begins - // forcing is still captured. Back up only keys not already held, so - // an already-forced value never clobbers the user's own original. - // (Guarding solely on the source id would permanently lose the - // user's value for any key a refetch newly forces.) + // forcing is still captured. Each key is backed up only if not + // already held, so an already-forced value never clobbers the + // user's own original. (Guarding solely on the source id would + // permanently lose the user's value for any key a refetch newly + // forces.) Assignments are per-key literals so each backup value + // type stays correlated with its key. const backup: CollectionForcedBackup = isNewSource ? {} : { ...(state._collectionForcedBackup ?? {}) }; - const snapshot = (key: K): void => { - if (!(key in backup)) backup[key] = base[key]; - }; - if (forced.fieldMapping) snapshot("fieldMapping"); - if (forced.defaultCardFace !== undefined) snapshot("defaultCardFace"); - if (forced.cardBackDisplay !== undefined) snapshot("cardBackDisplay"); - if (forced.cardBackStyle !== undefined) snapshot("cardBackStyle"); - if (forced.titleDisplayMode !== undefined) snapshot("titleDisplayMode"); - if (forced.showRankBadge !== undefined) snapshot("showRankBadge"); - if (forced.showDeviceBadge !== undefined) snapshot("showDeviceBadge"); - if (forced.rankPlaceholderText !== undefined) { - snapshot("rankPlaceholderText"); + if (forced.fieldMapping && !("fieldMapping" in backup)) { + backup.fieldMapping = base.fieldMapping; + } + if ( + forced.defaultCardFace !== undefined && + !("defaultCardFace" in backup) + ) { + backup.defaultCardFace = base.defaultCardFace; + } + if ( + forced.cardBackDisplay !== undefined && + !("cardBackDisplay" in backup) + ) { + backup.cardBackDisplay = base.cardBackDisplay; + } + if ( + forced.cardBackStyle !== undefined && + !("cardBackStyle" in backup) + ) { + backup.cardBackStyle = base.cardBackStyle; + } + if ( + forced.titleDisplayMode !== undefined && + !("titleDisplayMode" in backup) + ) { + backup.titleDisplayMode = base.titleDisplayMode; + } + if ( + forced.showRankBadge !== undefined && + !("showRankBadge" in backup) + ) { + backup.showRankBadge = base.showRankBadge; + } + if ( + forced.showDeviceBadge !== undefined && + !("showDeviceBadge" in backup) + ) { + backup.showDeviceBadge = base.showDeviceBadge; + } + if ( + forced.rankPlaceholderText !== undefined && + !("rankPlaceholderText" in backup) + ) { + backup.rankPlaceholderText = base.rankPlaceholderText; } updates._collectionForcedBackup =