From b93a06b16685aa52684fbd68c7c1f014b7f62b34 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Wed, 29 Jul 2026 18:19:02 +0100 Subject: [PATCH 1/2] fix(delete): fail when the deletion prompt cannot be answered `kosli delete service-account` and `kosli delete api-key` read the confirmation answer with bufio.Reader.ReadString and excluded io.EOF from the error path, so an EOF was indistinguishable from a typed refusal. When stdin is not interactive (any CI job, < /dev/null, a closed stdin) both commands printed the prompt, read EOF, reported "was cancelled." and returned nil - exit 0 without deleting anything. A pipeline that revokes credentials could not tell that the revocation had not happened, since the exit code is the only signal it consumes. EOF with nothing read means there was nobody to ask, which is not a refusal: it is now an error naming --assume-yes, so the command exits non-zero. A typed "n", any other word, or a bare Enter still cancels with exit 0, and an answer with no trailing newline (printf 'y' | kosli delete) is still honoured - ReadString returns io.EOF together with the bytes read before it. The EOF rule lives in one new shared helper, confirmDeletion, next to the other shared cmd helpers; the two per-command functions now only build their prompt. The test harness gained a stdin seam (cmdTestCase.stdin) so an answer can be typed at the prompt. Fix requests and warn instead of exiting when payload logging fails. The debug-only payload log used Logger.Error, which is log.Fatalf and so exits 1, while the message it printed said "Continuing with the request...". The message and the behaviour disagreed Fixes #1056 --- cmd/kosli/apiKey_test.go | 36 ++++++++++++++- cmd/kosli/cli_utils.go | 28 ++++++++++++ cmd/kosli/cli_utils_test.go | 76 +++++++++++++++++++++++++++++++ cmd/kosli/deleteApiKey.go | 21 ++++----- cmd/kosli/deleteServiceAccount.go | 21 ++++----- cmd/kosli/serviceAccount_test.go | 19 +++++++- cmd/kosli/testHelpers.go | 20 ++++++-- internal/requests/requests.go | 5 +- 8 files changed, 192 insertions(+), 34 deletions(-) diff --git a/cmd/kosli/apiKey_test.go b/cmd/kosli/apiKey_test.go index 11116b6dd..386b92b86 100644 --- a/cmd/kosli/apiKey_test.go +++ b/cmd/kosli/apiKey_test.go @@ -233,10 +233,18 @@ func (suite *ApiKeyCommandTestSuite) TestRotateApiKeyCmd() { func (suite *ApiKeyCommandTestSuite) TestDeleteApiKeyCmd() { tests := []cmdTestCase{ + { + wantError: true, + name: "an unanswerable prompt (empty stdin) fails instead of reporting success", + cmd: "delete api-key key-123 --service-account test-sa" + suite.defaultKosliArguments, + golden: "Are you sure you want to delete API key(s) key-123 for service account test-sa? [y/N] " + + "Error: cannot confirm deletion: stdin is not interactive, re-run with --assume-yes to delete without confirmation\n", + }, { wantError: false, - name: "delete without confirmation (empty stdin) is cancelled and makes no call", + name: "a typed refusal is cancelled and makes no call", cmd: "delete api-key key-123 --service-account test-sa" + suite.defaultKosliArguments, + stdin: "n\n", golden: "Are you sure you want to delete API key(s) key-123 for service account test-sa? [y/N] Deletion of API key(s) key-123 was cancelled.\n", }, { @@ -460,6 +468,32 @@ func (suite *ApiKeyCommandTestSuite) TestDeleteApiKeyNotFound() { runTestCmd(suite.T(), tests) } +// TestDeleteApiKeyConfirmed stubs a successful delete to verify that an answer +// typed at the confirmation prompt reaches the API. +func (suite *ApiKeyCommandTestSuite) TestDeleteApiKeyConfirmed() { + fake := httpfake.New() + defer fake.Close() + fake.NewHandler(). + Delete("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys/key-123"). + Reply(200). + BodyString(apiKeyFixture(suite.T(), "revoke_success.json")) + + args := fmt.Sprintf(" --host %s --org %s --api-token %s", fake.Server.URL, global.Org, global.ApiToken) + tests := []cmdTestCase{ + { + wantError: false, + // no trailing newline: ReadString returns the answer together with + // io.EOF, which must still be honoured as a confirmation + name: "a typed y without a trailing newline confirms and deletes", + cmd: "delete api-key key-123 --service-account test-sa" + args, + stdin: "y", + goldenRegex: `API key key-123 for service account test-sa was deleted!`, + }, + } + + runTestCmd(suite.T(), tests) +} + // TestApiErrorsAreSurfaced stubs the API with 4xx responses to verify that // create/rotate/list surface the server's error message instead of succeeding. func (suite *ApiKeyCommandTestSuite) TestApiErrorsAreSurfaced() { diff --git a/cmd/kosli/cli_utils.go b/cmd/kosli/cli_utils.go index 1471c045b..cc5d68714 100644 --- a/cmd/kosli/cli_utils.go +++ b/cmd/kosli/cli_utils.go @@ -1,6 +1,7 @@ package main import ( + "bufio" "encoding/json" "fmt" "io" @@ -71,6 +72,33 @@ func style(out io.Writer, s string, codes ...string) string { return strings.Join(codes, "") + s + ansiReset } +// confirmDeletion prints prompt (which carries no trailing newline, so the +// answer is typed on the same line) and reports whether the answer is an +// affirmative "y"/"yes" (case-insensitive). A typed "n", any other word, or a +// bare Enter is a refusal and returns (false, nil). +// +// Reaching EOF with nothing read means there was nobody to ask — stdin is not +// interactive — which is an error, not a refusal. Treating it as a refusal made +// the delete commands exit 0 without deleting anything, so a pipeline could not +// tell that the deletion had not happened (issue #1056). +func confirmDeletion(prompt string, in io.Reader) (bool, error) { + logger.Print("%s", prompt) + + answer, err := bufio.NewReader(in).ReadString('\n') + if err != nil && err != io.EOF { + return false, err + } + // ReadString returns io.EOF together with any bytes read before it, so a + // final line without a trailing newline (printf 'y' | kosli delete ...) is + // still a real answer; only an empty read means nobody answered. + if err == io.EOF && strings.TrimSpace(answer) == "" { + return false, fmt.Errorf("cannot confirm deletion: stdin is not interactive, re-run with --assume-yes to delete without confirmation") + } + + answer = strings.ToLower(strings.TrimSpace(answer)) + return answer == "y" || answer == "yes", nil +} + const ( bitbucket = "Bitbucket" github = "Github" diff --git a/cmd/kosli/cli_utils_test.go b/cmd/kosli/cli_utils_test.go index ef3e3bd4a..cf3e3e508 100644 --- a/cmd/kosli/cli_utils_test.go +++ b/cmd/kosli/cli_utils_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" log "github.com/kosli-dev/cli/internal/logger" @@ -1111,6 +1112,81 @@ func (suite *CliUtilsTestSuite) TestHandleArtifactExpression() { } } +// TestConfirmDeletion covers the confirmation prompt used by the delete +// commands. The critical case is an unanswerable prompt (stdin at EOF with +// nothing typed): it must be an error, not a refusal, so the command exits +// non-zero instead of reporting success without deleting (issue #1056). +func (suite *CliUtilsTestSuite) TestConfirmDeletion() { + for _, t := range []struct { + name string + input string + wantConfirmed bool + wantErr bool + }{ + { + name: "EOF with nothing typed is not a refusal but an error", + input: "", + wantErr: true, + }, + { + name: "an answer without a trailing newline is still honoured", + input: "y", + wantConfirmed: true, + }, + { + name: "y confirms", + input: "y\n", + wantConfirmed: true, + }, + { + name: "yes confirms", + input: "yes\n", + wantConfirmed: true, + }, + { + name: "the answer is case-insensitive", + input: " YES \n", + wantConfirmed: true, + }, + { + name: "n refuses", + input: "n\n", + }, + { + name: "a bare newline refuses", + input: "\n", + }, + { + name: "anything else refuses", + input: "maybe\n", + }, + } { + suite.Run(t.name, func() { + infoBuf := new(bytes.Buffer) + defer restoreLogger(newTestLoggerWithInfo(infoBuf))() + + confirmed, err := confirmDeletion("Are you sure? [y/N] ", strings.NewReader(t.input)) + + require.Equal(suite.T(), t.wantErr, err != nil, "unexpected error: %v", err) + if t.wantErr { + require.Contains(suite.T(), err.Error(), "--assume-yes", + "the error must tell the user how to delete non-interactively") + } + require.Equal(suite.T(), t.wantConfirmed, confirmed) + require.Equal(suite.T(), "Are you sure? [y/N] ", infoBuf.String(), + "the prompt is printed without a trailing newline") + }) + } +} + +// restoreLogger swaps the package-level logger for l and returns a function +// that puts the original back. +func restoreLogger(l *log.Logger) func() { + original := logger + logger = l + return func() { logger = original } +} + func Test_prefixEachLine(t *testing.T) { tests := []struct { name string diff --git a/cmd/kosli/deleteApiKey.go b/cmd/kosli/deleteApiKey.go index 7def63c1f..9a49ae028 100644 --- a/cmd/kosli/deleteApiKey.go +++ b/cmd/kosli/deleteApiKey.go @@ -1,7 +1,6 @@ package main import ( - "bufio" "fmt" "io" "net/http" @@ -18,7 +17,10 @@ const deleteApiKeyLongDesc = deleteApiKeyShortDesc + ` This permanently deletes the API key(s) identified by KEY-ID. Deletion is immediate and cannot be undone. You are asked to confirm before the key is deleted; use -^--assume-yes^/^--yes^ to skip the confirmation prompt.` +^--assume-yes^/^--yes^ to skip the confirmation prompt. + +When stdin is not interactive (e.g. in CI) the prompt cannot be answered and the +command fails without deleting anything, so pass ^--assume-yes^ there.` const deleteApiKeyExample = ` # delete an API key for a service account (asks for confirmation): @@ -138,17 +140,10 @@ func styleApiKeyIDs(keyIDs []string) []string { } // confirmApiKeyDeletion prompts the user to confirm deletion and returns true -// only when the answer is an affirmative "y"/"yes" (case-insensitive). The -// prompt has no trailing newline so the answer is typed on the same line. +// only when the answer is an affirmative "y"/"yes" (case-insensitive). It errors +// when the prompt cannot be answered at all, see confirmDeletion. func confirmApiKeyDeletion(keyIDs []string, serviceAccount string, in io.Reader) (bool, error) { - logger.Print("Are you sure you want to delete API key(s) %s for service account %s? [y/N] ", + prompt := fmt.Sprintf("Are you sure you want to delete API key(s) %s for service account %s? [y/N] ", strings.Join(styleApiKeyIDs(keyIDs), ", "), style(logger.Out, serviceAccount, ansiBold, ansiGreen)) - - answer, err := bufio.NewReader(in).ReadString('\n') - if err != nil && err != io.EOF { - return false, err - } - - answer = strings.ToLower(strings.TrimSpace(answer)) - return answer == "y" || answer == "yes", nil + return confirmDeletion(prompt, in) } diff --git a/cmd/kosli/deleteServiceAccount.go b/cmd/kosli/deleteServiceAccount.go index e397d56e6..c0caaaa05 100644 --- a/cmd/kosli/deleteServiceAccount.go +++ b/cmd/kosli/deleteServiceAccount.go @@ -1,7 +1,6 @@ package main import ( - "bufio" "fmt" "io" "net/http" @@ -19,7 +18,10 @@ const deleteServiceAccountLongDesc = deleteServiceAccountShortDesc + ` This permanently removes the service account(s) identified by SERVICE-ACCOUNT-NAME from the organization, along with their API keys. Deletion is immediate and cannot be undone. You are asked to confirm before deletion; use -^--assume-yes^/^--yes^ to skip the confirmation prompt.` +^--assume-yes^/^--yes^ to skip the confirmation prompt. + +When stdin is not interactive (e.g. in CI) the prompt cannot be answered and the +command fails without deleting anything, so pass ^--assume-yes^ there.` const deleteServiceAccountExample = ` # delete a service account (asks for confirmation): @@ -130,17 +132,10 @@ func styleServiceAccountNames(names []string) []string { // confirmServiceAccountDeletion prompts the user to confirm deletion and // returns true only when the answer is an affirmative "y"/"yes" -// (case-insensitive). The prompt has no trailing newline so the answer is -// typed on the same line. +// (case-insensitive). It errors when the prompt cannot be answered at all, see +// confirmDeletion. func confirmServiceAccountDeletion(names []string, in io.Reader) (bool, error) { - logger.Print("Are you sure you want to delete service account(s) %s? [y/N] ", + prompt := fmt.Sprintf("Are you sure you want to delete service account(s) %s? [y/N] ", strings.Join(styleServiceAccountNames(names), ", ")) - - answer, err := bufio.NewReader(in).ReadString('\n') - if err != nil && err != io.EOF { - return false, err - } - - answer = strings.ToLower(strings.TrimSpace(answer)) - return answer == "y" || answer == "yes", nil + return confirmDeletion(prompt, in) } diff --git a/cmd/kosli/serviceAccount_test.go b/cmd/kosli/serviceAccount_test.go index 6e9d09aa5..62855d333 100644 --- a/cmd/kosli/serviceAccount_test.go +++ b/cmd/kosli/serviceAccount_test.go @@ -174,10 +174,18 @@ func (suite *ServiceAccountCommandTestSuite) TestUpdateServiceAccountCmd() { func (suite *ServiceAccountCommandTestSuite) TestDeleteServiceAccountCmd() { tests := []cmdTestCase{ + { + wantError: true, + name: "an unanswerable prompt (empty stdin) fails instead of reporting success", + cmd: "delete service-account ci-bot" + suite.defaultKosliArguments, + golden: "Are you sure you want to delete service account(s) ci-bot? [y/N] " + + "Error: cannot confirm deletion: stdin is not interactive, re-run with --assume-yes to delete without confirmation\n", + }, { wantError: false, - name: "delete without confirmation (empty stdin) is cancelled and makes no call", + name: "a typed refusal is cancelled and makes no call", cmd: "delete service-account ci-bot" + suite.defaultKosliArguments, + stdin: "n\n", golden: "Are you sure you want to delete service account(s) ci-bot? [y/N] Deletion of service account(s) ci-bot was cancelled.\n", }, { @@ -280,6 +288,15 @@ func (suite *ServiceAccountCommandTestSuite) TestServiceAccountDeleteSuccess() { cmd: "delete service-account ci-bot --assume-yes" + args, goldenRegex: `service account ci-bot was deleted!`, }, + { + wantError: false, + // no trailing newline: ReadString returns the answer together with + // io.EOF, which must still be honoured as a confirmation + name: "a typed y without a trailing newline confirms and deletes", + cmd: "delete service-account ci-bot" + args, + stdin: "y", + goldenRegex: `service account ci-bot was deleted!`, + }, } runTestCmd(suite.T(), tests) diff --git a/cmd/kosli/testHelpers.go b/cmd/kosli/testHelpers.go index 96b7f05ea..682a87fd1 100644 --- a/cmd/kosli/testHelpers.go +++ b/cmd/kosli/testHelpers.go @@ -38,15 +38,25 @@ type cmdTestCase struct { goldenJson []jsonCheck // Use like this for array {"[0].compliant", false} goldenStdout string // expected stdout only (exact match, ignored when empty) goldenStderr string // expected stderr only (exact match, ignored when empty) + stdin string // fed to the command's stdin (empty means an immediate EOF) wantError bool additionalConfig interface{} } -// executeCommandC executes a command as a user would and returns the output -// split into combined, stdout-only, and stderr-only streams. +// executeCommandC executes a command as a user would, with an empty stdin (any +// read hits EOF immediately, as it does in CI), and returns the output split +// into combined, stdout-only, and stderr-only streams. +func executeCommandC(cmd string) (*cobra.Command, string, string, string, error) { + return executeCommandStdinC(cmd, "") +} + +// executeCommandStdinC executes a command as a user would with stdin as its +// standard input, and returns the output split into combined, stdout-only, and +// stderr-only streams. Use it for commands that read stdin, e.g. the delete +// commands' confirmation prompt. // This creates a new kosli command that is run, but it cannot be used in other tests // because newRootCmd overwrites the global options. -func executeCommandC(cmd string) (*cobra.Command, string, string, string, error) { +func executeCommandStdinC(cmd string, stdin string) (*cobra.Command, string, string, string, error) { args, err := shellwords.Parse(cmd) if err != nil { return nil, "", "", "", err @@ -67,7 +77,7 @@ func executeCommandC(cmd string) (*cobra.Command, string, string, string, error) root.SilenceErrors = false root.SetOut(outWriter) root.SetErr(errWriter) - root.SetIn(new(bytes.Buffer)) + root.SetIn(strings.NewReader(stdin)) root.SetArgs(normalizeBoolFlagArgs(root, args)) c, err := root.ExecuteC() @@ -89,7 +99,7 @@ func runTestCmd(t *testing.T, tests []cmdTestCase) { t.Error("golden and goldenPath cannot be set together") } t.Logf("running cmd: %s", tt.cmd) - _, combined, stdout, stderr, err := executeCommandC(tt.cmd) + _, combined, stdout, stderr, err := executeCommandStdinC(tt.cmd, tt.stdin) if (err != nil) != tt.wantError { t.Errorf("error expectation not matched\n\n WANT error is: %t\n\n but GOT: '%v'", tt.wantError, err) } diff --git a/internal/requests/requests.go b/internal/requests/requests.go index 0faf4a068..2ebc313eb 100644 --- a/internal/requests/requests.go +++ b/internal/requests/requests.go @@ -268,7 +268,10 @@ func (c *Client) Do(p *RequestParams) (*HTTPResponse, error) { c.Logger.Info("payload sent to: %s", req.URL) err := c.PayloadOutput(req, jsonFields, "this is the payload being sent:") if err != nil { - c.Logger.Error("failed to log payload: %v \nContinuing with the request...", err) + // Logger.Error is fatal (it exits), which would contradict the + // message and abort a request only because debug logging of its + // payload failed. Warn instead, as with the response body below. + c.Logger.Warn("failed to log payload: %v \nContinuing with the request...", err) } } resp, err := c.HttpClient.Do(req) From c4ced51313af2b6149099e08a9abb6774e995f39 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Wed, 29 Jul 2026 18:19:06 +0100 Subject: [PATCH 2/2] docs(delete): note the blank-line vs EOF distinction in confirmDeletion Review feedback on #1058: the two blank-looking inputs take different exit paths, which was deliberate but undocumented. A blank line terminated by a newline is a submitted answer and so a refusal (exit 0); blank input terminated by EOF was never submitted and errors. Say so in the doc comment and pin both in the table test. --- cmd/kosli/cli_utils.go | 5 +++++ cmd/kosli/cli_utils_test.go | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/cmd/kosli/cli_utils.go b/cmd/kosli/cli_utils.go index cc5d68714..5413442f7 100644 --- a/cmd/kosli/cli_utils.go +++ b/cmd/kosli/cli_utils.go @@ -81,6 +81,11 @@ func style(out io.Writer, s string, codes ...string) string { // interactive — which is an error, not a refusal. Treating it as a refusal made // the delete commands exit 0 without deleting anything, so a pipeline could not // tell that the deletion had not happened (issue #1056). +// +// Blank-looking input therefore splits on whether an answer was submitted at +// all, not on what it contains: a blank line terminated by a newline (`printf +// ' \n'`) is a deliberate refusal and exits 0, while blank input terminated by +// EOF (`printf ' '`) is no submission at all and errors. func confirmDeletion(prompt string, in io.Reader) (bool, error) { logger.Print("%s", prompt) diff --git a/cmd/kosli/cli_utils_test.go b/cmd/kosli/cli_utils_test.go index cf3e3e508..63c32ac2b 100644 --- a/cmd/kosli/cli_utils_test.go +++ b/cmd/kosli/cli_utils_test.go @@ -1128,6 +1128,17 @@ func (suite *CliUtilsTestSuite) TestConfirmDeletion() { input: "", wantErr: true, }, + { + // the two blank-looking inputs below split on whether an answer was + // submitted at all, not on what it contains + name: "blank input ending at EOF was never submitted, so it errors", + input: " ", + wantErr: true, + }, + { + name: "a blank line ending in a newline was submitted, so it refuses", + input: " \n", + }, { name: "an answer without a trailing newline is still honoured", input: "y",