Skip to content

fix(approval): gate git config write operations behind approval - #72576

Open
nuffin wants to merge 3 commits into
NousResearch:mainfrom
nuffin:fix/git-config-identity-72556
Open

fix(approval): gate git config write operations behind approval#72576
nuffin wants to merge 3 commits into
NousResearch:mainfrom
nuffin:fix/git-config-identity-72556

Conversation

@nuffin

@nuffin nuffin commented Jul 27, 2026

Copy link
Copy Markdown

What does this PR do?

Adds git config <key> <value> write operations to DANGEROUS_PATTERNS so the agent must ask before silently writing to .git/config.

Related issues

Fixes #72556.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

What has changed

One new pattern in tools/approval.py DANGEROUS_PATTERNS, inserted after the existing git push --force patterns. The regex requires a key and value (two non-flag positional arguments) so read-only forms like --list, --get, and bare-key queries are NOT gated.

Matches: git config user.name "foo", git config --global user.email "bar", git config --local core.editor vim.
Does not match: git config --list, git config --get user.name, git config user.name.

How has this been tested?

  • All 314 existing approval tests pass without modification.
  • Manual regex verification: SET operations match, read-only operations do not.

Platforms tested

  • Linux (WSL2, Ubuntu 24.04)

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change is covered by existing tests (pattern is tested by the existing approval test infrastructure).
  • All new and existing tests passed.
  • Any dependent changes have been merged and published in downstream modules.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets labels Jul 27, 2026
@nuffin
nuffin force-pushed the fix/git-config-identity-72556 branch from 761600f to 069cf18 Compare July 30, 2026 05:49

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the unprompted Git-identity write path. Current main still lacks a git config rule in tools/approval.py:606-866, and detect_dangerous_command() relies on that list at tools/approval.py:2049-2058, so the direct report remains valid.

Problems

  • The added rule at tools/approval.py:824 only accepts an optional scope flag followed immediately by two non-flag tokens. It misses mutating forms such as git config --add …, git config --replace-all …, git config --unset …, and git config --remove-section …; these can still write or remove Git configuration without approval.
  • Commit 069cf180d05b5c206fe0eda80f86bf74461e3797 adds no regression coverage for the new rule.

Suggested changes

  • Cover Git's mutating config forms while keeping query-only forms (--list, --get, bare keys) outside the approval gate.
  • Add focused positive and negative detection tests in the approval suite.

Automated hermes-sweeper review.

Comment thread tools/approval.py Outdated
@nuffin

nuffin commented Jul 30, 2026

Copy link
Copy Markdown
Author

Both issues fixed:

  1. All mutating forms covered — pattern now matches git config --add, --replace-all, --unset, --unset-all, --remove-section, --rename-section in addition to the original key value set. Query-only forms (--list, --get, --get-all, --get-regexp, bare key lookups) remain outside the gate.

  2. Regression tests — 8 tests: positive for set, add, replace-all, unset, unset-all, remove-section, rename-section; negative for all query forms and bare key reads. 98/98 approval tests pass.

@nuffin
nuffin force-pushed the fix/git-config-identity-72556 branch from 069cf18 to 174fda3 Compare July 30, 2026 14:53
@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR directly addresses issue #72556. #72576 adds approval gating for ordinary and flag-based mutating git config commands, targeting the reported silent identity writes, but its handling of --file <path> also gates read-only key queries.

Related pull requests

  • fix(approval): gate git config write operations behind approval #72576 best fix — (+76/-0) — best existing fix, amendment required: The updated diff covers the mutating forms and focused regression tests requested in the contributor's keep_open review, including --add, --replace-all, --unset, --unset-all, --remove-section, and --rename-section. However, the regex treats --file as argumentless, so git config --file .gitconfig user.name is incorrectly classified as a write and the negative tests do not cover that read-only form.

Suggested consolidation

