Skip to content

Harden file writes and git operations against symlink/option injection#48

Closed
devill wants to merge 5 commits into
mainfrom
claude/vigilant-bell-oug1h3
Closed

Harden file writes and git operations against symlink/option injection#48
devill wants to merge 5 commits into
mainfrom
claude/vigilant-bell-oug1h3

Conversation

@devill

@devill devill commented Jun 18, 2026

Copy link
Copy Markdown
Owner

Summary

This PR adds security hardening to prevent symlink-following attacks during file writes and option-injection attacks in git operations. It also fixes unsafe property lookups that could be exploited via prototype pollution.

Key Changes

File write safety (safe-fs.ts, safe-fs.test.ts)

  • Introduce safeWriteFileSync and safeCopyFileSync that use O_NOFOLLOW flag to prevent writes through symlinks
  • Reject attempts to write through symlinks with a new SymlinkWriteError exception
  • Prevents malicious working trees from redirecting writes outside the project root (e.g., to executable git hooks)
  • Replace all writeFileSync calls with safeWriteFileSync across the codebase (git hooks, baseline storage, config scaffolding, package scripts, jscpd config, skill installation)

Git operation safety (git/scope.ts, git/scope.test.ts)

  • Add safeRef validation to reject git revision arguments starting with - (which would be parsed as options)
  • Prevents option-injection attacks like --output=/tmp/pwn being passed as a commit hash
  • Apply validation to getChangedVsCommit and getMergeBase calls
  • Add -- separator in diff command to terminate option parsing
  • New UnsafeRefError exception for rejected refs

Tool bin path containment (detect/tool.ts, detect/tool.test.ts)

  • Add containedBin validation to reject package.json#bin paths that escape the package directory
  • Prevents RCE via malicious tool package.json entries like "bin": "../../evil.js"
  • Test case verifies that out-of-bounds bin paths cause tool detection to fail safely

Safe property lookups

  • Replace unsafe map?.[key] ?? fallback patterns with Object.hasOwn(map, key) checks in:
    • src/sensors/adapter.ts (smell mapping)
    • src/checks/eslint-wrap.ts (ESLint smell mapping)
    • src/checks/knip-wrap.ts (Knip smell mapping)
  • Prevents prototype pollution attacks where a malicious tool emits code like "constructor" that would incorrectly resolve to Object.prototype.constructor
  • Test case verifies that constructor as a smell ID is handled correctly

Rule ID slugification (prompts/slug.ts, prompts/slug.test.ts)

  • Extract slugify function to dedicated module (previously inline in loader.ts and registry.ts)
  • Enhance to replace both / and \ path separators (in addition to : and @)
  • Prevents path traversal via crafted rule IDs like ../../etc/passwd when mapping to prompt files
  • Test cases verify neutralization of both Unix and Windows path separators

Implementation Details

  • O_NOFOLLOW flag closes the TOCTOU window without requiring separate existence checks
  • Dangling symlinks are caught by ELOOP error on open, preventing writes to arbitrary locations
  • Git ref validation happens before command construction, preventing injection into git arguments
  • Bin path containment uses relative() to detect escape attempts (.. prefixes or absolute paths)
  • Property lookups use Object.hasOwn() instead of optional chaining to avoid prototype chain traversal

https://claude.ai/code/session_011JpXA4jQJRGPcMfpC3nmmA

claude added 5 commits June 17, 2026 21:04
`detectTool` read `bin` from node_modules/<tool>/package.json and joined it
to the package dir with no containment check, then spawned the result (a
`.js` path runs via node). A repo committing
`node_modules/eslint/package.json` with `"bin": {"eslint":"../../evil.js"}`
could make habit-hooks execute an arbitrary committed file on clone + run
(pre-commit/CI), with no `npm install` required.

Resolve the bin and reject any path that lands outside the tool's own
package directory; the tool is then treated as undetected (bundled fallback
is used) instead of spawning attacker-controlled code.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JpXA4jQJRGPcMfpC3nmmA
Every `init` writer and the baseline save did `existsSync(path)` then
`writeFileSync`/`copyFileSync`. Both follow symlinks, so a dangling symlink
committed in the working tree (target absent, so the existsSync guard sees
nothing) redirected the write outside the project root — including the git
pre-commit hook, which was also chmod'd 0o755, planting an executable at an
attacker-chosen path.

Add a shared safe-fs helper that opens with O_NOFOLLOW (fails closed with
SymlinkWriteError when the final path component is a symlink, no TOCTOU) and
route the config/eslint/knip/jscpd/ruff scaffolders, package.json scripts,
recommendation merge, baseline save, git hook, and skill copy through it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JpXA4jQJRGPcMfpC3nmmA
`branchBase`/`mainBranch` (from a committed habit-hooks.config.json, a pure
data file) and `--since` flowed unvalidated into `git merge-base` and
`git diff`. A value beginning with `-` is parsed by git as an option, not a
revision — and `git diff --name-only --output=<file> HEAD` writes an
arbitrary file. A trailing `--` does not help here because the revision sits
in the option position, so validate instead: refuse any ref starting with
`-` (UnsafeRefError).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JpXA4jQJRGPcMfpC3nmmA
`MAP[ruleId] ?? ruleId` returned an inherited member when a tool emitted a
rule id of `constructor`/`toString`/`__proto__`, so the `?? ruleId` fallback
never fired and the resolved smell became a function/object that stringified
to `[object Object]` in output. Use Object.hasOwn so only own keys map and
unknown rule ids pass through unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JpXA4jQJRGPcMfpC3nmmA
`slugify` replaced `/` but not `\`, so on Windows a crafted rule/smell id
could traverse out of the prompts directory when joined to it. Replace both
separators and extract the (previously duplicated) helper into one module so
loader and registry stay in sync. Not reachable from tool output today —
unknown smells never hit the file lookup — but defensive if routing changes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JpXA4jQJRGPcMfpC3nmmA
@devill devill closed this Jun 18, 2026
devill added a commit that referenced this pull request Jun 18, 2026
extractIssues / eslint-wrap / knip-wrap mapped a tool's smell id with
`map[id] ?? id`. When a tool emits an id that collides with an
Object.prototype member (`constructor`, `toString`, `__proto__`), the
lookup returns the inherited member, the `?? id` fallback never fires,
and the resolved smell stringifies to `[object Object]` in output.

Use Object.hasOwn so only own keys map and every other id passes through
unchanged. This is a correctness fix for untrusted tool output, not a
privilege/security boundary.

Cherry-picked from PR #48 (the rest of that PR was hardening against a
synthetic "malicious working tree" threat model that does not fire in
habit-hooks' actual usage, and is being dropped).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@devill devill deleted the claude/vigilant-bell-oug1h3 branch June 19, 2026 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants