From eda19a23248ac158cff85382b583f29c055779a7 Mon Sep 17 00:00:00 2001
From: Cian Johnston <cian@coder.com>
Date: Tue, 20 May 2025 17:21:27 +0100
Subject: [PATCH 1/8] fix: SSHConfig: check for multiple start/end blocks

---
 src/sshConfig.test.ts | 183 ++++++++++++++++++++++++++++++++++++++++++
 src/sshConfig.ts      |  22 ++++-
 src/util.ts           |  19 +++++
 3 files changed, 221 insertions(+), 3 deletions(-)

diff --git a/src/sshConfig.test.ts b/src/sshConfig.test.ts
index 03b73fab..88007b7b 100644
--- a/src/sshConfig.test.ts
+++ b/src/sshConfig.test.ts
@@ -249,6 +249,189 @@ Host coder-vscode.dev.coder.com--*
   })
 })
 
+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, mockFileSystem)
+  mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  await sshConfig.load()
+
+  // When we try to update the config, it should throw an error.
+  await expect(
+    sshConfig.update("dev.coder.com", {
+      Host: "coder-vscode.dev.coder.com--*",
+      ProxyCommand: "some-command-here",
+      ConnectTimeout: "0",
+      StrictHostKeyChecking: "no",
+      UserKnownHostsFile: "/dev/null",
+      LogLevel: "ERROR",
+    }),
+  ).rejects.toThrow(
+    `Malformed config: ssh config has 2 dev.coder.com START comments but 1 dev.coder.com END comments. Each START must have a matching END.`,
+  )
+})
+
+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
+
+# --- 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, mockFileSystem)
+  mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  await sshConfig.load()
+
+  // When we try to update the config, it should throw an error.
+  await expect(
+    sshConfig.update("dev.coder.com", {
+      Host: "coder-vscode.dev.coder.com--*",
+      ProxyCommand: "some-command-here",
+      ConnectTimeout: "0",
+      StrictHostKeyChecking: "no",
+      UserKnownHostsFile: "/dev/null",
+      LogLevel: "ERROR",
+    }),
+  ).rejects.toThrow(`Malformed config: ssh config has 2 dev.coder.com sections, please remove all but one.`)
+})
+
+it("correctly handles interspersed blocks with and without label", async () => {
+  const existentSSHConfig = `Host beforeconfig
+  HostName before.config.tld
+  User before
+
+# --- 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 ---
+
+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, mockFileSystem)
+  mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  await sshConfig.load()
+
+  const expectedOutput = `Host beforeconfig
+  HostName before.config.tld
+  User before
+
+# --- 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 ---
+
+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`
+
+  await sshConfig.update("dev.coder.com", {
+    Host: "coder-vscode.dev.coder.com--*",
+    ProxyCommand: "some-command-here",
+    ConnectTimeout: "0",
+    StrictHostKeyChecking: "no",
+    UserKnownHostsFile: "/dev/null",
+    LogLevel: "ERROR",
+  })
+
+  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
+    encoding: "utf-8",
+    mode: 384,
+  })
+})
+
 it("override values", async () => {
   mockFileSystem.readFile.mockRejectedValueOnce("No file found")
   const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
diff --git a/src/sshConfig.ts b/src/sshConfig.ts
index 133ed6a4..5ce35c9a 100644
--- a/src/sshConfig.ts
+++ b/src/sshConfig.ts
@@ -1,5 +1,6 @@
 import { mkdir, readFile, writeFile } from "fs/promises"
 import path from "path"
+import { countSubstring } from "./util"
 
 class SSHConfigBadFormat extends Error {}
 
@@ -123,14 +124,29 @@ export class SSHConfig {
    */
   private getBlock(label: string): Block | undefined {
     const raw = this.getRaw()
-    const startBlockIndex = raw.indexOf(this.startBlockComment(label))
-    const endBlockIndex = raw.indexOf(this.endBlockComment(label))
+    const startBlock = this.startBlockComment(label)
+    const endBlock = this.endBlockComment(label)
+    const startBlockCount = countSubstring(startBlock, raw)
+    const endBlockCount = countSubstring(endBlock, raw)
+    const startBlockIndex = raw.indexOf(startBlock)
+    const endBlockIndex = raw.indexOf(endBlock)
     const hasBlock = startBlockIndex > -1 && endBlockIndex > -1
 
     if (!hasBlock) {
       return
     }
 
+    if (startBlockCount !== endBlockCount) {
+      throw new SSHConfigBadFormat(
+        `Malformed config: ssh config has ${startBlockCount} ${label} START comments but ${endBlockCount} ${label} END comments. Each START must have a matching END.`,
+      )
+    }
+    if (startBlockCount > 1 || endBlockCount > 1) {
+      throw new SSHConfigBadFormat(
+        `Malformed config: ssh config has ${startBlockCount} ${label} sections, please remove all but one.`,
+      )
+    }
+
     if (startBlockIndex === -1) {
       throw new SSHConfigBadFormat("Start block not found")
     }
@@ -144,7 +160,7 @@ export class SSHConfig {
     }
 
     return {
-      raw: raw.substring(startBlockIndex, endBlockIndex + this.endBlockComment(label).length),
+      raw: raw.substring(startBlockIndex, endBlockIndex + endBlock.length),
     }
   }
 
diff --git a/src/util.ts b/src/util.ts
index 87707210..1b67e0ba 100644
--- a/src/util.ts
+++ b/src/util.ts
@@ -120,3 +120,22 @@ export function expandPath(input: string): string {
   const userHome = os.homedir()
   return input.replace(/\${userHome}/g, userHome)
 }
+
+/**
+ * Return the number of times a substring appears in a string.
+ * @param needle string
+ * @param haystack string
+ * @returns number
+ */
+export function countSubstring(needle: string, haystack: string): number {
+  if (needle.length < 1 || haystack.length < 1) {
+    return 0
+  }
+  let count = 0
+  let pos = haystack.indexOf(needle)
+  while (pos !== -1) {
+    count++
+    pos = haystack.indexOf(needle, pos + needle.length)
+  }
+  return count
+}

From d5dfbfc968c8184fad8d5df50cfe66acf4eda9e5 Mon Sep 17 00:00:00 2001
From: Cian Johnston <cian@coder.com>
Date: Wed, 21 May 2025 10:46:12 +0100
Subject: [PATCH 2/8] remove type comments

---
 src/util.ts | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/util.ts b/src/util.ts
index 1b67e0ba..edcf56ec 100644
--- a/src/util.ts
+++ b/src/util.ts
@@ -123,9 +123,6 @@ export function expandPath(input: string): string {
 
 /**
  * Return the number of times a substring appears in a string.
- * @param needle string
- * @param haystack string
- * @returns number
  */
 export function countSubstring(needle: string, haystack: string): number {
   if (needle.length < 1 || haystack.length < 1) {

From 752a2a86d85bb2d149f675e16b73f28297e1fd62 Mon Sep 17 00:00:00 2001
From: Cian Johnston <cian@coder.com>
Date: Wed, 21 May 2025 10:47:44 +0100
Subject: [PATCH 3/8] add tests for countSubstring

---
 src/util.test.ts | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/util.test.ts b/src/util.test.ts
index 4fffcc75..0c5da63a 100644
--- a/src/util.test.ts
+++ b/src/util.test.ts
@@ -1,5 +1,5 @@
-import { it, expect } from "vitest"
-import { parseRemoteAuthority, toSafeHost } from "./util"
+import { describe, it, expect } from "vitest"
+import { countSubstring, parseRemoteAuthority, toSafeHost } from "./util"
 
 it("ignore unrelated authorities", async () => {
   const tests = [
@@ -73,3 +73,35 @@ it("escapes url host", async () => {
   expect(() => toSafeHost("invalid url")).toThrow("Invalid URL")
   expect(toSafeHost("http://ignore-port.com:8080/")).toBe("ignore-port.com")
 })
+
+describe("countSubstring", () => {
+  it("handles empty strings", () => {
+    expect(countSubstring("", "")).toBe(0)
+    expect(countSubstring("foo", "")).toBe(0)
+    expect(countSubstring("", "foo")).toBe(0)
+  })
+
+  it("handles single character", () => {
+    expect(countSubstring("a", "a")).toBe(1)
+    expect(countSubstring("a", "b")).toBe(0)
+    expect(countSubstring("a", "aa")).toBe(2)
+    expect(countSubstring("a", "aaa")).toBe(3)
+    expect(countSubstring("a", "baaa")).toBe(3)
+  })
+
+  it("handles multiple characters", () => {
+    expect(countSubstring("foo", "foo")).toBe(1)
+    expect(countSubstring("foo", "bar")).toBe(0)
+    expect(countSubstring("foo", "foobar")).toBe(1)
+    expect(countSubstring("foo", "foobarbaz")).toBe(1)
+    expect(countSubstring("foo", "foobarbazfoo")).toBe(2)
+    expect(countSubstring("foo", "foobarbazfoof")).toBe(2)
+  })
+
+  it("does not handle overlapping substrings", () => {
+    expect(countSubstring("aa", "aaa")).toBe(1)
+    expect(countSubstring("aa", "aaaa")).toBe(2)
+    expect(countSubstring("aa", "aaaaa")).toBe(2)
+    expect(countSubstring("aa", "aaaaaa")).toBe(3)
+  })
+})

From 572a9380dcc37d682e727d4b09f819b71b7d0071 Mon Sep 17 00:00:00 2001
From: Cian Johnston <cian@coder.com>
Date: Wed, 21 May 2025 10:56:00 +0100
Subject: [PATCH 4/8] adjust error mesages to provide more useful output

---
 src/sshConfig.test.ts | 55 +++++++++++++++++++++++++++++++++++++++++--
 src/sshConfig.ts      |  4 ++--
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/src/sshConfig.test.ts b/src/sshConfig.test.ts
index 88007b7b..04f4d467 100644
--- a/src/sshConfig.test.ts
+++ b/src/sshConfig.test.ts
@@ -300,10 +300,59 @@ Host afterconfig
       LogLevel: "ERROR",
     }),
   ).rejects.toThrow(
-    `Malformed config: ssh config has 2 dev.coder.com START comments but 1 dev.coder.com END comments. Each START must have a matching END.`,
+    `Malformed config: Unterminated START CODER VSCODE dev.coder.com block: Each START block must have an END block.`,
   )
 })
 
+it("throws an error if there is a mismatched start and end block count (without label)", async () => {
+  // As above, but without a label.
+  const existentSSHConfig = `Host beforeconfig
+  HostName before.config.tld
+  User before
+
+# --- START CODER VSCODE ---
+Host coder-vscode.dev.coder.com--*
+  ConnectTimeout 0
+  LogLevel ERROR
+  ProxyCommand some-command-here
+  StrictHostKeyChecking no
+  UserKnownHostsFile /dev/null
+# missing END CODER VSCODE ---
+
+Host donotdelete
+  HostName dont.delete.me
+  User please
+
+# --- 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 ---
+
+Host afterconfig
+  HostName after.config.tld
+  User after`
+
+  const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
+  mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  await sshConfig.load()
+
+  // When we try to update the config, it should throw an error.
+  await expect(
+    sshConfig.update("", {
+      Host: "coder-vscode.dev.coder.com--*",
+      ProxyCommand: "some-command-here",
+      ConnectTimeout: "0",
+      StrictHostKeyChecking: "no",
+      UserKnownHostsFile: "/dev/null",
+      LogLevel: "ERROR",
+    }),
+  ).rejects.toThrow(`Malformed config: Unterminated START CODER VSCODE block: Each START block must have an END block.`)
+})
+
 it("throws an error if there are more than one sections with the same label", async () => {
   const existentSSHConfig = `Host beforeconfig
   HostName before.config.tld
@@ -349,7 +398,9 @@ Host afterconfig
       UserKnownHostsFile: "/dev/null",
       LogLevel: "ERROR",
     }),
