diff --git a/internal/notes/history.go b/internal/notes/history.go index 4237f9a..645b977 100644 --- a/internal/notes/history.go +++ b/internal/notes/history.go @@ -100,8 +100,51 @@ func initRepo(notesDir string) error { return nil } +// allowedGitSubcommands is the closed set of git subcommands runGit +// will invoke. This keeps future callers from accidentally passing a +// user-controlled string in the subcommand slot, and gives CodeQL a +// recognizable allow-list sanitizer for the go/command-injection rule. +var allowedGitSubcommands = map[string]bool{ + "init": true, + "config": true, + "add": true, + "log": true, +} + // runGit invokes `git -C ` and returns combined output. +// +// Security: +// - No shell — exec.Command takes argv directly. +// - notesDir must be non-empty and must not begin with "-" (which +// would let it be parsed as a git top-level flag). +// - args[0] must be in allowedGitSubcommands. +// - Any arg that follows a literal "--" separator must satisfy +// filepath.IsLocal — it's expected to be an in-tree relative path. +// +// runGit deliberately does NOT handle `commit`. Commit messages come +// from user input; they're routed through gitCommit which pipes the +// message via stdin so it never touches argv (see gitCommit below). func runGit(notesDir string, args ...string) (string, error) { + if notesDir == "" || strings.HasPrefix(notesDir, "-") { + return "", fmt.Errorf("runGit: invalid notesDir") + } + if len(args) == 0 { + return "", fmt.Errorf("runGit: no subcommand") + } + if !allowedGitSubcommands[args[0]] { + return "", fmt.Errorf("runGit: disallowed subcommand %q", args[0]) + } + seenDDash := false + for _, a := range args[1:] { + if a == "--" { + seenDDash = true + continue + } + if seenDDash && !filepath.IsLocal(a) { + return "", fmt.Errorf("runGit: non-local path arg %q", a) + } + } + full := append([]string{"-C", notesDir}, args...) cmd := exec.Command("git", full...) // Detach from any inherited GIT_* env — the caller's config must @@ -117,6 +160,28 @@ func runGit(notesDir string, args ...string) (string, error) { return buf.String(), err } +// gitCommit runs `git commit` reading the message from stdin. The +// message is never passed as a command-line argument, removing the +// only path by which user-controlled bytes could have reached the git +// argv. `--no-gpg-sign` keeps the commit unsigned (we rely on cosign +// for release signing, not per-commit GPG). +func gitCommit(notesDir, msg string) (string, error) { + if notesDir == "" || strings.HasPrefix(notesDir, "-") { + return "", fmt.Errorf("gitCommit: invalid notesDir") + } + cmd := exec.Command("git", "-C", notesDir, "commit", "--no-gpg-sign", "--allow-empty-message", "-F", "-") + cmd.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL=/dev/null", + "GIT_CONFIG_SYSTEM=/dev/null", + ) + cmd.Stdin = strings.NewReader(msg) + var buf bytes.Buffer + cmd.Stdout = &buf + cmd.Stderr = &buf + err := cmd.Run() + return buf.String(), err +} + // truncateForCommit caps a string to maxCommitMsgBytes (UTF-8-safe: cut // at byte boundary, then trim trailing invalid UTF-8 fragment by // walking back to a rune boundary). @@ -216,10 +281,7 @@ func autoCommit(notesDir, key, author, subject string, deleted bool) { // which is the desired behavior. `git commit` exits non-zero on // "nothing to commit" — treat that as a silent no-op rather than // a warning. - if out, err := runGit(notesDir, "commit", - "--no-gpg-sign", - "-m", msg, - ); err != nil { + if out, err := gitCommit(notesDir, msg); err != nil { if strings.Contains(out, "nothing to commit") || strings.Contains(out, "no changes added") { return