From 74cb734c4bb0c2891e94c9995fd9f048162c280a Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Fri, 21 Aug 2026 14:30:08 +0200 Subject: [PATCH] fix: two outputs that end the process in the middle of the work they announce MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `display.OutputWarning` and `display.OutputError` do not return. `OutputWithFormat` finishes on `ExitFunc(0)` and `ExitFunc(1)`, and `ExitFunc` is `os.Exit`, so everything after such a call is dead code in the shipped binary. An AST sweep of internal/ finds four such calls on main; both sites are here. **`logout` kept the key it said it removed.** The remote revocation is best effort — its own comment says "the credentials may already be invalid, in which case we still want to clean up the local configuration" — but each of its three unhappy paths called OutputWarning. So the command stopped there, exit 0, having printed "credentials were already invalid or revoked, skipping remote revocation", a sentence that reads as carrying on, while the local cleanup never ran. Reproduced against the built binary with a bogus key in ./ovh.conf: exit 0 and consumer_key untouched. Verified after the fix: removed. Three tests, one per unhappy path, using the real exit semantics rather than the suite's no-op ExitFunc — that stub is exactly what hid this for as long as it existed. The outcome of the revocation is now recorded and reported in the final message, which also makes it one document instead of two: under -o json a warning followed by a result put two JSON documents on one stdout, which no parser accepts. **One unreadable quota killed the whole registry plans listing.** formatContainerRegistryPlans called OutputError when imageStorage would not parse, and it runs once per plan inside the listing loop — so a single malformed value ended the command with the other plans already collected and never shown. A quota that will not parse is one unreadable cell; the raw value is kept, and the listing continues. Under test that call looked harmless, because the no-op ExitFunc let execution fall through to the next line with the size still zero — writing "0", a wrong quota wearing the shape of a real one. The test asserts on both halves. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- .../services/cloud/cloud_managed_registry.go | 20 +++- .../cloud/cloud_managed_registry_test.go | 57 ++++++++++++ internal/services/login/logout.go | 35 +++++-- internal/services/login/logout_test.go | 91 +++++++++++++++++++ 4 files changed, 192 insertions(+), 11 deletions(-) create mode 100644 internal/services/cloud/cloud_managed_registry_test.go diff --git a/internal/services/cloud/cloud_managed_registry.go b/internal/services/cloud/cloud_managed_registry.go index c252f458b..eb364d5a0 100644 --- a/internal/services/cloud/cloud_managed_registry.go +++ b/internal/services/cloud/cloud_managed_registry.go @@ -564,12 +564,22 @@ func formatContainerRegistryPlans(plan map[string]any) { // Extract and format registry limits if registryLimits, ok := plan["registryLimits"].(map[string]any); ok { if imageStorage, ok := registryLimits["imageStorage"].(json.Number); ok { - imageStorage, err := imageStorage.Int64() - if err != nil { - display.OutputError(&flags.OutputFormatConfig, "%s", err) + // A quota that will not parse is one unreadable cell, not a reason to + // answer nothing. This called display.OutputError, which does not + // return — OutputWithFormat finishes on ExitFunc(1), which is + // os.Exit — and it runs once per plan inside the listing loop, so a + // single malformed imageStorage killed the whole `registry plans` + // listing with the other plans already collected and never shown. + // + // Worse under test: display.ExitFunc is stubbed to a no-op there, so + // execution fell through to the line below with imageStorage still + // zero and wrote "0" — a wrong quota that looks like a real one. The + // raw value is kept instead, so the cell says what the API said. + if size, err := imageStorage.Int64(); err != nil { + plan["imageStorage"] = imageStorage.String() + } else { + plan["imageStorage"] = bytefmt.ByteSize(uint64(size)) } - - plan["imageStorage"] = bytefmt.ByteSize(uint64(imageStorage)) } if parallelRequest, ok := registryLimits["parallelRequest"].(json.Number); ok { plan["parallelRequest"] = parallelRequest diff --git a/internal/services/cloud/cloud_managed_registry_test.go b/internal/services/cloud/cloud_managed_registry_test.go new file mode 100644 index 000000000..5031af00b --- /dev/null +++ b/internal/services/cloud/cloud_managed_registry_test.go @@ -0,0 +1,57 @@ +// SPDX-FileCopyrightText: 2026 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package cloud + +import ( + "encoding/json" + "testing" + + "github.com/maxatome/go-testdeep/td" + "github.com/ovh/ovhcloud-cli/internal/display" +) + +// A quota that will not parse is one unreadable cell, not a reason to answer +// nothing. This ran display.OutputError, which does not return — +// OutputWithFormat finishes on ExitFunc(1), which is os.Exit — once per plan +// inside the listing loop, so a single malformed imageStorage killed the whole +// `registry plans` listing with the other plans already collected and never +// shown. +// +// Both halves are asserted, because the no-op ExitFunc the suite installs made +// the old code look like it merely printed a warning: execution fell through with +// imageStorage still zero and wrote "0" — a wrong quota wearing the shape of a +// real one. +func TestAPlanWithAnUnreadableQuotaDoesNotKillTheListing(t *testing.T) { + saved := display.ExitFunc + t.Cleanup(func() { display.ExitFunc = saved }) + + exited := false + display.ExitFunc = func(int) { exited = true } + + plan := map[string]any{ + "registryLimits": map[string]any{ + "imageStorage": json.Number("not-a-number"), + "parallelRequest": json.Number("42"), + }, + } + + formatContainerRegistryPlans(plan) + + td.Cmp(t, exited, false, "one bad plan must not stop the command") + td.Cmp(t, plan["imageStorage"], "not-a-number", + "the cell says what the API said, rather than the 0 the old fall-through wrote") + td.Cmp(t, plan["parallelRequest"], json.Number("42"), "and the rest of the plan is still read") +} + +// Positive control: a quota that does parse is still formatted. +func TestAReadableQuotaIsStillFormatted(t *testing.T) { + plan := map[string]any{ + "registryLimits": map[string]any{"imageStorage": json.Number("10737418240")}, + } + + formatContainerRegistryPlans(plan) + + td.Cmp(t, plan["imageStorage"], "10G") +} diff --git a/internal/services/login/logout.go b/internal/services/login/logout.go index 639cbadd7..496effb58 100644 --- a/internal/services/login/logout.go +++ b/internal/services/login/logout.go @@ -54,26 +54,49 @@ func Logout(_ *cobra.Command, _ []string) { // 1. Revoke the current credentials server-side (best effort): the // credentials may already be invalid, in which case we still want to clean // up the local configuration. + // + // Its outcome is recorded and reported at the end rather than printed here. + // It used to be printed here, by display.OutputWarning — and OutputWarning + // does not return: OutputWithFormat finishes on ExitFunc(0), which is + // os.Exit. So on every path but the happy one, `ovhcloud logout` stopped + // right here, exit 0, having printed "skipping remote revocation" — a + // sentence that reads as "carrying on" — while step 2 never ran and the + // credentials stayed in the configuration file. Reproduced against the built + // binary with a revoked key: exit 0, and consumer_key still on disk. + // + // The comment above already said the opposite of what the code did. + revocation := "not attempted" if httpLib.Client != nil { switch err := httpLib.Client.Post("/auth/logout", nil, nil); { case err == nil: - display.OutputInfo(&flags.OutputFormatConfig, nil, "🔒 API credentials revoked") + revocation = "revoked via the API" case isInvalidCredentialError(err): - display.OutputWarning(&flags.OutputFormatConfig, "credentials were already invalid or revoked, skipping remote revocation") + revocation = "already invalid or revoked, so nothing to revoke remotely" default: - display.OutputWarning(&flags.OutputFormatConfig, "could not revoke credentials via the API: %s", err) + revocation = fmt.Sprintf("could not be revoked via the API: %s", err) } } else { - display.OutputWarning(&flags.OutputFormatConfig, "API client not initialized, skipping remote revocation") + revocation = "not revoked remotely: the API client was not initialised" } // 2. Remove the credentials from the local configuration. + // + // This is the half that matters, and the half that used to be skipped: the + // remote revocation is a courtesy, taking the key off this disk is the + // command. if err := config.DeleteCredentials(cfg, path, section); err != nil { - display.OutputError(&flags.OutputFormatConfig, "failed to remove credentials from configuration: %s", err) + display.OutputError(&flags.OutputFormatConfig, + "failed to remove credentials from %s: %s\n The remote revocation %s, so the key on this disk is what is left to deal with.", + path, err, revocation) return } - display.OutputInfo(&flags.OutputFormatConfig, nil, "✅ Logged out successfully (credentials removed from %s)", path) + // One document, and it carries both halves. Under -o json the outcome of the + // revocation is a field rather than a separate document printed before this + // one — two JSON documents on one stdout is something no parser accepts. + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"configFile": path, "section": section, "remoteRevocation": revocation}, + "✅ Logged out: credentials removed from %s (remote revocation: %s)", path, revocation) } // confirmLogout asks the user to confirm the logout. The default (an empty diff --git a/internal/services/login/logout_test.go b/internal/services/login/logout_test.go index 884adbc28..e1accf55e 100644 --- a/internal/services/login/logout_test.go +++ b/internal/services/login/logout_test.go @@ -8,10 +8,18 @@ import ( "errors" "fmt" "net/http" + "os" + "path/filepath" + "strings" "testing" + "github.com/jarcoal/httpmock" "github.com/maxatome/go-testdeep/td" "github.com/ovh/go-ovh/ovh" + "github.com/ovh/ovhcloud-cli/internal/display" + "github.com/ovh/ovhcloud-cli/internal/flags" + httpLib "github.com/ovh/ovhcloud-cli/internal/http" + "gopkg.in/ini.v1" ) func TestIsInvalidCredentialError(t *testing.T) { @@ -35,3 +43,86 @@ func TestIsInvalidCredentialError(t *testing.T) { }) } } + +// exitSentinel stands in for os.Exit so a test can tell "the command stopped +// here" from "the command carried on". +type exitSentinel int + +// runLogoutWithRealExit runs Logout with display.ExitFunc panicking instead of +// exiting, and reports whether it stopped early. +// +// The suite-wide stub replaces ExitFunc with a no-op, which makes every +// terminating output look like a plain print — and that stub is exactly what hid +// this defect for as long as it existed. So this test does not use it. +func runLogoutWithRealExit(t *testing.T) (stopped bool) { + t.Helper() + saved := display.ExitFunc + t.Cleanup(func() { display.ExitFunc = saved }) + display.ExitFunc = func(int) { panic(exitSentinel(0)) } + + defer func() { + if r := recover(); r != nil { + if _, ok := r.(exitSentinel); !ok { + panic(r) + } + stopped = true + } + }() + + Logout(nil, nil) + + return false +} + +// The credentials on this disk are what `logout` exists to remove. The remote +// revocation is a courtesy, and it used to be able to cancel the command: each of +// its three unhappy paths called display.OutputWarning, which does not return — +// OutputWithFormat finishes on ExitFunc(0), which is os.Exit. So a revoked key +// produced "🟠 credentials were already invalid or revoked, skipping remote +// revocation", exit 0, and the key still in the file. +// +// Reproduced against the built binary before this fix, with a bogus key in +// ./ovh.conf: exit 0 and consumer_key untouched. Verified after: removed. +func TestLogoutRemovesTheKeyEvenWhenRevocationFails(t *testing.T) { + for _, tc := range []struct { + name string + status int + }{ + {"already revoked", http.StatusForbidden}, + {"unauthorized", http.StatusUnauthorized}, + {"API failing for another reason", http.StatusInternalServerError}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "ovh.conf") + td.Require(t).CmpNoError(os.WriteFile(path, []byte( + "[default]\nendpoint=ovh-eu\n\n[ovh-eu]\napplication_key=k\napplication_secret=s\nconsumer_key=SENTINEL\n"), 0o600)) + + cfg, err := ini.Load(path) + td.Require(t).CmpNoError(err) + + httpmock.Activate(t) + client, err := ovh.NewClient("ovh-eu", "k", "s", "c") + td.Require(t).CmpNoError(err) + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/auth/time", + httpmock.NewStringResponder(200, "0")) + httpmock.RegisterResponder("POST", "https://eu.api.ovh.com/1.0/auth/logout", + httpmock.NewStringResponder(tc.status, `{"message":"nope"}`)) + + savedClient, savedCfg, savedPath, savedYes := httpLib.Client, flags.CliConfig, flags.CliConfigPath, LogoutAssumeYes + httpLib.Client, flags.CliConfig, flags.CliConfigPath, LogoutAssumeYes = client, cfg, path, true + t.Cleanup(func() { + httpLib.Client, flags.CliConfig, flags.CliConfigPath, LogoutAssumeYes = savedClient, savedCfg, savedPath, savedYes + }) + + stopped := runLogoutWithRealExit(t) + + td.Cmp(t, stopped, false, "the command must not stop before removing the key") + + after, err := os.ReadFile(path) + td.Require(t).CmpNoError(err) + td.Cmp(t, strings.Contains(string(after), "SENTINEL"), false, + "the credential is still on disk: %s", after) + }) + } +}