ADE-90: Desktop/CLI security hardening: production renderer CSP, OS-bind the credential-store key, chmod the built-in-browser bridge socket

[arul28]

Fixes ADE-90

Summary

• Tighten packaged renderer CSP by removing production script-src 'unsafe-inline' and the blanket connect-src https: allowance while keeping the dev-only Vite inline-script allowance.
• OS-bind the encrypted file credential store key with macOS Keychain-derived material, add legacy ciphertext migration, and route desktop credentials through Electron safeStorage when available.
• Harden the built-in browser bridge Unix socket by enforcing private directory/socket modes, covering pre-existing directories, tightening umask during listen, and failing closed if socket chmod fails.

Validation

• npm --prefix apps/desktop run test -- src/main/rendererCsp.test.ts src/main/services/builtInBrowser/desktopBridgeServer.test.ts
• npm --prefix apps/ade-cli run test -- src/services/credentials/credentialStore.test.ts
• npm --prefix apps/ade-cli run typecheck
• npm --prefix apps/desktop run typecheck
• git diff --check

Risks

• Existing legacy credential files are migrated opportunistically on successful reads; if OS-bound material is unavailable for a migrated file, ADE now surfaces a clear unlock/credential-store availability error instead of silently falling back to the wrong format.
• style-src 'unsafe-inline' intentionally remains unchanged for existing renderer styling behavior.

Linked Linear issues

• ADE-90: Desktop/CLI security hardening: production renderer CSP, OS-bind the credential-store key, chmod the built-in-browser bridge socket - closes on merge

Open in ADE · lane branch · PR #490

Greptile Summary

This PR applies three hardening changes to the ADE desktop and CLI: (1) the packaged renderer CSP drops script-src 'unsafe-inline' and the blanket connect-src https: in production, (2) the file-based credential store derives its encryption key by HKDF-mixing the machine key with macOS Keychain-retrieved material, and (3) the built-in browser bridge Unix socket directory is chmod'd unconditionally and a restrictive umask is applied before server.listen().

• CSP tightening – removes 'unsafe-inline' from script-src in packaged builds and drops https: from connect-src in both prod and dev; dev mode retains 'unsafe-inline' for Vite preambles.
• Credential-store OS binding – introduces EncryptedFileCredentialStore.keyMaterialProvider, a macOS Keychain round-trip via security(1) using stdin (avoiding CLI argument exposure), HKDF derivation, and a transparent in-place migration from legacy machine-key-only files.
• Socket hardening – chmodSync(socketDir, 0o700) now runs unconditionally after mkdirSync to harden pre-existing directories, and process.umask(0o177) is set before listen() so the socket file itself is created restricted from the start.

Confidence Score: 4/5

Safe to merge with minor follow-up; the three previously flagged issues are all addressed, and no new blocking problems were found.

The credential-store migration logic is complex (two independent fallback chains that interact), and the keyMaterialProvider can be called twice within a single readAll→writeAll round-trip via the migration path. For the default in-process provider this is harmless due to the module-level cache, but the pattern is fragile for injected providers. The socket and CSP changes are clean and well-tested.

apps/ade-cli/src/services/credentials/credentialStore.ts — the migration write path in readAll() re-invokes keyMaterialProvider through writeAll, and the keychain bootstrap doesn't validate the round-trip after spawnSync succeeds.

Important Files Changed

