From c6e091093af294554f0fc3fa0bf9262b0fd4a0f2 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 3 Mar 2026 19:43:30 +0300 Subject: [PATCH 1/4] feat: delegate keyring operations to CLI instead of native binaries Replace @napi-rs/keyring (4 native .node binaries) with CLI subprocess calls. The extension now runs `coder login --use-token-as-session` to store tokens and `coder login token --url` to read them, keeping the credential format in sync with the CLI automatically. - Add CliCredentialManager that shells out to the Coder CLI - Update CliManager.configure() to accept binPath and delegate to CLI - Update LoginCoordinator to fetch CLI binary for keyring reads - Remove clearCredentials keyring cleanup (stale entries are harmless) - Remove @napi-rs/keyring dep, vendor script, supportedArchitectures - Delete KeyringStore and its tests --- esbuild.mjs | 2 +- package.json | 3 +- pnpm-lock.yaml | 135 ----------- pnpm-workspace.yaml | 13 -- scripts/vendor-keyring.mjs | 61 ----- src/cliConfig.ts | 2 +- src/core/cliCredentialManager.ts | 76 ++++++ src/core/cliManager.ts | 31 ++- src/core/container.ts | 15 +- src/keyringStore.ts | 194 ---------------- src/login/loginCoordinator.ts | 51 +++- src/remote/remote.ts | 2 + test/mocks/testHelpers.ts | 11 +- test/unit/core/cliCredentialManager.test.ts | 211 +++++++++++++++++ test/unit/core/cliManager.concurrent.test.ts | 4 +- test/unit/core/cliManager.test.ts | 68 +++--- test/unit/keyringStore.test.ts | 231 ------------------- test/unit/login/loginCoordinator.test.ts | 14 +- 18 files changed, 404 insertions(+), 720 deletions(-) delete mode 100644 scripts/vendor-keyring.mjs create mode 100644 src/core/cliCredentialManager.ts delete mode 100644 src/keyringStore.ts create mode 100644 test/unit/core/cliCredentialManager.test.ts delete mode 100644 test/unit/keyringStore.test.ts diff --git a/esbuild.mjs b/esbuild.mjs index 9a0e89f3..54fb4a1d 100644 --- a/esbuild.mjs +++ b/esbuild.mjs @@ -32,7 +32,7 @@ const buildOptions = { // undefined when bundled to CJS, causing runtime errors. openpgp: "./node_modules/openpgp/dist/node/openpgp.min.cjs", }, - external: ["vscode", "@napi-rs/keyring"], + external: ["vscode"], sourcemap: production ? "external" : true, minify: production, plugins: watch ? [logRebuildPlugin] : [], diff --git a/package.json b/package.json index ac338c9c..c4170c82 100644 --- a/package.json +++ b/package.json @@ -32,7 +32,7 @@ "test:integration": "tsc -p test --outDir out --noCheck && node esbuild.mjs && vscode-test", "test:webview": "vitest --project webview", "typecheck": "concurrently -g \"tsc --noEmit\" \"tsc --noEmit -p test\"", - "vscode:prepublish": "pnpm build:production && node scripts/vendor-keyring.mjs", + "vscode:prepublish": "pnpm build:production", "watch": "concurrently -n extension,webviews \"pnpm watch:extension\" \"pnpm watch:webviews\"", "watch:extension": "node esbuild.mjs --watch", "watch:webviews": "pnpm -r --filter \"./packages/*\" --parallel dev" @@ -468,7 +468,6 @@ "word-wrap": "1.2.5" }, "dependencies": { - "@napi-rs/keyring": "^1.2.0", "@peculiar/x509": "^1.14.3", "@repo/shared": "workspace:*", "axios": "1.13.6", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 77808dae..9f685f62 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -58,9 +58,6 @@ importers: .: dependencies: - '@napi-rs/keyring': - specifier: ^1.2.0 - version: 1.2.0 '@peculiar/x509': specifier: ^1.14.3 version: 1.14.3 @@ -989,87 +986,6 @@ packages: '@lit/reactive-element@2.1.2': resolution: {integrity: sha512-pbCDiVMnne1lYUIaYNN5wrwQXDtHaYtg7YEFPeW+hws6U47WeFvISGUWekPGKWOP1ygrs0ef0o1VJMk1exos5A==} - '@napi-rs/keyring-darwin-arm64@1.2.0': - resolution: {integrity: sha512-CA83rDeyONDADO25JLZsh3eHY8yTEtm/RS6ecPsY+1v+dSawzT9GywBMu2r6uOp1IEhQs/xAfxgybGAFr17lSA==} - engines: {node: '>= 10'} - cpu: [arm64] - os: [darwin] - - '@napi-rs/keyring-darwin-x64@1.2.0': - resolution: {integrity: sha512-dBHjtKRCj4ByfnfqIKIJLo3wueQNJhLRyuxtX/rR4K/XtcS7VLlRD01XXizjpre54vpmObj63w+ZpHG+mGM8uA==} - engines: {node: '>= 10'} - cpu: [x64] - os: [darwin] - - '@napi-rs/keyring-freebsd-x64@1.2.0': - resolution: {integrity: sha512-DPZFr11pNJSnaoh0dzSUNF+T6ORhy3CkzUT3uGixbA71cAOPJ24iG8e8QrLOkuC/StWrAku3gBnth2XMWOcR3Q==} - engines: {node: '>= 10'} - cpu: [x64] - os: [freebsd] - - '@napi-rs/keyring-linux-arm-gnueabihf@1.2.0': - resolution: {integrity: sha512-8xv6DyEMlvRdqJzp4F39RLUmmTQsLcGYYv/3eIfZNZN1O5257tHxTrFYqAsny659rJJK2EKeSa7PhrSibQqRWQ==} - engines: {node: '>= 10'} - cpu: [arm] - os: [linux] - - '@napi-rs/keyring-linux-arm64-gnu@1.2.0': - resolution: {integrity: sha512-Pu2V6Py+PBt7inryEecirl+t+ti8bhZphjP+W68iVaXHUxLdWmkgL9KI1VkbRHbx5k8K5Tew9OP218YfmVguIA==} - engines: {node: '>= 10'} - cpu: [arm64] - os: [linux] - libc: [glibc] - - '@napi-rs/keyring-linux-arm64-musl@1.2.0': - resolution: {integrity: sha512-8TDymrpC4P1a9iDEaegT7RnrkmrJN5eNZh3Im3UEV5PPYGtrb82CRxsuFohthCWQW81O483u1bu+25+XA4nKUw==} - engines: {node: '>= 10'} - cpu: [arm64] - os: [linux] - libc: [musl] - - '@napi-rs/keyring-linux-riscv64-gnu@1.2.0': - resolution: {integrity: sha512-awsB5XI1MYL7fwfjMDGmKOWvNgJEO7mM7iVEMS0fO39f0kVJnOSjlu7RHcXAF0LOx+0VfF3oxbWqJmZbvRCRHw==} - engines: {node: '>= 10'} - cpu: [riscv64] - os: [linux] - libc: [glibc] - - '@napi-rs/keyring-linux-x64-gnu@1.2.0': - resolution: {integrity: sha512-8E+7z4tbxSJXxIBqA+vfB1CGajpCDRyTyqXkBig5NtASrv4YXcntSo96Iah2QDR5zD3dSTsmbqJudcj9rKKuHQ==} - engines: {node: '>= 10'} - cpu: [x64] - os: [linux] - libc: [glibc] - - '@napi-rs/keyring-linux-x64-musl@1.2.0': - resolution: {integrity: sha512-8RZ8yVEnmWr/3BxKgBSzmgntI7lNEsY7xouNfOsQkuVAiCNmxzJwETspzK3PQ2FHtDxgz5vHQDEBVGMyM4hUHA==} - engines: {node: '>= 10'} - cpu: [x64] - os: [linux] - libc: [musl] - - '@napi-rs/keyring-win32-arm64-msvc@1.2.0': - resolution: {integrity: sha512-AoqaDZpQ6KPE19VBLpxyORcp+yWmHI9Xs9Oo0PJ4mfHma4nFSLVdhAubJCxdlNptHe5va7ghGCHj3L9Akiv4cQ==} - engines: {node: '>= 10'} - cpu: [arm64] - os: [win32] - - '@napi-rs/keyring-win32-ia32-msvc@1.2.0': - resolution: {integrity: sha512-EYL+EEI6bCsYi3LfwcQdnX3P/R76ENKNn+3PmpGheBsUFLuh0gQuP7aMVHM4rTw6UVe+L3vCLZSptq/oeacz0A==} - engines: {node: '>= 10'} - cpu: [ia32] - os: [win32] - - '@napi-rs/keyring-win32-x64-msvc@1.2.0': - resolution: {integrity: sha512-xFlx/TsmqmCwNU9v+AVnEJgoEAlBYgzFF5Ihz1rMpPAt4qQWWkMd4sCyM1gMJ1A/GnRqRegDiQpwaxGUHFtFbA==} - engines: {node: '>= 10'} - cpu: [x64] - os: [win32] - - '@napi-rs/keyring@1.2.0': - resolution: {integrity: sha512-d0d4Oyxm+v980PEq1ZH2PmS6cvpMIRc17eYpiU47KgW+lzxklMu6+HOEOPmxrpnF/XQZ0+Q78I2mgMhbIIo/dg==} - engines: {node: '>= 10'} - '@napi-rs/wasm-runtime@0.2.12': resolution: {integrity: sha512-ZVWUcfwY4E/yPitQJl481FjFo3K22D6qF0DuFH6Y/nbnE11GY5uguDxZMGXPQ8WQ0128MXQD7TnfHyK4oWoIJQ==} @@ -5006,57 +4922,6 @@ snapshots: dependencies: '@lit-labs/ssr-dom-shim': 1.5.1 - '@napi-rs/keyring-darwin-arm64@1.2.0': - optional: true - - '@napi-rs/keyring-darwin-x64@1.2.0': - optional: true - - '@napi-rs/keyring-freebsd-x64@1.2.0': - optional: true - - '@napi-rs/keyring-linux-arm-gnueabihf@1.2.0': - optional: true - - '@napi-rs/keyring-linux-arm64-gnu@1.2.0': - optional: true - - '@napi-rs/keyring-linux-arm64-musl@1.2.0': - optional: true - - '@napi-rs/keyring-linux-riscv64-gnu@1.2.0': - optional: true - - '@napi-rs/keyring-linux-x64-gnu@1.2.0': - optional: true - - '@napi-rs/keyring-linux-x64-musl@1.2.0': - optional: true - - '@napi-rs/keyring-win32-arm64-msvc@1.2.0': - optional: true - - '@napi-rs/keyring-win32-ia32-msvc@1.2.0': - optional: true - - '@napi-rs/keyring-win32-x64-msvc@1.2.0': - optional: true - - '@napi-rs/keyring@1.2.0': - optionalDependencies: - '@napi-rs/keyring-darwin-arm64': 1.2.0 - '@napi-rs/keyring-darwin-x64': 1.2.0 - '@napi-rs/keyring-freebsd-x64': 1.2.0 - '@napi-rs/keyring-linux-arm-gnueabihf': 1.2.0 - '@napi-rs/keyring-linux-arm64-gnu': 1.2.0 - '@napi-rs/keyring-linux-arm64-musl': 1.2.0 - '@napi-rs/keyring-linux-riscv64-gnu': 1.2.0 - '@napi-rs/keyring-linux-x64-gnu': 1.2.0 - '@napi-rs/keyring-linux-x64-musl': 1.2.0 - '@napi-rs/keyring-win32-arm64-msvc': 1.2.0 - '@napi-rs/keyring-win32-ia32-msvc': 1.2.0 - '@napi-rs/keyring-win32-x64-msvc': 1.2.0 - '@napi-rs/wasm-runtime@0.2.12': dependencies: '@emnapi/core': 1.7.1 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 613661d2..70f3d76e 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -27,16 +27,3 @@ onlyBuiltDependencies: - keytar - unrs-resolver - utf-8-validate - -# Install @napi-rs/keyring native binaries for macOS and Windows so they're -# available when building the universal VSIX (even on Linux CI). -# Only macOS and Windows use the keyring; Linux falls back to file storage. -supportedArchitectures: - os: - - current - - darwin - - win32 - cpu: - - current - - x64 - - arm64 diff --git a/scripts/vendor-keyring.mjs b/scripts/vendor-keyring.mjs deleted file mode 100644 index 5cf8cd31..00000000 --- a/scripts/vendor-keyring.mjs +++ /dev/null @@ -1,61 +0,0 @@ -/** - * Vendor @napi-rs/keyring into dist/node_modules/ for VSIX packaging. - * - * pnpm uses symlinks that vsce can't follow. This script resolves them and - * copies the JS wrapper plus macOS/Windows .node binaries into dist/, where - * Node's require() resolution finds them from dist/extension.js. - */ -import { - cpSync, - existsSync, - mkdirSync, - readdirSync, - realpathSync, - rmSync, -} from "node:fs"; -import { join, resolve } from "node:path"; - -const keyringPkg = resolve("node_modules/@napi-rs/keyring"); -const outputDir = resolve("dist/node_modules/@napi-rs/keyring"); - -if (!existsSync(keyringPkg)) { - console.error("@napi-rs/keyring not found — run pnpm install first"); - process.exit(1); -} - -// Copy the JS wrapper package (resolving pnpm symlinks) -const resolvedPkg = realpathSync(keyringPkg); -rmSync(outputDir, { recursive: true, force: true }); -mkdirSync(outputDir, { recursive: true }); -cpSync(resolvedPkg, outputDir, { recursive: true }); - -// Native binary packages live as siblings of the resolved keyring package in -// pnpm's content-addressable store (they aren't hoisted to node_modules). -const siblingsDir = resolve(resolvedPkg, ".."); -const nativePackages = [ - "keyring-darwin-arm64", - "keyring-darwin-x64", - "keyring-win32-arm64-msvc", - "keyring-win32-x64-msvc", -]; - -for (const pkg of nativePackages) { - const pkgDir = join(siblingsDir, pkg); - if (!existsSync(pkgDir)) { - console.error( - `Missing native package: ${pkg}\n` + - "Ensure supportedArchitectures in pnpm-workspace.yaml includes all target platforms.", - ); - process.exit(1); - } - const nodeFile = readdirSync(pkgDir).find((f) => f.endsWith(".node")); - if (!nodeFile) { - console.error(`No .node binary found in ${pkg}`); - process.exit(1); - } - cpSync(join(pkgDir, nodeFile), join(outputDir, nodeFile)); -} - -console.log( - `Vendored @napi-rs/keyring with ${nativePackages.length} platform binaries into dist/`, -); diff --git a/src/cliConfig.ts b/src/cliConfig.ts index 6831117e..7461651e 100644 --- a/src/cliConfig.ts +++ b/src/cliConfig.ts @@ -1,5 +1,5 @@ +import { isKeyringSupported } from "./core/cliCredentialManager"; import { getHeaderArgs } from "./headers"; -import { isKeyringSupported } from "./keyringStore"; import { escapeCommandArg } from "./util"; import type { WorkspaceConfiguration } from "vscode"; diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts new file mode 100644 index 00000000..dcd8f3b5 --- /dev/null +++ b/src/core/cliCredentialManager.ts @@ -0,0 +1,76 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +import { getHeaderArgs } from "../headers"; + +import type { WorkspaceConfiguration } from "vscode"; + +import type { Logger } from "../logging/logger"; + +const execFileAsync = promisify(execFile); + +/** + * Returns true on platforms where the OS keyring is supported (macOS, Windows). + */ +export function isKeyringSupported(): boolean { + return process.platform === "darwin" || process.platform === "win32"; +} + +/** + * Delegates credential storage to the Coder CLI to keep the credentials in sync. + */ +export class CliCredentialManager { + constructor(private readonly logger: Logger) {} + + /** + * Store a token by running: + * CODER_SESSION_TOKEN= login --use-token-as-session + * + * The token is passed via environment variable so it never appears in + * process argument lists. + */ + async storeToken( + binPath: string, + url: string, + token: string, + configs: Pick, + ): Promise { + const args = [ + ...getHeaderArgs(configs), + "login", + "--use-token-as-session", + url, + ]; + try { + await execFileAsync(binPath, args, { + env: { ...process.env, CODER_SESSION_TOKEN: token }, + }); + this.logger.info("Stored token via CLI for", url); + } catch (error) { + this.logger.error("Failed to store token via CLI:", error); + throw error; + } + } + + /** + * Read a token by running: + * login token --url + * + * Returns trimmed stdout, or undefined on any failure. + */ + async readToken( + binPath: string, + url: string, + configs: Pick, + ): Promise { + const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; + try { + const { stdout } = await execFileAsync(binPath, args); + const token = stdout.trim(); + return token || undefined; + } catch (error) { + this.logger.warn("Failed to read token via CLI:", error); + return undefined; + } + } +} diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 6f918eb0..474702b3 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,7 +10,7 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; -import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig"; +import { shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; import { vscodeProposed } from "../vscodeProposed"; @@ -22,9 +22,9 @@ import type { Api } from "coder/site/src/api/api"; import type { IncomingMessage } from "node:http"; import type { FeatureSet } from "../featureSet"; -import type { KeyringStore } from "../keyringStore"; import type { Logger } from "../logging/logger"; +import type { CliCredentialManager } from "./cliCredentialManager"; import type { PathResolver } from "./pathResolver"; export class CliManager { @@ -33,7 +33,7 @@ export class CliManager { constructor( private readonly output: Logger, private readonly pathResolver: PathResolver, - private readonly keyringStore: KeyringStore, + private readonly cliCredentialManager: CliCredentialManager, ) { this.binaryLock = new BinaryLock(output); } @@ -723,6 +723,7 @@ export class CliManager { url: string, token: string, featureSet: FeatureSet, + binPath: string, ) { if (!url) { throw new Error("URL is required to configure the CLI"); @@ -731,10 +732,14 @@ export class CliManager { const configs = vscode.workspace.getConfiguration(); if (shouldUseKeyring(configs, featureSet)) { try { - this.keyringStore.setToken(url, token); - this.output.info("Stored token in OS keyring for", url); + await this.cliCredentialManager.storeToken( + binPath, + url, + token, + configs, + ); } catch (error) { - this.output.error("Failed to store token in OS keyring:", error); + this.output.error("Failed to store token via CLI keyring:", error); vscode.window .showErrorMessage( `Failed to store session token in OS keyring: ${errToStr(error)}. ` + @@ -760,18 +765,12 @@ export class CliManager { } /** - * Remove credentials for a deployment from both keyring and file storage. + * Remove file-based credentials for a deployment. Keyring entries are not + * removed here because deleting requires the CLI binary, which may not be + * available at logout time. This is fine: stale keyring entries are harmless + * since the CLI overwrites them on next `coder login`. */ public async clearCredentials(safeHostname: string): Promise { - if (isKeyringEnabled(vscode.workspace.getConfiguration())) { - try { - this.keyringStore.deleteToken(safeHostname); - this.output.info("Removed keyring token for", safeHostname); - } catch (error) { - this.output.warn("Failed to remove keyring token", error); - } - } - const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); const urlPath = this.pathResolver.getUrlPath(safeHostname); await Promise.all([ diff --git a/src/core/container.ts b/src/core/container.ts index 51413c2e..5b7c5953 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -1,9 +1,9 @@ import * as vscode from "vscode"; -import { KeyringStore } from "../keyringStore"; import { type Logger } from "../logging/logger"; import { LoginCoordinator } from "../login/loginCoordinator"; +import { CliCredentialManager } from "./cliCredentialManager"; import { CliManager } from "./cliManager"; import { ContextManager } from "./contextManager"; import { MementoManager } from "./mementoManager"; @@ -19,7 +19,7 @@ export class ServiceContainer implements vscode.Disposable { private readonly pathResolver: PathResolver; private readonly mementoManager: MementoManager; private readonly secretsManager: SecretsManager; - private readonly keyringStore: KeyringStore; + private readonly cliCredentialManager: CliCredentialManager; private readonly cliManager: CliManager; private readonly contextManager: ContextManager; private readonly loginCoordinator: LoginCoordinator; @@ -36,18 +36,19 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); - this.keyringStore = new KeyringStore(this.logger); + this.cliCredentialManager = new CliCredentialManager(this.logger); this.cliManager = new CliManager( this.logger, this.pathResolver, - this.keyringStore, + this.cliCredentialManager, ); this.contextManager = new ContextManager(context); this.loginCoordinator = new LoginCoordinator( this.secretsManager, this.mementoManager, this.logger, - this.keyringStore, + this.cliCredentialManager, + this.cliManager, context.extension.id, ); } @@ -76,8 +77,8 @@ export class ServiceContainer implements vscode.Disposable { return this.contextManager; } - getKeyringStore(): KeyringStore { - return this.keyringStore; + getCliCredentialManager(): CliCredentialManager { + return this.cliCredentialManager; } getLoginCoordinator(): LoginCoordinator { diff --git a/src/keyringStore.ts b/src/keyringStore.ts deleted file mode 100644 index 4d1942d0..00000000 --- a/src/keyringStore.ts +++ /dev/null @@ -1,194 +0,0 @@ -import type { Logger } from "./logging/logger"; - -/** A single entry in the CLI's keyring credential map. */ -interface CredentialEntry { - coder_url: string; - api_token: string; -} - -type CredentialMap = Record; - -/** Subset of @napi-rs/keyring Entry used for credential storage. */ -export interface KeyringEntry { - getPassword(): string | null; - setPassword(password: string): void; - getSecret(): Uint8Array | null; - setSecret(secret: Uint8Array): void; - deleteCredential(): void; -} - -const KEYRING_SERVICE = "coder-v2-credentials"; -const KEYRING_ACCOUNT = "coder-login-credentials"; - -function createDefaultEntry(): KeyringEntry { - // Lazy require so Linux never loads the native binary. - // eslint-disable-next-line @typescript-eslint/no-require-imports - const { Entry } = require("@napi-rs/keyring") as { - Entry: { - new (service: string, username: string): KeyringEntry; - withTarget( - target: string, - service: string, - username: string, - ): KeyringEntry; - }; - }; - - if (process.platform === "darwin") { - // On macOS, withTarget selects a keychain domain, not an attribute — using - // it creates a separate entry the CLI can't find. The two-arg constructor - // matches the CLI's kSecAttrService + kSecAttrAccount lookup. - return new Entry(KEYRING_SERVICE, KEYRING_ACCOUNT); - } - - // On Windows, withTarget sets the credential's lookup key, matching the CLI. - return Entry.withTarget(KEYRING_SERVICE, KEYRING_SERVICE, KEYRING_ACCOUNT); -} - -/** Extracts the host from a URL for use as credential map key (matches CLI format). */ -function toHost(deploymentUrl: string): string { - try { - return new URL(deploymentUrl).host; - } catch { - return deploymentUrl; - } -} - -/** - * Finds the map key matching a safeHostname. VS Code identifies deployments by - * safeHostname (port stripped), while the CLI stores map keys via `toHost` - * which preserves ports. The fallback strips ports from map keys so VS Code's - * port-less hostname still matches a CLI-written entry with a port. - */ -function findMapKey( - map: CredentialMap, - safeHostname: string, -): string | undefined { - if (safeHostname in map) { - return safeHostname; - } - for (const key of Object.keys(map)) { - const hostWithoutPort = key.replace(/:\d+$/, ""); - if (hostWithoutPort === safeHostname) { - return key; - } - } - return undefined; -} - -/** - * Returns true on platforms where the OS keyring is supported (macOS, Windows). - */ -export function isKeyringSupported(): boolean { - return process.platform === "darwin" || process.platform === "win32"; -} - -/** - * Wraps @napi-rs/keyring with the credential encoding the Coder CLI expects. - * The native module is loaded lazily so Linux never touches it. - * - * Encoding (must match CLI): - * macOS: base64-encoded JSON via setPassword/getPassword - * Windows: raw UTF-8 JSON bytes via setSecret/getSecret - * - * Concurrency: setToken does read-modify-write on a shared entry, so concurrent - * writes can clobber each other. Callers recover by re-writing on reconnection. - */ -export class KeyringStore { - public constructor( - private readonly logger: Logger, - private readonly entryFactory: () => KeyringEntry = createDefaultEntry, - ) {} - - /** - * Store a token under the host extracted from deploymentUrl (includes port). - * The CLI stores map keys as host+port, so we must write in the same format. - */ - public setToken(deploymentUrl: string, token: string): void { - this.assertSupported(); - const entry = this.entryFactory(); - const map = this.readMap(entry); - const host = toHost(deploymentUrl); - map[host] = { coder_url: host, api_token: token }; - this.writeMap(entry, map); - } - - /** - * Look up a token by safeHostname (hostname without port). VS Code identifies - * deployments by safeHostname, so findMapKey bridges to the CLI's host+port keys. - */ - public getToken(safeHostname: string): string | undefined { - this.assertSupported(); - const entry = this.entryFactory(); - const map = this.readMap(entry); - const key = findMapKey(map, safeHostname); - return key !== undefined ? map[key].api_token : undefined; - } - - /** Remove a token by safeHostname, matching the same way as getToken. */ - public deleteToken(safeHostname: string): void { - this.assertSupported(); - const entry = this.entryFactory(); - const map = this.readMap(entry); - const key = findMapKey(map, safeHostname); - if (key === undefined) { - return; - } - delete map[key]; - if (Object.keys(map).length === 0) { - try { - entry.deleteCredential(); - } catch { - // NoEntry is fine — nothing to delete - } - } else { - this.writeMap(entry, map); - } - } - - private assertSupported(): void { - if (!isKeyringSupported()) { - throw new Error(`Keyring is not supported on ${process.platform}`); - } - } - - private readMap(entry: KeyringEntry): CredentialMap { - try { - const raw = this.readRaw(entry); - if (!raw) { - return {}; - } - return JSON.parse(raw) as CredentialMap; - } catch (error) { - this.logger.warn("Failed to read keyring credential map", error); - return {}; - } - } - - private readRaw(entry: KeyringEntry): string | null { - if (process.platform === "darwin") { - const password = entry.getPassword(); - return password !== null - ? Buffer.from(password, "base64").toString("utf-8") - : null; - } - if (process.platform === "win32") { - const secret = entry.getSecret(); - return secret !== null ? Buffer.from(secret).toString("utf-8") : null; - } - throw new Error(`Keyring is not supported on ${process.platform}`); - } - - private writeMap(entry: KeyringEntry, map: CredentialMap): void { - const json = JSON.stringify(map); - if (process.platform === "darwin") { - entry.setPassword(Buffer.from(json).toString("base64")); - return; - } - if (process.platform === "win32") { - entry.setSecret(Buffer.from(json)); - return; - } - throw new Error(`Keyring is not supported on ${process.platform}`); - } -} diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 8f7b3213..1a1a7e38 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -13,10 +13,11 @@ import { vscodeProposed } from "../vscodeProposed"; import type { User } from "coder/site/src/api/typesGenerated"; +import type { CliCredentialManager } from "../core/cliCredentialManager"; +import type { CliManager } from "../core/cliManager"; import type { MementoManager } from "../core/mementoManager"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; -import type { KeyringStore } from "../keyringStore"; import type { Logger } from "../logging/logger"; type LoginResult = @@ -41,7 +42,8 @@ export class LoginCoordinator implements vscode.Disposable { private readonly secretsManager: SecretsManager, private readonly mementoManager: MementoManager, private readonly logger: Logger, - private readonly keyringStore: KeyringStore, + private readonly cliCredentialManager: CliCredentialManager, + private readonly cliManager: CliManager, extensionId: string, ) { this.oauthAuthorizer = new OAuthAuthorizer( @@ -244,7 +246,7 @@ export class LoginCoordinator implements vscode.Disposable { } // Try keyring token (picks up tokens written by `coder login` in the terminal) - const keyringToken = this.getKeyringToken(deployment.safeHostname); + const keyringToken = await this.getCliKeyringToken(deployment); if ( keyringToken && keyringToken !== providedToken && @@ -303,14 +305,51 @@ export class LoginCoordinator implements vscode.Disposable { } } - private getKeyringToken(safeHostname: string): string | undefined { + /** + * Read a token from the CLI keyring. Fetches the CLI binary first (using + * an unauthenticated client) so the binary is available for keyring reads. + * Returns undefined if the keyring is disabled, the binary can't be fetched, + * or the CLI returns no token. + */ + private async getCliKeyringToken( + deployment: Deployment, + ): Promise { if (!isKeyringEnabled(vscode.workspace.getConfiguration())) { return undefined; } try { - return this.keyringStore.getToken(safeHostname); + const binPath = await this.ensureBinaryForKeyring( + deployment.url, + deployment.safeHostname, + ); + if (!binPath) { + return undefined; + } + return await this.cliCredentialManager.readToken( + binPath, + deployment.url, + vscode.workspace.getConfiguration(), + ); + } catch (error) { + this.logger.warn("Failed to read token from CLI keyring", error); + return undefined; + } + } + + /** + * Fetch or locate a CLI binary for the given deployment. Uses an + * unauthenticated client since getBuildInfo and binary downloads + * don't require auth. + */ + private async ensureBinaryForKeyring( + url: string, + safeHostname: string, + ): Promise { + try { + const client = CoderApi.create(url, "", this.logger); + return await this.cliManager.fetchBinary(client, safeHostname); } catch (error) { - this.logger.warn("Failed to read token from keyring", error); + this.logger.warn("Could not fetch CLI binary for keyring read:", error); return undefined; } } diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 0a659a19..87fe724d 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -252,6 +252,7 @@ export class Remote { baseUrlRaw, token, featureSet, + binaryPath, ); } @@ -268,6 +269,7 @@ export class Remote { auth.url, auth.token, featureSet, + binaryPath, ); this.logger.info( "Updated CLI config with new token for remote deployment", diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index e8f97f42..efe649cb 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -12,7 +12,7 @@ import type { User } from "coder/site/src/api/typesGenerated"; import type { IncomingMessage } from "node:http"; import type { CoderApi } from "@/api/coderApi"; -import type { KeyringStore } from "@/keyringStore"; +import type { CliCredentialManager } from "@/core/cliCredentialManager"; import type { Logger } from "@/logging/logger"; /** @@ -376,12 +376,11 @@ export class InMemorySecretStorage implements vscode.SecretStorage { } } -export function createMockKeyringStore(): KeyringStore { +export function createMockCliCredentialManager(): CliCredentialManager { return { - setToken: vi.fn(), - getToken: vi.fn().mockReturnValue(undefined), - deleteToken: vi.fn(), - } as unknown as KeyringStore; + storeToken: vi.fn().mockResolvedValue(undefined), + readToken: vi.fn().mockResolvedValue(undefined), + } as unknown as CliCredentialManager; } export function createMockLogger(): Logger { diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts new file mode 100644 index 00000000..cecfa97b --- /dev/null +++ b/test/unit/core/cliCredentialManager.test.ts @@ -0,0 +1,211 @@ +import { execFile } from "node:child_process"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + CliCredentialManager, + isKeyringSupported, +} from "@/core/cliCredentialManager"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +vi.mock("node:child_process", () => ({ + execFile: vi.fn(), +})); + +function stubPlatform(platform: string) { + vi.stubGlobal("process", { ...process, platform }); +} + +function mockExecFileSuccess(stdout = "") { + vi.mocked(execFile).mockImplementation( + (_cmd, _args, _opts, callback?: unknown) => { + // promisify(execFile) calls execFile with a callback as last argument + const cb = + typeof _opts === "function" + ? (_opts as (err: Error | null, result: { stdout: string }) => void) + : (callback as + | ((err: Error | null, result: { stdout: string }) => void) + | undefined); + if (cb) { + cb(null, { stdout }); + } + return {} as ReturnType; + }, + ); +} + +function mockExecFileFailure(message: string) { + vi.mocked(execFile).mockImplementation( + (_cmd, _args, _opts, callback?: unknown) => { + const cb = + typeof _opts === "function" + ? (_opts as (err: Error | null) => void) + : (callback as ((err: Error | null) => void) | undefined); + if (cb) { + cb(new Error(message)); + } + return {} as ReturnType; + }, + ); +} + +const mockConfigs = { + get: vi.fn().mockReturnValue(undefined), +}; + +describe("isKeyringSupported", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it.each([ + { platform: "darwin", expected: true }, + { platform: "win32", expected: true }, + { platform: "linux", expected: false }, + { platform: "freebsd", expected: false }, + ])("returns $expected for $platform", ({ platform, expected }) => { + stubPlatform(platform); + expect(isKeyringSupported()).toBe(expected); + }); +}); + +describe("CliCredentialManager", () => { + afterEach(() => { + vi.resetAllMocks(); + }); + + describe("storeToken", () => { + it("passes token via CODER_SESSION_TOKEN env var", async () => { + mockExecFileSuccess(); + const manager = new CliCredentialManager(createMockLogger()); + + await manager.storeToken( + "/usr/bin/coder", + "https://dev.coder.com", + "my-secret-token", + mockConfigs, + ); + + expect(execFile).toHaveBeenCalledWith( + "/usr/bin/coder", + ["login", "--use-token-as-session", "https://dev.coder.com"], + expect.objectContaining({ + env: expect.objectContaining({ + CODER_SESSION_TOKEN: "my-secret-token", + }), + }), + expect.any(Function), + ); + }); + + it("never passes token in args", async () => { + mockExecFileSuccess(); + const manager = new CliCredentialManager(createMockLogger()); + + await manager.storeToken( + "/usr/bin/coder", + "https://dev.coder.com", + "my-secret-token", + mockConfigs, + ); + + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; + expect(args).not.toContain("my-secret-token"); + }); + + it("throws when CLI fails", async () => { + mockExecFileFailure("login failed"); + const manager = new CliCredentialManager(createMockLogger()); + + await expect( + manager.storeToken( + "/usr/bin/coder", + "https://dev.coder.com", + "token", + mockConfigs, + ), + ).rejects.toThrow("login failed"); + }); + + it("includes header args when header command is set", async () => { + mockExecFileSuccess(); + const configWithHeaders = { + get: vi.fn((key: string) => + key === "coder.headerCommand" ? "my-header-cmd" : undefined, + ), + }; + const manager = new CliCredentialManager(createMockLogger()); + + await manager.storeToken( + "/usr/bin/coder", + "https://dev.coder.com", + "token", + configWithHeaders, + ); + + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; + expect(args).toContain("--header-command"); + }); + }); + + describe("readToken", () => { + it("returns trimmed stdout", async () => { + mockExecFileSuccess(" my-token\n"); + const manager = new CliCredentialManager(createMockLogger()); + + const token = await manager.readToken( + "/usr/bin/coder", + "https://dev.coder.com", + mockConfigs, + ); + + expect(token).toBe("my-token"); + }); + + it("returns undefined on empty stdout", async () => { + mockExecFileSuccess(" \n"); + const manager = new CliCredentialManager(createMockLogger()); + + const token = await manager.readToken( + "/usr/bin/coder", + "https://dev.coder.com", + mockConfigs, + ); + + expect(token).toBeUndefined(); + }); + + it("returns undefined on error", async () => { + mockExecFileFailure("no token found"); + const manager = new CliCredentialManager(createMockLogger()); + + const token = await manager.readToken( + "/usr/bin/coder", + "https://dev.coder.com", + mockConfigs, + ); + + expect(token).toBeUndefined(); + }); + + it("passes correct args", async () => { + mockExecFileSuccess("token"); + const manager = new CliCredentialManager(createMockLogger()); + + await manager.readToken( + "/usr/bin/coder", + "https://dev.coder.com", + mockConfigs, + ); + + const call = vi.mocked(execFile).mock.calls[0]; + expect(call[0]).toBe("/usr/bin/coder"); + expect(call[1]).toEqual([ + "login", + "token", + "--url", + "https://dev.coder.com", + ]); + }); + }); +}); diff --git a/test/unit/core/cliManager.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts index 5d32f0a2..0a6cd804 100644 --- a/test/unit/core/cliManager.concurrent.test.ts +++ b/test/unit/core/cliManager.concurrent.test.ts @@ -18,7 +18,7 @@ import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { - createMockKeyringStore, + createMockCliCredentialManager, createMockLogger, createMockStream, MockConfigurationProvider, @@ -83,7 +83,7 @@ function setupManager(testDir: string): CliManager { return new CliManager( createMockLogger(), new PathResolver(testDir, "/code/log"), - createMockKeyringStore(), + createMockCliCredentialManager(), ); } diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 2d386244..1d9dac2f 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -16,7 +16,7 @@ import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { - createMockKeyringStore, + createMockCliCredentialManager, createMockLogger, createMockStream, MockConfigurationProvider, @@ -25,8 +25,8 @@ import { } from "../../mocks/testHelpers"; import { expectPathsEqual } from "../../utils/platform"; +import type { CliCredentialManager } from "@/core/cliCredentialManager"; import type { FeatureSet } from "@/featureSet"; -import type { KeyringStore } from "@/keyringStore"; vi.mock("os"); vi.mock("axios"); @@ -59,7 +59,6 @@ vi.mock("@/cliConfig", async () => { return { ...actual, shouldUseKeyring: vi.fn(), - isKeyringEnabled: vi.fn(), }; }); @@ -86,7 +85,7 @@ describe("CliManager", () => { let mockUI: MockUserInteraction; let mockApi: Api; let mockAxios: AxiosInstance; - let mockKeyring: KeyringStore; + let mockCredManager: CliCredentialManager; const TEST_VERSION = "1.2.3"; const TEST_URL = "https://test.coder.com"; @@ -115,18 +114,17 @@ describe("CliManager", () => { mockConfig = new MockConfigurationProvider(); mockProgress = new MockProgressReporter(); mockUI = new MockUserInteraction(); - mockKeyring = createMockKeyringStore(); + mockCredManager = createMockCliCredentialManager(); manager = new CliManager( createMockLogger(), new PathResolver(BASE_PATH, "/code/log"), - mockKeyring, + mockCredManager, ); // Mock only what's necessary vi.mocked(os.platform).mockReturnValue(PLATFORM); vi.mocked(os.arch).mockReturnValue(ARCH); vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(true); }); afterEach(async () => { @@ -138,12 +136,15 @@ describe("CliManager", () => { }); describe("Configure CLI", () => { + const TEST_BIN = "/usr/bin/coder"; + function configure(token = "test-token") { return manager.configure( "dev.coder.com", "https://coder.example.com", token, MOCK_FEATURE_SET, + TEST_BIN, ); } @@ -160,7 +161,13 @@ describe("CliManager", () => { it("should throw when URL is empty", async () => { await expect( - manager.configure("dev.coder.com", "", "test-token", MOCK_FEATURE_SET), + manager.configure( + "dev.coder.com", + "", + "test-token", + MOCK_FEATURE_SET, + TEST_BIN, + ), ).rejects.toThrow("URL is required to configure the CLI"); }); @@ -181,6 +188,7 @@ describe("CliManager", () => { "https://coder.example.com", "token", MOCK_FEATURE_SET, + TEST_BIN, ); expect(memfs.readFileSync("/path/base/url", "utf8")).toBe( @@ -189,24 +197,26 @@ describe("CliManager", () => { expect(memfs.readFileSync("/path/base/session", "utf8")).toBe("token"); }); - it("should write to keyring and skip files when featureSet enables keyring", async () => { + it("should store via CLI credential manager when keyring enabled", async () => { vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); await configure("test-token"); - expect(mockKeyring.setToken).toHaveBeenCalledWith( + expect(mockCredManager.storeToken).toHaveBeenCalledWith( + TEST_BIN, "https://coder.example.com", "test-token", + expect.anything(), ); expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); }); - it("should throw and show error when keyring write fails", async () => { + it("should throw and show error when CLI keyring store fails", async () => { vi.mocked(cliConfig.shouldUseKeyring).mockReturnValue(true); - vi.mocked(mockKeyring.setToken).mockImplementation(() => { - throw new Error("keyring unavailable"); - }); + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( + new Error("keyring unavailable"), + ); await expect(configure("test-token")).rejects.toThrow( "keyring unavailable", @@ -231,27 +241,8 @@ describe("CliManager", () => { ); } - interface RemoveCredentialsCase { - scenario: string; - setup: () => void; - } - it.each([ - { scenario: "normally", setup: () => {} }, - { - scenario: "when keyring delete fails", - setup: () => - vi.mocked(mockKeyring.deleteToken).mockImplementation(() => { - throw new Error("keyring unavailable"); - }), - }, - { - scenario: "when keyring is disabled", - setup: () => - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false), - }, - ])("should remove credential files $scenario", async ({ setup }) => { + it("should remove credential files", async () => { seedCredentialFiles(); - setup(); await manager.clearCredentials("dev.coder.com"); expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); @@ -261,13 +252,6 @@ describe("CliManager", () => { await expect( manager.clearCredentials("dev.coder.com"), ).resolves.not.toThrow(); - expect(mockKeyring.deleteToken).toHaveBeenCalledWith("dev.coder.com"); - }); - - it("should skip keyring delete when keyring is disabled", async () => { - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false); - await manager.clearCredentials("dev.coder.com"); - expect(mockKeyring.deleteToken).not.toHaveBeenCalled(); }); }); @@ -709,7 +693,7 @@ describe("CliManager", () => { const manager = new CliManager( createMockLogger(), resolver, - createMockKeyringStore(), + createMockCliCredentialManager(), ); withSuccessfulDownload(); diff --git a/test/unit/keyringStore.test.ts b/test/unit/keyringStore.test.ts deleted file mode 100644 index f273fc00..00000000 --- a/test/unit/keyringStore.test.ts +++ /dev/null @@ -1,231 +0,0 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; - -import { - type KeyringEntry, - KeyringStore, - isKeyringSupported, -} from "@/keyringStore"; - -import { createMockLogger } from "../mocks/testHelpers"; - -/** - * In-memory backing store that simulates the OS keyring. - * Each call to `factory()` returns a fresh handle pointing to the same - * shared state, matching real @napi-rs/keyring behavior where - * `Entry.withTarget()` returns a new handle to the same credential. - */ -function createMockEntryFactory() { - let password: string | null = null; - let secret: Uint8Array | null = null; - - return { - factory: (): KeyringEntry => ({ - getPassword: () => password, - setPassword: (p: string) => { - password = p; - }, - getSecret: () => secret, - setSecret: (s: Uint8Array) => { - secret = s; - }, - deleteCredential: () => { - password = null; - secret = null; - }, - }), - getRawPassword: () => password, - getRawSecret: () => secret, - hasCredential: () => password !== null || secret !== null, - }; -} - -function stubPlatform(platform: string) { - vi.stubGlobal("process", { ...process, platform }); -} - -function createTestContext() { - const mockEntry = createMockEntryFactory(); - const store = new KeyringStore(createMockLogger(), mockEntry.factory); - return { store, mockEntry }; -} - -describe("isKeyringSupported", () => { - afterEach(() => { - vi.unstubAllGlobals(); - }); - - it.each([ - { platform: "darwin", expected: true }, - { platform: "win32", expected: true }, - { platform: "linux", expected: false }, - { platform: "freebsd", expected: false }, - ])("returns $expected for $platform", ({ platform, expected }) => { - stubPlatform(platform); - expect(isKeyringSupported()).toBe(expected); - }); -}); - -describe("KeyringStore", () => { - afterEach(() => { - vi.unstubAllGlobals(); - }); - - // CRUD behavior is platform-independent; darwin is used as the test platform. - describe("token operations", () => { - it("sets and gets a token", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "my-token"); - expect(store.getToken("dev.coder.com")).toBe("my-token"); - }); - - it("overwrites token for same deployment", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "old-token"); - store.setToken("https://dev.coder.com", "new-token"); - expect(store.getToken("dev.coder.com")).toBe("new-token"); - }); - - it("preserves other deployments on set", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token-1"); - store.setToken("https://staging.coder.com", "token-2"); - - expect(store.getToken("dev.coder.com")).toBe("token-1"); - expect(store.getToken("staging.coder.com")).toBe("token-2"); - }); - - it("returns undefined for missing deployment", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - expect(store.getToken("unknown.coder.com")).toBeUndefined(); - }); - - it("deletes token while preserving others", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token-1"); - store.setToken("https://staging.coder.com", "token-2"); - - store.deleteToken("dev.coder.com"); - - expect(store.getToken("dev.coder.com")).toBeUndefined(); - expect(store.getToken("staging.coder.com")).toBe("token-2"); - }); - - it("deletes entire credential when last token is removed", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token-1"); - store.deleteToken("dev.coder.com"); - expect(store.getToken("dev.coder.com")).toBeUndefined(); - // OS keyring entry itself should be cleaned up, not left as empty JSON - expect(mockEntry.hasCredential()).toBe(false); - }); - - it("handles delete of non-existent deployment gracefully", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token-1"); - store.deleteToken("unknown.coder.com"); - expect(store.getToken("dev.coder.com")).toBe("token-1"); - }); - - it("strips URL path and protocol, keeping only host", () => { - const { store } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com/some/path", "my-token"); - expect(store.getToken("dev.coder.com")).toBe("my-token"); - }); - - it("matches safeHostname to map key with port", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com:3000", "my-token"); - // safeHostname (port stripped) finds the entry stored with port - expect(store.getToken("dev.coder.com")).toBe("my-token"); - // Used the hostname + port should also work - expect(store.getToken("dev.coder.com:3000")).toBe("my-token"); - - store.deleteToken("dev.coder.com"); - expect(store.getToken("dev.coder.com")).toBeUndefined(); - expect(mockEntry.hasCredential()).toBe(false); - }); - }); - - describe("platform encoding", () => { - it("macOS: base64-encoded JSON via password", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token"); - const decoded = JSON.parse( - Buffer.from(mockEntry.getRawPassword()!, "base64").toString("utf-8"), - ); - expect(decoded).toEqual({ - "dev.coder.com": { coder_url: "dev.coder.com", api_token: "token" }, - }); - // Secret must be untouched; reading uses getPassword, not getSecret. - expect(mockEntry.getRawSecret()).toBeNull(); - expect(store.getToken("dev.coder.com")).toBe("token"); - }); - - it("macOS: returns undefined for corrupted credential", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com", "token"); - mockEntry - .factory() - .setPassword(Buffer.from("not-valid-json").toString("base64")); - expect(store.getToken("dev.coder.com")).toBeUndefined(); - }); - - it("Windows: raw UTF-8 JSON via secret", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("win32"); - store.setToken("https://dev.coder.com", "token"); - const decoded = JSON.parse( - Buffer.from(mockEntry.getRawSecret()!).toString("utf-8"), - ); - expect(decoded).toEqual({ - "dev.coder.com": { coder_url: "dev.coder.com", api_token: "token" }, - }); - // Password must be untouched; reading uses getSecret, not getPassword. - expect(mockEntry.getRawPassword()).toBeNull(); - expect(store.getToken("dev.coder.com")).toBe("token"); - }); - - it("Windows: returns undefined for corrupted credential", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("win32"); - store.setToken("https://dev.coder.com", "token"); - mockEntry.factory().setSecret(Buffer.from("not-valid-json")); - expect(store.getToken("dev.coder.com")).toBeUndefined(); - }); - - it("preserves port in map key for CLI compatibility", () => { - const { store, mockEntry } = createTestContext(); - stubPlatform("darwin"); - store.setToken("https://dev.coder.com:8080", "my-token"); - const decoded = JSON.parse( - Buffer.from(mockEntry.getRawPassword()!, "base64").toString("utf-8"), - ); - expect(decoded).toEqual({ - "dev.coder.com:8080": { - coder_url: "dev.coder.com:8080", - api_token: "my-token", - }, - }); - }); - - it("throws on unsupported platform", () => { - const { store } = createTestContext(); - stubPlatform("linux"); - const msg = "Keyring is not supported on linux"; - expect(() => store.setToken("https://dev.coder.com", "t")).toThrow(msg); - expect(() => store.getToken("dev.coder.com")).toThrow(msg); - expect(() => store.deleteToken("dev.coder.com")).toThrow(msg); - }); - }); -}); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 7d7a345f..3a6db752 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -10,7 +10,7 @@ import { maybeAskAuthMethod } from "@/promptUtils"; import { createAxiosError, - createMockKeyringStore, + createMockCliCredentialManager, createMockLogger, createMockUser, InMemoryMemento, @@ -118,11 +118,15 @@ function createTestContext() { const secretsManager = new SecretsManager(secretStorage, memento, logger); const mementoManager = new MementoManager(memento); + const mockCliManager = { + fetchBinary: vi.fn().mockRejectedValue(new Error("no binary")), + }; const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, - createMockKeyringStore(), + createMockCliCredentialManager(), + mockCliManager as unknown as import("@/core/cliManager").CliManager, "coder.coder-remote", ); @@ -307,11 +311,15 @@ describe("LoginCoordinator", () => { mockConfig.set("coder.tlsKeyFile", "/path/to/key.pem"); const logger = createMockLogger(); + const mockCliManager2 = { + fetchBinary: vi.fn().mockRejectedValue(new Error("no binary")), + }; const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, - createMockKeyringStore(), + createMockCliCredentialManager(), + mockCliManager2 as unknown as import("@/core/cliManager").CliManager, "coder.coder-remote", ); From c83964f2cf3e72b8dbbd93dfb0dda01db0a7db22 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 4 Mar 2026 01:46:29 +0300 Subject: [PATCH 2/4] refactor: derive safeHostname internally from url instead of passing both Make url the single source of truth for deployment identity. fetchBinary, configure, and clearCredentials now derive safeHostname via toSafeHost() internally, eliminating redundant parameters and preventing inconsistency. BinaryResolver and CliCredentialManager methods take url string instead of Deployment object. --- src/commands.ts | 18 +- src/core/cliCredentialManager.ts | 58 +++++- src/core/cliManager.ts | 30 +-- src/core/container.ts | 10 +- src/login/loginCoordinator.ts | 50 +---- src/remote/remote.ts | 12 +- test/mocks/testHelpers.ts | 1 + test/unit/core/cliCredentialManager.test.ts | 183 +++++++++++++------ test/unit/core/cliManager.concurrent.test.ts | 14 +- test/unit/core/cliManager.test.ts | 152 ++++++++------- test/unit/login/loginCoordinator.test.ts | 8 - 11 files changed, 316 insertions(+), 220 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 366e9f49..b350773d 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -214,13 +214,12 @@ export class Commands { this.logger.debug("Logging out"); const deployment = this.deploymentManager.getCurrentDeployment(); - const safeHostname = deployment?.safeHostname; await this.deploymentManager.clearDeployment(); - if (safeHostname) { - await this.cliManager.clearCredentials(safeHostname); - await this.secretsManager.clearAllAuthData(safeHostname); + if (deployment) { + await this.cliManager.clearCredentials(deployment.url); + await this.secretsManager.clearAllAuthData(deployment.safeHostname); } vscode.window @@ -287,7 +286,10 @@ export class Commands { if (selected.hostnames.length === 1) { const selectedHostname = selected.hostnames[0]; - await this.cliManager.clearCredentials(selectedHostname); + const auth = await this.secretsManager.getSessionAuth(selectedHostname); + if (auth?.url) { + await this.cliManager.clearCredentials(auth.url); + } await this.secretsManager.clearAllAuthData(selectedHostname); this.logger.info("Removed credentials for", selectedHostname); vscode.window.showInformationMessage( @@ -306,7 +308,10 @@ export class Commands { if (confirm === "Remove All") { await Promise.all( selected.hostnames.map(async (h) => { - await this.cliManager.clearCredentials(h); + const auth = await this.secretsManager.getSessionAuth(h); + if (auth?.url) { + await this.cliManager.clearCredentials(auth.url); + } await this.secretsManager.clearAllAuthData(h); }), ); @@ -455,7 +460,6 @@ export class Commands { const safeHost = toSafeHost(baseUrl); const binary = await this.cliManager.fetchBinary( this.extensionClient, - safeHost, ); const version = semver.parse(await cliUtils.version(binary)); diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index dcd8f3b5..ae051b72 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -9,6 +9,12 @@ import type { Logger } from "../logging/logger"; const execFileAsync = promisify(execFile); +/** + * Resolves a CLI binary path for a given deployment URL, fetching/downloading + * if needed. Returns the path or throws if unavailable. + */ +export type BinaryResolver = (url: string) => Promise; + /** * Returns true on platforms where the OS keyring is supported (macOS, Windows). */ @@ -18,9 +24,15 @@ export function isKeyringSupported(): boolean { /** * Delegates credential storage to the Coder CLI to keep the credentials in sync. + * + * For operations that don't have a binary path available (readToken, deleteToken), + * uses the injected BinaryResolver to fetch/locate the CLI binary. */ export class CliCredentialManager { - constructor(private readonly logger: Logger) {} + constructor( + private readonly logger: Logger, + private readonly resolveBinary: BinaryResolver, + ) {} /** * Store a token by running: @@ -29,7 +41,7 @@ export class CliCredentialManager { * The token is passed via environment variable so it never appears in * process argument lists. */ - async storeToken( + public async storeToken( binPath: string, url: string, token: string, @@ -56,13 +68,21 @@ export class CliCredentialManager { * Read a token by running: * login token --url * - * Returns trimmed stdout, or undefined on any failure. + * Resolves the CLI binary automatically. Returns trimmed stdout, + * or undefined if the binary can't be resolved or the CLI returns no token. */ - async readToken( - binPath: string, + public async readToken( url: string, configs: Pick, ): Promise { + let binPath: string; + try { + binPath = await this.resolveBinary(url); + } catch (error) { + this.logger.warn("Could not resolve CLI binary for token read:", error); + return undefined; + } + const args = [...getHeaderArgs(configs), "login", "token", "--url", url]; try { const { stdout } = await execFileAsync(binPath, args); @@ -73,4 +93,32 @@ export class CliCredentialManager { return undefined; } } + + /** + * Delete a token by running: + * logout --url + * + * Resolves the CLI binary automatically. Best-effort: logs warnings + * on failure but never throws. + */ + async deleteToken( + url: string, + configs: Pick, + ): Promise { + let binPath: string; + try { + binPath = await this.resolveBinary(url); + } catch (error) { + this.logger.warn("Could not resolve CLI binary for token delete:", error); + return; + } + + const args = [...getHeaderArgs(configs), "logout", "--url", url, "--yes"]; + try { + await execFileAsync(binPath, args); + this.logger.info("Deleted token via CLI for", url); + } catch (error) { + this.logger.warn("Failed to delete token via CLI:", error); + } + } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 474702b3..9007397a 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,8 +10,9 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; -import { shouldUseKeyring } from "../cliConfig"; +import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; +import { toSafeHost } from "../util"; import { vscodeProposed } from "../vscodeProposed"; import { BinaryLock } from "./binaryLock"; @@ -49,10 +50,12 @@ export class CliManager { * unable to download a working binary, whether because of network issues or * downloads being disabled. */ - public async fetchBinary( - restClient: Api, - safeHostname: string, - ): Promise { + public async fetchBinary(restClient: Api): Promise { + const baseUrl = restClient.getAxiosInstance().defaults.baseURL; + if (!baseUrl) { + throw new Error("REST client has no base URL configured"); + } + const safeHostname = toSafeHost(baseUrl); const cfg = vscode.workspace.getConfiguration("coder"); // Settings can be undefined when set to their defaults (true in this case), // so explicitly check against false. @@ -719,7 +722,6 @@ export class CliManager { * authentication) but the URL must be a non-empty string. */ public async configure( - safeHostname: string, url: string, token: string, featureSet: FeatureSet, @@ -728,6 +730,7 @@ export class CliManager { if (!url) { throw new Error("URL is required to configure the CLI"); } + const safeHostname = toSafeHost(url); const configs = vscode.workspace.getConfiguration(); if (shouldUseKeyring(configs, featureSet)) { @@ -765,12 +768,17 @@ export class CliManager { } /** - * Remove file-based credentials for a deployment. Keyring entries are not - * removed here because deleting requires the CLI binary, which may not be - * available at logout time. This is fine: stale keyring entries are harmless - * since the CLI overwrites them on next `coder login`. + * Remove credentials for a deployment. Clears both file-based credentials + * and keyring entries (via `coder logout`). Keyring deletion is best-effort: + * if it fails, file cleanup still runs. */ - public async clearCredentials(safeHostname: string): Promise { + public async clearCredentials(url: string): Promise { + const safeHostname = toSafeHost(url); + const configs = vscode.workspace.getConfiguration(); + if (isKeyringEnabled(configs)) { + await this.cliCredentialManager.deleteToken(url, configs); + } + const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); const urlPath = this.pathResolver.getUrlPath(safeHostname); await Promise.all([ diff --git a/src/core/container.ts b/src/core/container.ts index 5b7c5953..143687a3 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -1,5 +1,6 @@ import * as vscode from "vscode"; +import { CoderApi } from "../api/coderApi"; import { type Logger } from "../logging/logger"; import { LoginCoordinator } from "../login/loginCoordinator"; @@ -36,7 +37,13 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); - this.cliCredentialManager = new CliCredentialManager(this.logger); + this.cliCredentialManager = new CliCredentialManager( + this.logger, + async (url) => { + const client = CoderApi.create(url, "", this.logger); + return this.cliManager.fetchBinary(client); + }, + ); this.cliManager = new CliManager( this.logger, this.pathResolver, @@ -48,7 +55,6 @@ export class ServiceContainer implements vscode.Disposable { this.mementoManager, this.logger, this.cliCredentialManager, - this.cliManager, context.extension.id, ); } diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 1a1a7e38..cdaa8db6 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -4,7 +4,6 @@ import * as vscode from "vscode"; import { CoderApi } from "../api/coderApi"; import { needToken } from "../api/utils"; -import { isKeyringEnabled } from "../cliConfig"; import { CertificateError } from "../error/certificateError"; import { OAuthAuthorizer } from "../oauth/authorizer"; import { buildOAuthTokenData } from "../oauth/utils"; @@ -14,7 +13,6 @@ import { vscodeProposed } from "../vscodeProposed"; import type { User } from "coder/site/src/api/typesGenerated"; import type { CliCredentialManager } from "../core/cliCredentialManager"; -import type { CliManager } from "../core/cliManager"; import type { MementoManager } from "../core/mementoManager"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; @@ -43,7 +41,6 @@ export class LoginCoordinator implements vscode.Disposable { private readonly mementoManager: MementoManager, private readonly logger: Logger, private readonly cliCredentialManager: CliCredentialManager, - private readonly cliManager: CliManager, extensionId: string, ) { this.oauthAuthorizer = new OAuthAuthorizer( @@ -306,52 +303,15 @@ export class LoginCoordinator implements vscode.Disposable { } /** - * Read a token from the CLI keyring. Fetches the CLI binary first (using - * an unauthenticated client) so the binary is available for keyring reads. - * Returns undefined if the keyring is disabled, the binary can't be fetched, - * or the CLI returns no token. + * Read a token from the CLI keyring. */ private async getCliKeyringToken( deployment: Deployment, ): Promise { - if (!isKeyringEnabled(vscode.workspace.getConfiguration())) { - return undefined; - } - try { - const binPath = await this.ensureBinaryForKeyring( - deployment.url, - deployment.safeHostname, - ); - if (!binPath) { - return undefined; - } - return await this.cliCredentialManager.readToken( - binPath, - deployment.url, - vscode.workspace.getConfiguration(), - ); - } catch (error) { - this.logger.warn("Failed to read token from CLI keyring", error); - return undefined; - } - } - - /** - * Fetch or locate a CLI binary for the given deployment. Uses an - * unauthenticated client since getBuildInfo and binary downloads - * don't require auth. - */ - private async ensureBinaryForKeyring( - url: string, - safeHostname: string, - ): Promise { - try { - const client = CoderApi.create(url, "", this.logger); - return await this.cliManager.fetchBinary(client, safeHostname); - } catch (error) { - this.logger.warn("Could not fetch CLI binary for keyring read:", error); - return undefined; - } + return this.cliCredentialManager.readToken( + deployment.url, + vscode.workspace.getConfiguration(), + ); } /** diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 87fe724d..12b94bd8 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -215,10 +215,7 @@ export class Remote { if ( this.extensionContext.extensionMode === vscode.ExtensionMode.Production ) { - binaryPath = await this.cliManager.fetchBinary( - workspaceClient, - parts.safeHostname, - ); + binaryPath = await this.cliManager.fetchBinary(workspaceClient); } else { try { // In development, try to use `/tmp/coder` as the binary path. @@ -226,10 +223,7 @@ export class Remote { binaryPath = path.join(os.tmpdir(), "coder"); await fs.stat(binaryPath); } catch { - binaryPath = await this.cliManager.fetchBinary( - workspaceClient, - parts.safeHostname, - ); + binaryPath = await this.cliManager.fetchBinary(workspaceClient); } } @@ -248,7 +242,6 @@ export class Remote { // Write token to keyring or file (after CLI version is known) if (baseUrlRaw && token !== undefined) { await this.cliManager.configure( - parts.safeHostname, baseUrlRaw, token, featureSet, @@ -265,7 +258,6 @@ export class Remote { if (auth?.url) { try { await this.cliManager.configure( - parts.safeHostname, auth.url, auth.token, featureSet, diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index efe649cb..631a1282 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -380,6 +380,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { return { storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), + deleteToken: vi.fn().mockResolvedValue(undefined), } as unknown as CliCredentialManager; } diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index cecfa97b..e06987f7 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -4,6 +4,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { CliCredentialManager, isKeyringSupported, + type BinaryResolver, } from "@/core/cliCredentialManager"; import { createMockLogger } from "../../mocks/testHelpers"; @@ -53,6 +54,19 @@ const mockConfigs = { get: vi.fn().mockReturnValue(undefined), }; +const TEST_BIN = "/usr/bin/coder"; +const TEST_URL = "https://dev.coder.com"; + +function createSuccessResolver(): BinaryResolver { + return vi.fn().mockResolvedValue(TEST_BIN) as unknown as BinaryResolver; +} + +function createFailingResolver(): BinaryResolver { + return vi + .fn() + .mockRejectedValue(new Error("no binary")) as unknown as BinaryResolver; +} + describe("isKeyringSupported", () => { afterEach(() => { vi.unstubAllGlobals(); @@ -77,18 +91,21 @@ describe("CliCredentialManager", () => { describe("storeToken", () => { it("passes token via CODER_SESSION_TOKEN env var", async () => { mockExecFileSuccess(); - const manager = new CliCredentialManager(createMockLogger()); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); await manager.storeToken( - "/usr/bin/coder", - "https://dev.coder.com", + TEST_BIN, + TEST_URL, "my-secret-token", mockConfigs, ); expect(execFile).toHaveBeenCalledWith( - "/usr/bin/coder", - ["login", "--use-token-as-session", "https://dev.coder.com"], + TEST_BIN, + ["login", "--use-token-as-session", TEST_URL], expect.objectContaining({ env: expect.objectContaining({ CODER_SESSION_TOKEN: "my-secret-token", @@ -100,11 +117,14 @@ describe("CliCredentialManager", () => { it("never passes token in args", async () => { mockExecFileSuccess(); - const manager = new CliCredentialManager(createMockLogger()); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); await manager.storeToken( - "/usr/bin/coder", - "https://dev.coder.com", + TEST_BIN, + TEST_URL, "my-secret-token", mockConfigs, ); @@ -115,15 +135,13 @@ describe("CliCredentialManager", () => { it("throws when CLI fails", async () => { mockExecFileFailure("login failed"); - const manager = new CliCredentialManager(createMockLogger()); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); await expect( - manager.storeToken( - "/usr/bin/coder", - "https://dev.coder.com", - "token", - mockConfigs, - ), + manager.storeToken(TEST_BIN, TEST_URL, "token", mockConfigs), ).rejects.toThrow("login failed"); }); @@ -134,78 +152,135 @@ describe("CliCredentialManager", () => { key === "coder.headerCommand" ? "my-header-cmd" : undefined, ), }; - const manager = new CliCredentialManager(createMockLogger()); - - await manager.storeToken( - "/usr/bin/coder", - "https://dev.coder.com", - "token", - configWithHeaders, + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), ); + await manager.storeToken(TEST_BIN, TEST_URL, "token", configWithHeaders); + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; expect(args).toContain("--header-command"); }); }); describe("readToken", () => { - it("returns trimmed stdout", async () => { + it("resolves binary and returns trimmed stdout", async () => { mockExecFileSuccess(" my-token\n"); - const manager = new CliCredentialManager(createMockLogger()); + const resolver = createSuccessResolver(); + const manager = new CliCredentialManager(createMockLogger(), resolver); - const token = await manager.readToken( - "/usr/bin/coder", - "https://dev.coder.com", - mockConfigs, - ); + const token = await manager.readToken(TEST_URL, mockConfigs); + expect(resolver).toHaveBeenCalledWith(TEST_URL); expect(token).toBe("my-token"); }); it("returns undefined on empty stdout", async () => { mockExecFileSuccess(" \n"); - const manager = new CliCredentialManager(createMockLogger()); - - const token = await manager.readToken( - "/usr/bin/coder", - "https://dev.coder.com", - mockConfigs, + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), ); + const token = await manager.readToken(TEST_URL, mockConfigs); + expect(token).toBeUndefined(); }); - it("returns undefined on error", async () => { + it("returns undefined on CLI error", async () => { mockExecFileFailure("no token found"); - const manager = new CliCredentialManager(createMockLogger()); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); - const token = await manager.readToken( - "/usr/bin/coder", - "https://dev.coder.com", - mockConfigs, + const token = await manager.readToken(TEST_URL, mockConfigs); + + expect(token).toBeUndefined(); + }); + + it("returns undefined when binary resolver fails", async () => { + const manager = new CliCredentialManager( + createMockLogger(), + createFailingResolver(), ); + const token = await manager.readToken(TEST_URL, mockConfigs); + expect(token).toBeUndefined(); + expect(execFile).not.toHaveBeenCalled(); }); it("passes correct args", async () => { mockExecFileSuccess("token"); - const manager = new CliCredentialManager(createMockLogger()); - - await manager.readToken( - "/usr/bin/coder", - "https://dev.coder.com", - mockConfigs, + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), ); + await manager.readToken(TEST_URL, mockConfigs); + + const call = vi.mocked(execFile).mock.calls[0]; + expect(call[0]).toBe(TEST_BIN); + expect(call[1]).toEqual(["login", "token", "--url", TEST_URL]); + }); + }); + + describe("deleteToken", () => { + it("resolves binary and runs coder logout", async () => { + mockExecFileSuccess(); + const resolver = createSuccessResolver(); + const manager = new CliCredentialManager(createMockLogger(), resolver); + + await manager.deleteToken(TEST_URL, mockConfigs); + + expect(resolver).toHaveBeenCalledWith(TEST_URL); const call = vi.mocked(execFile).mock.calls[0]; - expect(call[0]).toBe("/usr/bin/coder"); - expect(call[1]).toEqual([ - "login", - "token", - "--url", - "https://dev.coder.com", - ]); + expect(call[0]).toBe(TEST_BIN); + expect(call[1]).toEqual(["logout", "--url", TEST_URL, "--yes"]); + }); + + it("does not throw when CLI fails", async () => { + mockExecFileFailure("logout failed"); + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); + + await expect( + manager.deleteToken(TEST_URL, mockConfigs), + ).resolves.not.toThrow(); + }); + + it("does not throw when binary resolver fails", async () => { + const manager = new CliCredentialManager( + createMockLogger(), + createFailingResolver(), + ); + + await expect( + manager.deleteToken(TEST_URL, mockConfigs), + ).resolves.not.toThrow(); + expect(execFile).not.toHaveBeenCalled(); + }); + + it("includes header args when header command is set", async () => { + mockExecFileSuccess(); + const configWithHeaders = { + get: vi.fn((key: string) => + key === "coder.headerCommand" ? "my-header-cmd" : undefined, + ), + }; + const manager = new CliCredentialManager( + createMockLogger(), + createSuccessResolver(), + ); + + await manager.deleteToken(TEST_URL, configWithHeaders); + + const args = vi.mocked(execFile).mock.calls[0][1] as string[]; + expect(args).toContain("--header-command"); }); }); }); diff --git a/test/unit/core/cliManager.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts index 0a6cd804..fe0b0fae 100644 --- a/test/unit/core/cliManager.concurrent.test.ts +++ b/test/unit/core/cliManager.concurrent.test.ts @@ -109,9 +109,9 @@ describe("CliManager Concurrent Downloads", () => { const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); const downloads = await Promise.all([ - manager.fetchBinary(mockApi, label), - manager.fetchBinary(mockApi, label), - manager.fetchBinary(mockApi, label), + manager.fetchBinary(mockApi), + manager.fetchBinary(mockApi), + manager.fetchBinary(mockApi), ]); expect(downloads).toHaveLength(3); @@ -143,14 +143,14 @@ describe("CliManager Concurrent Downloads", () => { const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); // Start first call and give it time to acquire the lock - const firstDownload = manager.fetchBinary(mockApi1, label); + const firstDownload = manager.fetchBinary(mockApi1); // Wait for the lock to be acquired before starting concurrent calls await new Promise((resolve) => setTimeout(resolve, 50)); const downloads = await Promise.all([ firstDownload, - manager.fetchBinary(mockApi2, label), - manager.fetchBinary(mockApi2, label), + manager.fetchBinary(mockApi2), + manager.fetchBinary(mockApi2), ]); expect(downloads).toHaveLength(3); @@ -188,7 +188,7 @@ describe("CliManager Concurrent Downloads", () => { const label = "test.coder.com"; const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); - await expect(manager.fetchBinary(mockApi, label)).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( `Unable to download binary: ${code}: ${message}`, ); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 1d9dac2f..845275aa 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -59,6 +59,7 @@ vi.mock("@/cliConfig", async () => { return { ...actual, shouldUseKeyring: vi.fn(), + isKeyringEnabled: vi.fn(), }; }); @@ -90,7 +91,7 @@ describe("CliManager", () => { const TEST_VERSION = "1.2.3"; const TEST_URL = "https://test.coder.com"; const BASE_PATH = "/path/base"; - const BINARY_DIR = `${BASE_PATH}/test/bin`; + const BINARY_DIR = `${BASE_PATH}/test.coder.com/bin`; const PLATFORM = "linux"; const ARCH = "amd64"; const BINARY_NAME = `coder-${PLATFORM}-${ARCH}`; @@ -140,7 +141,6 @@ describe("CliManager", () => { function configure(token = "test-token") { return manager.configure( - "dev.coder.com", "https://coder.example.com", token, MOCK_FEATURE_SET, @@ -151,50 +151,45 @@ describe("CliManager", () => { it("should write both url and token to correct paths", async () => { await configure("test-token"); - expect(memfs.readFileSync("/path/base/dev.coder.com/url", "utf8")).toBe( - "https://coder.example.com", - ); expect( - memfs.readFileSync("/path/base/dev.coder.com/session", "utf8"), + memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), + ).toBe("https://coder.example.com"); + expect( + memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), ).toBe("test-token"); }); it("should throw when URL is empty", async () => { await expect( - manager.configure( - "dev.coder.com", - "", - "test-token", - MOCK_FEATURE_SET, - TEST_BIN, - ), + manager.configure("", "test-token", MOCK_FEATURE_SET, TEST_BIN), ).rejects.toThrow("URL is required to configure the CLI"); }); it("should write empty string for token when provided", async () => { await configure(""); - expect(memfs.readFileSync("/path/base/dev.coder.com/url", "utf8")).toBe( - "https://coder.example.com", - ); expect( - memfs.readFileSync("/path/base/dev.coder.com/session", "utf8"), + memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), + ).toBe("https://coder.example.com"); + expect( + memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), ).toBe(""); }); - it("should use base path directly when label is empty", async () => { + it("should use hostname-specific path for URL", async () => { await manager.configure( - "", "https://coder.example.com", "token", MOCK_FEATURE_SET, TEST_BIN, ); - expect(memfs.readFileSync("/path/base/url", "utf8")).toBe( - "https://coder.example.com", - ); - expect(memfs.readFileSync("/path/base/session", "utf8")).toBe("token"); + expect( + memfs.readFileSync("/path/base/coder.example.com/url", "utf8"), + ).toBe("https://coder.example.com"); + expect( + memfs.readFileSync("/path/base/coder.example.com/session", "utf8"), + ).toBe("token"); }); it("should store via CLI credential manager when keyring enabled", async () => { @@ -208,8 +203,10 @@ describe("CliManager", () => { "test-token", expect.anything(), ); - expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); - expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + expect(memfs.existsSync("/path/base/coder.example.com/url")).toBe(false); + expect(memfs.existsSync("/path/base/coder.example.com/session")).toBe( + false, + ); }); it("should throw and show error when CLI keyring store fails", async () => { @@ -226,8 +223,10 @@ describe("CliManager", () => { expect.stringContaining("keyring unavailable"), "Open Settings", ); - expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); - expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + expect(memfs.existsSync("/path/base/coder.example.com/url")).toBe(false); + expect(memfs.existsSync("/path/base/coder.example.com/session")).toBe( + false, + ); }); }); @@ -243,29 +242,48 @@ describe("CliManager", () => { it("should remove credential files", async () => { seedCredentialFiles(); - await manager.clearCredentials("dev.coder.com"); + await manager.clearCredentials("https://dev.coder.com"); expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); expect(memfs.existsSync("/path/base/dev.coder.com/url")).toBe(false); }); it("should not throw when credential files don't exist", async () => { await expect( - manager.clearCredentials("dev.coder.com"), + manager.clearCredentials("https://dev.coder.com"), ).resolves.not.toThrow(); }); + + it("should call deleteToken when keyring is enabled", async () => { + vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(true); + seedCredentialFiles(); + await manager.clearCredentials("https://dev.coder.com"); + expect(mockCredManager.deleteToken).toHaveBeenCalledWith( + "https://dev.coder.com", + expect.anything(), + ); + // File cleanup still runs + expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); + }); + + it("should skip deleteToken when keyring is disabled", async () => { + vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false); + seedCredentialFiles(); + await manager.clearCredentials("https://dev.coder.com"); + expect(mockCredManager.deleteToken).not.toHaveBeenCalled(); + }); }); describe("Binary Version Validation", () => { it("rejects invalid server versions", async () => { mockApi.getBuildInfo = vi.fn().mockResolvedValue({ version: "invalid" }); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Got invalid version from deployment", ); }); it("accepts valid semver versions", async () => { withExistingBinary(TEST_VERSION); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); }); }); @@ -278,7 +296,7 @@ describe("CliManager", () => { it("reuses matching binary without downloading", async () => { withExistingBinary(TEST_VERSION); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).not.toHaveBeenCalled(); // Verify binary still exists @@ -288,7 +306,7 @@ describe("CliManager", () => { it("downloads when versions differ", async () => { withExistingBinary("1.0.0"); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).toHaveBeenCalled(); // Verify new binary exists @@ -301,7 +319,7 @@ describe("CliManager", () => { it("keeps mismatched binary when downloads disabled", async () => { mockConfig.set("coder.enableDownloads", false); withExistingBinary("1.0.0"); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).not.toHaveBeenCalled(); // Should still have the old version @@ -314,7 +332,7 @@ describe("CliManager", () => { it("downloads fresh binary when corrupted", async () => { withCorruptedBinary(); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).toHaveBeenCalled(); expect(memfs.existsSync(BINARY_PATH)).toBe(true); @@ -328,7 +346,7 @@ describe("CliManager", () => { expect(memfs.existsSync(BINARY_DIR)).toBe(false); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).toHaveBeenCalled(); @@ -342,7 +360,7 @@ describe("CliManager", () => { it("fails when downloads disabled and no binary", async () => { mockConfig.set("coder.enableDownloads", false); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download CLI because downloads are disabled", ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); @@ -356,7 +374,7 @@ describe("CliManager", () => { it("downloads with correct headers", async () => { withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(mockAxios.get).toHaveBeenCalledWith( `/bin/${BINARY_NAME}`, expect.objectContaining({ @@ -372,7 +390,7 @@ describe("CliManager", () => { it("uses custom binary source", async () => { mockConfig.set("coder.binarySource", "/custom/path"); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(mockAxios.get).toHaveBeenCalledWith( "/custom/path", expect.objectContaining({ @@ -386,7 +404,7 @@ describe("CliManager", () => { it("uses ETag for existing binaries", async () => { withExistingBinary("1.0.0"); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); // Verify ETag was computed from actual file content expect(mockAxios.get).toHaveBeenCalledWith( @@ -408,7 +426,7 @@ describe("CliManager", () => { memfs.writeFileSync(path.join(BINARY_DIR, "keeper.txt"), "keep"); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); // Verify old files were actually removed but other files kept expect(memfs.existsSync(path.join(BINARY_DIR, "coder.old-xyz"))).toBe( @@ -425,7 +443,7 @@ describe("CliManager", () => { withExistingBinary("1.0.0"); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); // Verify the old binary was backed up const files = readdir(BINARY_DIR); @@ -444,7 +462,7 @@ describe("CliManager", () => { it("handles 304 Not Modified", async () => { withExistingBinary("1.0.0"); withHttpResponse(304); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); // No change expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( @@ -458,7 +476,7 @@ describe("CliManager", () => { "Coder isn't supported for your platform. Please open an issue, we'd love to support it!", "Open an Issue", ); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Platform not supported", ); expect(vscode.env.openExternal).toHaveBeenCalledWith( @@ -476,7 +494,7 @@ describe("CliManager", () => { "Failed to download binary. Please open an issue.", "Open an Issue", ); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Failed to download binary", ); expect(vscode.env.openExternal).toHaveBeenCalledWith( @@ -496,7 +514,7 @@ describe("CliManager", () => { it("handles write stream errors", async () => { withStreamError("write", "disk full"); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download binary: disk full", ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); @@ -504,7 +522,7 @@ describe("CliManager", () => { it("handles read stream errors", async () => { withStreamError("read", "network timeout"); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download binary: network timeout", ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); @@ -512,8 +530,7 @@ describe("CliManager", () => { it("handles missing content-length", async () => { withSuccessfulDownload({ headers: {} }); - mockProgress.clearProgressReports(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(memfs.existsSync(BINARY_PATH)).toBe(true); // Without any content-length header, increment should be undefined. @@ -528,8 +545,7 @@ describe("CliManager", () => { "reports progress with %s header", async (header) => { withSuccessfulDownload({ headers: { [header]: "1024" } }); - mockProgress.clearProgressReports(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(memfs.existsSync(BINARY_PATH)).toBe(true); const reports = mockProgress.getProgressReports(); @@ -548,7 +564,7 @@ describe("CliManager", () => { it("shows download progress", async () => { withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(vscode.window.withProgress).toHaveBeenCalledWith( expect.objectContaining({ title: `Downloading Coder CLI for ${TEST_URL}`, @@ -560,7 +576,7 @@ describe("CliManager", () => { it("handles user cancellation", async () => { mockProgress.setCancellation(true); withSuccessfulDownload(); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Download aborted", ); expect(memfs.existsSync(BINARY_PATH)).toBe(false); @@ -571,7 +587,7 @@ describe("CliManager", () => { it("verifies valid signatures", async () => { withSuccessfulDownload(); withSignatureResponses([200]); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(pgp.verifySignature).toHaveBeenCalled(); const sigFile = expectFileInDir(BINARY_DIR, ".asc"); @@ -582,7 +598,7 @@ describe("CliManager", () => { withSuccessfulDownload(); withSignatureResponses([404, 200]); mockUI.setResponse("Signature not found", "Download signature"); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(mockAxios.get).toHaveBeenCalledTimes(3); const sigFile = expectFileInDir(BINARY_DIR, ".asc"); @@ -596,7 +612,7 @@ describe("CliManager", () => { createVerificationError("Invalid signature"), ); mockUI.setResponse("Signature does not match", "Run anyway"); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(memfs.existsSync(BINARY_PATH)).toBe(true); }); @@ -608,7 +624,7 @@ describe("CliManager", () => { createVerificationError("Invalid signature"), ); mockUI.setResponse("Signature does not match", undefined); - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Signature verification aborted", ); }); @@ -616,7 +632,7 @@ describe("CliManager", () => { it("skips verification when disabled", async () => { mockConfig.set("coder.disableSignatureVerification", true); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(pgp.verifySignature).not.toHaveBeenCalled(); const files = readdir(BINARY_DIR); @@ -631,7 +647,7 @@ describe("CliManager", () => { withSuccessfulDownload(); withHttpResponse(status); mockUI.setResponse(message, "Run without verification"); - const result = await manager.fetchBinary(mockApi, "test"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual(result, BINARY_PATH); expect(pgp.verifySignature).not.toHaveBeenCalled(); }); @@ -645,7 +661,7 @@ describe("CliManager", () => { withSuccessfulDownload(); withHttpResponse(status); mockUI.setResponse(message, undefined); // User cancels - await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow( + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Signature download aborted", ); }, @@ -660,7 +676,7 @@ describe("CliManager", () => { it("creates binary directory", async () => { expect(memfs.existsSync(BINARY_DIR)).toBe(false); withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(memfs.existsSync(BINARY_DIR)).toBe(true); const stats = memfs.statSync(BINARY_DIR); expect(stats.isDirectory()).toBe(true); @@ -668,7 +684,7 @@ describe("CliManager", () => { it("validates downloaded binary version", async () => { withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( mockBinaryContent(TEST_VERSION), ); @@ -676,7 +692,7 @@ describe("CliManager", () => { it("sets correct file permissions", async () => { withSuccessfulDownload(); - await manager.fetchBinary(mockApi, "test"); + await manager.fetchBinary(mockApi); const stats = memfs.statSync(BINARY_PATH); expect(stats.mode & 0o777).toBe(0o755); }); @@ -697,18 +713,12 @@ describe("CliManager", () => { ); withSuccessfulDownload(); - const result = await manager.fetchBinary(mockApi, "test label"); + const result = await manager.fetchBinary(mockApi); expectPathsEqual( result, - `${pathWithSpaces}/test label/bin/${BINARY_NAME}`, + `${pathWithSpaces}/test.coder.com/bin/${BINARY_NAME}`, ); }); - - it("handles empty deployment label", async () => { - withExistingBinary(TEST_VERSION, "/path/base/bin"); - const result = await manager.fetchBinary(mockApi, ""); - expectPathsEqual(result, path.join(BASE_PATH, "bin", BINARY_NAME)); - }); }); function createMockApi(version: string, url: string): Api { diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 3a6db752..fb0b3e08 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -118,15 +118,11 @@ function createTestContext() { const secretsManager = new SecretsManager(secretStorage, memento, logger); const mementoManager = new MementoManager(memento); - const mockCliManager = { - fetchBinary: vi.fn().mockRejectedValue(new Error("no binary")), - }; const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, createMockCliCredentialManager(), - mockCliManager as unknown as import("@/core/cliManager").CliManager, "coder.coder-remote", ); @@ -311,15 +307,11 @@ describe("LoginCoordinator", () => { mockConfig.set("coder.tlsKeyFile", "/path/to/key.pem"); const logger = createMockLogger(); - const mockCliManager2 = { - fetchBinary: vi.fn().mockRejectedValue(new Error("no binary")), - }; const coordinator = new LoginCoordinator( secretsManager, mementoManager, logger, createMockCliCredentialManager(), - mockCliManager2 as unknown as import("@/core/cliManager").CliManager, "coder.coder-remote", ); From 45e0f0ba971a1ec83e60cabda984926a53486084 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 5 Mar 2026 01:50:18 +0300 Subject: [PATCH 3/4] refactor: consistent credential API, lightweight binary lookup, quieter logging Make storeToken resolve its own binary internally via the injected BinaryResolver, matching readToken and deleteToken. Remove the binPath parameter from both storeToken and configure since callers no longer need to supply it. Add locateBinary to CliManager as a cheap stat-only lookup that the container resolver tries before falling back to fetchBinary. Downgrade implementation-detail log messages from info/warn to debug, keeping info for significant events (server version, download start, stored/deleted token). --- src/core/cliCredentialManager.ts | 59 ++-- src/core/cliManager.ts | 58 ++-- src/core/container.ts | 10 +- src/login/loginCoordinator.ts | 17 +- src/remote/remote.ts | 8 +- test/unit/core/cliCredentialManager.test.ts | 305 +++++++++----------- test/unit/core/cliManager.test.ts | 32 +- 7 files changed, 230 insertions(+), 259 deletions(-) diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index ae051b72..e0464ddd 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -1,6 +1,7 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; +import { isKeyringEnabled } from "../cliConfig"; import { getHeaderArgs } from "../headers"; import type { WorkspaceConfiguration } from "vscode"; @@ -23,10 +24,8 @@ export function isKeyringSupported(): boolean { } /** - * Delegates credential storage to the Coder CLI to keep the credentials in sync. - * - * For operations that don't have a binary path available (readToken, deleteToken), - * uses the injected BinaryResolver to fetch/locate the CLI binary. + * Delegates credential storage to the Coder CLI. All operations resolve the + * binary via the injected BinaryResolver before invoking it. */ export class CliCredentialManager { constructor( @@ -35,18 +34,22 @@ export class CliCredentialManager { ) {} /** - * Store a token by running: - * CODER_SESSION_TOKEN= login --use-token-as-session - * - * The token is passed via environment variable so it never appears in - * process argument lists. + * Store a token via `coder login --use-token-as-session`. + * Token is passed via CODER_SESSION_TOKEN env var, never in args. */ public async storeToken( - binPath: string, url: string, token: string, configs: Pick, ): Promise { + let binPath: string; + try { + binPath = await this.resolveBinary(url); + } catch (error) { + this.logger.debug("Could not resolve CLI binary for token store:", error); + throw error; + } + const args = [ ...getHeaderArgs(configs), "login", @@ -59,27 +62,28 @@ export class CliCredentialManager { }); this.logger.info("Stored token via CLI for", url); } catch (error) { - this.logger.error("Failed to store token via CLI:", error); + this.logger.debug("Failed to store token via CLI:", error); throw error; } } /** - * Read a token by running: - * login token --url - * - * Resolves the CLI binary automatically. Returns trimmed stdout, - * or undefined if the binary can't be resolved or the CLI returns no token. + * Read a token via `coder login token --url`. Returns trimmed stdout, + * or undefined on any failure (resolver, CLI, empty output). */ public async readToken( url: string, configs: Pick, ): Promise { + if (!isKeyringEnabled(configs)) { + return undefined; + } + let binPath: string; try { binPath = await this.resolveBinary(url); } catch (error) { - this.logger.warn("Could not resolve CLI binary for token read:", error); + this.logger.debug("Could not resolve CLI binary for token read:", error); return undefined; } @@ -89,27 +93,30 @@ export class CliCredentialManager { const token = stdout.trim(); return token || undefined; } catch (error) { - this.logger.warn("Failed to read token via CLI:", error); + this.logger.debug("Failed to read token via CLI:", error); return undefined; } } /** - * Delete a token by running: - * logout --url - * - * Resolves the CLI binary automatically. Best-effort: logs warnings - * on failure but never throws. + * Delete a token via `coder logout --url`. Best-effort: never throws. */ - async deleteToken( + public async deleteToken( url: string, configs: Pick, ): Promise { + if (!isKeyringEnabled(configs)) { + return; + } + let binPath: string; try { binPath = await this.resolveBinary(url); } catch (error) { - this.logger.warn("Could not resolve CLI binary for token delete:", error); + this.logger.debug( + "Could not resolve CLI binary for token delete:", + error, + ); return; } @@ -118,7 +125,7 @@ export class CliCredentialManager { await execFileAsync(binPath, args); this.logger.info("Deleted token via CLI for", url); } catch (error) { - this.logger.warn("Failed to delete token via CLI:", error); + this.logger.debug("Failed to delete token via CLI:", error); } } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 9007397a..9b07cade 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,7 +10,7 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; -import { isKeyringEnabled, shouldUseKeyring } from "../cliConfig"; +import { shouldUseKeyring } from "../cliConfig"; import * as pgp from "../pgp"; import { toSafeHost } from "../util"; import { vscodeProposed } from "../vscodeProposed"; @@ -39,6 +39,23 @@ export class CliManager { this.binaryLock = new BinaryLock(output); } + /** + * Return the path to a cached CLI binary for a deployment URL. + * Stat check only — no network, no subprocess. Throws if absent. + */ + public async locateBinary(url: string): Promise { + const safeHostname = toSafeHost(url); + const binPath = path.join( + this.pathResolver.getBinaryCachePath(safeHostname), + cliUtils.name(), + ); + const stat = await cliUtils.stat(binPath); + if (!stat) { + throw new Error(`No CLI binary found at ${binPath}`); + } + return binPath; + } + /** * Download and return the path to a working binary for the deployment with * the provided hostname using the provided client. If the hostname is empty, @@ -60,7 +77,10 @@ export class CliManager { // Settings can be undefined when set to their defaults (true in this case), // so explicitly check against false. const enableDownloads = cfg.get("enableDownloads") !== false; - this.output.info("Downloads are", enableDownloads ? "enabled" : "disabled"); + this.output.debug( + "Downloads are", + enableDownloads ? "enabled" : "disabled", + ); // Get the build info to compare with the existing binary version, if any, // and to log for debugging. @@ -80,18 +100,18 @@ export class CliManager { this.pathResolver.getBinaryCachePath(safeHostname), cliUtils.name(), ); - this.output.info("Using binary path", binPath); + this.output.debug("Using binary path", binPath); const stat = await cliUtils.stat(binPath); if (stat === undefined) { this.output.info("No existing binary found, starting download"); } else { - this.output.info("Existing binary size is", prettyBytes(stat.size)); + this.output.debug("Existing binary size is", prettyBytes(stat.size)); try { const version = await cliUtils.version(binPath); - this.output.info("Existing binary version is", version); + this.output.debug("Existing binary version is", version); // If we have the right version we can avoid the request entirely. if (version === buildInfo.version) { - this.output.info( + this.output.debug( "Using existing binary since it matches the server version", ); return binPath; @@ -130,19 +150,19 @@ export class CliManager { binPath, progressLogPath, ); - this.output.info("Acquired download lock"); + this.output.debug("Acquired download lock"); // If we waited for another process, re-check if binary is now ready if (lockResult.waited) { const latestBuildInfo = await restClient.getBuildInfo(); - this.output.info("Got latest server version", latestBuildInfo.version); + this.output.debug("Got latest server version", latestBuildInfo.version); const recheckAfterWait = await this.checkBinaryVersion( binPath, latestBuildInfo.version, ); if (recheckAfterWait.matches) { - this.output.info( + this.output.debug( "Using existing binary since it matches the latest server version", ); return binPath; @@ -174,7 +194,7 @@ export class CliManager { } finally { if (lockResult) { await lockResult.release(); - this.output.info("Released download lock"); + this.output.debug("Released download lock"); } } } @@ -721,12 +741,7 @@ export class CliManager { * Both URL and token are required. Empty tokens are allowed (e.g. mTLS * authentication) but the URL must be a non-empty string. */ - public async configure( - url: string, - token: string, - featureSet: FeatureSet, - binPath: string, - ) { + public async configure(url: string, token: string, featureSet: FeatureSet) { if (!url) { throw new Error("URL is required to configure the CLI"); } @@ -735,12 +750,7 @@ export class CliManager { const configs = vscode.workspace.getConfiguration(); if (shouldUseKeyring(configs, featureSet)) { try { - await this.cliCredentialManager.storeToken( - binPath, - url, - token, - configs, - ); + await this.cliCredentialManager.storeToken(url, token, configs); } catch (error) { this.output.error("Failed to store token via CLI keyring:", error); vscode.window @@ -775,9 +785,7 @@ export class CliManager { public async clearCredentials(url: string): Promise { const safeHostname = toSafeHost(url); const configs = vscode.workspace.getConfiguration(); - if (isKeyringEnabled(configs)) { - await this.cliCredentialManager.deleteToken(url, configs); - } + await this.cliCredentialManager.deleteToken(url, configs); const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); const urlPath = this.pathResolver.getUrlPath(safeHostname); diff --git a/src/core/container.ts b/src/core/container.ts index 143687a3..f3ab4cd5 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -37,11 +37,17 @@ export class ServiceContainer implements vscode.Disposable { context.globalState, this.logger, ); + // Circular ref: cliCredentialManager ↔ cliManager. Safe because + // the resolver is only called after construction. this.cliCredentialManager = new CliCredentialManager( this.logger, async (url) => { - const client = CoderApi.create(url, "", this.logger); - return this.cliManager.fetchBinary(client); + try { + return await this.cliManager.locateBinary(url); + } catch { + const client = CoderApi.create(url, "", this.logger); + return this.cliManager.fetchBinary(client); + } }, ); this.cliManager = new CliManager( diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index cdaa8db6..5dfb4677 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -243,7 +243,10 @@ export class LoginCoordinator implements vscode.Disposable { } // Try keyring token (picks up tokens written by `coder login` in the terminal) - const keyringToken = await this.getCliKeyringToken(deployment); + const keyringToken = await this.cliCredentialManager.readToken( + deployment.url, + vscode.workspace.getConfiguration(), + ); if ( keyringToken && keyringToken !== providedToken && @@ -302,18 +305,6 @@ export class LoginCoordinator implements vscode.Disposable { } } - /** - * Read a token from the CLI keyring. - */ - private async getCliKeyringToken( - deployment: Deployment, - ): Promise { - return this.cliCredentialManager.readToken( - deployment.url, - vscode.workspace.getConfiguration(), - ); - } - /** * Shows auth error via dialog or logs it for autoLogin. */ diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 12b94bd8..c676b522 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -241,12 +241,7 @@ export class Remote { // Write token to keyring or file (after CLI version is known) if (baseUrlRaw && token !== undefined) { - await this.cliManager.configure( - baseUrlRaw, - token, - featureSet, - binaryPath, - ); + await this.cliManager.configure(baseUrlRaw, token, featureSet); } // Listen for token changes for this deployment @@ -261,7 +256,6 @@ export class Remote { auth.url, auth.token, featureSet, - binaryPath, ); this.logger.info( "Updated CLI config with new token for remote deployment", diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index e06987f7..28bb94dc 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -1,6 +1,7 @@ import { execFile } from "node:child_process"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { isKeyringEnabled } from "@/cliConfig"; import { CliCredentialManager, isKeyringSupported, @@ -13,60 +14,73 @@ vi.mock("node:child_process", () => ({ execFile: vi.fn(), })); -function stubPlatform(platform: string) { - vi.stubGlobal("process", { ...process, platform }); -} +vi.mock("@/cliConfig", () => ({ + isKeyringEnabled: vi.fn().mockReturnValue(false), +})); -function mockExecFileSuccess(stdout = "") { +const TEST_BIN = "/usr/bin/coder"; +const TEST_URL = "https://dev.coder.com"; + +function stubExecFile(result: { stdout?: string } | { error: string }) { vi.mocked(execFile).mockImplementation( (_cmd, _args, _opts, callback?: unknown) => { - // promisify(execFile) calls execFile with a callback as last argument const cb = typeof _opts === "function" - ? (_opts as (err: Error | null, result: { stdout: string }) => void) + ? (_opts as (err: Error | null, result?: { stdout: string }) => void) : (callback as - | ((err: Error | null, result: { stdout: string }) => void) + | ((err: Error | null, result?: { stdout: string }) => void) | undefined); if (cb) { - cb(null, { stdout }); + if ("error" in result) { + cb(new Error(result.error)); + } else { + cb(null, { stdout: result.stdout ?? "" }); + } } return {} as ReturnType; }, ); } -function mockExecFileFailure(message: string) { - vi.mocked(execFile).mockImplementation( - (_cmd, _args, _opts, callback?: unknown) => { - const cb = - typeof _opts === "function" - ? (_opts as (err: Error | null) => void) - : (callback as ((err: Error | null) => void) | undefined); - if (cb) { - cb(new Error(message)); - } - return {} as ReturnType; - }, - ); +function lastExecArgs(): { + bin: string; + args: string[]; + env: NodeJS.ProcessEnv; +} { + const call = vi.mocked(execFile).mock.calls[0]; + return { + bin: call[0], + args: call[1] as string[], + env: (call[2] as { env: NodeJS.ProcessEnv }).env, + }; } -const mockConfigs = { - get: vi.fn().mockReturnValue(undefined), -}; - -const TEST_BIN = "/usr/bin/coder"; -const TEST_URL = "https://dev.coder.com"; - -function createSuccessResolver(): BinaryResolver { +function successResolver(): BinaryResolver { return vi.fn().mockResolvedValue(TEST_BIN) as unknown as BinaryResolver; } -function createFailingResolver(): BinaryResolver { +function failingResolver(): BinaryResolver { return vi .fn() .mockRejectedValue(new Error("no binary")) as unknown as BinaryResolver; } +const configs = { get: vi.fn().mockReturnValue(undefined) }; + +const configWithHeaders = { + get: vi.fn((key: string) => + key === "coder.headerCommand" ? "my-header-cmd" : undefined, + ), +}; + +function setup(resolver?: BinaryResolver) { + const r = resolver ?? successResolver(); + return { + resolver: r, + manager: new CliCredentialManager(createMockLogger(), r), + }; +} + describe("isKeyringSupported", () => { afterEach(() => { vi.unstubAllGlobals(); @@ -78,7 +92,7 @@ describe("isKeyringSupported", () => { { platform: "linux", expected: false }, { platform: "freebsd", expected: false }, ])("returns $expected for $platform", ({ platform, expected }) => { - stubPlatform(platform); + vi.stubGlobal("process", { ...process, platform }); expect(isKeyringSupported()).toBe(expected); }); }); @@ -89,198 +103,149 @@ describe("CliCredentialManager", () => { }); describe("storeToken", () => { - it("passes token via CODER_SESSION_TOKEN env var", async () => { - mockExecFileSuccess(); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - await manager.storeToken( - TEST_BIN, - TEST_URL, - "my-secret-token", - mockConfigs, - ); - - expect(execFile).toHaveBeenCalledWith( - TEST_BIN, - ["login", "--use-token-as-session", TEST_URL], - expect.objectContaining({ - env: expect.objectContaining({ - CODER_SESSION_TOKEN: "my-secret-token", - }), - }), - expect.any(Function), - ); - }); - - it("never passes token in args", async () => { - mockExecFileSuccess(); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); + it("resolves binary and invokes coder login", async () => { + stubExecFile({ stdout: "" }); + const { manager, resolver } = setup(); - await manager.storeToken( - TEST_BIN, - TEST_URL, - "my-secret-token", - mockConfigs, - ); + await manager.storeToken(TEST_URL, "my-secret-token", configs); - const args = vi.mocked(execFile).mock.calls[0][1] as string[]; - expect(args).not.toContain("my-secret-token"); + expect(resolver).toHaveBeenCalledWith(TEST_URL); + const exec = lastExecArgs(); + expect(exec.bin).toBe(TEST_BIN); + expect(exec.args).toEqual(["login", "--use-token-as-session", TEST_URL]); + // Token must only appear in env, never in args + expect(exec.env.CODER_SESSION_TOKEN).toBe("my-secret-token"); + expect(exec.args).not.toContain("my-secret-token"); }); - it("throws when CLI fails", async () => { - mockExecFileFailure("login failed"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); + it("throws when CLI exec fails", async () => { + stubExecFile({ error: "login failed" }); + const { manager } = setup(); await expect( - manager.storeToken(TEST_BIN, TEST_URL, "token", mockConfigs), + manager.storeToken(TEST_URL, "token", configs), ).rejects.toThrow("login failed"); }); - it("includes header args when header command is set", async () => { - mockExecFileSuccess(); - const configWithHeaders = { - get: vi.fn((key: string) => - key === "coder.headerCommand" ? "my-header-cmd" : undefined, - ), - }; - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - await manager.storeToken(TEST_BIN, TEST_URL, "token", configWithHeaders); - - const args = vi.mocked(execFile).mock.calls[0][1] as string[]; - expect(args).toContain("--header-command"); + it("throws when binary resolver fails", async () => { + const { manager } = setup(failingResolver()); + + await expect( + manager.storeToken(TEST_URL, "token", configs), + ).rejects.toThrow("no binary"); + expect(execFile).not.toHaveBeenCalled(); + }); + + it("forwards header command args", async () => { + stubExecFile({ stdout: "" }); + const { manager } = setup(); + + await manager.storeToken(TEST_URL, "token", configWithHeaders); + + expect(lastExecArgs().args).toContain("--header-command"); }); }); describe("readToken", () => { - it("resolves binary and returns trimmed stdout", async () => { - mockExecFileSuccess(" my-token\n"); - const resolver = createSuccessResolver(); - const manager = new CliCredentialManager(createMockLogger(), resolver); + it("returns trimmed token from CLI stdout", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: " my-token\n" }); + const { manager, resolver } = setup(); - const token = await manager.readToken(TEST_URL, mockConfigs); + const token = await manager.readToken(TEST_URL, configs); expect(resolver).toHaveBeenCalledWith(TEST_URL); expect(token).toBe("my-token"); + expect(lastExecArgs().args).toEqual([ + "login", + "token", + "--url", + TEST_URL, + ]); }); - it("returns undefined on empty stdout", async () => { - mockExecFileSuccess(" \n"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - const token = await manager.readToken(TEST_URL, mockConfigs); - - expect(token).toBeUndefined(); + it("returns undefined on whitespace-only stdout", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: " \n" }); + const { manager } = setup(); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); }); it("returns undefined on CLI error", async () => { - mockExecFileFailure("no token found"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - const token = await manager.readToken(TEST_URL, mockConfigs); - - expect(token).toBeUndefined(); + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ error: "no token found" }); + const { manager } = setup(); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); }); it("returns undefined when binary resolver fails", async () => { - const manager = new CliCredentialManager( - createMockLogger(), - createFailingResolver(), - ); - - const token = await manager.readToken(TEST_URL, mockConfigs); + vi.mocked(isKeyringEnabled).mockReturnValue(true); + const { manager } = setup(failingResolver()); - expect(token).toBeUndefined(); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); }); - it("passes correct args", async () => { - mockExecFileSuccess("token"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); - - await manager.readToken(TEST_URL, mockConfigs); + it("skips CLI when keyring is disabled", async () => { + stubExecFile({ stdout: "my-token" }); + const { manager } = setup(); - const call = vi.mocked(execFile).mock.calls[0]; - expect(call[0]).toBe(TEST_BIN); - expect(call[1]).toEqual(["login", "token", "--url", TEST_URL]); + expect(await manager.readToken(TEST_URL, configs)).toBeUndefined(); + expect(execFile).not.toHaveBeenCalled(); }); }); describe("deleteToken", () => { - it("resolves binary and runs coder logout", async () => { - mockExecFileSuccess(); - const resolver = createSuccessResolver(); - const manager = new CliCredentialManager(createMockLogger(), resolver); + it("resolves binary and invokes coder logout", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager, resolver } = setup(); - await manager.deleteToken(TEST_URL, mockConfigs); + await manager.deleteToken(TEST_URL, configs); expect(resolver).toHaveBeenCalledWith(TEST_URL); - const call = vi.mocked(execFile).mock.calls[0]; - expect(call[0]).toBe(TEST_BIN); - expect(call[1]).toEqual(["logout", "--url", TEST_URL, "--yes"]); + const exec = lastExecArgs(); + expect(exec.bin).toBe(TEST_BIN); + expect(exec.args).toEqual(["logout", "--url", TEST_URL, "--yes"]); }); - it("does not throw when CLI fails", async () => { - mockExecFileFailure("logout failed"); - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); + it("never throws on CLI error", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ error: "logout failed" }); + const { manager } = setup(); await expect( - manager.deleteToken(TEST_URL, mockConfigs), + manager.deleteToken(TEST_URL, configs), ).resolves.not.toThrow(); }); - it("does not throw when binary resolver fails", async () => { - const manager = new CliCredentialManager( - createMockLogger(), - createFailingResolver(), - ); + it("never throws when binary resolver fails", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + const { manager } = setup(failingResolver()); await expect( - manager.deleteToken(TEST_URL, mockConfigs), + manager.deleteToken(TEST_URL, configs), ).resolves.not.toThrow(); expect(execFile).not.toHaveBeenCalled(); }); - it("includes header args when header command is set", async () => { - mockExecFileSuccess(); - const configWithHeaders = { - get: vi.fn((key: string) => - key === "coder.headerCommand" ? "my-header-cmd" : undefined, - ), - }; - const manager = new CliCredentialManager( - createMockLogger(), - createSuccessResolver(), - ); + it("forwards header command args", async () => { + vi.mocked(isKeyringEnabled).mockReturnValue(true); + stubExecFile({ stdout: "" }); + const { manager } = setup(); await manager.deleteToken(TEST_URL, configWithHeaders); - const args = vi.mocked(execFile).mock.calls[0][1] as string[]; - expect(args).toContain("--header-command"); + expect(lastExecArgs().args).toContain("--header-command"); + }); + + it("skips CLI when keyring is disabled", async () => { + stubExecFile({ stdout: "" }); + const { manager } = setup(); + + await manager.deleteToken(TEST_URL, configs); + + expect(execFile).not.toHaveBeenCalled(); }); }); }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 845275aa..3f986d3a 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -59,7 +59,6 @@ vi.mock("@/cliConfig", async () => { return { ...actual, shouldUseKeyring: vi.fn(), - isKeyringEnabled: vi.fn(), }; }); @@ -137,14 +136,11 @@ describe("CliManager", () => { }); describe("Configure CLI", () => { - const TEST_BIN = "/usr/bin/coder"; - function configure(token = "test-token") { return manager.configure( "https://coder.example.com", token, MOCK_FEATURE_SET, - TEST_BIN, ); } @@ -161,7 +157,7 @@ describe("CliManager", () => { it("should throw when URL is empty", async () => { await expect( - manager.configure("", "test-token", MOCK_FEATURE_SET, TEST_BIN), + manager.configure("", "test-token", MOCK_FEATURE_SET), ).rejects.toThrow("URL is required to configure the CLI"); }); @@ -181,7 +177,6 @@ describe("CliManager", () => { "https://coder.example.com", "token", MOCK_FEATURE_SET, - TEST_BIN, ); expect( @@ -198,7 +193,6 @@ describe("CliManager", () => { await configure("test-token"); expect(mockCredManager.storeToken).toHaveBeenCalledWith( - TEST_BIN, "https://coder.example.com", "test-token", expect.anything(), @@ -230,6 +224,20 @@ describe("CliManager", () => { }); }); + describe("Locate Binary", () => { + it("returns path when binary exists", async () => { + withExistingBinary(TEST_VERSION); + const result = await manager.locateBinary(TEST_URL); + expectPathsEqual(result, BINARY_PATH); + }); + + it("throws when binary does not exist", async () => { + await expect(manager.locateBinary(TEST_URL)).rejects.toThrow( + "No CLI binary found at", + ); + }); + }); + describe("Clear Credentials", () => { function seedCredentialFiles() { memfs.mkdirSync("/path/base/dev.coder.com", { recursive: true }); @@ -253,8 +261,7 @@ describe("CliManager", () => { ).resolves.not.toThrow(); }); - it("should call deleteToken when keyring is enabled", async () => { - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(true); + it("should always call deleteToken (gating is internal)", async () => { seedCredentialFiles(); await manager.clearCredentials("https://dev.coder.com"); expect(mockCredManager.deleteToken).toHaveBeenCalledWith( @@ -264,13 +271,6 @@ describe("CliManager", () => { // File cleanup still runs expect(memfs.existsSync("/path/base/dev.coder.com/session")).toBe(false); }); - - it("should skip deleteToken when keyring is disabled", async () => { - vi.mocked(cliConfig.isKeyringEnabled).mockReturnValue(false); - seedCredentialFiles(); - await manager.clearCredentials("https://dev.coder.com"); - expect(mockCredManager.deleteToken).not.toHaveBeenCalled(); - }); }); describe("Binary Version Validation", () => { From e38a36e8f4cfdb1181b14b4c03b250830d91318c Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 5 Mar 2026 19:35:09 +0300 Subject: [PATCH 4/4] fix: filter --use-keyring and --global-config from user global flags These flags are managed by the extension and conflict with the auth mode it chooses. Passing --use-keyring=false with --url breaks keyring auth, and --global-config with --url causes the CLI to ignore the keyring entirely. --- package.json | 2 +- src/cliConfig.ts | 29 +++++++++++---- test/unit/cliConfig.test.ts | 72 ++++++++++++++++++++++++++++++++----- 3 files changed, 87 insertions(+), 16 deletions(-) diff --git a/package.json b/package.json index c4170c82..57394ad6 100644 --- a/package.json +++ b/package.json @@ -150,7 +150,7 @@ ] }, "coder.globalFlags": { - "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` flag is explicitly ignored.", + "markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.", "type": "array", "items": { "type": "string" diff --git a/src/cliConfig.ts b/src/cliConfig.ts index 7461651e..d947b1b7 100644 --- a/src/cliConfig.ts +++ b/src/cliConfig.ts @@ -32,12 +32,29 @@ export function getGlobalFlags( ? ["--url", escapeCommandArg(auth.url)] : ["--global-config", escapeCommandArg(auth.configDir)]; - // Last takes precedence/overrides previous ones - return [ - ...getGlobalFlagsRaw(configs), - ...authFlags, - ...getHeaderArgs(configs), - ]; + const raw = getGlobalFlagsRaw(configs); + const filtered: string[] = []; + for (let i = 0; i < raw.length; i++) { + if (isFlag(raw[i], "--use-keyring")) { + continue; + } + if (isFlag(raw[i], "--global-config")) { + // Skip the next item too when the value is a separate entry. + if (raw[i] === "--global-config") { + i++; + } + continue; + } + filtered.push(raw[i]); + } + + return [...filtered, ...authFlags, ...getHeaderArgs(configs)]; +} + +function isFlag(item: string, name: string): boolean { + return ( + item === name || item.startsWith(`${name}=`) || item.startsWith(`${name} `) + ); } /** diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index aaeb0fd5..5a14b844 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -68,18 +68,72 @@ describe("cliConfig", () => { ]); }); - it("should not filter duplicate global-config flags, last takes precedence", () => { + it.each(["--use-keyring", "--use-keyring=false", "--use-keyring=true"])( + "should filter %s from global flags", + (managedFlag) => { + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", [ + "--verbose", + managedFlag, + "--disable-direct-connections", + ]); + + expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ + "--verbose", + "--disable-direct-connections", + "--global-config", + '"/config/dir"', + ]); + }, + ); + + interface GlobalConfigCase { + scenario: string; + flags: string[]; + } + it.each([ + { + scenario: "space-separated in one item", + flags: ["-v", "--global-config /path/to/ignored"], + }, + { + scenario: "equals form", + flags: ["-v", "--global-config=/path/to/ignored"], + }, + { + scenario: "separate items", + flags: ["-v", "--global-config", "/path/to/ignored"], + }, + ])( + "should filter --global-config ($scenario) in both auth modes", + ({ flags }) => { + const urlAuth: CliAuth = { + mode: "url", + url: "https://dev.coder.com", + }; + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", flags); + + expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ + "-v", + "--global-config", + '"/config/dir"', + ]); + expect(getGlobalFlags(config, urlAuth)).toStrictEqual([ + "-v", + "--url", + '"https://dev.coder.com"', + ]); + }, + ); + + it("should not filter flags with similar prefixes", () => { const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", [ - "-v", - "--global-config /path/to/ignored", - "--disable-direct-connections", - ]); + config.set("coder.globalFlags", ["--global-configs", "--use-keyrings"]); expect(getGlobalFlags(config, globalConfigAuth)).toStrictEqual([ - "-v", - "--global-config /path/to/ignored", - "--disable-direct-connections", + "--global-configs", + "--use-keyrings", "--global-config", '"/config/dir"', ]);