Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@git-stunts/plumbing",
"version": "2.7.0",
"version": "2.7.1",
"description": "Robust async, stream-first Git plumbing for Node, Bun, and Deno.",
"type": "module",
"main": "index.js",
Expand Down
73 changes: 72 additions & 1 deletion src/domain/services/CommandSanitizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ export default class CommandSanitizer {
'check-attr',
'init',
'config',
'log'
'log',
'show'
]);

/**
Expand Down Expand Up @@ -72,6 +73,73 @@ export default class CommandSanitizer {
'--ext-cmd'
];

/**
* Per-command flag allowlists for commands with restricted flag sets.
* Only flags listed here are permitted for these commands.
* Commands not in this map have no additional restrictions beyond global prohibitions.
* @private
*/
static _COMMAND_FLAG_ALLOWLISTS = {
'show': new Set([
'--format', '--pretty', '-s', '--no-patch', '--quiet', '-q',
'--name-only', '--name-status', '--stat', '--numstat', '--shortstat',
'--oneline', '--abbrev-commit', '--no-abbrev-commit', '--date', '--no-notes'
]),
'log': new Set([
'--format', '--pretty', '-z', '--oneline',
'-n', '--max-count', '-1', '-2', '-3', '-4', '-5', '-10', '-20', '-50', '-100',
'--skip', '--since', '--until', '--after', '--before',
'--author', '--committer', '--grep', '--all-match', '--invert-grep',
'--regexp-ignore-case', '-i', '-E', '-F', '-P',
'--ancestry-path', '--first-parent', '--no-merges', '--merges',
'--reverse', '--date-order', '--author-date-order', '--topo-order',
'--abbrev-commit', '--no-abbrev-commit', '--abbrev', '--date',
'--relative-date', '--parents', '--children', '--left-right',
'--graph', '--decorate', '--no-decorate', '--source',
'--no-walk', '--stdin', '--cherry', '--cherry-pick', '--cherry-mark',
'--boundary', '--simplify-by-decoration'
])
};

