Skip to content

Commit ca32d99

Browse files
johnstcnjohnstcn
authored
fix: SSHConfig: atomically write ssh config (#511)
1 parent e65ee21 commit ca32d99

File tree

2 files changed

+150
-22
lines changed

2 files changed

+150
-22
lines changed

‎src/sshConfig.test.ts‎

Lines changed: 108 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,17 @@
22
import { it, afterEach, vi, expect } from "vitest"
33
import { SSHConfig } from "./sshConfig"
44

5-
const sshFilePath = "~/.config/ssh"
5+
// This is not the usual path to ~/.ssh/config, but
6+
// setting it to a different path makes it easier to test
7+
// and makes mistakes abundantly clear.
8+
const sshFilePath = "/Path/To/UserHomeDir/.sshConfigDir/sshConfigFile"
9+
const sshTempFilePathExpr = `^/Path/To/UserHomeDir/\\.sshConfigDir/\\.sshConfigFile\\.vscode-coder-tmp\\.[a-z0-9]+$`
610

711
const mockFileSystem = {
8-
readFile: vi.fn(),
912
mkdir: vi.fn(),
13+
readFile: vi.fn(),
14+
rename: vi.fn(),
15+
stat: vi.fn(),
1016
writeFile: vi.fn(),
1117
}
1218

@@ -16,6 +22,7 @@ afterEach(() => {
1622

1723
it("creates a new file and adds config with empty label", async () => {
1824
mockFileSystem.readFile.mockRejectedValueOnce("No file found")
25+
mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" })
1926

2027
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
2128
await sshConfig.load()
@@ -38,11 +45,20 @@ Host coder-vscode--*
3845
# --- END CODER VSCODE ---`
3946

4047
expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
41-
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
48+
expect(mockFileSystem.writeFile).toBeCalledWith(
49+
expect.stringMatching(sshTempFilePathExpr),
50+
expectedOutput,
51+
expect.objectContaining({
52+
encoding: "utf-8",
53+
mode: 0o600, // Default mode for new files.
54+
}),
55+
)
56+
expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
4257
})
4358

4459
it("creates a new file and adds the config", async () => {
4560
mockFileSystem.readFile.mockRejectedValueOnce("No file found")
61+
mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" })
4662

4763
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
4864
await sshConfig.load()
@@ -65,7 +81,15 @@ Host coder-vscode.dev.coder.com--*
6581
# --- END CODER VSCODE dev.coder.com ---`
6682

6783
expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
68-
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
84+
expect(mockFileSystem.writeFile).toBeCalledWith(
85+
expect.stringMatching(sshTempFilePathExpr),
86+
expectedOutput,
87+
expect.objectContaining({
88+
encoding: "utf-8",
89+
mode: 0o600, // Default mode for new files.
90+
}),
91+
)
92+
expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
6993
})
7094

7195
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 () => {
77101
StrictHostKeyChecking=no
78102
UserKnownHostsFile=/dev/null`
79103
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
104+
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
80105

81106
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
82107
await sshConfig.load()
@@ -100,10 +125,11 @@ Host coder-vscode.dev.coder.com--*
100125
UserKnownHostsFile /dev/null
101126
# --- END CODER VSCODE dev.coder.com ---`
102127

103-
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
128+
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
104129
encoding: "utf-8",
105-
mode: 384,
130+
mode: 0o644,
106131
})
132+
expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
107133
})
108134

109135
it("updates an existent coder config", async () => {
@@ -138,6 +164,7 @@ Host coder-vscode.dev.coder.com--*
138164
Host *
139165
SetEnv TEST=1`
140166
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
167+
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
141168

142169
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
143170
await sshConfig.load()
@@ -164,10 +191,11 @@ Host coder-vscode.dev-updated.coder.com--*
164191
Host *
165192
SetEnv TEST=1`
166193

167-
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
194+
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
168195
encoding: "utf-8",
169-
mode: 384,
196+
mode: 0o644,
170197
})
198+
expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
171199
})
172200

173201
it("does not remove deployment-unaware SSH config and adds the new one", async () => {
@@ -186,6 +214,7 @@ Host coder-vscode--*
186214
UserKnownHostsFile=/dev/null
187215
# --- END CODER VSCODE ---`
188216
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
217+
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
189218

190219
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
191220
await sshConfig.load()
@@ -209,16 +238,18 @@ Host coder-vscode.dev.coder.com--*
209238
UserKnownHostsFile /dev/null
210239
# --- END CODER VSCODE dev.coder.com ---`
211240

212-
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
241+
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
213242
encoding: "utf-8",
214-
mode: 384,
243+
mode: 0o644,
215244
})
245+
expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
216246
})
217247

218248
it("it does not remove a user-added block that only matches the host of an old coder SSH config", async () => {
219249
const existentSSHConfig = `Host coder-vscode--*
220250
ForwardAgent=yes`
221251
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
252+
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
222253

223254
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
224255
await sshConfig.load()
@@ -243,10 +274,11 @@ Host coder-vscode.dev.coder.com--*
243274
UserKnownHostsFile /dev/null
244275
# --- END CODER VSCODE dev.coder.com ---`
245276

246-
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
277+
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
247278
encoding: "utf-8",
248-
mode: 384,
279+
mode: 0o644,
249280
})
281+
expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
250282
})
251283

252284
it("throws an error if there is a missing end block", async () => {
@@ -476,6 +508,7 @@ Host afterconfig
476508

477509
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
478510
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
511+
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 })
479512
await sshConfig.load()
480513

