fix(security): restrict settings.json permissions and stop caching the raw apiKey by raymondginger2018-sudo · Pull Request #359 · lessweb/deepcode-cli

raymondginger2018-sudo · 2026-09-25T04:00:46Z

Summary

Two secret-hygiene fixes, both about the API key being reachable outside the app:

settings.json is now written with user-only permissions.
The OpenAI client cache key no longer contains the raw API key.

Problem

writeSettingsFile in packages/core/src/settings.ts used the plain fs defaults:

fs.mkdirSync(path.dirname(settingsPath), { recursive: true });
fs.writeFileSync(settingsPath, `${JSON.stringify(settings, null, 2)}\n`, "utf8");

~/.deepcode/settings.json stores the API key, so it was created world-readable under a
permissive umask and inherited the profile ACL on Windows (where Authenticated Users
can read it by default).

packages/core/src/common/openai-client.ts cached its client as
cachedOpenAIKey = \${connection.apiKey}::${connection.baseURL}``, leaving the plaintext key
resident in module state — visible in a heap dump or crash report.

Changes

new packages/core/src/common/private-storage.ts (+122)
- writePrivateFile — 0600 on POSIX. Windows ignores mode bits, so it additionally
  drops inherited ACEs (icacls <path> /inheritance:r) and grants the current user
  exclusive full control (icacls <path> /grant:r <user>:F).
- ensurePrivateDirectory — 0700, or a current-user-only ACL on Windows.
- Both return whether the platform's permission model was applied rather than failing
  invisibly; they report, they do not throw, so a caller can still write the file when a
  tool is missing (POSIX mode bits are likewise subject to umask). The Windows principal
  is resolved from USERDOMAIN/USERNAME first — under MSYS/Git-Bash a POSIX whoami
  sits earlier in PATH and answers with a bare, ambiguous name.
packages/core/src/settings.ts (+3/-2) — writeSettingsFile routes through those helpers.
packages/core/src/common/openai-client.ts (+4/-1) — cache on
sha256(apiKey).slice(0, 16)::baseURL: still one client per key + baseURL pair, no
plaintext secret retained.
new packages/core/src/tests/private-storage.test.ts (+100) — POSIX 0600/0700 mode
bits, Windows ACL restriction, and idempotency. The Windows test asserts that no inherited
ACE survives (icacls prints :(I) for inherited entries), reads the exact principal back
(DOMAIN\user:(F)), and rejects Authenticated Users/Everyone. When icacls cannot run it
reports a skip, never a vacuous pass — a file that could not be restricted must not look
like a green ACL test.

Correction: the first push was red, and it was a real bug

The first push of this branch (2 commits) failed CI on both Windows legs:

not ok 3 - restricts Windows ACL to the current user (Windows only)
error: 'current user must retain full control'
    at private-storage.test.ts:69:12

Investigating that red found a genuine defect in the implementation, not just a brittle
assertion: restrictWindowsAcl passed the program name as the first argument, so icacls
received two operands and exited with ERROR_INVALID_PARAMETER (87), while the catch
turned that hard failure into a silent no-op — the ACL was never touched and the file stayed
as readable as its parent directory allowed. Proven by A/B on Windows:

invocation	icacls rc	resulting ACL
`icacls <path> /inheritance:r` (intended)	0	single explicit `user:(F)`
`icacls icacls <path> /inheritance:r` (what the code did)	87	unchanged, still inherited

The same defect also exposed a vacuous test: locally the ACL did still contain inherited
ACEs, yet the old assertions passed — they matched only English principal names absent from
this machine, and the (F)/(I)(F) pattern was inverted. Fixed in the third commit
(fix(security): apply the settings ACL for real and assert it), which removes the duplicated
operand, resolves the principal from the environment first, and makes both helpers report
success so the failure can no longer be swallowed. The rewritten test fails against the old
implementation and passes against the new one.

Verification

gate	result
`npm run check` (typecheck + eslint + prettier)	rc=0
`npm test` (core workspace)	396 tests, pass 386, fail 0, skipped 10
`npm run build --workspace=@vegamo/deepcode-core`	rc=0
`npm run bundle`	rc=0 — markers in `packages/cli/dist/cli.js`: `writePrivateFile` ×3, `private`+`inheritance:r` ×1, `USERDOMAIN` ×3, `sha256` present; the broken `["icacls", path]` argv form is absent
end-to-end through `writeSettings` in an isolated `USERPROFILE`	file and directory end with a single explicit ACE for the current user; no `(I)`, no `Authenticated Users`, no `Everyone`

On Windows the file additionally keeps explicit SYSTEM and BUILTIN\Administrators ACEs;
both can take ownership regardless of the DACL, so removing them would be cosmetic. What the
change removes is the inherited group access (Authenticated Users and friends) that made
the key readable to other interactive users.

The Windows ACL case was exercised locally on Windows (icacls path, executed — not skipped).
Local toolchain was Node v25.2.1 / npm v12.0.2 while CI uses Node 22/24, so local results are
supporting evidence only; the upstream CI result is what counts.

Overlap

Checked every open PR by file list (50 PRs, excluding this one): 23 files match the paths this
PR touches, but a hunk-level scan for implementation markers (icacls, inheritance:r,
chmod, 0600, sha256, createHash, writePrivateFile) finds no competing fix, and no
open PR touches packages/core/src/common/private-storage.ts. Base is up to date with main
(3 ahead / 0 behind) and a test merge is clean.

Notes