-  ).rejects.toThrow(`Malformed config: ssh config has 2 dev.coder.com sections, please remove all but one.`)
+  ).rejects.toThrow(
+    `Malformed config: ssh config has 2 START CODER VSCODE dev.coder.com sections, please remove all but one.`,
+  )
 })
 
 it("correctly handles interspersed blocks with and without label", async () => {
diff --git a/src/sshConfig.ts b/src/sshConfig.ts
index 5ce35c9a..85750d7c 100644
--- a/src/sshConfig.ts
+++ b/src/sshConfig.ts
@@ -138,12 +138,12 @@ export class SSHConfig {
 
     if (startBlockCount !== endBlockCount) {
       throw new SSHConfigBadFormat(
-        `Malformed config: ssh config has ${startBlockCount} ${label} START comments but ${endBlockCount} ${label} END comments. Each START must have a matching END.`,
+        `Malformed config: Unterminated START CODER VSCODE ${label ? label + " " : ""}block: Each START block must have an END block.`,
       )
     }
     if (startBlockCount > 1 || endBlockCount > 1) {
       throw new SSHConfigBadFormat(
-        `Malformed config: ssh config has ${startBlockCount} ${label} sections, please remove all but one.`,
+        `Malformed config: ssh config has ${startBlockCount} START CODER VSCODE ${label ? label + " " : ""}sections, please remove all but one.`,
       )
     }
 

From bcebbd4771de09cf742eeaf3c9c645d28a9c0ab2 Mon Sep 17 00:00:00 2001
From: Cian Johnston <cian@coder.com>
Date: Wed, 21 May 2025 17:06:50 +0100
Subject: [PATCH 5/8] move malformed block check earlier

---
 src/sshConfig.test.ts | 38 ++++++++++++++++++++++++++++++++++++++
 src/sshConfig.ts      | 17 +++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/src/sshConfig.test.ts b/src/sshConfig.test.ts
index 04f4d467..23a0205c 100644
--- a/src/sshConfig.test.ts
+++ b/src/sshConfig.test.ts
@@ -249,6 +249,44 @@ Host coder-vscode.dev.coder.com--*
   })
 })
 
