Skip to content

fix(git): run git commands without a shell - #588

Open
stchigel wants to merge 2 commits into
all-contributors:mainfrom
stchigel:fix/git-commit-args-587
Open

fix(git): run git commands without a shell#588
stchigel wants to merge 2 commits into
all-contributors:mainfrom
stchigel:fix/git-commit-args-587

Conversation

@stchigel

@stchigel stchigel commented Oct 30, 2026 •
edited
Loading

Copy link
Copy Markdown

What: Fixes #587: With "commit": true, all-contributors add fails at the commit step with pathspec errors, leaving the files staged but not committed.

Why: This is a regression from #465. spawnGitCommand joins its arguments into one string and runs it through a shell, which causes three problems:

  • The commit message is split on spaces, so only the first word is used as the message and the rest are treated as pathspecs.
  • git add fails when the project path contains a space.
  • Shell metacharacters in a username or commit template are executed. Usernames aren't escaped because autoEscape is off (in src/util/template.js). (Confirmed this on Windows with &, did not test it on a POSIX shell)

How:

  • spawnGitCommand now uses execFile('git', args), so each argument reaches git intact with no shell involved.
  • getRemoteOriginData still uses exec, since its command is just a fixed string.
  • The existing tests asserted the broken joined string, so they now expect an argument array.
  • Added regression tests to test/e2e/cli.test.js. They run add octocat doc --commit in a temporary git repo whose path contains a space, and check that the commit gets the whole message and that shell metacharacters in the message are not executed. Both tests fail without the fix.

I also ran the reproduction steps from the issue using the fixed CLI, and confirmed the fix works.

Checklist:

  • Documentation N/A
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@stchigel

stchigel commented Oct 31, 2026

Copy link
Copy Markdown
Author

I left this as a draft for now just so a mantainer can check it. Since it's my first PR on this repo i dont want to make mistakes

@lwasser

lwasser commented Nov 2, 2026

Copy link
Copy Markdown
Member

@all-contributors please add @stchigel for bug

hey @stchigel thanks for this pr. it may take us a bit of time to get to this as we are working on the app right now but we will get to this soon!!

@allcontributors allcontributors Bot mentioned this pull request Nov 2, 2026
Merged
@allcontributors

allcontributors Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

@lwasser

I've put up a pull request to add @stchigel! 🎉

@stchigel
stchigel force-pushed the fix/git-commit-args-587 branch from 63d77df to 7c8014d Compare November 9, 2026 14:03
@stchigel

stchigel commented Nov 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Rebased onto main and fixed import order.
Also, the test is still in src/util/__tests__/, but I could move it to test/e2e/ if needed

@stchigel
stchigel marked this pull request as ready for review November 9, 2026 14:14
michaelfaith
michaelfaith requested changes Nov 9, 2026
View reviewed changes

@michaelfaith michaelfaith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution! It looks like there are some merge conflicts, and I do think this test would be better suited in the e2e area of tests, since it's doing a lot more than we'd generally do for unit tests 👍

@stchigel
stchigel force-pushed the fix/git-commit-args-587 branch from 7c8014d to 992c3ec Compare November 11, 2026 03:55
@stchigel

stchigel commented Nov 11, 2026

Copy link
Copy Markdown
Author

Rebased onto main (fixing the merge conflict) and move it to e2e as requested (while adapting it to match other e2e tests so it would work properly)

geniosoy and others added 2 commits November 11, 2026 04:13
@geniosoy @claude @michaelfaith
fix(git): run git commands without a shell
32a4c9a 
Since all-contributors#465, spawnGitCommand has joined its arguments into one string and
run it through a shell. The commit message was split on spaces, so
`all-contributors add` failed with `pathspec` errors when `commit` was
enabled. Paths containing spaces broke `git add` the same way, and shell
metacharacters in a username or commit template were executed.

Pass the arguments to execFile so each one reaches git intact.

Fixes all-contributors#587

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@geniosoy @claude @michaelfaith
test(e2e): cover add --commit with a real git repository
d46c218 
Move the regression test for all-contributors#587 from the unit tests to the e2e tests,
as it runs real git in a temporary repository. It now runs the CLI
itself with `add octocat doc --commit` and checks that:

- the commit is created with the whole message
- the project directory can contain a space
- shell metacharacters in the commit message are not executed

Add a `gitProject` fixture and a `runGit` helper to support it.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@michaelfaith
michaelfaith force-pushed the fix/git-commit-args-587 branch from 992c3ec to d46c218 Compare November 11, 2026 10:13
@michaelfaith
michaelfaith requested review from a team and michaelfaith November 11, 2026 10:14

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelfaith michaelfaith Awaiting requested review from michaelfaith

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unquoted commit message is causing npx all-contributors add to fail on v7.0.0

4 participants

@stchigel @lwasser @michaelfaith @geniosoy