Filename	Overview
apps/ade-cli/src/services/credentials/credentialStore.ts	Adds OS-bound key derivation, macOS Keychain bootstrap via stdin pipe (addressing prior CLI-arg-exposure comment), and safeStorage migration with hasMagic guard; previously flagged fallback-on-any-error is resolved.
apps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.ts	Socket hardening: unconditional chmodSync(socketDir, 0o700) fixes pre-existing-directory gap, umask 0o177 before listen mitigates TOCTOU, and chmod failure now closes the server (fail-closed).
apps/desktop/src/main/rendererCsp.ts	Removes 'unsafe-inline' from production script-src and drops blanket https: from connect-src; dev-mode retains 'unsafe-inline' for Vite.
apps/desktop/src/main/main.ts	Replaces four hardcoded EncryptedFileCredentialStore instantiations with createDesktopCredentialStore, which prefers ElectronSafeStorageCredentialStore and fails closed if safeStorage is unavailable but a safeStorage-format file exists.
apps/ade-cli/src/services/credentials/credentialStore.test.ts	Adds tests for OS-bound key derivation, legacy migration paths, and the safeStorage no-fallback-after-magic guarantee; coverage is solid for the new paths.
apps/desktop/src/main/rendererCsp.test.ts	Adds assertions for the tightened production CSP (no unsafe-inline in script-src, no https: in connect-src) and updates existing tests accordingly.
apps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.test.ts	New test file; exercises socket directory chmod on pre-existing 0o755 dirs and verifies umask is set before listen and restored after.

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---### Issue 1 of 2
apps/ade-cli/src/services/credentials/credentialStore.ts:303-325
`readAll()` migration silently writes with OS-bound key, but `writeAll()` internally calls `readOrCreateMachineKey` again. More importantly, `writeAll()` could silently fail (`catch {}`) and the values are returned — fine. But there's a different subtle problem: the migration write re-reads the `keyMaterialProvider()`. If the provider returns a different buffer on the second call (e.g., a one-shot env passphrase that gets consumed, or a cache miss returning a fresh Keychain read), the written key would differ from the one used on the first `readAll()` call inside `writeAll()`. The `cachedDefaultOsBoundKeyMaterial` prevents this for the default provider, but an externally-supplied `keyMaterialProvider` has no such guarantee. Capturing the resolved material once at the start of `readAll` and reusing it in the same invocation is safer.
```suggestion private readAll(): Record<string, string> { const raw = readJsonObject(this.credentialsPath); const machineKey = readOrCreateMachineKey(this.machineKeyPath); const osMaterial = this.keyMaterialProvider(); const key = deriveOsBoundCredentialKey(machineKey, osMaterial); try { return deserializeStore(raw, key); } catch (error) { if (key.equals(machineKey)) throw error; const values = deserializeStore(raw, machineKey); try { this.writeAllWithKey(values, key); } catch { // Preserve read compatibility if migration cannot rewrite right now. } return values; } } private writeAll(values: Record<string, string>): void { const machineKey = readOrCreateMachineKey(this.machineKeyPath); const key = deriveOsBoundCredentialKey(machineKey, this.keyMaterialProvider()); this.writeAllWithKey(values, key); } private writeAllWithKey(values: Record<string, string>, key: Buffer): void { writeFileAtomic(this.credentialsPath, `${JSON.stringify(serializeStore(values, key), null, 2)}\n`); }```### Issue 2 of 2
apps/ade-cli/src/services/credentials/credentialStore.ts:222-223
When `readOrCreateMacKeychainMaterial()` creates a new keychain entry, it reads the secret back indirectly by reusing the generated `secret` string. But if `spawnSync` succeeds (status 0) and the tool did not actually consume stdin as the password (e.g., a future macOS version changes `-w` stdin behaviour), the stored value could silently be empty. The subsequent call to `execFileSync("security find-generic-password ... -w")` would then return an empty string, causing `decoded.length >= 32` to be false and falling to `Buffer.from("", "utf8")`, which triggers the `osMaterial.length === 0` guard and silently disables OS binding. Consider adding an explicit `decoded.length < 32` guard on the stored-value path so that an unexpectedly short round-trip throws rather than silently falling back to no OS binding.
```suggestion if (result.status !== 0) return null; const decoded = Buffer.from(secret, "base64"); if (decoded.length < 32) return null; return decoded;```

_{Reviews (3): Last reviewed commit: "Harden existing browser bridge socket di..." | Re-trigger Greptile}

[linear-code]

ADE-90

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
ade	Ignored	Preview	May 31, 2026 10:39am

[arul28]

@copilot review but do not make fixes

[capy-ai]

Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews.

[coderabbitai]

Warning

Review limit reached

@arul28, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 13 minutes and 47 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1a86192c-35ac-4cce-a59c-149603f5c16d

📥 Commits

Reviewing files that changed from the base of the PR and between ca46616 and acb6163.

