From 49bc8df05607715e284c444a7dc48cf5fe5600b5 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 21 Sep 2026 18:44:34 +0300 Subject: [PATCH 1/2] fix(ssh): repair managed Windows SSH config permissions OpenSSH rejects a config file whose ACL grants access beyond the owner, SYSTEM, and Administrators, so a stale explicit grant on a generated fragment fails every connection with "Bad owner or permissions". Lock the Coder-managed SSH directory with checked icacls commands and let its inheritable grant repair the files beneath it. Inject the repair as a collaborator so the config writer stays platform-independent, and reject links, non-files, and paths that icacls could expand before granting anything. Repair existing deployment fragments on upgrade, and drive the real Windows tools from the native tests. Move the runtime architecture check into a global setup so vitest.config.mts stays declarative. Speed up the Windows ARM64 unit test job, where installing dependencies took eight minutes against one on x64. Neither bufferutil nor utf-8-validate ships a win32-arm64 prebuild, and both fall back to JavaScript, so skip their node-gyp builds. Hand the pnpm store cache to pnpm/action-setup, which keys on the same lockfile hash but falls back to the last store for the platform, so a dependency bump no longer refetches the whole store. A cold setup step now takes 2m31s. Fixes #1108 --- .gitattributes | 2 + .github/actions/setup/action.yml | 6 +- .github/workflows/ci.yaml | 6 + .vscodeignore | 2 +- CHANGELOG.md | 4 + CONTRIBUTING.md | 38 +++- pnpm-workspace.yaml | 5 +- src/remote/remote.ts | 3 + src/remote/sshConfig.ts | 105 ++++++++-- src/remote/windowsAcl.ts | 122 ++++++++++++ test/env-check.ts | 14 ++ test/unit/remote/sshConfig.test.ts | 189 +++++++++++++++++- test/unit/remote/windowsAcl.native.test.ts | 221 +++++++++++++++++++++ test/unit/remote/windowsAcl.test.ts | 186 +++++++++++++++++ vitest.config.mts | 1 + 15 files changed, 884 insertions(+), 20 deletions(-) create mode 100644 src/remote/windowsAcl.ts create mode 100644 test/env-check.ts create mode 100644 test/unit/remote/windowsAcl.native.test.ts create mode 100644 test/unit/remote/windowsAcl.test.ts diff --git a/.gitattributes b/.gitattributes index 6313b56c57..08d76f5cdb 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1,3 @@ * text=auto eol=lf +pnpm-lock.yaml linguist-generated=true +flake.lock linguist-generated=true diff --git a/.github/actions/setup/action.yml b/.github/actions/setup/action.yml index 27a655b4ae..f980648f76 100644 --- a/.github/actions/setup/action.yml +++ b/.github/actions/setup/action.yml @@ -10,11 +10,15 @@ runs: run: git config --global url."https://github.com/".insteadOf "git@github.com:" - uses: pnpm/action-setup@ea17c68df8912ef543352723c149a84f56e3d413 # v6.1.0 + with: + # Caches the pnpm store, keyed by lockfile hash with a prefix fallback, + # so a dependency bump reuses the rest of the store. setup-node's own + # pnpm cache restores on an exact hash only. + cache: true - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: node-version: "22" - cache: "pnpm" - name: Install dependencies shell: bash diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index fdcbfcff5e..6301c1bb6f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -48,6 +48,11 @@ jobs: name: Windows, electron-version: "latest", } + - { + os: windows-11-arm, + name: Windows ARM64, + electron-version: "latest", + } - { os: macos-15, name: macOS, electron-version: "latest" } steps: @@ -62,6 +67,7 @@ jobs: shell: bash env: CI: true + EXPECTED_ARCH: ${{ runner.arch }} test-integration: name: Integration Test (${{ matrix.name }}, VS Code ${{ matrix.vscode-version }}) diff --git a/.vscodeignore b/.vscodeignore index 93ceb676ab..0997b1288f 100644 --- a/.vscodeignore +++ b/.vscodeignore @@ -62,4 +62,4 @@ AGENTS.md # Storybook .storybook/** storybook-static/** -**/*.stories.* \ No newline at end of file +**/*.stories.* diff --git a/CHANGELOG.md b/CHANGELOG.md index b1c4b35055..a8ce8c5ff6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ replay the buffered connection logs instead. Close codes never reached the reconnect logic, so these closes retried forever. Server-initiated normal closes (`1000`/`1001`) keep reconnecting. +- Repair permissions on the Windows SSH config files the extension generates, + so connections stop failing with "Bad owner or permissions". Only you, + SYSTEM, and Administrators keep access to them. The extension leaves your own + SSH config untouched and needs no admin rights. ## [v1.16.3](https://github.com/coder/vscode-coder/releases/tag/v1.16.3) 2026-09-14 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ff4ed30b67..18bf8c53fa 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -87,6 +87,41 @@ command. Coder Remote periodically reads the `network-info-dir + "/" + matchingSSHPID` file to display network information. +### Windows SSH config permissions + +Windows files inherit their permissions from the directory they live in, so a +config the extension generates under `%APPDATA%\coder.coder-remote\ssh` can end +up readable by other accounts. OpenSSH rejects such a file with "Bad owner or +permissions" and skips the whole `Include`, which blocks every Coder host, not +just the one it came from. + +Before each managed write, `src/remote/windowsAcl.ts` locks the directory down +and lets its files inherit from it: + +| Step | Command | +| -------------------------------------------------------- | ---------------------------------------------------- | +| Read the current user's SID | `whoami.exe /user /fo csv /nh` | +| Clear the directory's own grants | `icacls.exe /reset` | +| Grant that user, SYSTEM, and Administrators full control | `icacls.exe /inheritance:r /grant:r ` | +| Clear each `*.conf` file so it inherits the directory | `icacls.exe /reset` | + +Resetting every `*.conf` file, not only the one being written, also repairs +files left behind by other deployments and editors. + +Worth knowing: + +- Like VS Code, the code checks exit codes but never reads ACLs back. It needs + no script, native module, ownership change, or elevation, and it leaves the + user's own SSH config alone. +- Links and non-files are rejected before the repair, because inheritable + grants reach children even without `/T`. That stops mistakes, not an attacker + racing the check. +- The repair is not atomic: a failure after `/reset` can leave the directory + with its parent's grants. + +`windowsAcl.native.test.ts` drives the real `icacls.exe`, `whoami.exe`, and +OpenSSH. Run it unelevated as well as in CI to catch privilege assumptions. + ## Other features The extension provides several sidebar panels: @@ -289,7 +324,8 @@ When updating the minimum Node.js version, update these files: Some dependencies are not directly used in the source but are required anyway. -- `bufferutil` and `utf-8-validate` are peer dependencies of `ws`. +- `bufferutil` and `utf-8-validate` are peer dependencies of `ws`. Their source + builds are off, so Windows on ARM64 uses their JavaScript fallback. - `ua-parser-js` and `dayjs` are used by the Coder API client. The coder client is vendored from coder/coder. Pin it to a release tag in diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index f0647df913..f29929bb40 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -32,13 +32,14 @@ dedupePeers: true allowBuilds: "@vscode/vsce-sign": true - bufferutil: true + # Only win32-arm64 lacks a prebuild, and its JavaScript fallback is fine. + bufferutil: false electron: true esbuild: true keytar: false odiff-bin: true unrs-resolver: true - utf-8-validate: true + utf-8-validate: false overrides: "@vscode-elements/elements": ^2.5.1 diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 9c18dad9a9..ce0321c1f1 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -67,6 +67,7 @@ import { sshSupportsSetEnv, type SshProperties, } from "./sshSupport"; +import { createManagedPermissions } from "./windowsAcl"; import { WorkspaceStateMachine } from "./workspaceStateMachine"; import type { Api } from "coder/site/src/api/api"; @@ -929,6 +930,8 @@ export class Remote { const coderConfig = new SshConfig( this.pathResolver.getSshConfigPath(safeHostname, hostEditorId(sshHost)), this.logger, + undefined, + createManagedPermissions(), ); // Options the user set themselves win the merge below, so they are exempt diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index e6fe17914f..8a95ff53cf 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -1,6 +1,7 @@ import { mkdir, readFile, + readdir, rename, stat, unlink, @@ -34,10 +35,20 @@ export interface SshValues { SetEnv?: string; } +/** + * Restricts the Coder-managed config directory and the files it generates. + * A config without one is not Coder-managed, so it is written untouched. + */ +export interface ManagedPermissions { + prepareDirectory(directory: string): Promise; + secure(filePath: string): Promise; +} + /** Injectable for tests. */ export interface FileSystem { mkdir: typeof mkdir; readFile: typeof readFile; + readdir: typeof readdir; rename: typeof rename; stat: typeof stat; unlink: typeof unlink; @@ -47,6 +58,7 @@ export interface FileSystem { const defaultFileSystem: FileSystem = { mkdir, readFile, + readdir, rename, stat, unlink, @@ -299,15 +311,19 @@ export class SshConfig { private readonly fileSystem: FileSystem; private readonly logger: Logger; private raw: string | undefined; + /** Marks this file as Coder-managed; absent for the user's own config. */ + private readonly permissions: ManagedPermissions | undefined; constructor( filePath: string, logger: Logger, fileSystem: FileSystem = defaultFileSystem, + permissions?: ManagedPermissions, ) { this.filePath = filePath; this.logger = logger; this.fileSystem = fileSystem; + this.permissions = permissions; } async load() { @@ -442,39 +458,99 @@ export class SshConfig { /** 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; - } - throw ex; - }); - await this.fileSystem.mkdir(path.dirname(this.filePath), { + const existingMode = await this.getFileMode(); + const fileName = path.basename(this.filePath); + const dirName = path.dirname(this.filePath); + await this.fileSystem.mkdir(dirName, { mode: 0o700, recursive: true, }); - const fileName = path.basename(this.filePath); - const dirName = path.dirname(this.filePath); + // Must come before any file reset or temporary write in this directory. + await this.permissions?.prepareDirectory(dirName); + await this.repairIncludedFiles(dirName); const tempPath = tempFilePath( `${dirName}/.${fileName}`, "vscode-coder-tmp", ); + await this.writeTemp(tempPath, existingMode); + await this.repairPermissions(tempPath); + await this.replaceWithTemp(tempPath); + } + + /** Preserve the existing file mode, defaulting to owner-only access. */ + private async getFileMode(): Promise { + try { + return (await this.fileSystem.stat(this.filePath)).mode; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return 0o600; + } + throw error; + } + } + + /** Repair every direct Include match; one unsafe sibling blocks every host. */ + private async repairIncludedFiles(dirName: string): Promise { + if (!this.permissions) return; + const entries = await this.fileSystem + .readdir(dirName, { withFileTypes: true }) + .catch((error: unknown) => { + this.logger.warn( + "Failed to enumerate Coder-managed SSH config files", + error, + ); + return []; + }); + for (const entry of entries) { + if (!entry.name.toLowerCase().endsWith(SSH_CONFIG_EXT)) continue; + const filePath = path.join(dirName, entry.name); + // On Windows, fopen fails on a directory, so OpenSSH aborts the whole + // Include. No ACL change fixes that, so report it instead. + if (!entry.isFile()) { + throw new Error( + `SSH config entry ${filePath} is not a regular file. Move or rename it so it no longer matches *.conf, then reconnect.`, + ); + } + await this.repairPermissions(filePath); + } + } + + /** Create the temporary file exclusively, leaving any preexisting path alone. */ + private async writeTemp(tempPath: string, mode: number): Promise { try { await this.fileSystem.writeFile(tempPath, this.getRaw(), { - mode: existingMode, encoding: "utf-8", + flag: "wx", + mode, }); } catch (err) { + // On EEXIST this write did not create the path, so it must not delete it. + if ((err as NodeJS.ErrnoException).code !== "EEXIST") { + await this.discardTemp(tempPath); + } throw new Error( `Failed to write temporary SSH config file at ${tempPath}: ${err instanceof Error ? err.message : String(err)}. ` + `Please check your disk space, permissions, and that the directory exists.`, { cause: err }, ); } + } + + /** Log a repair failure without preventing an SSH connection attempt. */ + private async repairPermissions(filePath: string): Promise { + try { + await this.permissions?.secure(filePath); + } catch (error) { + this.logger.warn( + "Failed to repair SSH config permissions", + filePath, + error, + ); + } + } + /** Replace the destination atomically, cleaning up if the rename fails. */ + private async replaceWithTemp(tempPath: string): Promise { try { await renameWithRetry( (src, dest) => this.fileSystem.rename(src, dest), @@ -493,6 +569,7 @@ export class SshConfig { } } + /** Attempt cleanup without hiding the original write or rename failure. */ private async discardTemp(tempPath: string): Promise { try { await this.fileSystem.unlink(tempPath); diff --git a/src/remote/windowsAcl.ts b/src/remote/windowsAcl.ts new file mode 100644 index 0000000000..f09b8298ef --- /dev/null +++ b/src/remote/windowsAcl.ts @@ -0,0 +1,122 @@ +import { execFile } from "node:child_process"; +import { lstat, readdir } from "node:fs/promises"; +import * as path from "node:path"; +import { promisify } from "node:util"; + +import { wrapError } from "../error/errorUtils"; + +import type { ManagedPermissions } from "./sshConfig"; + +const EXECUTE = promisify(execFile); + +/** Protect the Coder-managed SSH directory and repair its files by inheritance. */ +export const WINDOWS_ACL: ManagedPermissions = { prepareDirectory, secure }; + +/** Use DACL repair on Windows; other platforms rely on the file mode. */ +export function createManagedPermissions(): ManagedPermissions | undefined { + return process.platform === "win32" ? WINDOWS_ACL : undefined; +} + +/** Resolve a system tool without searching PATH or the working directory. */ +export function system32(name: string): string { + const systemRoot = process.env.SystemRoot; + if (!systemRoot || !isFullyQualifiedWindowsPath(systemRoot)) { + throw new Error("SystemRoot must be a fully qualified Windows path"); + } + return path.win32.join(systemRoot, "System32", name); +} + +/** Grant inheritable full control to the user, SYSTEM, and Administrators only. */ +async function prepareDirectory(target: string): Promise { + const directory = path.win32.normalize(target); + try { + checkPath(directory); + if (!(await lstat(directory)).isDirectory()) { + throw new Error("Expected a Coder-managed directory without links"); + } + // Inheritable grants reach children even without /T, so vet them first. + for (const entry of await readdir(directory)) { + await checkRegularFile(path.win32.join(directory, entry)); + } + const sid = await currentUserSid(); + // /grant:r alone leaves other trustees' explicit grants and denies. + await run("icacls.exe", [directory, "/reset"]); + await run("icacls.exe", [ + directory, + "/inheritance:r", + "/grant:r", + `*${sid}:(OI)(CI)F`, + "*S-1-5-18:(OI)(CI)F", // SYSTEM + "*S-1-5-32-544:(OI)(CI)F", // Administrators + ]); + } catch (error) { + throw wrapError( + "prepare SSH config directory permissions for", + directory, + error, + ); + } +} + +/** Drop a file's own permissions so it inherits the directory's. */ +async function secure(target: string): Promise { + const file = path.win32.normalize(target); + try { + await checkRegularFile(file); + await run("icacls.exe", [file, "/reset"]); + } catch (error) { + throw wrapError("repair SSH config permissions for", file, error); + } +} + +/** Accept drive-qualified or UNC paths without wildcards or control characters. */ +function isFullyQualifiedWindowsPath(target: string): boolean { + if (/[\0\r\n*?]/.test(target)) { + return false; + } + const normalized = path.win32.normalize(target); + return ( + /^[A-Za-z]:\\/.test(normalized) || + /^\\\\[^\\]+\\[^\\]+(?:\\|$)/.test(normalized) + ); +} + +/** Reject ambiguous paths and characters that can expand an icacls target. */ +function checkPath(target: string): void { + if (!isFullyQualifiedWindowsPath(target)) { + throw new Error( + "Expected a fully qualified Windows path without wildcards or control characters", + ); + } +} + +/** Reject links and non-files, which share their ACL with another target. */ +async function checkRegularFile(target: string): Promise { + checkPath(target); + const stat = await lstat(target); + if (!stat.isFile() || stat.nlink !== 1) { + throw new Error( + `Expected a regular file with no links: ${target}. Move or rename linked and non-file entries out of the Coder-managed SSH directory, then reconnect.`, + ); + } +} + +/** Read the SID, not the localized account name, from whoami's CSV output. */ +async function currentUserSid(): Promise { + const { stdout } = await run("whoami.exe", ["/user", "/fo", "csv", "/nh"]); + const sid = /,"(S-\d+(?:-\d+)+)"\s*$/.exec(stdout)?.[1]; + if (!sid) { + throw new Error("Could not read the current Windows user SID"); + } + return sid; +} + +/** Run a system tool without a shell, bounding its time and output. */ +function run(name: string, args: string[]) { + return EXECUTE(system32(name), args, { + windowsHide: true, + timeout: 10_000, + maxBuffer: 64 * 1024, + encoding: "utf8", + }); +} diff --git a/test/env-check.ts b/test/env-check.ts new file mode 100644 index 0000000000..9dc55224b5 --- /dev/null +++ b/test/env-check.ts @@ -0,0 +1,14 @@ +import assert from "node:assert/strict"; + +/** Fails the run when the test runtime is not the architecture CI expects. */ +export default function checkEnv(): void { + const expected = process.env.EXPECTED_ARCH?.toLowerCase(); + if (!expected) { + return; + } + assert.equal( + process.arch, + expected, + `Test runtime architecture is ${process.arch}, but CI expects ${expected}`, + ); +} diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index ce70715f74..a711653dfb 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -1,6 +1,7 @@ import { vol } from "memfs"; import * as fsPromises from "node:fs/promises"; import * as os from "node:os"; +import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { @@ -8,13 +9,17 @@ import { parseCoderSshOptions, parseSshConfig, SshConfig, + type ManagedPermissions, type SshValues, validateDeploymentSshOptions, } from "@/remote/sshConfig"; import { createMockLogger } from "../../mocks/testHelpers"; -vi.mock("node:fs/promises", async () => (await import("memfs")).fs.promises); +vi.mock("node:fs/promises", async () => { + const fs = (await import("memfs")).fs.promises; + return { ...fs, readdir: vi.fn(fs.readdir) }; +}); vi.mock("node:os", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, homedir: vi.fn(() => "/Path/To/UserHomeDir") }; @@ -22,6 +27,10 @@ vi.mock("node:os", async (importOriginal) => { const homeDir = "/Path/To/UserHomeDir"; const sshFilePath = "/Path/To/UserHomeDir/.sshConfigDir/sshConfigFile"; +const MANAGED_SSH_DIR = path.join(homeDir, ".sshConfigDir"); +const MANAGED_SSH_FILE_PATH = path.join(MANAGED_SSH_DIR, "deployment.conf"); +const TEMP_MARKER = "vscode-coder-tmp"; +const FAILURE = new Error("permission repair failed"); 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.`; @@ -83,6 +92,8 @@ const includeBlock = renderIncludeBlock(includeDir); const mockLogger = createMockLogger(); const readConfig = () => fsPromises.readFile(sshFilePath, "utf-8"); +const tempFiles = () => + Object.keys(vol.toJSON()).filter((file) => file.includes(TEMP_MARKER)); async function loadSshConfig( contents?: string, @@ -116,6 +127,8 @@ async function updateInclude( beforeEach(() => { vol.reset(); + vi.clearAllMocks(); + vi.mocked(fsPromises.readdir).mockReset(); vi.mocked(os.homedir).mockReturnValue(homeDir); }); @@ -378,6 +391,21 @@ describe("persistence", () => { ); }); + it("does not remove a preexisting exclusive temp path", async () => { + vi.spyOn(crypto, "randomUUID").mockReturnValue( + "00000000-0000-0000-0000-000000000000", + ); + const tempPath = + "/Path/To/UserHomeDir/.sshConfigDir/.sshConfigFile.vscode-coder-tmp-00000000"; + vol.fromJSON({ [tempPath]: "unrelated temp" }); + const sshConfig = await loadSshConfig(); + + await expect(sshConfig.update(BASE_SSH_VALUES)).rejects.toThrow( + "Failed to write temporary SSH config file", + ); + expect(await fsPromises.readFile(tempPath, "utf-8")).toBe("unrelated temp"); + }); + 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" }); @@ -410,6 +438,165 @@ describe("persistence", () => { }); }); +describe("managed config permissions", () => { + const sibling = path.join(MANAGED_SSH_DIR, "sibling.conf"); + const upperCase = path.join(MANAGED_SSH_DIR, "upper.CONF"); + const siblings = { [sibling]: "Host sibling", [upperCase]: "Host upper" }; + const existing = { [MANAGED_SSH_FILE_PATH]: "Host original" }; + const anyTempFile = expect.stringContaining(TEMP_MARKER); + + /** Repair that fails for the paths the test names. */ + function permissions(...failures: readonly string[]): ManagedPermissions { + return { + prepareDirectory: vi.fn(() => Promise.resolve()), + secure: vi.fn((filePath: string) => + failures.some((name) => filePath.includes(name)) + ? Promise.reject(FAILURE) + : Promise.resolve(), + ), + }; + } + + function updateManaged( + acl: ManagedPermissions, + files: Record, + ) { + vol.fromJSON(files); + return new SshConfig( + MANAGED_SSH_FILE_PATH, + mockLogger, + fsPromises, + acl, + ).update(BASE_SSH_VALUES); + } + + const securedPaths = (acl: ManagedPermissions) => + vi.mocked(acl.secure).mock.calls.flat(); + + interface RepairCase { + name: string; + files: Record; + failures: readonly string[]; + secured: unknown[]; + warnings: number; + } + it.each([ + { + name: "rewriting an existing deployment", + files: { ...existing, ...siblings }, + failures: [], + // The file being replaced is repaired too: the rename needs access to it. + secured: [MANAGED_SSH_FILE_PATH, sibling, upperCase, anyTempFile], + warnings: 0, + }, + { + name: "adding the first file of a new deployment", + files: siblings, + failures: [], + secured: [sibling, upperCase, anyTempFile], + warnings: 0, + }, + { + name: "a sibling and the temp file cannot be repaired", + files: { ...existing, ...siblings }, + failures: ["sibling.conf", TEMP_MARKER], + secured: [MANAGED_SSH_FILE_PATH, sibling, upperCase, anyTempFile], + warnings: 2, + }, + ])( + "repairs every Include match and the temp file when $name", + async ({ files, failures, secured, warnings }) => { + const acl = permissions(...failures); + + await updateManaged(acl, files); + + expect(acl.prepareDirectory).toHaveBeenCalledWith(MANAGED_SSH_DIR); + expect(securedPaths(acl)).toEqual(secured); + expect( + await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf-8"), + ).toContain("ProxyCommand some-command-here"); + expect(await fsPromises.readFile(sibling, "utf-8")).toBe("Host sibling"); + expect(mockLogger.warn).toHaveBeenCalledTimes(warnings); + expect(tempFiles()).toEqual([]); + }, + ); + + it("repairs the temp file before it replaces the config", async () => { + const acl = permissions(); + let destinationDuringRepair: string | undefined; + vi.mocked(acl.secure).mockImplementation(async (filePath) => { + if (filePath.includes(TEMP_MARKER)) { + destinationDuringRepair = await fsPromises.readFile( + MANAGED_SSH_FILE_PATH, + "utf-8", + ); + } + }); + + await updateManaged(acl, existing); + + // Undefined would mean the temp file was never repaired at all. + expect(destinationDuringRepair).toBe("Host original"); + }); + + it("writes and repairs nothing when directory preparation fails", async () => { + const acl = permissions(); + vi.mocked(acl.prepareDirectory).mockRejectedValueOnce(FAILURE); + + await expect( + updateManaged(acl, { ...existing, ...siblings }), + ).rejects.toThrow(FAILURE); + + expect(acl.secure).not.toHaveBeenCalled(); + expect(await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf8")).toBe( + "Host original", + ); + expect(tempFiles()).toEqual([]); + }); + + it("preserves the config and reports a directory matching the Include", async () => { + const directory = path.join(MANAGED_SSH_DIR, "directory.conf"); + + await expect( + updateManaged(permissions(), { ...existing, [directory]: null }), + ).rejects.toThrow(`SSH config entry ${directory} is not a regular file`); + + expect(await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf-8")).toBe( + "Host original", + ); + expect(tempFiles()).toEqual([]); + }); + + it("warns and still writes the config when enumeration fails", async () => { + const error = new Error("cannot enumerate"); + vi.mocked(fsPromises.readdir).mockRejectedValueOnce(error); + + await updateManaged(permissions(), existing); + + expect(await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf-8")).toContain( + "ProxyCommand some-command-here", + ); + expect(mockLogger.warn).toHaveBeenCalledWith( + "Failed to enumerate Coder-managed SSH config files", + error, + ); + }); + + it("neither repairs nor enumerates without injected permissions", async () => { + vol.fromJSON(existing); + + await new SshConfig(MANAGED_SSH_FILE_PATH, mockLogger, fsPromises).update( + BASE_SSH_VALUES, + ); + + expect(await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf-8")).toContain( + "ProxyCommand some-command-here", + ); + expect(fsPromises.readdir).not.toHaveBeenCalled(); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); +}); + describe("parseSshConfig", () => { interface ParseSshConfigCase { name: string; diff --git a/test/unit/remote/windowsAcl.native.test.ts b/test/unit/remote/windowsAcl.native.test.ts new file mode 100644 index 0000000000..5a48f5ea06 --- /dev/null +++ b/test/unit/remote/windowsAcl.native.test.ts @@ -0,0 +1,221 @@ +import { execFile as execFileCallback } from "node:child_process"; +import { randomUUID } from "node:crypto"; +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 { describe, expect, it, onTestFinished } from "vitest"; + +import { SshConfig, type SshValues } from "@/remote/sshConfig"; +import { system32, WINDOWS_ACL } from "@/remote/windowsAcl"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +import type { Logger } from "@/logging/logger"; + +const execFile = promisify(execFileCallback); +const GUESTS_SID = "S-1-5-32-546"; +const GUESTS_ALIAS = "BG"; + +// Real icacls and ssh, no mocks. Needs the Windows OpenSSH client. Run it +// without elevation as well as in CI to catch privilege assumptions. +describe.runIf(process.platform === "win32")( + "Windows SSH config ACL repair", + () => { + it( + "repairs the directory and every fragment so OpenSSH accepts them", + { timeout: 45_000 }, + async () => { + const { root, logger } = await nativeFixture(); + const directory = path.join(root, "coder.coder-remote", "ssh"); + const userConfig = path.join(root, "config"); + const current = path.join(directory, "current.conf"); + const other = path.join(directory, "other.conf"); + const host = "coder-acl-current"; + await fs.mkdir(directory, { recursive: true }); + await fs.writeFile( + userConfig, + `Include "${directory.replaceAll("\\", "/")}/*.conf"\n`, + ); + await fs.writeFile(current, renderConfig(host, "unsafe-user")); + await fs.writeFile(other, renderConfig("coder-acl-other", "other")); + + // A fragment Guests can reach makes OpenSSH refuse the whole include. + // One inherits the grant, the other blocks inheritance and keeps its + // own, which the directory repair alone would miss. + await grant(directory, GUESTS_SID, "(OI)(CI)(F)"); + await grant(current, GUESTS_SID, "(F)"); + await execFile(system32("icacls.exe"), [other, "/inheritancelevel:d"]); + await grant(other, GUESTS_SID, "(F)"); + const rootBefore = await savedAcl(root); + const userConfigBefore = await savedAcl(userConfig); + await expect(resolve(userConfig, host)).rejects.toMatchObject({ + stderr: expect.stringContaining("Bad owner or permissions"), + }); + + await managed(current, logger).update(sshValues(host, "repaired")); + + expect(await resolve(userConfig, host)).toContain( + "proxycommand repaired", + ); + expect(await savedAcl(directory)).not.toContain(GUESTS_ALIAS); + for (const fragment of [current, other]) { + const acl = await savedAcl(fragment); + expect(acl).toContain("A;ID;"); + expect(acl).not.toContain(GUESTS_ALIAS); + } + expect(logger.warn).not.toHaveBeenCalled(); + expect(await savedAcl(root)).toBe(rootBefore); + expect(await savedAcl(userConfig)).toBe(userConfigBefore); + + // /inheritance:r keeps a later parent grant out of the directory. + const repaired = await savedAcl(directory); + await grant(root, GUESTS_SID, "(OI)(CI)(F)"); + expect(await savedAcl(root)).not.toBe(rootBefore); + expect(await savedAcl(directory)).toBe(repaired); + + // Reconnecting still rewrites the file the repair locked down. + await managed(current, logger).update(sshValues(host, "rewritten")); + expect(await resolve(userConfig, host)).toContain( + "proxycommand rewritten", + ); + expect(await savedAcl(directory)).toBe(repaired); + expect(logger.warn).not.toHaveBeenCalled(); + }, + ); + + interface UnsafeEntryCase { + name: string; + /** Adds the unsafe entry and names extra paths that must not change. */ + create: (directory: string, root: string) => Promise; + } + it.each([ + { + name: "a hard link to a file outside it", + create: async (directory, root) => { + const external = path.join(root, "external.conf"); + await fs.writeFile(external, "# external"); + await fs.link(external, path.join(directory, "linked.conf")); + return [external]; + }, + }, + { + name: "a directory matching *.conf", + create: async (directory) => { + await fs.mkdir(path.join(directory, "folder.conf")); + return []; + }, + }, + ])( + "refuses to write when the managed directory holds $name", + async ({ create }) => { + const { root, logger } = await nativeFixture(); + const directory = path.join(root, "managed"); + const current = path.join(directory, "current.conf"); + await fs.mkdir(directory); + await fs.writeFile(current, "# unchanged"); + const watched = [ + directory, + current, + ...(await create(directory, root)), + ]; + await grant(directory, GUESTS_SID, "(OI)(CI)(F)"); + const before = await Promise.all(watched.map(savedAcl)); + + await expect( + managed(current, logger).update(sshValues("coder-unsafe", "unsafe")), + ).rejects.toThrow(); + + expect(await Promise.all(watched.map(savedAcl))).toEqual(before); + expect(await fs.readFile(current, "utf8")).toBe("# unchanged"); + expect( + (await fs.readdir(directory)).filter((name) => + name.includes("vscode-coder-tmp"), + ), + ).toEqual([]); + }, + ); + + it.each([ + { + name: "a directory junction", + create: async (root: string) => { + const target = path.join(root, "junction target"); + const candidate = path.join(root, "junction"); + await fs.mkdir(target); + await fs.symlink(target, candidate, "junction"); + return { candidate, unchanged: target }; + }, + }, + { + name: "a regular file", + create: async (root: string) => { + const candidate = path.join(root, "not a directory"); + await fs.writeFile(candidate, "unchanged"); + return { candidate, unchanged: candidate }; + }, + }, + ])("rejects $name as the managed directory", async ({ create }) => { + const { root } = await nativeFixture(); + const { candidate, unchanged } = await create(root); + const before = await savedAcl(unchanged); + + await expect(WINDOWS_ACL.prepareDirectory(candidate)).rejects.toThrow(); + expect(await savedAcl(unchanged)).toBe(before); + }); + }, +); + +const managed = (filePath: string, logger: Logger) => + new SshConfig(filePath, logger, undefined, WINDOWS_ACL); + +async function nativeFixture() { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "coder acl é space-")); + onTestFinished(() => fs.rm(root, { recursive: true, force: true })); + return { root, logger: createMockLogger() }; +} + +function sshValues(host: string, proxyCommand: string): SshValues { + return { + Host: host, + ProxyCommand: proxyCommand, + ConnectTimeout: "0", + StrictHostKeyChecking: "no", + UserKnownHostsFile: "NUL", + LogLevel: "ERROR", + ServerAliveInterval: "10", + ServerAliveCountMax: "3", + }; +} + +const renderConfig = (host: string, proxyCommand: string) => + `Host ${host}\n ProxyCommand ${proxyCommand}\n`; + +/** Dump a path's ACL so a test can compare it before and after a repair. */ +async function savedAcl(pathname: string): Promise { + const backup = path.join(os.tmpdir(), `coder-acl-${randomUUID()}.txt`); + try { + await execFile(system32("icacls.exe"), [pathname, "/save", backup]); + return await fs.readFile(backup, "utf16le"); + } finally { + await fs.rm(backup, { force: true }); + } +} + +/** Resolve a host through the user's config to prove OpenSSH reads the include. */ +async function resolve(userConfig: string, host: string): Promise { + const { stdout } = await execFile(system32("OpenSSH/ssh.exe"), [ + "-G", + "-F", + userConfig, + host, + ]); + return stdout; +} + +const grant = (pathname: string, sid: string, permissions: string) => + execFile(system32("icacls.exe"), [ + pathname, + "/grant", + `*${sid}:${permissions}`, + ]); diff --git a/test/unit/remote/windowsAcl.test.ts b/test/unit/remote/windowsAcl.test.ts new file mode 100644 index 0000000000..b5e531c349 --- /dev/null +++ b/test/unit/remote/windowsAcl.test.ts @@ -0,0 +1,186 @@ +import { promisify } from "node:util"; +import { beforeEach, describe, expect, it, onTestFinished, vi } from "vitest"; + +import { WINDOWS_ACL } from "@/remote/windowsAcl"; + +import type { Stats } from "node:fs"; + +type FileStat = Pick; + +const { execute, lstat, readdir } = vi.hoisted(() => ({ + execute: + vi.fn<(command: string, args: string[]) => Promise<{ stdout: string }>>(), + lstat: vi.fn<(target: string) => Promise>(), + readdir: vi.fn<() => Promise>(), +})); + +vi.mock("node:child_process", () => ({ + execFile: Object.assign(vi.fn(), { [promisify.custom]: execute }), +})); +vi.mock("node:fs/promises", () => ({ lstat, readdir })); + +const DIRECTORY = "C:\\Users\\coder\\AppData\\Roaming\\coder.coder-remote\\ssh"; +const FILE = `${DIRECTORY}\\file.conf`; +const ICACLS = "C:\\Windows\\System32\\icacls.exe"; +const WHOAMI = "C:\\Windows\\System32\\whoami.exe"; +const SID = "S-1-5-21-1-2-3-1001"; +const WHOAMI_CSV = `"account","${SID}"\r\n`; + +const DIR: FileStat = { + isDirectory: () => true, + isFile: () => false, + nlink: 1, +}; +const FILE_STAT: FileStat = { + isDirectory: () => false, + isFile: () => true, + nlink: 1, +}; +const LINKED: FileStat = { ...FILE_STAT, nlink: 2 }; + +/** Each run as one line, so a test can assert the whole transcript at once. */ +const commandLines = () => + execute.mock.calls.map(([command, args]) => `${command} ${args.join(" ")}`); + +const ranIcacls = () => commandLines().some((line) => line.startsWith(ICACLS)); + +beforeEach(() => { + execute.mockReset().mockResolvedValue({ stdout: WHOAMI_CSV }); + lstat + .mockReset() + .mockImplementation((target) => + Promise.resolve(target === DIRECTORY ? DIR : FILE_STAT), + ); + readdir.mockReset().mockResolvedValue(["file.conf"]); + vi.stubEnv("SystemRoot", "C:\\Windows"); + onTestFinished(() => { + vi.unstubAllEnvs(); + }); +}); + +describe("WINDOWS_ACL", () => { + it("restricts the directory, then resets files to inherit from it", async () => { + await WINDOWS_ACL.prepareDirectory(DIRECTORY.replaceAll("\\", "/")); + await WINDOWS_ACL.secure(FILE); + + expect(commandLines()).toEqual([ + `${WHOAMI} /user /fo csv /nh`, + `${ICACLS} ${DIRECTORY} /reset`, + `${ICACLS} ${DIRECTORY} /inheritance:r /grant:r *${SID}:(OI)(CI)F ` + + "*S-1-5-18:(OI)(CI)F *S-1-5-32-544:(OI)(CI)F", + `${ICACLS} ${FILE} /reset`, + ]); + }); + + interface RejectCase { + name: string; + error: string; + arrange?: () => void; + call: () => Promise; + /** Extra assertion for rows that must stop before a specific step. */ + assert?: () => void; + } + it.each([ + { + name: "a relative directory", + error: "fully qualified", + call: () => WINDOWS_ACL.prepareDirectory("relative"), + }, + { + name: "a wildcard file path", + error: "fully qualified", + call: () => WINDOWS_ACL.secure(`${DIRECTORY}\\*.conf`), + }, + { + name: "a relative SystemRoot", + error: "SystemRoot must be", + arrange: () => vi.stubEnv("SystemRoot", "Windows"), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + { + name: "a missing SystemRoot", + error: "SystemRoot must be", + arrange: () => vi.stubEnv("SystemRoot", undefined), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + { + name: "a linked directory, before enumerating it", + error: "directory without links", + arrange: () => lstat.mockResolvedValue(FILE_STAT), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + assert: () => expect(readdir).not.toHaveBeenCalled(), + }, + { + name: "a linked child, before permissions can propagate", + error: "regular file with no links", + arrange: () => + lstat.mockResolvedValueOnce(DIR).mockResolvedValueOnce(LINKED), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + { + name: "a linked file", + error: "regular file with no links", + arrange: () => lstat.mockResolvedValue(LINKED), + call: () => WINDOWS_ACL.secure(FILE), + }, + { + name: "whoami output with no SID", + error: "Could not read the current Windows user SID", + arrange: () => execute.mockResolvedValue({ stdout: '"account","nope"' }), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + { + name: "whoami output with an extra column", + error: "Could not read the current Windows user SID", + arrange: () => + execute.mockResolvedValue({ stdout: `${WHOAMI_CSV},"extra"` }), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + ])("rejects $name without changing permissions", async (testCase) => { + testCase.arrange?.(); + await expect(testCase.call()).rejects.toThrow(testCase.error); + expect(ranIcacls()).toBe(false); + testCase.assert?.(); + }); + + it("propagates an icacls failure without running the next command", async () => { + execute.mockImplementation((command) => + command === ICACLS + ? Promise.reject(new Error("access denied")) + : Promise.resolve({ stdout: WHOAMI_CSV }), + ); + + await expect(WINDOWS_ACL.prepareDirectory(DIRECTORY)).rejects.toThrow( + "access denied", + ); + await expect(WINDOWS_ACL.secure(FILE)).rejects.toThrow("access denied"); + + // The failing /reset must not be followed by a grant on a still-open directory. + expect(commandLines().some((line) => line.includes("/grant:r"))).toBe( + false, + ); + }); + + it.each([ + "C:\\Users\\coder\\config", + "C:/Users/coder/config", + "\\\\server\\share\\config", + ])("accepts the fully qualified path %s", async (target) => { + await expect(WINDOWS_ACL.secure(target)).resolves.toBeUndefined(); + }); + + it.each([ + "\\\\server", + "\\config", + "C:config", + "config", + "C:\\*.conf", + "C:\\file?.conf", + "C:\\file\r.conf", + "C:\\file\0.conf", + "C:\\file\n.conf", + ])("rejects the path %j", async (target) => { + await expect(WINDOWS_ACL.secure(target)).rejects.toThrow("fully qualified"); + expect(ranIcacls()).toBe(false); + }); +}); diff --git a/vitest.config.mts b/vitest.config.mts index 88ec0b90d9..9f1cbdd8ba 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -12,6 +12,7 @@ const testTimeout = process.platform === "win32" ? 10_000 : 5_000; export default defineConfig({ test: { testTimeout, + globalSetup: "./test/env-check.ts", projects: [ { extends: true, From 7593d1bc284c16f69aa34258ee2c9c8c234209a8 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 21 Sep 2026 18:44:49 +0300 Subject: [PATCH 2/2] fix(ssh): ignore entries that vanish while vetting the SSH directory Two connecting editor windows share the Coder-managed SSH directory, and each one writes its config through a temporary sibling file. A scan can read a directory entry that the other window renames away before the following lstat, and that ENOENT aborted directory preparation and the connection. Skip an entry that no longer exists, and rethrow every other error. --- src/remote/windowsAcl.ts | 9 ++++++++- test/unit/remote/windowsAcl.test.ts | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/remote/windowsAcl.ts b/src/remote/windowsAcl.ts index f09b8298ef..4684872a9c 100644 --- a/src/remote/windowsAcl.ts +++ b/src/remote/windowsAcl.ts @@ -36,7 +36,14 @@ async function prepareDirectory(target: string): Promise { } // Inheritable grants reach children even without /T, so vet them first. for (const entry of await readdir(directory)) { - await checkRegularFile(path.win32.join(directory, entry)); + try { + await checkRegularFile(path.win32.join(directory, entry)); + } catch (error) { + // Another window's temporary config can be renamed away mid-scan. + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error; + } + } } const sid = await currentUserSid(); // /grant:r alone leaves other trustees' explicit grants and denies. diff --git a/test/unit/remote/windowsAcl.test.ts b/test/unit/remote/windowsAcl.test.ts index b5e531c349..24f9314e43 100644 --- a/test/unit/remote/windowsAcl.test.ts +++ b/test/unit/remote/windowsAcl.test.ts @@ -72,6 +72,26 @@ describe("WINDOWS_ACL", () => { ]); }); + it("ignores an entry that disappears between readdir and lstat", async () => { + readdir.mockResolvedValue(["file.conf", "file.conf.tmp"]); + lstat.mockImplementation((target) => { + if (target === DIRECTORY) { + return Promise.resolve(DIR); + } + if (target.endsWith(".tmp")) { + return Promise.reject( + Object.assign(new Error("no such file"), { code: "ENOENT" }), + ); + } + return Promise.resolve(FILE_STAT); + }); + + await expect( + WINDOWS_ACL.prepareDirectory(DIRECTORY), + ).resolves.toBeUndefined(); + expect(ranIcacls()).toBe(true); + }); + interface RejectCase { name: string; error: string;