Skip to content

Commit 8cf3d1c

Browse files
committed
fix(ssh): simplify Windows ACL repair with directory inheritance
Replace the WSH setter and runtime ACL parsing with checked icacls commands. Repair existing deployment fragments on upgrade, retain path and link guards, and consolidate ACL helpers and tests. Generated by Coder Agents on behalf of @EhabY.
1 parent dddc9e9 commit 8cf3d1c

22 files changed

Lines changed: 643 additions & 1118 deletions

‎.github/workflows/ci.yaml‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -174,17 +174,6 @@ jobs:
174174
- name: Package extension
175175
run: pnpm vsce package --no-dependencies --out "${{ steps.setup.outputs.packageName }}"
176176

177-
# Repair degrades to a logged warning when the script is absent, so a
178-
# dropped asset would not fail any other check.
179-
- name: Verify the Windows ACL script shipped
180-
env:
181-
PACKAGE: ${{ steps.setup.outputs.packageName }}
182-
run: |
183-
if ! unzip -l "$PACKAGE" | grep -q "extension/assets/wsh/acl.js"; then
184-
echo "::error::assets/wsh/acl.js is missing from the VSIX. Check .vscodeignore."
185-
exit 1
186-
fi
187-
188177
- name: Upload artifact (PR)
189178
if: github.event_name == 'pull_request'
190179
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1

‎.prettierrc.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"overrides": [
33
{
4-
"files": ["*.jsonc", "assets/wsh/acl.js"],
4+
"files": "*.jsonc",
55
"options": {
66
"trailingComma": "none"
77
}

‎.vscodeignore‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ coverage/**
55

66
# Development files
77
src/**
8-
assets/wsh/tsconfig.json
98
test/**
109
scripts/**
1110
**/*.ts

‎CHANGELOG.md‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,10 @@
99

1010
### Fixed
1111

12-
- Repair permissions on Coder-managed Windows SSH config files, including other
13-
deployments' files matched by the shared Include. This fixes connections blocked
14-
by inherited permissions or stale account access in an unrelated config. The
15-
main SSH config and directory permissions are left unchanged. If repair fails,
16-
log a warning and still attempt the SSH connection.
12+
- Windows: fixed connections failing with "Bad owner or permissions" on the
13+
SSH config files the extension generates. The extension now repairs those
14+
permissions on connect, so only you, SYSTEM, and Administrators can read
15+
them. Your own SSH config is left untouched, and no admin rights are needed.
1716

1817
## [v1.16.3](https://github.com/coder/vscode-coder/releases/tag/v1.16.3) 2026-09-14
1918

‎CONTRIBUTING.md‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -215,23 +215,24 @@ Alternatively:
215215

216216
### Windows SSH config permissions
217217

218-
Repair requires Windows Script Host, JScript, and ADSI to be allowed by policy.
219-
The extension does not request elevation or bypass policy. A standard user can
220-
repair files when they have access to read and change the DACL (`READ_CONTROL`
221-
and `WRITE_DAC`). If repair fails, it logs a warning and still attempts the SSH
222-
connection, which OpenSSH may then reject. Write and rename failures still stop
223-
setup. If the script fails after inheritance is disabled, copied grants remain
224-
until a successful retry; access is not reset to the parent's permissions.
225-
226-
`assets/wsh/acl.js` ships as source in the universal VSIX and runs under Windows
227-
Script Host, so it uses ES3 syntax rather than Node.js. Its sibling
228-
`tsconfig.json` and `globals.d.ts` keep the WScript and ADSI types out of the
229-
extension's Node.js environment; `pnpm typecheck` covers both. Typechecking does
230-
not transpile the asset, so keep indexed loops: `for...of` fails the ES3 lint
231-
check. A typecheck is not a runtime compatibility check, so `acl.native.test.ts`
232-
drives the real `icacls.exe`, `cscript.exe`, and `ssh.exe` instead of mocks. It
233-
runs whenever the tests run on Windows, including x64 and ARM64 in CI, and needs
234-
the OpenSSH client installed.
218+
On Windows, the extension writes generated deployment configs to
219+
`%APPDATA%\coder.coder-remote\ssh`, and OpenSSH refuses to read the whole
220+
include when any of them is too permissive. Before each managed write,
221+
`src/remote/windowsAcl.ts` runs `whoami.exe` to find the current user, then
222+
`icacls.exe /reset` and `/inheritance:r /grant:r` on the directory, so only that
223+
user, SYSTEM, and Administrators keep inheritable full control. Every `*.conf`
224+
file in the directory is then reset to inherit it, which also repairs other
225+
deployments and editors on the first connection after an upgrade.
226+
227+
Like VS Code, the code checks command exit codes but never reads ACLs back. It
228+
needs no scripts, native addon, ownership change, or elevation, and it leaves
229+
the user's own SSH config alone. Links and non-files are rejected before the
230+
repair, because inheritable grants reach children even without `/T`. That stops
231+
mistakes, not an attacker racing the check. The repair is not atomic either: a
232+
failure after `/reset` can leave the directory with its parent's grants.
233+
234+
`windowsAcl.native.test.ts` drives the real `icacls.exe`, `whoami.exe`, and OpenSSH.
235+
Run it unelevated as well as in CI to catch privilege assumptions.
235236

236237
## Node.js Version
237238

‎assets/wsh/acl.js‎

Lines changed: 0 additions & 87 deletions
This file was deleted.

‎assets/wsh/globals.d.ts‎

Lines changed: 0 additions & 61 deletions
This file was deleted.

‎assets/wsh/tsconfig.json‎

Lines changed: 0 additions & 13 deletions
This file was deleted.

‎eslint.config.mjs‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -179,19 +179,6 @@ export default defineConfig(
179179
},
180180
},
181181

182-
// Windows Script Host runs JScript with ES3 syntax and globals.
183-
{
184-
files: ["assets/wsh/acl.js"],
185-
languageOptions: {
186-
ecmaVersion: 3,
187-
sourceType: "script",
188-
globals: {
189-
ActiveXObject: "readonly",
190-
WScript: "readonly",
191-
},
192-
},
193-
},
194-
195182
// Build config - ESM with Node globals
196183
{
197184
files: ["esbuild.mjs", "scripts/*.mjs", ".storybook/themes/*.{mjs,cjs}"],

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636
"test:extension": "cross-env ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs --project extension",
3737
"test:integration": "pnpm compile-tests:integration && node esbuild.mjs && vscode-test",
3838
"test:webview": "cross-env ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs --project webview",
39-
"typecheck": "concurrently -g -n extension,tests,packages,storybook,windows-acl \"tsc --noEmit\" \"tsc --noEmit -p test\" \"pnpm typecheck:packages\" \"tsc --noEmit -p .storybook\" \"tsc -p assets/wsh/tsconfig.json\"",
39+
"typecheck": "concurrently -g -n extension,tests,packages,storybook \"tsc --noEmit\" \"tsc --noEmit -p test\" \"pnpm typecheck:packages\" \"tsc --noEmit -p .storybook\"",
4040
"typecheck:packages": "pnpm -r --filter \"./packages/*\" --parallel typecheck",
4141
"watch": "concurrently -g -n extension,webviews \"pnpm watch:extension\" \"pnpm watch:webviews\"",
4242
"watch:extension": "node esbuild.mjs --watch",

0 commit comments

Comments
 (0)