From 954f03fc513d200cd78ca4003c90527ffe3b0792 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 4 Aug 2026 01:48:12 +0300 Subject: [PATCH 1/8] fix: include our own SSH config from the top of the user's SSH uses the first value it obtains for each option, so a catch-all "Host *" in the user's config beat the block we appended to the end of it, and connections aborted with "Unexpected SSH Config Option". Writing the block higher up would not be enough: it still loses to hosts pulled in by an Include above it, and a Host line moved over the options someone wrote outside any block would capture them. Write the blocks to ~/.ssh/coder/config instead and include that file from the first line of the user's config, where nothing can be parsed before it. Their config is written once to add the include, and the deployment's old block moves out of it on the next connect. The include path keeps its tilde: relative includes resolve against ~/.ssh no matter where the including file lives, and an absolute path would not survive a config synced between machines. Since placement now guarantees the options apply, the block that recomputed them and aborted the connection on a mismatch is gone. What remains of computeSshProperties reads RemoteCommand, which can only come from the user's config. --- CHANGELOG.md | 7 +++ src/remote/remote.ts | 70 +++++++++------------------- src/remote/sshConfig.ts | 67 +++++++++++++++++++++------ src/remote/sshSupport.ts | 22 --------- test/unit/remote/sshConfig.test.ts | 71 ++++++++++++++++++++++++++++- test/unit/remote/sshSupport.test.ts | 30 ------------ 6 files changed, 151 insertions(+), 116 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a42a5cac..8059de4aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,13 @@ ### Changed +- Write workspace SSH hosts to `~/.ssh/coder/config` and include that file + from the top of your own SSH config, rather than writing the block into your + config directly. SSH uses the first value it obtains for each option, so a + catch-all like `Host *` used to override the connection's `ProxyCommand` and + abort it with an "Unexpected SSH Config Option" error; now the extension's + options win and that error is gone. Your config is only written once, to add + the include, and the block for a deployment moves out of it on next connect. - Filter the Shared Workspaces view with the server-side `shared_with_user` query instead of filtering `shared:true` results on the client, so fewer workspaces are fetched and the view loads faster. Deployments too old to diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 0c29a0f5d..eafc631c9 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -62,7 +62,6 @@ import { applySettingOverrides, buildSshOverrides } from "./sshOverrides"; import { SshProcessMonitor } from "./sshProcess"; import { computeSshProperties, - findSshPropertyProblems, sshSupportsSetEnv, type SshProperties, } from "./sshSupport"; @@ -84,6 +83,13 @@ import type { SecretsManager } from "../core/secretsManager"; import type { Logger } from "../logging/logger"; import type { LoginCoordinator } from "../login/loginCoordinator"; +/** + * Our own config, included from the user's. Keep the tilde: relative includes + * always resolve against ~/.ssh, and an absolute path would not survive a + * config synced between machines. + */ +const CODER_SSH_CONFIG_PATH = "~/.ssh/coder/config"; + export interface RemoteDetails extends vscode.Disposable { safeHostname: string; url: string; @@ -897,19 +903,23 @@ export class Remote { featureSet: FeatureSet, cliAuth: CliAuth, ): Promise { - const sshConfigFile = this.getSshConfigPath(); - - const sshConfig = new SshConfig(sshConfigFile, this.logger); + // Our blocks live in our own file; the user's only gains the include. + const sshConfig = new SshConfig(this.getSshConfigPath(), this.logger); await sshConfig.load(); + const coderConfig = new SshConfig( + expandPath(CODER_SSH_CONFIG_PATH), + this.logger, + ); + await coderConfig.load(); // Options the user set themselves win the merge below, so they are exempt // from the deny list. Both sources are local and already trusted: whoever - // can write the SSH config could write any Host block directly, and the - // post-write check below still pins the critical options. + // can write the SSH config could write any Host block directly. const userConfigSsh = vscode.workspace .getConfiguration("coder") .get("sshConfig", []); const userConfig = parseSshConfig(userConfigSsh); + // The CLI writes its block to the user's config, so read it from there. const configSshOptions = parseCoderSshOptions(sshConfig.getRaw()); let deploymentSshConfig = {}; @@ -973,51 +983,15 @@ export class Remote { sshValues.SetEnv = "CODER_SSH_SESSION_TYPE=vscode"; } - await sshConfig.update(safeHostname, sshValues, sshConfigOverrides); + // Write our file before including it, so the include never dangles. + await coderConfig.update(safeHostname, sshValues, sshConfigOverrides); + await sshConfig.updateInclude(CODER_SSH_CONFIG_PATH, safeHostname); - // A user can provide a "Host *" entry in their SSH config to add options - // to all hosts. We need to ensure that the options we set are not - // overridden by the user's config. - const computedProperties = computeSshProperties( + // Mirror SSH's parse order; RemoteCommand can come from the user's config. + return computeSshProperties( hostName, - sshConfig.getRaw(), + `${coderConfig.getRaw()}\n${sshConfig.getRaw()}`, ); - const problems = findSshPropertyProblems(computedProperties, { - ProxyCommand: sshValues.ProxyCommand, - UserKnownHostsFile: sshValues.UserKnownHostsFile, - StrictHostKeyChecking: sshValues.StrictHostKeyChecking, - }); - if (problems.length > 0) { - await this.failSshConfigCheck(hostName, problems); - } - - return computedProperties; - } - - /** Show every unexpected option at once, close the remote, and abort. */ - private async failSshConfigCheck( - hostName: string, - problems: string[], - ): Promise { - const title = `Unexpected SSH Config Option${problems.length > 1 ? "s" : ""}`; - const detail = - `Your SSH config sets unexpected values for the "${hostName}" host. ` + - `Please fix the following and try again:\n\n` + - problems.map((problem) => `- ${problem}.`).join("\n"); - const result = await vscodeProposed.window.showErrorMessage( - title, - { - useCustom: true, - modal: true, - detail, - }, - "Reload Window", - ); - if (result === "Reload Window") { - await this.reloadWindow(); - } - await this.closeRemote(); - throw new Error("SSH config mismatch, closing remote"); } private watchSettings( diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index a1dfc095a..db12aae1b 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -62,6 +62,18 @@ const KEY_ONLY_REGEX = /^[a-zA-Z0-9-]+$/; /** Characters that would break a value out of its config line. */ const UNSAFE_CHARS_REGEX = /[\r\n\0]/; +const START_BLOCK_PREFIX = "# --- START CODER VSCODE"; +const END_BLOCK_PREFIX = "# --- END CODER VSCODE"; + +const INCLUDE_START = `${START_BLOCK_PREFIX} INCLUDE ---`; +const INCLUDE_END = `${END_BLOCK_PREFIX} INCLUDE ---`; + +/** Matches our include block wherever it currently sits. */ +const INCLUDE_BLOCK_REGEX = new RegExp( + `^${INCLUDE_START}$.*?^${INCLUDE_END}$`, + "ms", +); + /** * SSH options a deployment may not set, mirroring the server's validation of * --ssh-config-options (codersdk.ValidateSSHConfigOption). @@ -88,8 +100,8 @@ const DENIED_DEPLOYMENT_KEYS: ReadonlySet> = new Set([ ]); /** - * Options the post-write check in remote.ts pins to Coder's own value. Setting - * these in coder.sshConfig fails that check, so the error must not suggest it. + * Connection-critical options Coder always writes itself, so the error must + * not suggest overriding them in coder.sshConfig. */ const PINNED_KEYS: ReadonlySet> = new Set([ "proxycommand", @@ -288,10 +300,10 @@ export class SshConfig { private raw: string | undefined; private startBlockComment(safeHostname: string): string { - return `# --- START CODER VSCODE ${safeHostname} ---`; + return `${START_BLOCK_PREFIX} ${safeHostname} ---`; } private endBlockComment(safeHostname: string): string { - return `# --- END CODER VSCODE ${safeHostname} ---`; + return `${END_BLOCK_PREFIX} ${safeHostname} ---`; } constructor( @@ -338,6 +350,41 @@ export class SshConfig { await this.save(); } + /** + * Include `includePath` from the first line, so the options it holds win: + * SSH uses the first value it obtains for each one. Also drops the + * deployment's own block, which the included file supersedes. + */ + async updateInclude(includePath: string, safeHostname: string) { + const original = this.getRaw(); + const block = this.getBlock(safeHostname); + if (block) { + this.logger.debug("Removing superseded SSH config block", safeHostname); + this.removeBlock(block); + } + const include = [ + INCLUDE_START, + "# Your Coder workspaces, managed by the Coder VS Code extension. Keep first:", + "# SSH uses the first value found, so anything above this block overrides them.", + `Include ${includePath}`, + INCLUDE_END, + ].join("\n"); + const rest = this.getRaw().replace(INCLUDE_BLOCK_REGEX, "").trim(); + this.raw = rest ? `${include}\n\n${rest}` : include; + if (this.getRaw() !== original) { + this.logger.debug("Including SSH config", includePath); + await this.save(); + } + } + + private removeBlock(block: Block) { + const raw = this.getRaw(); + const start = raw.indexOf(block.raw); + const before = raw.slice(0, start).trimEnd(); + const after = raw.slice(start + block.raw.length).trimStart(); + this.raw = [before, after].filter(Boolean).join("\n\n"); + } + /** * Get the block for the deployment with the provided hostname. */ @@ -367,14 +414,6 @@ export class SshConfig { return; } - if (startBlockIndex === -1) { - throw new SshConfigBadFormat("Start block not found"); - } - - if (startBlockIndex === -1) { - throw new SshConfigBadFormat("End block not found"); - } - if (endBlockIndex < startBlockIndex) { throw new SshConfigBadFormat( "Malformed config, end block is before start block", @@ -414,8 +453,8 @@ export class SshConfig { const { Host, ...otherValues } = values; const lines = [ this.startBlockComment(safeHostname), - "# This section is managed by the Coder VS Code extension.", - "# Changes will be overwritten on the next workspace connection.", + "# Rewritten by the Coder VS Code extension on every connection.", + '# To change these options, use the "coder.sshConfig" setting instead.', `Host ${Host}`, ]; diff --git a/src/remote/sshSupport.ts b/src/remote/sshSupport.ts index 08f0b7fcb..b0e6f7d26 100644 --- a/src/remote/sshSupport.ts +++ b/src/remote/sshSupport.ts @@ -122,25 +122,3 @@ export function computeSshProperties( }); return merged; } - -/** - * Compare effective SSH properties against what Coder wrote, one problem per - * unexpected value. - */ -export function findSshPropertyProblems( - computed: SshProperties, - expected: Record, -): string[] { - const problems: string[] = []; - for (const [key, value] of Object.entries(expected)) { - const computedValue = computed[lowercase(key)]; - if (computedValue !== value) { - const actual = - computedValue === undefined - ? "is not set" - : `is set to "${computedValue}"`; - problems.push(`"${key}" ${actual}, but Coder expects "${value}"`); - } - } - return problems; -} diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index 8f875fbad..e4b24c5cb 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -17,8 +17,8 @@ import { createMockLogger } from "../../mocks/testHelpers"; const sshFilePath = "/Path/To/UserHomeDir/.sshConfigDir/sshConfigFile"; const sshTempFilePrefix = "/Path/To/UserHomeDir/.sshConfigDir/.sshConfigFile.vscode-coder-tmp-"; -const managedHeader = `# This section is managed by the Coder VS Code extension. -# Changes will be overwritten on the next workspace connection.`; +const managedHeader = `# Rewritten by the Coder VS Code extension on every connection. +# To change these options, use the "coder.sshConfig" setting instead.`; const mockFileSystem = { mkdir: vi.fn(), @@ -1097,3 +1097,70 @@ Host work-server }); }); }); + +describe("updateInclude", () => { + const include = `# --- START CODER VSCODE INCLUDE --- +# Your Coder workspaces, managed by the Coder VS Code extension. Keep first: +# SSH uses the first value found, so anything above this block overrides them. +Include ~/.ssh/coder/config +# --- END CODER VSCODE INCLUDE ---`; + + const managedBlock = `# --- START CODER VSCODE dev.coder.com --- +Host coder-vscode.dev.coder.com--* + ProxyCommand some-command-here +# --- END CODER VSCODE dev.coder.com ---`; + + /** Include our config in `existing`, returning what was written, if anything. */ + async function updateInclude(existing: string): Promise { + mockFileSystem.readFile.mockResolvedValueOnce(existing); + mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); + await sshConfig.load(); + await sshConfig.updateInclude("~/.ssh/coder/config", "dev.coder.com"); + return mockFileSystem.writeFile.mock.calls.at(-1)?.[1] as + string | undefined; + } + + it("goes above everything the user wrote", async () => { + const config = + "AddKeysToAgent yes\n\nInclude ~/.ssh/work\n\nHost *\n ConnectTimeout 5"; + + await expect(updateInclude(config)).resolves.toBe( + `${include}\n\n${config}`, + ); + }); + + it("creates the include in an empty config", async () => { + await expect(updateInclude("")).resolves.toBe(include); + }); + + it("leaves the file alone when the include is already first", async () => { + await expect( + updateInclude(`${include}\n\nHost *`), + ).resolves.toBeUndefined(); + }); + + it("moves an include that is no longer first", async () => { + const config = "Host *\n ConnectTimeout 5"; + + await expect(updateInclude(`${config}\n\n${include}`)).resolves.toBe( + `${include}\n\n${config}`, + ); + }); + + it("drops the block the included file supersedes", async () => { + const config = "Host *\n ConnectTimeout 5"; + + await expect(updateInclude(`${config}\n\n${managedBlock}`)).resolves.toBe( + `${include}\n\n${config}`, + ); + }); + + it("keeps blocks belonging to other deployments", async () => { + const other = managedBlock.replaceAll("dev.coder.com", "dev2.coder.com"); + + await expect(updateInclude(`${other}\n\n${managedBlock}`)).resolves.toBe( + `${include}\n\n${other}`, + ); + }); +}); diff --git a/test/unit/remote/sshSupport.test.ts b/test/unit/remote/sshSupport.test.ts index bfebb56e7..124fa608e 100644 --- a/test/unit/remote/sshSupport.test.ts +++ b/test/unit/remote/sshSupport.test.ts @@ -2,7 +2,6 @@ import { describe, it, expect } from "vitest"; import { computeSshProperties, - findSshPropertyProblems, sshSupportsSetEnv, sshVersionSupportsSetEnv, } from "@/remote/sshSupport"; @@ -193,32 +192,3 @@ Host coder-vscode.dev.coder.com--* }); }); }); - -describe("findSshPropertyProblems", () => { - const EXPECTED = { - ProxyCommand: "coder ssh --stdio %h", - StrictHostKeyChecking: "no", - }; - const MATCHING = { - proxycommand: "coder ssh --stdio %h", - stricthostkeychecking: "no", - }; - - it("passes when values match, ignoring options Coder does not pin", () => { - expect( - findSshPropertyProblems( - { ...MATCHING, localcommand: "echo hello" }, - EXPECTED, - ), - ).toEqual([]); - }); - - it("reports every mismatch at once", () => { - expect(findSshPropertyProblems({ proxycommand: "evil" }, EXPECTED)).toEqual( - [ - '"ProxyCommand" is set to "evil", but Coder expects "coder ssh --stdio %h"', - '"StrictHostKeyChecking" is not set, but Coder expects "no"', - ], - ); - }); -}); From 16fcc4962ed54cd966bba8fca91dfc0aeed9ac0d Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 4 Aug 2026 02:14:11 +0300 Subject: [PATCH 2/8] fix: ignore the SSH config file setting where ssh never reads it Antigravity and Windsurf/Devin renamed the setting to remote.antigravitySSH.configFile and remote.devinSSH.configFile, then spawn ssh without -F, so ssh reads ~/.ssh/config no matter what any of them say. The setting only feeds their own host tree. Honoring it, or a stale remote.SSH.configFile synced in from another editor, writes the workspace host to a file the connection never reads. Ignore it on those two and keep reading remote.SSH.configFile elsewhere: Microsoft's extension and Cursor's fork pass it to ssh with -F, and VSCodium's fork parses the file itself instead of running ssh. This drops the per-extension section map from #1060: the three extensions that do connect through the setting all read remote.SSH. --- CHANGELOG.md | 26 +++++++--- CONTRIBUTING.md | 7 +++ src/remote/remote.ts | 4 +- src/remote/sshExtension.ts | 36 ++++++------- test/unit/remote/sshExtension.test.ts | 75 +++++++++++---------------- 5 files changed, 72 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8059de4aa..eb0da22d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,25 @@ from published versions since it shows up in the VS Code extension changelog tab and is confusing to users. Add it back between releases if needed. --> +## Unreleased + +### Changed + +- Write workspace SSH hosts to `~/.ssh/coder/config` and include that file + from the top of your own SSH config, rather than writing the block into your + config directly. SSH uses the first value it obtains for each option, so a + catch-all like `Host *` used to override the connection's `ProxyCommand` and + abort it with an "Unexpected SSH Config Option" error; now the extension's + options win and that error is gone. Your config is only written once, to add + the include, and the block for a deployment moves out of it on next connect. + +### Fixed + +- Ignore the SSH config file setting on Antigravity and Windsurf/Devin. They + launch ssh without pointing it at a config file, so it always reads + `~/.ssh/config`, and honoring the setting wrote the workspace host where the + connection never looked. + ## [v1.16.0](https://github.com/coder/vscode-coder/releases/tag/v1.16.0) 2026-08-06 ### Added @@ -22,13 +41,6 @@ ### Changed -- Write workspace SSH hosts to `~/.ssh/coder/config` and include that file - from the top of your own SSH config, rather than writing the block into your - config directly. SSH uses the first value it obtains for each option, so a - catch-all like `Host *` used to override the connection's `ProxyCommand` and - abort it with an "Unexpected SSH Config Option" error; now the extension's - options win and that error is gone. Your config is only written once, to add - the include, and the block for a deployment moves out of it on next connect. - Filter the Shared Workspaces view with the server-side `shared_with_user` query instead of filtering `shared:true` results on the client, so fewer workspaces are fetched and the view loads faster. Deployments too old to diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 66a9ef5ec..9939445ec 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,6 +41,13 @@ Host coder-vscode.dev.coder.com--* LogLevel ERROR ``` +Which file that entry goes in depends on the Remote - SSH extension. Microsoft's +and Cursor's pass `remote.SSH.configFile` to ssh with `-F`, and VSCodium's parses +the file itself instead of running ssh, so all three connect through it. +Antigravity and Windsurf/Devin renamed the setting but spawn ssh without `-F`, so +ssh reads `~/.ssh/config` regardless; we ignore it there rather than write the +host where the connection never looks. + If any step fails, we show an error message. Once the error message is closed we close the remote so the Remote - SSH connection does not continue to connection. Otherwise, we yield, which lets the Remote - SSH continue. diff --git a/src/remote/remote.ts b/src/remote/remote.ts index eafc631c9..4c1ed2fe0 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -57,7 +57,7 @@ import { parseSshConfig, validateDeploymentSshOptions, } from "./sshConfig"; -import { getRemoteSshSetting } from "./sshExtension"; +import { getRemoteSshConfigFile } from "./sshExtension"; import { applySettingOverrides, buildSshOverrides } from "./sshOverrides"; import { SshProcessMonitor } from "./sshProcess"; import { @@ -888,7 +888,7 @@ export class Remote { } private getSshConfigPath(): string { - const configured = getRemoteSshSetting("configFile"); + const configured = getRemoteSshConfigFile(); return expandPath(configured || path.join("~", ".ssh", "config")); } diff --git a/src/remote/sshExtension.ts b/src/remote/sshExtension.ts index a56e98060..f2d80dec4 100644 --- a/src/remote/sshExtension.ts +++ b/src/remote/sshExtension.ts @@ -11,31 +11,25 @@ export const REMOTE_SSH_EXTENSION_IDS = [ export type RemoteSshExtensionId = (typeof REMOTE_SSH_EXTENSION_IDS)[number]; /** - * Sections each extension reads, in order. The rebranded forks renamed the - * whole `remote.SSH` section, so reading it directly misses them. + * Extensions that spawn ssh without `-F`, so it reads ~/.ssh/config whatever + * their renamed setting says. Honoring one would write the workspace host + * where the connection never looks. */ -const SETTING_SECTIONS: Readonly< - Record -> = { - "jeanp413.open-remote-ssh": ["remote.SSH"], - // Windsurf became Devin and reads both, preferring the new name. - "codeium.windsurf-remote-openssh": ["remote.devinSSH", "remote.windsurfSSH"], - "anysphere.remote-ssh": ["remote.SSH"], - "ms-vscode-remote.remote-ssh": ["remote.SSH"], - "google.antigravity-remote-openssh": ["remote.antigravitySSH"], -}; +const IGNORED_CONFIG_FILE: readonly RemoteSshExtensionId[] = [ + "google.antigravity-remote-openssh", + "codeium.windsurf-remote-openssh", +]; -/** First non-empty value for a string setting, e.g. `configFile`. */ -export function getRemoteSshSetting(key: string): string | undefined { +/** The SSH config file the active extension connects through, if configured. */ +export function getRemoteSshConfigFile(): string | undefined { const id = getRemoteSshExtension()?.id; - const sections = id ? SETTING_SECTIONS[id] : ["remote.SSH"]; - for (const section of sections) { - const value = vscode.workspace.getConfiguration(section).get(key); - if (value) { - return value; - } + if (id && IGNORED_CONFIG_FILE.includes(id)) { + return undefined; } - return undefined; + return ( + vscode.workspace.getConfiguration("remote.SSH").get("configFile") || + undefined + ); } /** diff --git a/test/unit/remote/sshExtension.test.ts b/test/unit/remote/sshExtension.test.ts index 6aa4a7f4e..a59fc2211 100644 --- a/test/unit/remote/sshExtension.test.ts +++ b/test/unit/remote/sshExtension.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; -import { getRemoteSshSetting } from "@/remote/sshExtension"; +import { getRemoteSshConfigFile } from "@/remote/sshExtension"; import { config, type Settings } from "../../mocks/testHelpers"; @@ -13,58 +13,41 @@ function setup(extensionId: string, settings: Settings = {}): void { ); } -describe("getRemoteSshSetting", () => { - interface SectionCase { - id: string; - key: string; - } +describe("getRemoteSshConfigFile", () => { + it.each([ + "ms-vscode-remote.remote-ssh", + "anysphere.remote-ssh", + "jeanp413.open-remote-ssh", + ])("reads the configured file for %s", (extensionId) => { + setup(extensionId, { "remote.SSH.configFile": "/custom/config" }); - it.each([ - { id: "ms-vscode-remote.remote-ssh", key: "remote.SSH.configFile" }, - { id: "anysphere.remote-ssh", key: "remote.SSH.configFile" }, - { id: "jeanp413.open-remote-ssh", key: "remote.SSH.configFile" }, - { - id: "google.antigravity-remote-openssh", - key: "remote.antigravitySSH.configFile", - }, - { - id: "codeium.windsurf-remote-openssh", - key: "remote.devinSSH.configFile", - }, - ])("reads the section $id uses", ({ id, key }) => { - setup(id, { [key]: "/custom/config" }); - - expect(getRemoteSshSetting("configFile")).toBe("/custom/config"); - }); - - it("falls back to the legacy Windsurf section", () => { - setup("codeium.windsurf-remote-openssh", { - "remote.windsurfSSH.configFile": "/legacy/config", - }); - - expect(getRemoteSshSetting("configFile")).toBe("/legacy/config"); + expect(getRemoteSshConfigFile()).toBe("/custom/config"); }); - it("prefers the Devin section over the legacy Windsurf one", () => { - setup("codeium.windsurf-remote-openssh", { - "remote.devinSSH.configFile": "/devin/config", - "remote.windsurfSSH.configFile": "/legacy/config", - }); - - expect(getRemoteSshSetting("configFile")).toBe("/devin/config"); - }); + it.each([ + ["google.antigravity-remote-openssh", "remote.antigravitySSH.configFile"], + ["codeium.windsurf-remote-openssh", "remote.devinSSH.configFile"], + ])( + "ignores the file %s never connects through", + (extensionId, settingKey) => { + setup(extensionId, { + [settingKey]: "/custom/config", + "remote.SSH.configFile": "/stale/config", + }); + + expect(getRemoteSshConfigFile()).toBeUndefined(); + }, + ); - it("ignores another extension's section", () => { - setup("google.antigravity-remote-openssh", { - "remote.SSH.configFile": "/custom/config", - }); + it("reads remote.SSH when no extension is installed", () => { + setup("", { "remote.SSH.configFile": "/custom/config" }); - expect(getRemoteSshSetting("configFile")).toBeUndefined(); + expect(getRemoteSshConfigFile()).toBe("/custom/config"); }); - it("defaults to remote.SSH when no extension is installed", () => { - setup("", { "remote.SSH.configFile": "/custom/config" }); + it("returns undefined when nothing is configured", () => { + setup("ms-vscode-remote.remote-ssh"); - expect(getRemoteSshSetting("configFile")).toBe("/custom/config"); + expect(getRemoteSshConfigFile()).toBeUndefined(); }); }); From 9ad80f4a1e0d01568ee63d2bb56e572392209181 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 4 Aug 2026 14:24:46 +0300 Subject: [PATCH 3/8] chore: say the include block is moved, not kept, at the top --- src/remote/sshConfig.ts | 5 +++-- test/unit/remote/sshConfig.test.ts | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index db12aae1b..5f1fb01a6 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -364,8 +364,9 @@ export class SshConfig { } const include = [ INCLUDE_START, - "# Your Coder workspaces, managed by the Coder VS Code extension. Keep first:", - "# SSH uses the first value found, so anything above this block overrides them.", + "# Your Coder workspaces, managed by the Coder VS Code extension.", + "# This block moves back to the top on every connect, since SSH uses the first", + "# value it finds. To override these options, use the coder.sshConfig setting.", `Include ${includePath}`, INCLUDE_END, ].join("\n"); diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index e4b24c5cb..698075887 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -1100,8 +1100,9 @@ Host work-server describe("updateInclude", () => { const include = `# --- START CODER VSCODE INCLUDE --- -# Your Coder workspaces, managed by the Coder VS Code extension. Keep first: -# SSH uses the first value found, so anything above this block overrides them. +# Your Coder workspaces, managed by the Coder VS Code extension. +# This block moves back to the top on every connect, since SSH uses the first +# value it finds. To override these options, use the coder.sshConfig setting. Include ~/.ssh/coder/config # --- END CODER VSCODE INCLUDE ---`; From 1c90eec7823da77b4dcbf78be6b04b261ec1e775 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 5 Aug 2026 16:23:45 +0300 Subject: [PATCH 4/8] fix: isolate SSH configs per editor --- CHANGELOG.md | 20 +- CONTRIBUTING.md | 52 +- package.json | 16 +- src/commands.ts | 34 +- src/core/commandManager.ts | 1 + src/core/pathResolver.ts | 4 + src/extension.ts | 4 + src/remote/remote.ts | 99 +- src/remote/sshConfig.ts | 377 +++-- src/remote/sshExtension.ts | 5 +- src/util/authority.ts | 117 +- test/mocks/testHelpers.ts | 33 +- test/mocks/vscode.runtime.ts | 58 +- test/unit/core/pathResolver.test.ts | 9 + test/unit/remote/remote.test.ts | 231 ++- test/unit/remote/sshConfig.openssh.test.ts | 134 ++ test/unit/remote/sshConfig.test.ts | 1516 ++++++++------------ test/unit/util/authority.test.ts | 363 ++--- 18 files changed, 1718 insertions(+), 1355 deletions(-) create mode 100644 test/unit/remote/sshConfig.openssh.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index eb0da22d9..827c72f4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,15 +7,21 @@ ## Unreleased +### Added + +- Add a **Coder: Open Generated SSH Configuration File** command that opens + this editor's generated workspace hosts file in a read-only editor. + ### Changed -- Write workspace SSH hosts to `~/.ssh/coder/config` and include that file - from the top of your own SSH config, rather than writing the block into your - config directly. SSH uses the first value it obtains for each option, so a - catch-all like `Host *` used to override the connection's `ProxyCommand` and - abort it with an "Unexpected SSH Config Option" error; now the extension's - options win and that error is gone. Your config is only written once, to add - the include, and the block for a deployment moves out of it on next connect. +- Write workspace SSH hosts to an editor-owned file in the extension's global + storage and include it from the top of your SSH config, instead of writing + the block into your config directly. Each VS Code-based editor gets its own + include and SSH host prefix, so VS Code, Cursor, Windsurf, and other clones + no longer overwrite one another's proxy settings, and legacy `coder-vscode` + authorities are migrated in other editors by automatically reopening the + window once. SSH uses the first value it finds for each option, so a + catch-all `Host *` can no longer override the connection's `ProxyCommand`. ### Fixed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9939445ec..77de98169 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,39 +14,57 @@ The `ssh-remote` scheme is registered by Microsoft's Remote - SSH extension and indicates that it should connect to the provided host name using SSH. The host name takes the format -`coder-vscode.----`. This is parsed by the CLI -(which is invoked via SSH's `ProxyCommand`) to route SSH to the right workspace. +`coder-.----`, where `` comes from +that product's URI scheme, such as `vscode`, `cursor`, or `windsurf`. The CLI is +invoked through SSH's `ProxyCommand` with this prefix so it can route SSH to the +right workspace. A legacy `coder-vscode` authority opened in another editor is +reopened once with that editor's prefix; legacy recent-folder entries remain +compatible when opening the same workspace. The Coder Remote extension also registers for the `onResolveRemoteAuthority:ssh-remote` [extension activation event](https://code.visualstudio.com/api/references/activation-events) to hook into this process, running before the Remote - SSH extension actually connects. -On activation of this event, we check if `vscode.workspace.workspaceFolders` -contains the `coder-vscode` prefix, and if so we delay activation to: +On activation of this event, we check whether the remote authority belongs to +the current editor, and if so we delay activation to: 1. Parse the host name to get the domain, username, and workspace. 2. Ensure the workspace is running. 3. Download the matching server binary to the client. 4. Configure the binary with the URL and token, asking the user for them if they are missing. Each domain gets its own config directory. -5. Add an entry to the user's SSH config for `coder-vscode.--*`. +5. Write an entry for `coder-.--*` to `ssh-config` in the + extension's global storage directory. +6. Add a per-editor `Include` block at the top of the user's SSH config. ```text -Host coder-vscode.dev.coder.com--* - ProxyCommand "/tmp/coder" --global-config "/home/kyle/.config/Code/User/globalStorage/coder.coder-remote/dev.coder.com" ssh --stdio --network-info-dir "/home/kyle/.config/Code/User/globalStorage/coder.coder-remote/net" --ssh-host-prefix coder-vscode.dev.coder.com-- %h - ConnectTimeout 0 - StrictHostKeyChecking no - UserKnownHostsFile /dev/null - LogLevel ERROR +# --- START CODER cursor --- +Include "~/.config/Cursor/User/globalStorage/coder.coder-remote/ssh-config" +# --- END CODER cursor --- + +# --- START CODER vscode --- +Include "~/.config/Code/User/globalStorage/coder.coder-remote/ssh-config" +# --- END CODER vscode --- +``` + +Each included file contains only that editor's host entries: + +```text +Host coder-cursor.dev.coder.com--* + ProxyCommand "/tmp/coder" --global-config "/home/kyle/.config/Cursor/User/globalStorage/coder.coder-remote/dev.coder.com" ssh --stdio --network-info-dir "/home/kyle/.config/Cursor/User/globalStorage/coder.coder-remote/net" --ssh-host-prefix coder-cursor.dev.coder.com-- %h + ConnectTimeout 0 + StrictHostKeyChecking no + UserKnownHostsFile /dev/null + LogLevel ERROR ``` -Which file that entry goes in depends on the Remote - SSH extension. Microsoft's -and Cursor's pass `remote.SSH.configFile` to ssh with `-F`, and VSCodium's parses -the file itself instead of running ssh, so all three connect through it. -Antigravity and Windsurf/Devin renamed the setting but spawn ssh without `-F`, so -ssh reads `~/.ssh/config` regardless; we ignore it there rather than write the -host where the connection never looks. +Which main file gains the include depends on the Remote - SSH extension. +Microsoft's and Cursor's pass `remote.SSH.configFile` to ssh with `-F`, and +VSCodium's parses the file itself instead of running ssh, so all three connect +through it. Antigravity and Windsurf/Devin renamed the setting but spawn ssh +without `-F`, so ssh reads `~/.ssh/config` regardless; we ignore the renamed +setting there rather than add the include where the connection never looks. If any step fails, we show an error message. Once the error message is closed we close the remote so the Remote - SSH connection does not continue to diff --git a/package.json b/package.json index e534270e7..c6b1f4537 100644 --- a/package.json +++ b/package.json @@ -478,12 +478,20 @@ }, { "command": "coder.viewLogs", - "title": "Coder: View Logs", + "title": "View Logs", + "category": "Coder", "icon": "$(list-unordered)" }, + { + "command": "coder.openSshConfig", + "title": "Open Generated SSH Configuration File", + "category": "Coder", + "icon": "$(file-code)" + }, { "command": "coder.exportTelemetry", - "title": "Coder: Export Telemetry", + "title": "Export Telemetry", + "category": "Coder", "icon": "$(save)" }, { @@ -616,6 +624,10 @@ "command": "coder.viewLogs", "when": "true" }, + { + "command": "coder.openSshConfig", + "when": "true" + }, { "command": "coder.exportTelemetry", "when": "true" diff --git a/src/commands.ts b/src/commands.ts index 8c38b4231..05fe1e6b0 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -50,7 +50,10 @@ import { toRemoteLogGlobs, } from "./supportBundle/remoteServerDataPath"; import { runExportTelemetryCommand } from "./telemetry/export/command"; -import { toRemoteAuthority } from "./util/authority"; +import { + isRemoteAuthorityCompatible, + toRemoteAuthority, +} from "./util/authority"; import { openInBrowser, toSafeHost } from "./util/uri"; import { vscodeProposed } from "./vscodeProposed"; import { parseNetcheckReport } from "./webviews/netcheck/types"; @@ -565,6 +568,24 @@ export class Commands { ); } + /** + * Open this editor's generated SSH config with the Coder workspace hosts. + */ + public async openSshConfig(): Promise { + const configPath = this.pathResolver.getSshConfigPath(); + try { + await openFile(configPath); + // The file is rewritten on every connection, so edits would be lost. + await vscode.commands.executeCommand( + "workbench.action.files.setActiveEditorReadonlyInSession", + ); + } catch { + vscode.window.showInformationMessage( + "No SSH config has been generated yet. It is written when you connect to a workspace.", + ); + } + } + /** * View the logs for the currently connected workspace. */ @@ -1475,12 +1496,11 @@ export class Commands { const output: { workspaces: Array<{ folderUri: vscode.Uri; remoteAuthority: string }>; } = await vscode.commands.executeCommand("_workbench.getRecentlyOpened"); - const opened = output.workspaces.filter( - // Remove recents that do not belong to this connection. The remote - // authority maps to a workspace/agent combination (using the SSH host - // name). There may also be some legacy connections that still may - // reference a workspace without an agent name, which will be missed. - (opened) => opened.folderUri?.authority === remoteAuthority, + const opened = output.workspaces.filter((opened) => + isRemoteAuthorityCompatible( + opened.folderUri?.authority, + remoteAuthority, + ), ); // openRecent will always use the most recent. Otherwise, if there are // multiple we ask the user which to use. diff --git a/src/core/commandManager.ts b/src/core/commandManager.ts index c9f721c3c..6868b3121 100644 --- a/src/core/commandManager.ts +++ b/src/core/commandManager.ts @@ -20,6 +20,7 @@ export const CODER_COMMAND_IDS = [ "coder.navigateToWorkspaceSettings", "coder.refreshWorkspaces", "coder.viewLogs", + "coder.openSshConfig", "coder.exportTelemetry", "coder.viewAnnouncements", "coder.searchMyWorkspaces", diff --git a/src/core/pathResolver.ts b/src/core/pathResolver.ts index 191534ba6..4327f2f8d 100644 --- a/src/core/pathResolver.ts +++ b/src/core/pathResolver.ts @@ -42,6 +42,10 @@ export class PathResolver { return path.join(this.basePath, "net"); } + public getSshConfigPath(): string { + return path.join(this.basePath, "ssh-config"); + } + /** * Return the directory where telemetry files are written. */ diff --git a/src/extension.ts b/src/extension.ts index 6dc82209e..67dd15672 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -350,6 +350,10 @@ async function doActivate( void allWorkspacesProvider.fetchAndRefresh(); }); commandManager.register("coder.viewLogs", commands.viewLogs.bind(commands)); + commandManager.register( + "coder.openSshConfig", + commands.openSshConfig.bind(commands), + ); commandManager.register( "coder.exportTelemetry", commands.exportTelemetry.bind(commands), diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 4c1ed2fe0..3d4e1cfb6 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -39,9 +39,11 @@ import { import { getHeaderCommand } from "../settings/headers"; import { escapeCommandArg, expandPath } from "../util"; import { - AuthorityPrefix, type AuthorityParts, + classifyRemoteAuthority, parseRemoteAuthority, + retargetRemoteAuthority, + toCurrentAuthorityHostPrefix, } from "../util/authority"; import { createStatusBarItem } from "../util/statusBar"; import { vscodeProposed } from "../vscodeProposed"; @@ -83,13 +85,6 @@ import type { SecretsManager } from "../core/secretsManager"; import type { Logger } from "../logging/logger"; import type { LoginCoordinator } from "../login/loginCoordinator"; -/** - * Our own config, included from the user's. Keep the tilde: relative includes - * always resolve against ~/.ssh, and an absolute path would not survive a - * config synced between machines. - */ -const CODER_SSH_CONFIG_PATH = "~/.ssh/coder/config"; - export interface RemoteDetails extends vscode.Disposable { safeHostname: string; url: string; @@ -166,6 +161,16 @@ export class Remote { return; } + switch (classifyRemoteAuthority(parts)) { + case "current": + break; + case "legacy": + await this.migrateLegacyAuthority(remoteAuthority, startupMode); + return; + case "foreign": + return; + } + this.logger.info("Setting up remote connection", { remoteAuthority, hostname: parts.safeHostname, @@ -724,6 +729,66 @@ export class Remote { return undefined; } + private async migrateLegacyAuthority( + remoteAuthority: string, + startupMode: StartupMode, + ): Promise { + const migratedAuthority = retargetRemoteAuthority(remoteAuthority); + const workspaceFile = vscode.workspace.workspaceFile; + const workspaceFolders = vscode.workspace.workspaceFolders ?? []; + const savedWorkspaceFile = + workspaceFile?.scheme === "untitled" ? undefined : workspaceFile; + if (!savedWorkspaceFile && workspaceFolders.length > 1) { + this.logger.warn( + "Cannot migrate an unsaved multi-root workspace", + remoteAuthority, + ); + const choice = await vscodeProposed.window.showWarningMessage( + "Opening the remote over the old coder-vscode SSH host", + { + modal: true, + useCustom: true, + detail: + "This editor now uses its own SSH hosts, but switching an unsaved multi-root workspace would drop its folders. " + + "To switch, save the workspace, then reload the window.", + }, + "Learn More", + ); + if (choice === "Learn More") { + await vscode.env.openExternal( + vscode.Uri.parse( + "https://code.visualstudio.com/docs/editing/workspaces/multi-root-workspaces", + ), + ); + } + return; + } + + await this.serviceContainer + .getMementoManager() + .setStartupMode(startupMode === "none" ? "start" : startupMode); + this.logger.info("Migrating legacy remote authority", { + from: remoteAuthority, + to: migratedAuthority, + }); + + const currentUri = savedWorkspaceFile ?? workspaceFolders[0]?.uri; + if (currentUri) { + await vscode.commands.executeCommand( + "vscode.openFolder", + currentUri.with({ + authority: retargetRemoteAuthority(currentUri.authority), + }), + false, + ); + return; + } + await vscode.commands.executeCommand("vscode.newWindow", { + remoteAuthority: migratedAuthority, + reuseWindow: true, + }); + } + private async resolveRemoteBinary(workspaceClient: Api): Promise { if ( this.extensionContext.extensionMode === vscode.ExtensionMode.Production @@ -906,10 +971,8 @@ export class Remote { // Our blocks live in our own file; the user's only gains the include. const sshConfig = new SshConfig(this.getSshConfigPath(), this.logger); await sshConfig.load(); - const coderConfig = new SshConfig( - expandPath(CODER_SSH_CONFIG_PATH), - this.logger, - ); + const coderConfigPath = this.pathResolver.getSshConfigPath(); + const coderConfig = new SshConfig(coderConfigPath, this.logger); await coderConfig.load(); // Options the user set themselves win the merge below, so they are exempt @@ -954,9 +1017,7 @@ export class Remote { userConfig, ); - const hostPrefix = safeHostname - ? `${AuthorityPrefix}.${safeHostname}--` - : `${AuthorityPrefix}--`; + const hostPrefix = toCurrentAuthorityHostPrefix(safeHostname); const proxyCommand = await this.buildProxyCommand( binaryPath, @@ -985,7 +1046,13 @@ export class Remote { // Write our file before including it, so the include never dangles. await coderConfig.update(safeHostname, sshValues, sshConfigOverrides); - await sshConfig.updateInclude(CODER_SSH_CONFIG_PATH, safeHostname); + await sshConfig.updateInclude( + { + id: vscode.env.uriScheme, + includePath: coderConfigPath, + }, + safeHostname, + ); // Mirror SSH's parse order; RemoteCommand can come from the user's config. return computeSshProperties( diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index 5f1fb01a6..e96ed5d7c 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -6,6 +6,7 @@ import { unlink, writeFile, } from "node:fs/promises"; +import * as os from "node:os"; import path from "node:path"; import { countSubstring, lowercase } from "../util"; @@ -13,6 +14,19 @@ import { renameWithRetry, tempFilePath } from "../util/fs"; import type { Logger } from "../logging/logger"; +class SshConfigBadFormat extends Error {} + +interface Block { + raw: string; + start: number; + end: number; +} + +interface Mutation { + apply(raw: string): string; + onSuccess?(): void; +} + export interface SshValues { Host: string; ProxyCommand: string; @@ -44,12 +58,6 @@ const defaultFileSystem: FileSystem = { writeFile, }; -class SshConfigBadFormat extends Error {} - -interface Block { - raw: string; -} - /** Matches an SSH config key at the start of a line (e.g. "ConnectTimeout", "LogLevel"). */ const SSH_KEY_REGEX = /^[a-zA-Z0-9-]+/; @@ -62,17 +70,36 @@ const KEY_ONLY_REGEX = /^[a-zA-Z0-9-]+$/; /** Characters that would break a value out of its config line. */ const UNSAFE_CHARS_REGEX = /[\r\n\0]/; -const START_BLOCK_PREFIX = "# --- START CODER VSCODE"; -const END_BLOCK_PREFIX = "# --- END CODER VSCODE"; +const UPDATE_ATTEMPTS = 3; -const INCLUDE_START = `${START_BLOCK_PREFIX} INCLUDE ---`; -const INCLUDE_END = `${END_BLOCK_PREFIX} INCLUDE ---`; +interface BlockMarkers { + start: string; + end: string; +} -/** Matches our include block wherever it currently sits. */ -const INCLUDE_BLOCK_REGEX = new RegExp( - `^${INCLUDE_START}$.*?^${INCLUDE_END}$`, - "ms", -); +// Labels are an editor ID for include blocks in the user's config and a +// deployment hostname for blocks in the editor-owned generated file. +function blockMarkers(label: string): BlockMarkers { + return { + start: `# --- START CODER ${label} ---`, + end: `# --- END CODER ${label} ---`, + }; +} + +// Released versions wrote deployment blocks with this label, both into the +// user's config and via early builds of the editor-owned file. +function legacyDeploymentMarkers(safeHostname: string): BlockMarkers { + return blockMarkers(`VSCODE ${safeHostname}`); +} + +// Kept at the top of the editor-owned generated file. +const CODER_SSH_CONFIG_HEADER = `# Coder workspace hosts. Do not edit; the Coder extension rewrites this file +# on every connection. Override options with the "coder.sshConfig" setting.`; + +export interface SshInclude { + id: string; + includePath: string; +} /** * SSH options a deployment may not set, mirroring the server's validation of @@ -299,13 +326,6 @@ export class SshConfig { private readonly logger: Logger; private raw: string | undefined; - private startBlockComment(safeHostname: string): string { - return `${START_BLOCK_PREFIX} ${safeHostname} ---`; - } - private endBlockComment(safeHostname: string): string { - return `${END_BLOCK_PREFIX} ${safeHostname} ---`; - } - constructor( filePath: string, logger: Logger, @@ -320,7 +340,10 @@ export class SshConfig { try { this.raw = await this.fileSystem.readFile(this.filePath, "utf-8"); this.logger.debug("Loaded SSH config", this.filePath); - } catch { + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error; + } this.logger.debug( "SSH config file not found, starting fresh", this.filePath, @@ -338,165 +361,194 @@ export class SshConfig { values: SshValues, overrides?: Record, ) { - const newBlock = this.buildBlock(safeHostname, values, overrides); - const block = this.getBlock(safeHostname); - if (block) { - this.logger.debug("Replacing SSH config block", safeHostname); - this.replaceBlock(block, newBlock); - } else { - this.logger.debug("Appending new SSH config block", safeHostname); - this.appendBlock(newBlock); + const block = this.renderDeploymentBlock(safeHostname, values, overrides); + await this.mutate({ + apply: (raw) => this.mergeDeployment(raw, safeHostname, block), + }); + } + + /** Include an editor's config first so its options win, removing its deployment block. */ + async updateInclude(include: SshInclude, safeHostname: string) { + const block = this.renderIncludeBlock(include); + await this.mutate({ + apply: (raw) => this.mergeInclude(raw, include.id, block, safeHostname), + onSuccess: () => + this.logger.debug("Including SSH config", include.includePath), + }); + } + + public getRaw() { + if (this.raw === undefined) { + throw new Error("SshConfig is not loaded. Try sshConfig.load()"); } - await this.save(); + + return this.raw; } /** - * Include `includePath` from the first line, so the options it holds win: - * SSH uses the first value it obtains for each one. Also drops the - * deployment's own block, which the included file supersedes. + * Render the deployment's block, validating everything written into it, + * including the hostname, which lands in the block marker comments. + * @throws {Error} when the hostname, values, or overrides fail validation. */ - async updateInclude(includePath: string, safeHostname: string) { - const original = this.getRaw(); - const block = this.getBlock(safeHostname); - if (block) { - this.logger.debug("Removing superseded SSH config block", safeHostname); - this.removeBlock(block); - } - const include = [ - INCLUDE_START, - "# Your Coder workspaces, managed by the Coder VS Code extension.", - "# This block moves back to the top on every connect, since SSH uses the first", - "# value it finds. To override these options, use the coder.sshConfig setting.", - `Include ${includePath}`, - INCLUDE_END, - ].join("\n"); - const rest = this.getRaw().replace(INCLUDE_BLOCK_REGEX, "").trim(); - this.raw = rest ? `${include}\n\n${rest}` : include; - if (this.getRaw() !== original) { - this.logger.debug("Including SSH config", includePath); - await this.save(); - } + private renderDeploymentBlock( + safeHostname: string, + values: SshValues, + overrides?: Record, + ): string { + validateSshValue("deployment hostname", safeHostname); + validateSshConfigOptions({ ...values }); + validateSshConfigOptions(overrides ?? {}); + const { Host, ...defaults } = values; + const config = mergeSshConfigValues(defaults, overrides ?? {}); + const options = Object.keys(config) + .sort() + .filter((key) => config[key] !== "") + .map((key) => ` ${key} ${config[key]}`); + const markers = blockMarkers(safeHostname); + return [markers.start, `Host ${Host}`, ...options, markers.end].join("\n"); } - private removeBlock(block: Block) { - const raw = this.getRaw(); - const start = raw.indexOf(block.raw); - const before = raw.slice(0, start).trimEnd(); - const after = raw.slice(start + block.raw.length).trimStart(); - this.raw = [before, after].filter(Boolean).join("\n\n"); + private mergeDeployment( + raw: string, + safeHostname: string, + block: string, + ): string { + let merged: string; + const existing = + this.findBlock(raw, blockMarkers(safeHostname)) ?? + this.findBlock(raw, legacyDeploymentMarkers(safeHostname)); + if (existing) { + this.logger.debug("Replacing SSH config block", safeHostname); + merged = this.replaceRange(raw, existing, block); + } else { + this.logger.debug("Appending new SSH config block", safeHostname); + merged = raw ? `${raw.trimEnd()}\n\n${block}` : block; + } + return merged.startsWith(CODER_SSH_CONFIG_HEADER) + ? merged + : `${CODER_SSH_CONFIG_HEADER}\n\n${merged}`; } - /** - * Get the block for the deployment with the provided hostname. - */ - private getBlock(safeHostname: string): Block | undefined { - const raw = this.getRaw(); - const startBlock = this.startBlockComment(safeHostname); - const endBlock = this.endBlockComment(safeHostname); - - const startBlockCount = countSubstring(startBlock, raw); - const endBlockCount = countSubstring(endBlock, raw); - if (startBlockCount !== endBlockCount) { + private findBlock(raw: string, markers: BlockMarkers): Block | undefined { + const startCount = countSubstring(markers.start, raw); + const endCount = countSubstring(markers.end, raw); + if (startCount !== endCount) { throw new SshConfigBadFormat( - `Malformed config: ${this.filePath} has an unterminated START CODER VSCODE ${safeHostname} block. Each START block must have an END block.`, + `Malformed config: ${this.filePath} has an unterminated "${markers.start}" block. Each START block must have an END block.`, + ); + } + if (startCount > 1) { + throw new SshConfigBadFormat( + `Malformed config: ${this.filePath} has ${startCount} "${markers.start}" blocks. Please remove all but one.`, ); } - if (startBlockCount > 1 || endBlockCount > 1) { + const start = raw.indexOf(markers.start); + const endMarkerStart = raw.indexOf(markers.end); + if (start === -1 || endMarkerStart === -1) return undefined; + if (endMarkerStart < start) { throw new SshConfigBadFormat( - `Malformed config: ${this.filePath} has ${startBlockCount} START CODER VSCODE ${safeHostname} sections. Please remove all but one.`, + `Malformed config: ${this.filePath} has an "${markers.end}" marker before its "${markers.start}" marker.`, ); } + const end = endMarkerStart + markers.end.length; + return { raw: raw.slice(start, end), start, end }; + } - const startBlockIndex = raw.indexOf(startBlock); - const endBlockIndex = raw.indexOf(endBlock); - const hasBlock = startBlockIndex > -1 && endBlockIndex > -1; - if (!hasBlock) { - return; + private replaceRange(raw: string, range: Block, replacement: string): string { + return raw.slice(0, range.start) + replacement + raw.slice(range.end); + } + + private renderIncludeBlock({ id, includePath }: SshInclude): string { + if (id.length === 0) { + throw new Error("Editor ID must not be empty."); } + const markers = blockMarkers(id); + return [ + markers.start, + "# Moves back to the top on connect; override options via coder.sshConfig.", + `Include "${this.escapeIncludePath(includePath)}"`, + markers.end, + ].join("\n"); + } - if (endBlockIndex < startBlockIndex) { - throw new SshConfigBadFormat( - "Malformed config, end block is before start block", + private escapeIncludePath(includePath: string): string { + // Prefer ~/... so quirks in the home path (spaces, %, glob characters) + // never reach the emitted argument. ssh expands the tilde itself. + const relative = path.relative(os.homedir(), includePath); + const argument = + relative && !relative.startsWith("..") && !path.isAbsolute(relative) + ? `~/${relative}` + : includePath; + // ssh_config has no escape for '"' inside a quoted argument, and + // OpenSSH 9.9+ fatals on unknown %-tokens in Include arguments. + if (/[\r\n\0"%]/.test(argument)) { + throw new Error( + "SSH include path must not contain CR, LF, NUL, %, or double-quote characters.", ); } - - return { - raw: raw.substring(startBlockIndex, endBlockIndex + endBlock.length), - }; + return argument.replaceAll("\\", "/").replace(/[*?[\]]/g, "\\$&"); } - /** - * buildBlock builds the ssh config block for the provided URL. The order of - * the keys is determinstic based on the input. Expected values are always in - * a consistent order followed by any additional overrides in sorted order. - * - * Validates everything written here, including the hostname, which lands in - * the block marker comments. - * - * @param safeHostname - The hostname for the deployment. - * @param values - The expected SSH values for using ssh with Coder. - * @param overrides - Overrides typically come from the deployment api and are - * used to override the default values. The overrides are - * given as key:value pairs where the key is the ssh config - * file key. If the key matches an expected value, the - * expected value is overridden. If it does not match an - * expected value, it is appended to the end of the block. - */ - private buildBlock( + private mergeInclude( + raw: string, + editorId: string, + includeBlock: string, safeHostname: string, - values: SshValues, - overrides?: Record, - ) { - validateSshValue("deployment hostname", safeHostname); - validateSshConfigOptions({ ...values }); - validateSshConfigOptions(overrides ?? {}); - const { Host, ...otherValues } = values; - const lines = [ - this.startBlockComment(safeHostname), - "# Rewritten by the Coder VS Code extension on every connection.", - '# To change these options, use the "coder.sshConfig" setting instead.', - `Host ${Host}`, - ]; - - // configValues is the merged values of the defaults and the overrides. - const configValues = mergeSshConfigValues(otherValues, overrides ?? {}); - - // keys is the sorted keys of the merged values. - const keys = Object.keys(configValues).sort(); - keys.forEach((key) => { - const value = configValues[key]; - if (value !== "") { - lines.push(this.withIndentation(`${key} ${value}`)); - } - }); - - lines.push(this.endBlockComment(safeHostname)); - return { - raw: lines.join("\n"), - }; + ): string { + let rest = raw; + const editorBlock = this.findBlock(rest, blockMarkers(editorId)); + if (editorBlock) { + rest = this.removeRange(rest, editorBlock); + } + const deployment = this.findBlock( + rest, + legacyDeploymentMarkers(safeHostname), + ); + if (deployment) { + this.logger.debug("Removing superseded SSH config block", safeHostname); + rest = this.removeRange(rest, deployment); + } + return [includeBlock, rest].filter(Boolean).join("\n\n"); } - private replaceBlock(oldBlock: Block, newBlock: Block) { - // A replacer function inserts $ sequences literally. - this.raw = this.getRaw().replace(oldBlock.raw, () => newBlock.raw); + private removeRange(raw: string, range: Block): string { + const before = raw.slice(0, range.start).trimEnd(); + const after = raw.slice(range.end).trimStart(); + return [before, after].filter(Boolean).join("\n\n"); } - private appendBlock(block: Block) { - const raw = this.getRaw(); + private async mutate(mutation: Mutation): Promise { + let snapshot = this.getRaw(); + for (let attempt = 0; attempt < UPDATE_ATTEMPTS; attempt++) { + const updated = mutation.apply(snapshot); + if (updated === snapshot) { + this.raw = snapshot; + return; + } - if (this.raw === "") { - this.raw = block.raw; - } else { - this.raw = `${raw.trimEnd()}\n\n${block.raw}`; + this.raw = updated; + if (!(await this.save(snapshot))) { + snapshot = await this.readForConflict(); + continue; + } + + const latest = await this.readForConflict(); + if (mutation.apply(latest) === latest) { + this.raw = latest; + mutation.onSuccess?.(); + return; + } + snapshot = latest; } - } - private withIndentation(text: string) { - return ` ${text}`; + this.raw = snapshot; + throw new Error( + `Failed to update SSH config at ${this.filePath} because it kept changing. Please try again.`, + ); } - private async save() { + private async save(expectedRaw?: string): Promise { // We want to preserve the original file mode. const existingMode = await this.fileSystem .stat(this.filePath) @@ -531,12 +583,26 @@ export class SshConfig { } try { + if (expectedRaw !== undefined) { + const latest = await this.readForConflict(); + if (latest !== expectedRaw) { + await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => { + this.logger.warn( + "Failed to clean up conflicted temp SSH config file", + tempPath, + unlinkErr, + ); + }); + return false; + } + } await renameWithRetry( (src, dest) => this.fileSystem.rename(src, dest), tempPath, this.filePath, ); this.logger.debug("Saved SSH config", this.filePath); + return true; } catch (err) { await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => { this.logger.warn( @@ -554,11 +620,14 @@ export class SshConfig { } } - public getRaw() { - if (this.raw === undefined) { - throw new Error("SshConfig is not loaded. Try sshConfig.load()"); + private async readForConflict(): Promise { + try { + return await this.fileSystem.readFile(this.filePath, "utf-8"); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return ""; + } + throw error; } - - return this.raw; } } diff --git a/src/remote/sshExtension.ts b/src/remote/sshExtension.ts index f2d80dec4..5aca747b8 100644 --- a/src/remote/sshExtension.ts +++ b/src/remote/sshExtension.ts @@ -11,9 +11,8 @@ export const REMOTE_SSH_EXTENSION_IDS = [ export type RemoteSshExtensionId = (typeof REMOTE_SSH_EXTENSION_IDS)[number]; /** - * Extensions that spawn ssh without `-F`, so it reads ~/.ssh/config whatever - * their renamed setting says. Honoring one would write the workspace host - * where the connection never looks. + * Extensions that spawn ssh without `-F`, so it always reads ~/.ssh/config + * and their renamed configFile setting never applies. */ const IGNORED_CONFIG_FILE: readonly RemoteSshExtensionId[] = [ "google.antigravity-remote-openssh", diff --git a/src/util/authority.ts b/src/util/authority.ts index b99200636..97327d2fa 100644 --- a/src/util/authority.ts +++ b/src/util/authority.ts @@ -1,5 +1,9 @@ +import * as vscode from "vscode"; + import { toSafeHost } from "./uri"; +export const LegacyAuthorityPrefix = "coder-vscode"; + export interface AuthorityParts { agent: string | undefined; sshHost: string; @@ -8,44 +12,89 @@ export interface AuthorityParts { workspace: string; } -// Prefix is a magic string that is prepended to SSH hosts to indicate that -// they should be handled by this extension. -export const AuthorityPrefix = "coder-vscode"; +export type AuthorityClassification = "current" | "legacy" | "foreign"; -const authorityHostPrefix = `${AuthorityPrefix}.`; +const sshRemotePrefix = "ssh-remote+"; const invalidAuthorityMessage = "Invalid Coder SSH authority. Must be: ----(.)"; +function currentAuthorityPrefix(): string { + const uriScheme = vscode.env.uriScheme; + if (!uriScheme) { + throw new Error("Editor URI scheme must not be empty."); + } + return `coder-${uriScheme}`; +} + +function getSshHostStart(authority: string): number | undefined { + if (authority.startsWith(sshRemotePrefix)) { + return sshRemotePrefix.length; + } + + for (const wrapper of [`@${sshRemotePrefix}`, `://${sshRemotePrefix}`]) { + const index = authority.lastIndexOf(wrapper); + if (index !== -1) { + return index + wrapper.length; + } + } + + return undefined; +} + +function classifySshHost(sshHost: string): AuthorityClassification | undefined { + const currentPrefix = currentAuthorityPrefix(); + if (sshHost.startsWith(`${currentPrefix}.`)) { + return "current"; + } + if ( + currentPrefix !== LegacyAuthorityPrefix && + sshHost.startsWith(`${LegacyAuthorityPrefix}.`) + ) { + return "legacy"; + } + // Deployment-unaware hosts like coder-vscode--ws stay foreign; their + // preserved config block still routes them. + return sshHost.startsWith("coder-") ? "foreign" : undefined; +} + +function authorityPrefix(classification: AuthorityClassification): string { + return classification === "legacy" + ? LegacyAuthorityPrefix + : currentAuthorityPrefix(); +} + /** * Given an authority, parse into the expected parts. * * The authority looks like `://ssh-remote+`, where the * SSH host names created by this extension match the format: - * coder-vscode.----(.) + * coder-.----(.) * * If this is not a Coder authority, return null. * * Throw an error if a Coder authority is invalid. */ export function parseRemoteAuthority(authority: string): AuthorityParts | null { - const authorityParts = authority.split("+"); - const sshHost = authorityParts[1]; - if (!sshHost) { + const sshHostStart = getSshHostStart(authority); + if (sshHostStart === undefined) { return null; } - const parts = sshHost.split("--"); - if (!parts[0].startsWith(authorityHostPrefix)) { + const sshHost = authority.slice(sshHostStart); + const classification = classifySshHost(sshHost); + if (!classification || classification === "foreign") { return null; } + // The classification guarantees the host starts with ".". + const prefix = `${authorityPrefix(classification)}.`; + const parts = sshHost.slice(prefix.length).split("--"); if (parts.length < 3) { throw new Error(invalidAuthorityMessage); } // Parse from the right because safe hostnames can contain "--". - const hostPrefix = parts.slice(0, -2).join("--"); - const safeHostname = hostPrefix.slice(authorityHostPrefix.length); + const safeHostname = parts.slice(0, -2).join("--"); const username = parts[parts.length - 2]; const workspaceAndAgent = parts[parts.length - 1]; if (!safeHostname || !username || !workspaceAndAgent) { @@ -73,15 +122,57 @@ export function parseRemoteAuthority(authority: string): AuthorityParts | null { }; } +export function classifyRemoteAuthority( + parts: AuthorityParts, +): AuthorityClassification { + return classifySshHost(parts.sshHost) ?? "foreign"; +} + export function toRemoteAuthority( baseUrl: string, workspaceOwner: string, workspaceName: string, workspaceAgent: string | undefined, ): string { - let remoteAuthority = `ssh-remote+${AuthorityPrefix}.${toSafeHost(baseUrl)}--${workspaceOwner}--${workspaceName}`; + let remoteAuthority = `ssh-remote+${currentAuthorityPrefix()}.${toSafeHost(baseUrl)}--${workspaceOwner}--${workspaceName}`; if (workspaceAgent) { remoteAuthority += `.${workspaceAgent}`; } return remoteAuthority; } + +export function toCurrentAuthorityHostPrefix(safeHostname?: string): string { + const prefix = currentAuthorityPrefix(); + return safeHostname ? `${prefix}.${safeHostname}--` : `${prefix}--`; +} + +export function retargetRemoteAuthority(authority: string): string { + const sshHostStart = getSshHostStart(authority); + if (sshHostStart === undefined) { + return authority; + } + + const sshHost = authority.slice(sshHostStart); + if (classifySshHost(sshHost) !== "legacy") { + return authority; + } + parseRemoteAuthority(authority); + return `${authority.slice(0, sshHostStart)}${currentAuthorityPrefix()}${sshHost.slice(LegacyAuthorityPrefix.length)}`; +} + +export function isRemoteAuthorityCompatible( + authority: string | undefined, + targetAuthority: string, +): boolean { + if (!authority) { + return false; + } + if (authority === targetAuthority) { + return true; + } + try { + return retargetRemoteAuthority(authority) === targetAuthority; + } catch { + return false; + } +} diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index f89a0a316..e886d0a12 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -25,12 +25,15 @@ import type { IncomingMessage } from "node:http"; import type { AgentMetadataEvent } from "@/api/api-helper"; import type { CoderApi } from "@/api/coderApi"; import type { CliCredentialManager } from "@/core/cliCredentialManager"; +import type { CliManager } from "@/core/cliManager"; import type { ServiceContainer } from "@/core/container"; import type { ContextManager } from "@/core/contextManager"; import type { MementoManager } from "@/core/mementoManager"; +import type { PathResolver } from "@/core/pathResolver"; import type { SecretsManager } from "@/core/secretsManager"; import type { Deployment } from "@/deployment/types"; import type { Logger } from "@/logging/logger"; +import type { LoginCoordinator } from "@/login/loginCoordinator"; import type { NetworkInfo } from "@/remote/sshProcess"; import type { TelemetryService } from "@/telemetry/service"; import type { @@ -50,6 +53,10 @@ interface ContextManagerLike { dispose(): void; } +interface LoginCoordinatorLike { + ensureLoggedInWithDialog: LoginCoordinator["ensureLoggedInWithDialog"]; +} + export function makeNetworkInfo( overrides: Partial = {}, ): NetworkInfo { @@ -249,6 +256,7 @@ export interface MessageCall { */ export class MockUserInteraction { private readonly responses = new Map(); + private readonly patternResponses: Array<[RegExp, string | undefined]> = []; private readonly _messageCalls: MessageCall[] = []; private inputBoxValue: string | undefined; private inputBoxValidateInput: ((value: string) => Promise) | undefined; @@ -259,10 +267,14 @@ export class MockUserInteraction { } /** - * Set a response for a specific message dialog + * Set a response for a message dialog, matched exactly or by pattern. */ - setResponse(message: string, response: string | undefined): void { - this.responses.set(message, response); + setResponse(message: string | RegExp, response: string | undefined): void { + if (typeof message === "string") { + this.responses.set(message, response); + } else { + this.patternResponses.push([message, response]); + } } /** @@ -307,6 +319,7 @@ export class MockUserInteraction { */ clear(): void { this.responses.clear(); + this.patternResponses.length = 0; this._messageCalls.length = 0; this.inputBoxValue = undefined; this.inputBoxValidateInput = undefined; @@ -318,7 +331,12 @@ export class MockUserInteraction { */ private setupVSCodeMock(): void { const getResponse = (message: string): string | undefined => { - return this.responses.get(message); + if (this.responses.has(message)) { + return this.responses.get(message); + } + return this.patternResponses.find(([pattern]) => + pattern.test(message), + )?.[1]; }; const handleMessage = @@ -581,7 +599,10 @@ export function createMockServiceContainer( secretsManager?: SecretsManager; mementoManager?: MementoManager; cliCredentialManager?: CliCredentialManager; + cliManager?: CliManager; + pathResolver?: PathResolver; contextManager?: ContextManagerLike; + loginCoordinator?: LoginCoordinatorLike; } = {}, ): ServiceContainer { const telemetry = overrides.telemetry ?? createTestTelemetryService(); @@ -601,8 +622,12 @@ export function createMockServiceContainer( require("mementoManager", overrides.mementoManager), getCliCredentialManager: () => require("cliCredentialManager", overrides.cliCredentialManager), + getCliManager: () => require("cliManager", overrides.cliManager), + getPathResolver: () => require("pathResolver", overrides.pathResolver), getContextManager: () => require("contextManager", overrides.contextManager) as ContextManager, + getLoginCoordinator: () => + require("loginCoordinator", overrides.loginCoordinator) as LoginCoordinator, } as ServiceContainer; } diff --git a/test/mocks/vscode.runtime.ts b/test/mocks/vscode.runtime.ts index 55cd482e7..252ae68af 100644 --- a/test/mocks/vscode.runtime.ts +++ b/test/mocks/vscode.runtime.ts @@ -86,6 +86,9 @@ export class Uri { constructor( public scheme: string, public path: string, + public authority = "", + public query = "", + public fragment = "", ) {} get fsPath(): string { return this.path; @@ -93,27 +96,61 @@ export class Uri { static file(p: string) { return new Uri("file", p); } - static parse(v: string) { - if (v.startsWith("file://")) { - return Uri.file(v.slice("file://".length)); + static from(components: { + scheme: string; + path?: string; + authority?: string; + query?: string; + fragment?: string; + }) { + return new Uri( + components.scheme, + components.path ?? "", + components.authority ?? "", + components.query ?? "", + components.fragment ?? "", + ); + } + with(change: { + scheme?: string; + path?: string; + authority?: string; + query?: string; + fragment?: string; + }) { + return new Uri( + change.scheme ?? this.scheme, + change.path ?? this.path, + change.authority ?? this.authority, + change.query ?? this.query, + change.fragment ?? this.fragment, + ); + } + static parse(value: string) { + if (value.startsWith("file://")) { + return Uri.file(value.slice("file://".length)); } - const [scheme, ...rest] = v.split(":"); + const [scheme, ...rest] = value.split(":"); return new Uri(scheme, rest.join(":")); } toString() { - return this.scheme === "file" - ? `file://${this.path}` - : `${this.scheme}:${this.path}`; + if (!this.authority && !this.query && !this.fragment) { + return this.scheme === "file" + ? `file://${this.path}` + : `${this.scheme}:${this.path}`; + } + const authority = this.authority ? `//${this.authority}` : ""; + const query = this.query ? `?${this.query}` : ""; + const fragment = this.fragment ? `#${this.fragment}` : ""; + return `${this.scheme}:${authority}${this.path}${query}${fragment}`; } static joinPath(base: Uri, ...paths: string[]) { - // Mirror vscode-uri: collapse slashes at the seams while preserving the - // leading "//" that separates the authority from the path. const head = base.path.replace(/\/+$/, ""); const tail = paths .map((p) => p.replace(/^\/+|\/+$/g, "")) .filter(Boolean) .join("/"); - return new Uri(base.scheme, tail ? `${head}/${tail}` : head); + return base.with({ path: tail ? `${head}/${tail}` : head }); } } @@ -181,6 +218,7 @@ export const commands = { export const workspace = { getConfiguration: vi.fn(), // your helpers override this workspaceFolders: [] as unknown[], + workspaceFile: undefined as Uri | undefined, fs: { readFile: vi.fn(), writeFile: vi.fn(), diff --git a/test/unit/core/pathResolver.test.ts b/test/unit/core/pathResolver.test.ts index b3191e6d3..c7deaaa3d 100644 --- a/test/unit/core/pathResolver.test.ts +++ b/test/unit/core/pathResolver.test.ts @@ -38,6 +38,15 @@ describe("PathResolver", () => { }); }); + describe("getSshConfigPath", () => { + it("uses the extension's global storage directory", () => { + expectPathsEqual( + pathResolver.getSshConfigPath(), + path.join(basePath, "ssh-config"), + ); + }); + }); + describe("getProxyLogPath", () => { const defaultLogPath = path.join(basePath, "log"); diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index 40bc7e0af..8b3b18a97 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -1,5 +1,6 @@ import { vol } from "memfs"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; import { MementoManager } from "@/core/mementoManager"; import { PathResolver } from "@/core/pathResolver"; @@ -9,31 +10,41 @@ import { Remote } from "@/remote/remote"; import { createTestTelemetryService } from "../../mocks/telemetry"; import { createMockLogger, + createMockServiceContainer, InMemoryMemento, InMemorySecretStorage, LogCollector, MockConfigurationProvider, + MockUserInteraction, } from "../../mocks/testHelpers"; -import type * as vscode from "vscode"; - import type { Commands } from "@/commands"; import type { CliManager } from "@/core/cliManager"; -import type { ServiceContainer } from "@/core/container"; -import type { ContextManager } from "@/core/contextManager"; import type { Logger } from "@/logging/logger"; -import type { LoginCoordinator } from "@/login/loginCoordinator"; + +const mockWorkspace = vscode.workspace as typeof vscode.workspace & { + workspaceFile: vscode.Uri | undefined; + workspaceFolders: vscode.WorkspaceFolder[]; +}; +const mockEnv = vscode.env as typeof vscode.env & { uriScheme: string }; vi.mock("node:fs/promises", async () => (await import("memfs")).fs.promises); const SAFE_HOSTNAME = "coder.example.com"; const REMOTE_AUTHORITY = "ssh-remote+coder-vscode.coder.example.com--testuser--test-workspace.main"; +const CURSOR_REMOTE_AUTHORITY = + "ssh-remote+coder-cursor.coder.example.com--testuser--test-workspace.main"; +const WINDSURF_REMOTE_AUTHORITY = + "ssh-remote+coder-windsurf.coder.example.com--testuser--test-workspace.main"; +const REMOTE_SSH_EXTENSION_ID = "anysphere.remote-ssh"; const MISMATCHED_URL = "https://cursor.example.com/private?token=sensitive-url-token"; const SESSION_TOKEN = "sensitive-session-token"; function createRemote(logger: Logger = createMockLogger()) { + new MockConfigurationProvider(); + const userInteraction = new MockUserInteraction(); const pathResolver = new PathResolver("/mock/global", "/mock/log"); vol.fromJSON({ [pathResolver.getUrlPath(SAFE_HOSTNAME)]: MISMATCHED_URL, @@ -47,16 +58,21 @@ function createRemote(logger: Logger = createMockLogger()) { const ensureLoggedInWithDialog = vi .fn() .mockResolvedValue({ success: false, reason: "user_dismissed" }); - const serviceContainer = { - getLogger: () => logger, - getPathResolver: () => pathResolver, - getCliManager: () => ({}) as CliManager, - getContextManager: () => ({}) as ContextManager, - getSecretsManager: () => secretsManager, - getLoginCoordinator: () => - ({ ensureLoggedInWithDialog }) as unknown as LoginCoordinator, - getTelemetryService: () => createTestTelemetryService(), - } as ServiceContainer; + const mementoManager = new MementoManager(new InMemoryMemento()); + const serviceContainer = createMockServiceContainer({ + logger, + pathResolver, + mementoManager, + cliManager: {} as CliManager, + contextManager: { + set: vi.fn(), + get: vi.fn(() => false), + dispose: vi.fn(), + }, + secretsManager, + loginCoordinator: { ensureLoggedInWithDialog }, + telemetry: createTestTelemetryService(), + }); return { remote: new Remote( @@ -64,15 +80,194 @@ function createRemote(logger: Logger = createMockLogger()) { {} as Commands, {} as vscode.ExtensionContext, ), + ensureLoggedInWithDialog, + mementoManager, secretsManager, + userInteraction, }; } describe("Remote", () => { beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); vol.reset(); - new MockConfigurationProvider(); + mockWorkspace.workspaceFile = undefined; + mockWorkspace.workspaceFolders = []; + mockEnv.uriScheme = "vscode"; + }); + + type UriOptions = Partial< + Pick + >; + const createUri = (path: string, options: UriOptions = {}) => + vscode.Uri.from({ + scheme: options.scheme ?? "cursor-remote", + authority: options.authority ?? REMOTE_AUTHORITY, + path, + query: options.query, + fragment: options.fragment, + }); + const setWorkspace = ( + folders: vscode.Uri[] = [], + workspaceFile?: vscode.Uri, + ) => { + mockWorkspace.workspaceFile = workspaceFile; + mockWorkspace.workspaceFolders = folders.map( + (uri) => ({ uri }) as vscode.WorkspaceFolder, + ); + return mockWorkspace.workspaceFolders; + }; + + it("migrates a legacy folder with its full URI", async () => { + mockEnv.uriScheme = "cursor"; + const { remote, mementoManager } = createRemote(); + setWorkspace([ + createUri("/workspace", { + query: "window=active", + fragment: "selection", + }), + ]); + + await expect( + remote.setup(REMOTE_AUTHORITY, "none", REMOTE_SSH_EXTENSION_ID), + ).resolves.toBeUndefined(); + + expect(vscode.commands.executeCommand).toHaveBeenCalledWith( + "vscode.openFolder", + createUri("/workspace", { + authority: CURSOR_REMOTE_AUTHORITY, + query: "window=active", + fragment: "selection", + }), + false, + ); + expect(await mementoManager.getAndClearStartupMode()).toBe("start"); + }); + + it("migrates a saved multi-root workspace file", async () => { + mockEnv.uriScheme = "cursor"; + const { remote, mementoManager } = createRemote(); + setWorkspace( + [createUri("/first-folder"), createUri("/second-folder")], + createUri("/project.code-workspace", { + query: "window=active", + fragment: "selection", + }), + ); + + await expect( + remote.setup(REMOTE_AUTHORITY, "none", REMOTE_SSH_EXTENSION_ID), + ).resolves.toBeUndefined(); + + expect(vscode.commands.executeCommand).toHaveBeenCalledWith( + "vscode.openFolder", + createUri("/project.code-workspace", { + authority: CURSOR_REMOTE_AUTHORITY, + query: "window=active", + fragment: "selection", + }), + false, + ); + expect(await mementoManager.getAndClearStartupMode()).toBe("start"); + }); + + interface EmptyWindowMigrationCase { + startupMode: "none" | "start" | "update"; + expectedStartupMode: "start" | "update"; + } + it.each([ + { startupMode: "none", expectedStartupMode: "start" }, + { startupMode: "start", expectedStartupMode: "start" }, + { startupMode: "update", expectedStartupMode: "update" }, + ])( + "migrates an empty window and preserves $startupMode startup mode", + async ({ startupMode, expectedStartupMode }) => { + mockEnv.uriScheme = "cursor"; + const { remote, mementoManager } = createRemote(); + + await expect( + remote.setup(REMOTE_AUTHORITY, startupMode, REMOTE_SSH_EXTENSION_ID), + ).resolves.toBeUndefined(); + + expect(vscode.commands.executeCommand).toHaveBeenCalledWith( + "vscode.newWindow", + { + remoteAuthority: CURSOR_REMOTE_AUTHORITY, + reuseWindow: true, + }, + ); + expect(await mementoManager.getAndClearStartupMode()).toBe( + expectedStartupMode, + ); + }, + ); + + it.each([ + { choice: undefined, docsUrls: [] }, + { + choice: "Learn More", + docsUrls: [expect.stringContaining("multi-root-workspaces")], + }, + ])( + "keeps an untitled multi-root workspace on the old host (choice: $choice)", + async ({ choice, docsUrls }) => { + mockEnv.uriScheme = "cursor"; + const { remote, mementoManager, userInteraction } = createRemote(); + setWorkspace( + [createUri("/first-folder"), createUri("/second-folder")], + createUri("/Untitled-1.code-workspace", { + scheme: "untitled", + authority: "", + }), + ); + userInteraction.setResponse(/coder-vscode SSH host/, choice); + + await expect( + remote.setup(REMOTE_AUTHORITY, "update", REMOTE_SSH_EXTENSION_ID), + ).resolves.toBeUndefined(); + + const warning = userInteraction + .getMessageCalls() + .find((call) => call.level === "warning"); + expect(warning?.message).toContain("coder-vscode SSH host"); + expect(warning?.items).toEqual(["Learn More"]); + expect(userInteraction.getExternalUrls()).toEqual(docsUrls); + expect(vscode.commands.executeCommand).not.toHaveBeenCalled(); + expect(await mementoManager.getAndClearStartupMode()).toBe("none"); + }, + ); + + it("continues setup for the current authority without reopening", async () => { + mockEnv.uriScheme = "cursor"; + const { remote, ensureLoggedInWithDialog } = createRemote(); + + await expect( + remote.setup(CURSOR_REMOTE_AUTHORITY, "none", REMOTE_SSH_EXTENSION_ID), + ).resolves.toBeUndefined(); + + expect(ensureLoggedInWithDialog).toHaveBeenCalledOnce(); + expect(vscode.commands.executeCommand).not.toHaveBeenCalledWith( + "vscode.openFolder", + expect.anything(), + expect.anything(), + ); + expect(vscode.commands.executeCommand).not.toHaveBeenCalledWith( + "vscode.newWindow", + expect.anything(), + ); + }); + + it("ignores a foreign authority", async () => { + mockEnv.uriScheme = "cursor"; + const { remote, ensureLoggedInWithDialog, mementoManager } = createRemote(); + + await expect( + remote.setup(WINDSURF_REMOTE_AUTHORITY, "none", REMOTE_SSH_EXTENSION_ID), + ).resolves.toBeUndefined(); + + expect(ensureLoggedInWithDialog).not.toHaveBeenCalled(); + expect(vscode.commands.executeCommand).not.toHaveBeenCalled(); + expect(await mementoManager.getAndClearStartupMode()).toBe("none"); }); it("ignores mismatched file auth and logs why", async () => { @@ -80,7 +275,7 @@ describe("Remote", () => { const { remote, secretsManager } = createRemote(logs); await expect( - remote.setup(REMOTE_AUTHORITY, "none", "anysphere.remote-ssh"), + remote.setup(REMOTE_AUTHORITY, "none", REMOTE_SSH_EXTENSION_ID), ).resolves.toBeUndefined(); expect(await secretsManager.getSessionAuth(SAFE_HOSTNAME)).toBeUndefined(); diff --git a/test/unit/remote/sshConfig.openssh.test.ts b/test/unit/remote/sshConfig.openssh.test.ts new file mode 100644 index 000000000..e8f005efe --- /dev/null +++ b/test/unit/remote/sshConfig.openssh.test.ts @@ -0,0 +1,134 @@ +import { execFile, spawnSync } from "node:child_process"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { promisify } from "node:util"; +import { afterEach, describe, expect, it } from "vitest"; + +import { SshConfig, type SshValues } from "@/remote/sshConfig"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +const run = promisify(execFile); +const sshAvailable = !spawnSync("ssh", ["-V"]).error; +// Generous for slow CI runners; each ssh -G call is local and fast. +const TEST_TIMEOUT_MS = 30_000; + +const sshValues = (hostname: string, proxyCommand: string): SshValues => ({ + Host: `coder-vscode.${hostname}--*`, + ProxyCommand: proxyCommand, + ConnectTimeout: "0", + StrictHostKeyChecking: "no", + UserKnownHostsFile: "/dev/null", + LogLevel: "ERROR", + ServerAliveInterval: "10", + ServerAliveCountMax: "3", +}); + +let tempDir: string | undefined; + +afterEach(async () => { + if (tempDir) { + await fs.rm(tempDir, { recursive: true, force: true }); + tempDir = undefined; + } +}); + +/** Real files and the real ssh binary; only the editor plumbing is absent. */ +async function createFixture(includeDirName: string) { + // Windows tmpdir paths use 8.3 short names ssh cannot glob; the product + // writes under the home directory anyway, emitted as a ~/... include. + const root = process.platform === "win32" ? os.homedir() : os.tmpdir(); + tempDir = await fs.mkdtemp(path.join(root, "coder-ssh-test-")); + const logger = createMockLogger(); + const includePath = path.join(tempDir, includeDirName, "ssh-config"); + const userConfigPath = path.join(tempDir, "config"); + + return { + includePath, + /** What the extension does on connect: write our file, then include it. */ + async connect(hostname: string, proxyCommand: string) { + const coderConfig = new SshConfig(includePath, logger); + await coderConfig.load(); + await coderConfig.update(hostname, sshValues(hostname, proxyCommand)); + const userConfig = new SshConfig(userConfigPath, logger); + await userConfig.load(); + await userConfig.updateInclude({ id: "vscode", includePath }, hostname); + }, + async seedUserConfig(contents: string) { + await fs.writeFile(userConfigPath, contents); + }, + async appendUserConfig(contents: string) { + await fs.appendFile(userConfigPath, contents); + }, + /** The host's effective options according to the real ssh. */ + async resolve(host: string) { + const { stdout } = await run("ssh", ["-G", "-F", userConfigPath, host], { + timeout: 10_000, + }); + return stdout; + }, + }; +} + +describe.skipIf(!sshAvailable)("include resolution by real OpenSSH", () => { + it( + "prefers the included file over a stale block and a later Host *", + async () => { + const ssh = await createFixture("Code Dir"); + await ssh.seedUserConfig( + [ + "# --- START CODER VSCODE dev.coder.com ---", + "Host coder-vscode.dev.coder.com--*", + " ProxyCommand echo stale-wins", + "# --- END CODER VSCODE dev.coder.com ---", + "", + ].join("\n"), + ); + await ssh.connect("dev.coder.com", "echo dev-wins"); + await ssh.connect("eu.coder.com", "echo eu-wins"); + await ssh.appendUserConfig( + "\nHost *\n ProxyCommand echo user-wins\n ConnectTimeout 9\n", + ); + + const resolved = await ssh.resolve( + "coder-vscode.dev.coder.com--user--ws", + ); + expect(resolved).toContain("proxycommand echo dev-wins"); + expect(resolved).toContain("connecttimeout 0"); + expect(resolved).not.toContain("stale-wins"); + expect( + await ssh.resolve("coder-vscode.eu.coder.com--user--ws"), + ).toContain("proxycommand echo eu-wins"); + }, + TEST_TIMEOUT_MS, + ); + + // Windows forbids these characters in file names. + it.skipIf(process.platform === "win32")( + "honors escaped glob characters in the include path", + async () => { + const ssh = await createFixture("we[i]rd *dir?"); + await ssh.connect("dev.coder.com", "echo dev-wins"); + expect( + await ssh.resolve("coder-vscode.dev.coder.com--user--ws"), + ).toContain("proxycommand echo dev-wins"); + }, + TEST_TIMEOUT_MS, + ); + + it( + "keeps ssh working when the included file is deleted", + async () => { + const ssh = await createFixture("Code Dir"); + await ssh.connect("dev.coder.com", "echo dev-wins"); + await ssh.appendUserConfig("\nHost *\n ProxyCommand echo user-wins\n"); + await fs.rm(ssh.includePath); + + expect( + await ssh.resolve("coder-vscode.dev.coder.com--user--ws"), + ).toContain("proxycommand echo user-wins"); + }, + TEST_TIMEOUT_MS, + ); +}); diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index 698075887..28b924a37 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -1,35 +1,31 @@ -import { it, afterEach, vi, expect, describe, beforeEach } from "vitest"; +import { vol } from "memfs"; +import * as fsPromises from "node:fs/promises"; +import * as os from "node:os"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { - SshConfig, + mergeSshConfigValues, parseCoderSshOptions, parseSshConfig, - mergeSshConfigValues, - validateDeploymentSshOptions, + SshConfig, + type SshInclude, type SshValues, + validateDeploymentSshOptions, } from "@/remote/sshConfig"; import { createMockLogger } from "../../mocks/testHelpers"; -// This is not the usual path to ~/.ssh/config, but -// setting it to a different path makes it easier to test -// and makes mistakes abundantly clear. -const sshFilePath = "/Path/To/UserHomeDir/.sshConfigDir/sshConfigFile"; -const sshTempFilePrefix = - "/Path/To/UserHomeDir/.sshConfigDir/.sshConfigFile.vscode-coder-tmp-"; -const managedHeader = `# Rewritten by the Coder VS Code extension on every connection. -# To change these options, use the "coder.sshConfig" setting instead.`; - -const mockFileSystem = { - mkdir: vi.fn(), - readFile: vi.fn(), - rename: vi.fn(), - stat: vi.fn(), - unlink: vi.fn().mockResolvedValue(undefined), - writeFile: vi.fn(), -}; +vi.mock("node:fs/promises", async () => (await import("memfs")).fs.promises); +vi.mock("node:os", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, homedir: vi.fn(() => "/Path/To/UserHomeDir") }; +}); -const mockLogger = createMockLogger(); +const homeDir = "/Path/To/UserHomeDir"; +const sshFilePath = "/Path/To/UserHomeDir/.sshConfigDir/sshConfigFile"; +const hostname = "dev.coder.com"; +const fileHeader = `# Coder workspace hosts. Do not edit; the Coder extension rewrites this file +# on every connection. Override options with the "coder.sshConfig" setting.`; const BASE_SSH_VALUES = { Host: "coder-vscode.dev.coder.com--*", @@ -54,20 +50,7 @@ const BENIGN_DEPLOYMENT_OPTIONS = { serveraliveinterval: "5", } as const; -afterEach(() => { - vi.clearAllMocks(); -}); - -it("creates a new file and adds the config", async () => { - mockFileSystem.readFile.mockRejectedValueOnce("No file found"); - mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" }); - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - await sshConfig.update("dev.coder.com", BASE_SSH_VALUES); - - const expectedOutput = `# --- START CODER VSCODE dev.coder.com --- -${managedHeader} +const deploymentBlock = `# --- START CODER dev.coder.com --- Host coder-vscode.dev.coder.com--* ConnectTimeout 0 LogLevel ERROR @@ -76,589 +59,651 @@ Host coder-vscode.dev.coder.com--* ServerAliveInterval 10 StrictHostKeyChecking no UserKnownHostsFile /dev/null +# --- END CODER dev.coder.com ---`; +const staleDeploymentBlock = `# --- START CODER dev.coder.com --- +Host stale +# --- END CODER dev.coder.com ---`; +const otherDeploymentBlock = `# --- START CODER other.coder.com --- +Host coder-vscode.other.coder.com--* +# --- END CODER other.coder.com ---`; +// Released versions wrote deployment blocks with the VSCODE label. +const legacyDeploymentBlock = `# --- START CODER VSCODE dev.coder.com --- +Host stale # --- END CODER VSCODE dev.coder.com ---`; +const legacyOtherDeploymentBlock = `# --- START CODER VSCODE other.coder.com --- +Host coder-vscode.other.coder.com--* +# --- END CODER VSCODE other.coder.com ---`; +const deploymentUnawareBlock = `# --- START CODER VSCODE --- +Host coder-vscode--* +# --- END CODER VSCODE ---`; - expect(mockFileSystem.readFile).toHaveBeenCalledWith( - sshFilePath, - expect.anything(), - ); - expect(mockFileSystem.writeFile).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - expectedOutput, - expect.objectContaining({ - encoding: "utf-8", - mode: 0o600, // Default mode for new files. - }), - ); - expect(mockFileSystem.rename).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - sshFilePath, - ); +const include = { + id: "vscode", + includePath: "~/.ssh/coder/config", +} satisfies SshInclude; + +function renderIncludeBlock(value: SshInclude): string { + return `# --- START CODER ${value.id} --- +# Moves back to the top on connect; override options via coder.sshConfig. +Include "${value.includePath}" +# --- END CODER ${value.id} ---`; +} + +const includeBlock = renderIncludeBlock(include); +const otherIncludeBlock = renderIncludeBlock({ + id: "windsurf", + includePath: "~/.ssh/windsurf/config", }); -it("adds a new coder config in an existent SSH configuration", async () => { - const existentSSHConfig = `Host coder.something - ConnectTimeout=0 - LogLevel ERROR - HostName coder.something - ProxyCommand command - StrictHostKeyChecking=no - UserKnownHostsFile=/dev/null`; - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); +const mockLogger = createMockLogger(); +// Captured before any spy so injected implementations can delegate to memfs. +const realReadFile = fsPromises.readFile; +const realRename = fsPromises.rename; + +const readConfig = () => realReadFile(sshFilePath, "utf-8"); + +async function loadSshConfig( + contents?: string, + mode = 0o644, +): Promise { + if (contents !== undefined) { + vol.fromJSON({ [sshFilePath]: contents }); + vol.chmodSync(sshFilePath, mode); + } + const sshConfig = new SshConfig(sshFilePath, mockLogger, fsPromises); await sshConfig.load(); - await sshConfig.update("dev.coder.com", BASE_SSH_VALUES); - - const expectedOutput = `${existentSSHConfig} - -# --- START CODER VSCODE dev.coder.com --- -${managedHeader} -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - ServerAliveCountMax 3 - ServerAliveInterval 10 - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com ---`; - - expect(mockFileSystem.writeFile).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - expectedOutput, - { - encoding: "utf-8", - mode: 0o644, + return sshConfig; +} + +async function updateDeployment( + contents?: string, + values: SshValues = BASE_SSH_VALUES, + overrides?: Record, +): Promise { + const sshConfig = await loadSshConfig(contents); + await sshConfig.update(hostname, values, overrides); +} + +async function updateInclude( + contents: string, + value: SshInclude = include, +): Promise { + const sshConfig = await loadSshConfig(contents); + await sshConfig.updateInclude(value, hostname); +} + +function injectConcurrentChangeBeforeRename(contents: string): void { + vi.spyOn(fsPromises, "readFile").mockImplementationOnce( + (filePath, options) => { + vol.writeFileSync(sshFilePath, contents); + return realReadFile(filePath, options); }, ); - expect(mockFileSystem.rename).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - sshFilePath, - ); -}); +} -it("updates an existent coder config", async () => { - const keepSSHConfig = `Host coder.something - HostName coder.something - ConnectTimeout=0 - StrictHostKeyChecking=no - UserKnownHostsFile=/dev/null - LogLevel ERROR - ProxyCommand command - -# --- START CODER VSCODE dev2.coder.com --- -Host coder-vscode.dev2.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev2.coder.com ---`; - - const existentSSHConfig = `${keepSSHConfig} +function injectConcurrentChangeAfterRename(contents: string): void { + vi.spyOn(fsPromises, "rename").mockImplementationOnce( + async (source, destination) => { + await realRename(source, destination); + vol.writeFileSync(sshFilePath, contents); + }, + ); +} -# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com --- +beforeEach(() => { + vol.reset(); + vi.mocked(os.homedir).mockReturnValue(homeDir); +}); -Host * - SetEnv TEST=1`; - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); +afterEach(() => { + vi.restoreAllMocks(); +}); - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - await sshConfig.update("dev.coder.com", { - ...BASE_SSH_VALUES, - Host: "coder-vscode.dev-updated.coder.com--*", - ProxyCommand: "some-updated-command-here", - ConnectTimeout: "1", - StrictHostKeyChecking: "yes", +describe("SshConfig.getRaw", () => { + it("throws before load", () => { + const sshConfig = new SshConfig(sshFilePath, mockLogger, fsPromises); + expect(() => sshConfig.getRaw()).toThrow("SshConfig is not loaded"); }); +}); - const expectedOutput = `${keepSSHConfig} +describe("SshConfig.update", () => { + it("renders the exact header and deployment config", async () => { + await updateDeployment(); -# --- START CODER VSCODE dev.coder.com --- -${managedHeader} -Host coder-vscode.dev-updated.coder.com--* - ConnectTimeout 1 - LogLevel ERROR - ProxyCommand some-updated-command-here - ServerAliveCountMax 3 - ServerAliveInterval 10 - StrictHostKeyChecking yes - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com --- - -Host * - SetEnv TEST=1`; + expect(await readConfig()).toBe(`${fileHeader}\n\n${deploymentBlock}`); + const configDir = vol.statSync("/Path/To/UserHomeDir/.sshConfigDir"); + expect(configDir.mode & 0o777).toBe(0o700); + }); - expect(mockFileSystem.writeFile).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - expectedOutput, + interface DeploymentMergeCase { + name: string; + existing: string; + expected: string; + } + it.each([ { - encoding: "utf-8", - mode: 0o644, + name: "appends after user config", + existing: "Host personal\n HostName example.com\n\n", + expected: `${fileHeader}\n\nHost personal\n HostName example.com\n\n${deploymentBlock}`, }, - ); - expect(mockFileSystem.rename).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - sshFilePath, - ); -}); - -it("does not remove deployment-unaware SSH config and adds the new one", async () => { - // Before the plugin supported multiple deployments, it would only write and - // overwrite this one block. We need to leave it alone so existing - // connections keep working. Only replace blocks specific to the deployment - // that we are targeting. Going forward, all new connections will use the new - // deployment-specific block. - const existentSSHConfig = `# --- START CODER VSCODE --- -Host coder-vscode--* - ConnectTimeout=0 - HostName coder.something - LogLevel ERROR - ProxyCommand command - StrictHostKeyChecking=no - UserKnownHostsFile=/dev/null -# --- END CODER VSCODE ---`; - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - await sshConfig.update("dev.coder.com", BASE_SSH_VALUES); - - const expectedOutput = `${existentSSHConfig} - -# --- START CODER VSCODE dev.coder.com --- -${managedHeader} -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - ServerAliveCountMax 3 - ServerAliveInterval 10 - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com ---`; - - expect(mockFileSystem.writeFile).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - expectedOutput, { - encoding: "utf-8", - mode: 0o644, + name: "replaces only the current deployment", + existing: `Host before\n\n${staleDeploymentBlock}\n\nHost after`, + expected: `${fileHeader}\n\nHost before\n\n${deploymentBlock}\n\nHost after`, }, - ); - expect(mockFileSystem.rename).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - sshFilePath, - ); -}); - -it("it does not remove a user-added block that only matches the host of an old coder SSH config", async () => { - const existentSSHConfig = `Host coder-vscode--* - ForwardAgent=yes`; - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); + { + name: "upgrades a legacy deployment marker in place", + existing: `Host before\n\n${legacyDeploymentBlock}\n\nHost after`, + expected: `${fileHeader}\n\nHost before\n\n${deploymentBlock}\n\nHost after`, + }, + { + name: "does not duplicate the header", + existing: `${fileHeader}\n\n${staleDeploymentBlock}`, + expected: `${fileHeader}\n\n${deploymentBlock}`, + }, + { + name: "preserves another deployment", + existing: otherDeploymentBlock, + expected: `${fileHeader}\n\n${otherDeploymentBlock}\n\n${deploymentBlock}`, + }, + { + name: "preserves deployment-unaware config", + existing: deploymentUnawareBlock, + expected: `${fileHeader}\n\n${deploymentUnawareBlock}\n\n${deploymentBlock}`, + }, + ])("$name", async ({ existing, expected }) => { + await updateDeployment(existing); + expect(await readConfig()).toBe(expected); + }); - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - await sshConfig.update("dev.coder.com", BASE_SSH_VALUES); + it("applies sorted case-insensitive overrides, additions, and removals", async () => { + await updateDeployment(undefined, BASE_SSH_VALUES, { + loglevel: "DEBUG", + ConnectTimeout: "500", + ExtraKey: "ExtraValue", + StrictHostKeyChecking: "", + ExtraRemove: "", + }); - const expectedOutput = `Host coder-vscode--* - ForwardAgent=yes + expect(await readConfig()).toBe(`${fileHeader} -# --- START CODER VSCODE dev.coder.com --- -${managedHeader} +# --- START CODER dev.coder.com --- Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR + ConnectTimeout 500 + ExtraKey ExtraValue ProxyCommand some-command-here ServerAliveCountMax 3 ServerAliveInterval 10 - StrictHostKeyChecking no UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com ---`; + loglevel DEBUG +# --- END CODER dev.coder.com ---`); + }); - expect(mockFileSystem.writeFile).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - expectedOutput, + interface MalformedDeploymentCase { + name: string; + existing: string; + error: string; + } + it.each([ { - encoding: "utf-8", - mode: 0o644, + name: "missing end marker", + existing: "# --- START CODER dev.coder.com ---", + error: 'unterminated "# --- START CODER dev.coder.com ---" block', }, - ); - expect(mockFileSystem.rename).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - sshFilePath, - ); -}); - -it("throws an error if there is a missing end block", async () => { - // The below config is missing an end block. - // This is a malformed config and should throw an error. - const existentSSHConfig = `Host beforeconfig - HostName before.config.tld - User before - -# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null + { + name: "extra start marker", + existing: `${staleDeploymentBlock}\n# --- START CODER dev.coder.com ---`, + error: 'unterminated "# --- START CODER dev.coder.com ---" block', + }, + { + name: "duplicate legacy blocks", + existing: `${legacyDeploymentBlock}\n${legacyDeploymentBlock}`, + error: 'has 2 "# --- START CODER VSCODE dev.coder.com ---" blocks', + }, + { + name: "end before start", + existing: + "# --- END CODER dev.coder.com ---\n# --- START CODER dev.coder.com ---", + error: + '"# --- END CODER dev.coder.com ---" marker before its "# --- START CODER dev.coder.com ---" marker', + }, + ])("rejects $name", async ({ existing, error }) => { + const sshConfig = await loadSshConfig(existing); + await expect(sshConfig.update(hostname, BASE_SSH_VALUES)).rejects.toThrow( + error, + ); + expect(await readConfig()).toBe(existing); + }); -Host afterconfig - HostName after.config.tld - User after`; + /** + * One case per input surface; the full character matrix is covered by the + * validateDeploymentSshOptions tests below. + */ + interface RejectCase { + name: string; + safeHostname?: string; + values?: SshValues; + overrides?: Record; + } - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - await sshConfig.load(); + it.each([ + { + name: "deployment hostname newline", + safeHostname: "dev.coder.com\nHost *", + }, + { + name: "Host value carriage return", + values: { ...BASE_SSH_VALUES, Host: "coder-vscode--*\rMatch all" }, + }, + { + name: "managed value newline", + values: { + ...BASE_SSH_VALUES, + ProxyCommand: "some-command-here\nRemoteCommand calc", + }, + }, + { + name: "override key whitespace", + overrides: { "ForwardAgent RemoteCommand": "yes" }, + }, + { + name: "override value newline", + overrides: { ForwardAgent: "yes\nRemoteCommand calc" }, + }, + ])( + "rejects unsafe serialization: $name", + async ({ + safeHostname = hostname, + values = BASE_SSH_VALUES, + overrides, + }) => { + const sshConfig = await loadSshConfig(); - // When we try to update the config, it should throw an error. - await expect( - sshConfig.update("dev.coder.com", BASE_SSH_VALUES), - ).rejects.toThrow( - `Malformed config: ${sshFilePath} has an unterminated START CODER VSCODE dev.coder.com block. Each START block must have an END block.`, + await expect( + sshConfig.update(safeHostname, values, overrides), + ).rejects.toThrow(); + expect(vol.existsSync(sshFilePath)).toBe(false); + }, ); -}); - -it("throws an error if there is a mismatched start and end block count", async () => { - // The below config contains two start blocks and one end block. - // This is a malformed config and should throw an error. - // Previously were were simply taking the first occurrences of the start and - // end blocks, which would potentially lead to loss of any content between the - // missing end block and the next start block. - const existentSSHConfig = `Host beforeconfig - HostName before.config.tld - User before -# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# missing END CODER VSCODE dev.coder.com --- - -Host donotdelete - HostName dont.delete.me - User please - -# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com --- - -Host afterconfig - HostName after.config.tld - User after`; - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - await sshConfig.load(); + it("accepts benign override options", async () => { + await updateDeployment(undefined, BASE_SSH_VALUES, USER_OVERRIDES); - // When we try to update the config, it should throw an error. - await expect( - sshConfig.update("dev.coder.com", BASE_SSH_VALUES), - ).rejects.toThrow( - `Malformed config: ${sshFilePath} has an unterminated START CODER VSCODE dev.coder.com block. Each START block must have an END block.`, - ); + const writtenConfig = await readConfig(); + expect(writtenConfig).toContain(" ForwardAgent yes"); + expect(writtenConfig).toContain(" IdentityFile ~/.ssh/coder identity"); + }); }); -it("throws an error if there are more than one sections with the same label", async () => { - const existentSSHConfig = `Host beforeconfig - HostName before.config.tld - User before - -# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com --- - -Host donotdelete - HostName dont.delete.me - User please +describe("SshConfig.updateInclude", () => { + interface IncludePositionCase { + name: string; + existing: string; + expected: string; + } + it.each([ + { name: "empty config", existing: "", expected: includeBlock }, + { + name: "prepends to user config", + existing: "Host *\n ConnectTimeout 5", + expected: `${includeBlock}\n\nHost *\n ConnectTimeout 5`, + }, + { + name: "already first", + existing: `${includeBlock}\n\nHost *`, + expected: `${includeBlock}\n\nHost *`, + }, + { + name: "moves to first", + existing: `Host *\n\n${includeBlock}`, + expected: `${includeBlock}\n\nHost *`, + }, + ])("handles $name", async ({ existing, expected }) => { + await updateInclude(existing); + expect(await readConfig()).toBe(expected); + }); -# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com --- + it("replaces the current editor block and preserves another editor", async () => { + const stale = includeBlock.replace("coder/config", "old/config"); + await updateInclude(`Host *\n\n${stale}\n\n${otherIncludeBlock}`); + expect(await readConfig()).toBe( + `${includeBlock}\n\nHost *\n\n${otherIncludeBlock}`, + ); + }); -Host afterconfig - HostName after.config.tld - User after`; + it("removes the current deployment and preserves other and deployment-unaware blocks", async () => { + await updateInclude( + `${legacyOtherDeploymentBlock}\n\n${legacyDeploymentBlock}\n\n${deploymentUnawareBlock}`, + ); + expect(await readConfig()).toBe( + `${includeBlock}\n\n${legacyOtherDeploymentBlock}\n\n${deploymentUnawareBlock}`, + ); + }); - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - await sshConfig.load(); + interface MalformedEditorCase { + name: string; + existing: string; + error: string; + } + it.each([ + { + name: "missing end marker", + existing: includeBlock.replace("# --- END CODER vscode ---", ""), + error: 'unterminated "# --- START CODER vscode ---" block', + }, + { + name: "mismatched end marker", + existing: includeBlock.replace( + "# --- END CODER vscode ---", + "# --- END CODER windsurf ---", + ), + error: 'unterminated "# --- START CODER vscode ---" block', + }, + { + name: "duplicate blocks", + existing: `${includeBlock}\n${includeBlock}`, + error: 'has 2 "# --- START CODER vscode ---" blocks', + }, + { + name: "end before start", + existing: "# --- END CODER vscode ---\n# --- START CODER vscode ---", + error: + '"# --- END CODER vscode ---" marker before its "# --- START CODER vscode ---" marker', + }, + ])("rejects $name", async ({ existing, error }) => { + await expect(updateInclude(existing)).rejects.toThrow(error); + expect(await readConfig()).toBe(existing); + }); - // When we try to update the config, it should throw an error. - await expect( - sshConfig.update("dev.coder.com", BASE_SSH_VALUES), - ).rejects.toThrow( - `Malformed config: ${sshFilePath} has 2 START CODER VSCODE dev.coder.com sections. Please remove all but one.`, - ); -}); + it("supports dashed editor IDs", async () => { + const dashed = { ...include, id: "vscode-insiders" }; + await updateInclude("", dashed); + expect(await readConfig()).toBe(renderIncludeBlock(dashed)); + }); -it("correctly handles interspersed blocks with and without label", async () => { - const existentSSHConfig = `Host beforeconfig - HostName before.config.tld - User before + it("rejects an empty editor ID", async () => { + await expect(updateInclude("", { ...include, id: "" })).rejects.toThrow( + "Editor ID must not be empty", + ); + }); -# --- START CODER VSCODE --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE --- + interface IncludePathEscapeCase { + includePath: string; + escaped: string; + } + it.each([ + { + includePath: "~/.ssh/we[i]rd/*?[config]", + escaped: "~/.ssh/we\\[i\\]rd/\\*\\?\\[config\\]", + }, + { + includePath: "C:\\Users\\Jane Doe\\config", + escaped: "C:/Users/Jane Doe/config", + }, + ])("escapes $includePath", async ({ includePath, escaped }) => { + await updateInclude("", { ...include, includePath }); + expect(await readConfig()).toContain(`Include "${escaped}"`); + }); -Host donotdelete - HostName dont.delete.me - User please + // A tilde swallows home-path quirks that ssh could not read back otherwise. + it.each([ + { label: "plain", home: homeDir }, + { label: "weird", home: "/home/we[i]rd %user" }, + ])( + "writes a $label home-relative include path with a tilde", + async ({ home }) => { + vi.mocked(os.homedir).mockReturnValue(home); + await updateInclude("", { + ...include, + includePath: `${home}/.config/Code/ssh-config`, + }); + expect(await readConfig()).toContain( + 'Include "~/.config/Code/ssh-config"', + ); + }, + ); -# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com --- + type InvalidIncludePath = string; + it.each([ + "path\rname", + "path\nname", + "path\0name", + 'path"name', + "path%name", + ])("rejects unrepresentable include paths", async (includePath) => { + await expect( + updateInclude("", { ...include, includePath }), + ).rejects.toThrow("must not contain CR, LF, NUL"); + }); +}); -Host afterconfig - HostName after.config.tld - User after`; +describe("persistence", () => { + interface FileModeCase { + name: string; + existing: string | undefined; + mode: number; + } + it.each([ + { name: "new file", existing: undefined, mode: 0o600 }, + { name: "existing file", existing: "Host *", mode: 0o640 }, + ])("uses the correct mode for a $name", async ({ existing, mode }) => { + const sshConfig = await loadSshConfig(existing, mode); + await sshConfig.update(hostname, BASE_SSH_VALUES); + expect(vol.statSync(sshFilePath).mode & 0o777).toBe(mode); + }); - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); - await sshConfig.load(); + type FileSystemErrorStage = "load" | "conflict read" | "stat"; + it.each(["load", "conflict read", "stat"])( + "propagates non-ENOENT %s errors", + async (stage) => { + const denied = Object.assign(new Error("denied"), { code: "EACCES" }); + if (stage === "load") { + vol.fromJSON({ [sshFilePath]: "Host initial" }); + vi.spyOn(fsPromises, "readFile").mockRejectedValueOnce(denied); + const sshConfig = new SshConfig(sshFilePath, mockLogger, fsPromises); + await expect(sshConfig.load()).rejects.toBe(denied); + return; + } + const sshConfig = await loadSshConfig("Host initial"); + if (stage === "conflict read") { + vi.spyOn(fsPromises, "readFile").mockRejectedValueOnce(denied); + } else { + vi.spyOn(fsPromises, "stat").mockRejectedValueOnce(denied); + } + await expect(sshConfig.update(hostname, BASE_SSH_VALUES)).rejects.toThrow( + "denied", + ); + }, + ); - const expectedOutput = `Host beforeconfig - HostName before.config.tld - User before + it("wraps write failures", async () => { + const sshConfig = await loadSshConfig("Host initial"); + vi.spyOn(fsPromises, "writeFile").mockRejectedValueOnce( + new Error("EACCES"), + ); + await expect(sshConfig.update(hostname, BASE_SSH_VALUES)).rejects.toThrow( + /Failed to write temporary SSH config file.*EACCES/, + ); + }); -# --- START CODER VSCODE --- -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE --- + it("wraps rename failures and removes the temporary file", async () => { + const sshConfig = await loadSshConfig("Host initial"); + const error = Object.assign(new Error("EXDEV"), { code: "EXDEV" }); + vi.spyOn(fsPromises, "rename").mockRejectedValueOnce(error); + await expect(sshConfig.update(hostname, BASE_SSH_VALUES)).rejects.toThrow( + "Failed to rename temporary SSH config file", + ); + const leftoverTempFiles = Object.keys(vol.toJSON()).filter((filePath) => + filePath.includes("vscode-coder-tmp"), + ); + expect(leftoverTempFiles).toEqual([]); + }); -Host donotdelete - HostName dont.delete.me - User please + it("retries a transient Windows rename failure", async () => { + const realPlatform = process.platform; + Object.defineProperty(process, "platform", { value: "win32" }); + vi.useFakeTimers(); + try { + const sshConfig = await loadSshConfig("Host initial"); + const error = Object.assign(new Error("EPERM"), { code: "EPERM" }); + const renameSpy = vi + .spyOn(fsPromises, "rename") + .mockRejectedValueOnce(error); + const update = sshConfig.update(hostname, BASE_SSH_VALUES); + await vi.advanceTimersByTimeAsync(100); + await update; + expect(renameSpy).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + Object.defineProperty(process, "platform", { value: realPlatform }); + } + }); -# --- START CODER VSCODE dev.coder.com --- -${managedHeader} -Host coder-vscode.dev.coder.com--* - ConnectTimeout 0 - LogLevel ERROR - ProxyCommand some-command-here - ServerAliveCountMax 3 - ServerAliveInterval 10 - StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER VSCODE dev.coder.com --- + it("retries an update conflict detected before rename", async () => { + const sshConfig = await loadSshConfig("Host initial"); + const writeFileSpy = vi.spyOn(fsPromises, "writeFile"); + const renameSpy = vi.spyOn(fsPromises, "rename"); + injectConcurrentChangeBeforeRename(otherDeploymentBlock); + await sshConfig.update(hostname, BASE_SSH_VALUES); + expect(await readConfig()).toBe( + `${fileHeader}\n\n${otherDeploymentBlock}\n\n${deploymentBlock}`, + ); + expect(writeFileSpy).toHaveBeenCalledTimes(2); + expect(renameSpy).toHaveBeenCalledTimes(1); + }); -Host afterconfig - HostName after.config.tld - User after`; + it("retries an include conflict detected after rename", async () => { + const sshConfig = await loadSshConfig("Host initial"); + const renameSpy = vi.spyOn(fsPromises, "rename"); + injectConcurrentChangeAfterRename( + `${otherIncludeBlock}\n\nHost concurrent\n\n${includeBlock}\n\n${legacyDeploymentBlock}`, + ); + await sshConfig.updateInclude(include, hostname); + expect(await readConfig()).toBe( + `${includeBlock}\n\n${otherIncludeBlock}\n\nHost concurrent`, + ); + expect(renameSpy).toHaveBeenCalledTimes(2); + }); - await sshConfig.update("dev.coder.com", BASE_SSH_VALUES); + it("fails after bounded optimistic retries", async () => { + vol.fromJSON({ [sshFilePath]: "Host initial" }); + const writeFileSpy = vi.spyOn(fsPromises, "writeFile"); + const renameSpy = vi.spyOn(fsPromises, "rename"); + let destinationReads = 0; + vi.spyOn(fsPromises, "readFile").mockImplementation((filePath, options) => { + if (filePath === sshFilePath && ++destinationReads > 1) { + vol.writeFileSync(sshFilePath, `Host revision-${destinationReads}`); + } + return realReadFile(filePath, options); + }); + const sshConfig = new SshConfig(sshFilePath, mockLogger, fsPromises); + await sshConfig.load(); + await expect(sshConfig.updateInclude(include, hostname)).rejects.toThrow( + "because it kept changing", + ); + expect(writeFileSpy).toHaveBeenCalledTimes(3); + expect(renameSpy).not.toHaveBeenCalled(); + }); +}); - expect(mockFileSystem.writeFile).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - expectedOutput, +describe("parseSshConfig", () => { + interface ParseSshConfigCase { + name: string; + input: string[]; + expected: Record; + } + it.each([ { - encoding: "utf-8", - mode: 0o644, + name: "parses space and equals separators", + input: ["ConnectTimeout 10", "LogLevel=DEBUG"], + expected: { ConnectTimeout: "10", LogLevel: "DEBUG" }, }, - ); - expect(mockFileSystem.rename).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - sshFilePath, - ); -}); - -it("override values", async () => { - mockFileSystem.readFile.mockRejectedValueOnce("No file found"); - mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" }); - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - await sshConfig.update("dev.coder.com", BASE_SSH_VALUES, { - loglevel: "DEBUG", // This tests case insensitive - ConnectTimeout: "500", - ExtraKey: "ExtraValue", - Foo: "bar", - Buzz: "baz", - // Remove this key - StrictHostKeyChecking: "", - ExtraRemove: "", + { + name: "accumulates non-empty SetEnv values", + input: ["SetEnv A=1", "setenv=B=2 C=3", "SetEnv="], + expected: { SetEnv: "A=1 B=2 C=3" }, + }, + { + name: "skips malformed lines", + input: ["malformed", "Key:value", "# comment", "key=value"], + expected: { key: "value" }, + }, + ])("$name", ({ input, expected }) => { + expect(parseSshConfig(input)).toEqual(expected); }); - - const expectedOutput = `# --- START CODER VSCODE dev.coder.com --- -${managedHeader} -Host coder-vscode.dev.coder.com--* - Buzz baz - ConnectTimeout 500 - ExtraKey ExtraValue - Foo bar - ProxyCommand some-command-here - ServerAliveCountMax 3 - ServerAliveInterval 10 - UserKnownHostsFile /dev/null - loglevel DEBUG -# --- END CODER VSCODE dev.coder.com ---`; - - expect(mockFileSystem.readFile).toHaveBeenCalledWith( - sshFilePath, - expect.anything(), - ); - expect(mockFileSystem.writeFile).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - expectedOutput, - expect.objectContaining({ - encoding: "utf-8", - mode: 0o600, // Default mode for new files. - }), - ); - expect(mockFileSystem.rename).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - sshFilePath, - ); }); -describe("SSH config serialization", () => { - /** - * One case per input surface; the full character matrix is covered by the - * validateDeploymentSshOptions tests below. - */ - interface RejectCase { +describe("mergeSshConfigValues", () => { + interface MergeSshConfigCase { name: string; - safeHostname?: string; - values?: SshValues; - overrides?: Record; + config: Record; + overrides: Record; + expected: Record; } - - it.each([ + it.each([ { - name: "deployment hostname newline", - safeHostname: "dev.coder.com\nHost *", + name: "overrides case-insensitively and preserves other values", + config: { LogLevel: "ERROR", Keep: "yes" }, + overrides: { loglevel: "DEBUG" }, + expected: { loglevel: "DEBUG", Keep: "yes" }, }, { - name: "Host value carriage return", - values: { ...BASE_SSH_VALUES, Host: "coder-vscode--*\rMatch all" }, + name: "adds and removes keys", + config: { Remove: "value" }, + overrides: { Remove: "", Add: "value" }, + expected: { Add: "value" }, }, { - name: "managed value newline", - values: { - ...BASE_SSH_VALUES, - ProxyCommand: "some-command-here\nRemoteCommand calc", - }, + name: "combines SetEnv and ignores an empty override", + config: { SetEnv: "A=1" }, + overrides: { setenv: "B=2" }, + expected: { SetEnv: "A=1 B=2" }, }, { - name: "override key whitespace", - overrides: { "ForwardAgent RemoteCommand": "yes" }, + name: "keeps SetEnv for an empty override", + config: { SetEnv: "A=1" }, + overrides: { SetEnv: "" }, + expected: { SetEnv: "A=1" }, }, { - name: "override value newline", - overrides: { ForwardAgent: "yes\nRemoteCommand calc" }, - }, - ])( - "rejects $name", - async ({ - safeHostname = "dev.coder.com", - values = BASE_SSH_VALUES, - overrides, - }) => { - mockFileSystem.readFile.mockRejectedValueOnce("No file found"); - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - - await expect( - sshConfig.update(safeHostname, values, overrides), - ).rejects.toThrow(); - expect(mockFileSystem.writeFile).not.toHaveBeenCalled(); + name: "adds SetEnv from overrides", + config: {}, + overrides: { SetEnv: "A=1" }, + expected: { SetEnv: "A=1" }, }, - ); - - it("accepts benign override options", async () => { - mockFileSystem.readFile.mockRejectedValueOnce("No file found"); - mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" }); - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - - await sshConfig.update("dev.coder.com", BASE_SSH_VALUES, USER_OVERRIDES); - - const writtenConfig = mockFileSystem.writeFile.mock.calls[0]?.[1]; - expect(writtenConfig).toContain(" ForwardAgent yes"); - expect(writtenConfig).toContain(" IdentityFile ~/.ssh/coder identity"); + ])("$name", ({ config, overrides, expected }) => { + expect(mergeSshConfigValues(config, overrides)).toEqual(expected); }); +}); - it("uses literal replacement text and preserves surrounding config", async () => { - const existentSshConfig = `Host before - IdentityFile ~/.ssh/before - -# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* -# --- END CODER VSCODE dev.coder.com --- - -Host after - IdentityFile ~/.ssh/after`; - mockFileSystem.readFile.mockResolvedValueOnce(existentSshConfig); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 }); - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - await sshConfig.update("dev.coder.com", BASE_SSH_VALUES, { - IdentityFile: "$& $` $' $ $$", - }); - - const writtenConfig = String(mockFileSystem.writeFile.mock.calls[0]?.[1]); - expect(writtenConfig).toContain(" IdentityFile $& $` $' $ $$"); - expect( - writtenConfig.startsWith(`Host before - IdentityFile ~/.ssh/before - -`), - ).toBe(true); - expect( - writtenConfig.endsWith(` +describe("parseCoderSshOptions", () => { + const coderBlock = (...lines: string[]) => + `# ------------START-CODER-----------\n${lines.join("\n")}\n# ------------END-CODER------------`; -Host after - IdentityFile ~/.ssh/after`), - ).toBe(true); + interface ParseCoderOptionsCase { + name: string; + raw: string; + expected: Record; + } + it.each([ + { name: "no block", raw: "Host personal", expected: {} }, + { + name: "options only", + raw: coderBlock( + "# :wait=yes", + "# :ssh-option=ForwardX11=yes", + "# :ssh-option=SetEnv=FOO=1", + "# :ssh-option=SetEnv=BAR=2", + ), + expected: { ForwardX11: "yes", SetEnv: "FOO=1 BAR=2" }, + }, + { + name: "flexible marker dashes", + raw: "# ---START-CODER---\n# :ssh-option=ForwardX11=yes\n# ---END-CODER---", + expected: { ForwardX11: "yes" }, + }, + ])("$name", ({ raw, expected }) => { + expect(parseCoderSshOptions(raw)).toEqual(expected); }); }); @@ -750,7 +795,7 @@ describe("validateDeploymentSshOptions", () => { ).toThrow( 'To allow "LocalCommand", set the option yourself in the "coder.sshConfig" setting', ); - // Overriding a pinned option fails the post-write check instead. + // Coder always writes the pinned options itself. expect(() => validateDeploymentSshOptions({ ProxyCommand: "evil" }, {}), ).toThrow('Coder manages "ProxyCommand", which cannot be overridden.'); @@ -784,384 +829,3 @@ describe("validateDeploymentSshOptions", () => { ).toThrow('"LocalCommand"'); }); }); - -it("fails if we are unable to write the temporary file", async () => { - const existentSSHConfig = `Host beforeconfig - HostName before.config.tld - User before`; - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 }); - mockFileSystem.writeFile.mockRejectedValueOnce(new Error("EACCES")); - - await sshConfig.load(); - - expect(mockFileSystem.readFile).toHaveBeenCalledWith( - sshFilePath, - expect.anything(), - ); - await expect( - sshConfig.update("dev.coder.com", BASE_SSH_VALUES), - ).rejects.toThrow(/Failed to write temporary SSH config file.*EACCES/); -}); - -it("cleans up temp file when rename fails", async () => { - mockFileSystem.readFile.mockResolvedValueOnce("Host existing\n HostName x"); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 }); - mockFileSystem.writeFile.mockResolvedValueOnce(""); - const err = new Error("EXDEV"); - (err as NodeJS.ErrnoException).code = "EXDEV"; - mockFileSystem.rename.mockRejectedValueOnce(err); - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - await expect( - sshConfig.update("dev.coder.com", { - ...BASE_SSH_VALUES, - ProxyCommand: "cmd", - }), - ).rejects.toThrow(/Failed to rename temporary SSH config file/); - expect(mockFileSystem.unlink).toHaveBeenCalledWith( - expect.stringContaining(sshTempFilePrefix), - ); -}); - -describe("rename retry on Windows", () => { - const realPlatform = process.platform; - - beforeEach(() => { - Object.defineProperty(process, "platform", { value: "win32" }); - vi.useFakeTimers(); - }); - afterEach(() => { - vi.useRealTimers(); - Object.defineProperty(process, "platform", { value: realPlatform }); - }); - - it("retries on transient EPERM and succeeds", async () => { - mockFileSystem.readFile.mockResolvedValueOnce( - "Host existing\n HostName x", - ); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 }); - mockFileSystem.writeFile.mockResolvedValueOnce(""); - const err = new Error("EPERM"); - (err as NodeJS.ErrnoException).code = "EPERM"; - mockFileSystem.rename - .mockRejectedValueOnce(err) - .mockResolvedValueOnce(undefined); - - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - const promise = sshConfig.update("dev.coder.com", { - ...BASE_SSH_VALUES, - ProxyCommand: "cmd", - }); - - await vi.advanceTimersByTimeAsync(100); - await promise; - - expect(mockFileSystem.rename).toHaveBeenCalledTimes(2); - expect(mockFileSystem.unlink).not.toHaveBeenCalled(); - }); -}); - -describe("parseSshConfig", () => { - interface ParseTest { - name: string; - input: string[]; - expected: Record; - } - - it.each([ - { - name: "space separator", - input: ["Key value"], - expected: { Key: "value" }, - }, - { - name: "equals separator", - input: ["Key=value"], - expected: { Key: "value" }, - }, - { - name: "SetEnv with space", - input: ["SetEnv MY_VAR=value OTHER_VAR=othervalue"], - expected: { SetEnv: "MY_VAR=value OTHER_VAR=othervalue" }, - }, - { - name: "SetEnv with equals", - input: ["SetEnv=MY_VAR=value OTHER_VAR=othervalue"], - expected: { SetEnv: "MY_VAR=value OTHER_VAR=othervalue" }, - }, - { - name: "accumulates SetEnv entries", - input: ["SetEnv A=1", "setenv B=2 C=3"], - expected: { SetEnv: "A=1 B=2 C=3" }, - }, - { - name: "skips malformed lines", - input: ["malformed", "# comment", "key=value", " indented"], - expected: { key: "value" }, - }, - { - name: "value with spaces", - input: ["ProxyCommand ssh -W %h:%p proxy"], - expected: { ProxyCommand: "ssh -W %h:%p proxy" }, - }, - { - name: "quoted value with spaces", - input: ['SetEnv key="Hello world"'], - expected: { SetEnv: 'key="Hello world"' }, - }, - { - name: "multiple keys", - input: ["ConnectTimeout 10", "LogLevel=DEBUG", "SetEnv VAR=1"], - expected: { ConnectTimeout: "10", LogLevel: "DEBUG", SetEnv: "VAR=1" }, - }, - { - name: "ignores empty SetEnv", - input: ["SetEnv=", "SetEnv "], - expected: {}, - }, - ])("$name", ({ input, expected }) => { - expect(parseSshConfig(input)).toEqual(expected); - }); -}); - -describe("mergeSshConfigValues", () => { - interface MergeTest { - name: string; - config: Record; - overrides: Record; - expected: Record; - } - - it.each([ - { - name: "overrides case-insensitively", - config: { LogLevel: "ERROR" }, - overrides: { loglevel: "DEBUG" }, - expected: { loglevel: "DEBUG" }, - }, - { - name: "removes keys with empty string", - config: { LogLevel: "ERROR", Foo: "bar" }, - overrides: { LogLevel: "" }, - expected: { Foo: "bar" }, - }, - { - name: "adds new keys from overrides", - config: { LogLevel: "ERROR" }, - overrides: { NewKey: "value" }, - expected: { LogLevel: "ERROR", NewKey: "value" }, - }, - { - name: "preserves keys not in overrides", - config: { A: "1", B: "2" }, - overrides: { B: "3" }, - expected: { A: "1", B: "3" }, - }, - { - name: "concatenates SetEnv values", - config: { SetEnv: "A=1" }, - overrides: { SetEnv: "B=2" }, - expected: { SetEnv: "A=1 B=2" }, - }, - { - name: "concatenates SetEnv case-insensitively", - config: { SetEnv: "A=1" }, - overrides: { setenv: "B=2" }, - expected: { SetEnv: "A=1 B=2" }, - }, - { - name: "SetEnv only in override", - config: {}, - overrides: { SetEnv: "B=2" }, - expected: { SetEnv: "B=2" }, - }, - { - name: "SetEnv only in config", - config: { SetEnv: "A=1" }, - overrides: {}, - expected: { SetEnv: "A=1" }, - }, - { - name: "SetEnv with other values", - config: { SetEnv: "A=1", LogLevel: "ERROR" }, - overrides: { SetEnv: "B=2", Timeout: "10" }, - expected: { SetEnv: "A=1 B=2", LogLevel: "ERROR", Timeout: "10" }, - }, - { - name: "ignores empty SetEnv override", - config: { SetEnv: "A=1 B=2" }, - overrides: { SetEnv: "" }, - expected: { SetEnv: "A=1 B=2" }, - }, - ])("$name", ({ config, overrides, expected }) => { - expect(mergeSshConfigValues(config, overrides)).toEqual(expected); - }); -}); - -describe("parseCoderSshOptions", () => { - const coderBlock = (...lines: string[]) => - `# ------------START-CODER-----------\n${lines.join("\n")}\n# ------------END-CODER------------`; - - interface SshOptionTestCase { - name: string; - raw: string; - expected: Record; - } - it.each([ - { - name: "empty string", - raw: "", - expected: {}, - }, - { - name: "no CLI block", - raw: "Host myhost\n HostName example.com", - expected: {}, - }, - { - name: "single option", - raw: coderBlock("# :ssh-option=ForwardX11=yes"), - expected: { ForwardX11: "yes" }, - }, - { - name: "multiple options", - raw: coderBlock( - "# :ssh-option=ForwardX11=yes", - "# :ssh-option=ForwardX11Trusted=yes", - ), - expected: { ForwardX11: "yes", ForwardX11Trusted: "yes" }, - }, - { - name: "ignores non-ssh-option keys", - raw: coderBlock( - "# :wait=yes", - "# :disable-autostart=true", - "# :ssh-option=ForwardX11=yes", - ), - expected: { ForwardX11: "yes" }, - }, - { - name: "accumulates SetEnv across lines", - raw: coderBlock( - "# :ssh-option=SetEnv=FOO=1", - "# :ssh-option=SetEnv=BAR=2", - ), - expected: { SetEnv: "FOO=1 BAR=2" }, - }, - { - name: "tolerates different dash counts in markers", - raw: `# ---START-CODER---\n# :ssh-option=ForwardX11=yes\n# ---END-CODER---`, - expected: { ForwardX11: "yes" }, - }, - ])("$name", ({ raw, expected }) => { - expect(parseCoderSshOptions(raw)).toEqual(expected); - }); - - it("extracts only ssh-options from a full config", () => { - const raw = `Host personal-server - HostName 10.0.0.1 - User admin - -# ------------START-CODER----------- -# This file is managed by coder. DO NOT EDIT. -# -# You should not hand-edit this file, changes may be overwritten. -# For more information, see https://coder.com/docs -# -# :wait=yes -# :disable-autostart=true -# :ssh-option=ForwardX11=yes -# :ssh-option=ForwardX11Trusted=yes - -Host coder.mydeployment--* - ConnectTimeout 0 - ForwardX11 yes - ForwardX11Trusted yes - StrictHostKeyChecking no - UserKnownHostsFile /dev/null - LogLevel ERROR - ProxyCommand /usr/bin/coder ssh --stdio --ssh-host-prefix coder.mydeployment-- %h -# ------------END-CODER------------ - -Host work-server - HostName 10.0.0.2 - User work`; - expect(parseCoderSshOptions(raw)).toEqual({ - ForwardX11: "yes", - ForwardX11Trusted: "yes", - }); - }); -}); - -describe("updateInclude", () => { - const include = `# --- START CODER VSCODE INCLUDE --- -# Your Coder workspaces, managed by the Coder VS Code extension. -# This block moves back to the top on every connect, since SSH uses the first -# value it finds. To override these options, use the coder.sshConfig setting. -Include ~/.ssh/coder/config -# --- END CODER VSCODE INCLUDE ---`; - - const managedBlock = `# --- START CODER VSCODE dev.coder.com --- -Host coder-vscode.dev.coder.com--* - ProxyCommand some-command-here -# --- END CODER VSCODE dev.coder.com ---`; - - /** Include our config in `existing`, returning what was written, if anything. */ - async function updateInclude(existing: string): Promise { - mockFileSystem.readFile.mockResolvedValueOnce(existing); - mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); - const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); - await sshConfig.load(); - await sshConfig.updateInclude("~/.ssh/coder/config", "dev.coder.com"); - return mockFileSystem.writeFile.mock.calls.at(-1)?.[1] as - string | undefined; - } - - it("goes above everything the user wrote", async () => { - const config = - "AddKeysToAgent yes\n\nInclude ~/.ssh/work\n\nHost *\n ConnectTimeout 5"; - - await expect(updateInclude(config)).resolves.toBe( - `${include}\n\n${config}`, - ); - }); - - it("creates the include in an empty config", async () => { - await expect(updateInclude("")).resolves.toBe(include); - }); - - it("leaves the file alone when the include is already first", async () => { - await expect( - updateInclude(`${include}\n\nHost *`), - ).resolves.toBeUndefined(); - }); - - it("moves an include that is no longer first", async () => { - const config = "Host *\n ConnectTimeout 5"; - - await expect(updateInclude(`${config}\n\n${include}`)).resolves.toBe( - `${include}\n\n${config}`, - ); - }); - - it("drops the block the included file supersedes", async () => { - const config = "Host *\n ConnectTimeout 5"; - - await expect(updateInclude(`${config}\n\n${managedBlock}`)).resolves.toBe( - `${include}\n\n${config}`, - ); - }); - - it("keeps blocks belonging to other deployments", async () => { - const other = managedBlock.replaceAll("dev.coder.com", "dev2.coder.com"); - - await expect(updateInclude(`${other}\n\n${managedBlock}`)).resolves.toBe( - `${include}\n\n${other}`, - ); - }); -}); diff --git a/test/unit/util/authority.test.ts b/test/unit/util/authority.test.ts index 8b0dd5f2d..bdbc0d45e 100644 --- a/test/unit/util/authority.test.ts +++ b/test/unit/util/authority.test.ts @@ -1,234 +1,241 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; +import * as vscode from "vscode"; import { + type AuthorityClassification, type AuthorityParts, + classifyRemoteAuthority, + isRemoteAuthorityCompatible, parseRemoteAuthority, + retargetRemoteAuthority, + toCurrentAuthorityHostPrefix, toRemoteAuthority, } from "@/util/authority"; -describe("parseRemoteAuthority", () => { - const remoteAuthority = (sshHost: string) => `vscode://ssh-remote+${sshHost}`; +const env = vscode.env as typeof vscode.env & { uriScheme: string }; +const CURSOR_AUTHORITY = "ssh-remote+coder-cursor.dev.coder.com--foo--bar.main"; +const LEGACY_AUTHORITY = "ssh-remote+coder-vscode.dev.coder.com--foo--bar.main"; +const WINDSURF_AUTHORITY = + "ssh-remote+coder-windsurf.dev.coder.com--foo--bar.main"; - it.each([ - { label: "missing SSH host", input: "vscode://ssh-remote" }, - { label: "empty SSH host", input: "vscode://ssh-remote+" }, - { - label: "non-Coder host", - input: remoteAuthority("some-unrelated-host.com"), - }, - { - label: "prefix without safeHostname separator", - input: remoteAuthority("coder-vscode--foo--bar"), - }, - { - label: "similar prefix", - input: remoteAuthority("coder-vscode-test--foo--bar"), - }, - { label: "wrong prefix", input: remoteAuthority("coder--foo--bar") }, - ])("ignores unrelated authority: $label", ({ input }) => { - expect(parseRemoteAuthority(input)).toBe(null); - }); +const parts = (prefix: string): AuthorityParts => ({ + agent: "main", + sshHost: `${prefix}.dev.coder.com--foo--bar.main`, + safeHostname: "dev.coder.com", + username: "foo", + workspace: "bar", +}); - it.each([ - { - label: "missing username and workspace", - sshHost: "coder-vscode.dev.coder.com", - }, - { - label: "missing workspace", - sshHost: "coder-vscode.dev.coder.com--foo", - }, - { - label: "manual host using Coder prefix", - sshHost: "coder-vscode.personal-host", - }, - { - label: "empty username", - sshHost: "coder-vscode.dev.coder.com----bar", - }, - { - label: "empty workspace", - sshHost: "coder-vscode.dev.coder.com--foo--", - }, - { - label: "empty hostname", - sshHost: "coder-vscode.--foo--bar", - }, +afterEach(() => { + env.uriScheme = "vscode"; +}); + +describe("parseRemoteAuthority", () => { + interface ClassificationCase { + editor: string; + prefix: string; + expected: AuthorityClassification; + } + it.each([ + { editor: "vscode", prefix: "coder-vscode", expected: "current" }, + { editor: "vscode", prefix: "coder-vscode-insiders", expected: "foreign" }, + { editor: "cursor", prefix: "coder-vscode", expected: "legacy" }, { - label: "empty trailing segment", - sshHost: "coder-vscode.dev.coder.com--foo--bar--", + editor: "vscode-insiders", + prefix: "coder-vscode-insiders", + expected: "current", }, { - label: "empty workspace before agent separator", - sshHost: "coder-vscode.dev.coder.com--foo--.agent", + editor: "vscode-insiders", + prefix: "coder-vscode", + expected: "legacy", }, - { - label: "empty agent after separator", - sshHost: "coder-vscode.dev.coder.com--foo--bar.", + ])( + "classifies $prefix as $expected in $editor", + ({ editor, prefix, expected }) => { + env.uriScheme = editor; + expect(classifyRemoteAuthority(parts(prefix))).toBe(expected); }, - ])("rejects invalid authority: $label", ({ sshHost }) => { - expect(() => parseRemoteAuthority(remoteAuthority(sshHost))).toThrow( + ); + + interface MalformedAuthorityCase { + editor: string; + sshHost: string; + } + it.each([ + { editor: "vscode", sshHost: "coder-vscode.dev.coder.com--foo" }, + { editor: "cursor", sshHost: "coder-vscode.dev.coder.com--foo" }, + { editor: "vscode", sshHost: "coder-vscode.--foo--bar" }, + { editor: "vscode", sshHost: "coder-vscode.dev.coder.com----bar" }, + { editor: "vscode", sshHost: "coder-vscode.dev.coder.com--foo--" }, + { editor: "vscode", sshHost: "coder-vscode.dev.coder.com--foo--.main" }, + { editor: "vscode", sshHost: "coder-vscode.dev.coder.com--foo--bar." }, + ])("rejects malformed current or legacy authority", ({ editor, sshHost }) => { + env.uriScheme = editor; + expect(() => parseRemoteAuthority(`ssh-remote+${sshHost}`)).toThrow( "Invalid Coder SSH authority", ); }); + it("ignores unrelated and malformed foreign authorities", () => { + expect(parseRemoteAuthority("github.com")).toBeNull(); + expect(parseRemoteAuthority("ssh-remote+coder-vscode")).toBeNull(); + env.uriScheme = "cursor"; + expect( + parseRemoteAuthority("ssh-remote+coder-windsurf.dev.coder.com--foo"), + ).toBeNull(); + }); + + it.each(["vscode", "cursor"])( + "ignores deployment-unaware historical hosts in %s", + (editor) => { + env.uriScheme = editor; + // Old versions created hosts matching the preserved `Host coder-vscode--*` block. + expect( + parseRemoteAuthority("ssh-remote+coder-vscode--user--workspace.main"), + ).toBeNull(); + }, + ); + interface ParseCase { - label: string; sshHost: string; safeHostname: string; workspace: string; - agent?: string; - username?: string; + agent: string; } - - it("round trips generated remote authorities", () => { - const authority = toRemoteAuthority( - "https://ほげ", - "alice", - "workspace", - "main", - ); - - expect(authority).toBe( - "ssh-remote+coder-vscode.xn--18j4d--alice--workspace.main", - ); - expect(parseRemoteAuthority(authority)).toStrictEqual({ - agent: "main", - sshHost: "coder-vscode.xn--18j4d--alice--workspace.main", - safeHostname: "xn--18j4d", - username: "alice", - workspace: "workspace", - } satisfies AuthorityParts); - }); - it.each([ { - label: "hostname without agent", sshHost: "coder-vscode.dev.coder.com--foo--bar", safeHostname: "dev.coder.com", workspace: "bar", + agent: "", }, { - label: "hostname with agent", - sshHost: "coder-vscode.dev.coder.com--foo--bar.baz", - safeHostname: "dev.coder.com", - workspace: "bar", - agent: "baz", - }, - { - label: "hostname containing delimiter", - sshHost: "coder-vscode.test--domain.com--foo--bar", - safeHostname: "test--domain.com", - workspace: "bar", - }, - { - label: "Punycode hostname containing delimiter", - sshHost: "coder-vscode.xn--test---8o4.example--foo--bar", - safeHostname: "xn--test---8o4.example", - workspace: "bar", - }, - { - label: "hostname with repeated delimiters and agent", - sshHost: "coder-vscode.first--middle--last.example--foo--bar.baz", + sshHost: "coder-vscode.first--middle--last.example--foo--bar.main", safeHostname: "first--middle--last.example", workspace: "bar", - agent: "baz", - }, - { - label: "hostname with many consecutive dashes", - sshHost: "coder-vscode.foo---------------bar.com--foo--bar", - safeHostname: "foo---------------bar.com", - workspace: "bar", - }, - { - label: "ambiguous workspace/agent separator", - sshHost: "coder-vscode.dev.coder.com--foo--bar.baz.qux", - safeHostname: "dev.coder.com", - workspace: "bar.baz.qux", + agent: "main", }, ])( - "parses $label", - ({ sshHost, safeHostname, workspace, agent, username }) => { - expect(parseRemoteAuthority(remoteAuthority(sshHost))).toStrictEqual({ - agent: agent ?? "", + "parses $sshHost from the right", + ({ sshHost, safeHostname, workspace, agent }) => { + expect(parseRemoteAuthority(`ssh-remote+${sshHost}`)).toStrictEqual({ + agent, sshHost, safeHostname, - username: username ?? "foo", + username: "foo", workspace, } satisfies AuthorityParts); }, ); -}); -describe("toRemoteAuthority", () => { - interface ToRemoteAuthorityCase { - url: string; - owner: string; - workspace: string; - agent: string | undefined; - expected: string; + interface WrappedAuthorityCase { + label: string; + authority: string; } - it.each([ + it.each([ + { label: "plain", authority: CURSOR_AUTHORITY }, + { label: "URI", authority: `vscode://${CURSOR_AUTHORITY}` }, { - url: "https://dev.coder.com", - owner: "foo", - workspace: "bar", - agent: undefined, - expected: "ssh-remote+coder-vscode.dev.coder.com--foo--bar", - }, - { - url: "http://dev.coder.com:3000", - owner: "foo", - workspace: "bar", - agent: "baz", - expected: "ssh-remote+coder-vscode.dev.coder.com--foo--bar.baz", + label: "multiply nested", + authority: `attached-container+def@dev-container+abc@${CURSOR_AUTHORITY}`, }, + ])("parses $label wrapper", ({ authority }) => { + env.uriScheme = "cursor"; + expect(parseRemoteAuthority(authority)).toStrictEqual( + parts("coder-cursor"), + ); + }); +}); + +describe("authority construction", () => { + it("preserves the editor URI scheme and integrates with toSafeHost", () => { + env.uriScheme = "cursor--dev"; + expect( + toRemoteAuthority("https://ほげ", "alice", "workspace", "main"), + ).toBe("ssh-remote+coder-cursor--dev.xn--18j4d--alice--workspace.main"); + }); + + it("omits an absent agent", () => { + expect( + toRemoteAuthority("https://dev.coder.com", "foo", "bar", undefined), + ).toBe("ssh-remote+coder-vscode.dev.coder.com--foo--bar"); + }); + + interface CurrentHostPrefixCase { + hostname: string | undefined; + expected: string; + } + it.each([ + { hostname: undefined, expected: "coder-vscode-insiders--" }, { - url: "https://coder.example.com/some/path?q=1", - owner: "alice", - workspace: "web", - agent: "", - expected: "ssh-remote+coder-vscode.coder.example.com--alice--web", + hostname: "dev.coder.com", + expected: "coder-vscode-insiders.dev.coder.com--", }, + ])("formats current host prefix for $hostname", ({ hostname, expected }) => { + env.uriScheme = "vscode-insiders"; + expect(toCurrentAuthorityHostPrefix(hostname)).toBe(expected); + }); + + it("rejects an empty editor URI scheme at prefix construction", () => { + env.uriScheme = ""; + expect(() => toCurrentAuthorityHostPrefix()).toThrow("must not be empty"); + }); +}); + +describe("authority migration", () => { + it("leaves unrelated authorities unchanged", () => { + expect(retargetRemoteAuthority("github.com")).toBe("github.com"); + }); + + interface RetargetCase { + label: string; + authority: string; + expected: string; + } + it.each([ { - url: "http://192.168.1.5:8080", - owner: "foo", - workspace: "bar", - agent: undefined, - expected: "ssh-remote+coder-vscode.192.168.1.5--foo--bar", + label: "plain", + authority: LEGACY_AUTHORITY, + expected: CURSOR_AUTHORITY, }, { - url: "http://localhost:3000", - owner: "dev", - workspace: "ws", - agent: "main", - expected: "ssh-remote+coder-vscode.localhost--dev--ws.main", + label: "multiply nested", + authority: `attached-container+def@dev-container+abc@${LEGACY_AUTHORITY}`, + expected: `attached-container+def@dev-container+abc@${CURSOR_AUTHORITY}`, }, - { - url: "https://sub.DOMAIN.Example.COM", - owner: "foo", - workspace: "bar", - agent: undefined, - expected: "ssh-remote+coder-vscode.sub.domain.example.com--foo--bar", + ])( + "preserves the $label wrapper while retargeting", + ({ authority, expected }) => { + env.uriScheme = "cursor"; + expect(retargetRemoteAuthority(authority)).toBe(expected); }, + ); + + interface CompatibilityCase { + label: string; + authority: string | undefined; + expected: boolean; + } + it.each([ + { label: "exact current", authority: CURSOR_AUTHORITY, expected: true }, + { label: "retargeted legacy", authority: LEGACY_AUTHORITY, expected: true }, + { label: "missing", authority: undefined, expected: false }, { - url: "https://ほげ:8080", - owner: "foo", - workspace: "bar", - agent: undefined, - expected: "ssh-remote+coder-vscode.xn--18j4d--foo--bar", + label: "malformed legacy", + authority: "ssh-remote+coder-vscode", + expected: false, }, + { label: "foreign", authority: WINDSURF_AUTHORITY, expected: false }, { - url: "https://عربي", - owner: "foo", - workspace: "bar", - agent: undefined, - expected: "ssh-remote+coder-vscode.xn--ngbrx4e--foo--bar", - }, - ])( - "builds authority for $url", - ({ url, owner, workspace, agent, expected }) => { - expect(toRemoteAuthority(url, owner, workspace, agent)).toBe(expected); + label: "different wrapper", + authority: `dev-container+abc@${LEGACY_AUTHORITY}`, + expected: false, }, - ); + ])("requires exact compatibility for $label", ({ authority, expected }) => { + env.uriScheme = "cursor"; + expect(isRemoteAuthorityCompatible(authority, CURSOR_AUTHORITY)).toBe( + expected, + ); + }); }); From 519ebb1fb30a26f33669724511d44b34c4889b38 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 6 Aug 2026 01:28:02 +0300 Subject: [PATCH 5/8] chore: address review feedback - Rename the ambiguous getSshConfigPath pair to getMainSshConfigPath and getIncludedSshConfigPath. - Move the generated-file header back to the top if the user prepended content, instead of duplicating it. - Drop the legacy marker lookup in mergeDeployment; the editor-owned file is new, so it can never contain legacy blocks. - Report unbalanced START/END marker counts accurately. - Note that mutate() only retries conflicts and blame concurrent editors in its failure message. - Rename readForConflict to read. - classifySshHost always returns a classification; foreign and undefined were handled identically everywhere. --- src/commands.ts | 2 +- src/core/pathResolver.ts | 6 ++++- src/remote/remote.ts | 6 ++--- src/remote/sshConfig.ts | 41 +++++++++++++++++++---------- src/util/authority.ts | 12 ++++----- test/unit/core/pathResolver.test.ts | 4 +-- test/unit/remote/sshConfig.test.ts | 34 +++++++++++++++--------- 7 files changed, 66 insertions(+), 39 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 05fe1e6b0..bcf9e439e 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -572,7 +572,7 @@ export class Commands { * Open this editor's generated SSH config with the Coder workspace hosts. */ public async openSshConfig(): Promise { - const configPath = this.pathResolver.getSshConfigPath(); + const configPath = this.pathResolver.getIncludedSshConfigPath(); try { await openFile(configPath); // The file is rewritten on every connection, so edits would be lost. diff --git a/src/core/pathResolver.ts b/src/core/pathResolver.ts index 4327f2f8d..bd6dd91dc 100644 --- a/src/core/pathResolver.ts +++ b/src/core/pathResolver.ts @@ -42,7 +42,11 @@ export class PathResolver { return path.join(this.basePath, "net"); } - public getSshConfigPath(): string { + /** + * The editor-owned generated SSH config, referenced by an Include in the + * user's main config. + */ + public getIncludedSshConfigPath(): string { return path.join(this.basePath, "ssh-config"); } diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 3d4e1cfb6..9dd7f8c1b 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -952,7 +952,7 @@ export class Remote { return ["--log-dir", escapeCommandArg(logDir), "-v"]; } - private getSshConfigPath(): string { + private getMainSshConfigPath(): string { const configured = getRemoteSshConfigFile(); return expandPath(configured || path.join("~", ".ssh", "config")); } @@ -969,9 +969,9 @@ export class Remote { cliAuth: CliAuth, ): Promise { // Our blocks live in our own file; the user's only gains the include. - const sshConfig = new SshConfig(this.getSshConfigPath(), this.logger); + const sshConfig = new SshConfig(this.getMainSshConfigPath(), this.logger); await sshConfig.load(); - const coderConfigPath = this.pathResolver.getSshConfigPath(); + const coderConfigPath = this.pathResolver.getIncludedSshConfigPath(); const coderConfig = new SshConfig(coderConfigPath, this.logger); await coderConfig.load(); diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index e96ed5d7c..0357b020b 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -86,8 +86,7 @@ function blockMarkers(label: string): BlockMarkers { }; } -// Released versions wrote deployment blocks with this label, both into the -// user's config and via early builds of the editor-owned file. +// Released versions wrote deployment blocks with this label into the user's config. function legacyDeploymentMarkers(safeHostname: string): BlockMarkers { return blockMarkers(`VSCODE ${safeHostname}`); } @@ -414,9 +413,7 @@ export class SshConfig { block: string, ): string { let merged: string; - const existing = - this.findBlock(raw, blockMarkers(safeHostname)) ?? - this.findBlock(raw, legacyDeploymentMarkers(safeHostname)); + const existing = this.findBlock(raw, blockMarkers(safeHostname)); if (existing) { this.logger.debug("Replacing SSH config block", safeHostname); merged = this.replaceRange(raw, existing, block); @@ -424,9 +421,23 @@ export class SshConfig { this.logger.debug("Appending new SSH config block", safeHostname); merged = raw ? `${raw.trimEnd()}\n\n${block}` : block; } - return merged.startsWith(CODER_SSH_CONFIG_HEADER) - ? merged - : `${CODER_SSH_CONFIG_HEADER}\n\n${merged}`; + return this.moveHeaderToTop(merged); + } + + /** The user may have added content above the header; move it back to the top. */ + private moveHeaderToTop(merged: string): string { + const start = merged.indexOf(CODER_SSH_CONFIG_HEADER); + if (start === 0) { + return merged; + } + if (start > 0) { + merged = this.removeRange(merged, { + raw: CODER_SSH_CONFIG_HEADER, + start, + end: start + CODER_SSH_CONFIG_HEADER.length, + }); + } + return `${CODER_SSH_CONFIG_HEADER}\n\n${merged}`; } private findBlock(raw: string, markers: BlockMarkers): Block | undefined { @@ -434,7 +445,7 @@ export class SshConfig { const endCount = countSubstring(markers.end, raw); if (startCount !== endCount) { throw new SshConfigBadFormat( - `Malformed config: ${this.filePath} has an unterminated "${markers.start}" block. Each START block must have an END block.`, + `Malformed config: ${this.filePath} has ${startCount} "${markers.start}" and ${endCount} "${markers.end}" markers. Each START marker must have exactly one END marker.`, ); } if (startCount > 1) { @@ -520,6 +531,8 @@ export class SshConfig { private async mutate(mutation: Mutation): Promise { let snapshot = this.getRaw(); + // Retries only handle concurrent writers (save() returns false on a + // conflict); I/O errors like EACCES throw immediately and are not retried. for (let attempt = 0; attempt < UPDATE_ATTEMPTS; attempt++) { const updated = mutation.apply(snapshot); if (updated === snapshot) { @@ -529,11 +542,11 @@ export class SshConfig { this.raw = updated; if (!(await this.save(snapshot))) { - snapshot = await this.readForConflict(); + snapshot = await this.read(); continue; } - const latest = await this.readForConflict(); + const latest = await this.read(); if (mutation.apply(latest) === latest) { this.raw = latest; mutation.onSuccess?.(); @@ -544,7 +557,7 @@ export class SshConfig { this.raw = snapshot; throw new Error( - `Failed to update SSH config at ${this.filePath} because it kept changing. Please try again.`, + `Failed to update SSH config at ${this.filePath} because it kept changing, likely due to another editor writing it at the same time. Please try again.`, ); } @@ -584,7 +597,7 @@ export class SshConfig { try { if (expectedRaw !== undefined) { - const latest = await this.readForConflict(); + const latest = await this.read(); if (latest !== expectedRaw) { await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => { this.logger.warn( @@ -620,7 +633,7 @@ export class SshConfig { } } - private async readForConflict(): Promise { + private async read(): Promise { try { return await this.fileSystem.readFile(this.filePath, "utf-8"); } catch (error) { diff --git a/src/util/authority.ts b/src/util/authority.ts index 97327d2fa..1b9446fd0 100644 --- a/src/util/authority.ts +++ b/src/util/authority.ts @@ -41,7 +41,7 @@ function getSshHostStart(authority: string): number | undefined { return undefined; } -function classifySshHost(sshHost: string): AuthorityClassification | undefined { +function classifySshHost(sshHost: string): AuthorityClassification { const currentPrefix = currentAuthorityPrefix(); if (sshHost.startsWith(`${currentPrefix}.`)) { return "current"; @@ -52,9 +52,9 @@ function classifySshHost(sshHost: string): AuthorityClassification | undefined { ) { return "legacy"; } - // Deployment-unaware hosts like coder-vscode--ws stay foreign; their - // preserved config block still routes them. - return sshHost.startsWith("coder-") ? "foreign" : undefined; + // Anything else is foreign, including deployment-unaware hosts like + // coder-vscode--ws; their preserved config block still routes them. + return "foreign"; } function authorityPrefix(classification: AuthorityClassification): string { @@ -82,7 +82,7 @@ export function parseRemoteAuthority(authority: string): AuthorityParts | null { const sshHost = authority.slice(sshHostStart); const classification = classifySshHost(sshHost); - if (!classification || classification === "foreign") { + if (classification === "foreign") { return null; } @@ -125,7 +125,7 @@ export function parseRemoteAuthority(authority: string): AuthorityParts | null { export function classifyRemoteAuthority( parts: AuthorityParts, ): AuthorityClassification { - return classifySshHost(parts.sshHost) ?? "foreign"; + return classifySshHost(parts.sshHost); } export function toRemoteAuthority( diff --git a/test/unit/core/pathResolver.test.ts b/test/unit/core/pathResolver.test.ts index c7deaaa3d..bf1c3b3df 100644 --- a/test/unit/core/pathResolver.test.ts +++ b/test/unit/core/pathResolver.test.ts @@ -38,10 +38,10 @@ describe("PathResolver", () => { }); }); - describe("getSshConfigPath", () => { + describe("getIncludedSshConfigPath", () => { it("uses the extension's global storage directory", () => { expectPathsEqual( - pathResolver.getSshConfigPath(), + pathResolver.getIncludedSshConfigPath(), path.join(basePath, "ssh-config"), ); }); diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index 28b924a37..469b98964 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -191,16 +191,16 @@ describe("SshConfig.update", () => { existing: `Host before\n\n${staleDeploymentBlock}\n\nHost after`, expected: `${fileHeader}\n\nHost before\n\n${deploymentBlock}\n\nHost after`, }, - { - name: "upgrades a legacy deployment marker in place", - existing: `Host before\n\n${legacyDeploymentBlock}\n\nHost after`, - expected: `${fileHeader}\n\nHost before\n\n${deploymentBlock}\n\nHost after`, - }, { name: "does not duplicate the header", existing: `${fileHeader}\n\n${staleDeploymentBlock}`, expected: `${fileHeader}\n\n${deploymentBlock}`, }, + { + name: "moves the header back to the top", + existing: `Host personal\n\n${fileHeader}\n\n${staleDeploymentBlock}`, + expected: `${fileHeader}\n\nHost personal\n\n${deploymentBlock}`, + }, { name: "preserves another deployment", existing: otherDeploymentBlock, @@ -248,17 +248,25 @@ Host coder-vscode.dev.coder.com--* { name: "missing end marker", existing: "# --- START CODER dev.coder.com ---", - error: 'unterminated "# --- START CODER dev.coder.com ---" block', + error: + 'has 1 "# --- START CODER dev.coder.com ---" and 0 "# --- END CODER dev.coder.com ---" markers', }, { name: "extra start marker", existing: `${staleDeploymentBlock}\n# --- START CODER dev.coder.com ---`, - error: 'unterminated "# --- START CODER dev.coder.com ---" block', + error: + 'has 2 "# --- START CODER dev.coder.com ---" and 1 "# --- END CODER dev.coder.com ---" markers', }, { - name: "duplicate legacy blocks", - existing: `${legacyDeploymentBlock}\n${legacyDeploymentBlock}`, - error: 'has 2 "# --- START CODER VSCODE dev.coder.com ---" blocks', + name: "extra end marker", + existing: `${staleDeploymentBlock}\n# --- END CODER dev.coder.com ---`, + error: + 'has 1 "# --- START CODER dev.coder.com ---" and 2 "# --- END CODER dev.coder.com ---" markers', + }, + { + name: "duplicate blocks", + existing: `${staleDeploymentBlock}\n${staleDeploymentBlock}`, + error: 'has 2 "# --- START CODER dev.coder.com ---" blocks', }, { name: "end before start", @@ -389,7 +397,8 @@ describe("SshConfig.updateInclude", () => { { name: "missing end marker", existing: includeBlock.replace("# --- END CODER vscode ---", ""), - error: 'unterminated "# --- START CODER vscode ---" block', + error: + 'has 1 "# --- START CODER vscode ---" and 0 "# --- END CODER vscode ---" markers', }, { name: "mismatched end marker", @@ -397,7 +406,8 @@ describe("SshConfig.updateInclude", () => { "# --- END CODER vscode ---", "# --- END CODER windsurf ---", ), - error: 'unterminated "# --- START CODER vscode ---" block', + error: + 'has 1 "# --- START CODER vscode ---" and 0 "# --- END CODER vscode ---" markers', }, { name: "duplicate blocks", From 53c6c8b5c4009568413a0958028fba80e40cdf89 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 6 Aug 2026 01:39:17 +0300 Subject: [PATCH 6/8] fix: share one SSH config dir across editors with a glob include Replace the per-editor include block with a single editor-agnostic block: Include "~/.local/share/coder.coder-remote/ssh/*.conf" Each (editor, deployment) pair owns one file in that shared directory (vscode--dev.coder.com.conf), fully regenerated on connect, so concurrent writers are single-writer per file and last-writer-wins is correct. The include line is identical no matter which editor writes it, so the user's config stops churning once it is in place and the cross-editor race on it disappears in steady state; the optimistic-retry machinery now only guards the include placement and legacy cleanup. The directory lives in the platform data dir (XDG_DATA_HOME, Application Support, APPDATA) instead of per-editor global storage so every editor emits the same include. OpenSSH resolves glob includes through glob(3) on every platform, including Win32-OpenSSH since v7.7, and a missing directory is a non-fatal no-match, verified by the real-ssh tests. Connects always rewrite the deployment file so its mtime marks the last connect, and any editor sweeps files older than a week on connect; the next connect to that deployment recreates its file. --- src/commands.ts | 54 ++- src/core/pathResolver.ts | 47 ++- src/remote/remote.ts | 38 +- src/remote/sshConfig.ts | 300 +++++----------- src/util/authority.ts | 5 +- test/unit/core/pathResolver.test.ts | 103 +++++- test/unit/remote/sshConfig.openssh.test.ts | 38 +- test/unit/remote/sshConfig.test.ts | 381 ++++++--------------- test/unit/util/authority.test.ts | 20 +- 9 files changed, 428 insertions(+), 558 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index bcf9e439e..4cce6b499 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -52,6 +52,7 @@ import { import { runExportTelemetryCommand } from "./telemetry/export/command"; import { isRemoteAuthorityCompatible, + parseRemoteAuthority, toRemoteAuthority, } from "./util/authority"; import { openInBrowser, toSafeHost } from "./util/uri"; @@ -568,22 +569,57 @@ export class Commands { ); } - /** - * Open this editor's generated SSH config with the Coder workspace hosts. - */ + /** Open this editor's generated SSH config, picking a deployment when several exist. */ public async openSshConfig(): Promise { - const configPath = this.pathResolver.getIncludedSshConfigPath(); + const hostname = await this.pickSshHostname(); + if (!hostname) { + return; + } try { - await openFile(configPath); - // The file is rewritten on every connection, so edits would be lost. - await vscode.commands.executeCommand( - "workbench.action.files.setActiveEditorReadonlyInSession", - ); + await openFile(this.pathResolver.getSshConfigPath(hostname)); } catch { vscode.window.showInformationMessage( "No SSH config has been generated yet. It is written when you connect to a workspace.", ); + return; } + // The file is rewritten on every connection, so edits would be lost. + await vscode.commands.executeCommand( + "workbench.action.files.setActiveEditorReadonlyInSession", + ); + } + + /** A connected window resolves to its own deployment; otherwise ask. */ + private async pickSshHostname(): Promise { + const remoteAuthority = vscode.env.remoteAuthority; + if (remoteAuthority) { + try { + const parts = parseRemoteAuthority(remoteAuthority); + if (parts) { + return parts.safeHostname; + } + } catch { + // Malformed Coder authority; fall through to the picker. + } + } + const hostnames = ( + await readdirOrEmpty(this.pathResolver.getSshConfigDir()) + ) + .map((file) => this.pathResolver.parseSshConfigFile(file)) + .filter((name) => name !== undefined); + if (hostnames.length === 0) { + vscode.window.showInformationMessage( + "No SSH config has been generated yet. It is written when you connect to a workspace.", + ); + return undefined; + } + if (hostnames.length === 1) { + return hostnames[0]; + } + return vscode.window.showQuickPick(hostnames, { + title: "Open generated SSH configuration", + placeHolder: "Select a deployment", + }); } /** diff --git a/src/core/pathResolver.ts b/src/core/pathResolver.ts index bd6dd91dc..4057d66cf 100644 --- a/src/core/pathResolver.ts +++ b/src/core/pathResolver.ts @@ -1,3 +1,4 @@ +import * as os from "node:os"; import * as path from "node:path"; import * as vscode from "vscode"; @@ -43,11 +44,49 @@ export class PathResolver { } /** - * The editor-owned generated SSH config, referenced by an Include in the - * user's main config. + * Directory of generated SSH configs, glob-included from the user's config. + * Lives in the platform data dir so every editor emits the same include. */ - public getIncludedSshConfigPath(): string { - return path.join(this.basePath, "ssh-config"); + public getSshConfigDir(): string { + switch (process.platform) { + case "win32": + return path.join( + process.env.APPDATA || path.join(os.homedir(), "AppData", "Roaming"), + "coder.coder-remote", + "ssh", + ); + case "darwin": + return path.join( + os.homedir(), + "Library", + "Application Support", + "coder.coder-remote", + "ssh", + ); + default: + return path.join( + process.env.XDG_DATA_HOME || + path.join(os.homedir(), ".local", "share"), + "coder.coder-remote", + "ssh", + ); + } + } + + /** This editor's generated SSH config for one deployment. */ + public getSshConfigPath(safeHostname: string): string { + return path.join( + this.getSshConfigDir(), + `${vscode.env.uriScheme}--${safeHostname}.conf`, + ); + } + + /** The deployment hostname if this editor generated the file, else undefined. */ + public parseSshConfigFile(fileName: string): string | undefined { + const prefix = `${vscode.env.uriScheme}--`; + return fileName.startsWith(prefix) && fileName.endsWith(".conf") + ? fileName.slice(prefix.length, -".conf".length) + : undefined; } /** diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 9dd7f8c1b..534fa64ae 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -54,6 +54,7 @@ import { migrateAuthToSecretsStorage } from "./migration"; import { SshConfig, type SshValues, + cleanupStaleSshConfigs, mergeSshConfigValues, parseCoderSshOptions, parseSshConfig, @@ -161,14 +162,11 @@ export class Remote { return; } - switch (classifyRemoteAuthority(parts)) { - case "current": - break; - case "legacy": - await this.migrateLegacyAuthority(remoteAuthority, startupMode); - return; - case "foreign": - return; + // parseRemoteAuthority returned null for foreign hosts, so this is + // either the current editor's authority or a migratable legacy one. + if (classifyRemoteAuthority(parts) === "legacy") { + await this.migrateLegacyAuthority(remoteAuthority, startupMode); + return; } this.logger.info("Setting up remote connection", { @@ -968,12 +966,14 @@ export class Remote { featureSet: FeatureSet, cliAuth: CliAuth, ): Promise { - // Our blocks live in our own file; the user's only gains the include. + // One file per (editor, deployment); the user's config gains one shared include. const sshConfig = new SshConfig(this.getMainSshConfigPath(), this.logger); await sshConfig.load(); - const coderConfigPath = this.pathResolver.getIncludedSshConfigPath(); - const coderConfig = new SshConfig(coderConfigPath, this.logger); - await coderConfig.load(); + // Never loaded: update() regenerates it without reading the old content. + const coderConfig = new SshConfig( + this.pathResolver.getSshConfigPath(safeHostname), + this.logger, + ); // Options the user set themselves win the merge below, so they are exempt // from the deny list. Both sources are local and already trusted: whoever @@ -1045,14 +1045,12 @@ export class Remote { } // Write our file before including it, so the include never dangles. - await coderConfig.update(safeHostname, sshValues, sshConfigOverrides); - await sshConfig.updateInclude( - { - id: vscode.env.uriScheme, - includePath: coderConfigPath, - }, - safeHostname, - ); + await coderConfig.update(sshValues, sshConfigOverrides); + const sharedSshConfigDir = this.pathResolver.getSshConfigDir(); + await sshConfig.updateInclude(sharedSshConfigDir, safeHostname); + // Our file was just written, so only other unused deployments are swept. + // Never throws, and the connection does not depend on it. + void cleanupStaleSshConfigs(sharedSshConfigDir, this.logger); // Mirror SSH's parse order; RemoteCommand can come from the user's config. return computeSshProperties( diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index 0357b020b..7479f3071 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -10,6 +10,7 @@ import * as os from "node:os"; import path from "node:path"; import { countSubstring, lowercase } from "../util"; +import { cleanupFiles } from "../util/fileCleanup"; import { renameWithRetry, tempFilePath } from "../util/fs"; import type { Logger } from "../logging/logger"; @@ -17,16 +18,10 @@ import type { Logger } from "../logging/logger"; class SshConfigBadFormat extends Error {} interface Block { - raw: string; start: number; end: number; } -interface Mutation { - apply(raw: string): string; - onSuccess?(): void; -} - export interface SshValues { Host: string; ProxyCommand: string; @@ -39,7 +34,7 @@ export interface SshValues { SetEnv?: string; } -/** Interface for the file system to make it easier to test. */ +/** Injectable for tests. */ export interface FileSystem { mkdir: typeof mkdir; readFile: typeof readFile; @@ -58,10 +53,10 @@ const defaultFileSystem: FileSystem = { writeFile, }; -/** Matches an SSH config key at the start of a line (e.g. "ConnectTimeout", "LogLevel"). */ +/** An SSH config key at the start of a line, e.g. "ConnectTimeout". */ const SSH_KEY_REGEX = /^[a-zA-Z0-9-]+/; -/** Matches the Coder CLI's START-CODER / END-CODER block, flexible on dash count. */ +/** The Coder CLI's START-CODER block, flexible on dash count. */ const CODER_BLOCK_REGEX = /^# -+START-CODER-+$(.*?)^# -+END-CODER-+$/ms; /** Matches a string that is only an SSH config key. */ @@ -70,34 +65,43 @@ const KEY_ONLY_REGEX = /^[a-zA-Z0-9-]+$/; /** Characters that would break a value out of its config line. */ const UNSAFE_CHARS_REGEX = /[\r\n\0]/; -const UPDATE_ATTEMPTS = 3; - interface BlockMarkers { start: string; end: string; } -// Labels are an editor ID for include blocks in the user's config and a -// deployment hostname for blocks in the editor-owned generated file. -function blockMarkers(label: string): BlockMarkers { +/** Released versions wrote deployment blocks with these markers into the user's config. */ +function legacyDeploymentMarkers(safeHostname: string): BlockMarkers { return { - start: `# --- START CODER ${label} ---`, - end: `# --- END CODER ${label} ---`, + start: `# --- START CODER VSCODE ${safeHostname} ---`, + end: `# --- END CODER VSCODE ${safeHostname} ---`, }; } -// Released versions wrote deployment blocks with this label into the user's config. -function legacyDeploymentMarkers(safeHostname: string): BlockMarkers { - return blockMarkers(`VSCODE ${safeHostname}`); -} +/** Shared include block; identical bytes from every editor, so writers converge. */ +const INCLUDE_MARKERS: BlockMarkers = { + start: "# --- START CODER ---", + end: "# --- END CODER ---", +}; -// Kept at the top of the editor-owned generated file. +/** Header of the generated per-deployment file. */ const CODER_SSH_CONFIG_HEADER = `# Coder workspace hosts. Do not edit; the Coder extension rewrites this file # on every connection. Override options with the "coder.sshConfig" setting.`; -export interface SshInclude { - id: string; - includePath: string; +/** Connects rewrite the file, so anything older is unused and safe to sweep. */ +const STALE_CONFIG_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; + +/** Delete generated configs (from any editor) not connected to recently. */ +export async function cleanupStaleSshConfigs( + dir: string, + logger: Logger, +): Promise { + await cleanupFiles(dir, logger, { + label: "generated SSH config", + filter: (name) => name.endsWith(".conf"), + select: (files, now) => + files.filter((file) => now - file.mtime > STALE_CONFIG_MAX_AGE_MS), + }); } /** @@ -216,30 +220,24 @@ export function parseCoderSshOptions(raw: string): Record { return parseSshConfig(sshOptionLines); } -/** - * Parse an array of SSH config lines into a Record. - * Handles both "Key value" and "Key=value" formats. - * Accumulates SetEnv values since SSH allows multiple environment variables. - */ +/** Parse "Key value" or "Key=value" lines, accumulating SetEnv values. */ export function parseSshConfig(lines: string[]): Record { return lines.reduce( (acc, line) => { const keyMatch = SSH_KEY_REGEX.exec(line); if (!keyMatch) { - return acc; // Malformed line + return acc; } const key = keyMatch[0]; const separator = line.at(key.length); if (separator !== "=" && separator !== " ") { - return acc; // Malformed line + return acc; } const value = line.slice(key.length + 1); - // Accumulate SetEnv values since there can be multiple. if (key.toLowerCase() === "setenv") { - // Ignore empty SetEnv values if (value !== "") { const existing = acc["SetEnv"]; acc["SetEnv"] = existing ? `${existing} ${value}` : value; @@ -253,20 +251,13 @@ export function parseSshConfig(lines: string[]): Record { ); } -/** - * Merge the given SSH config with the provided overrides. The merge handles - * key case insensitivity, so casing in the key does not matter. - */ +/** Merge overrides into config; keys match case-insensitively. */ export function mergeSshConfigValues( config: Record, overrides: Record, ): Record { const merged: Record = {}; - // We need to do a case insensitive match for the overrides as ssh config keys are case insensitive. - // To get the correct key:value, use: - // key = caseInsensitiveOverrides[key.toLowerCase()] - // value = overrides[key] const caseInsensitiveOverrides: Record = {}; Object.keys(overrides).forEach((key) => { caseInsensitiveOverrides[key.toLowerCase()] = key; @@ -274,13 +265,12 @@ export function mergeSshConfigValues( Object.keys(config).forEach((key) => { const lower = key.toLowerCase(); - // If the key is in overrides, use the override value. if (caseInsensitiveOverrides[lower]) { const correctCaseKey = caseInsensitiveOverrides[lower]; const value = overrides[correctCaseKey]; delete caseInsensitiveOverrides[lower]; - // Special handling for SetEnv - concatenate values instead of replacing. + // SetEnv concatenates instead of replacing. if (lower === "setenv") { if (value === "") { merged["SetEnv"] = config[key]; @@ -290,25 +280,22 @@ export function mergeSshConfigValues( return; } - // If the value is empty, do not add the key. It is being removed. + // An empty override removes the key. if (value !== "") { merged[correctCaseKey] = value; } return; } - // If no override, take the original value. if (config[key] !== "") { merged[key] = config[key]; } }); - // Add remaining overrides. Object.keys(caseInsensitiveOverrides).forEach((lower) => { const correctCaseKey = caseInsensitiveOverrides[lower]; const value = overrides[correctCaseKey]; - // Special handling for SetEnv - concatenate if already exists if (lower === "setenv" && merged["SetEnv"]) { merged["SetEnv"] = `${merged["SetEnv"]} ${value}`; } else { @@ -336,44 +323,33 @@ export class SshConfig { } async load() { - try { - this.raw = await this.fileSystem.readFile(this.filePath, "utf-8"); - this.logger.debug("Loaded SSH config", this.filePath); - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== "ENOENT") { - throw error; - } - this.logger.debug( - "SSH config file not found, starting fresh", - this.filePath, - ); - this.raw = ""; - } + this.raw = await this.read(); + this.logger.debug("Loaded SSH config", this.filePath); } /** - * Update the block for the deployment with the provided hostname. - * @throws {Error} when the hostname, values, or overrides fail validation. + * Regenerate the whole per-deployment file; last-writer-wins. Always + * writes, so the file's mtime marks the last connect for the stale sweep. */ - async update( - safeHostname: string, - values: SshValues, - overrides?: Record, - ) { - const block = this.renderDeploymentBlock(safeHostname, values, overrides); - await this.mutate({ - apply: (raw) => this.mergeDeployment(raw, safeHostname, block), - }); + async update(values: SshValues, overrides?: Record) { + const block = this.renderDeploymentBlock(values, overrides); + this.raw = `${CODER_SSH_CONFIG_HEADER}\n\n${block}`; + await this.save(); } - /** Include an editor's config first so its options win, removing its deployment block. */ - async updateInclude(include: SshInclude, safeHostname: string) { - const block = this.renderIncludeBlock(include); - await this.mutate({ - apply: (raw) => this.mergeInclude(raw, include.id, block, safeHostname), - onSuccess: () => - this.logger.debug("Including SSH config", include.includePath), - }); + /** + * Keep the shared include first so our options win, removing superseded + * blocks. Read-modify-write with no locking, like the CLI's config-ssh; + * when the include is already in place nothing is written. + */ + async updateInclude(includeDir: string, safeHostname: string) { + const block = this.renderIncludeBlock(includeDir); + const raw = await this.read(); + this.raw = this.mergeInclude(raw, block, safeHostname); + if (this.raw !== raw) { + await this.save(); + this.logger.debug("Including SSH config dir", includeDir); + } } public getRaw() { @@ -385,16 +361,13 @@ export class SshConfig { } /** - * Render the deployment's block, validating everything written into it, - * including the hostname, which lands in the block marker comments. - * @throws {Error} when the hostname, values, or overrides fail validation. + * Render the deployment's block, validating everything written into it. + * @throws {Error} when the values or overrides fail validation. */ private renderDeploymentBlock( - safeHostname: string, values: SshValues, overrides?: Record, ): string { - validateSshValue("deployment hostname", safeHostname); validateSshConfigOptions({ ...values }); validateSshConfigOptions(overrides ?? {}); const { Host, ...defaults } = values; @@ -403,41 +376,7 @@ export class SshConfig { .sort() .filter((key) => config[key] !== "") .map((key) => ` ${key} ${config[key]}`); - const markers = blockMarkers(safeHostname); - return [markers.start, `Host ${Host}`, ...options, markers.end].join("\n"); - } - - private mergeDeployment( - raw: string, - safeHostname: string, - block: string, - ): string { - let merged: string; - const existing = this.findBlock(raw, blockMarkers(safeHostname)); - if (existing) { - this.logger.debug("Replacing SSH config block", safeHostname); - merged = this.replaceRange(raw, existing, block); - } else { - this.logger.debug("Appending new SSH config block", safeHostname); - merged = raw ? `${raw.trimEnd()}\n\n${block}` : block; - } - return this.moveHeaderToTop(merged); - } - - /** The user may have added content above the header; move it back to the top. */ - private moveHeaderToTop(merged: string): string { - const start = merged.indexOf(CODER_SSH_CONFIG_HEADER); - if (start === 0) { - return merged; - } - if (start > 0) { - merged = this.removeRange(merged, { - raw: CODER_SSH_CONFIG_HEADER, - start, - end: start + CODER_SSH_CONFIG_HEADER.length, - }); - } - return `${CODER_SSH_CONFIG_HEADER}\n\n${merged}`; + return [`Host ${Host}`, ...options].join("\n"); } private findBlock(raw: string, markers: BlockMarkers): Block | undefined { @@ -453,46 +392,38 @@ export class SshConfig { `Malformed config: ${this.filePath} has ${startCount} "${markers.start}" blocks. Please remove all but one.`, ); } + if (startCount === 0) { + return undefined; + } const start = raw.indexOf(markers.start); const endMarkerStart = raw.indexOf(markers.end); - if (start === -1 || endMarkerStart === -1) return undefined; if (endMarkerStart < start) { throw new SshConfigBadFormat( `Malformed config: ${this.filePath} has an "${markers.end}" marker before its "${markers.start}" marker.`, ); } - const end = endMarkerStart + markers.end.length; - return { raw: raw.slice(start, end), start, end }; - } - - private replaceRange(raw: string, range: Block, replacement: string): string { - return raw.slice(0, range.start) + replacement + raw.slice(range.end); + return { start, end: endMarkerStart + markers.end.length }; } - private renderIncludeBlock({ id, includePath }: SshInclude): string { - if (id.length === 0) { - throw new Error("Editor ID must not be empty."); - } - const markers = blockMarkers(id); + private renderIncludeBlock(includeDir: string): string { return [ - markers.start, + INCLUDE_MARKERS.start, "# Moves back to the top on connect; override options via coder.sshConfig.", - `Include "${this.escapeIncludePath(includePath)}"`, - markers.end, + `Include "${this.escapeIncludePath(includeDir)}/*.conf"`, + INCLUDE_MARKERS.end, ].join("\n"); } private escapeIncludePath(includePath: string): string { - // Prefer ~/... so quirks in the home path (spaces, %, glob characters) - // never reach the emitted argument. ssh expands the tilde itself. + // Emit ~/... so home-path quirks (spaces, %, glob chars) never reach ssh. const relative = path.relative(os.homedir(), includePath); const argument = relative && !relative.startsWith("..") && !path.isAbsolute(relative) ? `~/${relative}` : includePath; - // ssh_config has no escape for '"' inside a quoted argument, and - // OpenSSH 9.9+ fatals on unknown %-tokens in Include arguments. + // No escape exists for '"' in quoted arguments; OpenSSH 9.9+ fatals on + // unknown %-tokens in Include arguments. if (/[\r\n\0"%]/.test(argument)) { throw new Error( "SSH include path must not contain CR, LF, NUL, %, or double-quote characters.", @@ -503,22 +434,16 @@ export class SshConfig { private mergeInclude( raw: string, - editorId: string, includeBlock: string, safeHostname: string, ): string { let rest = raw; - const editorBlock = this.findBlock(rest, blockMarkers(editorId)); - if (editorBlock) { - rest = this.removeRange(rest, editorBlock); - } - const deployment = this.findBlock( - rest, - legacyDeploymentMarkers(safeHostname), - ); - if (deployment) { - this.logger.debug("Removing superseded SSH config block", safeHostname); - rest = this.removeRange(rest, deployment); + const superseded = [INCLUDE_MARKERS, legacyDeploymentMarkers(safeHostname)]; + for (const markers of superseded) { + const block = this.findBlock(rest, markers); + if (block) { + rest = this.removeRange(rest, block); + } } return [includeBlock, rest].filter(Boolean).join("\n\n"); } @@ -529,51 +454,20 @@ export class SshConfig { return [before, after].filter(Boolean).join("\n\n"); } - private async mutate(mutation: Mutation): Promise { - let snapshot = this.getRaw(); - // Retries only handle concurrent writers (save() returns false on a - // conflict); I/O errors like EACCES throw immediately and are not retried. - for (let attempt = 0; attempt < UPDATE_ATTEMPTS; attempt++) { - const updated = mutation.apply(snapshot); - if (updated === snapshot) { - this.raw = snapshot; - return; - } - - this.raw = updated; - if (!(await this.save(snapshot))) { - snapshot = await this.read(); - continue; - } - - const latest = await this.read(); - if (mutation.apply(latest) === latest) { - this.raw = latest; - mutation.onSuccess?.(); - return; - } - snapshot = latest; - } - - this.raw = snapshot; - throw new Error( - `Failed to update SSH config at ${this.filePath} because it kept changing, likely due to another editor writing it at the same time. Please try again.`, - ); - } - - private async save(expectedRaw?: string): Promise { - // We want to preserve the original file mode. + /** Atomically write raw via a temp file. */ + private async save(): Promise { + // Preserve the existing file mode. const existingMode = await this.fileSystem .stat(this.filePath) .then((stat) => stat.mode) .catch((ex: NodeJS.ErrnoException) => { if (ex.code === "ENOENT") { - return 0o600; // default to 0600 if file does not exist + return 0o600; } - throw ex; // Any other error is unexpected + throw ex; }); await this.fileSystem.mkdir(path.dirname(this.filePath), { - mode: 0o700, // only owner has rwx permission, not group or everyone. + mode: 0o700, recursive: true, }); const fileName = path.basename(this.filePath); @@ -596,34 +490,14 @@ export class SshConfig { } try { - if (expectedRaw !== undefined) { - const latest = await this.read(); - if (latest !== expectedRaw) { - await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => { - this.logger.warn( - "Failed to clean up conflicted temp SSH config file", - tempPath, - unlinkErr, - ); - }); - return false; - } - } await renameWithRetry( (src, dest) => this.fileSystem.rename(src, dest), tempPath, this.filePath, ); this.logger.debug("Saved SSH config", this.filePath); - return true; } catch (err) { - await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => { - this.logger.warn( - "Failed to clean up temp SSH config file", - tempPath, - unlinkErr, - ); - }); + await this.discardTemp(tempPath); throw new Error( `Failed to rename temporary SSH config file at ${tempPath} to ${this.filePath}: ${ err instanceof Error ? err.message : String(err) @@ -633,6 +507,16 @@ export class SshConfig { } } + private async discardTemp(tempPath: string): Promise { + await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => { + this.logger.warn( + "Failed to clean up temp SSH config file", + tempPath, + unlinkErr, + ); + }); + } + private async read(): Promise { try { return await this.fileSystem.readFile(this.filePath, "utf-8"); diff --git a/src/util/authority.ts b/src/util/authority.ts index 1b9446fd0..035450d48 100644 --- a/src/util/authority.ts +++ b/src/util/authority.ts @@ -141,9 +141,8 @@ export function toRemoteAuthority( return remoteAuthority; } -export function toCurrentAuthorityHostPrefix(safeHostname?: string): string { - const prefix = currentAuthorityPrefix(); - return safeHostname ? `${prefix}.${safeHostname}--` : `${prefix}--`; +export function toCurrentAuthorityHostPrefix(safeHostname: string): string { + return `${currentAuthorityPrefix()}.${safeHostname}--`; } export function retargetRemoteAuthority(authority: string): string { diff --git a/test/unit/core/pathResolver.test.ts b/test/unit/core/pathResolver.test.ts index bf1c3b3df..a6117dc56 100644 --- a/test/unit/core/pathResolver.test.ts +++ b/test/unit/core/pathResolver.test.ts @@ -1,5 +1,6 @@ +import * as os from "node:os"; import * as path from "path"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { PathResolver } from "@/core/pathResolver"; @@ -38,13 +39,105 @@ describe("PathResolver", () => { }); }); - describe("getIncludedSshConfigPath", () => { - it("uses the extension's global storage directory", () => { + describe("getSshConfigDir", () => { + const realPlatform = process.platform; + afterEach(() => { + Object.defineProperty(process, "platform", { value: realPlatform }); + }); + + interface SharedDirCase { + name: string; + platform: NodeJS.Platform; + env: Record; + expected: string; + } + it.each([ + { + name: "XDG_DATA_HOME on Linux", + platform: "linux", + env: { XDG_DATA_HOME: "/xdg/data" }, + expected: path.join("/xdg/data", "coder.coder-remote", "ssh"), + }, + { + name: "the XDG default on Linux", + platform: "linux", + env: { XDG_DATA_HOME: "" }, + expected: path.join( + os.homedir(), + ".local", + "share", + "coder.coder-remote", + "ssh", + ), + }, + { + name: "Application Support on macOS", + platform: "darwin", + env: {}, + expected: path.join( + os.homedir(), + "Library", + "Application Support", + "coder.coder-remote", + "ssh", + ), + }, + { + name: "APPDATA on Windows", + platform: "win32", + env: { + APPDATA: path.join("C:", "Users", "jane", "AppData", "Roaming"), + }, + expected: path.join( + "C:", + "Users", + "jane", + "AppData", + "Roaming", + "coder.coder-remote", + "ssh", + ), + }, + { + name: "the profile default on Windows", + platform: "win32", + env: { APPDATA: "" }, + expected: path.join( + os.homedir(), + "AppData", + "Roaming", + "coder.coder-remote", + "ssh", + ), + }, + ])("uses $name", ({ platform, env, expected }) => { + Object.defineProperty(process, "platform", { value: platform }); + for (const [key, value] of Object.entries(env)) { + vi.stubEnv(key, value); + } + expectPathsEqual(pathResolver.getSshConfigDir(), expected); + }); + }); + + describe("getSshConfigPath", () => { + it("names the file after the editor and deployment", () => { expectPathsEqual( - pathResolver.getIncludedSshConfigPath(), - path.join(basePath, "ssh-config"), + pathResolver.getSshConfigPath("dev.coder.com"), + path.join(pathResolver.getSshConfigDir(), "vscode--dev.coder.com.conf"), ); }); + + it("parses the hostname only from this editor's generated files", () => { + expect( + pathResolver.parseSshConfigFile("vscode--dev.coder.com.conf"), + ).toBe("dev.coder.com"); + expect( + pathResolver.parseSshConfigFile("cursor--dev.coder.com.conf"), + ).toBeUndefined(); + expect( + pathResolver.parseSshConfigFile("vscode--notes.txt"), + ).toBeUndefined(); + }); }); describe("getProxyLogPath", () => { diff --git a/test/unit/remote/sshConfig.openssh.test.ts b/test/unit/remote/sshConfig.openssh.test.ts index e8f005efe..b8ffaf7d1 100644 --- a/test/unit/remote/sshConfig.openssh.test.ts +++ b/test/unit/remote/sshConfig.openssh.test.ts @@ -14,8 +14,12 @@ const sshAvailable = !spawnSync("ssh", ["-V"]).error; // Generous for slow CI runners; each ssh -G call is local and fast. const TEST_TIMEOUT_MS = 30_000; -const sshValues = (hostname: string, proxyCommand: string): SshValues => ({ - Host: `coder-vscode.${hostname}--*`, +const sshValues = ( + hostname: string, + proxyCommand: string, + editor = "vscode", +): SshValues => ({ + Host: `coder-${editor}.${hostname}--*`, ProxyCommand: proxyCommand, ConnectTimeout: "0", StrictHostKeyChecking: "no", @@ -41,19 +45,21 @@ async function createFixture(includeDirName: string) { const root = process.platform === "win32" ? os.homedir() : os.tmpdir(); tempDir = await fs.mkdtemp(path.join(root, "coder-ssh-test-")); const logger = createMockLogger(); - const includePath = path.join(tempDir, includeDirName, "ssh-config"); + const includeDir = path.join(tempDir, includeDirName); const userConfigPath = path.join(tempDir, "config"); return { - includePath, + includeDir, /** What the extension does on connect: write our file, then include it. */ - async connect(hostname: string, proxyCommand: string) { - const coderConfig = new SshConfig(includePath, logger); - await coderConfig.load(); - await coderConfig.update(hostname, sshValues(hostname, proxyCommand)); + async connect(hostname: string, proxyCommand: string, editor = "vscode") { + const coderConfig = new SshConfig( + path.join(includeDir, `${editor}--${hostname}.conf`), + logger, + ); + await coderConfig.update(sshValues(hostname, proxyCommand, editor)); const userConfig = new SshConfig(userConfigPath, logger); await userConfig.load(); - await userConfig.updateInclude({ id: "vscode", includePath }, hostname); + await userConfig.updateInclude(includeDir, hostname); }, async seedUserConfig(contents: string) { await fs.writeFile(userConfigPath, contents); @@ -87,6 +93,7 @@ describe.skipIf(!sshAvailable)("include resolution by real OpenSSH", () => { ); await ssh.connect("dev.coder.com", "echo dev-wins"); await ssh.connect("eu.coder.com", "echo eu-wins"); + await ssh.connect("dev.coder.com", "echo cursor-wins", "cursor"); await ssh.appendUserConfig( "\nHost *\n ProxyCommand echo user-wins\n ConnectTimeout 9\n", ); @@ -100,15 +107,18 @@ describe.skipIf(!sshAvailable)("include resolution by real OpenSSH", () => { expect( await ssh.resolve("coder-vscode.eu.coder.com--user--ws"), ).toContain("proxycommand echo eu-wins"); + expect( + await ssh.resolve("coder-cursor.dev.coder.com--user--ws"), + ).toContain("proxycommand echo cursor-wins"); }, TEST_TIMEOUT_MS, ); - // Windows forbids these characters in file names. - it.skipIf(process.platform === "win32")( + // Brackets are legal in file names on every platform, unlike * and ?. + it( "honors escaped glob characters in the include path", async () => { - const ssh = await createFixture("we[i]rd *dir?"); + const ssh = await createFixture("we[i]rd [dir]"); await ssh.connect("dev.coder.com", "echo dev-wins"); expect( await ssh.resolve("coder-vscode.dev.coder.com--user--ws"), @@ -118,12 +128,12 @@ describe.skipIf(!sshAvailable)("include resolution by real OpenSSH", () => { ); it( - "keeps ssh working when the included file is deleted", + "keeps ssh working when the included directory is deleted", async () => { const ssh = await createFixture("Code Dir"); await ssh.connect("dev.coder.com", "echo dev-wins"); await ssh.appendUserConfig("\nHost *\n ProxyCommand echo user-wins\n"); - await fs.rm(ssh.includePath); + await fs.rm(ssh.includeDir, { recursive: true }); expect( await ssh.resolve("coder-vscode.dev.coder.com--user--ws"), diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index 469b98964..797a7fbd6 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -4,11 +4,11 @@ import * as os from "node:os"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { + cleanupStaleSshConfigs, mergeSshConfigValues, parseCoderSshOptions, parseSshConfig, SshConfig, - type SshInclude, type SshValues, validateDeploymentSshOptions, } from "@/remote/sshConfig"; @@ -50,22 +50,14 @@ const BENIGN_DEPLOYMENT_OPTIONS = { serveraliveinterval: "5", } as const; -const deploymentBlock = `# --- START CODER dev.coder.com --- -Host coder-vscode.dev.coder.com--* +const deploymentBlock = `Host coder-vscode.dev.coder.com--* ConnectTimeout 0 LogLevel ERROR ProxyCommand some-command-here ServerAliveCountMax 3 ServerAliveInterval 10 StrictHostKeyChecking no - UserKnownHostsFile /dev/null -# --- END CODER dev.coder.com ---`; -const staleDeploymentBlock = `# --- START CODER dev.coder.com --- -Host stale -# --- END CODER dev.coder.com ---`; -const otherDeploymentBlock = `# --- START CODER other.coder.com --- -Host coder-vscode.other.coder.com--* -# --- END CODER other.coder.com ---`; + UserKnownHostsFile /dev/null`; // Released versions wrote deployment blocks with the VSCODE label. const legacyDeploymentBlock = `# --- START CODER VSCODE dev.coder.com --- Host stale @@ -77,30 +69,20 @@ const deploymentUnawareBlock = `# --- START CODER VSCODE --- Host coder-vscode--* # --- END CODER VSCODE ---`; -const include = { - id: "vscode", - includePath: "~/.ssh/coder/config", -} satisfies SshInclude; +const includeDir = "~/.ssh/coder"; -function renderIncludeBlock(value: SshInclude): string { - return `# --- START CODER ${value.id} --- +function renderIncludeBlock(dir: string): string { + return `# --- START CODER --- # Moves back to the top on connect; override options via coder.sshConfig. -Include "${value.includePath}" -# --- END CODER ${value.id} ---`; +Include "${dir}/*.conf" +# --- END CODER ---`; } -const includeBlock = renderIncludeBlock(include); -const otherIncludeBlock = renderIncludeBlock({ - id: "windsurf", - includePath: "~/.ssh/windsurf/config", -}); +const includeBlock = renderIncludeBlock(includeDir); const mockLogger = createMockLogger(); -// Captured before any spy so injected implementations can delegate to memfs. -const realReadFile = fsPromises.readFile; -const realRename = fsPromises.rename; -const readConfig = () => realReadFile(sshFilePath, "utf-8"); +const readConfig = () => fsPromises.readFile(sshFilePath, "utf-8"); async function loadSshConfig( contents?: string, @@ -121,33 +103,15 @@ async function updateDeployment( overrides?: Record, ): Promise { const sshConfig = await loadSshConfig(contents); - await sshConfig.update(hostname, values, overrides); + await sshConfig.update(values, overrides); } async function updateInclude( contents: string, - value: SshInclude = include, + dir: string = includeDir, ): Promise { const sshConfig = await loadSshConfig(contents); - await sshConfig.updateInclude(value, hostname); -} - -function injectConcurrentChangeBeforeRename(contents: string): void { - vi.spyOn(fsPromises, "readFile").mockImplementationOnce( - (filePath, options) => { - vol.writeFileSync(sshFilePath, contents); - return realReadFile(filePath, options); - }, - ); -} - -function injectConcurrentChangeAfterRename(contents: string): void { - vi.spyOn(fsPromises, "rename").mockImplementationOnce( - async (source, destination) => { - await realRename(source, destination); - vol.writeFileSync(sshFilePath, contents); - }, - ); + await sshConfig.updateInclude(dir, hostname); } beforeEach(() => { @@ -175,45 +139,15 @@ describe("SshConfig.update", () => { expect(configDir.mode & 0o777).toBe(0o700); }); - interface DeploymentMergeCase { - name: string; - existing: string; - expected: string; - } - it.each([ - { - name: "appends after user config", - existing: "Host personal\n HostName example.com\n\n", - expected: `${fileHeader}\n\nHost personal\n HostName example.com\n\n${deploymentBlock}`, - }, - { - name: "replaces only the current deployment", - existing: `Host before\n\n${staleDeploymentBlock}\n\nHost after`, - expected: `${fileHeader}\n\nHost before\n\n${deploymentBlock}\n\nHost after`, - }, - { - name: "does not duplicate the header", - existing: `${fileHeader}\n\n${staleDeploymentBlock}`, - expected: `${fileHeader}\n\n${deploymentBlock}`, - }, - { - name: "moves the header back to the top", - existing: `Host personal\n\n${fileHeader}\n\n${staleDeploymentBlock}`, - expected: `${fileHeader}\n\nHost personal\n\n${deploymentBlock}`, - }, - { - name: "preserves another deployment", - existing: otherDeploymentBlock, - expected: `${fileHeader}\n\n${otherDeploymentBlock}\n\n${deploymentBlock}`, - }, - { - name: "preserves deployment-unaware config", - existing: deploymentUnawareBlock, - expected: `${fileHeader}\n\n${deploymentUnawareBlock}\n\n${deploymentBlock}`, - }, - ])("$name", async ({ existing, expected }) => { - await updateDeployment(existing); - expect(await readConfig()).toBe(expected); + it("regenerates the whole file over existing content", async () => { + await updateDeployment("Host personal\n HostName example.com\n\n"); + expect(await readConfig()).toBe(`${fileHeader}\n\n${deploymentBlock}`); + }); + + it("rewrites an unchanged file so its mtime marks the last connect", async () => { + const renameSpy = vi.spyOn(fsPromises, "rename"); + await updateDeployment(`${fileHeader}\n\n${deploymentBlock}`); + expect(renameSpy).toHaveBeenCalledTimes(1); }); it("applies sorted case-insensitive overrides, additions, and removals", async () => { @@ -227,7 +161,6 @@ describe("SshConfig.update", () => { expect(await readConfig()).toBe(`${fileHeader} -# --- START CODER dev.coder.com --- Host coder-vscode.dev.coder.com--* ConnectTimeout 500 ExtraKey ExtraValue @@ -235,52 +168,7 @@ Host coder-vscode.dev.coder.com--* ServerAliveCountMax 3 ServerAliveInterval 10 UserKnownHostsFile /dev/null - loglevel DEBUG -# --- END CODER dev.coder.com ---`); - }); - - interface MalformedDeploymentCase { - name: string; - existing: string; - error: string; - } - it.each([ - { - name: "missing end marker", - existing: "# --- START CODER dev.coder.com ---", - error: - 'has 1 "# --- START CODER dev.coder.com ---" and 0 "# --- END CODER dev.coder.com ---" markers', - }, - { - name: "extra start marker", - existing: `${staleDeploymentBlock}\n# --- START CODER dev.coder.com ---`, - error: - 'has 2 "# --- START CODER dev.coder.com ---" and 1 "# --- END CODER dev.coder.com ---" markers', - }, - { - name: "extra end marker", - existing: `${staleDeploymentBlock}\n# --- END CODER dev.coder.com ---`, - error: - 'has 1 "# --- START CODER dev.coder.com ---" and 2 "# --- END CODER dev.coder.com ---" markers', - }, - { - name: "duplicate blocks", - existing: `${staleDeploymentBlock}\n${staleDeploymentBlock}`, - error: 'has 2 "# --- START CODER dev.coder.com ---" blocks', - }, - { - name: "end before start", - existing: - "# --- END CODER dev.coder.com ---\n# --- START CODER dev.coder.com ---", - error: - '"# --- END CODER dev.coder.com ---" marker before its "# --- START CODER dev.coder.com ---" marker', - }, - ])("rejects $name", async ({ existing, error }) => { - const sshConfig = await loadSshConfig(existing); - await expect(sshConfig.update(hostname, BASE_SSH_VALUES)).rejects.toThrow( - error, - ); - expect(await readConfig()).toBe(existing); + loglevel DEBUG`); }); /** @@ -289,16 +177,11 @@ Host coder-vscode.dev.coder.com--* */ interface RejectCase { name: string; - safeHostname?: string; values?: SshValues; overrides?: Record; } it.each([ - { - name: "deployment hostname newline", - safeHostname: "dev.coder.com\nHost *", - }, { name: "Host value carriage return", values: { ...BASE_SSH_VALUES, Host: "coder-vscode--*\rMatch all" }, @@ -320,16 +203,10 @@ Host coder-vscode.dev.coder.com--* }, ])( "rejects unsafe serialization: $name", - async ({ - safeHostname = hostname, - values = BASE_SSH_VALUES, - overrides, - }) => { + async ({ values = BASE_SSH_VALUES, overrides }) => { const sshConfig = await loadSshConfig(); - await expect( - sshConfig.update(safeHostname, values, overrides), - ).rejects.toThrow(); + await expect(sshConfig.update(values, overrides)).rejects.toThrow(); expect(vol.existsSync(sshFilePath)).toBe(false); }, ); @@ -362,8 +239,8 @@ describe("SshConfig.updateInclude", () => { expected: `${includeBlock}\n\nHost *`, }, { - name: "moves to first", - existing: `Host *\n\n${includeBlock}`, + name: "moves a stale block to first", + existing: `Host *\n\n${includeBlock.replace("coder/*.conf", "old/*.conf")}`, expected: `${includeBlock}\n\nHost *`, }, ])("handles $name", async ({ existing, expected }) => { @@ -371,14 +248,6 @@ describe("SshConfig.updateInclude", () => { expect(await readConfig()).toBe(expected); }); - it("replaces the current editor block and preserves another editor", async () => { - const stale = includeBlock.replace("coder/config", "old/config"); - await updateInclude(`Host *\n\n${stale}\n\n${otherIncludeBlock}`); - expect(await readConfig()).toBe( - `${includeBlock}\n\nHost *\n\n${otherIncludeBlock}`, - ); - }); - it("removes the current deployment and preserves other and deployment-unaware blocks", async () => { await updateInclude( `${legacyOtherDeploymentBlock}\n\n${legacyDeploymentBlock}\n\n${deploymentUnawareBlock}`, @@ -395,95 +264,66 @@ describe("SshConfig.updateInclude", () => { } it.each([ { - name: "missing end marker", - existing: includeBlock.replace("# --- END CODER vscode ---", ""), - error: - 'has 1 "# --- START CODER vscode ---" and 0 "# --- END CODER vscode ---" markers', - }, - { - name: "mismatched end marker", - existing: includeBlock.replace( - "# --- END CODER vscode ---", - "# --- END CODER windsurf ---", - ), + name: "extra end marker", + existing: `${includeBlock}\n# --- END CODER ---`, error: - 'has 1 "# --- START CODER vscode ---" and 0 "# --- END CODER vscode ---" markers', + 'has 1 "# --- START CODER ---" and 2 "# --- END CODER ---" markers', }, { name: "duplicate blocks", existing: `${includeBlock}\n${includeBlock}`, - error: 'has 2 "# --- START CODER vscode ---" blocks', + error: 'has 2 "# --- START CODER ---" blocks', }, { name: "end before start", - existing: "# --- END CODER vscode ---\n# --- START CODER vscode ---", + existing: "# --- END CODER ---\n# --- START CODER ---", error: - '"# --- END CODER vscode ---" marker before its "# --- START CODER vscode ---" marker', + '"# --- END CODER ---" marker before its "# --- START CODER ---" marker', }, ])("rejects $name", async ({ existing, error }) => { await expect(updateInclude(existing)).rejects.toThrow(error); expect(await readConfig()).toBe(existing); }); - it("supports dashed editor IDs", async () => { - const dashed = { ...include, id: "vscode-insiders" }; - await updateInclude("", dashed); - expect(await readConfig()).toBe(renderIncludeBlock(dashed)); - }); - - it("rejects an empty editor ID", async () => { - await expect(updateInclude("", { ...include, id: "" })).rejects.toThrow( - "Editor ID must not be empty", - ); - }); - interface IncludePathEscapeCase { - includePath: string; + dir: string; escaped: string; } it.each([ { - includePath: "~/.ssh/we[i]rd/*?[config]", - escaped: "~/.ssh/we\\[i\\]rd/\\*\\?\\[config\\]", + dir: "~/.ssh/we[i]rd/*?[dir]", + escaped: "~/.ssh/we\\[i\\]rd/\\*\\?\\[dir\\]", }, { - includePath: "C:\\Users\\Jane Doe\\config", - escaped: "C:/Users/Jane Doe/config", + dir: "C:\\Users\\Jane Doe\\ssh", + escaped: "C:/Users/Jane Doe/ssh", }, - ])("escapes $includePath", async ({ includePath, escaped }) => { - await updateInclude("", { ...include, includePath }); - expect(await readConfig()).toContain(`Include "${escaped}"`); + ])("escapes $dir", async ({ dir, escaped }) => { + await updateInclude("", dir); + expect(await readConfig()).toContain(`Include "${escaped}/*.conf"`); }); // A tilde swallows home-path quirks that ssh could not read back otherwise. - it.each([ - { label: "plain", home: homeDir }, - { label: "weird", home: "/home/we[i]rd %user" }, - ])( - "writes a $label home-relative include path with a tilde", - async ({ home }) => { - vi.mocked(os.homedir).mockReturnValue(home); - await updateInclude("", { - ...include, - includePath: `${home}/.config/Code/ssh-config`, - }); - expect(await readConfig()).toContain( - 'Include "~/.config/Code/ssh-config"', - ); - }, - ); + it("writes a home-relative include path with a tilde", async () => { + const home = "/home/we[i]rd %user"; + vi.mocked(os.homedir).mockReturnValue(home); + await updateInclude("", `${home}/.local/share/coder.coder-remote/ssh`); + expect(await readConfig()).toContain( + 'Include "~/.local/share/coder.coder-remote/ssh/*.conf"', + ); + }); - type InvalidIncludePath = string; - it.each([ + type InvalidIncludeDir = string; + it.each([ "path\rname", "path\nname", "path\0name", 'path"name', "path%name", - ])("rejects unrepresentable include paths", async (includePath) => { - await expect( - updateInclude("", { ...include, includePath }), - ).rejects.toThrow("must not contain CR, LF, NUL"); + ])("rejects unrepresentable include paths", async (dir) => { + await expect(updateInclude("", dir)).rejects.toThrow( + "must not contain CR, LF, NUL", + ); }); }); @@ -498,12 +338,12 @@ describe("persistence", () => { { name: "existing file", existing: "Host *", mode: 0o640 }, ])("uses the correct mode for a $name", async ({ existing, mode }) => { const sshConfig = await loadSshConfig(existing, mode); - await sshConfig.update(hostname, BASE_SSH_VALUES); + await sshConfig.update(BASE_SSH_VALUES); expect(vol.statSync(sshFilePath).mode & 0o777).toBe(mode); }); - type FileSystemErrorStage = "load" | "conflict read" | "stat"; - it.each(["load", "conflict read", "stat"])( + type FileSystemErrorStage = "load" | "include read" | "stat"; + it.each(["load", "include read", "stat"])( "propagates non-ENOENT %s errors", async (stage) => { const denied = Object.assign(new Error("denied"), { code: "EACCES" }); @@ -515,14 +355,15 @@ describe("persistence", () => { return; } const sshConfig = await loadSshConfig("Host initial"); - if (stage === "conflict read") { + if (stage === "include read") { vi.spyOn(fsPromises, "readFile").mockRejectedValueOnce(denied); - } else { - vi.spyOn(fsPromises, "stat").mockRejectedValueOnce(denied); + await expect( + sshConfig.updateInclude(includeDir, hostname), + ).rejects.toThrow("denied"); + return; } - await expect(sshConfig.update(hostname, BASE_SSH_VALUES)).rejects.toThrow( - "denied", - ); + vi.spyOn(fsPromises, "stat").mockRejectedValueOnce(denied); + await expect(sshConfig.update(BASE_SSH_VALUES)).rejects.toThrow("denied"); }, ); @@ -531,7 +372,7 @@ describe("persistence", () => { vi.spyOn(fsPromises, "writeFile").mockRejectedValueOnce( new Error("EACCES"), ); - await expect(sshConfig.update(hostname, BASE_SSH_VALUES)).rejects.toThrow( + await expect(sshConfig.update(BASE_SSH_VALUES)).rejects.toThrow( /Failed to write temporary SSH config file.*EACCES/, ); }); @@ -540,7 +381,7 @@ describe("persistence", () => { const sshConfig = await loadSshConfig("Host initial"); const error = Object.assign(new Error("EXDEV"), { code: "EXDEV" }); vi.spyOn(fsPromises, "rename").mockRejectedValueOnce(error); - await expect(sshConfig.update(hostname, BASE_SSH_VALUES)).rejects.toThrow( + await expect(sshConfig.update(BASE_SSH_VALUES)).rejects.toThrow( "Failed to rename temporary SSH config file", ); const leftoverTempFiles = Object.keys(vol.toJSON()).filter((filePath) => @@ -549,70 +390,46 @@ describe("persistence", () => { expect(leftoverTempFiles).toEqual([]); }); - it("retries a transient Windows rename failure", async () => { - const realPlatform = process.platform; - Object.defineProperty(process, "platform", { value: "win32" }); - vi.useFakeTimers(); - try { - const sshConfig = await loadSshConfig("Host initial"); - const error = Object.assign(new Error("EPERM"), { code: "EPERM" }); - const renameSpy = vi - .spyOn(fsPromises, "rename") - .mockRejectedValueOnce(error); - const update = sshConfig.update(hostname, BASE_SSH_VALUES); - await vi.advanceTimersByTimeAsync(100); - await update; - expect(renameSpy).toHaveBeenCalledTimes(2); - } finally { - vi.useRealTimers(); - Object.defineProperty(process, "platform", { value: realPlatform }); - } - }); - - it("retries an update conflict detected before rename", async () => { + it("writes over a concurrent change using the freshly read content", async () => { const sshConfig = await loadSshConfig("Host initial"); - const writeFileSpy = vi.spyOn(fsPromises, "writeFile"); - const renameSpy = vi.spyOn(fsPromises, "rename"); - injectConcurrentChangeBeforeRename(otherDeploymentBlock); - await sshConfig.update(hostname, BASE_SSH_VALUES); - expect(await readConfig()).toBe( - `${fileHeader}\n\n${otherDeploymentBlock}\n\n${deploymentBlock}`, - ); - expect(writeFileSpy).toHaveBeenCalledTimes(2); - expect(renameSpy).toHaveBeenCalledTimes(1); + // The include update reads right before merging, so it picks up content + // written after load(). + vol.writeFileSync(sshFilePath, "Host concurrent"); + await sshConfig.updateInclude(includeDir, hostname); + expect(await readConfig()).toBe(`${includeBlock}\n\nHost concurrent`); }); - it("retries an include conflict detected after rename", async () => { - const sshConfig = await loadSshConfig("Host initial"); + it("does not rewrite the file when the include is already in place", async () => { + const sshConfig = await loadSshConfig(`${includeBlock}\n\nHost *`); + const writeFileSpy = vi.spyOn(fsPromises, "writeFile"); const renameSpy = vi.spyOn(fsPromises, "rename"); - injectConcurrentChangeAfterRename( - `${otherIncludeBlock}\n\nHost concurrent\n\n${includeBlock}\n\n${legacyDeploymentBlock}`, - ); - await sshConfig.updateInclude(include, hostname); - expect(await readConfig()).toBe( - `${includeBlock}\n\n${otherIncludeBlock}\n\nHost concurrent`, - ); - expect(renameSpy).toHaveBeenCalledTimes(2); + await sshConfig.updateInclude(includeDir, hostname); + expect(writeFileSpy).not.toHaveBeenCalled(); + expect(renameSpy).not.toHaveBeenCalled(); }); +}); - it("fails after bounded optimistic retries", async () => { - vol.fromJSON({ [sshFilePath]: "Host initial" }); - const writeFileSpy = vi.spyOn(fsPromises, "writeFile"); - const renameSpy = vi.spyOn(fsPromises, "rename"); - let destinationReads = 0; - vi.spyOn(fsPromises, "readFile").mockImplementation((filePath, options) => { - if (filePath === sshFilePath && ++destinationReads > 1) { - vol.writeFileSync(sshFilePath, `Host revision-${destinationReads}`); - } - return realReadFile(filePath, options); +describe("cleanupStaleSshConfigs", () => { + it("removes only generated configs not written for a week", async () => { + const dir = "/Path/To/UserHomeDir/ssh"; + vol.fromJSON({ + [`${dir}/vscode--old.coder.com.conf`]: "stale", + [`${dir}/cursor--fresh.coder.com.conf`]: "fresh", + [`${dir}/unrelated.txt`]: "keep", }); - const sshConfig = new SshConfig(sshFilePath, mockLogger, fsPromises); - await sshConfig.load(); - await expect(sshConfig.updateInclude(include, hostname)).rejects.toThrow( - "because it kept changing", + const eightDaysAgo = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000); + vol.utimesSync( + `${dir}/vscode--old.coder.com.conf`, + eightDaysAgo, + eightDaysAgo, ); - expect(writeFileSpy).toHaveBeenCalledTimes(3); - expect(renameSpy).not.toHaveBeenCalled(); + + await cleanupStaleSshConfigs(dir, mockLogger); + + expect(Object.keys(vol.toJSON()).sort()).toEqual([ + `${dir}/cursor--fresh.coder.com.conf`, + `${dir}/unrelated.txt`, + ]); }); }); diff --git a/test/unit/util/authority.test.ts b/test/unit/util/authority.test.ts index bdbc0d45e..f4d7120c7 100644 --- a/test/unit/util/authority.test.ts +++ b/test/unit/util/authority.test.ts @@ -162,24 +162,18 @@ describe("authority construction", () => { ).toBe("ssh-remote+coder-vscode.dev.coder.com--foo--bar"); }); - interface CurrentHostPrefixCase { - hostname: string | undefined; - expected: string; - } - it.each([ - { hostname: undefined, expected: "coder-vscode-insiders--" }, - { - hostname: "dev.coder.com", - expected: "coder-vscode-insiders.dev.coder.com--", - }, - ])("formats current host prefix for $hostname", ({ hostname, expected }) => { + it("formats the current host prefix", () => { env.uriScheme = "vscode-insiders"; - expect(toCurrentAuthorityHostPrefix(hostname)).toBe(expected); + expect(toCurrentAuthorityHostPrefix("dev.coder.com")).toBe( + "coder-vscode-insiders.dev.coder.com--", + ); }); it("rejects an empty editor URI scheme at prefix construction", () => { env.uriScheme = ""; - expect(() => toCurrentAuthorityHostPrefix()).toThrow("must not be empty"); + expect(() => toCurrentAuthorityHostPrefix("dev.coder.com")).toThrow( + "must not be empty", + ); }); }); From 803d31ab5429458fe7e0c1202bf0fe89871cb19a Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 6 Aug 2026 16:18:16 +0300 Subject: [PATCH 7/8] refactor: simplify the SSH config plumbing - Derive the editor identity once in currentEditorId and use it for both authority prefixes and generated config file names, so the two encodings cannot drift and both reject an empty URI scheme. - Let pathResolver own the generated-file extension; the include glob and the stale-file sweep reuse it instead of repeating ".conf". - Drop the discarded parseRemoteAuthority call in retargetRemoteAuthority; both callers operate on already-parsed authorities, and without it isRemoteAuthorityCompatible no longer needs a try/catch. - Collapse the platform switch in getSshConfigDir to vary only the data root. - Deduplicate the "no SSH config generated" message in commands. --- src/commands.ts | 11 ++++----- src/core/pathResolver.ts | 52 +++++++++++++++++++--------------------- src/remote/sshConfig.ts | 5 ++-- src/util/authority.ts | 22 ++++++++--------- 4 files changed, 44 insertions(+), 46 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 4cce6b499..ff2ecba5a 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -87,6 +87,9 @@ import type { PongMessage, } from "./workspace/duplicateWorkspaceIpc"; +const NO_SSH_CONFIG_MESSAGE = + "No SSH config has been generated yet. It is written when you connect to a workspace."; + interface OpenOptions { workspaceOwner?: string; workspaceName?: string; @@ -578,9 +581,7 @@ export class Commands { try { await openFile(this.pathResolver.getSshConfigPath(hostname)); } catch { - vscode.window.showInformationMessage( - "No SSH config has been generated yet. It is written when you connect to a workspace.", - ); + vscode.window.showInformationMessage(NO_SSH_CONFIG_MESSAGE); return; } // The file is rewritten on every connection, so edits would be lost. @@ -608,9 +609,7 @@ export class Commands { .map((file) => this.pathResolver.parseSshConfigFile(file)) .filter((name) => name !== undefined); if (hostnames.length === 0) { - vscode.window.showInformationMessage( - "No SSH config has been generated yet. It is written when you connect to a workspace.", - ); + vscode.window.showInformationMessage(NO_SSH_CONFIG_MESSAGE); return undefined; } if (hostnames.length === 1) { diff --git a/src/core/pathResolver.ts b/src/core/pathResolver.ts index 4057d66cf..ddd862f41 100644 --- a/src/core/pathResolver.ts +++ b/src/core/pathResolver.ts @@ -3,6 +3,26 @@ import * as path from "node:path"; import * as vscode from "vscode"; import { expandPath } from "../util"; +import { currentEditorId } from "../util/authority"; + +/** Extension of generated SSH config files; the include glob matches on it. */ +export const SSH_CONFIG_EXT = ".conf"; + +/** The per-user data dir of the platform, shared by every editor. */ +function platformDataDir(): string { + switch (process.platform) { + case "win32": + return ( + process.env.APPDATA || path.join(os.homedir(), "AppData", "Roaming") + ); + case "darwin": + return path.join(os.homedir(), "Library", "Application Support"); + default: + return ( + process.env.XDG_DATA_HOME || path.join(os.homedir(), ".local", "share") + ); + } +} export class PathResolver { constructor( @@ -48,44 +68,22 @@ export class PathResolver { * Lives in the platform data dir so every editor emits the same include. */ public getSshConfigDir(): string { - switch (process.platform) { - case "win32": - return path.join( - process.env.APPDATA || path.join(os.homedir(), "AppData", "Roaming"), - "coder.coder-remote", - "ssh", - ); - case "darwin": - return path.join( - os.homedir(), - "Library", - "Application Support", - "coder.coder-remote", - "ssh", - ); - default: - return path.join( - process.env.XDG_DATA_HOME || - path.join(os.homedir(), ".local", "share"), - "coder.coder-remote", - "ssh", - ); - } + return path.join(platformDataDir(), "coder.coder-remote", "ssh"); } /** This editor's generated SSH config for one deployment. */ public getSshConfigPath(safeHostname: string): string { return path.join( this.getSshConfigDir(), - `${vscode.env.uriScheme}--${safeHostname}.conf`, + `${currentEditorId()}--${safeHostname}${SSH_CONFIG_EXT}`, ); } /** The deployment hostname if this editor generated the file, else undefined. */ public parseSshConfigFile(fileName: string): string | undefined { - const prefix = `${vscode.env.uriScheme}--`; - return fileName.startsWith(prefix) && fileName.endsWith(".conf") - ? fileName.slice(prefix.length, -".conf".length) + const prefix = `${currentEditorId()}--`; + return fileName.startsWith(prefix) && fileName.endsWith(SSH_CONFIG_EXT) + ? fileName.slice(prefix.length, -SSH_CONFIG_EXT.length) : undefined; } diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index 7479f3071..e162f06bf 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -9,6 +9,7 @@ import { import * as os from "node:os"; import path from "node:path"; +import { SSH_CONFIG_EXT } from "../core/pathResolver"; import { countSubstring, lowercase } from "../util"; import { cleanupFiles } from "../util/fileCleanup"; import { renameWithRetry, tempFilePath } from "../util/fs"; @@ -98,7 +99,7 @@ export async function cleanupStaleSshConfigs( ): Promise { await cleanupFiles(dir, logger, { label: "generated SSH config", - filter: (name) => name.endsWith(".conf"), + filter: (name) => name.endsWith(SSH_CONFIG_EXT), select: (files, now) => files.filter((file) => now - file.mtime > STALE_CONFIG_MAX_AGE_MS), }); @@ -410,7 +411,7 @@ export class SshConfig { return [ INCLUDE_MARKERS.start, "# Moves back to the top on connect; override options via coder.sshConfig.", - `Include "${this.escapeIncludePath(includeDir)}/*.conf"`, + `Include "${this.escapeIncludePath(includeDir)}/*${SSH_CONFIG_EXT}"`, INCLUDE_MARKERS.end, ].join("\n"); } diff --git a/src/util/authority.ts b/src/util/authority.ts index 035450d48..bcccabaf1 100644 --- a/src/util/authority.ts +++ b/src/util/authority.ts @@ -18,12 +18,17 @@ const sshRemotePrefix = "ssh-remote+"; const invalidAuthorityMessage = "Invalid Coder SSH authority. Must be: ----(.)"; -function currentAuthorityPrefix(): string { +/** This editor's identity, keeping its SSH hosts and files apart from other editors'. */ +export function currentEditorId(): string { const uriScheme = vscode.env.uriScheme; if (!uriScheme) { throw new Error("Editor URI scheme must not be empty."); } - return `coder-${uriScheme}`; + return uriScheme; +} + +function currentAuthorityPrefix(): string { + return `coder-${currentEditorId()}`; } function getSshHostStart(authority: string): number | undefined { @@ -155,7 +160,6 @@ export function retargetRemoteAuthority(authority: string): string { if (classifySshHost(sshHost) !== "legacy") { return authority; } - parseRemoteAuthority(authority); return `${authority.slice(0, sshHostStart)}${currentAuthorityPrefix()}${sshHost.slice(LegacyAuthorityPrefix.length)}`; } @@ -166,12 +170,8 @@ export function isRemoteAuthorityCompatible( if (!authority) { return false; } - if (authority === targetAuthority) { - return true; - } - try { - return retargetRemoteAuthority(authority) === targetAuthority; - } catch { - return false; - } + return ( + authority === targetAuthority || + retargetRemoteAuthority(authority) === targetAuthority + ); } From 3a576887d86197799974f7b9171d649b826df602 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 6 Aug 2026 16:47:12 +0300 Subject: [PATCH 8/8] fix: read the remote authority through the proposed API module env.remoteAuthority is part of the resolvers API proposal, which our own vscode module is not granted, so reading it in pickSshHostname threw "CANNOT use API proposal: resolvers" and the Open Generated SSH Configuration File command failed. Read it via vscodeProposed like every other call site, and fall through to the deployment picker when the proposed API is unavailable. A lint rule now rejects plain vscode.env.remoteAuthority so the mistake cannot come back; the test doubles cannot catch it because both modules resolve to the same mock. --- eslint.config.mjs | 6 ++++++ src/commands.ts | 11 ++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/eslint.config.mjs b/eslint.config.mjs index 5ea6d07ab..062201e43 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -123,6 +123,12 @@ export default defineConfig( message: "Do not use registerCommand('coder.*', ...) directly. Use the CommandManager class instead.", }, + { + selector: + "MemberExpression[property.name='remoteAuthority'][object.property.name='env'][object.object.name='vscode']", + message: + "env.remoteAuthority is a proposed API (resolvers) and throws through our own vscode module. Read it via vscodeProposed.env.remoteAuthority.", + }, ], }, }, diff --git a/src/commands.ts b/src/commands.ts index ff2ecba5a..4bab9e9fa 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -592,16 +592,17 @@ export class Commands { /** A connected window resolves to its own deployment; otherwise ask. */ private async pickSshHostname(): Promise { - const remoteAuthority = vscode.env.remoteAuthority; - if (remoteAuthority) { - try { + try { + // remoteAuthority is a proposed API; our own vscode module may not read it. + const remoteAuthority = vscodeProposed.env.remoteAuthority; + if (remoteAuthority) { const parts = parseRemoteAuthority(remoteAuthority); if (parts) { return parts.safeHostname; } - } catch { - // Malformed Coder authority; fall through to the picker. } + } catch { + // Malformed Coder authority or unavailable API; fall through to the picker. } const hostnames = ( await readdirOrEmpty(this.pathResolver.getSshConfigDir())