From 9b62285654aecbe7ff86a65e69b7fbd470e0d901 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 02:22:40 +0200 Subject: [PATCH] fix(bonfire): mask secret flag values in the wrong-directory hint - the Try line replaces the value of every long flag whose name contains password, passwd, secret, token or key with ***, in the --flag value and --flag=value forms; other arguments are unchanged - README and the writing-commands page say so (WR-08) --- docs/console/writing-commands.md | 2 +- modules/bonfire/README.md | 2 +- modules/bonfire/errors.go | 52 +++++++++++++++++++++++++++++--- modules/bonfire/errors_test.go | 44 +++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 7 deletions(-) diff --git a/docs/console/writing-commands.md b/docs/console/writing-commands.md index a862ce4c..5cc19088 100644 --- a/docs/console/writing-commands.md +++ b/docs/console/writing-commands.md @@ -202,7 +202,7 @@ An application binary also explains the most common mistake: running it from the cd ../..; bin/acme migrate ``` -Only the message changes: the binary still reads its configuration from `config/` in the working directory. `summer` is not affected, because its commands find the application root themselves. +The suggested command repeats your arguments, except that the value of a flag whose name contains `password`, `passwd`, `secret`, `token` or `key` is masked as `***` (`--password ***`, `--api-token=***`), so a secret typed on the command line does not reach logs that capture stderr. Only the message changes: the binary still reads its configuration from `config/` in the working directory. `summer` is not affected, because its commands find the application root themselves. ## Documentation coverage diff --git a/modules/bonfire/README.md b/modules/bonfire/README.md index d3d04fce..a47d5684 100644 --- a/modules/bonfire/README.md +++ b/modules/bonfire/README.md @@ -46,7 +46,7 @@ Use cases: - Hidden commands (`bonfire.Command.Hidden`) run but stay out of the list unless `list --all`. - JSON catalog: `list --format=json` prints every command, hidden ones flagged, with arguments, options, defaults and examples as a `bonfire.Listing` (`bonfire.Describe` builds it from a command slice). - Documentation coverage: `bonfire.CheckDocs` reports commands lacking a description, long help, examples, or argument and option descriptions. -- Error block and exit codes: `bonfire.Execute` runs the root through fang and returns `bonfire.ExitOK`, `bonfire.ExitFailure`, or `bonfire.ExitUsage` for an unknown command or flag and a wrong number of arguments (`bonfire.UsageError`). Errors print on the error stream as a block (`bonfire.RenderError`): the ERROR badge and the message, the explanation and Try lines of `bonfire.WithHint` (`{bin}` becomes `bin/` or `summer`), cobra's "did you mean" suggestions as Try lines, and "Run with -v for details." or, at `-v`, the wrapped error chain; every message passes through `sunscreen.Scrub`. Errors wrapped by `bonfire.Reported` print nothing. An error carrying a code through `bonfire.WithExitCode` exits with that code; summer uses it to exit with the code of an app binary it ran. `bonfire.Fail` prints errors that happen before the root exists. For an application binary (any root other than `summer`) that fails, other than with a usage error, while the working directory has no `config/` directory and a parent directory holds `summer.yaml`, both add the hint "This command runs from the application root." with `cd ; bin/ ` (only the message changes). +- Error block and exit codes: `bonfire.Execute` runs the root through fang and returns `bonfire.ExitOK`, `bonfire.ExitFailure`, or `bonfire.ExitUsage` for an unknown command or flag and a wrong number of arguments (`bonfire.UsageError`). Errors print on the error stream as a block (`bonfire.RenderError`): the ERROR badge and the message, the explanation and Try lines of `bonfire.WithHint` (`{bin}` becomes `bin/` or `summer`), cobra's "did you mean" suggestions as Try lines, and "Run with -v for details." or, at `-v`, the wrapped error chain; every message passes through `sunscreen.Scrub`. Errors wrapped by `bonfire.Reported` print nothing. An error carrying a code through `bonfire.WithExitCode` exits with that code; summer uses it to exit with the code of an app binary it ran. `bonfire.Fail` prints errors that happen before the root exists. For an application binary (any root other than `summer`) that fails, other than with a usage error, while the working directory has no `config/` directory and a parent directory holds `summer.yaml`, both add the hint "This command runs from the application root." with `cd ; bin/ ` (only the message changes); the values of flags named like a password, secret, token or key are masked as `***` in that line. - Name checks at boot: two commands with the same name, or a command named `list` or `help`, make `bonfire.NewRoot` fail with `bonfire.ErrDuplicateCommand`; a command flag that reuses a global option's name or shorthand fails with `bonfire.ErrReservedFlag` instead of panicking. - Injectable streams (`bonfire.NewRootIO`, `bonfire.NewOutput`) so commands can be tested against buffers. - In-process calls: `bonfire.Call` runs a named command with arguments against any writer (Laravel `Artisan::call`), and `bonfire.Catalog` holds an application binary's final command list so code outside the Cobra root, such as the conga scheduler, can call any registered command. diff --git a/modules/bonfire/errors.go b/modules/bonfire/errors.go index 9347823d..4d4e3085 100644 --- a/modules/bonfire/errors.go +++ b/modules/bonfire/errors.go @@ -205,7 +205,8 @@ const wrongDirectoryExplanation = "This command runs from the application root." // name other than summer) failed while the working directory dir ("" means // os.Getwd) has no config directory and a parent directory holds // summer.yaml: compass then found no configuration, which is why the command -// failed (D-19). The Try line is "cd ; bin/ ". +// failed (D-19). The Try line is "cd ; bin/ ", +// with the values of secret-looking flags masked (tryArgs). // Usage errors, Reported errors and summer are left alone; nothing else // about the failure changes. func wrongDirectory(name string, err error, args []string, dir string) error { @@ -239,13 +240,54 @@ func wrongDirectory(name string, err error, args []string, dir string) error { if rerr != nil { return err } - try := "cd " + shellQuote(rel) + "; {bin}" - for _, a := range args { - try += " " + shellQuote(a) - } + try := "cd " + shellQuote(rel) + "; {bin}" + tryArgs(args) return &HintError{Err: err, Explanation: wrongDirectoryExplanation, Try: try} } +// secretWords mark a long flag whose value the Try line masks. +var secretWords = []string{"password", "passwd", "secret", "token", "key"} + +// secretFlag reports whether the long flag name (without "--") looks like it +// carries a secret. +func secretFlag(name string) bool { + name = strings.ToLower(name) + for _, w := range secretWords { + if strings.Contains(name, w) { + return true + } + } + return false +} + +// tryArgs renders args for the wrong-directory Try line, each preceded by a +// space and quoted for a shell. The value of a long flag named like a +// secret becomes ***, in the "--flag=value" form and in the "--flag value" +// form (the next argument, when it does not start with "-"), so a password +// typed on the command line does not reach stderr and CI logs. +func tryArgs(args []string) string { + var b strings.Builder + for i := 0; i < len(args); i++ { + a := args[i] + b.WriteByte(' ') + name, long := strings.CutPrefix(a, "--") + flag, _, inline := strings.Cut(name, "=") + if !long || flag == "" || !secretFlag(flag) { + b.WriteString(shellQuote(a)) + continue + } + if inline { + b.WriteString(shellQuote("--"+flag) + "=***") + continue + } + b.WriteString(shellQuote(a)) + if i+1 < len(args) && !strings.HasPrefix(args[i+1], "-") { + b.WriteString(" ***") + i++ + } + } + return b.String() +} + // shellQuote single-quotes s when a shell would split or expand it. func shellQuote(s string) string { if s != "" && !strings.ContainsAny(s, " \t\n'\"\\$`*?[]{}()<>|&;#~!") { diff --git a/modules/bonfire/errors_test.go b/modules/bonfire/errors_test.go index 357afdd2..b6497b37 100644 --- a/modules/bonfire/errors_test.go +++ b/modules/bonfire/errors_test.go @@ -218,3 +218,47 @@ func TestWithExitCode(t *testing.T) { t.Errorf("exitCode through a wrapper = %d, want 4", code) } } + +// TestWrongDirectoryMasksSecrets (WR-08): the wrong-directory Try line +// replaces the value of every flag named like a password, secret, token or +// key with ***, in both the "--flag value" and "--flag=value" forms, and +// keeps every other argument. +func TestWrongDirectoryMasksSecrets(t *testing.T) { + plainEnv(t) + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "summer.yaml"), []byte("binary: acme\n"), 0o644); err != nil { + t.Fatal(err) + } + sub := filepath.Join(root, "bin") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + fail := errors.New("lagoon: database.dsn is empty") + cases := []struct { + args []string + want string + }{ + {[]string{"admin:create", "--email", "a@b.c", "--password", "hunter2"}, "cd ..; {bin} admin:create --email a@b.c --password ***"}, + {[]string{"admin:create", "--email", "a@b.c", "--password=hunter2"}, "cd ..; {bin} admin:create --email a@b.c --password=***"}, + {[]string{"sync", "--client-secret", "s3cr3t", "--verbose"}, "cd ..; {bin} sync --client-secret *** --verbose"}, + {[]string{"sync", "--api-token=t0k3n"}, "cd ..; {bin} sync --api-token=***"}, + {[]string{"sync", "--key", "k3y", "acme blog"}, "cd ..; {bin} sync --key *** 'acme blog'"}, + {[]string{"sync", "--PASSWD", "hunter2"}, "cd ..; {bin} sync --PASSWD ***"}, + {[]string{"sync", "--token", "--force"}, "cd ..; {bin} sync --token --force"}, + } + for _, tc := range cases { + got := wrongDirectory("acme", fail, tc.args, sub) + var hint *HintError + if !errors.As(got, &hint) { + t.Fatalf("%q: no hint: %v", tc.args, got) + } + if hint.Try != tc.want { + t.Errorf("%q: Try = %q, want %q", tc.args, hint.Try, tc.want) + } + for _, secret := range []string{"hunter2", "s3cr3t", "t0k3n", "k3y"} { + if strings.Contains(hint.Try, secret) { + t.Errorf("%q: Try line repeats %q: %q", tc.args, secret, hint.Try) + } + } + } +}