Skip to content

Commit f164c8f

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.
1 parent f121c2e commit f164c8f

22 files changed

Lines changed: 669 additions & 1122 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
@@ -21,11 +21,10 @@
2121
replay the buffered connection logs instead. Close codes never reached the
2222
reconnect logic, so these closes retried forever. Server-initiated normal
2323
closes (`1000`/`1001`) keep reconnecting.
24-
- Repair permissions on Coder-managed Windows SSH config files, including other
25-
deployments' files matched by the shared Include. This fixes connections blocked
26-
by inherited permissions or stale account access in an unrelated config. The
27-
main SSH config and directory permissions are left unchanged. If repair fails,
28-
log a warning and still attempt the SSH connection.
24+
- Repair permissions on the Windows SSH config files the extension generates,
25+
so connections stop failing with "Bad owner or permissions". Only you,
26+
SYSTEM, and Administrators keep access to them. The extension leaves your own
27+
SSH config untouched and needs no admin rights.
2928

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

‎CONTRIBUTING.md‎

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -89,23 +89,38 @@ file to display network information.
8989

9090
### Windows SSH config permissions
9191

92-
Repair requires Windows Script Host, JScript, and ADSI to be allowed by policy.
93-
The extension does not request elevation or bypass policy. A standard user can
94-
repair files when they have access to read and change the DACL (`READ_CONTROL`
95-
and `WRITE_DAC`). If repair fails, it logs a warning and still attempts the SSH
96-
connection, which OpenSSH may then reject. Write and rename failures still stop
97-
setup. If the script fails after inheritance is disabled, copied grants remain
98-
until a successful retry; access is not reset to the parent's permissions.
99-
100-
`assets/wsh/acl.js` ships as source in the universal VSIX and runs under Windows
101-
Script Host, so it uses ES3 syntax rather than Node.js. Its sibling
102-
`tsconfig.json` and `globals.d.ts` keep the WScript and ADSI types out of the
103-
extension's Node.js environment; `pnpm typecheck` covers both. Typechecking does
104-
not transpile the asset, so keep indexed loops: `for...of` fails the ES3 lint
105-
check. A typecheck is not a runtime compatibility check, so `acl.native.test.ts`
106-
drives the real `icacls.exe`, `cscript.exe`, and `ssh.exe` instead of mocks. It
107-
runs whenever the tests run on Windows, including x64 and ARM64 in CI, and needs
108-
the OpenSSH client installed.
92+
Windows files inherit their permissions from the directory they live in, so a
93+
config the extension generates under `%APPDATA%\coder.coder-remote\ssh` can end
94+
up readable by other accounts. OpenSSH rejects such a file with "Bad owner or
95+
permissions" and skips the whole `Include`, which blocks every Coder host, not
96+
just the one it came from.
97+
98+
Before each managed write, `src/remote/windowsAcl.ts` locks the directory down
99+
and lets its files inherit from it:
100+
101+
| Step | Command |
102+
| -------------------------------------------------------- | ---------------------------------------------------- |
103+
| Read the current user's SID | `whoami.exe /user /fo csv /nh` |
104+
| Clear the directory's own grants | `icacls.exe <dir> /reset` |
105+
| Grant that user, SYSTEM, and Administrators full control | `icacls.exe <dir> /inheritance:r /grant:r <trustee>` |
106+
| Clear each `*.conf` file so it inherits the directory | `icacls.exe <file> /reset` |
107+
108+
Resetting every `*.conf` file, not only the one being written, also repairs
109+
files left behind by other deployments and editors.
110+
111+
Worth knowing:
112+
113+
- Like VS Code, the code checks exit codes but never reads ACLs back. It needs
114+
no script, native module, ownership change, or elevation, and it leaves the
115+
user's own SSH config alone.
116+
- Links and non-files are rejected before the repair, because inheritable
117+
grants reach children even without `/T`. That stops mistakes, not an attacker
118+
racing the check.
119+
- The repair is not atomic: a failure after `/reset` can leave the directory
120+
with its parent's grants.
121+
122+
`windowsAcl.native.test.ts` drives the real `icacls.exe`, `whoami.exe`, and
123+
OpenSSH. Run it unelevated as well as in CI to catch privilege assumptions.
109124

110125
## Other features
111126

‎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
@@ -77,19 +77,6 @@ export default defineConfig(
7777
},
7878
},
7979

80-
// Windows Script Host runs JScript with ES3 syntax and globals.
81-
{
82-
files: ["assets/wsh/acl.js"],
83-
languageOptions: {
84-
ecmaVersion: 3,
85-
sourceType: "script",
86-
globals: {
87-
ActiveXObject: "readonly",
88-
WScript: "readonly",
89-
},
90-
},
91-
},
92-
9380
// Package.json linting.
9481
packageJson.configs.recommended,
9582
{

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
"test:extension": "cross-env ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs --project extension",
3939
"test:integration": "pnpm build:test && node esbuild.mjs && vscode-test",
4040
"test:webview": "cross-env ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs --project webview",
41-
"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\"",
41+
"typecheck": "concurrently -g -n extension,tests,packages,storybook \"tsc --noEmit\" \"tsc --noEmit -p test\" \"pnpm typecheck:packages\" \"tsc --noEmit -p .storybook\"",
4242
"typecheck:packages": "pnpm -r --filter \"./packages/*\" --parallel typecheck",
4343
"watch": "concurrently -g -n extension,webviews \"pnpm watch:extension\" \"pnpm watch:webviews\"",
4444
"watch:extension": "node esbuild.mjs --watch",

0 commit comments

Comments
 (0)