From bfa86b086ffade654a05c87b266d7853b2036008 Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Wed, 14 Oct 2026 09:17:50 +0200 Subject: [PATCH 1/5] fix: enforce and persist workspace-scoped auto-approve command lists (#1939) Workspace-scope zoo-code.allowedCommands/deniedCommands settings were never enforced, and saving the auto-approve dialog flattened the merged dialog list into user-global settings, leaking project entries into every workspace. Activation additionally seeded global state from the effective config, permanently copying workspace overrides into global settings. - Read/enforcement: new src/core/auto-approval/commandScope.ts unions the extension global-state layer with the workspace layer from inspect().workspaceFolderValue ?? workspaceValue; ClineProvider.getState() and getStateToPostToWebview() share the same merged value so enforcement and display cannot drift (old mergeCommandLists deleted). - Write/scope-leak: splitCommandListScopes() + resolveWorkspaceWriteTarget() keep workspace entries in their folder/workspace scope, route new entries to Global, remove deleted entries from the owning scope, and persist only the global layer to extension global state. - Activation seed leak: seedAllowedCommands.ts reads inspect().globalValue only, so workspace overrides are no longer folded into (and copied permanently to) global state at activation. - Dead code: remove vestigial allowedCommands/deniedCommands webview message handlers (zero senders), their WebviewMessage union members, and the orphaned commands payload. Closes #1939 --- packages/types/src/vscode-extension-host.ts | 3 - src/__tests__/extension.spec.ts | 71 ++++- .../__tests__/commandScope.spec.ts | 247 ++++++++++++++++++ src/core/auto-approval/commandScope.ts | 139 ++++++++++ src/core/webview/ClineProvider.ts | 73 +----- .../webview/__tests__/ClineProvider.spec.ts | 69 +++++ .../__tests__/webviewMessageHandler.spec.ts | 90 +++++++ src/core/webview/webviewMessageHandler.ts | 80 +++--- src/extension.ts | 9 +- .../__tests__/seedAllowedCommands.spec.ts | 121 +++++++++ src/utils/seedAllowedCommands.ts | 40 +++ 11 files changed, 824 insertions(+), 118 deletions(-) create mode 100644 src/core/auto-approval/__tests__/commandScope.spec.ts create mode 100644 src/core/auto-approval/commandScope.ts create mode 100644 src/utils/__tests__/seedAllowedCommands.spec.ts create mode 100644 src/utils/seedAllowedCommands.ts diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 2f1222f8ee..95316f5e74 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -603,9 +603,7 @@ export interface WebviewMessage { | "getDismissedUpsells" | "openMarkdownPreview" | "updateSettings" - | "allowedCommands" | "getTaskWithAggregatedCosts" - | "deniedCommands" | "openDebugApiHistory" | "openDebugUiHistory" | "downloadErrorDiagnostics" @@ -662,7 +660,6 @@ export interface WebviewMessage { stepIndex?: number isLaunchAction?: boolean forceShow?: boolean - commands?: string[] audioType?: AudioType serverName?: string toolName?: string diff --git a/src/__tests__/extension.spec.ts b/src/__tests__/extension.spec.ts index d0a8b274b8..ba8c885c0a 100644 --- a/src/__tests__/extension.spec.ts +++ b/src/__tests__/extension.spec.ts @@ -18,6 +18,7 @@ vi.mock("vscode", () => ({ registerTextDocumentContentProvider: vi.fn(), getConfiguration: vi.fn().mockReturnValue({ get: vi.fn().mockReturnValue([]), + inspect: vi.fn(), }), createFileSystemWatcher: vi.fn().mockReturnValue({ onDidCreate: vi.fn(), @@ -222,15 +223,17 @@ vi.mock("../api/providers/fetchers/modelCache", () => ({ describe("extension.ts", () => { let mockContext: vscode.ExtensionContext + let globalStateValues: Map let settingsUpdatedHandler: ((data: Record) => void | Promise) | undefined beforeEach(() => { vi.clearAllMocks() + globalStateValues = new Map() mockContext = { extensionPath: "/test/path", globalState: { - get: vi.fn().mockReturnValue(undefined), + get: vi.fn((key: string) => globalStateValues.get(key)), update: vi.fn(), }, subscriptions: [], @@ -404,6 +407,72 @@ describe("extension.ts", () => { expect(dotenv.config).toHaveBeenCalledTimes(1) }) + describe("allowedCommands activation seeding", () => { + function setConfigScope( + vscode: typeof import("vscode"), + scope: { globalValue?: string[]; workspaceValue?: string[] }, + ) { + const { globalValue, workspaceValue } = scope + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + // Key-scoped so proxy/other activation reads still see no value. + get: vi.fn((key: string) => + key === "allowedCommands" ? [...(globalValue ?? []), ...(workspaceValue ?? [])] : undefined, + ), + inspect: vi.fn().mockReturnValue({ + key: "allowedCommands", + globalValue, + workspaceValue, + }), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + } + + beforeEach(() => { + vi.resetModules() + }) + + afterEach(async () => { + const vscode = await import("vscode") + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue([]), + inspect: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + }) + + test("does not seed global state from a workspace-scoped override", async () => { + const vscode = await import("vscode") + setConfigScope(vscode, { workspaceValue: ["docker"] }) + + const { activate } = await import("../extension") + await activate(mockContext) + + expect(mockContext.globalState.update).not.toHaveBeenCalledWith("allowedCommands", expect.anything()) + expect(globalStateValues.get("allowedCommands")).toBeUndefined() + }) + + test("seeds global state from the user-scoped value when it is empty", async () => { + const vscode = await import("vscode") + setConfigScope(vscode, { globalValue: ["git"] }) + + const { activate } = await import("../extension") + await activate(mockContext) + + expect(mockContext.globalState.update).toHaveBeenCalledWith("allowedCommands", ["git"]) + }) + + test("does not overwrite global state that is already populated", async () => { + const vscode = await import("vscode") + setConfigScope(vscode, { globalValue: ["git"] }) + globalStateValues.set("allowedCommands", ["npm"]) + + const { activate } = await import("../extension") + await activate(mockContext) + + expect(mockContext.globalState.update).not.toHaveBeenCalledWith("allowedCommands", expect.anything()) + expect(globalStateValues.get("allowedCommands")).toEqual(["npm"]) + }) + }) + describe("cloud organization settings handling", () => { beforeEach(() => { vi.resetModules() diff --git a/src/core/auto-approval/__tests__/commandScope.spec.ts b/src/core/auto-approval/__tests__/commandScope.spec.ts new file mode 100644 index 0000000000..7784c4a1bd --- /dev/null +++ b/src/core/auto-approval/__tests__/commandScope.spec.ts @@ -0,0 +1,247 @@ +import * as vscode from "vscode" + +import { readCommandListScopes, resolveWorkspaceWriteTarget, splitCommandListScopes } from "../commandScope" + +vi.mock("vscode", () => ({ + ConfigurationTarget: { + Global: 1, + Workspace: 2, + WorkspaceFolder: 3, + }, + workspace: { + getConfiguration: vi.fn(), + }, +})) + +interface ConfigDouble { + inspect?: { + key: string + defaultValue?: string[] + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + } + effective?: string[] +} + +function mockConfiguration({ inspect, effective = [] }: ConfigDouble = {}) { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue(effective), + inspect: vi.fn().mockReturnValue(inspect), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) +} + +describe("readCommandListScopes", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("reads the workspace layer from the workspace-scoped configuration value", () => { + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker"] } }) + + const scopes = readCommandListScopes("deniedCommands", []) + + expect(scopes.workspace).toEqual(["docker"]) + expect(scopes.merged).toEqual(["docker"]) + }) + + it("prefers the workspace-folder value over the workspace value", () => { + mockConfiguration({ + inspect: { + key: "deniedCommands", + workspaceValue: ["docker"], + workspaceFolderValue: ["pnpm docker"], + }, + }) + + const scopes = readCommandListScopes("deniedCommands", []) + + expect(scopes.workspace).toEqual(["pnpm docker"]) + expect(scopes.merged).toEqual(["pnpm docker"]) + }) + + it("ignores the effective get() value so global-scoped entries are not double-counted", () => { + // A real VS Code get() folds globalValue into the effective result, so + // reading the workspace layer from get() would leak "npm" (a + // user-settings-only entry) into the workspace layer. + mockConfiguration({ + inspect: { key: "allowedCommands", globalValue: ["npm"], workspaceValue: ["docker"] }, + effective: ["npm", "docker"], + }) + + const scopes = readCommandListScopes("allowedCommands", []) + + expect(scopes.workspace).toEqual(["docker"]) + expect(scopes.merged).toEqual(["docker"]) + }) + + it("returns an empty workspace layer when only globalValue is set", () => { + mockConfiguration({ + inspect: { key: "allowedCommands", globalValue: ["npm"] }, + effective: ["npm"], + }) + + const scopes = readCommandListScopes("allowedCommands", []) + + expect(scopes.workspace).toEqual([]) + expect(scopes.merged).toEqual([]) + }) + + it("unions global and workspace layers with deduplication and stable order", () => { + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker", "rm"] } }) + + const scopes = readCommandListScopes("deniedCommands", ["git", "docker"]) + + expect(scopes.global).toEqual(["git", "docker"]) + expect(scopes.workspace).toEqual(["docker", "rm"]) + expect(scopes.merged).toEqual(["git", "docker", "rm"]) + }) + + it("drops blank and non-string entries from both layers", () => { + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker", " "] } }) + + const scopes = readCommandListScopes("deniedCommands", ["git", "", " "] as string[]) + + expect(scopes.global).toEqual(["git"]) + expect(scopes.workspace).toEqual(["docker"]) + expect(scopes.merged).toEqual(["git", "docker"]) + }) + + it("treats a missing inspect result as an empty workspace layer", () => { + mockConfiguration({ inspect: undefined }) + + const scopes = readCommandListScopes("deniedCommands", ["git"]) + + expect(scopes.workspace).toEqual([]) + expect(scopes.merged).toEqual(["git"]) + }) + + it("falls back to an empty workspace layer when the configuration lookup throws", () => { + vi.mocked(vscode.workspace.getConfiguration).mockImplementation(() => { + throw new Error("configuration unavailable") + }) + + const scopes = readCommandListScopes("deniedCommands", ["git"]) + + expect(scopes.workspace).toEqual([]) + expect(scopes.merged).toEqual(["git"]) + }) +}) + +describe("splitCommandListScopes", () => { + it("keeps workspace entries in the workspace layer and routes new entries to global", () => { + // The issue #1939 leak: the dialog sends the merged list back, and the + // workspace-only entry "docker" must not end up in the global write. + const result = splitCommandListScopes(["docker", "aws"], ["docker"], []) + + expect(result.workspaceWrite).toEqual(["docker"]) + expect(result.globalWrite).toEqual(["aws"]) + }) + + it("keeps a global entry in the global layer", () => { + const result = splitCommandListScopes(["git"], [], ["git"]) + + expect(result.workspaceWrite).toEqual([]) + expect(result.globalWrite).toEqual(["git"]) + }) + + it("keeps an entry present in both layers in both layers when retained", () => { + // Semantics: do not silently delete the user's global entry just + // because the current folder happens to define it too; the entry was + // deliberately authored in both scopes and the user kept it. + const result = splitCommandListScopes(["docker"], ["docker"], ["docker"]) + + expect(result.workspaceWrite).toEqual(["docker"]) + expect(result.globalWrite).toEqual(["docker"]) + }) + + it("removes a deleted entry from the workspace layer without touching global", () => { + // The user kept "git" and deleted workspace-only "docker"; "git" must + // survive at its own (global) layer. + const result = splitCommandListScopes(["git"], ["docker"], ["git"]) + + expect(result.workspaceWrite).toEqual([]) + expect(result.globalWrite).toEqual(["git"]) + }) + + it("removes a deleted entry from every layer that contained it", () => { + // "docker" lived in both layers and the user deleted it; kept "rm" + // stays at the workspace layer only. + const result = splitCommandListScopes(["rm"], ["docker", "rm"], ["docker"]) + + expect(result.workspaceWrite).toEqual(["rm"]) + expect(result.globalWrite).toEqual([]) + }) + + it("routes entries the dialog seeded to the layer they came from", () => { + const result = splitCommandListScopes( + ["git", "docker", "pnpm docker", "aws"], + ["docker", "pnpm docker"], + ["git"], + ) + + expect(result.workspaceWrite).toEqual(["docker", "pnpm docker"]) + expect(result.globalWrite).toEqual(["git", "aws"]) + }) + + it("orders the global write as existing global entries then new entries in dialog order", () => { + const result = splitCommandListScopes(["aws", "git", "npm"], ["docker"], ["git"]) + + expect(result.globalWrite).toEqual(["git", "aws", "npm"]) + }) + + it("drops blank and non-string entries from the incoming list", () => { + const result = splitCommandListScopes(["git", "", " ", 42 as unknown as string], [], ["git"]) + + expect(result.globalWrite).toEqual(["git"]) + expect(result.workspaceWrite).toEqual([]) + }) + + it("deduplicates layers so re-writing cannot introduce duplicates", () => { + const result = splitCommandListScopes(["docker", "docker"], ["docker", "docker"], ["docker"]) + + expect(result.workspaceWrite).toEqual(["docker"]) + expect(result.globalWrite).toEqual(["docker"]) + }) +}) + +describe("resolveWorkspaceWriteTarget", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("returns the workspace-folder target when a folder value is set", () => { + mockConfiguration({ + inspect: { key: "deniedCommands", workspaceValue: ["docker"], workspaceFolderValue: ["pnpm docker"] }, + }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBe(vscode.ConfigurationTarget.WorkspaceFolder) + }) + + it("returns the workspace target when only a workspace value is set", () => { + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker"] } }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBe(vscode.ConfigurationTarget.Workspace) + }) + + it("returns undefined when only a global value is set, so no workspace layer is created", () => { + mockConfiguration({ inspect: { key: "deniedCommands", globalValue: ["npm"] } }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() + }) + + it("returns undefined when there is no inspect result", () => { + mockConfiguration({ inspect: undefined }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() + }) + + it("returns undefined when the configuration lookup throws", () => { + vi.mocked(vscode.workspace.getConfiguration).mockImplementation(() => { + throw new Error("configuration unavailable") + }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() + }) +}) diff --git a/src/core/auto-approval/commandScope.ts b/src/core/auto-approval/commandScope.ts new file mode 100644 index 0000000000..55c895e8c3 --- /dev/null +++ b/src/core/auto-approval/commandScope.ts @@ -0,0 +1,139 @@ +import * as vscode from "vscode" + +import { Package } from "../../shared/package" + +export type CommandListConfigKey = "allowedCommands" | "deniedCommands" + +export interface CommandListScopes { + global: string[] + workspace: string[] + merged: string[] +} + +/** + * Reads an auto-approval command list from both configuration layers and + * composes the list that should be enforced and displayed. + * + * The workspace layer is read via `inspect()` rather than `get()` on purpose: + * `get()` returns the effective value, which already folds `globalValue` into + * the result. Unioning that with the global layer would double-count entries + * and make removals from global settings unable to shrink the enforced set. + * + * @param configKey - The extension configuration key to read. + * @param globalList - The list persisted in extension global state. + * @returns The per-layer lists and their deduplicated set union. + */ +export function readCommandListScopes( + configKey: CommandListConfigKey, + globalList: string[] | undefined, +): CommandListScopes { + const global = sanitizeCommandList(globalList) + let workspace: string[] = [] + + try { + const inspected = vscode.workspace.getConfiguration(Package.name).inspect(configKey) + // The folder value wins over the workspace value, matching VS Code's own + // resolution precedence for multi-root workspaces. + workspace = sanitizeCommandList(inspected?.workspaceFolderValue ?? inspected?.workspaceValue) + } catch { + // Configuration access can fail (for example before the workspace is + // ready); an absent workspace layer must not break enforcement. + workspace = [] + } + + // Set union: an entry present in both layers collapses to one, so + // composition cannot change prefix-match outcomes. + const merged = [...new Set([...global, ...workspace])] + + return { global, workspace, merged } +} + +export interface CommandListWriteScopes { + globalWrite: string[] + workspaceWrite: string[] +} + +/** + * Splits the merged command list the settings dialog saves back into the two + * configuration layers it was composed from, so saving the dialog cannot + * flatten workspace-scoped entries into the user's global settings. + * + * Semantics: + * - An entry the user kept that exists in the workspace layer stays in the + * workspace layer. If the same entry also exists in the global layer it + * stays there too: dropping it from the global write would silently delete + * a user's global entry just because the current folder happens to define + * it as well. + * - An entry the user removed is dropped from every layer that contained it, + * which makes workspace-scoped entries deletable from the dialog. + * - A brand-new entry (in neither layer) lands in the global layer, matching + * the pre-existing behavior of routing dialog edits to + * `ConfigurationTarget.Global`. + * + * @param incoming - The full merged list sent by the webview on save. + * @param workspaceLayer - The current workspace-scope configuration value. + * @param globalLayer - The list currently persisted in extension global state. + * @returns The per-layer values to write back. + */ +export function splitCommandListScopes( + incoming: string[], + workspaceLayer: string[], + globalLayer: string[], +): CommandListWriteScopes { + const kept = new Set(sanitizeCommandList(incoming)) + const workspace = sanitizeCommandList(workspaceLayer) + const global = sanitizeCommandList(globalLayer) + const known = new Set([...workspace, ...global]) + + // Deduplicated so re-writing a layer cannot introduce duplicates. + const workspaceWrite = [...new Set(workspace.filter((command) => kept.has(command)))] + const globalWrite = [ + ...new Set(global.filter((command) => kept.has(command))), + // Preserve the dialog's ordering for entries the user just added. + ...[...kept].filter((command) => !known.has(command)), + ] + + return { globalWrite, workspaceWrite } +} + +/** + * Picks the configuration target that owns the current workspace-scope layer + * for `configKey`, mirroring how `readCommandListScopes` reads it via an + * unscoped `getConfiguration(Package.name)` (which resolves folder values + * from the first workspace folder). + * + * - `workspaceFolderValue` set -> write at `WorkspaceFolder` scope. + * - Otherwise `workspaceValue` set -> write at `Workspace` scope. + * - Neither set -> `undefined`: no workspace layer exists, so none should be + * created (an empty workspace layer must not materialize in settings.json). + * + * @param configKey - The extension configuration key to inspect. + * @returns The workspace-scoped target to write, or `undefined` when no + * workspace layer exists. + */ +export function resolveWorkspaceWriteTarget( + configKey: CommandListConfigKey, +): vscode.ConfigurationTarget.WorkspaceFolder | vscode.ConfigurationTarget.Workspace | undefined { + try { + const inspected = vscode.workspace.getConfiguration(Package.name).inspect(configKey) + if (inspected?.workspaceFolderValue !== undefined) { + return vscode.ConfigurationTarget.WorkspaceFolder + } + if (inspected?.workspaceValue !== undefined) { + return vscode.ConfigurationTarget.Workspace + } + return undefined + } catch { + // Same defensive posture as readCommandListScopes: configuration + // access failures mean we create no workspace layer. + return undefined + } +} + +function sanitizeCommandList(list: unknown): string[] { + if (!Array.isArray(list)) { + return [] + } + + return list.filter((command): command is string => typeof command === "string" && command.trim().length > 0) +} diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 874e2f8f08..78ac7c4870 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -112,6 +112,7 @@ import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/provi import { ContextProxy } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" +import { readCommandListScopes } from "../auto-approval/commandScope" import { PendingActionSettlementError, Task } from "../task/Task" import { webviewMessageHandler } from "./webviewMessageHandler" @@ -2528,62 +2529,6 @@ export class ClineProvider } } - /** - * Merges allowed commands from global state and workspace configuration - * with proper validation and deduplication - */ - private mergeAllowedCommands(globalStateCommands?: string[]): string[] { - return this.mergeCommandLists("allowedCommands", "allowed", globalStateCommands) - } - - /** - * Merges denied commands from global state and workspace configuration - * with proper validation and deduplication - */ - private mergeDeniedCommands(globalStateCommands?: string[]): string[] { - return this.mergeCommandLists("deniedCommands", "denied", globalStateCommands) - } - - /** - * Common utility for merging command lists from global state and workspace configuration. - * Implements the Command Denylist feature's merging strategy with proper validation. - * - * @param configKey - VSCode workspace configuration key - * @param commandType - Type of commands for error logging - * @param globalStateCommands - Commands from global state - * @returns Merged and deduplicated command list - */ - private mergeCommandLists( - configKey: "allowedCommands" | "deniedCommands", - commandType: "allowed" | "denied", - globalStateCommands?: string[], - ): string[] { - try { - // Validate and sanitize global state commands - const validGlobalCommands = Array.isArray(globalStateCommands) - ? globalStateCommands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] - - // Get workspace configuration commands - const workspaceCommands = vscode.workspace.getConfiguration(Package.name).get(configKey) || [] - - // Validate and sanitize workspace commands - const validWorkspaceCommands = Array.isArray(workspaceCommands) - ? workspaceCommands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] - - // Combine and deduplicate commands - // Global state takes precedence over workspace configuration - const mergedCommands = [...new Set([...validGlobalCommands, ...validWorkspaceCommands])] - - return mergedCommands - } catch (error) { - console.error(`Error merging ${commandType} commands:`, error) - // Return empty array as fallback to prevent crashes - return [] - } - } - async getStateToPostToWebview({ includeTaskHistory = true }: GetStateOptions = {}): Promise { // Ensure the store is initialized before reading task history await this.taskHistoryStore.initialized @@ -2705,8 +2650,6 @@ export class ClineProvider const telemetryKey = process.env.POSTHOG_API_KEY const machineId = vscode.env.machineId const vscodeTelemetryEnabled = vscode.env.isTelemetryEnabled - const mergedAllowedCommands = this.mergeAllowedCommands(allowedCommands) - const mergedDeniedCommands = this.mergeDeniedCommands(deniedCommands) const cwd = this.cwd const currentTask = this.getCurrentTask() let currentTaskMode: string | undefined @@ -2786,8 +2729,8 @@ export class ClineProvider checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, shouldShowAnnouncement: telemetrySetting !== "unset" && lastShownAnnouncementId !== this.latestAnnouncementId, - allowedCommands: mergedAllowedCommands, - deniedCommands: mergedDeniedCommands, + allowedCommands: allowedCommands ?? [], + deniedCommands: deniedCommands ?? [], soundVolume: soundVolume ?? 0.5, writeDelayMs: writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, diffFuzzyThreshold: diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, @@ -3016,8 +2959,11 @@ export class ClineProvider autoCondenseContext: stateValues.autoCondenseContext ?? true, autoCondenseContextPercent: stateValues.autoCondenseContextPercent ?? 100, taskHistory: includeTaskHistory ? this.taskHistoryStore.getAll() : [], - allowedCommands: stateValues.allowedCommands, - deniedCommands: stateValues.deniedCommands, + // Enforce and display the set union of the global-state layer and the + // workspace-scope configuration layer; getStateToPostToWebview shows + // exactly these values, so display cannot drift from enforcement. + allowedCommands: readCommandListScopes("allowedCommands", stateValues.allowedCommands).merged, + deniedCommands: readCommandListScopes("deniedCommands", stateValues.deniedCommands).merged, soundEnabled: stateValues.soundEnabled ?? false, ttsEnabled: stateValues.ttsEnabled ?? false, ttsSpeed: stateValues.ttsSpeed ?? 1.0, @@ -3350,6 +3296,9 @@ export class ClineProvider if (configuration) { await this.setValues(configuration) + // Deliberately Global-targeted: these writes carry "start this task with + // exactly this auto-approval policy" semantics (e.g. from handoff/delegation + // payloads), unlike the scope-split persistence in the settings dialog. if (configuration.allowedCommands) { await vscode.workspace .getConfiguration(Package.name) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index b5f304fe56..4a96fadde0 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1615,6 +1615,75 @@ describe("ClineProvider", () => { expect(state.alwaysDenyUnapprovedCommands).toBe(false) }) + describe("command list scope merging", () => { + // getConfiguration overrides must not leak past these tests: + // clearAllMocks does not remove mockReturnValue defaults. + afterEach(() => { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue([]), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + }) + + test("getState enforces workspace-scope denied commands even when global state is empty", async () => { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue([]), + inspect: vi + .fn() + .mockImplementation((key: string) => + key === "deniedCommands" ? { key, workspaceValue: ["docker"] } : undefined, + ), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + + const state = await provider.getState() + + expect(state.deniedCommands).toEqual(["docker"]) + }) + + test("getStateToPostToWebview shows the same merged command lists as getState", async () => { + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("deniedCommands", ["git"]) + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue(["git", "docker"]), + inspect: vi + .fn() + .mockImplementation((key: string) => + key === "deniedCommands" ? { key, workspaceValue: ["docker"] } : undefined, + ), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + + const state = await provider.getStateToPostToWebview() + + // Display is the set union of global state and workspace scope, matching + // what getState() enforces. + expect(state.deniedCommands).toEqual(["git", "docker"]) + expect(state.allowedCommands).toEqual([]) + }) + + test("getState prefers the workspace-folder value for multi-root workspaces", async () => { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue(["docker", "pnpm docker"]), + inspect: vi + .fn() + .mockImplementation((key: string) => + key === "deniedCommands" + ? { key, workspaceValue: ["docker"], workspaceFolderValue: ["pnpm docker"] } + : undefined, + ), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) + + const state = await provider.getState() + + // The folder value replaces (not unions with) the workspace value, and + // the effective get() value is ignored so global-scoped entries do not + // leak into the workspace layer. + expect(state.deniedCommands).toEqual(["pnpm docker"]) + }) + }) + test("getState returns the saved allowed read files", async () => { await provider.contextProxy.setValue("allowedReadFiles", ["notes.md"]) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index dcd70f92f1..3d2f6abc21 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -178,6 +178,11 @@ vi.mock("vscode", () => { const showTextDocument = vi.fn().mockResolvedValue(undefined) return { + ConfigurationTarget: { + Global: 1, + Workspace: 2, + WorkspaceFolder: 3, + }, window: { showInformationMessage, showErrorMessage, @@ -1429,6 +1434,91 @@ describe("webviewMessageHandler - allowlists together", () => { }) }) +describe("webviewMessageHandler - command list scope-aware persistence", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + // Mirrors the real VS Code configuration API: `inspect()` exposes the + // per-scope values, and `update()` records the target it was written to. + // Extension global state is left unset (getValue returns undefined), i.e. + // the global layer starts empty. + function mockCommandConfiguration(inspected: { + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + }) { + const update = vi.fn() + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue([]), + inspect: vi.fn().mockReturnValue(inspected), + update, + } as never) + return update + } + + it("keeps workspace-scoped denied commands out of the global write when saving the merged list", async () => { + const update = mockCommandConfiguration({ workspaceValue: ["docker"] }) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + // "docker" stays at the workspace scope instead of leaking into the + // user's global settings (issue #1939 leg B). + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) + // Extension global state holds only the global layer, so the next save + // cannot double-home the workspace entry. + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) + }) + + it("writes the workspace-folder value at the folder target in multi-root workspaces", async () => { + const update = mockCommandConfiguration({ workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.WorkspaceFolder) + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) + }) + + it("clears a deleted workspace entry at the workspace scope without adding it to global", async () => { + const update = mockCommandConfiguration({ workspaceValue: ["docker"] }) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: [] }, + }) + + // Deleting a project entry must clear it where it lives... + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Workspace) + // ...and must not copy it into global settings. + expect(update).not.toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Global) + expect(update).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", []) + }) + + it("does not create a workspace layer when none exists", async () => { + const update = mockCommandConfiguration({}) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { allowedCommands: ["git"] }, + }) + + // Only the Global write happens; saving must not materialize an empty + // workspace-scoped value in settings.json. + expect(update).toHaveBeenCalledTimes(1) + expect(update).toHaveBeenCalledWith("allowedCommands", ["git"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("allowedCommands", ["git"]) + }) +}) + describe("webviewMessageHandler - terminalProfile", () => { beforeEach(() => { vi.clearAllMocks() diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 4ba94d454c..374427f966 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -59,6 +59,11 @@ import { } from "./rulesMessageHandler" import { changeLanguage, t } from "../../i18n" import { Package } from "../../shared/package" +import { + readCommandListScopes, + resolveWorkspaceWriteTarget, + splitCommandListScopes, +} from "../auto-approval/commandScope" import { type RouterName, toRouterName } from "../../shared/api" import { MessageEnhancer } from "./messageEnhancer" @@ -751,26 +756,43 @@ export const webviewMessageHandler = async ( if (key === "language") { newValue = value ?? "en" changeLanguage(newValue as Language) - } else if (key === "allowedCommands") { + } else if (key === "allowedCommands" || key === "deniedCommands") { const commands = value ?? [] - newValue = Array.isArray(commands) + const incoming = Array.isArray(commands) ? commands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) : [] - await vscode.workspace - .getConfiguration(Package.name) - .update("allowedCommands", newValue, vscode.ConfigurationTarget.Global) - } else if (key === "deniedCommands") { - const commands = value ?? [] + // The webview always sends the full merged list (it displays + // the union of the global-state and workspace-scope layers). + // Read both layers BEFORE any write and split the incoming + // list by origin scope, so saving the dialog cannot flatten + // workspace-scoped entries into the user's global settings. + const currentScopes = readCommandListScopes(key, getGlobalState(key)) + const { globalWrite, workspaceWrite } = splitCommandListScopes( + incoming, + currentScopes.workspace, + currentScopes.global, + ) + const workspaceTarget = resolveWorkspaceWriteTarget(key) + const config = vscode.workspace.getConfiguration(Package.name) + + // Write the workspace layer first, at the target that owns it. + // When no workspace layer exists the write is skipped entirely + // so saving never materializes one; when entries were deleted + // the `[]` write clears them at that scope. + if (workspaceTarget !== undefined) { + await config.update(key, workspaceWrite, workspaceTarget) + } - newValue = Array.isArray(commands) - ? commands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] + await config.update(key, globalWrite, vscode.ConfigurationTarget.Global) - await vscode.workspace - .getConfiguration(Package.name) - .update("deniedCommands", newValue, vscode.ConfigurationTarget.Global) + // Extension global state keeps only the global layer: + // enforcement re-adds the workspace layer through + // readCommandListScopes, so storing the merged list here + // would double-home workspace entries and re-leak them on the + // next save. + newValue = globalWrite } else if (key === "allowedReadFiles" || key === "allowedWriteFiles") { const patterns = value ?? [] @@ -1712,38 +1734,6 @@ export const webviewMessageHandler = async ( // Cancel any pending auto-approval timeout for the current task provider.getCurrentTask()?.cancelAutoApprovalTimeout() break - case "allowedCommands": { - // Validate and sanitize the commands array - const commands = message.commands ?? [] - const validCommands = Array.isArray(commands) - ? commands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] - - await updateGlobalState("allowedCommands", validCommands) - - // Also update workspace settings. - await vscode.workspace - .getConfiguration(Package.name) - .update("allowedCommands", validCommands, vscode.ConfigurationTarget.Global) - - break - } - case "deniedCommands": { - // Validate and sanitize the commands array - const commands = message.commands ?? [] - const validCommands = Array.isArray(commands) - ? commands.filter((cmd) => typeof cmd === "string" && cmd.trim().length > 0) - : [] - - await updateGlobalState("deniedCommands", validCommands) - - // Also update workspace settings. - await vscode.workspace - .getConfiguration(Package.name) - .update("deniedCommands", validCommands, vscode.ConfigurationTarget.Global) - - break - } case "openCustomModesSettings": { const customModesFilePath = await provider.customModesManager.getCustomModesFilePath() diff --git a/src/extension.ts b/src/extension.ts index 1d71f834df..080ca5bae9 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -38,6 +38,7 @@ import { CodeIndexManagerRegistry } from "./services/code-index/code-index-manag import { CodeIndexScope } from "./services/code-index/code-index-scope" import { MdmService } from "./services/mdm/MdmService" import { migrateSettings } from "./utils/migrateSettings" +import { seedAllowedCommands } from "./utils/seedAllowedCommands" import { autoImportSettings } from "./utils/autoImportSettings" import { API } from "./extension/api" @@ -163,13 +164,7 @@ export async function activate(context: vscode.ExtensionContext) { // Initialize Zoo Code auth service for extension session token management. await initZooCodeAuth(context) - // Get default commands from configuration. - const defaultCommands = vscode.workspace.getConfiguration(Package.name).get("allowedCommands") || [] - - // Initialize global state if not already set. - if (!context.globalState.get("allowedCommands")) { - context.globalState.update("allowedCommands", defaultCommands) - } + await seedAllowedCommands(context.globalState) const contextProxy = await ContextProxy.getInstance(context) const updateTelemetryState = () => { diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts new file mode 100644 index 0000000000..19c2623dfb --- /dev/null +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -0,0 +1,121 @@ +// npx vitest run utils/__tests__/seedAllowedCommands.spec.ts + +import * as vscode from "vscode" + +import { seedAllowedCommands } from "../seedAllowedCommands" + +vi.mock("vscode", () => ({ + workspace: { + getConfiguration: vi.fn(), + }, +})) + +interface ConfigDouble { + inspect?: { + key: string + defaultValue?: string[] + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + } + effective?: string[] + inspectThrows?: boolean +} + +function mockConfiguration({ inspect, effective = [], inspectThrows = false }: ConfigDouble = {}) { + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ + get: vi.fn().mockReturnValue(effective), + inspect: vi.fn().mockImplementation(() => { + if (inspectThrows) { + throw new Error("configuration access failed") + } + return inspect + }), + update: vi.fn(), + } as unknown as vscode.WorkspaceConfiguration) +} + +function mockMemento(existing: unknown = undefined) { + const store = new Map() + if (existing !== undefined) { + store.set("allowedCommands", existing) + } + + return { + get: vi.fn((key: string) => store.get(key)), + update: vi.fn(async (key: string, value: unknown) => { + store.set(key, value) + }), + } +} + +describe("seedAllowedCommands", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("leaves global state unset when only a workspace-scoped value exists", async () => { + // A real VS Code get() folds the workspace override into the effective + // value, so this is the leak scenario: reading get() would seed + // ["docker"] (from .vscode/settings.json) into global state. + mockConfiguration({ + inspect: { key: "allowedCommands", globalValue: undefined, workspaceValue: ["docker"] }, + effective: ["docker"], + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toBeUndefined() + }) + + it("seeds from the global-scope value when global state is empty", async () => { + mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", ["git"]) + expect(globalState.get("allowedCommands")).toEqual(["git"]) + }) + + it("does not overwrite global state that is already populated", async () => { + mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) + const globalState = mockMemento(["npm"]) + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toEqual(["npm"]) + }) + + it("leaves global state untouched when there is no inspect result at all", async () => { + mockConfiguration({ inspect: undefined, effective: ["git"] }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).not.toHaveBeenCalled() + }) + + it("skips the seed without throwing when configuration access fails", async () => { + mockConfiguration({ inspectThrows: true }) + const globalState = mockMemento() + + await expect(seedAllowedCommands(globalState as unknown as vscode.Memento)).resolves.toBeUndefined() + + expect(globalState.update).not.toHaveBeenCalled() + }) + + it("does not share a reference with the inspected configuration value", async () => { + const globalValue = ["git"] + mockConfiguration({ inspect: { key: "allowedCommands", globalValue } }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState as unknown as vscode.Memento) + + expect(globalState.update).toHaveBeenCalledWith("allowedCommands", expect.arrayContaining(["git"])) + expect(globalState.update.mock.calls[0][1]).not.toBe(globalValue) + }) +}) diff --git a/src/utils/seedAllowedCommands.ts b/src/utils/seedAllowedCommands.ts new file mode 100644 index 0000000000..6a56173e47 --- /dev/null +++ b/src/utils/seedAllowedCommands.ts @@ -0,0 +1,40 @@ +import * as vscode from "vscode" + +import { Package } from "../shared/package" + +/** + * Seeds the extension's global-state allowlist from the `allowedCommands` + * setting, but only when global state holds no list yet (first run). + * + * The seed reads the global-scope value via `inspect()` rather than the + * effective `get()` on purpose: `get()` folds in workspace-scoped overrides, + * so a project's `.vscode/settings.json` would otherwise be copied + * permanently into global state the first time the extension activates inside + * that project — a leak that survives closing the project. + * + * @param globalState - The extension global-state memento to seed. + */ +export async function seedAllowedCommands(globalState: vscode.Memento): Promise { + if (globalState.get("allowedCommands")) { + return + } + + let globalValue: string[] | undefined + try { + globalValue = vscode.workspace.getConfiguration(Package.name).inspect("allowedCommands")?.globalValue + } catch { + // Configuration access can fail (for example before the workspace is + // ready); skipping the seed only defers it to a later activation. + return + } + + // An absent global-scope value leaves global state untouched: seeding the + // package.json default or a workspace override would defeat the point of + // scoping the seed to the user's own settings. + if (!Array.isArray(globalValue)) { + return + } + + // Copy so global state never shares a reference with the configuration object. + await globalState.update("allowedCommands", [...globalValue]) +} From 420cbe80f46de74fb8df54273186eb4ee34c320e Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Wed, 14 Oct 2026 18:29:01 +0200 Subject: [PATCH 2/5] fix: compensate failed command-list saves and scope config access per review (#1939) - Rewind the workspace-layer command-list write when the global-layer write fails: the raw previous value is captured before saving, the failed key skips its extension global-state update while the other keys still save, and a failed rewind is reported alongside the original error. - Read and write the command lists through one configuration object scoped to the first workspace folder, so workspace-folder targets are legal and resolve against the right folder's settings (the VS Code API rejects folder-targeted updates on an unscoped configuration). - Narrow the try/catch around configuration lookups so a failed lookup stays distinct from an absent value; behavior is unchanged, the guarded failure mode is now testable. - Add regression tests for the compensation path, the scoped reads and writes, and the seed guard; tighten comments. --- src/__tests__/extension.spec.ts | 4 + .../__tests__/commandScope.spec.ts | 52 +++++- src/core/auto-approval/commandScope.ts | 66 +++++--- .../__tests__/webviewMessageHandler.spec.ts | 151 ++++++++++++++++-- src/core/webview/webviewMessageHandler.ts | 73 +++++++-- .../__tests__/seedAllowedCommands.spec.ts | 61 ++++--- src/utils/seedAllowedCommands.ts | 12 +- 7 files changed, 349 insertions(+), 70 deletions(-) diff --git a/src/__tests__/extension.spec.ts b/src/__tests__/extension.spec.ts index ba8c885c0a..3b52363763 100644 --- a/src/__tests__/extension.spec.ts +++ b/src/__tests__/extension.spec.ts @@ -424,6 +424,8 @@ describe("extension.ts", () => { workspaceValue, }), update: vi.fn(), + // The double scripts key-scoped get() and per-scope inspect() + // shapes the shared helper cannot express without obscuring them. } as unknown as vscode.WorkspaceConfiguration) } @@ -436,6 +438,8 @@ describe("extension.ts", () => { vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ get: vi.fn().mockReturnValue([]), inspect: vi.fn(), + // Restores the neutral default after the scope fixtures above; + // update is intentionally absent because no test runs after this. } as unknown as vscode.WorkspaceConfiguration) }) diff --git a/src/core/auto-approval/__tests__/commandScope.spec.ts b/src/core/auto-approval/__tests__/commandScope.spec.ts index 7784c4a1bd..58c48fbac3 100644 --- a/src/core/auto-approval/__tests__/commandScope.spec.ts +++ b/src/core/auto-approval/__tests__/commandScope.spec.ts @@ -1,7 +1,16 @@ import * as vscode from "vscode" +import { Package } from "../../../shared/package" +import { makeUri, makeWorkspaceConfiguration } from "../../../test-utils/vscode" import { readCommandListScopes, resolveWorkspaceWriteTarget, splitCommandListScopes } from "../commandScope" +// Scriptable stand-in for `vscode.workspace.workspaceFolders`: the module +// mock's getter reads through this object, so a test can change the open +// folders without re-mocking the module. +const workspaceState = vi.hoisted(() => ({ + workspaceFolders: undefined as { uri: unknown }[] | undefined, +})) + vi.mock("vscode", () => ({ ConfigurationTarget: { Global: 1, @@ -9,6 +18,9 @@ vi.mock("vscode", () => ({ WorkspaceFolder: 3, }, workspace: { + get workspaceFolders() { + return workspaceState.workspaceFolders + }, getConfiguration: vi.fn(), }, })) @@ -25,11 +37,12 @@ interface ConfigDouble { } function mockConfiguration({ inspect, effective = [] }: ConfigDouble = {}) { - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue(effective), - inspect: vi.fn().mockReturnValue(inspect), - update: vi.fn(), - } as unknown as vscode.WorkspaceConfiguration) + const config = makeWorkspaceConfiguration() + // The shared double is scripted after creation so the per-test fixture + // values need no type assertion at the construction site. + vi.mocked(config.get).mockReturnValue(effective) + vi.mocked(config.inspect).mockReturnValue(inspect) + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) } describe("readCommandListScopes", () => { @@ -192,6 +205,8 @@ describe("splitCommandListScopes", () => { }) it("drops blank and non-string entries from the incoming list", () => { + // Deliberate wrong runtime type: configuration lists arrive as unknown, + // so the sanitizer must drop non-strings instead of throwing on them. const result = splitCommandListScopes(["git", "", " ", 42 as unknown as string], [], ["git"]) expect(result.globalWrite).toEqual(["git"]) @@ -209,6 +224,7 @@ describe("splitCommandListScopes", () => { describe("resolveWorkspaceWriteTarget", () => { beforeEach(() => { vi.clearAllMocks() + workspaceState.workspaceFolders = undefined }) it("returns the workspace-folder target when a folder value is set", () => { @@ -244,4 +260,30 @@ describe("resolveWorkspaceWriteTarget", () => { expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() }) + + it("resolves the target from the configuration scoped to the first workspace folder", () => { + const folderA = makeUri("/mock/folder-a") + const folderB = makeUri("/mock/folder-b") + workspaceState.workspaceFolders = [{ uri: folderA }, { uri: folderB }] + mockConfiguration({ inspect: { key: "deniedCommands", workspaceFolderValue: ["pnpm docker"] } }) + + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBe(vscode.ConfigurationTarget.WorkspaceFolder) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderA) + // Only the first folder is ever in scope: reading any other folder's + // configuration would silently target the wrong folder's settings file. + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalledWith(Package.name, folderB) + }) + + it.each([undefined, [] as { uri: unknown }[]])( + "falls back to an unscoped configuration when no workspace folder is open (%s)", + (workspaceFolders) => { + workspaceState.workspaceFolders = workspaceFolders + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker"] } }) + + // With no resource to resolve against, no folder value can be + // reported, so the folder target stays unreachable. + expect(resolveWorkspaceWriteTarget("deniedCommands")).toBe(vscode.ConfigurationTarget.Workspace) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, undefined) + }, + ) }) diff --git a/src/core/auto-approval/commandScope.ts b/src/core/auto-approval/commandScope.ts index 55c895e8c3..4778ac1f3e 100644 --- a/src/core/auto-approval/commandScope.ts +++ b/src/core/auto-approval/commandScope.ts @@ -10,6 +10,20 @@ export interface CommandListScopes { merged: string[] } +/** + * The configuration object for every command-list read and write, scoped to + * the first workspace folder. Sharing one scoped access path keeps the two + * sides symmetric: `inspect()` can report `workspaceFolderValue`, and + * `ConfigurationTarget.WorkspaceFolder` updates are legal (the API contract + * throws for that target when the configuration is not scoped to a + * resource). In a multi-root workspace only the first folder's values are in + * scope; with no folder open the configuration is unscoped and reports no + * folder value. + */ +export function commandListConfiguration(): vscode.WorkspaceConfiguration { + return vscode.workspace.getConfiguration(Package.name, vscode.workspace.workspaceFolders?.[0]?.uri) +} + /** * Reads an auto-approval command list from both configuration layers and * composes the list that should be enforced and displayed. @@ -28,19 +42,23 @@ export function readCommandListScopes( globalList: string[] | undefined, ): CommandListScopes { const global = sanitizeCommandList(globalList) - let workspace: string[] = [] + // Only the configuration lookup itself is guarded: it can fail (for example + // before the workspace is ready), and an absent workspace layer must not + // break enforcement. The value reads below have no failure mode of their + // own, so the catch covers exactly the failure it exists to absorb. + let inspected: { workspaceValue?: string[]; workspaceFolderValue?: string[] } | undefined try { - const inspected = vscode.workspace.getConfiguration(Package.name).inspect(configKey) - // The folder value wins over the workspace value, matching VS Code's own - // resolution precedence for multi-root workspaces. - workspace = sanitizeCommandList(inspected?.workspaceFolderValue ?? inspected?.workspaceValue) + inspected = commandListConfiguration().inspect(configKey) } catch { - // Configuration access can fail (for example before the workspace is - // ready); an absent workspace layer must not break enforcement. - workspace = [] + inspected = undefined } + // The folder value wins over the workspace value, matching VS Code's + // folder-over-workspace precedence. An absent inspect result composes to + // an empty workspace layer: enforcement must not break. + const workspace = sanitizeCommandList(inspected?.workspaceFolderValue ?? inspected?.workspaceValue) + // Set union: an entry present in both layers collapses to one, so // composition cannot change prefix-match outcomes. const merged = [...new Set([...global, ...workspace])] @@ -98,9 +116,11 @@ export function splitCommandListScopes( /** * Picks the configuration target that owns the current workspace-scope layer - * for `configKey`, mirroring how `readCommandListScopes` reads it via an - * unscoped `getConfiguration(Package.name)` (which resolves folder values - * from the first workspace folder). + * for `configKey`, inspecting the same `commandListConfiguration()` object + * `readCommandListScopes` reads. Because that configuration is scoped to the + * first workspace folder, folder values resolve against that folder's + * settings; with no folder open the configuration is unscoped and reports no + * folder value, so no folder target can be selected. * * - `workspaceFolderValue` set -> write at `WorkspaceFolder` scope. * - Otherwise `workspaceValue` set -> write at `Workspace` scope. @@ -114,20 +134,22 @@ export function splitCommandListScopes( export function resolveWorkspaceWriteTarget( configKey: CommandListConfigKey, ): vscode.ConfigurationTarget.WorkspaceFolder | vscode.ConfigurationTarget.Workspace | undefined { + // Same defensive posture as readCommandListScopes: only the configuration + // lookup itself may fail, and a failure means we create no workspace layer. + let inspected: { workspaceValue?: string[]; workspaceFolderValue?: string[] } | undefined try { - const inspected = vscode.workspace.getConfiguration(Package.name).inspect(configKey) - if (inspected?.workspaceFolderValue !== undefined) { - return vscode.ConfigurationTarget.WorkspaceFolder - } - if (inspected?.workspaceValue !== undefined) { - return vscode.ConfigurationTarget.Workspace - } - return undefined + inspected = commandListConfiguration().inspect(configKey) } catch { - // Same defensive posture as readCommandListScopes: configuration - // access failures mean we create no workspace layer. - return undefined + inspected = undefined + } + + if (inspected?.workspaceFolderValue !== undefined) { + return vscode.ConfigurationTarget.WorkspaceFolder + } + if (inspected?.workspaceValue !== undefined) { + return vscode.ConfigurationTarget.Workspace } + return undefined } function sanitizeCommandList(list: unknown): string[] { diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 3d2f6abc21..ab906d9c45 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -71,6 +71,8 @@ vi.mock("@roo-code/telemetry", () => ({ import type { ModelRecord } from "@roo-code/types" +import { Package } from "../../../shared/package" +import { makeWorkspaceConfiguration } from "../../../test-utils/vscode" import { webviewMessageHandler } from "../webviewMessageHandler" import type { ClineProvider } from "../ClineProvider" import { flushModels, getModels } from "../../../api/providers/fetchers/modelCache" @@ -1439,21 +1441,44 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { vi.clearAllMocks() }) + // The file-level vscode mock opens one workspace folder, so the command + // list configuration resolves to its uri. + const mockWorkspaceFolderUri = { fsPath: "/mock/workspace" } + // Mirrors the real VS Code configuration API: `inspect()` exposes the // per-scope values, and `update()` records the target it was written to. // Extension global state is left unset (getValue returns undefined), i.e. // the global layer starts empty. - function mockCommandConfiguration(inspected: { - globalValue?: string[] - workspaceValue?: string[] - workspaceFolderValue?: string[] - }) { + // + // `updateSequence` / `inspectSequence` let a failure test script what + // successive `update()` / `inspect()` calls return (each `update()` return + // is awaited by the handler, so a rejected promise fails that write), in + // call order; a plain value resolves like the default mock. + function mockCommandConfiguration( + inspected: { + globalValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + }, + updateSequence?: unknown[], + inspectSequence?: unknown[], + ) { const update = vi.fn() - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue([]), - inspect: vi.fn().mockReturnValue(inspected), - update, - } as never) + if (updateSequence) { + update.mockImplementation(() => updateSequence.shift()) + } + const inspect = vi.fn() + if (inspectSequence) { + inspect.mockImplementation(() => inspectSequence.shift()) + } else { + inspect.mockReturnValue(inspected) + } + // The shared structurally complete double keeps the configuration type + // correct; the scripted members are installed per test. + const config = makeWorkspaceConfiguration() + config.update = update + config.inspect = inspect + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) return update } @@ -1466,7 +1491,7 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { }) // "docker" stays at the workspace scope instead of leaking into the - // user's global settings (issue #1939 leg B). + // user's global settings (issue #1939: persist without scope leakage). expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) // Extension global state holds only the global layer, so the next save @@ -1482,6 +1507,10 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { updatedSettings: { deniedCommands: ["docker", "aws"] }, }) + // Read, target resolution, and write all run on the configuration + // scoped to the first workspace folder — the folder-target update is + // only legal on a resource-scoped configuration. + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, mockWorkspaceFolderUri) expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.WorkspaceFolder) expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) @@ -1517,6 +1546,106 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { expect(update).toHaveBeenCalledWith("allowedCommands", ["git"], vscode.ConfigurationTarget.Global) expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("allowedCommands", ["git"]) }) + + it("restores the previous workspace value when the global write fails", async () => { + // update() call order: workspace write succeeds, global write rejects. + // The raw folder-layer value differs from the sanitized post-split layer + // (and from the workspace-layer value), so the restore call is uniquely + // identifiable and provably reads the folder value for a folder target. + const update = mockCommandConfiguration( + { workspaceValue: ["npm"], workspaceFolderValue: ["pnpm docker", "docker"] }, + [undefined, Promise.reject(new Error("disk on fire"))], + ) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // A second key pins that only the failed command-list key is skipped. + updatedSettings: { deniedCommands: ["docker", "aws"], alwaysDenyUnapprovedCommands: true }, + }) + + // The capture, the failed write, and the restore all ran on the + // configuration scoped to the first workspace folder. + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, mockWorkspaceFolderUri) + // The raw previous folder value (entries this save dropped included) is + // written back at the target the failed save chose. + expect(update).toHaveBeenCalledWith( + "deniedCommands", + ["pnpm docker", "docker"], + vscode.ConfigurationTarget.WorkspaceFolder, + ) + expect(update).toHaveBeenCalledTimes(3) + // The failed key's extension global-state write is skipped... + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalledWith("deniedCommands", expect.anything()) + // ...while the other key of the same payload still persists. + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("alwaysDenyUnapprovedCommands", true) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to save deniedCommands: disk on fire") + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) + + it("does not write global or extension state when the workspace write fails", async () => { + // The first update() call is the workspace write; it rejects, so the + // global write must never be attempted for this key. The rejection is a + // plain string to pin the non-Error rendering of the failure message. + const update = mockCommandConfiguration({ workspaceValue: ["docker"] }, [Promise.reject("workspace locked")]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + // Nothing was changed yet, so there is nothing to restore: only the + // failed workspace write happened, and no compensation write followed. + expect(update).toHaveBeenCalledTimes(1) + expect(update).not.toHaveBeenCalledWith("deniedCommands", expect.anything(), vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to save deniedCommands: workspace locked") + }) + + it("saves both layers when the capture of the previous value finds no inspect result", async () => { + // inspect() call order: readCommandListScopes, resolveWorkspaceWriteTarget, + // then the compensation capture — only the capture call sees no result. + // A missing capture result must not abort the save: the writes proceed, + // and only a later failure could find nothing to restore. + const inspected = { workspaceValue: ["docker"] } + const update = mockCommandConfiguration(inspected, undefined, [inspected, inspected, undefined]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + expect(update).toHaveBeenCalledTimes(2) + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) + expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() + }) + + it("reports partial failure when the compensating restore also fails", async () => { + // update() call order: workspace write succeeds, global write rejects, + // restore write rejects too. The raw workspace value differs from the + // post-split layer so the restore call is uniquely identifiable. + const update = mockCommandConfiguration({ workspaceValue: ["docker", "old"] }, [ + undefined, + Promise.reject(new Error("disk on fire")), + Promise.reject(new Error("restore also failed")), + ]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + updatedSettings: { deniedCommands: ["docker", "aws"] }, + }) + + // The restore was attempted at the owning target even though it failed. + expect(update).toHaveBeenCalledWith("deniedCommands", ["docker", "old"], vscode.ConfigurationTarget.Workspace) + expect(update).toHaveBeenCalledTimes(3) + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to save deniedCommands: disk on fire. The workspace-scoped restore also failed: Error: restore also failed. Review the command lists in your user and workspace settings.", + ) + // No exception escaped the handler: the state post still ran. + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) }) describe("webviewMessageHandler - terminalProfile", () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 374427f966..92c651d381 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -60,6 +60,7 @@ import { import { changeLanguage, t } from "../../i18n" import { Package } from "../../shared/package" import { + commandListConfiguration, readCommandListScopes, resolveWorkspaceWriteTarget, splitCommandListScopes, @@ -775,17 +776,69 @@ export const webviewMessageHandler = async ( currentScopes.global, ) const workspaceTarget = resolveWorkspaceWriteTarget(key) - const config = vscode.workspace.getConfiguration(Package.name) - - // Write the workspace layer first, at the target that owns it. - // When no workspace layer exists the write is skipped entirely - // so saving never materializes one; when entries were deleted - // the `[]` write clears them at that scope. - if (workspaceTarget !== undefined) { - await config.update(key, workspaceWrite, workspaceTarget) - } + const config = commandListConfiguration() + + // The two configuration writes below are not atomic, so the raw + // previous workspace value is captured before the write: a failed + // global write rewinds the workspace layer to exactly what the + // configuration held beforehand, including the entries this save + // drops. The raw inspected value is used rather than the sanitized + // layer so the rewind reproduces the file contents instead of a + // cleaned-up approximation of them. + let previousWorkspaceValue: string[] | undefined + let workspaceWriteApplied = false + try { + // Capture the raw previous value for the compensation path + // before anything is written, from the target that owns the + // workspace layer, so the rewind reproduces exactly what that + // target held. With no inspect result there is nothing + // trustworthy to rewind to; the layer can only have been + // created or changed by this save's own writes, so the + // compensation write clears it. + const previousInspected = config.inspect(key) + if (previousInspected !== undefined) { + previousWorkspaceValue = + workspaceTarget === vscode.ConfigurationTarget.WorkspaceFolder + ? previousInspected.workspaceFolderValue + : previousInspected.workspaceValue + } - await config.update(key, globalWrite, vscode.ConfigurationTarget.Global) + // Write the workspace layer first, at the target that owns it. + // When no workspace layer exists the write is skipped entirely + // so saving never materializes one; when entries were deleted + // the `[]` write clears them at that scope. + if (workspaceTarget !== undefined) { + await config.update(key, workspaceWrite, workspaceTarget) + workspaceWriteApplied = true + } + + await config.update(key, globalWrite, vscode.ConfigurationTarget.Global) + } catch (error) { + const failure = error instanceof Error ? error.message : String(error) + + // A failed workspace write leaves nothing changed, so only a + // global-write failure has a changed workspace layer to rewind, + // and the rewind goes to the same target the write used. + if (workspaceWriteApplied) { + try { + await config.update(key, previousWorkspaceValue, workspaceTarget) + } catch (restoreError) { + // The rewind itself failed: the configuration keeps the + // partially written values and the extension global-state + // write is skipped, so stored state never reflects a save + // that did not complete. The user hears about both failures. + vscode.window.showErrorMessage( + `Failed to save ${key}: ${failure}. The workspace-scoped restore also failed: ${restoreError}. Review the command lists in your user and workspace settings.`, + ) + continue + } + } + + // Only this key's save is abandoned; the remaining keys of the + // payload persist normally. + vscode.window.showErrorMessage(`Failed to save ${key}: ${failure}`) + continue + } // Extension global state keeps only the global layer: // enforcement re-adds the workspace layer through diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts index 19c2623dfb..6467cb1ef8 100644 --- a/src/utils/__tests__/seedAllowedCommands.spec.ts +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -2,6 +2,7 @@ import * as vscode from "vscode" +import { makeWorkspaceConfiguration } from "../../test-utils/vscode" import { seedAllowedCommands } from "../seedAllowedCommands" vi.mock("vscode", () => ({ @@ -23,16 +24,17 @@ interface ConfigDouble { } function mockConfiguration({ inspect, effective = [], inspectThrows = false }: ConfigDouble = {}) { - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({ - get: vi.fn().mockReturnValue(effective), - inspect: vi.fn().mockImplementation(() => { - if (inspectThrows) { - throw new Error("configuration access failed") - } - return inspect - }), - update: vi.fn(), - } as unknown as vscode.WorkspaceConfiguration) + const config = makeWorkspaceConfiguration() + // The shared double is scripted after creation so the per-test fixture + // values need no type assertion at the construction site. + vi.mocked(config.get).mockReturnValue(effective) + vi.mocked(config.inspect).mockImplementation(() => { + if (inspectThrows) { + throw new Error("configuration access failed") + } + return inspect + }) + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) } function mockMemento(existing: unknown = undefined) { @@ -41,9 +43,12 @@ function mockMemento(existing: unknown = undefined) { store.set("allowedCommands", existing) } + // Structurally complete Memento double (keys/get/update), so the call + // sites need no type assertion. return { - get: vi.fn((key: string) => store.get(key)), - update: vi.fn(async (key: string, value: unknown) => { + keys: vi.fn((): readonly string[] => [...store.keys()]), + get: vi.fn().mockImplementation((key: string) => store.get(key)), + update: vi.fn().mockImplementation(async (key: string, value: unknown) => { store.set(key, value) }), } @@ -64,7 +69,7 @@ describe("seedAllowedCommands", () => { }) const globalState = mockMemento() - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).not.toHaveBeenCalled() expect(globalState.get("allowedCommands")).toBeUndefined() @@ -74,7 +79,7 @@ describe("seedAllowedCommands", () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) const globalState = mockMemento() - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", ["git"]) expect(globalState.get("allowedCommands")).toEqual(["git"]) @@ -84,17 +89,37 @@ describe("seedAllowedCommands", () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) const globalState = mockMemento(["npm"]) - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).not.toHaveBeenCalled() expect(globalState.get("allowedCommands")).toEqual(["npm"]) }) + it("inspects the global value under the extension's own section key", async () => { + // The seed must read the extension's own section; asking VS Code about + // a different (or empty) section key would silently never find the + // user's global list. + mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] } }) + // Structurally complete Memento double (keys/get/update), so the call + // site needs no type assertion. + const globalState: vscode.Memento = { + keys: () => [], + get: vi.fn(), + update: vi.fn(), + } + + await seedAllowedCommands(globalState) + + const config = vi.mocked(vscode.workspace.getConfiguration).mock.results[0].value + expect(config.inspect).toHaveBeenCalledWith("allowedCommands") + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", ["git"]) + }) + it("leaves global state untouched when there is no inspect result at all", async () => { mockConfiguration({ inspect: undefined, effective: ["git"] }) const globalState = mockMemento() - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).not.toHaveBeenCalled() }) @@ -103,7 +128,7 @@ describe("seedAllowedCommands", () => { mockConfiguration({ inspectThrows: true }) const globalState = mockMemento() - await expect(seedAllowedCommands(globalState as unknown as vscode.Memento)).resolves.toBeUndefined() + await expect(seedAllowedCommands(globalState)).resolves.toBeUndefined() expect(globalState.update).not.toHaveBeenCalled() }) @@ -113,7 +138,7 @@ describe("seedAllowedCommands", () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue } }) const globalState = mockMemento() - await seedAllowedCommands(globalState as unknown as vscode.Memento) + await seedAllowedCommands(globalState) expect(globalState.update).toHaveBeenCalledWith("allowedCommands", expect.arrayContaining(["git"])) expect(globalState.update.mock.calls[0][1]).not.toBe(globalValue) diff --git a/src/utils/seedAllowedCommands.ts b/src/utils/seedAllowedCommands.ts index 6a56173e47..3deab4897b 100644 --- a/src/utils/seedAllowedCommands.ts +++ b/src/utils/seedAllowedCommands.ts @@ -19,18 +19,22 @@ export async function seedAllowedCommands(globalState: vscode.Memento): Promise< return } - let globalValue: string[] | undefined + // Only the configuration lookup itself is guarded: it can fail (for example + // before the workspace is ready), and skipping the seed then only defers it + // to a later activation. The `?.globalValue` read below stays outside the + // guarded region so a genuine lookup failure stays distinguishable from an + // absent value rather than being swallowed as one. + let inspected: { globalValue?: string[] } | undefined try { - globalValue = vscode.workspace.getConfiguration(Package.name).inspect("allowedCommands")?.globalValue + inspected = vscode.workspace.getConfiguration(Package.name).inspect("allowedCommands") } catch { - // Configuration access can fail (for example before the workspace is - // ready); skipping the seed only defers it to a later activation. return } // An absent global-scope value leaves global state untouched: seeding the // package.json default or a workspace override would defeat the point of // scoping the seed to the user's own settings. + const globalValue = inspected?.globalValue if (!Array.isArray(globalValue)) { return } From 95ed76acf885726b6dd6a67c086dc6163da6621d Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Wed, 14 Oct 2026 21:32:29 +0200 Subject: [PATCH 3/5] fix: derive command-list save scope from a single settings read (#1939) Saving an allowed/denied command list now inspects the configuration once per key and derives the workspace layer, the rollback value and the write target from that one snapshot, instead of three reads that could disagree. When the inspection is unavailable the key is skipped with a visible error and nothing is written for it, which also removes a path where a workspace-only entry could be re-copied into user settings; the other keys of the same save still persist. Regression tests cover the single-read invariant and both abort cases. --- .../__tests__/commandScope.spec.ts | 25 ++++++ src/core/auto-approval/commandScope.ts | 72 +++++++++++----- .../__tests__/webviewMessageHandler.spec.ts | 82 ++++++++++++++----- src/core/webview/webviewMessageHandler.ts | 72 +++++++++------- 4 files changed, 180 insertions(+), 71 deletions(-) diff --git a/src/core/auto-approval/__tests__/commandScope.spec.ts b/src/core/auto-approval/__tests__/commandScope.spec.ts index 58c48fbac3..f09c4ecad2 100644 --- a/src/core/auto-approval/__tests__/commandScope.spec.ts +++ b/src/core/auto-approval/__tests__/commandScope.spec.ts @@ -140,6 +140,19 @@ describe("readCommandListScopes", () => { expect(scopes.workspace).toEqual([]) expect(scopes.merged).toEqual(["git"]) }) + + it("takes the workspace layer from a caller-supplied snapshot without reading the configuration", () => { + // The save path threads one captured inspection through every + // derivation, so the snapshot is authoritative: the configuration's own + // value must never be consulted a second time. + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["stale"] } }) + + const scopes = readCommandListScopes("deniedCommands", [], { workspaceValue: ["docker"] }) + + expect(scopes.workspace).toEqual(["docker"]) + expect(scopes.merged).toEqual(["docker"]) + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalled() + }) }) describe("splitCommandListScopes", () => { @@ -261,6 +274,18 @@ describe("resolveWorkspaceWriteTarget", () => { expect(resolveWorkspaceWriteTarget("deniedCommands")).toBeUndefined() }) + it("resolves the target from a caller-supplied snapshot without reading the configuration", () => { + // The snapshot alone decides the target, so a folder value the live + // configuration does not report still selects the folder target — + // keeping split, write, and rollback on one captured state. + mockConfiguration({ inspect: { key: "deniedCommands", workspaceValue: ["docker"] } }) + + expect(resolveWorkspaceWriteTarget("deniedCommands", { workspaceFolderValue: ["pnpm docker"] })).toBe( + vscode.ConfigurationTarget.WorkspaceFolder, + ) + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalled() + }) + it("resolves the target from the configuration scoped to the first workspace folder", () => { const folderA = makeUri("/mock/folder-a") const folderB = makeUri("/mock/folder-b") diff --git a/src/core/auto-approval/commandScope.ts b/src/core/auto-approval/commandScope.ts index 4778ac1f3e..d57e77bf7d 100644 --- a/src/core/auto-approval/commandScope.ts +++ b/src/core/auto-approval/commandScope.ts @@ -10,6 +10,17 @@ export interface CommandListScopes { merged: string[] } +/** + * The workspace-scoped layers of a command-list key as reported by + * `WorkspaceConfiguration.inspect()`. Capturing it once lets a save derive + * the read layers, the write target, and a rollback value from a single + * configuration state instead of three independent reads that could disagree. + */ +export interface CommandListInspection { + workspaceValue?: string[] + workspaceFolderValue?: string[] +} + /** * The configuration object for every command-list read and write, scoped to * the first workspace folder. Sharing one scoped access path keeps the two @@ -35,23 +46,33 @@ export function commandListConfiguration(): vscode.WorkspaceConfiguration { * * @param configKey - The extension configuration key to read. * @param globalList - The list persisted in extension global state. + * @param inspectionOverride - A snapshot captured by the caller before any + * write; when present it is used as-is and no fresh lookup happens, so a + * save's reads, target resolution, and rollback stay derived from one + * configuration state. Without it the lookup runs guarded (lenient): this is + * the enforcement/display path, where an unavailable workspace layer must + * degrade to empty rather than fail. * @returns The per-layer lists and their deduplicated set union. */ export function readCommandListScopes( configKey: CommandListConfigKey, globalList: string[] | undefined, + inspectionOverride?: CommandListInspection, ): CommandListScopes { const global = sanitizeCommandList(globalList) - // Only the configuration lookup itself is guarded: it can fail (for example - // before the workspace is ready), and an absent workspace layer must not - // break enforcement. The value reads below have no failure mode of their - // own, so the catch covers exactly the failure it exists to absorb. - let inspected: { workspaceValue?: string[]; workspaceFolderValue?: string[] } | undefined - try { - inspected = commandListConfiguration().inspect(configKey) - } catch { - inspected = undefined + let inspected: CommandListInspection | undefined = inspectionOverride + if (inspected === undefined) { + // Only the configuration lookup itself is guarded: it can fail (for + // example before the workspace is ready), and an absent workspace layer + // must not break enforcement. The value reads below have no failure mode + // of their own, so the catch covers exactly the failure it exists to + // absorb. + try { + inspected = commandListConfiguration().inspect(configKey) + } catch { + inspected = undefined + } } // The folder value wins over the workspace value, matching VS Code's @@ -116,11 +137,12 @@ export function splitCommandListScopes( /** * Picks the configuration target that owns the current workspace-scope layer - * for `configKey`, inspecting the same `commandListConfiguration()` object - * `readCommandListScopes` reads. Because that configuration is scoped to the - * first workspace folder, folder values resolve against that folder's - * settings; with no folder open the configuration is unscoped and reports no - * folder value, so no folder target can be selected. + * for `configKey`. Reading the layers runs through the same + * `commandListConfiguration()` object `readCommandListScopes` uses; because + * that configuration is scoped to the first workspace folder, folder values + * resolve against that folder's settings; with no folder open the + * configuration is unscoped and reports no folder value, so no folder target + * can be selected. * * - `workspaceFolderValue` set -> write at `WorkspaceFolder` scope. * - Otherwise `workspaceValue` set -> write at `Workspace` scope. @@ -128,19 +150,27 @@ export function splitCommandListScopes( * created (an empty workspace layer must not materialize in settings.json). * * @param configKey - The extension configuration key to inspect. + * @param inspectionOverride - A snapshot captured by the caller; when present + * it alone decides the target and no fresh lookup happens, keeping a save's + * split, write, and rollback on one configuration state. Without it the + * lookup is guarded leniently: a failure means no workspace layer exists. * @returns The workspace-scoped target to write, or `undefined` when no * workspace layer exists. */ export function resolveWorkspaceWriteTarget( configKey: CommandListConfigKey, + inspectionOverride?: CommandListInspection, ): vscode.ConfigurationTarget.WorkspaceFolder | vscode.ConfigurationTarget.Workspace | undefined { - // Same defensive posture as readCommandListScopes: only the configuration - // lookup itself may fail, and a failure means we create no workspace layer. - let inspected: { workspaceValue?: string[]; workspaceFolderValue?: string[] } | undefined - try { - inspected = commandListConfiguration().inspect(configKey) - } catch { - inspected = undefined + let inspected: CommandListInspection | undefined = inspectionOverride + if (inspected === undefined) { + // Same defensive posture as readCommandListScopes: only the + // configuration lookup itself may fail, and a failure means we create no + // workspace layer. + try { + inspected = commandListConfiguration().inspect(configKey) + } catch { + inspected = undefined + } } if (inspected?.workspaceFolderValue !== undefined) { diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index ab906d9c45..af26431b4c 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -1453,7 +1453,8 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // `updateSequence` / `inspectSequence` let a failure test script what // successive `update()` / `inspect()` calls return (each `update()` return // is awaited by the handler, so a rejected promise fails that write), in - // call order; a plain value resolves like the default mock. + // call order; a plain value resolves like the default mock, and an Error + // element makes that call throw. function mockCommandConfiguration( inspected: { globalValue?: string[] @@ -1469,7 +1470,13 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { } const inspect = vi.fn() if (inspectSequence) { - inspect.mockImplementation(() => inspectSequence.shift()) + inspect.mockImplementation(() => { + const next = inspectSequence.shift() + if (next instanceof Error) { + throw next + } + return next + }) } else { inspect.mockReturnValue(inspected) } @@ -1479,11 +1486,11 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { config.update = update config.inspect = inspect vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) - return update + return { update, inspect } } it("keeps workspace-scoped denied commands out of the global write when saving the merged list", async () => { - const update = mockCommandConfiguration({ workspaceValue: ["docker"] }) + const { update, inspect } = mockCommandConfiguration({ workspaceValue: ["docker"] }) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1497,10 +1504,14 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // Extension global state holds only the global layer, so the next save // cannot double-home the workspace entry. expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) + // Exactly one snapshot per key: the scope read, write target, and + // rollback all derive from the same inspect(), so a helper re-reading + // the configuration would raise this count and could diverge. + expect(inspect).toHaveBeenCalledTimes(1) }) it("writes the workspace-folder value at the folder target in multi-root workspaces", async () => { - const update = mockCommandConfiguration({ workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }) + const { update } = mockCommandConfiguration({ workspaceValue: ["npm"], workspaceFolderValue: ["docker"] }) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1517,7 +1528,7 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { }) it("clears a deleted workspace entry at the workspace scope without adding it to global", async () => { - const update = mockCommandConfiguration({ workspaceValue: ["docker"] }) + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1533,7 +1544,7 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { }) it("does not create a workspace layer when none exists", async () => { - const update = mockCommandConfiguration({}) + const { update } = mockCommandConfiguration({}) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1552,7 +1563,7 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // The raw folder-layer value differs from the sanitized post-split layer // (and from the workspace-layer value), so the restore call is uniquely // identifiable and provably reads the folder value for a folder target. - const update = mockCommandConfiguration( + const { update } = mockCommandConfiguration( { workspaceValue: ["npm"], workspaceFolderValue: ["pnpm docker", "docker"] }, [undefined, Promise.reject(new Error("disk on fire"))], ) @@ -1586,7 +1597,9 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { // The first update() call is the workspace write; it rejects, so the // global write must never be attempted for this key. The rejection is a // plain string to pin the non-Error rendering of the failure message. - const update = mockCommandConfiguration({ workspaceValue: ["docker"] }, [Promise.reject("workspace locked")]) + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }, [ + Promise.reject("workspace locked"), + ]) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", @@ -1601,31 +1614,56 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to save deniedCommands: workspace locked") }) - it("saves both layers when the capture of the previous value finds no inspect result", async () => { - // inspect() call order: readCommandListScopes, resolveWorkspaceWriteTarget, - // then the compensation capture — only the capture call sees no result. - // A missing capture result must not abort the save: the writes proceed, - // and only a later failure could find nothing to restore. - const inspected = { workspaceValue: ["docker"] } - const update = mockCommandConfiguration(inspected, undefined, [inspected, inspected, undefined]) + it("aborts the key without writing when the configuration inspection has no result", async () => { + // The save derives scopes, write target, and rollback from one inspect() + // snapshot, so a missing snapshot means the workspace layer is unknown — + // proceeding would classify the retained workspace entry "docker" as new + // and re-copy it into global settings (issue #1939's leak). The key is + // aborted instead: no configuration write, no extension global state. + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }, undefined, [undefined]) await webviewMessageHandler(mockClineProvider, { type: "updateSettings", updatedSettings: { deniedCommands: ["docker", "aws"] }, }) - expect(update).toHaveBeenCalledTimes(2) - expect(update).toHaveBeenCalledWith("deniedCommands", ["docker"], vscode.ConfigurationTarget.Workspace) - expect(update).toHaveBeenCalledWith("deniedCommands", ["aws"], vscode.ConfigurationTarget.Global) - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["aws"]) - expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() + expect(update).not.toHaveBeenCalled() + // Leak pin: the workspace entry must not land in the global layer. + expect(update).not.toHaveBeenCalledWith("deniedCommands", ["docker", "aws"], vscode.ConfigurationTarget.Global) + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to save deniedCommands: the configuration value could not be read", + ) + // No exception escaped the handler: the state post still ran. + expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() + }) + + it("aborts the command-list key when the inspection throws, while other keys of the payload still save", async () => { + const { update } = mockCommandConfiguration({ workspaceValue: ["docker"] }, undefined, [ + new Error("configuration unavailable"), + ]) + + await webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // A second key pins that only the aborted command-list key is skipped. + updatedSettings: { deniedCommands: ["docker", "aws"], alwaysDenyUnapprovedCommands: true }, + }) + + // The aborted key wrote nothing at any scope... + expect(update).not.toHaveBeenCalled() + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalledWith("deniedCommands", expect.anything()) + // ...while the other key of the same payload still persists. + expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("alwaysDenyUnapprovedCommands", true) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to save deniedCommands: configuration unavailable", + ) }) it("reports partial failure when the compensating restore also fails", async () => { // update() call order: workspace write succeeds, global write rejects, // restore write rejects too. The raw workspace value differs from the // post-split layer so the restore call is uniquely identifiable. - const update = mockCommandConfiguration({ workspaceValue: ["docker", "old"] }, [ + const { update } = mockCommandConfiguration({ workspaceValue: ["docker", "old"] }, [ undefined, Promise.reject(new Error("disk on fire")), Promise.reject(new Error("restore also failed")), diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 92c651d381..6d198f57fe 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -60,6 +60,7 @@ import { import { changeLanguage, t } from "../../i18n" import { Package } from "../../shared/package" import { + type CommandListInspection, commandListConfiguration, readCommandListScopes, resolveWorkspaceWriteTarget, @@ -766,43 +767,58 @@ export const webviewMessageHandler = async ( // The webview always sends the full merged list (it displays // the union of the global-state and workspace-scope layers). - // Read both layers BEFORE any write and split the incoming - // list by origin scope, so saving the dialog cannot flatten - // workspace-scoped entries into the user's global settings. - const currentScopes = readCommandListScopes(key, getGlobalState(key)) + // Splitting that list by origin scope needs the workspace + // layer, the write target, and the rollback value to describe + // the SAME configuration state, so they are all derived from + // one inspect() snapshot taken before any write. Without a + // snapshot the key's save is aborted rather than guessed at: + // reading the workspace layer as empty would classify a + // workspace-scoped entry as brand new and re-copy it into + // global settings. + // Only the snapshot read is guarded: a failing + // commandListConfiguration() lookup propagates instead of being + // downgraded to a per-key save failure. + const config = commandListConfiguration() + let inspection: CommandListInspection | undefined + let inspectionFailure: string | undefined + try { + inspection = config.inspect(key) + } catch (error) { + inspectionFailure = error instanceof Error ? error.message : String(error) + } + if (inspection === undefined) { + // No snapshot: the workspace layer could only be guessed + // at, and a guessed-empty layer re-copies workspace entries + // into global settings, so this key's save is abandoned + // while the remaining keys of the payload persist normally. + vscode.window.showErrorMessage( + `Failed to save ${key}: ${inspectionFailure ?? "the configuration value could not be read"}`, + ) + continue + } + + const currentScopes = readCommandListScopes(key, getGlobalState(key), inspection) const { globalWrite, workspaceWrite } = splitCommandListScopes( incoming, currentScopes.workspace, currentScopes.global, ) - const workspaceTarget = resolveWorkspaceWriteTarget(key) - const config = commandListConfiguration() + const workspaceTarget = resolveWorkspaceWriteTarget(key, inspection) // The two configuration writes below are not atomic, so the raw - // previous workspace value is captured before the write: a failed - // global write rewinds the workspace layer to exactly what the - // configuration held beforehand, including the entries this save - // drops. The raw inspected value is used rather than the sanitized - // layer so the rewind reproduces the file contents instead of a - // cleaned-up approximation of them. - let previousWorkspaceValue: string[] | undefined + // previous workspace value comes from the same snapshot the split + // used, taken from the target that owns the workspace layer: a + // failed global write rewinds that target to exactly what it held + // beforehand, including the entries this save drops. The raw + // inspected value is used rather than the sanitized layer so the + // rewind reproduces the file contents instead of a cleaned-up + // approximation of them. + const previousWorkspaceValue = + workspaceTarget === vscode.ConfigurationTarget.WorkspaceFolder + ? inspection.workspaceFolderValue + : inspection.workspaceValue let workspaceWriteApplied = false try { - // Capture the raw previous value for the compensation path - // before anything is written, from the target that owns the - // workspace layer, so the rewind reproduces exactly what that - // target held. With no inspect result there is nothing - // trustworthy to rewind to; the layer can only have been - // created or changed by this save's own writes, so the - // compensation write clears it. - const previousInspected = config.inspect(key) - if (previousInspected !== undefined) { - previousWorkspaceValue = - workspaceTarget === vscode.ConfigurationTarget.WorkspaceFolder - ? previousInspected.workspaceFolderValue - : previousInspected.workspaceValue - } - // Write the workspace layer first, at the target that owns it. // When no workspace layer exists the write is skipped entirely // so saving never materializes one; when entries were deleted From 7c294cc9ac5d1573d50418eef2eae94350b33549 Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Thu, 15 Oct 2026 00:16:04 +0200 Subject: [PATCH 4/5] fix: restore default command allowlist on fresh installs (#1939) Fresh installs keep the built-in git command defaults again: with no user, workspace, or folder value set, the declared default list is seeded into global state. A workspace or folder override still wins and nothing is written over it. Scope layers are read through the folder-aware settings lookup shared with the command-list paths. --- src/__tests__/extension.spec.ts | 4 + .../__tests__/seedAllowedCommands.spec.ts | 152 ++++++++++++++++-- src/utils/seedAllowedCommands.ts | 55 +++++-- 3 files changed, 181 insertions(+), 30 deletions(-) diff --git a/src/__tests__/extension.spec.ts b/src/__tests__/extension.spec.ts index 3b52363763..6a2e2596dd 100644 --- a/src/__tests__/extension.spec.ts +++ b/src/__tests__/extension.spec.ts @@ -420,6 +420,10 @@ describe("extension.ts", () => { ), inspect: vi.fn().mockReturnValue({ key: "allowedCommands", + // Real inspect() always reports the declared package default; + // including it here lets the workspace-override test prove the + // default stays unseeded beside an override, not just absent. + defaultValue: ["git log", "git diff", "git show"], globalValue, workspaceValue, }), diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts index 6467cb1ef8..a62ce469a9 100644 --- a/src/utils/__tests__/seedAllowedCommands.spec.ts +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -2,11 +2,22 @@ import * as vscode from "vscode" -import { makeWorkspaceConfiguration } from "../../test-utils/vscode" +import { Package } from "../../shared/package" +import { makeUri, makeWorkspaceConfiguration } from "../../test-utils/vscode" import { seedAllowedCommands } from "../seedAllowedCommands" +// Scriptable stand-in for `vscode.workspace.workspaceFolders`: the module +// mock's getter reads through this object, so a test can change the open +// folders without re-mocking the module. +const workspaceState = vi.hoisted(() => ({ + workspaceFolders: undefined as { uri: unknown }[] | undefined, +})) + vi.mock("vscode", () => ({ workspace: { + get workspaceFolders() { + return workspaceState.workspaceFolders + }, getConfiguration: vi.fn(), }, })) @@ -24,17 +35,31 @@ interface ConfigDouble { } function mockConfiguration({ inspect, effective = [], inspectThrows = false }: ConfigDouble = {}) { - const config = makeWorkspaceConfiguration() - // The shared double is scripted after creation so the per-test fixture - // values need no type assertion at the construction site. - vi.mocked(config.get).mockReturnValue(effective) - vi.mocked(config.inspect).mockImplementation(() => { - if (inspectThrows) { - throw new Error("configuration access failed") - } - return inspect - }) - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(config) + // Real VS Code reports `workspaceFolderValue` only for a configuration + // looked up against a resource; an unscoped lookup reports no folder + // layer no matter what any folder's settings contain. Scripting that + // split per call is what lets these tests tell a scoped lookup apart + // from an unscoped one. + vi.mocked(vscode.workspace.getConfiguration).mockImplementation( + (_section?: string, scope?: vscode.ConfigurationScope | null) => { + const config = makeWorkspaceConfiguration() + // The shared double is scripted per call so the per-test fixture + // values need no type assertion at the construction site. + vi.mocked(config.get).mockReturnValue(effective) + vi.mocked(config.inspect).mockImplementation(() => { + if (inspectThrows) { + throw new Error("configuration access failed") + } + if (inspect === undefined) { + return undefined + } + // No resource to resolve against => no folder layer, matching + // the real contract for unscoped (and explicit-null) lookups. + return scope === undefined || scope === null ? { ...inspect, workspaceFolderValue: undefined } : inspect + }) + return config + }, + ) } function mockMemento(existing: unknown = undefined) { @@ -57,14 +82,23 @@ function mockMemento(existing: unknown = undefined) { describe("seedAllowedCommands", () => { beforeEach(() => { vi.clearAllMocks() + workspaceState.workspaceFolders = undefined }) it("leaves global state unset when only a workspace-scoped value exists", async () => { // A real VS Code get() folds the workspace override into the effective // value, so this is the leak scenario: reading get() would seed - // ["docker"] (from .vscode/settings.json) into global state. + // ["docker"] (from .vscode/settings.json) into global state. The + // defaultValue mirrors real VS Code, where the declared default is + // always present: the workspace override replaced it deliberately, so + // re-granting it from global state is also forbidden. mockConfiguration({ - inspect: { key: "allowedCommands", globalValue: undefined, workspaceValue: ["docker"] }, + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + globalValue: undefined, + workspaceValue: ["docker"], + }, effective: ["docker"], }) const globalState = mockMemento() @@ -75,6 +109,92 @@ describe("seedAllowedCommands", () => { expect(globalState.get("allowedCommands")).toBeUndefined() }) + it("leaves global state unset when only a workspace-folder-scoped value exists", async () => { + // A folder override is visible only to a lookup resolved against that + // folder's resource, so this fixture opens a folder and scripts the + // folder layer (which the resource-aware double reports only for the + // scoped call). An unscoped seed would read "no value anywhere" here + // and re-grant the default the folder removed — this test fails on + // that regression. + const folderA = makeUri("/mock/folder-a") + workspaceState.workspaceFolders = [{ uri: folderA }] + mockConfiguration({ + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + globalValue: undefined, + workspaceFolderValue: ["docker"], + }, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + // The folder layer only reaches the seed through a resource-scoped + // lookup, so the seed must request one (same pin as the save path). + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderA) + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toBeUndefined() + }) + + it("resolves the folder layer against the first workspace folder only", async () => { + // Reading any other folder's configuration would silently decide the + // seed from the wrong folder's settings file. + const folderA = makeUri("/mock/folder-a") + const folderB = makeUri("/mock/folder-b") + workspaceState.workspaceFolders = [{ uri: folderA }, { uri: folderB }] + mockConfiguration({ + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + workspaceFolderValue: ["docker"], + }, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, folderA) + expect(vscode.workspace.getConfiguration).not.toHaveBeenCalledWith(Package.name, folderB) + expect(globalState.update).not.toHaveBeenCalled() + }) + + it("treats an explicitly empty workspace value as an override that suppresses the default", async () => { + // Presence is checked with `undefined`, not truthiness: a workspace that + // set the list to [] opted out of the default as deliberately as one + // that set ["docker"]. + mockConfiguration({ + inspect: { + key: "allowedCommands", + defaultValue: ["git log", "git diff", "git show"], + globalValue: undefined, + workspaceValue: [], + }, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toBeUndefined() + }) + + it("seeds the declared default on a fresh install with no value set at any scope", async () => { + const defaultValue = ["git log", "git diff", "git show"] + mockConfiguration({ + inspect: { key: "allowedCommands", defaultValue }, + effective: defaultValue, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", defaultValue) + // Copy, like the global-value path: global state must not share a + // reference with the configuration object. + expect(globalState.update.mock.calls[0][1]).not.toBe(defaultValue) + }) + it("seeds from the global-scope value when global state is empty", async () => { mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] }, effective: ["git"] }) const globalState = mockMemento() @@ -98,7 +218,8 @@ describe("seedAllowedCommands", () => { it("inspects the global value under the extension's own section key", async () => { // The seed must read the extension's own section; asking VS Code about // a different (or empty) section key would silently never find the - // user's global list. + // user's global list. With no folder open there is no resource to + // scope against, so the explicit unscoped call is (section, undefined). mockConfiguration({ inspect: { key: "allowedCommands", globalValue: ["git"] } }) // Structurally complete Memento double (keys/get/update), so the call // site needs no type assertion. @@ -110,6 +231,7 @@ describe("seedAllowedCommands", () => { await seedAllowedCommands(globalState) + expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith(Package.name, undefined) const config = vi.mocked(vscode.workspace.getConfiguration).mock.results[0].value expect(config.inspect).toHaveBeenCalledWith("allowedCommands") expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", ["git"]) diff --git a/src/utils/seedAllowedCommands.ts b/src/utils/seedAllowedCommands.ts index 3deab4897b..1afa0bbfcb 100644 --- a/src/utils/seedAllowedCommands.ts +++ b/src/utils/seedAllowedCommands.ts @@ -1,16 +1,30 @@ import * as vscode from "vscode" -import { Package } from "../shared/package" +import { commandListConfiguration } from "../core/auto-approval/commandScope" /** * Seeds the extension's global-state allowlist from the `allowedCommands` * setting, but only when global state holds no list yet (first run). * - * The seed reads the global-scope value via `inspect()` rather than the - * effective `get()` on purpose: `get()` folds in workspace-scoped overrides, - * so a project's `.vscode/settings.json` would otherwise be copied - * permanently into global state the first time the extension activates inside - * that project — a leak that survives closing the project. + * The seed reads scoped values via `inspect()` rather than the effective + * `get()` on purpose: `get()` folds in workspace-scoped overrides, so a + * project's `.vscode/settings.json` would otherwise be copied permanently + * into global state the first time the extension activates inside that + * project — a leak that survives closing the project. The lookup goes + * through `commandListConfiguration()`, the folder-scoped accessor shared + * with the command-list read and write paths: only a configuration resolved + * against a resource reports `workspaceFolderValue` at all, so the folder + * layer is visible here exactly where enforcement can see it. In a + * multi-root workspace only the first folder is in scope; with no folder + * open the configuration is unscoped and reports no folder value. + * + * Selection: the global-scope value wins. With nothing set at any scope (a + * fresh install) the declared package default is seeded, because the + * enforcement path reads only global state plus workspace scopes and never + * falls back to that default on its own. A workspace-scoped value is an + * explicit choice and suppresses the default: array settings replace across + * scopes, so seeding it would re-grant the auto-approvals the override + * removed. * * @param globalState - The extension global-state memento to seed. */ @@ -21,24 +35,35 @@ export async function seedAllowedCommands(globalState: vscode.Memento): Promise< // Only the configuration lookup itself is guarded: it can fail (for example // before the workspace is ready), and skipping the seed then only defers it - // to a later activation. The `?.globalValue` read below stays outside the + // to a later activation. The `inspected` reads below stay outside the // guarded region so a genuine lookup failure stays distinguishable from an // absent value rather than being swallowed as one. - let inspected: { globalValue?: string[] } | undefined + let inspected: + | { + globalValue?: string[] + defaultValue?: string[] + workspaceValue?: string[] + workspaceFolderValue?: string[] + } + | undefined try { - inspected = vscode.workspace.getConfiguration(Package.name).inspect("allowedCommands") + inspected = commandListConfiguration().inspect("allowedCommands") } catch { return } - // An absent global-scope value leaves global state untouched: seeding the - // package.json default or a workspace override would defeat the point of - // scoping the seed to the user's own settings. - const globalValue = inspected?.globalValue - if (!Array.isArray(globalValue)) { + // The `undefined` checks, not truthiness: an explicitly empty array still + // counts as an override. + const seedValue = + inspected?.globalValue ?? + (inspected?.workspaceValue === undefined && inspected?.workspaceFolderValue === undefined + ? inspected?.defaultValue + : undefined) + + if (!Array.isArray(seedValue)) { return } // Copy so global state never shares a reference with the configuration object. - await globalState.update("allowedCommands", [...globalValue]) + await globalState.update("allowedCommands", [...seedValue]) } From 0cf1628953271db530d2d75fbaf59bd5b7ac11e7 Mon Sep 17 00:00:00 2001 From: Franz Daubner Date: Thu, 15 Oct 2026 11:11:32 +0200 Subject: [PATCH 5/5] test: pin command-list seed and settings-lookup edge cases (#1939) Add three tests, no production change: an explicitly empty seeded allowlist is left untouched instead of re-seeded, a settings lookup that cannot return aborts the whole update-settings message instead of partially applying it, and a workspace override added after a fresh-install seed composes as a union with the seeded defaults. --- .../__tests__/webviewMessageHandler.spec.ts | 26 ++++++++++ .../__tests__/seedAllowedCommands.spec.ts | 51 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index af26431b4c..1a3ffaa3ae 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -1659,6 +1659,32 @@ describe("webviewMessageHandler - command list scope-aware persistence", () => { ) }) + it("propagates a configuration lookup failure instead of degrading it to a per-key abort", async () => { + // The commandListConfiguration() lookup sits outside the guarded + // snapshot read on purpose: a lookup that cannot return a configuration + // object at all is a different failure class than an unreadable value, + // so it aborts the entire message rather than the one key. + vi.mocked(vscode.workspace.getConfiguration).mockImplementation(() => { + throw new Error("workspace not ready") + }) + + await expect( + webviewMessageHandler(mockClineProvider, { + type: "updateSettings", + // A second key pins that the whole message aborts, not just the + // command-list key: with the guarded per-key failure paths this + // key would still persist. + updatedSettings: { deniedCommands: ["docker", "aws"], alwaysDenyUnapprovedCommands: true }, + }), + ).rejects.toThrow("workspace not ready") + + // Nothing persisted for any key, no per-key error handling ran, and the + // state post was skipped — the rejection surfaced to the caller instead. + expect(mockClineProvider.contextProxy.setValue).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() + expect(mockClineProvider.postStateToWebview).not.toHaveBeenCalled() + }) + it("reports partial failure when the compensating restore also fails", async () => { // update() call order: workspace write succeeds, global write rejects, // restore write rejects too. The raw workspace value differs from the diff --git a/src/utils/__tests__/seedAllowedCommands.spec.ts b/src/utils/__tests__/seedAllowedCommands.spec.ts index a62ce469a9..50c1dd5fc6 100644 --- a/src/utils/__tests__/seedAllowedCommands.spec.ts +++ b/src/utils/__tests__/seedAllowedCommands.spec.ts @@ -2,6 +2,7 @@ import * as vscode from "vscode" +import { readCommandListScopes } from "../../core/auto-approval/commandScope" import { Package } from "../../shared/package" import { makeUri, makeWorkspaceConfiguration } from "../../test-utils/vscode" import { seedAllowedCommands } from "../seedAllowedCommands" @@ -215,6 +216,56 @@ describe("seedAllowedCommands", () => { expect(globalState.get("allowedCommands")).toEqual(["npm"]) }) + it("leaves an explicitly empty global-state list untouched", async () => { + // The guard treats presence, not emptiness: saving [] is a deliberate + // choice to auto-approve nothing, and re-seeding the package default + // would silently re-grant the approvals the user just revoked. + mockConfiguration({ inspect: { key: "allowedCommands", defaultValue: ["git log", "git diff", "git show"] } }) + const globalState = mockMemento([]) + + await seedAllowedCommands(globalState) + + expect(globalState.update).not.toHaveBeenCalled() + expect(globalState.get("allowedCommands")).toEqual([]) + }) + + it("composes a later workspace override as a union with the seeded defaults", async () => { + // Full schedule: a fresh activation seeds the declared default, then a + // workspace override appears in a later activation. The seed + // early-returns because global state already holds a list, so the + // override is never persisted — and the enforcement read composes the + // two layers as a set union. Neither list replaces the other; the + // longest-prefix matcher decides which entry governs a given command. + const defaultValue = ["git log", "git diff", "git show"] + mockConfiguration({ + inspect: { key: "allowedCommands", defaultValue }, + effective: defaultValue, + }) + const globalState = mockMemento() + + await seedAllowedCommands(globalState) + expect(globalState.update).toHaveBeenCalledExactlyOnceWith("allowedCommands", defaultValue) + + // A later activation in a workspace whose settings file sets the list: + // the guard early-returns before any inspection, so no second write. + mockConfiguration({ + inspect: { key: "allowedCommands", defaultValue, workspaceValue: ["docker ps"] }, + effective: ["docker ps"], + }) + + await seedAllowedCommands(globalState) + expect(globalState.update).toHaveBeenCalledTimes(1) + expect(globalState.get("allowedCommands")).toEqual(defaultValue) + + // The enforcement view (ClineProvider.getState) keeps both layers and + // merges them: the override adds entries, it does not replace the + // seeded defaults, and vice versa. + const scopes = readCommandListScopes("allowedCommands", globalState.get("allowedCommands")) + expect(scopes.global).toEqual(defaultValue) + expect(scopes.workspace).toEqual(["docker ps"]) + expect(scopes.merged).toEqual([...defaultValue, "docker ps"]) + }) + it("inspects the global value under the extension's own section key", async () => { // The seed must read the extension's own section; asking VS Code about // a different (or empty) section key would silently never find the