+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
+
+Host afterconfig
+  HostName after.config.tld
+  User after`
+
+  const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
+  mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  await sshConfig.load()
+
+  // When we try to update the config, it should throw an error.
+  await expect(
+    sshConfig.update("dev.coder.com", {
+      Host: "coder-vscode.dev.coder.com--*",
+      ProxyCommand: "some-command-here",
+      ConnectTimeout: "0",
+      StrictHostKeyChecking: "no",
+      UserKnownHostsFile: "/dev/null",
+      LogLevel: "ERROR",
+    }),
+  ).rejects.toThrow(
+    `Malformed config: Unterminated START CODER VSCODE dev.coder.com block: Each START block must have an END block.`,
+  )
+})
+
 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.
diff --git a/src/sshConfig.ts b/src/sshConfig.ts
index 85750d7c..14384f5e 100644
--- a/src/sshConfig.ts
+++ b/src/sshConfig.ts
@@ -126,27 +126,28 @@ export class SSHConfig {
     const raw = this.getRaw()
     const startBlock = this.startBlockComment(label)
     const endBlock = this.endBlockComment(label)
+
     const startBlockCount = countSubstring(startBlock, raw)
     const endBlockCount = countSubstring(endBlock, raw)
-    const startBlockIndex = raw.indexOf(startBlock)
-    const endBlockIndex = raw.indexOf(endBlock)
-    const hasBlock = startBlockIndex > -1 && endBlockIndex > -1
-
-    if (!hasBlock) {
-      return
-    }
-
     if (startBlockCount !== endBlockCount) {
       throw new SSHConfigBadFormat(
         `Malformed config: Unterminated START CODER VSCODE ${label ? label + " " : ""}block: Each START block must have an END block.`,
       )
     }
+
     if (startBlockCount > 1 || endBlockCount > 1) {
       throw new SSHConfigBadFormat(
         `Malformed config: ssh config has ${startBlockCount} START CODER VSCODE ${label ? label + " " : ""}sections, please remove all but one.`,
       )
     }
 
+    const startBlockIndex = raw.indexOf(startBlock)
+    const endBlockIndex = raw.indexOf(endBlock)
+    const hasBlock = startBlockIndex > -1 && endBlockIndex > -1
+    if (!hasBlock) {
+      return
+    }
+
     if (startBlockIndex === -1) {
       throw new SSHConfigBadFormat("Start block not found")
     }

From a9265beaac0d85ec529faa3f3114dcb2fc579454 Mon Sep 17 00:00:00 2001
From: Cian Johnston <cian@coder.com>
Date: Wed, 21 May 2025 17:52:31 +0100
Subject: [PATCH 6/8] add config file path to message

---
 src/sshConfig.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/sshConfig.ts b/src/sshConfig.ts
index 14384f5e..a949cdcc 100644
--- a/src/sshConfig.ts
+++ b/src/sshConfig.ts
@@ -131,13 +131,13 @@ export class SSHConfig {
     const endBlockCount = countSubstring(endBlock, raw)
     if (startBlockCount !== endBlockCount) {
       throw new SSHConfigBadFormat(
-        `Malformed config: Unterminated START CODER VSCODE ${label ? label + " " : ""}block: Each START block must have an END block.`,
+        `Malformed config: ${this.filePath} has an unterminated START CODER VSCODE ${label ? label + " " : ""}block. Each START block must have an END block.`,
       )
     }
 
     if (startBlockCount > 1 || endBlockCount > 1) {
       throw new SSHConfigBadFormat(
-        `Malformed config: ssh config has ${startBlockCount} START CODER VSCODE ${label ? label + " " : ""}sections, please remove all but one.`,
+        `Malformed config: ${this.filePath} has ${startBlockCount} START CODER VSCODE ${label ? label + " " : ""}sections. Please remove all but one.`,
       )
     }
 

From e65ee216dd8131faf6d7ddd41e6568b50cd9e3e6 Mon Sep 17 00:00:00 2001
From: Cian Johnston <cian@coder.com>
Date: Wed, 21 May 2025 18:03:07 +0100
Subject: [PATCH 7/8] fixup! add config file path to message

---
 src/sshConfig.test.ts | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/sshConfig.test.ts b/src/sshConfig.test.ts
index 23a0205c..1a6a081d 100644
--- a/src/sshConfig.test.ts
+++ b/src/sshConfig.test.ts
@@ -283,7 +283,7 @@ Host afterconfig
       LogLevel: "ERROR",
     }),
   ).rejects.toThrow(
-    `Malformed config: Unterminated START CODER VSCODE dev.coder.com block: Each START block must have an END block.`,
+    `Malformed config: ${sshFilePath} has an unterminated START CODER VSCODE dev.coder.com block. Each START block must have an END block.`,
   )
 })
 
@@ -338,7 +338,7 @@ Host afterconfig
       LogLevel: "ERROR",
     }),
   ).rejects.toThrow(
-    `Malformed config: Unterminated START CODER VSCODE dev.coder.com block: Each START block must have an END block.`,
+    `Malformed config: ${sshFilePath} has an unterminated START CODER VSCODE dev.coder.com block. Each START block must have an END block.`,
   )
 })
 
@@ -388,7 +388,9 @@ Host afterconfig
       UserKnownHostsFile: "/dev/null",
       LogLevel: "ERROR",
     }),
-  ).rejects.toThrow(`Malformed config: Unterminated START CODER VSCODE block: Each START block must have an END block.`)
+  ).rejects.toThrow(
+    `Malformed config: ${sshFilePath} has an unterminated START CODER VSCODE block. Each START block must have an END block.`,
+  )
 })
 
 it("throws an error if there are more than one sections with the same label", async () => {
@@ -437,7 +439,7 @@ Host afterconfig
       LogLevel: "ERROR",
     }),
   ).rejects.toThrow(
-    `Malformed config: ssh config has 2 START CODER VSCODE dev.coder.com sections, please remove all but one.`,
+    `Malformed config: ${sshFilePath} has 2 START CODER VSCODE dev.coder.com sections. Please remove all but one.`,
   )
 })
 

From ca32d994f1efdab673c0d55ad628f92a7bd671a5 Mon Sep 17 00:00:00 2001
From: Cian Johnston <public@cianjohnston.ie>
Date: Fri, 23 May 2025 01:45:32 +0100
Subject: [PATCH 8/8] fix: SSHConfig: atomically write ssh config (#511)

---
 src/sshConfig.test.ts | 123 ++++++++++++++++++++++++++++++++++++------
 src/sshConfig.ts      |  49 ++++++++++++++---
 2 files changed, 150 insertions(+), 22 deletions(-)

diff --git a/src/sshConfig.test.ts b/src/sshConfig.test.ts
index 1a6a081d..d4a8e41d 100644
--- a/src/sshConfig.test.ts
+++ b/src/sshConfig.test.ts
@@ -2,11 +2,17 @@
 import { it, afterEach, vi, expect } from "vitest"
 import { SSHConfig } from "./sshConfig"
 
-const sshFilePath = "~/.config/ssh"
+// 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 sshTempFilePathExpr = `^/Path/To/UserHomeDir/\\.sshConfigDir/\\.sshConfigFile\\.vscode-coder-tmp\\.[a-z0-9]+$`
 
 const mockFileSystem = {
-  readFile: vi.fn(),
   mkdir: vi.fn(),
+  readFile: vi.fn(),
+  rename: vi.fn(),
+  stat: vi.fn(),
   writeFile: vi.fn(),
 }
 
@@ -16,6 +22,7 @@ afterEach(() => {
 
 it("creates a new file and adds config with empty label", async () => {
   mockFileSystem.readFile.mockRejectedValueOnce("No file found")
+  mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" })
 
   const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
   await sshConfig.load()
@@ -38,11 +45,20 @@ Host coder-vscode--*
 # --- END CODER VSCODE ---`
 
   expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
