From f3e0337f738c014d34b4555ca55640d7682fed4c Mon Sep 17 00:00:00 2001 From: kvyb Date: Tue, 7 Oct 2025 00:22:43 +0800 Subject: [PATCH] fix: testing works with keychains, migration verification, and CRUD operations. Fixes security CLI argument ordering and credential.ts error handling. --- esbuild.mjs | 13 ++ scripts/test-standalone-core-api-server.ts | 13 +- .../{ => __tests__}/credential.test.ts | 2 +- src/core/storage/credential.ts | 49 +++-- src/standalone/vscode-context.ts | 14 +- src/test/e2e/standalone-migration.test.ts | 196 ++++++++++-------- src/test/e2e/utils/migrate-secrets.ts | 31 +++ 7 files changed, 215 insertions(+), 103 deletions(-) rename src/core/storage/{ => __tests__}/credential.test.ts (99%) create mode 100644 src/test/e2e/utils/migrate-secrets.ts diff --git a/esbuild.mjs b/esbuild.mjs index 933cdd19f0..a20c23ef10 100644 --- a/esbuild.mjs +++ b/esbuild.mjs @@ -197,6 +197,19 @@ async function main() { } else { await extensionCtx.rebuild() await extensionCtx.dispose() + + // Also build the migration test utility for E2E tests when building standalone + if (standalone) { + const migrationConfig = { + ...baseConfig, + entryPoints: ["src/test/e2e/utils/migrate-secrets.ts"], + outfile: `${destDir}/migrate-secrets.js`, + external: ["vscode", "@grpc/reflection", "grpc-health-check", "better-sqlite3"], + } + const migrationCtx = await esbuild.context(migrationConfig) + await migrationCtx.rebuild() + await migrationCtx.dispose() + } } } diff --git a/scripts/test-standalone-core-api-server.ts b/scripts/test-standalone-core-api-server.ts index 94b2f72072..40dc92ad95 100644 --- a/scripts/test-standalone-core-api-server.ts +++ b/scripts/test-standalone-core-api-server.ts @@ -79,7 +79,8 @@ async function main(): Promise { } const extensionsDir = path.join(distDir, "vsce-extension") - const userDataDir = mkdtempSync(path.join(os.tmpdir(), "vsce")) + // Respect incoming CLINE_DIR from parent (e.g., E2E tests that seed legacy secrets) + const userDataDir = process.env.CLINE_DIR ? process.env.CLINE_DIR : mkdtempSync(path.join(os.tmpdir(), "vsce")) const clineTestWorkspace = mkdtempSync(path.join(os.tmpdir(), "cline-test-workspace-")) console.log("Starting HostBridge test server...") @@ -134,16 +135,22 @@ async function main(): Promise { CLINE_DIR: userDataDir, INSTALL_DIR: extensionsDir, }, - stdio: "inherit", + stdio: "pipe", }) childProcesses.push(coreService) + // Proxy core service output so callers that pipe this script can observe logs + coreService.stdout?.on("data", (chunk) => process.stdout.write(chunk)) + coreService.stderr?.on("data", (chunk) => process.stderr.write(chunk)) + const shutdown = async () => { console.log("\nShutting down services...") while (childProcesses.length > 0) { const child = childProcesses.pop() - if (child && !child.killed) child.kill("SIGINT") + if (child && !child.killed) { + child.kill("SIGINT") + } } await ClineApiServerMock.stopGlobalServer() diff --git a/src/core/storage/credential.test.ts b/src/core/storage/__tests__/credential.test.ts similarity index 99% rename from src/core/storage/credential.test.ts rename to src/core/storage/__tests__/credential.test.ts index aafe3bff7a..c64f2a0e7d 100644 --- a/src/core/storage/credential.test.ts +++ b/src/core/storage/__tests__/credential.test.ts @@ -4,7 +4,7 @@ import { expect } from "chai" import * as sinon from "sinon" import { ErrorService } from "@/services/error" import { Logger } from "@/services/logging/Logger" -import { CredentialStorage } from "./credential" +import { CredentialStorage } from "../credential" const platform = os.platform() diff --git a/src/core/storage/credential.ts b/src/core/storage/credential.ts index 0b83050f91..af0bf91b4f 100644 --- a/src/core/storage/credential.ts +++ b/src/core/storage/credential.ts @@ -55,6 +55,11 @@ export class CredentialStorage extends ClineStorage { const result = await this.exec(this.commands.get({ service, account, target })) return result || undefined } catch (error) { + // Return undefined if the key doesn't exist (expected behavior) + // "The specified item could not be found" is not an error, just means key doesn't exist + if (error instanceof Error && error.message.includes("could not be found")) { + return undefined + } throw error } } @@ -111,18 +116,38 @@ export class CredentialStorage extends ClineStorage { const PLATFORM_COMMANDS: PlatformCommands = { [PLATFORM_OS.MacOS]: { - get: ({ service, account }) => ({ - command: "security", - args: ["find-generic-password", "-s", service, "-a", account, "-w"], - }), - store: ({ service, account, value }) => ({ - command: "security", - args: ["add-generic-password", "-s", service, "-a", account, "-w", value], - }), - delete: ({ service, account }) => ({ - command: "security", - args: ["delete-generic-password", "-s", service, "-a", account], - }), + get: ({ service, account }) => { + const keychain = process.env.CLINE_KEYCHAIN + const args = ["find-generic-password", "-s", service, "-a", account, "-w"] + // Note: find-generic-password doesn't support -k flag + // If using a custom keychain, specify it at the end as a positional argument + if (keychain && keychain.length > 0) { + args.push(keychain) + } + return { command: "security", args } + }, + store: ({ service, account, value }) => { + const keychain = process.env.CLINE_KEYCHAIN + const args = ["add-generic-password", "-s", service, "-a", account] + // For test keychains, allow all apps to access (makes it manageable in Keychain Access) + if (keychain && keychain.length > 0) { + args.push("-A") + } + args.push("-w", value) + // Keychain must be specified as positional argument at the end + if (keychain && keychain.length > 0) { + args.push(keychain) + } + return { command: "security", args } + }, + delete: ({ service, account }) => { + const keychain = process.env.CLINE_KEYCHAIN + const args = ["delete-generic-password", "-s", service, "-a", account] + if (keychain && keychain.length > 0) { + args.push("-k", keychain) + } + return { command: "security", args } + }, }, [PLATFORM_OS.Linux]: { get: ({ service, account }) => ({ diff --git a/src/standalone/vscode-context.ts b/src/standalone/vscode-context.ts index 78463abace..3f1ed2055e 100644 --- a/src/standalone/vscode-context.ts +++ b/src/standalone/vscode-context.ts @@ -1,5 +1,5 @@ import { spawnSync } from "node:child_process" -import { mkdirSync } from "fs" +import { existsSync, mkdirSync } from "fs" import os from "os" import path from "path" import type { Extension, ExtensionContext } from "vscode" @@ -41,12 +41,22 @@ export function initializeContext(clineDir?: string) { const EXTENSION_DIR = path.join(INSTALL_DIR, "extension") const EXTENSION_MODE = process.env.IS_DEV === "true" ? ExtensionMode.Development : ExtensionMode.Production + // In minimal contexts (e.g., migration tests), the packaged extension assets may not exist. + // Read package.json best-effort to avoid crashing during migrate-only runs. + let extensionPackageJson: any + try { + const pkgPath = path.join(EXTENSION_DIR, "package.json") + extensionPackageJson = existsSync(pkgPath) ? readJson(pkgPath) : { name: "cline-standalone", version: "0.0.0" } + } catch { + extensionPackageJson = { name: "cline-standalone", version: "0.0.0" } + } + const extension: Extension = { id: ExtensionRegistryInfo.id, isActive: true, extensionPath: EXTENSION_DIR, extensionUri: URI.file(EXTENSION_DIR), - packageJSON: readJson(path.join(EXTENSION_DIR, "package.json")), + packageJSON: extensionPackageJson, exports: undefined, // There are no API exports in the standalone version. activate: async () => {}, extensionKind: ExtensionKind.UI, diff --git a/src/test/e2e/standalone-migration.test.ts b/src/test/e2e/standalone-migration.test.ts index bee67bedf7..73ef0b5ce1 100644 --- a/src/test/e2e/standalone-migration.test.ts +++ b/src/test/e2e/standalone-migration.test.ts @@ -5,11 +5,15 @@ import * as path from "node:path" import { expect, test } from "@playwright/test" function hasCommand(cmd: string): boolean { - if (process.platform === "win32") return true + if (process.platform === "win32") { + return true + } const result = spawnSync("sh", ["-c", `command -v ${cmd}`], { stdio: "ignore" }) return result.status === 0 } +test.setTimeout(90000) + test("Standalone migration moves secrets.json to OS keychain and removes file (macOS only)", async () => { const platform = os.platform() if (platform !== "darwin") { @@ -28,93 +32,111 @@ test("Standalone migration moves secrets.json to OS keychain and removes file (m const secretsPath = path.join(dataDir, "secrets.json") fs.writeFileSync(secretsPath, JSON.stringify({ openRouterApiKey: "migrate-me" }, null, 2)) - // Ensure no preexisting key in OS keychain - const service = "Cline: openRouterApiKey" - const account = "cline_openRouterApiKey" - spawnSync("security", ["delete-generic-password", "-s", service, "-a", account], { stdio: "ignore" }) - - // Start the standalone core service via helper script - const procEnv = { - ...process.env, - CLINE_DIR: userDataDir, - E2E_TEST: "true", - CLINE_ENVIRONMENT: "local", - } - - const server: ChildProcess = spawn( - process.platform === "win32" ? "npx.cmd" : "npx", - ["tsx", "scripts/test-standalone-core-api-server.ts"], - { stdio: "pipe", env: procEnv }, - ) - - // Capture logs - let output = "" - server.stdout?.on("data", (d) => (output += String(d))) - server.stderr?.on("data", (d) => (output += String(d))) - - // Helper to wait for a regex in output up to timeoutMs - async function waitFor(pattern: RegExp, timeoutMs: number): Promise { - const start = Date.now() - while (Date.now() - start < timeoutMs) { - if (pattern.test(output)) return true - await new Promise((r) => setTimeout(r, 200)) - } - return false - } - - // Wait for migration complete log (deterministic), up to 45s - const successSeen = await waitFor( - /Secrets migration: (migrated .* entries; removed secrets\.json|all entries already present; removed secrets\.json)/i, - 45000, - ) - // If an abort log appeared, surface it - const abortSeen = - /Secrets migration aborted and rolled back/i.test(output) || /Migration from secrets\.json failed/i.test(output) - - // Stop server + // Ephemeral, unlocked test keychain to avoid GUI prompts + const keychainPath = path.join(os.tmpdir(), `cline-tests-${Date.now()}-${Math.random().toString(36).slice(2)}.keychain-db`) + const keychainPwd = "cline-test-pass" try { - server.kill("SIGINT") - } catch {} + spawnSync("security", ["create-keychain", "-p", keychainPwd, keychainPath], { stdio: "ignore" }) + spawnSync("security", ["set-keychain-settings", "-lut", "3600", keychainPath], { stdio: "ignore" }) + spawnSync("security", ["unlock-keychain", "-p", keychainPwd, keychainPath], { stdio: "ignore" }) - expect(abortSeen).toBeFalsy() - expect(successSeen).toBeTruthy() - // File should be removed - expect(fs.existsSync(secretsPath)).toBeFalsy() + // Ensure no preexisting key in test keychain + const service = "Cline: openRouterApiKey" + const account = "cline_openRouterApiKey" + spawnSync("security", ["delete-generic-password", "-s", service, "-a", account, "-k", keychainPath], { stdio: "ignore" }) - // Quick keychain presence check (5s max) - let present = false - const checkStart = Date.now() - while (Date.now() - checkStart < 5000) { - const status = spawnSync("security", ["find-generic-password", "-s", service, "-a", account, "-w"], { - stdio: "ignore", - }).status - if (status === 0) { - present = true - break + // Run the migration script (compiled from standalone build) + const procEnv = { ...process.env, CLINE_DIR: userDataDir, CLINE_KEYCHAIN: keychainPath } + + // Verify migration script exists + const migrationScript = path.join(process.cwd(), "dist-standalone", "migrate-secrets.js") + if (!fs.existsSync(migrationScript)) { + throw new Error("Migration script not found. Run: npm run compile-standalone") } - await new Promise((r) => setTimeout(r, 250)) + + const migrator: ChildProcess = spawn("node", [migrationScript], { stdio: "pipe", env: procEnv }) + let output = "" + migrator.stdout?.on("data", (d) => { + output += String(d) + }) + migrator.stderr?.on("data", (d) => { + output += String(d) + }) + + // Wait for migration to complete by polling for file removal (source of truth) + const end = Date.now() + 60000 + let successSeen = false + let abortSeen = false + while (Date.now() < end) { + // If we saw an abort, surface immediately + abortSeen = /Secrets migration aborted and rolled back|Migration from secrets\.json failed/i.test(output) + if (abortSeen) { + break + } + if (!fs.existsSync(secretsPath)) { + successSeen = true + break + } + await new Promise((r) => setTimeout(r, 250)) + } + + try { + migrator.kill("SIGINT") + } catch { + // Ignore if already exited + } + + if (abortSeen || !successSeen) { + console.log("Migration output:", output) + } + expect(abortSeen).toBeFalsy() + expect(successSeen).toBeTruthy() + // File should be removed + expect(fs.existsSync(secretsPath)).toBeFalsy() + + // Quick keychain presence check (5s max) + let present = false + const checkStart = Date.now() + while (Date.now() - checkStart < 5000) { + // Keychain path must be positional argument at the end, not with -k flag + const status = spawnSync("security", ["find-generic-password", "-s", service, "-a", account, "-w", keychainPath], { + stdio: "ignore", + }).status + if (status === 0) { + present = true + break + } + await new Promise((r) => setTimeout(r, 250)) + } + expect(present).toBeTruthy() + + // Second run: ensure no re-migration and legacy file is not recreated + const server2: ChildProcess = spawn( + process.platform === "win32" ? "npx.cmd" : "npx", + ["tsx", "scripts/test-standalone-core-api-server.ts"], + { stdio: "pipe", env: procEnv }, + ) + + // Capture output for debugging if needed + server2.stdout?.on("data", () => { + // Output captured but not used - server just needs to run + }) + server2.stderr?.on("data", () => { + // Output captured but not used - server just needs to run + }) + + // Give it a short window to initialize + await new Promise((r) => setTimeout(r, 4000)) + try { + server2.kill("SIGINT") + } catch {} + + // secrets.json should not have been recreated + expect(fs.existsSync(secretsPath)).toBeFalsy() + } finally { + // Clean up test keychain + spawnSync("security", ["delete-keychain", keychainPath], { stdio: "ignore" }) } - expect(present).toBeTruthy() - - // Second run: ensure no re-migration and legacy file is not recreated - const server2: ChildProcess = spawn( - process.platform === "win32" ? "npx.cmd" : "npx", - ["tsx", "scripts/test-standalone-core-api-server.ts"], - { stdio: "pipe", env: procEnv }, - ) - - let outputSecond = "" - server2.stdout?.on("data", (d) => (outputSecond += String(d))) - server2.stderr?.on("data", (d) => (outputSecond += String(d))) - - // Give it a short window to initialize - await new Promise((r) => setTimeout(r, 4000)) - try { - server2.kill("SIGINT") - } catch {} - - // secrets.json should not have been recreated - expect(fs.existsSync(secretsPath)).toBeFalsy() }) test("Standalone deps warning emitted when deps missing (best-effort)", async () => { @@ -136,8 +158,12 @@ test("Standalone deps warning emitted when deps missing (best-effort)", async () ) let output2 = "" - server.stdout?.on("data", (d) => (output2 += String(d))) - server.stderr?.on("data", (d) => (output2 += String(d))) + server.stdout?.on("data", (d) => { + output2 += String(d) + }) + server.stderr?.on("data", (d) => { + output2 += String(d) + }) await new Promise((r) => setTimeout(r, 6000)) try { diff --git a/src/test/e2e/utils/migrate-secrets.ts b/src/test/e2e/utils/migrate-secrets.ts new file mode 100644 index 0000000000..6d8822effd --- /dev/null +++ b/src/test/e2e/utils/migrate-secrets.ts @@ -0,0 +1,31 @@ +#!/usr/bin/env node +/** + * Minimal migration script for E2E testing. + * This runs only the migration logic without starting the full server. + */ + +import { initializeContext, runLegacySecretsMigrationIfNeeded } from "../../../standalone/vscode-context" + +async function main() { + const clineDir = process.env.CLINE_DIR + if (!clineDir) { + console.error("CLINE_DIR environment variable not set") + process.exit(1) + } + + try { + // Initialize context (sets up storage backends) + initializeContext(clineDir) + + // Run migration + await runLegacySecretsMigrationIfNeeded() + + console.log("MIGRATION_DONE") + process.exit(0) + } catch (error) { + console.error("MIGRATION_FAILED", error) + process.exit(1) + } +} + +main()