From f0b0e12e580798d9834b87174b24c18d9a52d35f Mon Sep 17 00:00:00 2001 From: bowenliang123 Date: Sun, 11 Oct 2026 17:03:42 +0800 Subject: [PATCH] fix(client): guard the settings binding against hostile values and dead scopes Three review passes (standards, contract, over-engineering) over the whole settings surface left four follow-ups worth taking: - sync() wraps its snapshot parsing in the compatibility.md parser gate's bounded catch: a served value that throws on property access keeps the current state instead of throwing into the mirror's fan-out, with the mandated hostile fixture beside the happy path. - the attach disposer now clears the scope, so a set() after disposal never writes into a dead form; a superseded attach keeps the newer scope bound. - dead surface trimmed: six type re-exports nothing imported through the module, a single-caller query helper, and an orphaned export on the settings-card props interface. --- src/client/components/settingsCard.tsx | 2 +- src/client/settings.ts | 21 +++++++++---- src/client/settingsJump.ts | 7 ++--- tests/client/settings.spec.ts | 42 ++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 12 deletions(-) diff --git a/src/client/components/settingsCard.tsx b/src/client/components/settingsCard.tsx index 95eb3a3..b9527ef 100644 --- a/src/client/components/settingsCard.tsx +++ b/src/client/components/settingsCard.tsx @@ -7,7 +7,7 @@ import { IconChevronDown } from '../primitives' import type { SettingsField, SettingsState } from '../settings' import type { ViewKit } from '../viewkit' -export interface SettingsCardProps { +interface SettingsCardProps { useContextSettings?: (selector: (state: SettingsState) => T) => T set?: (field: SettingsField, value: string) => void } diff --git a/src/client/settings.ts b/src/client/settings.ts index 54e59ff..643b92b 100644 --- a/src/client/settings.ts +++ b/src/client/settings.ts @@ -3,7 +3,7 @@ import type { DefaultDeltaBase, DefaultFileSort, DefaultGranularity, DefaultDurationCurve, DefaultPlacement, DefaultToolSort, DefaultTrendMode, FleetTab, InsightsEntry, SettingsField } from '../shared/types' -export type { DefaultDeltaBase, DefaultFileSort, DefaultGranularity, DefaultDurationCurve, DefaultPlacement, DefaultToolSort, DefaultTrendMode, FleetTab, InsightsEntry, SettingsField } from '../shared/types' +export type { DefaultDeltaBase, DefaultFileSort, DefaultToolSort, SettingsField } from '../shared/types' /** The bound settings form (`ctx.configForms.get`), as consumed. */ export interface SettingsScopeLike { @@ -100,12 +100,16 @@ export function createContextSettings(): ContextSettings { } const sync = (bound: SettingsScopeLike): { placement?: DefaultPlacement; insightsEntry?: InsightsEntry; fleetTab?: FleetTab } => { const snap = bound.getSnapshot() - const prefs = prefsOf(snap.value) + // Bounded catch (the compatibility.md parser gate): a value that throws on property access + // keeps the current state instead of throwing into the mirror's fan-out. + let prefs: Prefs = {} + let raw: Record | undefined + try { + prefs = prefsOf(snap.value) + raw = snap.value !== null && typeof snap.value === 'object' ? snap.value as Record : undefined + } catch { /* hostile value */ } // Fail open: a config problem must never leave an entry hidden. A value the plugin cannot understand // degrades to the field's default; a section without the field (older Host half) keeps the current state. - const raw = snap.value !== null && typeof snap.value === 'object' - ? snap.value as Record - : undefined publish({ status: snap.status === 'ready' || snap.status === 'unavailable' ? snap.status : 'loading', placement: prefs.placement ?? (raw?.defaultPlacement === undefined ? state.placement : 'all'), @@ -143,7 +147,12 @@ export function createContextSettings(): ContextSettings { attach(bound) { scope = bound sync(bound) - return bound.subscribe(() => { sync(bound) }) + const unsubscribe = bound.subscribe(() => { sync(bound) }) + return () => { + unsubscribe() + // A set() after disposal must not write into a dead form. + if (scope === bound) scope = undefined + } }, set(field, value) { publish({ ...state, ...prefsOf({ [field]: value }) }) diff --git a/src/client/settingsJump.ts b/src/client/settingsJump.ts index 2a2c5f4..23dc32b 100644 --- a/src/client/settingsJump.ts +++ b/src/client/settingsJump.ts @@ -11,12 +11,9 @@ const PACKAGE_KEY = 'dsh-context' /** 40 ticks at the 100ms tick: enough for the page's package load. */ const POLL_TICKS = 40 -function buttonsOf(doc: Document): HTMLButtonElement[] { - return [...doc.querySelectorAll('button')] -} - function findPanelEntry(doc: Document): HTMLButtonElement | undefined { - return buttonsOf(doc).find(b => PLUGIN_LABELS.has(b.getAttribute('aria-label') ?? '')) + return [...doc.querySelectorAll('button')] + .find(b => PLUGIN_LABELS.has(b.getAttribute('aria-label') ?? '')) } /** The bundle card's open control: the one button inside the card that is not the enable switch (the diff --git a/tests/client/settings.spec.ts b/tests/client/settings.spec.ts index 9cb3170..1d0f348 100644 --- a/tests/client/settings.spec.ts +++ b/tests/client/settings.spec.ts @@ -438,4 +438,46 @@ describe('attach', () => { assert.equal(calls, 0) assert.equal(s.defaultGranularity(), 'step') }) + + test('a set after the disposer runs never reaches the dead scope', () => { + const s = createContextSettings() + const scope = new TestSettingsScope({ status: 'ready', value: {}, writable: true }) + const detach = s.attach(scope) + detach() + s.set('defaultTrendMode', 'delta') + assert.deepEqual(scope.sets, []) + }) + + test('disposing a superseded attach keeps the newer scope bound', () => { + const s = createContextSettings() + const first = new TestSettingsScope({ status: 'ready', value: {}, writable: true }) + const second = new TestSettingsScope({ status: 'ready', value: {}, writable: true }) + const detachFirst = s.attach(first) + s.attach(second) + detachFirst() + s.set('defaultTrendMode', 'delta') + assert.deepEqual(first.sets, []) + assert.deepEqual(second.sets, [{ field: 'defaultTrendMode', value: 'delta' }]) + }) + + test('a snapshot value that throws on property access keeps the current state', () => { + const s = createContextSettings() + const hostile = new Proxy({}, { get() { throw new Error('hostile') } }) + s.attach(new TestSettingsScope({ status: 'ready', value: hostile, writable: true })) + assert.deepEqual(s.store.getSnapshot(), { + status: 'ready', + placement: 'all', + granularity: 'step', + mode: 'total', + deltaBase: 'step', + toolSort: 'count', + fileSort: 'count', + insightsEntry: 'show', + durationCurve: 'show', + fleetTab: 'show', + promptPrefix: '', + promptSuffix: '', + writable: true, + }) + }) })