📒 Files selected for processing (7)

• apps/ade-cli/src/services/credentials/credentialStore.test.ts
• apps/ade-cli/src/services/credentials/credentialStore.ts
• apps/desktop/src/main/main.ts
• apps/desktop/src/main/rendererCsp.test.ts
• apps/desktop/src/main/rendererCsp.ts
• apps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.test.ts
• apps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ade-90-desktop-cli-security-hardening-production-renderer-csp-os-bind-the-credential-store-key-chmod-the-built-in-browser-bridge-socket

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cursor]

PR Review

Scope: 7 file(s), +320 / −26
Verdict: Minor issues

This PR tightens production renderer CSP (drops blanket https: in connect-src and packaged unsafe-inline scripts), OS-binds file-store encryption via macOS Keychain HKDF on Darwin, routes desktop credential access through safeStorage with legacy migration, and hardens the built-in browser bridge Unix socket directory/socket modes. The direction is sound; a couple of availability edge cases around credential fallback are worth addressing before merge.

---

🐛 Functionality

[Medium] OS-bound credential file unreadable when key material is unavailable

File: apps/ade-cli/src/services/credentials/credentialStore.ts:L263-L278
Issue: After credentials are re-encrypted with the OS-bound HKDF key, readAll() only falls back to the on-disk .machine-key when the derived key differs from machineKey. If keyMaterialProvider() returns null (locked macOS Keychain, ADE_CREDENTIAL_STORE_DISABLE_OS_BINDING=1, or non-Darwin), decryption uses machineKey and fails with no legacy fallback—reads throw and stored tokens are unavailable until OS material is available again.
Repro: On macOS, write credentials with default OS binding; lock the login Keychain or run with ADE_CREDENTIAL_STORE_DISABLE_OS_BINDING=1; call getSync on the same secretsDir (CLI daemon or desktop file-store fallback).
Fix: On decrypt failure, attempt deserializeStore(raw, machineKey) whenever OS material is non-null (not only when !key.equals(machineKey)), or keep a version flag in the envelope so pre-bound ciphertext remains readable during keychain lock.

[Medium] safeStorage-migrated blob incompatible with file-store fallback

File: apps/desktop/src/main/main.ts:L376-L389
Issue: createDesktopCredentialStore() returns a bare EncryptedFileCredentialStore when safeStorage.isEncryptionAvailable() is false. If credentials.json.enc was already rewritten as a safeStorage ciphertext, the file store path tries AES-GCM JSON deserialization and fails—there is no path to read the safeStorage blob without Electron.
Repro: On macOS, persist credentials via ElectronSafeStorageCredentialStore; later start ADE when safeStorage.isEncryptionAvailable() is false (Keychain unavailable / headless mis-detect); observe credential reads error despite the file existing.
Fix: When safeStorage is unavailable, still wrap with a read-only migration path (or detect safeStorage magic/format and surface a clear “unlock Keychain” error), or avoid returning the AES store alone once the file is in safeStorage format.

---

🔒 Security

[Low] Unix socket may retain permissive mode if chmod fails

File: apps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.ts:L119-L128
Issue: The bridge sets 0o600 on the socket in the listen callback, but only logs on failure. A default umask can leave the socket group/world accessible on multi-user hosts, weakening the hardening this PR adds.
Attack path: Local attacker on a shared machine connects to a world-readable desktop-bridge.sock and invokes allowlisted built_in_browser.* JSON-RPC (browser control) without going through ADE UI.
Fix: Treat chmod failure as fatal for Unix sockets (stop listening / retry), or set umask(0o077) before listen and verify mode before advertising the socket path to the daemon.

---

Notes

• CSP changes align with actual renderer network usage (IPC + localhost simulator paths); tests document the intended packaged vs dev split.
• OS binding and safeStorage migration tests cover happy-path upgrades well; Linux/CI still rely on file-only encryption (no Darwin Keychain), which matches current platform support.
• Could not run Vitest in this environment (vitest not on PATH under apps/ade-cli); findings are from static review of the diff at b4bbcf9f.

_{Sent by Cursor Automation: BUGBOT in Versic}