-  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
+  expect(mockFileSystem.writeFile).toBeCalledWith(
+    expect.stringMatching(sshTempFilePathExpr),
+    expectedOutput,
+    expect.objectContaining({
+      encoding: "utf-8",
+      mode: 0o600, // Default mode for new files.
+    }),
+  )
+  expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
 })
 
 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, mockFileSystem)
   await sshConfig.load()
@@ -65,7 +81,15 @@ Host coder-vscode.dev.coder.com--*
 # --- END CODER VSCODE dev.coder.com ---`
 
   expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
-  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
+  expect(mockFileSystem.writeFile).toBeCalledWith(
+    expect.stringMatching(sshTempFilePathExpr),
+    expectedOutput,
+    expect.objectContaining({
+      encoding: "utf-8",
+      mode: 0o600, // Default mode for new files.
+    }),
+  )
+  expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
 })
 
 it("adds a new coder config in an existent SSH configuration", async () => {
@@ -77,6 +101,7 @@ it("adds a new coder config in an existent SSH configuration", async () => {
   StrictHostKeyChecking=no
   UserKnownHostsFile=/dev/null`
   mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
 
   const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
   await sshConfig.load()
@@ -100,10 +125,11 @@ Host coder-vscode.dev.coder.com--*
   UserKnownHostsFile /dev/null
 # --- END CODER VSCODE dev.coder.com ---`
 
-  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
+  expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
     encoding: "utf-8",
-    mode: 384,
+    mode: 0o644,
   })
+  expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
 })
 
 it("updates an existent coder config", async () => {
@@ -138,6 +164,7 @@ Host coder-vscode.dev.coder.com--*
 Host *
   SetEnv TEST=1`
   mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
 
   const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
   await sshConfig.load()
@@ -164,10 +191,11 @@ Host coder-vscode.dev-updated.coder.com--*
 Host *
   SetEnv TEST=1`
 
-  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
+  expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
     encoding: "utf-8",
-    mode: 384,
+    mode: 0o644,
   })
