feat(security): atomic writes, input validators, and static analysis tooling

[brooksc]

Overview

Standalone security utilities and static analysis tooling — no coordinator logic, no UI. This is PR 1 of 4 splitting #100 as requested in the round-4 review.

PR sequence:

PR	Branch	Contents
1 (this PR)	coordinator-1-security	Atomic writes, input validators, static analysis configs
2	coordinator-2-mcp-backend	MCP coordinator engine + REST API hardening
3	coordinator-3-store-ipc	Frontend store wiring + IPC handlers
4	coordinator-4-ui	UI components + coordinator entry points

PRs 2–4 are stacked on each other; nothing coordinator-related is user-visible until PR 4 adds the NewTaskDialog checkbox and Settings toggle.

---

What's in this PR

electron/mcp/atomic.ts

Crash-safe file writes via temp file + rename:

• Temp file written to same directory as target (avoids EXDEV across filesystem boundaries)
• fsync before rename for file durability; directory fsync after rename for directory-entry durability (platform-safe catch for Windows/network mounts)
• Sync variant for use in catch blocks; async variant for normal paths

electron/mcp/validation.ts

• validateBranchName() — mirrors git check-ref-format --branch: rejects shell metacharacters (:, ^, ~, etc.), double dots, path components starting with ., any path component ending in .lock, and other git-illegal patterns
• validateUUID() — enforces v4 UUID (version nibble=4, variant nibble∈{8,9,a,b})

Static analysis configs