umask can only clear bits, so the explicit 0600/0700 can never end up more permissive
than intended; the Windows ACL path is independent of umask.
Scoped deliberately: only the two secret-handling fixes. No session, MCP or bash changes.
This re-submits the secret-handling part of a PR I closed earlier (fix(security): private settings permissions + hash apiKey cache key #267); the unrelated
hardening commits from that branch are not part of this diff.

`writeSettingsFile` created the settings directory with `fs.mkdirSync(..., { recursive: true })` and wrote `settings.json` with `fs.writeFileSync` defaults. The file therefore ended up world-readable under a permissive umask, and `Authenticated Users` could read it under the default Windows profile ACL. `settings.json` stores the API key. Add `common/private-storage.ts` with two helpers: - `writePrivateFile` — 0600 on POSIX; on Windows, mode bits are ignored by the OS, so it also drops inherited ACEs (`icacls /inheritance:r`) and grants the current user exclusive full control (`icacls /grant:r <user>:F`). - `ensurePrivateDirectory` — 0700, or a current-user-only ACL on Windows, for `~/.deepcode`. Both are best-effort: when `whoami`/`icacls` is unavailable the file is still written, matching the POSIX path where mode bits are subject to umask. Tests (`tests/private-storage.test.ts`): POSIX mode bits 0600/0700, Windows ACL restriction, idempotency, and that the current user keeps full control. The Windows assertion accepts both the `(F)` and `(I)(F)` forms, because `icacls` prints inherited ACEs differently depending on the Windows build.

The module-level client cache stored `cachedOpenAIKey = `${apiKey}::${baseURL}``, so the raw API key stayed resident in module state for the lifetime of the process: a heap dump, crash report, or memory leak would contain it verbatim. Cache on `sha256(apiKey).slice(0, 16)::baseURL` instead. Behaviour is unchanged — one cached client per key and baseURL pair — but no plaintext secret is retained.

`restrictWindowsAcl` passed the program name as the first argument, so icacls received two operands ("icacls" and the target) and exited with ERROR_INVALID_PARAMETER (87). The failure was swallowed as best-effort, so the ACL was never touched while the caller kept going: on Windows the settings file (which stores the API key) stayed readable by whatever the parent directory grants, and the 0600 intent was silently unmet. - drop the duplicated "icacls" operand - resolve the principal from USERDOMAIN/USERNAME before falling back to whoami, which under MSYS/Git-Bash answers with a bare, ambiguous name - return whether the ACL is now user-only instead of failing invisibly - tests: assert that no inherited ACE survives (this is the assertion that catches the no-op), read the exact principal back instead of matching a loose pattern, and skip loudly when icacls cannot run Verified on Windows: the file and its directory now carry a single explicit ACE for the current user, with no Authenticated Users/Everyone entry.

raymondginger2018-sudo · 2026-09-25T04:22:46Z

Correction — the first push of this branch was red on Windows, and it exposed a real bug

The initial 2-commit push failed the Test step on both Windows legs:

not ok 3 - restricts Windows ACL to the current user (Windows only)
error: 'current user must retain full control'
    at private-storage.test.ts:69:12

Root cause (not a flaky test): restrictWindowsAcl passed the program name as the first
argument, so icacls received two operands and exited with ERROR_INVALID_PARAMETER (87).
The catch swallowed that hard failure, so the ACL was never applied and the file kept
whatever its parent directory granted — the 0600 intent was silently unmet. A/B on Windows:

invocation	icacls rc	resulting ACL
`icacls <path> /inheritance:r` (intended)	0	single explicit `user:(F)`
`icacls icacls <path> /inheritance:r` (what the code did)	87	unchanged, inherited ACEs remain

The same defect proved the original test was vacuous: locally the ACL still contained
inherited ACEs and the test passed anyway, because the absence checks matched only English
principal names absent from this machine and the (F)/(I)(F) pattern was inverted. A green
test was masking a security no-op.

Fixed in 1e5d6b71:

the duplicated "icacls" operand is gone;
the principal is resolved from USERDOMAIN/USERNAME first (a POSIX whoami answers with a
bare, ambiguous name under MSYS/Git-Bash);
both helpers now return whether the platform permission model was applied instead of
failing invisibly;
the test asserts that no inherited ACE survives (the assertion that would have caught the
no-op), reads the exact principal back, and reports a skip rather than a vacuous pass when
icacls cannot run. The rewritten test fails against the old implementation.

CI at 1e5d6b71: all six legs green — run 36093691176, ubuntu/macos/windows × Node 22/24.

The description above has been updated accordingly; the earlier claim that the Windows
assertion "accepts both the (F) and (I)(F) forms" was wrong and has been removed.

raymondginger2018-sudo added 3 commits September 25, 2026 11:54

Navigation Menu

fix(security): restrict settings.json permissions and stop caching the raw apiKey - #359

fix(security): restrict settings.json permissions and stop caching the raw apiKey#359
raymondginger2018-sudo wants to merge 3 commits into
lessweb:mainfrom
raymondginger2018-sudo:pr/private-storage-and-key-hash

raymondginger2018-sudo commented Sep 25, 2026 •

edited

Loading

Uh oh!

raymondginger2018-sudo commented Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

raymondginger2018-sudo commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

Correction: the first push was red, and it was a real bug

Verification

Overlap

Notes

Uh oh!

raymondginger2018-sudo commented Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

raymondginger2018-sudo commented Sep 25, 2026 •

edited

Loading