Skip to content

fix: validate denylisted git args on every token - #784

Merged
EndBug merged 2 commits into
mainfrom
cursor/fix-skipnext-denylist-bypass
Aug 19, 2026
Merged

fix: validate denylisted git args on every token#784
EndBug merged 2 commits into
mainfrom
cursor/fix-skipnext-denylist-bypass

Conversation

@EndBug

@EndBug EndBug commented Aug 18, 2026

Copy link
Copy Markdown
Owner

Summary

  • Stop skipping the next argv after a guessed value-taking short option (-Sm, -tm, …). That skip disagreed with Git’s cluster parser and let --pathspec-from-file / --pathspec-file-nul reach git, which prints the file into workflow logs.
  • matchGitArgs now runs the --upload-pack, -F/--file, --pathspec-from-file, and scheme:: checks on every token (the same rule --upload-pack already used).
  • Extra-arg forms like commit: -m "-F" or -m "foo::bar" are now rejected; glued -m-F is still allowed. The message input is unchanged.

Test plan

  • Unit: -Sm --pathspec-from-file=/x -Sm --pathspec-file-nul and -tm --pathspec-from-file=/x throw; same clusters with scheme:: throw unless allowUnsafeGitProtocols.
  • Integration: commit: -Sm --pathspec-from-file=<dummy> -Sm --pathspec-file-nul fails with not allowed and does not print dummy markers or move HEAD.
  • Confirm a normal commit: --signoff (or similar extra args) still works.

Made with Cursor

Summary by CodeRabbit

  • Bug Fixes

    • Improved detection and rejection of dangerous Git flags, including when provided as option values or within short-option clusters.
    • Blocked unsafe remote-helper URLs in message values unless explicitly permitted.
    • Prevented blocked commit arguments from exposing file contents or altering the current commit state.
  • Tests

    • Added integration coverage for blocked arguments across varied option placements.
    • Expanded validation for message, pathspec, and remote-helper URL scenarios.

Short-option clusters such as -Sm were treated as consuming the next
argument, which let --pathspec-from-file reach git and leak file
contents into workflow logs.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 03b21445-98c0-4e79-a321-4128eaf186da

📥 Commits

Reviewing files that changed from the base of the PR and between 3e1240d and ae97483.

📒 Files selected for processing (4)
  • lib/index.js
  • src/util.ts
  • test/integration/action.test.ts
  • test/util.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

matchGitArgs now applies Git argument denylists to every parsed token, including option values and short-option clusters. Tests cover blocked message and pathspec files, unsafe remote-helper URLs, nondisclosure, and unchanged repository state.

Changes

Git argument validation

Layer / File(s) Summary
All-token denylist scanning
src/util.ts
matchGitArgs no longer skips presumed option values. It checks message-file flags, pathspec-file flags, remote-helper URLs, and overrides on every token.
Validation and integration coverage
test/util.test.ts, test/integration/action.test.ts
Tests cover blocked values after message flags and short-option clusters. Integration tests verify rejection, nondisclosure, and unchanged HEAD.

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

Merge Risk: ⚪ Minimal · up to ae974

The change is merge-ready after normal checks and review; no actionable merge-blocking risk remains.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. 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 and concisely describes the main change: validating denylisted Git arguments on every token.
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.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/fix-skipnext-denylist-bypass

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

Comment @coderabbitai help to get the list of available commands.

@EndBug
EndBug marked this pull request as ready for review August 18, 2026 23:44
@EndBug
EndBug enabled auto-merge (squash) August 19, 2026 14:46
@EndBug
EndBug merged commit 3446398 into main Aug 19, 2026
11 checks passed
@EndBug
EndBug deleted the cursor/fix-skipnext-denylist-bypass branch August 19, 2026 14:47
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.

1 participant