security(step): refuse check_diff patches that write inside .git

[jdx]

https://entire.io/gh/jdx/hk/trails/162

When a step defines check_diff, hk applies the patch it prints in-process instead of running the fixer. That applier (built on diffy) accepted patches that name paths inside .git, which git apply refuses. A check_diff tool that printed (or was tricked into printing) +++ .git/hooks/post-commit could therefore plant a hook that runs on the next commit, or edit .git/config.

What changes

• Patches that name any path component git treats as its own directory are refused as a whole, before anything is written. The check mirrors git's verify_path: .git in any case, with trailing dots or spaces (.git.), NTFS alternate data streams (.git::$INDEX_ALLOCATION), the NTFS short name git~1, and .git with HFS+-ignorable Unicode inserted. Both sides of a ---/+++ pair are checked, in any directory depth (sub/.git/config). Look-alikes such as .gitignore and .github still apply.
• A refused patch behaves like any patch that does not apply: hk warns and runs the step's fixer instead.
• hk now creates its state directory (command output, saved stash patches, logs) with mode 0700 on Unix. Directories above it, such as ~/.local/state, are created as usual, and an existing state directory keeps its permissions.

Before and after, with a check_diff that prints a patch creating .git/hooks/post-commit next to a valid change:

before: the in-process applier writes .git/hooks/post-commit
after:  fmt: check_diff printed a patch that doesn't apply, so the fixer ran instead:
        .git/hooks/post-commit: patches may not touch git's own directory

Validation

• cargo test: 525 passed, 0 failed (new unit tests cover the is_dot_git spellings, both-sides refusal, nothing written on refusal, and look-alikes still applying).
• test/apply_check_diff.bats: 41 ok of a 41-test plan, for both the libgit2 and no-libgit2 runs. The new test fails when the check is disabled (I temporarily disabled it to confirm) and passes with it.
• test/state_dir_permissions.bats (new): 1 of 1 ok; the state directory is drwx------ and its parent keeps default permissions.
• scripts/validate-apple-pkl.py passes. hk check --all: all steps pass except cargo-check, which fails on this machine because of existing manifest warnings (package.homepage redundant, unused tracing-error) unrelated to this change.

Limits

Existing state directories are not tightened. Windows has no 0700 equivalent here, and the .git checks are written for all platforms but were only run on Linux. Other path-based risks, such as symlinks and hard links, were already refused before this change.

AI-assisted — Tool: Claude Code; model: anthropic/claude-sonnet-5-5; version: unavailable.

🤖 Generated with Claude Code

---

Note

High Risk
Changes security-sensitive patch application and how locally stored hook output/stash data is created on disk; incorrect matching could block legitimate diffs or still allow .git writes.

Overview
Closes a gap where the in-process check_diff patch applier could write under .git (e.g. hooks) even though git apply would reject those paths. Patches are now rejected before any write when any path component matches git’s verify_path rules—case variants, trailing dots/spaces, git~1, NTFS stream suffixes, HFS ignorable Unicode, nested paths like sub/.git/config, and both --- and +++ sides on modify hunks. Look-alikes such as .gitignore still apply; a refused patch behaves like a non-applicable patch (warning + fixer runs).

Separately, create_state_dir_all creates directories under HK_STATE_DIR with 0700 on Unix (logs, output, stash patch backups), while parents outside that tree and already-existing dirs are unchanged. Call sites for patches, log files, and HK_OUTPUT_FILE now use this helper instead of plain create_dir_all.

New unit, bats, and state_dir_permissions tests cover .git spellings, end-to-end fix behavior, and state-dir mode.

