diff --git a/docs/cli-quality.md b/docs/cli-quality.md index 1e5e617cc9..f8d6943e4b 100644 --- a/docs/cli-quality.md +++ b/docs/cli-quality.md @@ -30,6 +30,7 @@ cancelled; it cannot become a successful skipped check after a dependency fails. | --- | --- | | Isolated suites | Every tracked or new non-ignored `.test.ts`, `.test.tsx`, `.test.js`, `.test.mjs` file, including SDK, extension and desktop contracts | | Runtime contracts | Late/partial/zero usage, retries, split messages, replay, background progress, cancellation, stale executions, persisted metadata | +| Windows credentials | Real DPAPI encrypt/decrypt, no native calls for missing files, one decryption for 20 agent reads, fresh classified reads, external refresh/logout and corrupt-record errors on Node 22 and 24 | | Stress | Seeded 10,000-event streams, 1/2/8/20 agents, bounded SDK progress queues with start and final events preserved | | Rendered terminal | Compare changing Ink output against a Unicode-aware VT screen, with bounded group height and input retained | | Installed CLI | Real npm tarball, actual agent/query/auth/parser paths, local HTTP/SSE server, PTY input and resize; SDK streaming JSON through ordinary stdin/stdout pipes | diff --git a/package.json b/package.json index 1a456a61d0..2b6e18fa51 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@verboo/code", - "version": "0.15.21", + "version": "0.15.22", "description": "Verboo Code — coding agent for the Verboo platform", "type": "module", "bin": { diff --git a/scripts/check-cli-quality.ts b/scripts/check-cli-quality.ts index 65d5c3dc12..9184af6f80 100644 --- a/scripts/check-cli-quality.ts +++ b/scripts/check-cli-quality.ts @@ -19,6 +19,10 @@ if (!terminalOnly) { if (!process.argv.includes('--suite')) { run([node, 'scripts/setup-pty.mjs']) run([node, 'scripts/prepare-cli-package.mjs', '--install-only']) + if (process.platform === 'win32') { + run([process.execPath, 'build', 'src/utils/secureStorage/windowsCredentialStorage.ts', '--target', 'node', '--outfile', '.artifacts/windows-credentials/secure-storage.mjs']) + run([node, '--test', 'scripts/e2e/windows-credentials.mjs']) + } run([node, '--test', '--test-name-pattern=installed CLI: 1 agents, 80x24, fullscreen=false', 'scripts/e2e/cli.e2e.mjs']) run([node, '--test', '--test-concurrency=1', 'scripts/e2e/cli.e2e.mjs']) } diff --git a/scripts/e2e/windows-credentials.mjs b/scripts/e2e/windows-credentials.mjs new file mode 100644 index 0000000000..4e18f83472 --- /dev/null +++ b/scripts/e2e/windows-credentials.mjs @@ -0,0 +1,99 @@ +import assert from 'node:assert/strict' +import { after, before, describe, test } from 'node:test' +import childProcess from 'node:child_process' +import { syncBuiltinESMExports } from 'node:module' +import { mkdirSync, mkdtempSync, readFileSync, readdirSync, writeFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { pathToFileURL } from 'node:url' + +// Compile the production module for the same Node runtime as the installed CLI +// matrix. These checks use real PowerShell/DPAPI and an isolated fake record. +describe('Windows DPAPI credential integration', { skip: process.platform !== 'win32' }, () => { + const originalSpawnSync = childProcess.spawnSync + const originalEnv = { ...process.env } + const moduleUrl = pathToFileURL(resolve('.artifacts/windows-credentials/secure-storage.mjs')).href + const initial = { verbooInstallationId: 'fixture-installation', mcpOAuth: { fixture: { accessToken: 'fixture-token', expiresAt: 123456789, serverName: 'fixture-ação-東京', serverUrl: 'https://example.invalid' } } } + const updated = { ...initial, verbooInstallationId: 'refreshed-installation' } + let storage + let directory + let powershellCalls = 0 + + before(async () => { + mkdirSync(resolve('.artifacts/pty'), { recursive: true }) + directory = mkdtempSync(resolve('.artifacts/pty/native-credentials-')) + process.env.VERBOO_CONFIG_DIR = directory + process.env.VERBOO_ENABLE_LEGACY_WINDOWS_PASSWORDVAULT = '0' + process.env.OPENCLAUDE_ENABLE_LEGACY_WINDOWS_PASSWORDVAULT = '0' + childProcess.spawnSync = (...args) => { + if (String(args[0]).toLowerCase().endsWith('powershell.exe')) powershellCalls++ + return originalSpawnSync(...args) + } + syncBuiltinESMExports() + storage = (await import(moduleUrl)).windowsCredentialStorage + }) + + after(() => { + childProcess.spawnSync = originalSpawnSync + syncBuiltinESMExports() + for (const key of ['VERBOO_CONFIG_DIR', 'VERBOO_ENABLE_LEGACY_WINDOWS_PASSWORDVAULT', 'OPENCLAUDE_ENABLE_LEGACY_WINDOWS_PASSWORDVAULT']) { + if (originalEnv[key] === undefined) delete process.env[key] + else process.env[key] = originalEnv[key] + } + if (directory) writeFileSync(join(directory, 'native-checks.json'), JSON.stringify({ node: process.version, powershellCalls }, null, 2)) + }) + + function mutateInAnotherProcess(operation, value) { + const script = `import { readFileSync } from 'node:fs'; const { windowsCredentialStorage: storage } = await import(${JSON.stringify(moduleUrl)}); const result = ${operation === 'write' ? "storage.update(JSON.parse(readFileSync(0, 'utf8'))).success" : 'storage.delete()'}; if (!result) process.exit(1);` + const result = originalSpawnSync(process.execPath, ['--input-type=module', '-e', script], { env: process.env, input: value ? JSON.stringify(value) : '', encoding: 'utf8', timeout: 20_000, windowsHide: true }) + assert.ifError(result.error) + assert.equal(result.status, 0, result.stderr) + } + + test('missing credentials require no native process for 20 agents', async () => { + assert.deepEqual(storage.readResult(), { kind: 'missing' }) + const records = await Promise.all(Array.from({ length: 20 }, () => storage.readAsync())) + assert.deepEqual(records, Array(20).fill(null)) + assert.equal(powershellCalls, 0) + }) + + test('real encrypted credentials round-trip and decrypt once for 20 agents', async () => { + assert.deepEqual(storage.update(initial), { success: true }) + const files = readdirSync(directory).filter(file => file.endsWith('.secure.dpapi')) + assert.equal(files.length, 1) + const encrypted = readFileSync(join(directory, files[0]), 'utf8') + assert.match(encrypted, /^[A-Za-z\d+/]+={0,2}$/) + assert.ok(!encrypted.includes('fixture-token')) + const beforeReads = powershellCalls + const records = await Promise.all(Array.from({ length: 20 }, () => storage.readAsync())) + assert.deepEqual(records, Array(20).fill(initial)) + assert.equal(powershellCalls - beforeReads, 1) + }) + + test('classified reads bypass the decrypted cache', () => { + const beforeRead = powershellCalls + assert.deepEqual(storage.readResult(), { kind: 'ok', data: initial }) + assert.equal(powershellCalls - beforeRead, 1) + assert.deepEqual(storage.read(), initial) + }) + + test('another process can refresh and remove credentials without stale reads', () => { + mutateInAnotherProcess('write', updated) + const beforeRead = powershellCalls + assert.deepEqual(storage.read(), updated) + assert.equal(powershellCalls - beforeRead, 1) + mutateInAnotherProcess('delete') + assert.equal(storage.read(), null) + assert.deepEqual(storage.readResult(), { kind: 'missing' }) + assert.equal(powershellCalls - beforeRead, 1) + }) + + test('corrupt encrypted data remains an error and does not reuse the cache', () => { + assert.deepEqual(storage.update(initial), { success: true }) + assert.deepEqual(storage.read(), initial) + const file = readdirSync(directory).find(file => file.endsWith('.secure.dpapi')) + writeFileSync(join(directory, file), 'invalid encrypted data') + assert.equal(storage.readResult().kind, 'error') + assert.equal(storage.read(), null) + assert.equal(storage.delete(), true) + }) +}) diff --git a/src/utils/secureStorage/platformStorage.test.ts b/src/utils/secureStorage/platformStorage.test.ts index 4b4d21cb1b..9be7e29051 100644 --- a/src/utils/secureStorage/platformStorage.test.ts +++ b/src/utils/secureStorage/platformStorage.test.ts @@ -1,5 +1,6 @@ import { expect, test, mock, describe, beforeEach, afterEach } from "bun:test"; +import * as fs from 'node:fs'; import { linuxSecretStorage } from "./linuxSecretStorage.js"; import { windowsCredentialStorage } from "./windowsCredentialStorage.js"; import { macOsKeychainStorage } from "./macOsKeychainStorage.js"; @@ -22,12 +23,27 @@ mock.module("execa", () => ({ execaSync: mockExecaSync, })); +const realReadFileSync = fs.readFileSync; +let encryptedFile: string | Error = 'encrypted-fixture'; +let fileGeneration = 0; +mock.module('node:fs', () => ({ + ...fs, + readFileSync: (...args: Parameters) => { + if (String(args[0]).endsWith('.secure.dpapi')) { + if (encryptedFile instanceof Error) throw encryptedFile; + return encryptedFile; + } + return realReadFileSync(...args); + }, +})); + describe("Secure Storage Platform Implementations", () => { const originalEnv = process.env; beforeEach(async () => { await acquireSharedMutationLock("platformStorage.test.ts"); process.env = { ...originalEnv }; + encryptedFile = `encrypted-fixture-${++fileGeneration}`; mockExecaSync.mockClear(); // Default mock behavior mockExecaSync.mockImplementation(() => ({ exitCode: 0, stdout: "" })); @@ -80,8 +96,9 @@ describe("Secure Storage Platform Implementations", () => { }); test("Windows classified reads distinguish a missing DPAPI file", () => { - mockExecaSync.mockReturnValue({ exitCode: 2, stdout: "", stderr: "" }); + encryptedFile = Object.assign(new Error('Missing'), { code: 'ENOENT' }); expect(windowsCredentialStorage.readResult?.()).toEqual({ kind: "missing" }); + expect(mockExecaSync).not.toHaveBeenCalled(); }); test("Keychain classified reads bypass the process cache", () => { @@ -151,7 +168,7 @@ describe("Secure Storage Platform Implementations", () => { const [command, args, options] = execaCalls()[0]; expect(command).toBe('powershell.exe'); expect(args.slice(0, 4)).toEqual(['-NoLogo', '-NoProfile', '-NonInteractive', '-Command']); - expect(options.input).toBe(''); + expect(options.input).toBe(encryptedFile); expect(options.timeout).toBe(10_000); }); @@ -248,6 +265,130 @@ describe("Secure Storage Platform Implementations", () => { }); }); + describe('Windows credential read cache', () => { + const freshData = { verbooInstallationId: 'fresh-installation' }; + const respondWith = (data: object) => mockExecaSync.mockReturnValue({ exitCode: 0, stdout: JSON.stringify(data) }); + + test('an absent file never starts PowerShell during repeated agent reads', async () => { + encryptedFile = Object.assign(new Error('Missing'), { code: 'ENOENT' }); + for (let i = 0; i < 20; i++) expect(await windowsCredentialStorage.readAsync()).toBeNull(); + expect(mockExecaSync).not.toHaveBeenCalled(); + }); + + test('filesystem permission errors stay distinct from missing credentials', () => { + encryptedFile = Object.assign(new Error('Denied'), { code: 'EACCES' }); + expect(windowsCredentialStorage.readResult?.()).toMatchObject({ kind: 'error' }); + expect(windowsCredentialStorage.read()).toBeNull(); + expect(mockExecaSync).not.toHaveBeenCalled(); + }); + + test('concurrent-agent reads decrypt an unchanged record only once', async () => { + respondWith(testData); + const records = await Promise.all(Array.from({ length: 20 }, () => windowsCredentialStorage.readAsync())); + expect(records).toEqual(Array.from({ length: 20 }, () => testData)); + expect(mockExecaSync).toHaveBeenCalledTimes(1); + expect(execaCalls()[0][2].input).toBe(encryptedFile); + expect(execaCalls()[0][1].join(' ')).not.toContain(encryptedFile as string); + }); + + test('classifying a read always bypasses and invalidates the cache', () => { + respondWith(testData); + expect(windowsCredentialStorage.read()).toEqual(testData); + respondWith(freshData); + expect(windowsCredentialStorage.readResult?.()).toEqual({ kind: 'ok', data: freshData }); + expect(windowsCredentialStorage.read()).toEqual(freshData); + expect(mockExecaSync).toHaveBeenCalledTimes(3); + }); + + test('a changed encrypted record is visible without a TTL delay', () => { + respondWith(testData); + windowsCredentialStorage.read(); + encryptedFile = (encryptedFile as string).replace('fixture', 'changed'); + respondWith(freshData); + expect(windowsCredentialStorage.read()).toEqual(freshData); + expect(mockExecaSync).toHaveBeenCalledTimes(2); + }); + + test('logout and recreation cannot recover an old cached record', () => { + const original = encryptedFile; + respondWith(testData); + windowsCredentialStorage.read(); + encryptedFile = Object.assign(new Error('Missing'), { code: 'ENOENT' }); + expect(windowsCredentialStorage.read()).toBeNull(); + encryptedFile = original; + respondWith(freshData); + expect(windowsCredentialStorage.read()).toEqual(freshData); + expect(mockExecaSync).toHaveBeenCalledTimes(2); + }); + + test('a failed write invalidates previously decrypted data', () => { + respondWith(testData); + windowsCredentialStorage.read(); + mockExecaSync.mockReturnValue({ exitCode: 1, stdout: '' }); + expect(windowsCredentialStorage.update(freshData).success).toBe(false); + respondWith(freshData); + expect(windowsCredentialStorage.read()).toEqual(freshData); + expect(mockExecaSync).toHaveBeenCalledTimes(3); + }); + + test('deleting credentials invalidates previously decrypted data', () => { + respondWith(testData); + windowsCredentialStorage.read(); + expect(windowsCredentialStorage.delete()).toBe(true); + respondWith(freshData); + expect(windowsCredentialStorage.read()).toEqual(freshData); + expect(mockExecaSync).toHaveBeenCalledTimes(3); + }); + + test('scoped configurations cannot share a decrypted record', () => { + respondWith(testData); + windowsCredentialStorage.read(); + process.env.VERBOO_CONFIG_DIR = '/tmp/another-credential-cache-scope'; + respondWith(freshData); + expect(windowsCredentialStorage.read()).toEqual(freshData); + expect(mockExecaSync).toHaveBeenCalledTimes(2); + }); + + test('mutating a returned object cannot poison the cached snapshot', () => { + respondWith(testData); + const record = windowsCredentialStorage.read()!; + record.mcpOAuth!['test-server'].accessToken = 'mutated'; + expect(windowsCredentialStorage.read()).toEqual(testData); + expect(mockExecaSync).toHaveBeenCalledTimes(1); + }); + + test('decryption failures are not cached', () => { + mockExecaSync.mockReturnValue({ exitCode: 3, stdout: '' }); + expect(windowsCredentialStorage.read()).toBeNull(); + respondWith(testData); + expect(windowsCredentialStorage.read()).toEqual(testData); + expect(windowsCredentialStorage.read()).toEqual(testData); + expect(mockExecaSync).toHaveBeenCalledTimes(2); + }); + + test('the decrypted input and cache key remain the same snapshot during replacement', () => { + const before = encryptedFile; + mockExecaSync.mockImplementationOnce(() => { + encryptedFile = 'replacement-during-decrypt'; + return { exitCode: 0, stdout: JSON.stringify(testData) }; + }); + expect(windowsCredentialStorage.read()).toEqual(testData); + expect(execaCalls()[0][2].input).toBe(before); + respondWith(freshData); + expect(windowsCredentialStorage.read()).toEqual(freshData); + expect(execaCalls()[1][2].input).toBe('replacement-during-decrypt'); + }); + + test('missing native files retain explicitly enabled legacy reads', () => { + process.env.VERBOO_ENABLE_LEGACY_WINDOWS_PASSWORDVAULT = '1'; + encryptedFile = Object.assign(new Error('Missing'), { code: 'ENOENT' }); + respondWith(testData); + expect(windowsCredentialStorage.read()).toEqual(testData); + expect(mockExecaSync).toHaveBeenCalledTimes(1); + expect(powershellScript()).toContain('PasswordVault'); + }); + }); + describe("Linux secret-tool Interaction", () => { test("update passes payload via stdin", () => { linuxSecretStorage.update(testData); diff --git a/src/utils/secureStorage/windowsCredentialStorage.ts b/src/utils/secureStorage/windowsCredentialStorage.ts index 28c4039a0a..3d6364aab4 100644 --- a/src/utils/secureStorage/windowsCredentialStorage.ts +++ b/src/utils/secureStorage/windowsCredentialStorage.ts @@ -1,4 +1,5 @@ import { execaSync } from 'execa' +import { readFileSync } from 'node:fs' import { join } from 'path' import { getClaudeConfigHomeDir } from '../envUtils.js' import { jsonParse, jsonStringify } from '../slowOperations.js' @@ -34,14 +35,44 @@ function shouldUseLegacyPasswordVault(): boolean { return (process.env.VERBOO_ENABLE_LEGACY_WINDOWS_PASSWORDVAULT ?? process.env.OPENCLAUDE_ENABLE_LEGACY_WINDOWS_PASSWORDVAULT) === '1' } +type DpapiFileRead = + | { kind: 'ok'; encrypted: string } + | { kind: 'missing' } + | { kind: 'error'; warning: string } + +function readDpapiFile(path: string): DpapiFileRead { + try { + return { kind: 'ok', encrypted: readFileSync(path, 'utf8').trim() } + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if (code === 'ENOENT' || code === 'ENOTDIR') return { kind: 'missing' } + return { kind: 'error', warning: `Windows credential file could not be read (${code ?? 'unknown error'}).` } + } +} + +// Every read still checks the encrypted file. A login, refresh or logout in +// another process becomes visible immediately, without starting PowerShell +// again for each agent that uses an unchanged credential record. +let readCache: { + path: string + entropy: string + encrypted: string + data: SecureStorageData +} | null = null + function runPowerShell( script: string, options?: { input?: string }, ): ReturnType | null { try { - return execaSync('powershell.exe', ['-NoLogo', '-NoProfile', '-NonInteractive', '-Command', script], { - // Credential operations must not load shell profiles or wait for input - // from the CLI's terminal. An empty input closes stdin for reads/deletes. + // Execa reads/writes UTF-8 pipes; Windows console code pages must not + // corrupt non-ASCII credential metadata on either side of the pipe. + const utf8Script = `[Console]::InputEncoding = New-Object System.Text.UTF8Encoding($false) +[Console]::OutputEncoding = New-Object System.Text.UTF8Encoding($false) +${script}` + return execaSync('powershell.exe', ['-NoLogo', '-NoProfile', '-NonInteractive', '-Command', utf8Script], { + // Credential operations use a closed input pipe, never the CLI terminal + // or interactive shell profiles. Delete operations receive empty input. input: options?.input ?? '', reject: false, timeout: 10_000, @@ -102,24 +133,22 @@ function readLegacyPasswordVault(): SecureStorageData | null { export const windowsCredentialStorage: SecureStorage = { name: 'credential-locker-dpapi', read(): SecureStorageData | null { - const filePath = escapePowerShellSingleQuoted( - getWindowsSecureStorageFilePath(), - ) - const entropy = escapePowerShellSingleQuoted( - getWindowsSecureStorageEntropy(), - ) + const path = getWindowsSecureStorageFilePath() + const rawEntropy = getWindowsSecureStorageEntropy() + const file = readDpapiFile(path) + if (file.kind !== 'ok') { + readCache = null + return readLegacyPasswordVault() + } + if (readCache?.path === path && readCache.entropy === rawEntropy && readCache.encrypted === file.encrypted) { + return structuredClone(readCache.data) + } + readCache = null + const entropy = escapePowerShellSingleQuoted(rawEntropy) const script = ` try { Add-Type -AssemblyName System.Security - $path = '${filePath}' - if (!(Test-Path -LiteralPath $path)) { - exit 1 - } - - $protectedBase64 = [System.IO.File]::ReadAllText( - $path, - [System.Text.Encoding]::UTF8 - ).Trim() + $protectedBase64 = [Console]::In.ReadToEnd().Trim() if (-not $protectedBase64) { exit 1 } @@ -137,11 +166,15 @@ export const windowsCredentialStorage: SecureStorage = { } ` - const result = runPowerShell(script) + // Decrypt exactly the bytes used as the cache key, even if another + // process replaces the file while PowerShell starts. Never use argv for it. + const result = runPowerShell(script, { input: file.encrypted }) const stdout = typeof result?.stdout === 'string' ? result.stdout : '' if (result?.exitCode === 0 && stdout) { try { - return jsonParse(stdout) + const data = jsonParse(stdout) + readCache = { path, entropy: rawEntropy, encrypted: file.encrypted, data: structuredClone(data) } + return data } catch { return readLegacyPasswordVault() } @@ -150,14 +183,15 @@ export const windowsCredentialStorage: SecureStorage = { return readLegacyPasswordVault() }, readResult(): SecureStorageReadResult { - const filePath = escapePowerShellSingleQuoted(getWindowsSecureStorageFilePath()) + // Stateful read-modify-write callers must always decrypt a fresh snapshot. + readCache = null + const file = readDpapiFile(getWindowsSecureStorageFilePath()) + if (file.kind !== 'ok') return file const entropy = escapePowerShellSingleQuoted(getWindowsSecureStorageEntropy()) const script = ` try { Add-Type -AssemblyName System.Security - $path = '${filePath}' - if (!(Test-Path -LiteralPath $path)) { exit 2 } - $protectedBase64 = [System.IO.File]::ReadAllText($path, [System.Text.Encoding]::UTF8).Trim() + $protectedBase64 = [Console]::In.ReadToEnd().Trim() if (-not $protectedBase64) { exit 3 } $protectedBytes = [Convert]::FromBase64String($protectedBase64) $entropyBytes = [System.Text.Encoding]::UTF8.GetBytes('${entropy}') @@ -168,7 +202,7 @@ export const windowsCredentialStorage: SecureStorage = { [Console]::Out.Write([System.Text.Encoding]::UTF8.GetString($bytes)) } catch { exit 3 } ` - const result = runPowerShell(script) + const result = runPowerShell(script, { input: file.encrypted }) const stdout = typeof result?.stdout === 'string' ? result.stdout : '' if (result?.exitCode === 0 && stdout) { try { @@ -177,7 +211,6 @@ export const windowsCredentialStorage: SecureStorage = { return { kind: 'error', warning: 'DPAPI returned malformed JSON.' } } } - if (result?.exitCode === 2) return { kind: 'missing' } if (result?.exitCode === 3 && shouldUseLegacyPasswordVault()) { const legacy = readLegacyPasswordVault() if (legacy) return { kind: 'ok', data: legacy } @@ -188,6 +221,7 @@ export const windowsCredentialStorage: SecureStorage = { return this.read() }, update(data: SecureStorageData): { success: boolean; warning?: string } { + readCache = null const filePath = escapePowerShellSingleQuoted( getWindowsSecureStorageFilePath(), ) @@ -244,6 +278,7 @@ export const windowsCredentialStorage: SecureStorage = { } }, delete(): boolean { + readCache = null const filePath = escapePowerShellSingleQuoted( getWindowsSecureStorageFilePath(), )