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) + }) + } +}