-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: harden for option injection #40
fix: harden for option injection #40
Conversation
49bffee
to
6701f3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed. The implementation is way more secure than it was, but I would still recommend hardening.
|
||
export function add(pathSpecs: string[]) { | ||
assert( | ||
pathSpecs.every((it) => !it.startsWith('-')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this check in some corner cases will not be enough. There are multiple shell characters which will bypass this restriction. For example, run next command in bash:
git add ``-e
: !/bin/sh
git add ${}-e
: !/bin/sh
Our current implementation in not passing shell
option to the nodeSpawnSync
, leaving us safe.
const { stdout, stderr, status } = nodeSpawnSync(command, args, { encoding: 'utf8', ...options })
Nevertheless, if any client (or malicious actor via PP) will pass options with the shell=true
, it would be easy to bypass startswith
check
> node
const cp = require('child_process')
cp.spawnSync('git', ['add', '``-e'], { encoding: 'utf8', shell:false}) // not vulnerable
{
status: 128,
signal: null,
output: [ null, '', "fatal: pathspec '``-e' did not match any files\n" ],
pid: 38227,
stdout: '',
stderr: "fatal: pathspec '``-e' did not match any files\n"
}
> cp.spawnSync('git', ['add', '``-e'], { encoding: 'utf8', shell:true}) // vulnerable
Explicitly whitelist the flags that can be used with the
commit
andresetLastCommit
functionCloses #37