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() {