Fix SetEnv SSH config parsing and accumulation (#684)

EhabY · web-flow · commit e1099b7932d5 · 2025-12-22T12:07:17.000+03:00
- Add `parseSshConfig` function to correctly parse `SetEnv MY_VAR=value` (previously split on first `=` producing incorrect key/value) - Modify `mergeSshConfigValues` to concatenate `SetEnv` values instead of replacing, preserving the default `CODER_SSH_SESSION_TYPE` - Ignore empty `SetEnv` values to prevent accidentally clearing defaults Closes #559
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Fixed
+
+- Fixed `SetEnv` SSH config parsing and accumulation with user-defined values.
+
 ## [v1.11.6](https://github.com/coder/vscode-coder/releases/tag/v1.11.6) 2025-12-15
 
 ### Added
diff --git a/package.json b/package.json
@@ -36,7 +36,7 @@
 			"title": "Coder",
 			"properties": {
 				"coder.sshConfig": {
-					"markdownDescription": "These values will be included in the ssh config file. Eg: `'ConnectTimeout=10'` will set the timeout to 10 seconds. Any values included here will override anything provided by default or by the deployment. To unset a value that is written by default, set the value to the empty string, Eg: `'ConnectTimeout='` will unset it.",
+					"markdownDescription": "These values will be included in the ssh config file. Eg: `'ConnectTimeout=10'` will set the timeout to 10 seconds. Any values included here will override anything provided by default or by the deployment. To unset a value that is written by default, set the value to the empty string, Eg: `'ConnectTimeout='` will unset it.\n\nNote: `SetEnv` values are accumulated across multiple entries and cannot be unset.",
 					"type": "array",
 					"items": {
 						"title": "SSH Config Value",
diff --git a/src/remote/remote.ts b/src/remote/remote.ts
@@ -42,7 +42,12 @@ import {
 } from "../util";
 import { WorkspaceMonitor } from "../workspace/workspaceMonitor";
 
-import { SSHConfig, type SSHValues, mergeSSHConfigValues } from "./sshConfig";
+import {
+	SSHConfig,
+	type SSHValues,
+	mergeSshConfigValues,
+	parseSshConfig,
+} from "./sshConfig";
 import { SshProcessMonitor } from "./sshProcess";
 import { computeSSHProperties, sshSupportsSetEnv } from "./sshSupport";
 import { WorkspaceStateMachine } from "./workspaceStateMachine";
@@ -754,29 +759,11 @@ export class Remote {
 
 		// deploymentConfig is now set from the remote coderd deployment.
 		// Now override with the user's config.
-		const userConfigSSH =
+		const userConfigSsh =
 			vscode.workspace.getConfiguration("coder").get<string[]>("sshConfig") ||
 			[];
-		// Parse the user's config into a Record<string, string>.
-		const userConfig = userConfigSSH.reduce(
-			(acc, line) => {
-				let i = line.indexOf("=");
-				if (i === -1) {
-					i = line.indexOf(" ");
-					if (i === -1) {
-						// This line is malformed. The setting is incorrect, and does not match
-						// the pattern regex in the settings schema.
-						return acc;
-					}
-				}
-				const key = line.slice(0, i);
-				const value = line.slice(i + 1);
-				acc[key] = value;
-				return acc;
-			},
-			{} as Record<string, string>,
-		);
-		const sshConfigOverrides = mergeSSHConfigValues(
+		const userConfig = parseSshConfig(userConfigSsh);
+		const sshConfigOverrides = mergeSshConfigValues(
 			deploymentSSHConfig,
 			userConfig,
 		);
@@ -819,7 +806,7 @@ export class Remote {
 		if (sshSupportsSetEnv()) {
 			// This allows for tracking the number of extension
 			// users connected to workspaces!
-			sshValues.SetEnv = " CODER_SSH_SESSION_TYPE=vscode";
+			sshValues.SetEnv = "CODER_SSH_SESSION_TYPE=vscode";
 		}
 
 		await sshConfig.update(safeHostname, sshValues, sshConfigOverrides);
diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts
@@ -36,10 +36,48 @@ const defaultFileSystem: FileSystem = {
 	writeFile,
 };
 
+/**
+ * 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.
+ */
+export function parseSshConfig(lines: string[]): Record<string, string> {
+	return lines.reduce(
+		(acc, line) => {
+			// Match key pattern (same as VS Code settings: ^[a-zA-Z0-9-]+)
+			const keyMatch = line.match(/^[a-zA-Z0-9-]+/);
+			if (!keyMatch) {
+				return acc; // Malformed line
+			}
+
+			const key = keyMatch[0];
+			const separator = line.at(key.length);
+			if (separator !== "=" && separator !== " ") {
+				return acc; // Malformed line
+			}
+
+			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;
+				}
+			} else {
+				acc[key] = value;
+			}
+			return acc;
+		},
+		{} as Record<string, string>,
+	);
+}
+
 // mergeSSHConfigValues will take a given ssh config and merge it with the overrides
 // provided. The merge handles key case insensitivity, so casing in the "key" does
 // not matter.
-export function mergeSSHConfigValues(
+export function mergeSshConfigValues(
 	config: Record<string, string>,
 	overrides: Record<string, string>,
 ): Record<string, string> {
@@ -62,11 +100,21 @@ export function mergeSSHConfigValues(
 			const value = overrides[correctCaseKey];
 			delete caseInsensitiveOverrides[lower];
 
-			// If the value is empty, do not add the key. It is being removed.
-			if (value === "") {
+			// Special handling for SetEnv - concatenate values instead of replacing.
+			if (lower === "setenv") {
+				if (value === "") {
+					merged["SetEnv"] = config[key];
+				} else {
+					merged["SetEnv"] = `${config[key]} ${value}`;
+				}
 				return;
 			}
-			merged[correctCaseKey] = value;
+
+			// If the value is empty, do not add the key. It is being removed.
+			if (value !== "") {
+				merged[correctCaseKey] = value;
+			}
+
 			return;
 		}
 		// If no override, take the original value.
@@ -78,7 +126,14 @@ export function mergeSSHConfigValues(
 	// Add remaining overrides.
 	Object.keys(caseInsensitiveOverrides).forEach((lower) => {
 		const correctCaseKey = caseInsensitiveOverrides[lower];
-		merged[correctCaseKey] = overrides[correctCaseKey];
+		const value = overrides[correctCaseKey];
+
+		// Special handling for SetEnv - concatenate if already exists
+		if (lower === "setenv" && merged["SetEnv"]) {
+			merged["SetEnv"] = `${merged["SetEnv"]} ${value}`;
+		} else {
+			merged[correctCaseKey] = value;
+		}
 	});
 
 	return merged;
@@ -203,7 +258,7 @@ export class SSHConfig {
 		const lines = [this.startBlockComment(safeHostname), `Host ${Host}`];
 
 		// configValues is the merged values of the defaults and the overrides.
-		const configValues = mergeSSHConfigValues(otherValues, overrides || {});
+		const configValues = mergeSshConfigValues(otherValues, overrides || {});
 
 		// keys is the sorted keys of the merged values.
 		const keys = (
diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts
@@ -1,6 +1,10 @@
-import { it, afterEach, vi, expect } from "vitest";
+import { it, afterEach, vi, expect, describe } from "vitest";
 
-import { SSHConfig } from "@/remote/sshConfig";
+import {
+	SSHConfig,
+	parseSshConfig,
+	mergeSshConfigValues,
+} from "@/remote/sshConfig";
 
 // This is not the usual path to ~/.ssh/config, but
 // setting it to a different path makes it easier to test
@@ -712,3 +716,140 @@ it("fails if we are unable to rename the temporary file", async () => {
 		}),
 	).rejects.toThrow(/Failed to rename temporary SSH config file.*EACCES/);
 });
+
+describe("parseSshConfig", () => {
+	type ParseTest = {
+		name: string;
+		input: string[];
+		expected: Record<string, string>;
+	};
+
+	it.each<ParseTest>([
+		{
+			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", () => {
+	type MergeTest = {
+		name: string;
+		config: Record<string, string>;
+		overrides: Record<string, string>;
+		expected: Record<string, string>;
+	};
+
+	it.each<MergeTest>([
+		{
+			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);
+	});
+});
