fix(dashboard): guard git review credential diffs

[necoweb3]

Summary

The dashboard Git/review API could disclose credential files through git diff surfaces even though the dashboard file browser and file preview routes now block those same paths.

Affected routes include:

• GET /api/git/review/diff
• GET /api/git/file-diff
• GET /api/git/review/commit-context
• GET /api/git/status / GET /api/git/review/list line-count reads for untracked files

For untracked files, web_git.py synthesizes an all-add diff using:

git diff --no-index -- <os.devnull> <file_path>

[egilewski]

suggesting changes

The new guard still lets the git review diff surfaces disclose credential contents when a protected file is renamed to a safe-looking destination. review_diff() and file_diff_vs_head() check only the requested destination path before running git diff -- safe.txt, and review_commit_context() filters the _walk_entries() paths after that walker has already dropped the porcelain-v2 rename origin record. I reproduced this on the PR head with a synthetic tracked .env containing OPENAI_API_KEY=sk-secret, then git mv .env safe.txt; direct calls to hermes_cli.web_git.review_diff(..., "safe.txt", ...), file_diff_vs_head(..., "safe.txt"), and review_commit_context(...) all returned output containing the synthetic secret. That leaves the protected credential file readable through the review diff and commit-context endpoints after a rename.

Security evidence: authenticated dashboard Git/review API calls can read working-tree and commit-context diffs from a user-selected repository; protected credential file contents must not be exposed through review diff surfaces even when Git reports a rename with a non-protected destination; current main discloses direct .env diffs and the rename case; the PR head blocks direct untracked/tracked .env diff and commit-context reads but still discloses the rename destination case; focused web-git tests passed locally as 13 passed; and the residual bypass probe asserted the imported PR module path before exercising the synthetic .env rename.

Signed: GPT-5.5-xhigh in Codex

[alt-glitch]

Priority corrected: P1 → P3 (triage calibration).

Assessed against the repo SECURITY.md: this is §3.2 out-of-scope hardening, not a
§3.1 boundary crossing. Guards git-review credential diffs — disclosure is only to the same authenticated operator (inside the envelope). NOTE: also rename-bypassable — incomplete fix.

The dashboard is loopback-by-default and its caller is the operator/agent already
inside the trust envelope, so an in-process guard over operator-owned files does not
cross an OS-isolation boundary or expose an unauthorized external surface. Per the
"PoC or GTFO" policy, type/security alone does not earn P0/P1.

Keeping type/security (this is valid low-priority hardening) and adding needs-repro.
To re-escalate: attach a reproducible exploit reachable by an unauthorized external
party and state the in-scope class, i.e. security-scope: in-scope §3.1 <class> with the
attacker path. Without that, priority stays P3.

[necoweb3]

Updated the branch to handle rename/copy origins.

The git review diff guard now keeps the UI/display path as the rename destination, but tracks all porcelain-v2 paths for safety decisions. Diff, file-diff, and commit-context now reject the safe-looking destination when it originated from a protected path.

Validation:
python -m pytest tests/hermes_cli/test_web_server_git.py -q -k "env or review_diff or commit_context" --timeout-method=thread
5 passed, 9 deselected

[egilewski]

suggesting changes

The rename-origin guard still only consults the working-tree status for the requested display path, so it covers an uncommitted rename row but misses the same credential path once the rename is committed on a branch and the worktree is clean. On a run-owned patch replay against current GitHub main 7203898ce47c9ab90e64866d6cff0e6e9ad8d1cc, I created a synthetic repo, committed .env with OPENAI_API_KEY=sk-review-secret, checked out a feature branch, ran git mv .env safe.txt, committed that rename, and then called hermes_cli.web_git.review_diff(repo, "safe.txt", "branch", None, False). That returned a branch diff containing the synthetic secret, so GET /api/git/review/diff?scope=branch&file=safe.txt can still disclose a protected credential file after it is renamed to a safe-looking destination in branch history.

The submitted branch is stale relative to current GitHub main, so I replayed the PR patch onto the current main tree for validation; this does not by itself prove the submitted branch merges cleanly, although git merge-tree --write-tree 7203898ce47c9ab90e64866d6cff0e6e9ad8d1cc cf736fa3c4ce48740524e608af949d0eabd043a8 produced a tree without conflicts. The replayed tree passed git diff --check and the focused Git review tests as 5 passed, 9 deselected; the synthetic probe showed direct .env and uncommitted-rename cases fixed, but committed_branch_rename_dotenv_to_safe still failed because _status_paths_for_display_path() relies on git status --porcelain=v2 -z, which no longer carries rename-origin data after the branch commit.

Signed: GPT-5.5-xhigh in Codex

[teknium1]

Thanks for extending the guard to uncommitted rename origins. The direct-path and dirty-worktree cases are covered, and the approach reuses the existing agent.file_safety.get_read_block_error() policy.

Problems

• hermes_cli/web_git.py:358 checks rename origins only through current git status. After committing .env → safe.txt, the worktree is clean, so the helper falls back to safe.txt; review_diff(..., scope="branch") then runs the historical range diff. Current main executes that branch diff at hermes_cli/web_git.py:311-313.
• The added rename test performs git mv but does not commit it or exercise scope=branch, so it cannot catch that bypass.

Suggested changes

• For historical scopes, inspect rename/copy metadata from the exact diff range and reject when either source or destination is protected; keep working-tree status only for working-tree scopes.
• Add a committed, clean-worktree rename regression test for /api/git/review/diff?scope=branch&file=safe.txt.

Automated hermes-sweeper review.

[teknium1]

This lookup only sees working-tree status. After .env is renamed to safe.txt and committed, status is clean and the helper falls back to safe.txt, so scope="branch" still emits the historical rename diff. Derive both rename endpoints from the requested diff range for historical scopes.

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address the dashboard Git/review credential-path hardening gap through complementary surfaces: #58036 suppresses protected content from diff, status-count, and commit-context reads, while #58040 blocks protected-path staging, reverting, and committing. Both diffs cover direct paths and Git-detected rename origins but retain documented bypasses outside their current regression coverage.

Related pull requests

• #58036 related — (+145/-12) — keep open, changes requested: The diff applies the existing read-protection policy to untracked counts, review/file diffs, commit context, and porcelain-v2 rename origins, addressing direct-path and dirty-worktree disclosure. Consistent with the keep_open review on #58036, it does not inspect rename metadata from the historical branch diff range, so a committed .env → safe.txt rename with a clean worktree can still disclose the protected content.
• #58040 related — (+167/-14) — keep open, changes requested: The diff rejects targeted protected-path mutations, replaces bulk stage/revert/auto-stage operations with filtered path lists, checks Git-reported rename origins, and maps denials to HTTP 403. Consistent with the keep_open review on #58040, a plain filesystem move appears as a protected tag-1 deletion plus an unrelated untracked safe-looking destination, allowing that destination to remain eligible for bulk stage, revert, or commit.

Suggested consolidation

Do not merge either PR yet: keep #58036 open until historical diff-range rename/copy origins are checked and covered by a committed clean-worktree branch-diff regression test, and keep #58040 open until deletion-plus-untracked filesystem moves fail closed across bulk stage, revert, and auto-stage commit with matching tests. The PRs are complementary rather than duplicates—#58036 covers read/disclosure paths and #58040 covers mutation paths—so neither should be closed in favor of the other.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 19 kB of PR diffs, 3 kB of issue/PR text, 12 kB of discussion (10 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.