fix(guardrails): resolve persisted aliases via alias.<sub>.command subkey - #2448
fix(guardrails): resolve persisted aliases via alias.<sub>.command subkey#2448kyle-sexton wants to merge 1 commit into
Conversation
…bkey When git stores an alias only under alias.<name>.command, persisted_alias() now falls back to that subkey after alias.<name> returns empty. Fixes #1022. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 3m 51s —— View job Security Review
This PR touches only IMPORTANT / Confidence: Medium-High — persisted-alias fallback lets the benign persisted_alias(), lines 483–489: if [[ -z "${_persisted_alias[$key]+x}" ]]; then
val=$(git -C "$dir" config --get "alias.$sub" 2>/dev/null)
if [[ -z "$val" ]]; then
val=$(git -C "$dir" config --get "alias.$sub.command" 2>/dev/null)
fi
_persisted_alias["$key"]="$val"
fiThis checks This is the same twin-spelling ambiguity this file's own CHANGELOG already documents as security-relevant (
The inline-alias path in this same file already implements that "check both spellings independently, union the danger" rule (comment at line 662: "Re-check EVERY spelling (plain and Exploit scenario: an attacker/agent (the guard's own threat model already covers config-alias smuggling) sets, in a repo or global config reachable before the hook runs: then runs Suggested fix: mirror the existing MAX-DANGER-UNION approach used for inline aliases — fetch both No other security issues found in this diff. The test file addition ( Supply-chain / unpinned-action / permissions / dangerous-trigger concerns are out of scope here per instructions (zizmor's lane) — none of the changed files are workflow files regardless. |
|
Claude finished @kyle-sexton's task in 4m 28s —— View job Review completeScope: Findings (posted inline)
Additional note (no changed line to anchor this to)
Not reviewed here
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d4aa172dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [[ -z "$val" ]]; then | ||
| val=$(git -C "$dir" config --get "alias.$sub.command" 2>/dev/null) |
There was a problem hiding this comment.
When only alias.c.command is configured, Git treats everything after alias. as the alias name: this defines git c.command, not git c, consistent with the official alias.* documentation. I verified with Git 2.43.0 that git c.command runs the expansion while git c reports an unknown command, but this fallback re-expands git c as commit and can falsely block it; it also adds a second git config process to every ordinary alias miss. Only alias.$sub should be queried—the dotted alias will already be found naturally when $sub itself is c.command.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if [[ -z "${_persisted_alias[$key]+x}" ]]; then | ||
| val=$(git -C "$dir" config --get "alias.$sub" 2>/dev/null) | ||
| if [[ -z "$val" ]]; then | ||
| val=$(git -C "$dir" config --get "alias.$sub.command" 2>/dev/null) | ||
| fi | ||
| _persisted_alias["$key"]="$val" | ||
| fi |
There was a problem hiding this comment.
🔴 Important — persisted alias resolution doesn't apply the same "max-danger union" the inline (-c) path already enforces, reopening the exact bypass this file's own docs warn about.
This fallback treats alias.$sub and alias.$sub.command as mutually exclusive (checks .command only when the plain form is empty). But git config allows both keys to be set simultaneously ([alias] c = ... and [alias "c"] command = ... are distinct config keys), and this codebase already has an established, tested policy for exactly that ambiguity — see hook::git_alias_expansion in hook-utils.sh (used for the inline -c alias.c=... path), which independently collects both spellings into HOOK_GIT_ALIAS_EXPS so the caller re-checks each one:
claude-code-plugins/plugins/guardrails/hooks/hook-utils.sh
Lines 1644 to 1663 in 2d4aa17
That function's own comment states the rationale directly: "Keep the LAST value WITHIN each spelling separately, never collapsed across the two, so one spelling's value cannot mask the other's." The inline path even has dedicated regression tests for this ("MAX-DANGER UNION") in this PR's own test file:
The pre-existing docblock immediately above this function (lines 507-511, unchanged by this PR) describes the same intended design for the whole alias-reexpansion path: "every hop re-checks both alias spellings (alias.<sub> and alias.<sub>.command) independently, so a chain where each hop defines both walks 2^depth analysis paths." persisted_alias() as written can never produce that branching — it always resolves to a single merged value.
Concrete bypass: with a persisted config git config alias.c status + git config alias.c.command commit (or the equivalent in .git/config/~/.gitconfig), git c -m 'bypass$'\n'b' would commit through the (unblocked) .command spelling while persisted_alias() only ever reports the benign status value, since it's checked first and is non-empty. This mirrors the inline "commit .command masked by a benign plain" case the test suite already guards against — just not for the persisted-config path.
Suggest resolving both keys unconditionally (a single git config --get-regexp '^alias\.'"$sub"'(\.command)?$' fork could get both without doubling the cost) and returning both values (e.g. an array, mirroring HOOK_GIT_ALIAS_EXPS) so check_segment's persisted-alias branch loops over each present spelling independently, matching the inline branch's existing loop at line 667.
| # --- persisted alias.<sub>.command when alias.<sub> is empty (#1022) ---------- | ||
| PCMD="$TEST_TMPDIR/persisted-command-subkey" | ||
| mkdir -p "$PCMD" | ||
| ( | ||
| cd "$PCMD" || exit 1 | ||
| git init -q . | ||
| git config user.email t@e.st | ||
| git config user.name t | ||
| git config alias.c.command commit | ||
| ) >/dev/null 2>&1 | ||
|
|
||
| if [[ -d "$PCMD/.git" ]]; then | ||
| MSYS_NO_PATHCONV=1 jq -n --arg c "git c -m \$'bypass\nb'" --arg d "$PCMD" \ | ||
| '{tool_name:"Bash",tool_input:{command:$c},cwd:$d}' | | ||
| bash "$HOOK" >/dev/null 2>&1 | ||
| assert_exit "persisted alias.<sub>.command only: git c -m" 2 $? | ||
| fi | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion — test coverage: missing the "max-danger union" cases for the persisted-alias path.
This only tests the reported scenario (plain empty, .command set). The inline -c path has dedicated "union" tests a few hundred lines up (commit plain masked by a benign .command, commit .command masked by a benign plain, both spellings benign non-commit) — see lines 316-319. Given the correctness concern raised on persisted_alias() above, this suite should gain the persisted-config equivalents, e.g.:
git config alias.c status
git config alias.c.command commit
# git c -m '...\n...' should still be BLOCKED (currently would be allowed)and the reverse ordering / both-benign case, mirroring lines 316-319.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
2 participants
Closes #1022