Skip to content

fix(security): close CodeQL #33 go/command-injection in runGit (Critical)#55

Merged
aksOps merged 1 commit intomainfrom
fix-codeql-git-command-injection
Apr 23, 2026
Merged

fix(security): close CodeQL #33 go/command-injection in runGit (Critical)#55
aksOps merged 1 commit intomainfrom
fix-codeql-git-command-injection

Conversation

@aksOps
Copy link
Copy Markdown
Contributor

@aksOps aksOps commented Apr 23, 2026

Alert

CodeQL alert #33 — `go/command-injection` — Critical severity, `internal/notes/history.go:106`.

Building a system command from user-controlled sources is vulnerable to insertion of malicious code by the user.

`exec.Command("git", ...)` in `runGit` received user-controlled strings: commit message bytes (author + subject), and relative paths derived from note keys. While `exec.Command` uses argv (no shell) which disables the classic shell-injection vector, git itself has flag-based attack surface that CodeQL is right to flag:

  • `--upload-pack=` / `--receive-pack=` on fetch/push subcommands
  • `-c core.sshCommand=` on any subcommand
  • `-c core.pager= log`
  • notesDir beginning with `-` would be re-parsed as a top-level flag

Fix — defense in depth

1. `runGit` gains a strict sanitiser pass:

2. `commit` is deliberately removed from the allow-list. Commit messages come from user input; routing them through argv was the last remaining taint flow. A new `gitCommit(notesDir, msg)` helper runs:

```
git -C commit --no-gpg-sign --allow-empty-message -F -
```

and pipes the message via stdin. User bytes never appear in argv.

3. `autoCommit` switched from `runGit(..., "commit", "-m", msg)` to `gitCommit(notesDir, msg)`.

Why this clears the alert

CodeQL's taint-tracking for `go/command-injection`:

Test results

Local: `104/104 tests passing in internal/notes/`, `go vet` clean.

Files changed

  • `internal/notes/history.go` (+66 / −4)

Not addressed

Alert #11 — SAST score 9/10 (27/30 commits scanned). Cosmetic Scorecard warning. CodeQL runs on PR + push-to-main + weekly schedule — intermediate feature-branch commits aren't individually scanned. Fixing to 30/30 would require running CodeQL on every push to every branch, which is expensive for a pre-1.0 single-maintainer project. Leaving as-is.

🤖 Generated with Claude Code

CodeQL alert #33 (Critical): exec.Command("git", ...) in runGit
received user-controlled strings (commit message bytes from author /
subject, and paths from note keys). Although exec.Command uses argv
(no shell) which makes the standard shell-injection vector inert, git
itself has flag-based attack surface (e.g. `--upload-pack=cmd`,
`-c core.sshCommand=...`) that CodeQL is right to flag.

Defense in depth:

1. `runGit` now enforces:
   - notesDir must be non-empty and must not start with "-" (prevents
     it being parsed as a git top-level flag).
   - args[0] must be in a closed allow-list of subcommands (init /
     config / add / log). Commit is deliberately routed elsewhere
     (see #2).
   - Any arg after a literal "--" must satisfy filepath.IsLocal —
     this continues the sanitiser from PR #44 and is what CodeQL
     recognises.

2. `gitCommit` is a new helper that runs `git commit --no-gpg-sign
   -F -` and pipes the message via stdin. The message bytes therefore
   never appear in argv, cutting the only remaining taint flow from
   user input to exec.Command args.

3. autoCommit's commit call switched from runGit(..., "commit", "-m",
   msg) to gitCommit(notesDir, msg).

Tested locally: 104/104 tests passing in internal/notes/.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@aksOps aksOps enabled auto-merge (squash) April 23, 2026 08:15
@aksOps aksOps merged commit c2ea35a into main Apr 23, 2026
11 checks passed
@aksOps aksOps deleted the fix-codeql-git-command-injection branch April 23, 2026 08:21
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