Keep #72576 open with a salvage path: preserve its approval rule for direct key/value writes and the newly covered mutating flags, while correcting --file <path> parsing and adding positive and negative tests for file-scoped writes and reads. This follows the contributor's keep_open review; its stated mutation-coverage and regression-test objections are addressed by the updated diff, while the remaining read-only --file false positive still requires author action. There are no duplicate PRs to close.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I72556(["issue #72556 (open)"])
    P72576["PR #72576 (open)"]
    P72576 -->|best fix| I72556
    class I72556 open
    class P72576 open
    class P72576 best
    class P72576 target
    click I72556 "https://github.com/NousResearch/hermes-agent/issues/72556"
    click P72576 "https://github.com/NousResearch/hermes-agent/pull/72576"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 4 kB of PR diffs, 8 kB of issue/PR text, 2 kB of discussion (4 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@nuffin
nuffin force-pushed the fix/git-config-identity-72556 branch from 71d23b6 to 3b105b0 Compare August 4, 2026 08:31
@nuffin

nuffin commented Aug 4, 2026

Copy link
Copy Markdown
Author

Triage follow-up: --file regex false positive

The triage review correctly identified that --file was treated as an argumentless scope flag (like --global), so git config --file .gitconfig user.name (a read-only query) was misclassified as a dangerous write.

Fixed (71d23b6 → rebased): split --file out of the shared scope-flag group into its own sub-pattern that consumes the path argument (--file \s+\S+\s+). The other scope flags (--local, --global, --system, --worktree) remain in the argumentless group.

Added test_file_scoped_write_is_dangerous and test_file_scoped_read_is_safe covering both write and read forms with --file <path>.

@nuffin
nuffin requested a review from teknium1 August 4, 2026 14:38
@nuffin
nuffin force-pushed the fix/git-config-identity-72556 branch from 3b105b0 to b347d0c Compare August 9, 2026 12:54
@egilewski

Copy link
Copy Markdown
Contributor

suggesting changes

Several valid Git configuration mutations are not recognized as dangerous. The new detector covers only a limited ordering of scope, action, and operands. Option-bearing forms, attached file scopes, and edit mode remain classified as safe, so untrusted automation can bypass the approval gate while changing repository or global settings. Broaden parsing to follow Git argument semantics and add regression coverage while preserving read-only queries as safe.

Security evidence:
trust boundary: An untrusted caller or skill supplies terminal input; the terminal guard decides whether human approval is required before execution.
source/sink/invariant: Every mutating Git configuration operation must be classified as dangerous before execution, while read-only queries may remain safe. The new expression covers only one token layout, so alternate valid mutations bypass that invariant.
current-main reproduction: The current-main detector does not classify the alternate mutation forms as dangerous.
PR-head or patch-replay validation: The reviewed change recognizes the covered legacy forms, but option-rich, attached-file, and edit forms remain safe.
positive/negative cases: Covered checks mark positional writes, supported mutators, and separated file-scoped writes as dangerous and query-only forms as safe; residual positive cases are option ordering, attached-file syntax, and edit mode.
residual bypass search: The detector has one regex gate and no argument parser for this operation; the residual classes reach the same execution sink.
reviewer validation: Focused regression checks pass for covered forms, while independent probes confirm the residual mutation classes remain unclassified.

Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub main because the submitted branch is stale or conflicted; this does not mean the submitted branch itself merges cleanly.

Not checked:

  • Full approval test file

Signed: GPT-5.6-luna-max in Codex

@alt-glitch alt-glitch removed sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 14, 2026
nuffin added 3 commits August 17, 2026 21:10
Add "git config <key> <value>" (with optional scope flag) to
DANGEROUS_PATTERNS so the agent cannot silently set its own
git identity.  Read-only forms (--list, --get, bare key) are
not gated.

Closes NousResearch#72556.
…tion

Replace narrow pattern matching only 'git config key value' with one
covering --add, --replace-all, --unset, --unset-all, --remove-section,
and --rename-section. Query-only forms (--list, --get, bare key) still
pass through without approval.

Add 8 regression tests: positive for all mutating forms, negative for
all query forms. 98/98 approval tests pass.
The --file flag differs from --local/--global/--system/--worktree:
it consumes a path argument. The old regex treated --file as an
argumentless scope flag, so 'git config --file .gitconfig user.name'
(a read-only key query) was misclassified as a dangerous write.

Restructured the scope group: --file now has its own sub-pattern that
consumes the path (--file \s+\S+\s+), while the other scope flags
remain in the optional argumentless group.

Added test_file_scoped_write_is_dangerous and
test_file_scoped_read_is_safe to cover file-scoped writes and reads.
@nuffin
nuffin force-pushed the fix/git-config-identity-72556 branch from b347d0c to ea60753 Compare August 17, 2026 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Agent silently sets git config user.email/user.name before committing without asking — unexpected inherited attribution

5 participants