• .semgrep/ — rules for unsafe Electron APIs, IPC auth, and unsafe fs operations (scoped to electron/mcp/** + electron/ipc/register.ts)
• .gitleaks.toml — token/secret leak detection
• .dependency-cruiser.cjs — architecture linting
• knip.config.ts — dead code detection

Note: the static analysis tools (semgrep, gitleaks, dependency-cruiser, knip) are not yet wired into CI — they require system/devDependency setup that lands in PR 2. Configs are included here as they're used to validate the coordinator code in PR 2.

Tests (35 cases)

• electron/mcp/validation.test.ts — accepted names, all rejection cases, UUID v4 enforcement
• electron/mcp/atomic.test.ts — write, overwrite, mode, no temp-file residue

---

Test plan

• npm run typecheck && npm test && npm run lint — all pass
• electron/mcp/validation.test.ts and electron/mcp/atomic.test.ts — 35/35

🤖 Generated with Claude Code

[johannesjo]

Thanks for splitting this out. I took another careful pass over the PR and found a few things worth addressing before merge:

1. electron/mcp/atomic.ts:50 / electron/mcp/atomic.ts:83 - overwriting an existing file without options.mode replaces the old inode with a temp file created under the process umask. That means an existing 0600 token/config file can silently become 0644 after an atomic overwrite. I reproduced this locally. Please preserve the existing target mode by default when the file exists, or default these helpers to a restrictive mode for security-sensitive writes, and add a regression test for overwriting a 0600 file.

2. electron/mcp/validation.ts:4 - validateBranchName('HEAD') currently passes, but git check-ref-format --branch HEAD rejects it. Since this helper is documented as mirroring Git branch validation, please reject HEAD explicitly and add a test case.

3. .semgrep/filesystem-safety.yml:3 - direct-writefile-in-mcp-coordinator only matches unqualified writeFileSync(...), but the code this rule is scoped to uses fs.writeFileSync(...) (electron/ipc/register.ts:586 is an existing example). As written, this guardrail will miss the main pattern it is intended to catch. Please add a fs.writeFileSync(...) pattern, likely via pattern-either.

4. Non-blocking but confusing: the new Semgrep/Gitleaks/Knip/dependency-cruiser configs are not installed, scripted, or run in CI yet (package.json scripts and .github/workflows/ci.yml only run typecheck/lint/format). The PR body calls this out for CI, but knip.config.ts:7 also references electron/mcp/server.ts, which does not exist in this PR. If these configs are intentionally staged for the next PR, please either add a clear TODO/comment in the config or defer future-file references to the PR that introduces them.

Verification run locally on the PR head:

• npm run compile passed
• npm run lint passed
• npm run typecheck passed
• npm run format:check passed
• npm test -- electron/mcp passed, 35 tests

[brooksc]

Thanks for the thorough review. Here's how each issue was addressed in the updated commit (a1ad1b4):

1. Atomic overwrite silently downgrades file mode (high)

Root cause: passing mode to open()/openSync() is subject to the process umask, so 0600 becomes 0644 under a 0022 umask.

Fix: open the temp file without a mode argument, then call fchmodSync(fd, mode) / fh.chmod(mode) immediately after — fchmod bypasses the umask and sets exact bits. Added resolveMode/resolveModeAsync helpers that stat the existing target and inherit its current mode when the caller doesn't pass one, so overwriting a 0600 file without specifying mode preserves 0600.

New regression tests cover:

• Overwrite a 0600 file without mode option → mode stays 0600
• Write with mode: 0o666 under forced umask(0o022) → mode is exactly 0o666, not 0o644

Both async and sync variants.

2. validateBranchName('HEAD') passes when git rejects it (medium)

Fix: replaced the broad RESERVED_GIT_NAMES set (which incorrectly blocked FETCH_HEAD, ORIG_HEAD, etc.) with a single if (value === 'HEAD') guard. Verified with git check-ref-format --branch — HEAD exits 1 (rejected), FETCH_HEAD/ORIG_HEAD exit 0 (accepted). Tests updated: HEAD is rejected, FETCH_HEAD and ORIG_HEAD are accepted.

3. Semgrep rule misses fs.writeFileSync(...) (medium)

Fix: replaced the single pattern: writeFileSync($PATH, $DATA, ...) with pattern-either covering both writeFileSync(...) and fs.writeFileSync(...).

4. knip.config.ts forward-references electron/mcp/server.ts which doesn't exist yet (non-blocking)

Fix: added an inline comment on that entry:

'electron/mcp/server.ts', // added in coordinator-2-mcp-backend; listed here so knip tracks it from the start

The Semgrep/Gitleaks/Knip/dependency-cruiser configs being staged-but-not-wired-to-CI is intentional and called out in the PR description — they land as infrastructure groundwork that the follow-on PRs will wire up.

[johannesjo]

Thanks for the updates. I took another pass over the current head (3c04783) and found a few follow-up issues worth addressing before this gets used by the coordinator work:

1. electron/mcp/atomic.ts:83 / electron/mcp/atomic.ts:120 - the temp file is still created with the process-default mode before being tightened with fchmod/chmod. For an overwrite that preserves an existing 0600 target, the temp file can briefly exist as 0644 under a common 0022 umask before the chmod runs. That leaves a small but real exposure window for exactly the secret/config-file case this helper is meant to make safer. Please pass the resolved mode into open/openSync when it is known, then keep the chmod afterward to correct for umask when the requested final mode is wider.

2. electron/mcp/atomic.ts:85 - the sync path uses writeSync(fd, data) and ignores the returned byte count. If writeSync short-writes, the helper will fsync and rename a truncated temp file over the target. The async variant uses FileHandle.writeFile, so the sync variant should use writeFileSync(fd, data) or loop until all bytes are written.

3. .semgrep/electron-security.yml:30 - no-shell-true-in-spawn only matches child_process.spawn(...), but this repo commonly imports spawn directly (electron/ipc/register.ts, electron/ipc/git.ts, etc.). As written, the rule would miss spawn(..., { shell: true }) in the prevailing local style. Please add a named-import pattern as well.

4. The Semgrep configs are broader than the PR description says and look likely to fail the current baseline once wired. For example, .semgrep/ipc-auth.yml:13 scans all electron/** and should match the existing ?token=${token} URLs in electron/remote/server.ts; .semgrep/electron-security.yml:12 scans all src/** for innerHTML assignments and should match existing renderer code such as the Mermaid render path. If the intent is coordinator-only guardrails for the stacked PRs, these should be scoped or allowlisted before CI starts running them.

Verification on the PR head:

• npm run typecheck passed
• npm run lint passed
• npm run format:check passed
• npm test -- electron/mcp/validation.test.ts electron/mcp/atomic.test.ts passed, 42 tests

[brooksc]

Thanks for the detailed follow-up. All four issues are addressed in the updated head (60bdf5a).

---

1. Temp-file mode exposure window (electron/mcp/atomic.ts)

Was: openSync(tmp, 'w') created the temp file with process-default mode (0o666 & ~umask, typically 0o644), then fchmodSync tightened it — leaving a brief window at 0o644 for a 0o600 target.

Fix: Now passes the resolved mode to openSync/open as the creation argument first (openSync(tmp, 'w', mode)), so the temp file starts at mode & ~umask (for 0o600 this is 0o600 regardless of umask). The fchmod/chmod call is kept immediately after to correct for umask when the final mode is wider — matching the pattern you suggested.

---

2. writeSync short-write (electron/mcp/atomic.ts)

Was: writeSync(fd, data) ignores the returned byte count, risking a truncated write being fsync'd and renamed over the target.

Fix: Replaced with writeFileSync(fd, data), which loops internally until all bytes are written. The async path was already correct (FileHandle.writeFile).

---

3. no-shell-true-in-spawn missing named-import pattern (.semgrep/electron-security.yml)

Was: pattern: child_process.spawn(...) only matched the qualified form — spawn(...) (the prevailing local style via direct import) was silently missed.

Fix: Replaced the single pattern: with pattern-either: covering both child_process.spawn($CMD, $ARGS, {shell: true, ...}) and spawn($CMD, $ARGS, {shell: true, ...}). Also updated the object pattern to {shell: true, ...} so it matches when additional keys (e.g. stdio) are present.

---

4. Semgrep rules producing false positives on existing baseline (.semgrep/)

no-inner-html-without-sanitize: Narrowed from **/src/** to **/electron/**. The existing renderer-side innerHTML assignments (Mermaid SVG, highlighted HTML, SolidJS innerHTML= prop) in src/ are all safe — the rule is meant to guard the Electron main process, not the already-reviewed renderer paths.

token-embedded-in-url-template: Added paths.exclude: ['**/electron/remote/server.ts']. The three existing ?token=${token} lines there are the intentional mobile QR-code URL builder — the token variable is the mobile access token, not the coordinator token. Excluding the file preserves the guardrail for all new code while not flagging the pre-existing safe usage.

---

npm test -- electron/mcp/atomic.test.ts electron/mcp/validation.test.ts — all 42 tests pass. npm run check clean.

[johannesjo]

Thank you very much! I made some small adjustments after in a seperate commit. Hope this will be a good basis for the rest of the feature.