From c24d9ca2f533f4986e23283cd945d765fc68afcd Mon Sep 17 00:00:00 2001 From: Thomas Vilte Date: Thu, 30 Jul 2026 19:16:08 -0300 Subject: [PATCH] fix(cli): prevent completion description leakage and improve error handling --- cmd/matecommit/main.go | 23 +++-- internal/commands/completion/completion.go | 7 +- .../commands/completion/completion_test.go | 88 +++++++++++++++++++ 3 files changed, 109 insertions(+), 9 deletions(-) create mode 100644 internal/commands/completion/completion_test.go diff --git a/cmd/matecommit/main.go b/cmd/matecommit/main.go index c3f2db2..aac052f 100644 --- a/cmd/matecommit/main.go +++ b/cmd/matecommit/main.go @@ -3,7 +3,6 @@ package main import ( "context" "fmt" - "log" "net/http" "os" "strings" @@ -39,11 +38,18 @@ import ( func main() { app, err := initializeApp() if err != nil { - log.Fatalf("Error starting the CLI: %v", err) + fmt.Fprintf(os.Stderr, "Error starting the CLI: %v\n", err) + os.Exit(1) } + // Not log.Fatal: internal/logger.Initialize calls slog.SetDefault, which + // (per log/slog's documented behavior) redirects the stdlib log package + // through the slog handler at Info level. With the handler's default + // Warn threshold, that silently drops every fatal error unless --debug + // is passed — and even then it prints mislabeled as "[INFO]". if err := app.Run(context.Background(), os.Args); err != nil { - log.Fatal(err) + fmt.Fprintln(os.Stderr, err) + os.Exit(1) } } @@ -60,7 +66,7 @@ func initializeApp() (*cli.Command, error) { translations, err := i18n.NewTranslations(cfgApp.Language, "") if err != nil { - log.Fatalf("Error loading translations: %v", err) + return nil, fmt.Errorf("error loading translations: %w", err) } ctx := context.Background() @@ -92,9 +98,12 @@ func initializeApp() (*cli.Command, error) { Usage: translations.GetMessage("flags_global.debug_flag", 0, nil), }, &cli.BoolFlag{ - Name: "verbose", - Aliases: []string{"v"}, - Usage: translations.GetMessage("flags_global.verbose_flag", 0, nil), + // No -v alias: urfave/cli auto-registers -v for --version, + // and since it doesn't detect the collision, -v (and even + // the long --verbose form) ends up resolving to --version, + // silently skipping whatever command was actually requested. + Name: "verbose", + Usage: translations.GetMessage("flags_global.verbose_flag", 0, nil), }, }, Before: func(ctx context.Context, c *cli.Command) (context.Context, error) { diff --git a/internal/commands/completion/completion.go b/internal/commands/completion/completion.go index 7b5cd6d..2f20184 100644 --- a/internal/commands/completion/completion.go +++ b/internal/commands/completion/completion.go @@ -22,8 +22,11 @@ _mate_commit_bash_autocomplete() { # Construct the command line with previous words and append the completion flag # We strip the current word being completed (index COMP_CWORD) to ask for suggestions based on the context so far local cmd_context=("${COMP_WORDS[@]:0:$COMP_CWORD}") - opts=$( "${cmd_context[@]}" --generate-shell-completion ) - + # Suggestions come as "name:description" (urfave/cli's default format); + # compgen -W splits on whitespace, so the description would otherwise be + # torn apart into bogus extra candidates. Keep only the name per line. + opts=$( "${cmd_context[@]}" --generate-shell-completion | cut -d: -f1 ) + COMPREPLY=( $(compgen -W "${opts}" -- ${cur}) ) return 0 fi diff --git a/internal/commands/completion/completion_test.go b/internal/commands/completion/completion_test.go new file mode 100644 index 0000000..a8dba78 --- /dev/null +++ b/internal/commands/completion/completion_test.go @@ -0,0 +1,88 @@ +package completion + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/thomas-vilte/matecommit/internal/i18n" + "github.com/urfave/cli/v3" +) + +func generateBashScript(t *testing.T) string { + t.Helper() + + translations, err := i18n.NewTranslations("en", "../../i18n/locales") + require.NoError(t, err) + + app := &cli.Command{ + Name: "test", + Commands: []*cli.Command{NewCompletionCommand(translations)}, + } + + oldStdout := os.Stdout + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stdout = w + defer func() { os.Stdout = oldStdout }() + + require.NoError(t, app.Run(context.Background(), []string{"test", "completion", "bash"})) + require.NoError(t, w.Close()) + + data := make([]byte, 0, 4096) + buf := make([]byte, 4096) + for { + n, readErr := r.Read(buf) + data = append(data, buf[:n]...) + if readErr != nil { + break + } + } + + return string(data) +} + +// TestBashCompletion_StripsDescriptions verifies the generated bash +// completion function against a real bash process. urfave/cli's +// --generate-shell-completion emits "name:description" lines, and compgen +// -W splits its wordlist on whitespace — so without stripping the +// description first, each of its words leaks in as a bogus extra +// candidate instead of just the real command/flag name. +func TestBashCompletion_StripsDescriptions(t *testing.T) { + if _, err := exec.LookPath("bash"); err != nil { + t.Skip("bash not available") + } + + script := generateBashScript(t) + + dir := t.TempDir() + scriptFile := filepath.Join(dir, "completion.bash") + require.NoError(t, os.WriteFile(scriptFile, []byte(script), 0600)) + + fakeBin := filepath.Join(dir, "matecommit") + fakeBinContent := "#!/bin/sh\n" + + "echo 'config:Manage configuration'\n" + + "echo 'suggest:Generate commit message suggestions'\n" + require.NoError(t, os.WriteFile(fakeBin, []byte(fakeBinContent), 0700)) + + driver := ` +source "` + scriptFile + `" +COMP_WORDS=(matecommit conf) +COMP_CWORD=1 +_mate_commit_bash_autocomplete +printf '%s\n' "${COMPREPLY[@]}" +` + cmd := exec.Command("bash", "-c", driver) + cmd.Env = append(os.Environ(), "PATH="+dir+":"+os.Getenv("PATH")) + out, err := cmd.CombinedOutput() + require.NoError(t, err, string(out)) + + lines := strings.Fields(strings.TrimSpace(string(out))) + assert.Equal(t, []string{"config"}, lines, + "description words must not leak into the completion candidates") +}