Skip to content

Conversation

@flyingrobots
Copy link
Member

@flyingrobots flyingrobots commented Jan 29, 2026

Summary

  • Add strict per-command flag allowlists for show and log commands to enforce read-only plumbing usage
  • Implement _validateCommandFlags() static method that rejects non-whitelisted flags with ProhibitedFlagError
  • Add comprehensive test coverage for security edge cases

Changes

CommandSanitizer.js

  • Added _COMMAND_FLAG_ALLOWLISTS with whitelisted flags for:
    • show: --format, --pretty, -s, --no-patch, --quiet, -q, --name-only, --stat, etc.
    • log: --format, --pretty, -z, -n, --max-count, --ancestry-path, --first-parent, --reverse, etc.
  • Added _validateCommandFlags() method to validate flags against allowlists

Test Coverage (24 new tests)

  • Per-command flag allowlists: Whitelisted flags allowed, non-whitelisted flags rejected (--diff-filter, --follow, -p)
  • Argument injection protection: Proves spawn() safety against shell metacharacters (;, backticks, $(), pipes, newlines)
  • NUL-terminated output (log -z): Various flag combinations for reliable commit parsing
  • Edge cases: Empty arrays, commands with no flags/refs, --flag=value parsing

Test plan

  • All 34 sanitizer tests pass in Docker (docker-compose run --rm node-test)
  • Verify show with disallowed flag throws ProhibitedFlagError
  • Verify log -z works for commit history traversal

Summary by CodeRabbit

  • New Features

    • Enhanced command validation with per-command flag allowlists, including support for the show and log commands to tighten allowed flag usage.
  • Tests

    • Added extensive tests for allowed/disallowed flags, injection protection, end-of-options behavior, numeric short forms, and related edge cases.
  • Chores

    • Bumped package version to 2.7.1.

✏️ Tip: You can customize this high-level summary in your review settings.

- Add 'show' to the allowlist of safe Git commands.
- Update version to 2.7.1.
- Add unit tests for 'log' and 'show' sanitization.
- Introduce `_COMMAND_FLAG_ALLOWLISTS` to restrict flags for 'show' and 'log'.
- Add `_validateCommandFlags` to prevent unauthorized flag injection.
- Add comprehensive test suite covering flag validation, shell metacharacter
  safety, and NUL-terminated output scenarios.
@coderabbitai
Copy link

coderabbitai bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

Adds per-command flag allowlists and validation to CommandSanitizer (including show and log), extends allowed commands, bumps package version to 2.7.1, and introduces extensive tests covering allowlists, flag parsing, injection safety, NUL-terminated output, and edge cases.

Changes

Cohort / File(s) Summary
Version Management
package.json
Version bumped from 2.7.02.7.1.
Command Sanitization
src/domain/services/CommandSanitizer.js
Added show to _ALLOWED_COMMANDS; introduced static _COMMAND_FLAG_ALLOWLISTS and _validateCommandFlags(command, args, commandIndex); integrated per-command flag validation into sanitize; enforces command-specific allowed flags and numeric short-form handling.
Tests
test/domain/services/CommandSanitizer.test.js
Added comprehensive tests for show and log allowlists, flag acceptance/rejection (including -n10, --max-count=10), end-of-options (--) behavior, NUL-terminated log -z cases, injection/shell-character protection, edge cases (empty input, no-arg commands), and cache/memoization behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐇 I hopped through flags both meek and bold,
Whitelists stitched where stories told,
Show and log now mind their line,
Tests nibble every little sign,
A carrot-cheer for tidy code!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(sanitizer): add per-command flag allowlists for show and log' directly and clearly describes the main change: introducing per-command flag allowlists for the show and log commands in the CommandSanitizer module.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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 and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/domain/services/CommandSanitizer.js`:
- Around line 118-133: The loop in CommandSanitizer.sanitize is treating the
standard Git end-of-options marker '--' as a flag and rejecting it; update the
loop that iterates over args (starting at commandIndex + 1) to detect a bare
'--' and stop flag validation (break out of the loop) so subsequent args
(pathspecs or refs that begin with '-') are not checked against allowlist;
ensure the check happens before the startsWith('-')/flag parsing logic and keep
ProhibitedFlagError unchanged for real disallowed flags.

Stop flag validation when encountering bare '--' marker, allowing
pathspecs and refs that begin with '-' to pass through without
being checked against the allowlist.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/domain/services/CommandSanitizer.js`:
- Around line 132-136: The sanitize logic currently does exact allowlist checks
on flagPart and rejects combined numeric short forms like "-n10" and "-15";
update CommandSanitizer.sanitize to recognize and normalize numeric short forms
before allowlist validation by treating "-15" as equivalent to "-n15" and
accepting "-n<digits>" (e.g. via a regex check for /^-(?:n?\d+)$/), then
validate the normalized flag (or bypass allowlist for numeric forms) instead of
throwing ProhibitedFlagError for these cases; add unit tests for ['log', '-n10',
'HEAD'] and ['log', '-15', 'HEAD'] to prevent regressions.

Add regex check for /^-n?\d+$/ to recognize git's numeric shorthand
forms like -n10 and -15 (equivalent to -n 10 and -n 15). These are
now allowed when -n is in the command's flag allowlist.
@flyingrobots flyingrobots merged commit 6e483df into main Jan 29, 2026
3 checks passed
@flyingrobots flyingrobots deleted the v2.8.0 branch January 29, 2026 03:41
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