+  expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
 })
 
 it("does not remove deployment-unaware SSH config and adds the new one", async () => {
@@ -186,6 +214,7 @@ Host coder-vscode--*
   UserKnownHostsFile=/dev/null
 # --- END CODER VSCODE ---`
   mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
 
   const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
   await sshConfig.load()
@@ -209,16 +238,18 @@ Host coder-vscode.dev.coder.com--*
   UserKnownHostsFile /dev/null
 # --- END CODER VSCODE dev.coder.com ---`
 
-  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
+  expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
     encoding: "utf-8",
-    mode: 384,
+    mode: 0o644,
   })
+  expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), 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 })
 
   const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
   await sshConfig.load()
@@ -243,10 +274,11 @@ Host coder-vscode.dev.coder.com--*
   UserKnownHostsFile /dev/null
 # --- END CODER VSCODE dev.coder.com ---`
 
-  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
+  expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
     encoding: "utf-8",
-    mode: 384,
+    mode: 0o644,
   })
+  expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
 })
 
 it("throws an error if there is a missing end block", async () => {
@@ -476,6 +508,7 @@ Host afterconfig
 
   const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
   mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
   await sshConfig.load()
 
   const expectedOutput = `Host beforeconfig
@@ -517,14 +550,17 @@ Host afterconfig
     LogLevel: "ERROR",
   })
 
-  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
+  expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
     encoding: "utf-8",
-    mode: 384,
+    mode: 0o644,
   })
+  expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
 })
 
 it("override values", async () => {
   mockFileSystem.readFile.mockRejectedValueOnce("No file found")
+  mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" })
+
   const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
   await sshConfig.load()
   await sshConfig.update(
@@ -561,5 +597,62 @@ Host coder-vscode.dev.coder.com--*
 # --- END CODER VSCODE dev.coder.com ---`
 
   expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
-  expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
+  expect(mockFileSystem.writeFile).toBeCalledWith(
+    expect.stringMatching(sshTempFilePathExpr),
+    expectedOutput,
+    expect.objectContaining({
+      encoding: "utf-8",
+      mode: 0o600, // Default mode for new files.
+    }),
+  )
+  expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
+})
+
+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, mockFileSystem)
+  mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 })
+  mockFileSystem.writeFile.mockRejectedValueOnce(new Error("EACCES"))
+
+  await sshConfig.load()
+
+  expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
+  await expect(
+    sshConfig.update("dev.coder.com", {
+      Host: "coder-vscode.dev.coder.com--*",
+      ProxyCommand: "some-command-here",
+      ConnectTimeout: "0",
+      StrictHostKeyChecking: "no",
+      UserKnownHostsFile: "/dev/null",
+      LogLevel: "ERROR",
+    }),
+  ).rejects.toThrow(/Failed to write temporary SSH config file.*EACCES/)
+})
+
+it("fails if we are unable to rename the temporary file", async () => {
+  const existentSSHConfig = `Host beforeconfig
+  HostName before.config.tld
+  User before`
+
+  const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
+  mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
+  mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 })
+  mockFileSystem.writeFile.mockResolvedValueOnce("")
+  mockFileSystem.rename.mockRejectedValueOnce(new Error("EACCES"))
+
+  await sshConfig.load()
+  await expect(
+    sshConfig.update("dev.coder.com", {
+      Host: "coder-vscode.dev.coder.com--*",
+      ProxyCommand: "some-command-here",
+      ConnectTimeout: "0",
+      StrictHostKeyChecking: "no",
+      UserKnownHostsFile: "/dev/null",
+      LogLevel: "ERROR",
+    }),
+  ).rejects.toThrow(/Failed to rename temporary SSH config file.*EACCES/)
 })