^{Reviewed by Cursor Bugbot for commit f758bfd. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• Security
  • Patches targeting Git’s internal .git directory are now rejected, while similarly named paths such as .gitignore remain valid.
• Bug Fixes
  • Directories created within the application’s state directory now receive owner-only permissions on Unix. Parent directories retain their existing permissions.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 7 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2613306c-6024-447c-afc5-8e99bbed808a

📥 Commits

Reviewing files that changed from the base of the PR and between 0b55b95 and f758bfd.

📒 Files selected for processing (7)

• src/env.rs
• src/git.rs
• src/logger.rs
• src/main.rs
• src/step/diff.rs
• test/apply_check_diff.bats
• test/state_dir_permissions.bats

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
• Review profile: CHILL
• Plan: Advanced
• Run ID: f8a634f3-5a0e-434f-a332-807f29b86c42

📥 Commits

Reviewing files that changed from the base of the PR and between 16bd59a and 0b55b95.

📒 Files selected for processing (7)

• src/env.rs
• src/git.rs
• src/logger.rs
• src/main.rs
• src/step/diff.rs
• test/apply_check_diff.bats
• test/state_dir_permissions.bats

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds restricted-permission creation for state-directory paths and uses it for patches, logs, and output files. It also rejects patch operations that target Git’s internal directory and adds regression tests for both changes.

Changes

State directory permissions

Layer / File(s)	Summary
Create and use restricted state directories src/env.rs, src/git.rs, src/logger.rs, src/main.rs, test/state_dir_permissions.bats	A shared helper creates paths within the state directory with Unix mode 0700. The patches, log, and output paths use the helper. A Bats test checks state-directory and parent-directory permissions.

Git-internal patch path protection

Layer / File(s)	Summary
Validate patch paths before application src/step/diff.rs, test/apply_check_diff.bats	Patch application rejects paths that name Git’s internal directory, including recognized platform-specific spellings and either side of a modify operation. Tests cover rejected paths and confirm that .gitignore patches remain allowed.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 0b55b

The change makes hk's state directory private on Unix and refuses patches that write inside .git. No merge-blocking risk was identified from the supplied evidence.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0b55b

The changes strengthen filesystem protections without expanding privileges. No introduced security regression was identified in the reviewed code. Existing directory permissions and crash-recovery limitations remain unchanged; cross-platform validation remains limited.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The relevant attack surface is patch text produced by a check_diff command and applied with hk’s filesystem authority. Sensitive outcomes include repository hook or configuration modification and exposure of saved changes or command output to other local users. The reviewed changes constrain those outcomes rather than add authority.

Security Findings and Attack Paths

• inferred — The new preflight checks interrupt the described direct path from crafted patch headers to Git hook or configuration writes. This is a restriction on patch application, not a sandbox for the configured check or fixer commands.

Trust Boundaries and Controls

• observed — Git-directory rejection supplements existing checks against paths outside the working directory, symlink traversal and multiple hard links. Owner-only state-directory creation adds a local confidentiality control for newly created Unix state directories.

Resilience and Maintainability Implications

• observed — Returned write errors trigger best-effort restoration, and failed restoration stops execution instead of invoking the fixer on uncertain state. Direct writes and unlocked rollback are not a durable transaction across interruption or concurrent edits. Those recovery limitations predate this PR and are not retained as introduced concerns.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 68.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: refusing check_diff patches that write inside Git’s .git directory. It is concise and specific.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.}

[greptile-apps]

Confidence Score: 5/5

[Critical risk] Blocks patches from writing to git internals.

This PR appears safe to merge; no outstanding finding or new actionable issue remains.

Summary

This PR rejects check_diff patches that name Git’s metadata directory and creates new state directories with owner-only permissions on Unix.

• Patch rejection occurs before writes and falls back to the configured fixer.
• The follow-up change restores support for a single-component relative HK_STATE_DIR.

_{Reviews (2) · Last reviewed commit: "fix(step): create a single-component rel..."}

[github-actions]

Instruction counts

Nothing was compared, and so nothing was gated. No series appears on both sides: either the base has no measurements recorded, or the two were measured on different runner classes, which are deliberately not comparable — counts shift between machine types by more than a real regression does.

New, nothing to compare against: builtins on jdx-perf-v1-ubuntu24.04-x64-hk-rust1.97.1-img5263c143, check on jdx-perf-v1-ubuntu24.04-x64-hk-rust1.97.1-img5263c143, usage on jdx-perf-v1-ubuntu24.04-x64-hk-rust1.97.1-img5263c143, validate on jdx-perf-v1-ubuntu24.04-x64-hk-rust1.97.1-img5263c143

_{Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run.}

_{Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.}

_{f758bfd51c20 vs a1a3cca13f9d · measured on the runner, not pushed to the history.}