/**
* Validates command-specific flags against the allowlist.
* @param {string} command - The git subcommand (e.g., 'show', 'log')
* @param {string[]} args - All arguments including flags
* @param {number} commandIndex - Index of the command in args array
* @throws {ProhibitedFlagError} If a flag is not in the allowlist
* @private
*/
static _validateCommandFlags(command, args, commandIndex) {
const allowlist = CommandSanitizer._COMMAND_FLAG_ALLOWLISTS[command];
if (!allowlist) {
return; // No allowlist = no additional restrictions
}

for (let i = commandIndex + 1; i < args.length; i++) {
const arg = args[i];

// Stop flag validation at end-of-options marker
// Everything after '--' is a pathspec or ref, not a flag
if (arg === '--') {
break;
}

// Skip non-flag arguments (refs, paths, etc.)
if (!arg.startsWith('-')) {
continue;
}

// Handle --flag=value format: extract just the flag portion
const flagPart = arg.includes('=') ? arg.split('=')[0] : arg;

if (!allowlist.has(flagPart)) {
throw new ProhibitedFlagError(arg, 'CommandSanitizer.sanitize', {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
message: `Flag '${flagPart}' is not allowed for '${command}' command`
});
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}

/**
* Dynamically allows a command.
* @param {string} commandName
Expand Down Expand Up @@ -158,6 +226,9 @@ export default class CommandSanitizer {
throw new ValidationError(`Prohibited git command detected: ${command}`, 'CommandSanitizer.sanitize', { command });
}

// Validate per-command flag allowlists
CommandSanitizer._validateCommandFlags(command, args, subcommandIndex !== -1 ? subcommandIndex : 0);

let totalLength = 0;
for (const arg of args) {
totalLength += arg.length;
Expand Down
150 changes: 148 additions & 2 deletions test/domain/services/CommandSanitizer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ describe('CommandSanitizer', () => {
expect(() => sanitizer.sanitize(['rev-parse', 'HEAD'])).not.toThrow();
});

it('allows log command for commit history traversal', () => {
expect(() => sanitizer.sanitize(['log', '--format=%H', '-z', 'HEAD'])).not.toThrow();
});

it('allows show command for reading commit messages', () => {
expect(() => sanitizer.sanitize(['show', '--format=%B', '-s', 'HEAD'])).not.toThrow();
});

it('throws ValidationError for unlisted commands', () => {
expect(() => sanitizer.sanitize(['push', 'origin', 'main'])).toThrow(ValidationError);
});
Expand Down Expand Up @@ -46,13 +54,151 @@ describe('CommandSanitizer', () => {

it('handles cache eviction', () => {
const smallSanitizer = new CommandSanitizer({ maxCacheSize: 2 });

const args1 = ['rev-parse', 'HEAD'];
const args2 = ['cat-file', '-p', '4b825dc642cb6eb9a060e54bf8d69288fbee4904'];
const args3 = ['ls-tree', 'HEAD'];

smallSanitizer.sanitize(args1);
smallSanitizer.sanitize(args2);
smallSanitizer.sanitize(args3);
});

describe('Per-command flag allowlists', () => {
describe('show command', () => {
it('allows whitelisted flags for show', () => {
expect(() => sanitizer.sanitize(['show', '--format=%B', '-s', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['show', '--pretty=oneline', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['show', '--no-patch', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['show', '--quiet', 'HEAD'])).not.toThrow();
});

it('rejects non-whitelisted flags for show', () => {
expect(() => sanitizer.sanitize(['show', '--diff-filter=A', 'HEAD'])).toThrow(ProhibitedFlagError);
expect(() => sanitizer.sanitize(['show', '--follow', 'HEAD'])).toThrow(ProhibitedFlagError);
expect(() => sanitizer.sanitize(['show', '-p', 'HEAD'])).toThrow(ProhibitedFlagError);
});

it('allows show with no ref (defaults to HEAD)', () => {
expect(() => sanitizer.sanitize(['show'])).not.toThrow();
expect(() => sanitizer.sanitize(['show', '--format=%B'])).not.toThrow();
});
});

describe('log command', () => {
it('allows whitelisted flags for log', () => {
expect(() => sanitizer.sanitize(['log', '--format=%H', '-z', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['log', '-n', '10', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['log', '--max-count=50', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['log', '--oneline', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['log', '--first-parent', 'HEAD'])).not.toThrow();
});

it('rejects non-whitelisted flags for log', () => {
expect(() => sanitizer.sanitize(['log', '--diff-filter=M', 'HEAD'])).toThrow(ProhibitedFlagError);
expect(() => sanitizer.sanitize(['log', '--follow', 'HEAD'])).toThrow(ProhibitedFlagError);
expect(() => sanitizer.sanitize(['log', '-p', 'HEAD'])).toThrow(ProhibitedFlagError);
});

it('allows log with no arguments', () => {
expect(() => sanitizer.sanitize(['log'])).not.toThrow();
});
});

describe('other commands have no additional restrictions', () => {
it('allows any flags for rev-parse', () => {
expect(() => sanitizer.sanitize(['rev-parse', '--show-toplevel'])).not.toThrow();
expect(() => sanitizer.sanitize(['rev-parse', '--abbrev-ref', 'HEAD'])).not.toThrow();
});

it('allows any flags for cat-file', () => {
expect(() => sanitizer.sanitize(['cat-file', '-p', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['cat-file', '-t', 'HEAD'])).not.toThrow();
});
});
});

describe('Argument injection protection (spawn safety)', () => {
it('safely handles shell metacharacters in flag values', () => {
// spawn() passes args directly - no shell parsing
expect(() => sanitizer.sanitize(['log', '--format=%; rm -rf /', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['log', '--author=foo; cat /etc/passwd', 'HEAD'])).not.toThrow();
});

it('safely handles backticks in arguments', () => {
expect(() => sanitizer.sanitize(['log', '--format=`whoami`', 'HEAD'])).not.toThrow();
});

it('safely handles $() command substitution in arguments', () => {
expect(() => sanitizer.sanitize(['log', '--format=$(id)', 'HEAD'])).not.toThrow();
});

it('safely handles pipe characters in arguments', () => {
expect(() => sanitizer.sanitize(['log', '--format=%s | malicious', 'HEAD'])).not.toThrow();
});

it('safely handles newlines in arguments', () => {
expect(() => sanitizer.sanitize(['log', '--format=%s\n%b', 'HEAD'])).not.toThrow();
});

it('safely handles quotes in arguments', () => {
expect(() => sanitizer.sanitize(['log', '--author="John Doe"', 'HEAD'])).not.toThrow();
expect(() => sanitizer.sanitize(['log', "--author='Jane Doe'", 'HEAD'])).not.toThrow();
});
});

describe('NUL-terminated output (log -z)', () => {
it('allows log with -z flag', () => {
expect(() => sanitizer.sanitize(['log', '-z', '--format=%H', 'HEAD'])).not.toThrow();
});

it('allows log with -z and multiple format specifiers', () => {
expect(() => sanitizer.sanitize(['log', '-z', '--format=%H%x00%s%x00%b'])).not.toThrow();
});

it('allows log with -z combined with -n', () => {
expect(() => sanitizer.sanitize(['log', '-z', '-n', '10', 'HEAD'])).not.toThrow();
});

it('allows log with -z and --max-count', () => {
expect(() => sanitizer.sanitize(['log', '-z', '--max-count=50', 'main..HEAD'])).not.toThrow();
});

it('allows log with -z and ancestry path traversal', () => {
expect(() => sanitizer.sanitize(['log', '-z', '--ancestry-path', '--format=%H', 'main..HEAD'])).not.toThrow();
});

it('allows log with -z and first-parent for linear history', () => {
expect(() => sanitizer.sanitize(['log', '-z', '--first-parent', '--format=%H%x00%P', 'HEAD'])).not.toThrow();
});

it('allows log with -z and reverse for chronological order', () => {
expect(() => sanitizer.sanitize(['log', '-z', '--reverse', '--format=%H', 'HEAD~10..HEAD'])).not.toThrow();
});
});

describe('Edge cases', () => {
it('throws for empty array', () => {
expect(() => sanitizer.sanitize([])).toThrow(ValidationError);
});

it('allows commands with only the command name (no flags or refs)', () => {
expect(() => sanitizer.sanitize(['show'])).not.toThrow();
expect(() => sanitizer.sanitize(['log'])).not.toThrow();
expect(() => sanitizer.sanitize(['rev-parse'])).not.toThrow();
});

it('handles flags with = values correctly', () => {
expect(() => sanitizer.sanitize(['log', '--format=%H'])).not.toThrow();
expect(() => sanitizer.sanitize(['log', '--max-count=10'])).not.toThrow();
});

it('stops flag validation at end-of-options marker (--)', () => {
// Args after '--' are pathspecs/refs, not flags - should not be validated
expect(() => sanitizer.sanitize(['show', '--format=%B', '--', '-weird-ref-name'])).not.toThrow();
expect(() => sanitizer.sanitize(['log', '--oneline', '--', '--not-a-flag'])).not.toThrow();
// Disallowed flag BEFORE '--' should still throw
expect(() => sanitizer.sanitize(['show', '-p', '--', 'file.txt'])).toThrow(ProhibitedFlagError);
});
});
});