From 72b1b348798416cc4d01fbd8fdc4ff2c3dfd1bd3 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Tue, 27 Oct 2026 07:00:41 -0400 Subject: [PATCH 1/5] fix(license,mcp,reset): scrub human stderr, gate legacy MCP secrets, clean image-switch volumes (D-0979, D-0737, D-0802) --- cmd/commands/license_migrate_actions.go | 2 +- cmd/commands/mcp_tools_mutate.go | 10 ++++++++++ cmd/commands/reset.go | 16 ++++++++++++++++ cmd/commands/stop.go | 15 +++++++++++++++ cmd/commands/uninstall.go | 16 ++++++++++++++++ internal/docker/volume.go | 18 ++++++++++++++++++ 6 files changed, 76 insertions(+), 1 deletion(-) diff --git a/cmd/commands/license_migrate_actions.go b/cmd/commands/license_migrate_actions.go index c605e863f..0b67a3173 100644 --- a/cmd/commands/license_migrate_actions.go +++ b/cmd/commands/license_migrate_actions.go @@ -70,7 +70,7 @@ func runMigrateCheck(ctx context.Context, pingURL string, doc *licenseMigrateDat // Check migration status from the server. status, err := fetchMigrationStatus(ctx, key, pingURL) if err != nil { - doc.Keys = append(doc.Keys, licenseMigrateKey{Key: masked, Product: pp.DisplayName, Remaining: "?", Status: licenseScrub(fmt.Sprintf("Error: %v", err), migrateKeyFlag)}) + doc.Keys = append(doc.Keys, licenseMigrateKey{Key: masked, Product: pp.DisplayName, Remaining: "?", Status: licenseScrub(fmt.Sprintf("Error: %v", err), migrateKeyFlag)}) tbl.AddRow(masked, pp.DisplayName, "?", licenseScrub(fmt.Sprintf("Error: %v", err), key)) continue } diff --git a/cmd/commands/mcp_tools_mutate.go b/cmd/commands/mcp_tools_mutate.go index 505864ea3..b6933c7ae 100644 --- a/cmd/commands/mcp_tools_mutate.go +++ b/cmd/commands/mcp_tools_mutate.go @@ -29,6 +29,8 @@ import ( "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" + "github.com/nself-org/cli/internal/compat" + "github.com/nself-org/cli/internal/config" "github.com/nself-org/cli/internal/plugin" ) @@ -81,6 +83,14 @@ func mcpConfigSetHandler() server.ToolHandlerFunc { if err := validateConfigKey(key); err != nil { return mcpErrorResult("Error: %v", err) } + // D-0737: in v1.4 mode the legacy nself_config_set tool bypasses the + // invoke.Gate round trip that the generated v1.5 tool enforces. Refuse + // secret keys here so a token-less loopback client cannot write + // POSTGRES_PASSWORD or any other secret without confirmation. V15 mode + // replaces this tool with the generated gated version (P7-SURF-03). + if !compat.V15() && config.IsSecretKey(key) { + return mcpErrorResult("Error: setting secret key %q requires the v1.5 gated tool; upgrade or use `nself config set` interactively", key) + } if err := validateConfigValue(value); err != nil { return mcpErrorResult("Error: %v", err) } diff --git a/cmd/commands/reset.go b/cmd/commands/reset.go index 75084f35f..20fc77fc2 100644 --- a/cmd/commands/reset.go +++ b/cmd/commands/reset.go @@ -118,6 +118,22 @@ func runReset(cmd *cobra.Command, args []string) error { fmt.Println(" Containers stopped and volumes removed.") volumesRemoved = true } + // D-0802: compose down --volumes only removes volumes named in the + // current compose file. After db image switch, the old postgres + // volume (labelled org.nself.image-switch) survives because the new + // compose file no longer references it. Remove any such leftover + // volumes here so reset actually wipes all project data. + if volumesRemoved { + if switched, lvErr := docker.VolumeListByLabel(cmd.Context(), "org.nself.image-switch"); lvErr == nil { + for _, v := range switched { + if rmErr := docker.VolumeRemove(cmd.Context(), v); rmErr != nil { + fmt.Fprintf(os.Stderr, "Warning: removing image-switch volume %s failed: %v\n", v, rmErr) + } else if !buildEnvelopeWanted(cmd) { + fmt.Printf(" Removed image-switch volume %s.\n", v) + } + } + } + } } else { fmt.Println("No docker-compose.yml found — skipping container shutdown.") } diff --git a/cmd/commands/stop.go b/cmd/commands/stop.go index 2640f5726..deaf890dd 100644 --- a/cmd/commands/stop.go +++ b/cmd/commands/stop.go @@ -187,6 +187,21 @@ func runStopFlow(cmd *cobra.Command, args []string) error { if err := compose.ComposeDown(ctx, workdir, downOpts); err != nil { return fmt.Errorf("compose down failed: %w", err) } + // D-0802: when --volumes is set, also remove postgres volumes that + // db image switch kept. They carry the org.nself.image-switch label + // and are absent from the current compose file, so compose down + // --volumes does not remove them. + if volumes { + if switched, lvErr := docker.VolumeListByLabel(ctx, "org.nself.image-switch"); lvErr == nil { + for _, v := range switched { + if rmErr := docker.VolumeRemove(ctx, v); rmErr != nil { + fmt.Fprintf(os.Stderr, "Warning: removing image-switch volume %s failed: %v\n", v, rmErr) + } else if verbose { + fmt.Printf(" Removed image-switch volume %s.\n", v) + } + } + } + } // Summary. When psErr != nil we could not determine the running container // count beforehand, so report "stopped" generically. When psErr == nil but diff --git a/cmd/commands/uninstall.go b/cmd/commands/uninstall.go index e2fdbfb90..53bc3faf3 100644 --- a/cmd/commands/uninstall.go +++ b/cmd/commands/uninstall.go @@ -171,6 +171,22 @@ func uninstallProject(cmd *cobra.Command) (res UninstallResult, done bool, err e _, _ = fmt.Fprintf(cmd.ErrOrStderr(), " %s docker compose down: %v (continuing)\n", ui.C(ui.Yellow, ui.IconWarning), err) } + // D-0802: when purge is requested, also remove any postgres volumes left + // behind by db image switch. Those carry the org.nself.image-switch + // label and are not named in the current compose file, so compose down + // --volumes does not touch them. + if purge && volumesDeleted { + if switched, lvErr := docker.VolumeListByLabel(cmd.Context(), "org.nself.image-switch"); lvErr == nil { + for _, v := range switched { + if rmErr := docker.VolumeRemove(cmd.Context(), v); rmErr != nil { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), " %s removing image-switch volume %s: %v (continuing)\n", + ui.C(ui.Yellow, ui.IconWarning), v, rmErr) + } else if !jsonMode { + fmt.Printf(" Removed image-switch volume %s.\n", v) + } + } + } + } removed := []string{} var failed []error diff --git a/internal/docker/volume.go b/internal/docker/volume.go index edfaa0ab2..7dce2bb65 100644 --- a/internal/docker/volume.go +++ b/internal/docker/volume.go @@ -111,3 +111,21 @@ func VolumeLabels(ctx context.Context, name string) (labels map[string]string, e } return labels, true, nil } + +// VolumeListByLabel returns the names of volumes that carry the given label key. +// An empty result is not an error. Docker daemon unavailability is returned as +// an error so callers can distinguish "no matches" from "cannot list". +func VolumeListByLabel(ctx context.Context, labelKey string) ([]string, error) { + out, err := runCapture(ctx, "volume", "ls", "--filter", "label="+labelKey, "--format", "{{.Name}}") + if err != nil { + return nil, err + } + var names []string + for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { + line = strings.TrimSpace(line) + if line != "" { + names = append(names, line) + } + } + return names, nil +} From 7342c0e127d06a5c084c048ee66a76d27b4bc4e6 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Tue, 27 Oct 2026 16:37:42 -0400 Subject: [PATCH 2/5] fix(docker,license,mcp): scope image-switch volume cleanup to the project, keep the key out of errors, test the MCP secret guard (D-0802, D-0979, D-0737) - D-0802: compose down -v now removes the old data volume a db image switch kept plus the project's switch-labelled volumes, found from the switch state and the compose project label, so other projects' volumes are never listed. List and removal errors fail the down instead of being swallowed. One hook in ComposeDown covers reset, stop --volumes and uninstall --purge, so cmd/commands does not grow (package-size ratchet). - D-0979: the ping_api reads the migration key only from the GET query, so it stays there URL-encoded; transport errors now drop the request URL so the key never prints. license refresh is pinned to its fixed failure codes. - D-0737: add a legacy secret-key refusal test for nself_config_set. --- cmd/commands/license_migrate_api_test.go | 47 +++++ cmd/commands/license_refresh_down_test.go | 28 +++ cmd/commands/mcp_tools_mutate.go | 16 +- cmd/commands/mcp_tools_mutate_secret_test.go | 78 +++++++++ cmd/commands/reset.go | 16 -- cmd/commands/stop.go | 15 -- cmd/commands/uninstall.go | 16 -- internal/config/secretkeys.go | 14 ++ internal/docker/compose.go | 5 +- internal/docker/compose_hooks.go | 8 + internal/docker/compose_hooks_test.go | 38 ++++ internal/docker/volume.go | 19 +- internal/docker/volume_test.go | 30 ++++ internal/httptimeout/strip.go | 19 ++ internal/httptimeout/strip_test.go | 26 +++ internal/postgres/imageswitch/leftover.go | 75 ++++++++ .../postgres/imageswitch/leftover_test.go | 164 ++++++++++++++++++ 17 files changed, 548 insertions(+), 66 deletions(-) create mode 100644 cmd/commands/license_migrate_api_test.go create mode 100644 cmd/commands/license_refresh_down_test.go create mode 100644 cmd/commands/mcp_tools_mutate_secret_test.go create mode 100644 internal/httptimeout/strip.go create mode 100644 internal/httptimeout/strip_test.go create mode 100644 internal/postgres/imageswitch/leftover.go create mode 100644 internal/postgres/imageswitch/leftover_test.go diff --git a/cmd/commands/license_migrate_api_test.go b/cmd/commands/license_migrate_api_test.go new file mode 100644 index 000000000..b98cc526d --- /dev/null +++ b/cmd/commands/license_migrate_api_test.go @@ -0,0 +1,47 @@ +package commands + +// Purpose: fetchMigrationStatus sends the key URL-encoded in the query (the +// ping_api reads it nowhere else) and never lets it escape through a +// transport error, which net/http otherwise builds from the full request URL. +// Inputs: a synthetic key and httptest servers. Outputs: pass/fail. +// Constraints: loopback only. + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +const migrateTestKey = "synthetic key&x=1#frag" + +func TestFetchMigrationStatusEncodesKey(t *testing.T) { + var gotKey, gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotKey, gotPath = r.URL.Query().Get("key"), r.URL.Path + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + if _, err := fetchMigrationStatus(context.Background(), migrateTestKey, srv.URL); err != nil { + t.Fatal(err) + } + if gotPath != "/license/migration-status" || gotKey != migrateTestKey { + t.Errorf("server saw path %q key %q", gotPath, gotKey) + } +} + +func TestFetchMigrationStatusTransportErrorOmitsKey(t *testing.T) { + srv := httptest.NewServer(http.NotFoundHandler()) + url := srv.URL + srv.Close() // connection refused + _, err := fetchMigrationStatus(context.Background(), migrateTestKey, url) + if err == nil { + t.Fatal("want a transport error") + } + for _, leak := range []string{migrateTestKey, "synthetic", "key=", "migration-status"} { + if strings.Contains(err.Error(), leak) { + t.Errorf("transport error leaks %q: %v", leak, err) + } + } +} diff --git a/cmd/commands/license_refresh_down_test.go b/cmd/commands/license_refresh_down_test.go new file mode 100644 index 000000000..677023c0a --- /dev/null +++ b/cmd/commands/license_refresh_down_test.go @@ -0,0 +1,28 @@ +package commands + +// Purpose: `license refresh` against an unreachable ping_api prints only the +// fixed failure code, never the key, its mask, its hash or the request URL. +// Inputs: the grace fixture with its server shut down. Outputs: pass/fail. +// Constraints: loopback only. + +import ( + "strings" + "testing" + + "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/license" +) + +func TestLicenseRefreshTransportFailureIsFixedCode(t *testing.T) { + f := newGraceFixture(t) + t.Setenv("NSELF_PING_API_URL", "http://127.0.0.1:1") // nothing listens + stdout, stderr, err := runRefresh(t, f.project, map[string]string{"project": f.project, "quiet": "true"}) + if stdout != "" || stderr != "entitlement refresh failed: ping-unavailable\n" || errs.ExitCodeFor(err) != 1 { + t.Fatalf("stdout %q stderr %q exit %d", stdout, stderr, errs.ExitCodeFor(err)) + } + for _, s := range []string{graceKey, license.MaskKey(graceKey), license.HashKey(graceKey), "127.0.0.1"} { + if strings.Contains(stdout+stderr, s) { + t.Errorf("output contains %q", s) + } + } +} diff --git a/cmd/commands/mcp_tools_mutate.go b/cmd/commands/mcp_tools_mutate.go index b6933c7ae..d7b8d8c66 100644 --- a/cmd/commands/mcp_tools_mutate.go +++ b/cmd/commands/mcp_tools_mutate.go @@ -24,12 +24,12 @@ package commands import ( "context" + "errors" "fmt" "strings" "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" - "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/config" "github.com/nself-org/cli/internal/plugin" ) @@ -80,17 +80,9 @@ func mcpConfigSetHandler() server.ToolHandlerFunc { value, _ := args["value"].(string) envFlag, _ := args["env"].(string) - if err := validateConfigKey(key); err != nil { + if err := errors.Join(validateConfigKey(key), config.RefuseLegacyMCPSecretSet(key)); err != nil { // D-0737 return mcpErrorResult("Error: %v", err) } - // D-0737: in v1.4 mode the legacy nself_config_set tool bypasses the - // invoke.Gate round trip that the generated v1.5 tool enforces. Refuse - // secret keys here so a token-less loopback client cannot write - // POSTGRES_PASSWORD or any other secret without confirmation. V15 mode - // replaces this tool with the generated gated version (P7-SURF-03). - if !compat.V15() && config.IsSecretKey(key) { - return mcpErrorResult("Error: setting secret key %q requires the v1.5 gated tool; upgrade or use `nself config set` interactively", key) - } if err := validateConfigValue(value); err != nil { return mcpErrorResult("Error: %v", err) } @@ -107,9 +99,7 @@ func mcpConfigSetHandler() server.ToolHandlerFunc { if err != nil { return mcpErrorResult("Error: %v", err) } - return mcp.NewToolResultStructuredOnly(ConfigSetResult{ - Key: key, File: envFile, Created: !updated, - }), nil + return mcp.NewToolResultStructuredOnly(ConfigSetResult{Key: key, File: envFile, Created: !updated}), nil } } diff --git a/cmd/commands/mcp_tools_mutate_secret_test.go b/cmd/commands/mcp_tools_mutate_secret_test.go new file mode 100644 index 000000000..ce8d262e9 --- /dev/null +++ b/cmd/commands/mcp_tools_mutate_secret_test.go @@ -0,0 +1,78 @@ +package commands + +// Purpose: proves the v1.4 legacy nself_config_set tool refuses secret keys +// (D-0737) and still writes ordinary keys; in v1.5 the generated gated tool +// replaces it, so the legacy guard does not apply there. +// Inputs: synthetic keys and a temp project. Outputs: pass/fail. +// Constraints: no network, temp dirs only. + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + + "github.com/nself-org/cli/internal/compat/compattest" +) + +func TestMCPConfigSetRefusesSecretKeyInLegacyMode(t *testing.T) { + compattest.Set(t, false) + dir := t.TempDir() + envPath := filepath.Join(dir, ".env") + if err := os.WriteFile(envPath, []byte("PROJECT_NAME=demo\n"), 0o600); err != nil { + t.Fatal(err) + } + defer chdir(t, dir)() + + for _, key := range []string{"POSTGRES_PASSWORD", "HASURA_GRAPHQL_ADMIN_SECRET", "S3_SECRET_KEY", "BASIC_AUTH"} { + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"key": key, "value": "synthetic-value"} + res, err := mcpConfigSetHandler()(context.Background(), req) + if err != nil { + t.Fatal(err) + } + text := res.Content[0].(mcp.TextContent).Text + if !strings.Contains(text, "requires the v1.5 gated tool") { + t.Errorf("%s: want a secret-key refusal, got %q", key, text) + } + } + got, _ := os.ReadFile(envPath) + if string(got) != "PROJECT_NAME=demo\n" { + t.Errorf(".env changed by a refused call: %q", got) + } + + // A non-secret key still passes through. + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"key": "ABC", "value": "v"} + if res, err := mcpConfigSetHandler()(context.Background(), req); err != nil || !isConfigSetOK(res) { + t.Fatalf("non-secret key refused: %v %#v", err, res) + } +} + +func TestMCPConfigSetSecretGuardIsLegacyOnly(t *testing.T) { + compattest.Set(t, true) + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ".env"), []byte("PROJECT_NAME=demo\n"), 0o600); err != nil { + t.Fatal(err) + } + defer chdir(t, dir)() + + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"key": "POSTGRES_PASSWORD", "value": "synthetic-value"} + res, err := mcpConfigSetHandler()(context.Background(), req) + if err != nil { + t.Fatal(err) + } + if !isConfigSetOK(res) { + t.Errorf("v1.5 hit the legacy guard: %#v", res) + } +} + +// isConfigSetOK reports whether the handler took the success path. +func isConfigSetOK(res *mcp.CallToolResult) bool { + _, ok := res.StructuredContent.(ConfigSetResult) + return ok +} diff --git a/cmd/commands/reset.go b/cmd/commands/reset.go index 20fc77fc2..75084f35f 100644 --- a/cmd/commands/reset.go +++ b/cmd/commands/reset.go @@ -118,22 +118,6 @@ func runReset(cmd *cobra.Command, args []string) error { fmt.Println(" Containers stopped and volumes removed.") volumesRemoved = true } - // D-0802: compose down --volumes only removes volumes named in the - // current compose file. After db image switch, the old postgres - // volume (labelled org.nself.image-switch) survives because the new - // compose file no longer references it. Remove any such leftover - // volumes here so reset actually wipes all project data. - if volumesRemoved { - if switched, lvErr := docker.VolumeListByLabel(cmd.Context(), "org.nself.image-switch"); lvErr == nil { - for _, v := range switched { - if rmErr := docker.VolumeRemove(cmd.Context(), v); rmErr != nil { - fmt.Fprintf(os.Stderr, "Warning: removing image-switch volume %s failed: %v\n", v, rmErr) - } else if !buildEnvelopeWanted(cmd) { - fmt.Printf(" Removed image-switch volume %s.\n", v) - } - } - } - } } else { fmt.Println("No docker-compose.yml found — skipping container shutdown.") } diff --git a/cmd/commands/stop.go b/cmd/commands/stop.go index deaf890dd..2640f5726 100644 --- a/cmd/commands/stop.go +++ b/cmd/commands/stop.go @@ -187,21 +187,6 @@ func runStopFlow(cmd *cobra.Command, args []string) error { if err := compose.ComposeDown(ctx, workdir, downOpts); err != nil { return fmt.Errorf("compose down failed: %w", err) } - // D-0802: when --volumes is set, also remove postgres volumes that - // db image switch kept. They carry the org.nself.image-switch label - // and are absent from the current compose file, so compose down - // --volumes does not remove them. - if volumes { - if switched, lvErr := docker.VolumeListByLabel(ctx, "org.nself.image-switch"); lvErr == nil { - for _, v := range switched { - if rmErr := docker.VolumeRemove(ctx, v); rmErr != nil { - fmt.Fprintf(os.Stderr, "Warning: removing image-switch volume %s failed: %v\n", v, rmErr) - } else if verbose { - fmt.Printf(" Removed image-switch volume %s.\n", v) - } - } - } - } // Summary. When psErr != nil we could not determine the running container // count beforehand, so report "stopped" generically. When psErr == nil but diff --git a/cmd/commands/uninstall.go b/cmd/commands/uninstall.go index 53bc3faf3..e2fdbfb90 100644 --- a/cmd/commands/uninstall.go +++ b/cmd/commands/uninstall.go @@ -171,22 +171,6 @@ func uninstallProject(cmd *cobra.Command) (res UninstallResult, done bool, err e _, _ = fmt.Fprintf(cmd.ErrOrStderr(), " %s docker compose down: %v (continuing)\n", ui.C(ui.Yellow, ui.IconWarning), err) } - // D-0802: when purge is requested, also remove any postgres volumes left - // behind by db image switch. Those carry the org.nself.image-switch - // label and are not named in the current compose file, so compose down - // --volumes does not touch them. - if purge && volumesDeleted { - if switched, lvErr := docker.VolumeListByLabel(cmd.Context(), "org.nself.image-switch"); lvErr == nil { - for _, v := range switched { - if rmErr := docker.VolumeRemove(cmd.Context(), v); rmErr != nil { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), " %s removing image-switch volume %s: %v (continuing)\n", - ui.C(ui.Yellow, ui.IconWarning), v, rmErr) - } else if !jsonMode { - fmt.Printf(" Removed image-switch volume %s.\n", v) - } - } - } - } removed := []string{} var failed []error diff --git a/internal/config/secretkeys.go b/internal/config/secretkeys.go index e1e00f26f..296576526 100644 --- a/internal/config/secretkeys.go +++ b/internal/config/secretkeys.go @@ -11,8 +11,10 @@ package config // side: a match only redacts. internal/env must not keep a second list. import ( + "fmt" "strings" + "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/credmask" ) @@ -33,6 +35,18 @@ func IsSecretKey(key string) bool { return upper == "AUTH" || strings.HasSuffix(upper, "_AUTH") } +// RefuseLegacyMCPSecretSet is the D-0737 guard of the v1.4 nself_config_set +// MCP tool, which skips the invoke.Gate round trip of the generated v1.5 tool: +// in v1.4 mode it refuses secret keys so a token-less loopback client cannot +// write POSTGRES_PASSWORD or any other secret unconfirmed. Nil in v1.5 mode, +// where the generated gated tool replaces the legacy one (P7-SURF-03). +func RefuseLegacyMCPSecretSet(key string) error { + if compat.V15() || !IsSecretKey(key) { + return nil + } + return fmt.Errorf("setting secret key %q requires the v1.5 gated tool; upgrade or use `nself config set` interactively", key) +} + // MaskURLCredentials masks the credentials of a value that holds a URL, a Go // DSN, a bare user:pass@host or password=/pwd= pairs. The rules and their // bounds are documented on credmask.URL. diff --git a/internal/docker/compose.go b/internal/docker/compose.go index e05c65afb..59548d3fa 100644 --- a/internal/docker/compose.go +++ b/internal/docker/compose.go @@ -93,7 +93,10 @@ func (c *Compose) ComposeDown(ctx context.Context, workdir string, opts DownOpti if opts.Timeout > 0 { args = append(args, "-t", fmt.Sprintf("%d", opts.Timeout)) } - return c.Run(ctx, workdir, args...) + if err := c.Run(ctx, workdir, args...); err != nil || !opts.RemoveVolumes || VolumesRemovedHook == nil { + return err + } + return VolumesRemovedHook(ctx, workdir) } // ComposeUpNoDeps runs `docker compose up -d --no-deps [services...]`. diff --git a/internal/docker/compose_hooks.go b/internal/docker/compose_hooks.go index 921f74ebf..e18139d2d 100644 --- a/internal/docker/compose_hooks.go +++ b/internal/docker/compose_hooks.go @@ -76,3 +76,11 @@ func RunPreDownHooks(ctx context.Context, workdir string) (release func()) { } } } + +// VolumesRemovedHook, when set at init, runs after a successful +// `docker compose down -v` in workdir and its error fails ComposeDown. The +// postgres image switch sets it (internal/postgres/imageswitch cannot be +// imported from here) to remove the old data volume a switch kept, which the +// current compose file no longer names, so reset, stop --volumes and +// uninstall --purge really wipe the project's data (D-0802). +var VolumesRemovedHook func(ctx context.Context, workdir string) error diff --git a/internal/docker/compose_hooks_test.go b/internal/docker/compose_hooks_test.go index f8cf17111..83161ddd2 100644 --- a/internal/docker/compose_hooks_test.go +++ b/internal/docker/compose_hooks_test.go @@ -2,6 +2,7 @@ package docker import ( "context" + "errors" "os" "path/filepath" "runtime" @@ -81,3 +82,40 @@ func TestComposeDownRemoveImages(t *testing.T) { t.Errorf("args = %q, want %q", got, want) } } + +func TestVolumesRemovedHook(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake docker is a POSIX shell script") + } + dir := t.TempDir() + fail := filepath.Join(dir, "fail") + script := "#!/bin/sh\n[ -e '" + fail + "' ] && exit 1\nexit 0\n" + bin := filepath.Join(dir, "docker") + if err := os.WriteFile(bin, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + c := &Compose{DockerPath: bin} + var calls []string + hookErr := error(nil) + old := VolumesRemovedHook + VolumesRemovedHook = func(_ context.Context, workdir string) error { calls = append(calls, workdir); return hookErr } + t.Cleanup(func() { VolumesRemovedHook = old }) + + if err := c.ComposeDown(context.Background(), dir, DownOptions{}); err != nil || len(calls) != 0 { + t.Fatalf("down without -v: err %v calls %v", err, calls) + } + if err := c.ComposeDown(context.Background(), dir, DownOptions{RemoveVolumes: true}); err != nil || !slices.Equal(calls, []string{dir}) { + t.Fatalf("down -v: err %v calls %v", err, calls) + } + hookErr = errors.New("volume is in use") + if err := c.ComposeDown(context.Background(), dir, DownOptions{RemoveVolumes: true}); err == nil || !strings.Contains(err.Error(), "in use") { + t.Fatalf("a hook failure must fail the down, got %v", err) + } + calls = nil + if err := os.WriteFile(fail, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := c.ComposeDown(context.Background(), dir, DownOptions{RemoveVolumes: true}); err == nil || len(calls) != 0 { + t.Fatalf("hook ran after a failed down: err %v calls %v", err, calls) + } +} diff --git a/internal/docker/volume.go b/internal/docker/volume.go index 7dce2bb65..defbc630d 100644 --- a/internal/docker/volume.go +++ b/internal/docker/volume.go @@ -112,11 +112,20 @@ func VolumeLabels(ctx context.Context, name string) (labels map[string]string, e return labels, true, nil } -// VolumeListByLabel returns the names of volumes that carry the given label key. -// An empty result is not an error. Docker daemon unavailability is returned as -// an error so callers can distinguish "no matches" from "cannot list". -func VolumeListByLabel(ctx context.Context, labelKey string) ([]string, error) { - out, err := runCapture(ctx, "volume", "ls", "--filter", "label="+labelKey, "--format", "{{.Name}}") +// VolumeListByLabels returns the names of the volumes that carry every given +// label filter ("key" or "key=value"; docker ANDs repeated filters). An empty +// result is not an error. Docker daemon unavailability is returned as an error +// so callers can distinguish "no matches" from "cannot list". With no filters +// it returns nothing, never every volume on the host. +func VolumeListByLabels(ctx context.Context, labels ...string) ([]string, error) { + if len(labels) == 0 { + return nil, nil + } + args := []string{"volume", "ls"} + for _, l := range labels { + args = append(args, "--filter", "label="+l) + } + out, err := runCapture(ctx, append(args, "--format", "{{.Name}}")...) if err != nil { return nil, err } diff --git a/internal/docker/volume_test.go b/internal/docker/volume_test.go index 0d999e194..9603a7463 100644 --- a/internal/docker/volume_test.go +++ b/internal/docker/volume_test.go @@ -141,3 +141,33 @@ func TestVolumeLabels(t *testing.T) { t.Errorf("a volume with no labels: %v %v %v", got, ok, err) } } + +func TestVolumeListByLabelsAndsFilters(t *testing.T) { + log := fakeVolumeDocker(t, "a\n\n b \n", "") + got, err := VolumeListByLabels(context.Background(), "org.nself.image-switch", "com.docker.compose.project=demo") + if err != nil || !reflect.DeepEqual(got, []string{"a", "b"}) { + t.Fatalf("got %v, %v", got, err) + } + want := "volume ls --filter label=org.nself.image-switch --filter label=com.docker.compose.project=demo --format {{.Name}}" + if c := lastCall(t, log); c != want { + t.Errorf("call = %q, want %q", c, want) + } +} + +func TestVolumeListByLabelsNoFiltersListsNothing(t *testing.T) { + log := fakeVolumeDocker(t, "every\nvolume\n", "") + got, err := VolumeListByLabels(context.Background()) + if err != nil || got != nil { + t.Fatalf("got %v, %v", got, err) + } + if _, statErr := os.Stat(log); statErr == nil { + t.Error("docker was called with no filters") + } +} + +func TestVolumeListByLabelsReturnsDaemonError(t *testing.T) { + fakeVolumeDocker(t, "", "Cannot connect to the Docker daemon") + if _, err := VolumeListByLabels(context.Background(), "x"); err == nil { + t.Fatal("a daemon failure must be an error, not an empty list") + } +} diff --git a/internal/httptimeout/strip.go b/internal/httptimeout/strip.go new file mode 100644 index 000000000..c325c1395 --- /dev/null +++ b/internal/httptimeout/strip.go @@ -0,0 +1,19 @@ +package httptimeout + +import ( + "errors" + "net/url" +) + +// StripURL returns the cause of a *url.Error and err itself otherwise. +// +// Purpose: net/http quotes the whole request URL in a transport error, so a +// credential sent in a query string would reach logs and terminals. +// Inputs: an error from http.Client.Do. Outputs: the same error without the URL. +func StripURL(err error) error { + var ue *url.Error + if errors.As(err, &ue) { + return ue.Err + } + return err +} diff --git a/internal/httptimeout/strip_test.go b/internal/httptimeout/strip_test.go new file mode 100644 index 000000000..b5b5468f7 --- /dev/null +++ b/internal/httptimeout/strip_test.go @@ -0,0 +1,26 @@ +package httptimeout + +import ( + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestStripURLDropsTheRequestURL(t *testing.T) { + srv := httptest.NewServer(http.NotFoundHandler()) + target := srv.URL + "/p?key=synthetic-secret" + srv.Close() // connection refused + _, err := http.Get(target) + if err == nil || !strings.Contains(err.Error(), "synthetic-secret") { + t.Fatalf("precondition: net/http error should quote the URL, got %v", err) + } + if got := StripURL(err); strings.Contains(got.Error(), "synthetic-secret") || strings.Contains(got.Error(), "/p") { + t.Errorf("StripURL kept the URL: %v", got) + } + plain := errors.New("plain") + if StripURL(plain) != plain || StripURL(nil) != nil { + t.Error("non-URL errors and nil must pass through") + } +} diff --git a/internal/postgres/imageswitch/leftover.go b/internal/postgres/imageswitch/leftover.go new file mode 100644 index 000000000..d97e9a00f --- /dev/null +++ b/internal/postgres/imageswitch/leftover.go @@ -0,0 +1,75 @@ +package imageswitch + +// leftover.go — the data volumes a finished switch leaves behind. +// +// Purpose: after a switch the compose file names only the new volume, so +// `compose down --volumes` (reset, stop --volumes, uninstall --purge) cannot +// remove the kept old volume or the volumes of earlier switches. This finds +// them, scoped to this project, so those commands can wipe all project data. +// Inputs: the project directory (its switch state names the compose project). +// Outputs: volume names; none when the project never switched. +// Constraints: never lists volumes of another compose project; a volume whose +// compose project label names a different project is skipped; list errors are +// returned, never swallowed. + +import ( + "context" + "errors" + "fmt" + "slices" + + "github.com/nself-org/cli/internal/docker" +) + +func init() { + docker.VolumesRemovedHook = func(ctx context.Context, workdir string) error { + _, err := RemoveLeftovers(ctx, workdir) + return err + } +} + +const labelComposeProject = "com.docker.compose.project" + +// LeftoverVolumes returns the project's data volumes that a switch kept: the +// old volume recorded in the state, and every volume of the same compose +// project carrying the switch label (earlier switches' new volumes). +func LeftoverVolumes(ctx context.Context, projectDir string) ([]string, error) { + st, err := LoadState(projectDir) + if err != nil || st == nil || st.ComposeProject == "" { + return nil, err + } + names, err := docker.VolumeListByLabels(ctx, labelRun, labelComposeProject+"="+st.ComposeProject) + if err != nil { + return nil, err + } + if old := st.OldVolume; old != "" && !slices.Contains(names, old) { + labels, exists, err := docker.VolumeLabels(ctx, old) + if err != nil { + return nil, err + } + if p := labels[labelComposeProject]; exists && (p == "" || p == st.ComposeProject) { + names = append(names, old) + } + } + return names, nil +} + +// RemoveLeftovers removes LeftoverVolumes and returns the names removed. A list +// or removal failure is returned (all removals are still attempted), so the +// caller never reports the data as gone when it is not. +func RemoveLeftovers(ctx context.Context, projectDir string) ([]string, error) { + names, err := LeftoverVolumes(ctx, projectDir) + if err != nil { + return nil, fmt.Errorf("listing image-switch volumes: %w", err) + } + var removed []string + var errs []error + for _, v := range names { + if err := docker.VolumeRemove(ctx, v); err != nil { + errs = append(errs, fmt.Errorf("removing image-switch volume %s: %w", v, err)) + continue + } + removed = append(removed, v) + } + return removed, errors.Join(errs...) +} diff --git a/internal/postgres/imageswitch/leftover_test.go b/internal/postgres/imageswitch/leftover_test.go new file mode 100644 index 000000000..a42f34945 --- /dev/null +++ b/internal/postgres/imageswitch/leftover_test.go @@ -0,0 +1,164 @@ +package imageswitch + +// Purpose: D-0802. RemoveLeftovers removes the old volume a switch kept +// and the project's switch-labelled volumes, leaves every other project's +// volumes alone, and returns list/remove failures instead of swallowing them. +// Inputs: a fake docker that filters a directory of volume label files the +// way `docker volume ls --filter label=...` does, and a state.json. +// Outputs: pass/fail. Constraints: POSIX shell, temp dirs only. + +import ( + "context" + "os" + "path/filepath" + "runtime" + "sort" + "strings" + "testing" + + "github.com/nself-org/cli/internal/docker" +) + +const fakeVolumeDockerScript = `#!/bin/sh +D="$FAKE_VOLS" +echo "$*" >> "$D/calls.log" +[ -n "$FAKE_LS_FAIL" ] && [ "$1 $2" = "volume ls" ] && { echo "Cannot connect to the Docker daemon" >&2; exit 1; } +[ -n "$FAKE_RM_FAIL" ] && [ "$1 $2" = "volume rm" ] && { echo "volume is in use" >&2; exit 1; } +case "$1 $2" in +"volume ls") + shift 2; filters="" + while [ $# -gt 0 ]; do case "$1" in --filter) filters="$filters ${2#label=}"; shift 2;; *) shift;; esac; done + for f in "$D"/*.labels; do + [ -e "$f" ] || continue + name=$(basename "$f" .labels); labels=$(cat "$f"); ok=1 + for want in $filters; do + case " $labels " in *" $want "*) ;; *) case "$want" in *=*) ok=0;; *) case " $labels " in *" $want="*) ;; *) ok=0;; esac;; esac;; esac + done + [ $ok = 1 ] && echo "$name" + done ;; +"volume inspect") + for a; do name=$a; done + [ -e "$D/$name.labels" ] || { echo "Error response from daemon: get $name: no such volume" >&2; exit 1; } + printf '{'; sep=; for t in $(cat "$D/$name.labels"); do printf '%s"%s":"%s"' "$sep" "${t%%=*}" "${t#*=}"; sep=,; done; printf '}' ;; +"volume rm") rm -f "$D/$3.labels" ;; +esac +exit 0 +` + +func setupLeftoverFixture(t *testing.T) (project, vols string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("fake docker is a POSIX shell script") + } + bin, vols, project := t.TempDir(), t.TempDir(), t.TempDir() + if err := os.WriteFile(filepath.Join(bin, "docker"), []byte(fakeVolumeDockerScript), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("FAKE_VOLS", vols) + for name, labels := range map[string]string{ + // the kept old volume: compose labels only, no switch label + "alpha_postgres_data": "com.docker.compose.project=alpha com.docker.compose.volume=postgres_data", + // the new volume and an earlier switch's volume + "alpha_postgres_data_pgvector_2": "com.docker.compose.project=alpha org.nself.image-switch=sw-2", + "alpha_postgres_data_pgvector_1": "com.docker.compose.project=alpha org.nself.image-switch=sw-1", + // another project's switched volume and unrelated volumes + "beta_postgres_data_pgvector_1": "com.docker.compose.project=beta org.nself.image-switch=sw-9", + "beta_postgres_data": "com.docker.compose.project=beta com.docker.compose.volume=postgres_data", + "other_data": "", + } { + if err := os.WriteFile(filepath.Join(vols, name+".labels"), []byte(labels), 0o600); err != nil { + t.Fatal(err) + } + } + state := `{"version":1,"run_id":"sw-2","phase":"done","compose_project":"alpha",` + + `"old_volume":"alpha_postgres_data","new_volume":"alpha_postgres_data_pgvector_2"}` + sd := filepath.Join(project, ".nself", "image-switch") + if err := os.MkdirAll(sd, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(sd, "state.json"), []byte(state), 0o600); err != nil { + t.Fatal(err) + } + return project, vols +} + +func remainingVolumes(t *testing.T, vols string) []string { + t.Helper() + files, _ := filepath.Glob(filepath.Join(vols, "*.labels")) + var out []string + for _, f := range files { + out = append(out, strings.TrimSuffix(filepath.Base(f), ".labels")) + } + sort.Strings(out) + return out +} + +func TestRemoveSwitchLeftoversIsScopedToTheProject(t *testing.T) { + project, vols := setupLeftoverFixture(t) + removed, err := RemoveLeftovers(context.Background(), project) + if err != nil { + t.Fatal(err) + } + sort.Strings(removed) + want := []string{"alpha_postgres_data", "alpha_postgres_data_pgvector_1", "alpha_postgres_data_pgvector_2"} + if strings.Join(removed, ",") != strings.Join(want, ",") { + t.Errorf("removed %v, want %v", removed, want) + } + left := remainingVolumes(t, vols) + if strings.Join(left, ",") != "beta_postgres_data,beta_postgres_data_pgvector_1,other_data" { + t.Errorf("other projects' volumes were touched: %v", left) + } +} + +func TestRemoveSwitchLeftoversSkipsOldVolumeOfAnotherProject(t *testing.T) { + project, vols := setupLeftoverFixture(t) + // A copied project directory: the state names a volume that now belongs to beta. + if err := os.WriteFile(filepath.Join(vols, "alpha_postgres_data.labels"), []byte("com.docker.compose.project=beta"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := RemoveLeftovers(context.Background(), project); err != nil { + t.Fatal(err) + } + if !strings.Contains(strings.Join(remainingVolumes(t, vols), ","), "alpha_postgres_data,") { + t.Errorf("a volume labelled for another project was removed: %v", remainingVolumes(t, vols)) + } +} + +func TestRemoveSwitchLeftoversNoStateTouchesNothing(t *testing.T) { + _, vols := setupLeftoverFixture(t) + removed, err := RemoveLeftovers(context.Background(), t.TempDir()) + if err != nil || len(removed) != 0 { + t.Fatalf("removed %v, err %v", removed, err) + } + if _, statErr := os.Stat(filepath.Join(vols, "calls.log")); statErr == nil { + t.Error("docker was called for a project that never switched") + } +} + +func TestRemoveSwitchLeftoversReturnsListAndRemoveErrors(t *testing.T) { + project, _ := setupLeftoverFixture(t) + t.Setenv("FAKE_LS_FAIL", "1") + if _, err := RemoveLeftovers(context.Background(), project); err == nil || !strings.Contains(err.Error(), "listing image-switch volumes") { + t.Fatalf("list failure not returned: %v", err) + } + t.Setenv("FAKE_LS_FAIL", "") + t.Setenv("FAKE_RM_FAIL", "1") + removed, err := RemoveLeftovers(context.Background(), project) + if err == nil || len(removed) != 0 || !strings.Contains(err.Error(), "alpha_postgres_data") { + t.Fatalf("remove failure not returned: removed %v err %v", removed, err) + } +} + +// ComposeDown with volumes is what reset, stop --volumes and uninstall --purge +// run; the hook this package registers must make it remove the kept volume. +func TestComposeDownVolumesRemovesSwitchLeftovers(t *testing.T) { + project, vols := setupLeftoverFixture(t) + c := &docker.Compose{DockerPath: "docker"} + if err := c.ComposeDown(context.Background(), project, docker.DownOptions{RemoveVolumes: true, RemoveOrphans: true}); err != nil { + t.Fatal(err) + } + if left := strings.Join(remainingVolumes(t, vols), ","); left != "beta_postgres_data,beta_postgres_data_pgvector_1,other_data" { + t.Errorf("remaining volumes = %s", left) + } +} From 69289def5e1098352d1a6e715f6b8ef75e7ebfb1 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Wed, 28 Oct 2026 16:56:56 -0400 Subject: [PATCH 3/5] fix(config): add the compat.V15 marker for the legacy MCP secret-set guard (D-0737) --- .github/wiki/Compat-V15.md | 1 + internal/config/secretkeys.go | 1 + 2 files changed, 2 insertions(+) diff --git a/.github/wiki/Compat-V15.md b/.github/wiki/Compat-V15.md index a5c96fdcd..840f90936 100644 --- a/.github/wiki/Compat-V15.md +++ b/.github/wiki/Compat-V15.md @@ -73,6 +73,7 @@ The table is generated. Never hand-edit it; run `compat-markers.sh --write-wiki` | D-0676 | bare stream path streams as path:// with a notice | refused | `internal/backup/stream_dest_compat.go` | | D-0685 | credential guard before E138 for a missing minio plugin | guard only for plugin-backed MinIO | `internal/config/validator_ports_services.go` | | D-0686 | S3_SECRET_KEY accepted at any strength | must be at least 16 characters when object storage is enabled | `internal/config/validator.go` | +| D-0737 | legacy MCP nself_config_set refuses secret keys | the generated gated v1.5 tool handles them | `internal/config/secretkeys.go` | | D-0752 | plugin disable never confirmed | prod-class and hand-edit confirmation before the marker is written. | `cmd/commands/plugin_disable_gate.go` | | D-0757 | nginx/sites may be a symlink out of the project and the build writes through it | v1.5 refuses a project's own nginx/sites that resolves outside the project | `internal/build/nginx_sites_dir.go` | | D-0843 | a downgrade only warns | refused unless --allow-downgrade is given | `internal/plugin/installer_downgrade.go` | diff --git a/internal/config/secretkeys.go b/internal/config/secretkeys.go index 296576526..370905c24 100644 --- a/internal/config/secretkeys.go +++ b/internal/config/secretkeys.go @@ -41,6 +41,7 @@ func IsSecretKey(key string) bool { // write POSTGRES_PASSWORD or any other secret unconfirmed. Nil in v1.5 mode, // where the generated gated tool replaces the legacy one (P7-SURF-03). func RefuseLegacyMCPSecretSet(key string) error { + // compat.V15(D-0737): legacy MCP nself_config_set refuses secret keys -> the generated gated v1.5 tool handles them if compat.V15() || !IsSecretKey(key) { return nil } From 4898330e48c19110c01dd158fb1174d3fe66530f Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Wed, 28 Oct 2026 19:16:21 -0400 Subject: [PATCH 4/5] test(migrate): ignore the generator's second-resolution timestamp in the v1.4 artifact comparison --- cmd/commands/migrate_supabase_v15_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cmd/commands/migrate_supabase_v15_test.go b/cmd/commands/migrate_supabase_v15_test.go index b89fe52e8..1913fefa2 100644 --- a/cmd/commands/migrate_supabase_v15_test.go +++ b/cmd/commands/migrate_supabase_v15_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "regexp" "strings" "testing" @@ -15,6 +16,10 @@ import ( "github.com/spf13/pflag" ) +// generatedStamp matches the one-second-resolution timestamp the generator embeds in +// supabase-migration.sql; two Generate calls straddling a second boundary differ only there. +var generatedStamp = regexp.MustCompile(`(?m)^-- Generated: \d+$`) + func resetSupabaseFlags(t *testing.T) { t.Helper() migrateSupabaseCmd.Flags().VisitAll(func(f *pflag.Flag) { @@ -91,7 +96,7 @@ func TestMigrateSupabaseModes(t *testing.T) { want := spmigrate.Generate("abcdefghijklmnopqrst", &spmigrate.PullResult{Tables: []spmigrate.Table{{Schema: "public", Name: "notes"}}}) for name, content := range map[string]string{"supabase-migration.sql": want.MigrationSQL, "hasura-metadata.yaml": want.HasuraMetadata, "import-auth-users.sh": want.AuthImportScript} { got, rerr := os.ReadFile(filepath.Join(out, name)) - if rerr != nil || string(got) != content { + if rerr != nil || generatedStamp.ReplaceAllString(string(got), "-- Generated: ") != generatedStamp.ReplaceAllString(content, "-- Generated: ") { t.Fatalf("v1.4 artifact %s differs from the generator output (err=%v)", name, rerr) } } From 8c7719b76cc8d3bd015d8eea5ce9c27b8d397426 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Thu, 29 Oct 2026 17:50:22 -0400 Subject: [PATCH 5/5] fix(license): strip the request URL from migration-status transport errors (D-0979) Rebased onto main, which moved the key into the X-License-Key header. The migration-status tests now assert the header and an empty query, and the transport error goes through httptimeout.StripURL so neither the key nor the endpoint reaches stderr. Drops a mis-indented duplicate of main's scrub line. --- cmd/commands/license_migrate_actions.go | 2 +- cmd/commands/license_migrate_api.go | 2 +- cmd/commands/license_migrate_api_test.go | 12 ++++++------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cmd/commands/license_migrate_actions.go b/cmd/commands/license_migrate_actions.go index 0b67a3173..c605e863f 100644 --- a/cmd/commands/license_migrate_actions.go +++ b/cmd/commands/license_migrate_actions.go @@ -70,7 +70,7 @@ func runMigrateCheck(ctx context.Context, pingURL string, doc *licenseMigrateDat // Check migration status from the server. status, err := fetchMigrationStatus(ctx, key, pingURL) if err != nil { - doc.Keys = append(doc.Keys, licenseMigrateKey{Key: masked, Product: pp.DisplayName, Remaining: "?", Status: licenseScrub(fmt.Sprintf("Error: %v", err), migrateKeyFlag)}) + doc.Keys = append(doc.Keys, licenseMigrateKey{Key: masked, Product: pp.DisplayName, Remaining: "?", Status: licenseScrub(fmt.Sprintf("Error: %v", err), migrateKeyFlag)}) tbl.AddRow(masked, pp.DisplayName, "?", licenseScrub(fmt.Sprintf("Error: %v", err), key)) continue } diff --git a/cmd/commands/license_migrate_api.go b/cmd/commands/license_migrate_api.go index f999f381b..4265d732b 100644 --- a/cmd/commands/license_migrate_api.go +++ b/cmd/commands/license_migrate_api.go @@ -80,7 +80,7 @@ func fetchMigrationStatus(ctx context.Context, key string, pingURL string) (*mig resp, err := httptimeout.License.Do(req) if err != nil { - return nil, err + return nil, httptimeout.StripURL(err) } defer func() { _ = resp.Body.Close() }() diff --git a/cmd/commands/license_migrate_api_test.go b/cmd/commands/license_migrate_api_test.go index b98cc526d..42b35a765 100644 --- a/cmd/commands/license_migrate_api_test.go +++ b/cmd/commands/license_migrate_api_test.go @@ -1,7 +1,7 @@ package commands -// Purpose: fetchMigrationStatus sends the key URL-encoded in the query (the -// ping_api reads it nowhere else) and never lets it escape through a +// Purpose: fetchMigrationStatus keeps the key out of the request URL (it goes +// in X-License-Key) and never lets the key or the endpoint escape through a // transport error, which net/http otherwise builds from the full request URL. // Inputs: a synthetic key and httptest servers. Outputs: pass/fail. // Constraints: loopback only. @@ -17,17 +17,17 @@ import ( const migrateTestKey = "synthetic key&x=1#frag" func TestFetchMigrationStatusEncodesKey(t *testing.T) { - var gotKey, gotPath string + var gotHeader, gotPath, gotQuery string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotKey, gotPath = r.URL.Query().Get("key"), r.URL.Path + gotHeader, gotPath, gotQuery = r.Header.Get("X-License-Key"), r.URL.Path, r.URL.RawQuery _, _ = w.Write([]byte(`{}`)) })) defer srv.Close() if _, err := fetchMigrationStatus(context.Background(), migrateTestKey, srv.URL); err != nil { t.Fatal(err) } - if gotPath != "/license/migration-status" || gotKey != migrateTestKey { - t.Errorf("server saw path %q key %q", gotPath, gotKey) + if gotPath != "/license/migration-status" || gotQuery != "" || gotHeader != migrateTestKey { + t.Errorf("server saw path %q query %q header %q", gotPath, gotQuery, gotHeader) } }