481514
const expectedOutput = `Host beforeconfig
@@ -517,14 +550,17 @@ Host afterconfig
517550
LogLevel: "ERROR",
518551
})
519552

520-
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
553+
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), expectedOutput, {
521554
encoding: "utf-8",
522-
mode: 384,
555+
mode: 0o644,
523556
})
557+
expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
524558
})
525559

526560
it("override values", async () => {
527561
mockFileSystem.readFile.mockRejectedValueOnce("No file found")
562+
mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" })
563+
528564
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
529565
await sshConfig.load()
530566
await sshConfig.update(
@@ -561,5 +597,62 @@ Host coder-vscode.dev.coder.com--*
561597
# --- END CODER VSCODE dev.coder.com ---`
562598

563599
expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
564-
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
600+
expect(mockFileSystem.writeFile).toBeCalledWith(
601+
expect.stringMatching(sshTempFilePathExpr),
602+
expectedOutput,
603+
expect.objectContaining({
604+
encoding: "utf-8",
605+
mode: 0o600, // Default mode for new files.
606+
}),
607+
)
608+
expect(mockFileSystem.rename).toBeCalledWith(expect.stringMatching(sshTempFilePathExpr), sshFilePath)
609+
})
610+
611+
it("fails if we are unable to write the temporary file", async () => {
612+
const existentSSHConfig = `Host beforeconfig
613+
HostName before.config.tld
614+
User before`
615+
616+
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
617+
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
618+
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 })
619+
mockFileSystem.writeFile.mockRejectedValueOnce(new Error("EACCES"))
620+
621+
await sshConfig.load()
622+
623+
expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
624+
await expect(
625+
sshConfig.update("dev.coder.com", {
626+
Host: "coder-vscode.dev.coder.com--*",
627+
ProxyCommand: "some-command-here",
628+
ConnectTimeout: "0",
629+
StrictHostKeyChecking: "no",
630+
UserKnownHostsFile: "/dev/null",
631+
LogLevel: "ERROR",
632+
}),
633+
).rejects.toThrow(/Failed to write temporary SSH config file.*EACCES/)
634+
})
635+
636+
it("fails if we are unable to rename the temporary file", async () => {
637+
const existentSSHConfig = `Host beforeconfig
638+
HostName before.config.tld
639+
User before`
640+
641+
const sshConfig = new SSHConfig(sshFilePath, mockFileSystem)
642+
mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig)
643+
mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 })
644+
mockFileSystem.writeFile.mockResolvedValueOnce("")
645+
mockFileSystem.rename.mockRejectedValueOnce(new Error("EACCES"))
646+
647+
await sshConfig.load()
648+
await expect(
649+
sshConfig.update("dev.coder.com", {
650+
Host: "coder-vscode.dev.coder.com--*",
651+
ProxyCommand: "some-command-here",
652+
ConnectTimeout: "0",
653+
StrictHostKeyChecking: "no",
654+
UserKnownHostsFile: "/dev/null",
655+
LogLevel: "ERROR",
656+
}),
657+
).rejects.toThrow(/Failed to rename temporary SSH config file.*EACCES/)
565658
})

‎src/sshConfig.ts‎

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdir, readFile, writeFile } from "fs/promises"
1+
import { mkdir, readFile, rename, stat, writeFile } from "fs/promises"
22
import path from "path"
33
import { countSubstring } from "./util"
44

@@ -20,14 +20,18 @@ export interface SSHValues {
2020

2121
// Interface for the file system to make it easier to test
2222
export interface FileSystem {
23-
readFile: typeof readFile
2423
mkdir: typeof mkdir
24+
readFile: typeof readFile
25+
rename: typeof rename
26+
stat: typeof stat
2527
writeFile: typeof writeFile
2628
}
2729

2830
const defaultFileSystem: FileSystem = {
29-
readFile,
3031
mkdir,
32+
readFile,
33+
rename,
34+
stat,
3135
writeFile,
3236
}
3337

@@ -220,14 +224,45 @@ export class SSHConfig {
220224
}
221225

222226
private async save() {
227+
// We want to preserve the original file mode.
228+
const existingMode = await this.fileSystem
229+
.stat(this.filePath)
230+
.then((stat) => stat.mode)
231+
.catch((ex) => {
232+
if (ex.code && ex.code === "ENOENT") {
233+
return 0o600 // default to 0600 if file does not exist
234+
}
235+
throw ex // Any other error is unexpected
236+
})
223237
await this.fileSystem.mkdir(path.dirname(this.filePath), {
224238
mode: 0o700, // only owner has rwx permission, not group or everyone.
225239
recursive: true,
226240
})
227-
return this.fileSystem.writeFile(this.filePath, this.getRaw(), {
228-
mode: 0o600, // owner rw
229-
encoding: "utf-8",
230-
})
241+
const randSuffix = Math.random().toString(36).substring(8)
242+
const fileName = path.basename(this.filePath)
243+
const dirName = path.dirname(this.filePath)
244+
const tempFilePath = `${dirName}/.${fileName}.vscode-coder-tmp.${randSuffix}`
245+
try {
246+
await this.fileSystem.writeFile(tempFilePath, this.getRaw(), {
247+
mode: existingMode,
248+
encoding: "utf-8",
249+
})
250+
} catch (err) {
251+
throw new Error(
252+
`Failed to write temporary SSH config file at ${tempFilePath}: ${err instanceof Error ? err.message : String(err)}. ` +
253+
`Please check your disk space, permissions, and that the directory exists.`,
254+
)
255+
}
256+
257+
try {
258+
await this.fileSystem.rename(tempFilePath, this.filePath)
259+
} catch (err) {
260+
throw new Error(
261+
`Failed to rename temporary SSH config file at ${tempFilePath} to ${this.filePath}: ${
262+
err instanceof Error ? err.message : String(err)
263+
}. Please check your disk space, permissions, and that the directory exists.`,
264+
)
265+
}
231266
}
232267

233268
public getRaw() {

0 commit comments

Comments
 (0)