From 707372bf657e205c6fe58812d11dee42a9671028 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 03:57:55 +0200 Subject: [PATCH] fix(bonfire): mask secret flag values that start with a dash (WR-11) --- docs/console/writing-commands.md | 2 +- modules/bonfire/errors.go | 9 ++++++--- modules/bonfire/errors_test.go | 18 ++++++++++++------ 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/docs/console/writing-commands.md b/docs/console/writing-commands.md index 61c327a9..c9fa4558 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 ``` -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. +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 ***` (also a value that starts with `-`), `--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/errors.go b/modules/bonfire/errors.go index 589963b8..dd31a9d1 100644 --- a/modules/bonfire/errors.go +++ b/modules/bonfire/errors.go @@ -306,8 +306,11 @@ func secretFlag(name string) bool { // 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. +// form, so a password typed on the command line does not reach stderr and +// CI logs. In the "--flag value" form the next argument is masked whatever +// its first character (pflag takes "-Xk9" as the value too); only "--" +// is left alone. tryArgs cannot see flag types, so a secret-named boolean +// flag over-masks the argument after it, which is the safe side. func tryArgs(args []string) string { var b strings.Builder for i := 0; i < len(args); i++ { @@ -324,7 +327,7 @@ func tryArgs(args []string) string { continue } b.WriteString(shellQuote(a)) - if i+1 < len(args) && !strings.HasPrefix(args[i+1], "-") { + if i+1 < len(args) && args[i+1] != "--" { b.WriteString(" ***") i++ } diff --git a/modules/bonfire/errors_test.go b/modules/bonfire/errors_test.go index 1441c49a..b9457a84 100644 --- a/modules/bonfire/errors_test.go +++ b/modules/bonfire/errors_test.go @@ -219,10 +219,11 @@ func TestWithExitCode(t *testing.T) { } } -// 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. +// TestWrongDirectoryMasksSecrets (WR-08, WR-11): 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 whatever the value's first character, and keeps every other +// argument; only a following "--" is not taken as the value. func TestWrongDirectoryMasksSecrets(t *testing.T) { plainEnv(t) root := t.TempDir() @@ -244,7 +245,12 @@ func TestWrongDirectoryMasksSecrets(t *testing.T) { {[]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"}, + // pflag takes the next argument as the value of a non-boolean + // flag even when it starts with "-": "--force" is the token here. + {[]string{"sync", "--token", "--force"}, "cd ..; {bin} sync --token ***"}, + {[]string{"admin:create", "--email", "a@b.c", "--password", "-Xk9!q"}, "cd ..; {bin} admin:create --email a@b.c --password ***"}, + {[]string{"sync", "--client-secret", "-s3cr3t", "--verbose"}, "cd ..; {bin} sync --client-secret *** --verbose"}, + {[]string{"sync", "--token", "--", "x"}, "cd ..; {bin} sync --token -- x"}, } for _, tc := range cases { got := wrongDirectory("acme", fail, tc.args, sub) @@ -255,7 +261,7 @@ func TestWrongDirectoryMasksSecrets(t *testing.T) { 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"} { + for _, secret := range []string{"hunter2", "s3cr3t", "t0k3n", "k3y", "-Xk9", "-s3cr3t"} { if strings.Contains(hint.Try, secret) { t.Errorf("%q: Try line repeats %q: %q", tc.args, secret, hint.Try) }