diff --git a/src/sshConfig.ts b/src/sshConfig.ts
index a949cdcc..4a75b209 100644
--- a/src/sshConfig.ts
+++ b/src/sshConfig.ts
@@ -1,4 +1,4 @@
-import { mkdir, readFile, writeFile } from "fs/promises"
+import { mkdir, readFile, rename, stat, writeFile } from "fs/promises"
 import path from "path"
 import { countSubstring } from "./util"
 
@@ -20,14 +20,18 @@ export interface SSHValues {
 
 // Interface for the file system to make it easier to test
 export interface FileSystem {
-  readFile: typeof readFile
   mkdir: typeof mkdir
+  readFile: typeof readFile
+  rename: typeof rename
+  stat: typeof stat
   writeFile: typeof writeFile
 }
 
 const defaultFileSystem: FileSystem = {
-  readFile,
   mkdir,
+  readFile,
+  rename,
+  stat,
   writeFile,
 }
 
@@ -220,14 +224,45 @@ export class SSHConfig {
   }
 
   private async save() {
+    // We want to preserve the original file mode.
+    const existingMode = await this.fileSystem
+      .stat(this.filePath)
+      .then((stat) => stat.mode)
+      .catch((ex) => {
+        if (ex.code && ex.code === "ENOENT") {
+          return 0o600 // default to 0600 if file does not exist
+        }
+        throw ex // Any other error is unexpected
+      })
     await this.fileSystem.mkdir(path.dirname(this.filePath), {
       mode: 0o700, // only owner has rwx permission, not group or everyone.
       recursive: true,
     })
-    return this.fileSystem.writeFile(this.filePath, this.getRaw(), {
-      mode: 0o600, // owner rw
-      encoding: "utf-8",
-    })
+    const randSuffix = Math.random().toString(36).substring(8)
+    const fileName = path.basename(this.filePath)
+    const dirName = path.dirname(this.filePath)
+    const tempFilePath = `${dirName}/.${fileName}.vscode-coder-tmp.${randSuffix}`
+    try {
+      await this.fileSystem.writeFile(tempFilePath, this.getRaw(), {
+        mode: existingMode,
+        encoding: "utf-8",
+      })
+    } catch (err) {
+      throw new Error(
+        `Failed to write temporary SSH config file at ${tempFilePath}: ${err instanceof Error ? err.message : String(err)}. ` +
+          `Please check your disk space, permissions, and that the directory exists.`,
+      )
+    }
+
+    try {
+      await this.fileSystem.rename(tempFilePath, this.filePath)
+    } catch (err) {
+      throw new Error(
+        `Failed to rename temporary SSH config file at ${tempFilePath} to ${this.filePath}: ${
+          err instanceof Error ? err.message : String(err)
+        }. Please check your disk space, permissions, and that the directory exists.`,
+      )
+    }
   }
 
   public getRaw() {