From 95603737ad4368a1a0bef0d5ca479d95bbc26b2e Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:58:01 -0700 Subject: [PATCH 01/19] fix(config): write credentials atomically and re-tighten permissions Save() now writes to a temp file in the same directory and renames it into place, so a crash mid-write cannot corrupt the credentials file. It also chmods the file to 0600 on every save, so a pre-existing over-permissive file gets re-tightened rather than left as-is. --- internal/commands/config/manager.go | 27 ++++++++++++- internal/commands/config/manager_test.go | 48 ++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/internal/commands/config/manager.go b/internal/commands/config/manager.go index acfa6ba1..19ed4567 100644 --- a/internal/commands/config/manager.go +++ b/internal/commands/config/manager.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "strings" "time" ) @@ -227,9 +228,33 @@ func (m *ConfigManager) Save() error { if err != nil { return fmt.Errorf("failed to marshal config: %w", err) } - if err := os.WriteFile(configPath, configData, 0600); err != nil { + + tmpFile, err := os.CreateTemp(filepath.Dir(configPath), ".megaport-config-*.tmp") + if err != nil { + return fmt.Errorf("failed to create temp config file: %w", err) + } + tmpPath := tmpFile.Name() + defer os.Remove(tmpPath) // no-op once the rename below succeeds + + if _, err := tmpFile.Write(configData); err != nil { + tmpFile.Close() + return fmt.Errorf("failed to write temp config file: %w", err) + } + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("failed to close temp config file: %w", err) + } + if err := chmodFile(tmpPath, 0600); err != nil { + return fmt.Errorf("failed to set permissions on temp config file: %w", err) + } + if err := renameFile(tmpPath, configPath); err != nil { return fmt.Errorf("failed to write config file: %w", err) } + // Rename already carries over the temp file's 0600 mode, but re-tighten + // explicitly in case configPath pre-existed with looser permissions on a + // platform where rename doesn't replace the mode bits. + if err := chmodFile(configPath, 0600); err != nil { + return fmt.Errorf("failed to set permissions on config file: %w", err) + } return nil } diff --git a/internal/commands/config/manager_test.go b/internal/commands/config/manager_test.go index 57187241..af17601e 100644 --- a/internal/commands/config/manager_test.go +++ b/internal/commands/config/manager_test.go @@ -667,17 +667,59 @@ func TestReadOnlyConfigFile(t *testing.T) { err = os.WriteFile(configPath, data, 0644) require.NoError(t, err) - // Make the config file read-only - err = os.Chmod(configPath, 0444) + // Make the config directory read-only. Save() now writes atomically + // (temp file + rename), so a read-only config file no longer blocks a + // write; only a read-only directory does, since it prevents creating + // the temp file. + err = os.Chmod(configDir, 0555) require.NoError(t, err) + defer os.Chmod(configDir, 0700) //nolint:errcheck // best-effort restore so cleanup can remove tempDir // Try to load config - should fail due to version migration - // attempting to save to read-only file + // attempting to save while the config directory is read-only _, err = NewConfigManager() assert.Error(t, err) assert.Contains(t, err.Error(), "permission denied") } +func TestSaveRetightensOverPermissiveFile(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("Skipping test when running as root") + } + + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + + configPath, err := GetConfigFilePath() + require.NoError(t, err) + + // Simulate a pre-existing config file left world-readable. + require.NoError(t, os.Chmod(configPath, 0644)) + + require.NoError(t, manager.CreateProfile("test-profile", "access123", "secret123", "production", "")) + + info, err := os.Stat(configPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0600), info.Mode().Perm(), "Save should re-tighten an over-permissive config file to 0600") +} + +func TestSaveLeavesNoTempFile(t *testing.T) { + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + require.NoError(t, manager.CreateProfile("test-profile", "access123", "secret123", "production", "")) + + configPath, err := GetConfigFilePath() + require.NoError(t, err) + + matches, err := filepath.Glob(filepath.Join(filepath.Dir(configPath), ".megaport-config-*.tmp")) + require.NoError(t, err) + assert.Empty(t, matches, "Save should not leave temp files behind after a successful write") +} + func TestProfileNameCaseSensitivity(t *testing.T) { setupTestConfig(t) From 2f29a5fe38c152ad68b94da3cb3c928c428173a0 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:58:06 -0700 Subject: [PATCH 02/19] fix(config): require same-source credentials for --env login Previously --env login could take the access key from the environment while falling back to the active profile for the secret key (or vice versa) when only one var was set. Both halves must now come from the same source, or login errors instead of silently mixing credentials. --- internal/commands/config/login.go | 27 +++++++------ internal/commands/config/login_test.go | 55 ++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/internal/commands/config/login.go b/internal/commands/config/login.go index e99627ed..3cacd632 100644 --- a/internal/commands/config/login.go +++ b/internal/commands/config/login.go @@ -154,24 +154,27 @@ var loginFuncWithOutput = func(ctx context.Context, outputFormat string) (*megap } else { // Credential selection: if --env flag is used, prefer env vars over profile if utils.Env != "" { - // --env flag was explicitly set, prioritize environment variables - accessKey = os.Getenv("MEGAPORT_ACCESS_KEY") - secretKey = os.Getenv("MEGAPORT_SECRET_KEY") - - // If env vars are empty, fall back to profile - if accessKey == "" || secretKey == "" { + // --env flag was explicitly set, prioritize environment variables, but + // never mix halves across sources: both keys must come from the + // environment, or both from the profile, never one from each. + envAccessKey := os.Getenv("MEGAPORT_ACCESS_KEY") + envSecretKey := os.Getenv("MEGAPORT_SECRET_KEY") + + switch { + case envAccessKey != "" && envSecretKey != "": + accessKey = envAccessKey + secretKey = envSecretKey + case envAccessKey == "" && envSecretKey == "": manager, err := NewConfigManager() if err == nil { profile, _, err := manager.GetCurrentProfile() if err == nil { - if accessKey == "" { - accessKey = profile.AccessKey - } - if secretKey == "" { - secretKey = profile.SecretKey - } + accessKey = profile.AccessKey + secretKey = profile.SecretKey } } + default: + return nil, fmt.Errorf("only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set; with --env, both must come from the environment or neither should be set") } } else { // No --env flag, use original priority: profile > env vars diff --git a/internal/commands/config/login_test.go b/internal/commands/config/login_test.go index 48e58d1f..2dd71f47 100644 --- a/internal/commands/config/login_test.go +++ b/internal/commands/config/login_test.go @@ -352,6 +352,61 @@ func TestProfileOverrideLogin(t *testing.T) { }) } +func TestEnvFlagPartialEnvVarsDoesNotMixWithProfile(t *testing.T) { + // Save and restore non-env-var globals + originalEnv := utils.Env + originalProfileOverride := utils.ProfileOverride + defer func() { + utils.Env = originalEnv + utils.ProfileOverride = originalProfileOverride + }() + + tempDir, err := os.MkdirTemp("", "megaport-login-test") + assert.NoError(t, err) + t.Cleanup(func() { os.RemoveAll(tempDir) }) + t.Setenv("MEGAPORT_CONFIG_DIR", tempDir) + + manager, err := NewConfigManager() + assert.NoError(t, err) + err = manager.CreateProfile("prod", "profile-access", "profile-secret", "production", "") + assert.NoError(t, err) + err = manager.UseProfile("prod") + assert.NoError(t, err) + + utils.ProfileOverride = "" + utils.Env = "production" + + t.Run("only access key set in env errors instead of mixing with profile", func(t *testing.T) { + t.Setenv("MEGAPORT_ACCESS_KEY", "env-access-key") + t.Setenv("MEGAPORT_SECRET_KEY", "") + + _, err := LoginWithOutput(context.Background(), "json") + assert.Error(t, err) + assert.Contains(t, err.Error(), "only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set") + }) + + t.Run("only secret key set in env errors instead of mixing with profile", func(t *testing.T) { + t.Setenv("MEGAPORT_ACCESS_KEY", "") + t.Setenv("MEGAPORT_SECRET_KEY", "env-secret-key") + + _, err := LoginWithOutput(context.Background(), "json") + assert.Error(t, err) + assert.Contains(t, err.Error(), "only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set") + }) + + t.Run("neither env var set falls back fully to profile", func(t *testing.T) { + t.Setenv("MEGAPORT_ACCESS_KEY", "") + t.Setenv("MEGAPORT_SECRET_KEY", "") + + _, err := LoginWithOutput(context.Background(), "json") + assert.Error(t, err) + // Reaches the Authorize call (network error) rather than failing on + // missing credentials or the partial-env-var mixing guard. + assert.NotContains(t, err.Error(), "access key not provided") + assert.NotContains(t, err.Error(), "only one of") + }) +} + func TestNewUnauthenticatedClient(t *testing.T) { // Save and restore non-env-var globals originalEnv := utils.Env From 0918f77fed215ec7b4e9dd457da4df973daeef37 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:58:09 -0700 Subject: [PATCH 03/19] fix(locations): guard against nil entries in locations get locations get iterated the raw []*megaport.LocationV3 without the nil check that filterLocations already applies, so a nil entry in the response could panic. Skip nil entries the same way filterLocations does. --- internal/commands/locations/locations_actions.go | 3 +++ internal/commands/locations/locations_actions_test.go | 10 +++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/internal/commands/locations/locations_actions.go b/internal/commands/locations/locations_actions.go index 8f0c9f77..e868aa28 100644 --- a/internal/commands/locations/locations_actions.go +++ b/internal/commands/locations/locations_actions.go @@ -322,6 +322,9 @@ func GetLocation(cmd *cobra.Command, args []string, noColor bool, outputFormat s var targetLocation *megaport.LocationV3 for _, loc := range locations { + if loc == nil { + continue + } if loc.ID == locationID { targetLocation = loc break diff --git a/internal/commands/locations/locations_actions_test.go b/internal/commands/locations/locations_actions_test.go index cc26849a..a984a4f5 100644 --- a/internal/commands/locations/locations_actions_test.go +++ b/internal/commands/locations/locations_actions_test.go @@ -347,7 +347,7 @@ func TestGetLocation(t *testing.T) { name: "invalid ID arg", args: []string{"abc"}, setupMock: func(m *MockLocationsService) {}, - expectedErr: "invalid location ID", + expectedErr: "Invalid location ID", }, { name: "not found", @@ -365,6 +365,14 @@ func TestGetLocation(t *testing.T) { }, expectedErr: "failed to list locations", }, + { + name: "nil entry in list does not panic", + args: []string{"2"}, + setupMock: func(m *MockLocationsService) { + m.ListLocationsV3Result = append([]*megaport.LocationV3{nil}, testLocationsV3...) + }, + expectedOutput: "London Data Center", + }, { name: "client creation error", args: []string{"1"}, From 2fa34d7ccb22f08d71df0bb02526c44a1325b9df Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:58:16 -0700 Subject: [PATCH 04/19] fix(cli): classify usage errors by type, not message substring Exit-code classification previously matched "unknown flag"/"invalid"/ "arg(s)" against error text, so an API error that happened to contain those words could be misclassified as a usage error. Flag-parse failures and missing-required-flag errors are now tagged as a typed usage error at the point cobra generates them (SetFlagErrorFunc and a proactive ValidateRequiredFlags check), and ParseInt now returns a typed *ValidationError instead of a plain error, so classifyError can key off error types instead of wording. isCobraUsageError's substring match remains only as a fallback for the "unknown command" and "arg(s)" cases that run before those hooks and have no typed equivalent. --- cmd/megaport/megaport.go | 28 +++++++++++++++ cmd/megaport/megaport_test.go | 34 +++++++++++++++++++ internal/commands/ix/ix_test.go | 12 +++---- .../mcr/mcr_actions_prefix_filter_test.go | 2 +- internal/commands/mcr/mcr_prompts_test.go | 16 ++++----- internal/commands/mve/mve_prompts_test.go | 10 +++--- .../partners/partners_actions_test.go | 2 +- internal/commands/ports/ports_prompts_test.go | 10 +++--- internal/commands/users/users_actions_test.go | 8 ++--- internal/validation/parse.go | 12 +++---- internal/validation/parse_test.go | 22 ++++++------ 11 files changed, 108 insertions(+), 48 deletions(-) diff --git a/cmd/megaport/megaport.go b/cmd/megaport/megaport.go index 9085cf2e..7728a630 100644 --- a/cmd/megaport/megaport.go +++ b/cmd/megaport/megaport.go @@ -22,6 +22,13 @@ func init() { // Initialize common components InitializeCommon() + // Structurally tag flag-parsing failures (unknown flag, unknown shorthand + // flag, malformed value) as usage errors at the point cobra generates them, + // instead of pattern-matching the message text later in exitCodeFromError. + rootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { + return exitcodes.NewUsageError(err) + }) + // Apply non-WASM specific initialization rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { defaultWarnings := applyDefaultSettings(cmd) @@ -75,6 +82,14 @@ func init() { return utils.FinishPreRunError(cmd, args, exitcodes.NewUsageError(err)) } + // Run cobra's own required-flags check here, ahead of when cobra would + // normally run it after PersistentPreRunE, so a missing required flag + // gets tagged as a typed usage error instead of falling back to + // isCobraUsageError's message-substring match. + if err := cmd.ValidateRequiredFlags(); err != nil { + return utils.FinishPreRunError(cmd, args, exitcodes.NewUsageError(err)) + } + return nil } @@ -313,6 +328,19 @@ func exitCodeFromError(err error) int { return exitcodes.General } +// isCobraUsageError is a defensive fallback for cobra's own error shapes. +// Flag-parse failures and missing-required-flag errors are now tagged as a +// typed usage error at the source (rootCmd.SetFlagErrorFunc and the +// ValidateRequiredFlags check in PersistentPreRunE), so in practice those +// reach exitCodeFromError as a *exitcodes.CLIError and never fall through to +// this substring match. "unknown command" (from cobra.Command.Find) and +// "arg(s)" (from the per-command Args validator) run before +// PersistentPreRunE and have no equivalent hook, so they still rely on this +// match. Matching here on any of these patterns is safe because every RunE +// in this codebase is wrapped by utils.Wrap*, which always converts its own +// return value to a typed *exitcodes.CLIError before it reaches cobra - so a +// plain, untyped error surfacing all the way to exitCodeFromError can only +// have originated from cobra itself, never from application or API text. func isCobraUsageError(msg string) bool { cobraPatterns := []string{ "unknown command", diff --git a/cmd/megaport/megaport_test.go b/cmd/megaport/megaport_test.go index 590c9313..726bb6fa 100644 --- a/cmd/megaport/megaport_test.go +++ b/cmd/megaport/megaport_test.go @@ -351,3 +351,37 @@ func TestExitCodeFromError_CobraArgValidators(t *testing.T) { }) } } + +// TestUnknownFlagIsTypedUsageError verifies that rootCmd.SetFlagErrorFunc +// tags a flag-parse failure as a typed *exitcodes.CLIError at the point +// cobra generates it, rather than relying on exitCodeFromError's message +// substring match. +func TestUnknownFlagIsTypedUsageError(t *testing.T) { + rootCmd.SetArgs([]string{"version", "--totally-bogus-flag"}) + var execErr error + _ = output.CaptureOutput(func() { + execErr = rootCmd.Execute() + }) + + require.Error(t, execErr) + var cliErr *exitcodes.CLIError + require.True(t, errors.As(execErr, &cliErr), "expected a typed *exitcodes.CLIError, got %T: %v", execErr, execErr) + assert.Equal(t, exitcodes.Usage, cliErr.Code) +} + +// TestMissingRequiredFlagIsTypedUsageError verifies that the proactive +// cmd.ValidateRequiredFlags() check added to PersistentPreRunE tags a +// missing-required-flag error as a typed *exitcodes.CLIError ahead of +// cobra's own (redundant) ValidateRequiredFlags call. +func TestMissingRequiredFlagIsTypedUsageError(t *testing.T) { + rootCmd.SetArgs([]string{"billing-market", "set"}) + var execErr error + _ = output.CaptureOutput(func() { + execErr = rootCmd.Execute() + }) + + require.Error(t, execErr) + var cliErr *exitcodes.CLIError + require.True(t, errors.As(execErr, &cliErr), "expected a typed *exitcodes.CLIError, got %T: %v", execErr, execErr) + assert.Equal(t, exitcodes.Usage, cliErr.Code) +} diff --git a/internal/commands/ix/ix_test.go b/internal/commands/ix/ix_test.go index c4e853f3..e66c259f 100644 --- a/internal/commands/ix/ix_test.go +++ b/internal/commands/ix/ix_test.go @@ -1268,7 +1268,7 @@ func TestBuildIXRequestFromPrompt(t *testing.T) { "Los Angeles IX", "notanumber", }, - expectedError: "invalid ASN", + expectedError: "Invalid ASN", }, { name: "empty MAC address", @@ -1291,7 +1291,7 @@ func TestBuildIXRequestFromPrompt(t *testing.T) { "00:11:22:33:44:55", "notanumber", }, - expectedError: "invalid rate limit", + expectedError: "Invalid rate limit", }, { name: "invalid VLAN (non-numeric)", @@ -1304,7 +1304,7 @@ func TestBuildIXRequestFromPrompt(t *testing.T) { "1000", "notanumber", }, - expectedError: "invalid VLAN", + expectedError: "Invalid VLAN", }, { name: "prompt error on first prompt", @@ -1474,7 +1474,7 @@ func TestBuildUpdateIXRequestFromPrompt(t *testing.T) { "", // name "notanumber", // rate-limit }, - expectedError: "invalid rate limit", + expectedError: "Invalid rate limit", }, { name: "invalid VLAN", @@ -1484,7 +1484,7 @@ func TestBuildUpdateIXRequestFromPrompt(t *testing.T) { "", // cost-centre "notanumber", // vlan }, - expectedError: "invalid VLAN", + expectedError: "Invalid VLAN", }, { name: "invalid ASN", @@ -1496,7 +1496,7 @@ func TestBuildUpdateIXRequestFromPrompt(t *testing.T) { "", // mac-address "notanumber", // asn }, - expectedError: "invalid ASN", + expectedError: "Invalid ASN", }, { name: "prompt error", diff --git a/internal/commands/mcr/mcr_actions_prefix_filter_test.go b/internal/commands/mcr/mcr_actions_prefix_filter_test.go index 28cd8e91..a508d375 100644 --- a/internal/commands/mcr/mcr_actions_prefix_filter_test.go +++ b/internal/commands/mcr/mcr_actions_prefix_filter_test.go @@ -66,5 +66,5 @@ func TestUpdateMCRPrefixFilterList_NonNumericID_PrintsToStderr(t *testing.T) { require.Error(t, err) }) - assert.Contains(t, stderr, "invalid prefix filter list ID", "failing command must print its error to stderr") + assert.Contains(t, stderr, "Invalid prefix filter list ID", "failing command must print its error to stderr") } diff --git a/internal/commands/mcr/mcr_prompts_test.go b/internal/commands/mcr/mcr_prompts_test.go index 51d08a83..74b18578 100644 --- a/internal/commands/mcr/mcr_prompts_test.go +++ b/internal/commands/mcr/mcr_prompts_test.go @@ -72,7 +72,7 @@ func TestPromptForMCRDetails_InvalidTerm(t *testing.T) { _, err := promptForMCRDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") } func TestPromptForMCRDetails_InvalidPortSpeed(t *testing.T) { @@ -93,7 +93,7 @@ func TestPromptForMCRDetails_InvalidLocationID(t *testing.T) { _, err := promptForMCRDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid location ID") + assert.Contains(t, err.Error(), "Invalid location ID") } func TestPromptForMCRDetails_MarketplaceVisibilityPromptError(t *testing.T) { @@ -125,7 +125,7 @@ func TestPromptForMCRDetails_InvalidPortSpeedNotNumeric(t *testing.T) { _, err := promptForMCRDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid port speed") + assert.Contains(t, err.Error(), "Invalid port speed") assert.NotContains(t, err.Error(), "strconv") } @@ -148,7 +148,7 @@ func TestPromptForMCRDetails_InvalidASN(t *testing.T) { _, err := promptForMCRDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid ASN") + assert.Contains(t, err.Error(), "Invalid ASN") assert.NotContains(t, err.Error(), "strconv") } @@ -219,7 +219,7 @@ func TestPromptForUpdateMCRDetails_InvalidTermNotNumeric(t *testing.T) { _, err := promptForUpdateMCRDetails("mcr-123", "", true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") assert.NotContains(t, err.Error(), "strconv") } @@ -269,7 +269,7 @@ func TestPromptForUpdateMCRDetails_InvalidASN(t *testing.T) { _, err := promptForUpdateMCRDetails("mcr-123", "", true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid ASN") + assert.Contains(t, err.Error(), "Invalid ASN") assert.NotContains(t, err.Error(), "strconv") } @@ -425,7 +425,7 @@ func TestPromptForIPSecTunnelCount(t *testing.T) { {"valid count 10", "10", 10, false, ""}, {"valid count 20", "20", 20, false, ""}, {"valid count 30", "30", 30, false, ""}, - {"non-numeric input", "abc", 0, true, "invalid tunnel count"}, + {"non-numeric input", "abc", 0, true, "Invalid tunnel count"}, } for _, tt := range tests { @@ -460,7 +460,7 @@ func TestPromptForIPSecTunnelCountUpdate(t *testing.T) { {"valid count 30", "30", 30, false, ""}, {"zero disables IPSec", "0", 0, false, ""}, {"empty input requires value", "", 0, true, "tunnel count is required"}, - {"non-numeric input", "abc", 0, true, "invalid tunnel count"}, + {"non-numeric input", "abc", 0, true, "Invalid tunnel count"}, } for _, tt := range tests { diff --git a/internal/commands/mve/mve_prompts_test.go b/internal/commands/mve/mve_prompts_test.go index a2135b40..a22cbbbb 100644 --- a/internal/commands/mve/mve_prompts_test.go +++ b/internal/commands/mve/mve_prompts_test.go @@ -151,7 +151,7 @@ func TestPromptMVEBaseDetails_InvalidTerm(t *testing.T) { _, _, _, _, _, err := promptMVEBaseDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") } func TestPromptMVEBaseDetails_InvalidLocationID(t *testing.T) { @@ -162,7 +162,7 @@ func TestPromptMVEBaseDetails_InvalidLocationID(t *testing.T) { _, _, _, _, _, err := promptMVEBaseDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid location ID") + assert.Contains(t, err.Error(), "Invalid location ID") } func TestPromptMVEBaseDetails_InvalidImageID(t *testing.T) { @@ -176,7 +176,7 @@ func TestPromptMVEBaseDetails_InvalidImageID(t *testing.T) { _, _, _, _, _, err := promptMVEBaseDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid image ID") + assert.Contains(t, err.Error(), "Invalid image ID") } // promptForUpdateMVEDetails tests @@ -217,7 +217,7 @@ func TestPromptForUpdateMVEDetails_InvalidContractTerm(t *testing.T) { _, err := promptForUpdateMVEDetails("mve-123", "", nil, true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid contract term") + assert.Contains(t, err.Error(), "Invalid contract term") } func TestPromptForUpdateMVEDetails_UpdateVnicDescriptions(t *testing.T) { @@ -397,7 +397,7 @@ func TestPromptMVEVnics_InvalidVLAN(t *testing.T) { _, err := promptMVEVnics(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid VLAN ID") + assert.Contains(t, err.Error(), "Invalid VLAN ID") } // promptMVEVendorConfig tests — cisco/palo_alto admin password handling diff --git a/internal/commands/partners/partners_actions_test.go b/internal/commands/partners/partners_actions_test.go index fa03a206..0f72c55c 100644 --- a/internal/commands/partners/partners_actions_test.go +++ b/internal/commands/partners/partners_actions_test.go @@ -109,7 +109,7 @@ func TestFindPartners(t *testing.T) { "", "table", }, - expectedError: "invalid location ID", + expectedError: "Invalid location ID", setupMock: func(t *testing.T, m *MockPartnerService) { m.listPartnersResponse = []*megaport.PartnerMegaport{} m.listPartnersErr = nil diff --git a/internal/commands/ports/ports_prompts_test.go b/internal/commands/ports/ports_prompts_test.go index c6ef3a93..dd6248cb 100644 --- a/internal/commands/ports/ports_prompts_test.go +++ b/internal/commands/ports/ports_prompts_test.go @@ -76,7 +76,7 @@ func TestPromptForPortDetails_InvalidTerm(t *testing.T) { _, err := promptForPortDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") } func TestPromptForPortDetails_InvalidPortSpeed(t *testing.T) { @@ -98,7 +98,7 @@ func TestPromptForPortDetails_InvalidLocationID(t *testing.T) { _, err := promptForPortDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid location ID") + assert.Contains(t, err.Error(), "Invalid location ID") } func TestPromptForPortDetails_InvalidMarketplaceVisibility(t *testing.T) { @@ -234,7 +234,7 @@ func TestPromptForUpdatePortDetails_InvalidTermNotNumeric(t *testing.T) { _, err := promptForUpdatePortDetails("port-123", "", true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") assert.NotContains(t, err.Error(), "strconv") } @@ -246,7 +246,7 @@ func TestPromptForLAGPortDetails_InvalidTerm(t *testing.T) { _, err := promptForLAGPortDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") assert.NotContains(t, err.Error(), "strconv") } @@ -258,6 +258,6 @@ func TestPromptForLAGPortDetails_InvalidLocationID(t *testing.T) { _, err := promptForLAGPortDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid location ID") + assert.Contains(t, err.Error(), "Invalid location ID") assert.NotContains(t, err.Error(), "strconv") } diff --git a/internal/commands/users/users_actions_test.go b/internal/commands/users/users_actions_test.go index 22dd5aea..36540880 100644 --- a/internal/commands/users/users_actions_test.go +++ b/internal/commands/users/users_actions_test.go @@ -114,7 +114,7 @@ func TestGetUser(t *testing.T) { name: "invalid employee ID", args: []string{"abc"}, setupMock: func(m *MockUserManagementService) {}, - expectedError: "invalid employee ID", + expectedError: "Invalid employee ID", }, { name: "nil user", @@ -368,7 +368,7 @@ func TestUpdateUser(t *testing.T) { name: "invalid employee ID", args: []string{"abc"}, setupMock: func(m *MockUserManagementService) {}, - expectedError: "invalid employee ID", + expectedError: "Invalid employee ID", }, } @@ -478,7 +478,7 @@ func TestDeleteUser(t *testing.T) { args: []string{"abc"}, force: true, setupMock: func(m *MockUserManagementService) {}, - expectedError: "invalid employee ID", + expectedError: "Invalid employee ID", }, } @@ -572,7 +572,7 @@ func TestDeactivateUser(t *testing.T) { args: []string{"abc"}, force: true, setupMock: func(m *MockUserManagementService) {}, - expectedError: "invalid employee ID", + expectedError: "Invalid employee ID", }, } diff --git a/internal/validation/parse.go b/internal/validation/parse.go index 3dc3ac21..7ef24b5e 100644 --- a/internal/validation/parse.go +++ b/internal/validation/parse.go @@ -1,21 +1,17 @@ package validation import ( - "fmt" "strconv" ) // ParseInt converts a user-supplied string into an int. On failure it returns a -// friendly error naming the field and the offending value, instead of leaking -// strconv internals like `strconv.Atoi: parsing "x": invalid syntax`. -// -// The message keeps the lowercase "invalid " prefix so exit-code -// classification still tags it as a usage error. For ID arguments the field -// should contain "ID" (e.g. "location ID") so that classification holds. +// friendly, typed *ValidationError naming the field and the offending value, +// instead of leaking strconv internals like `strconv.Atoi: parsing "x": invalid +// syntax` or relying on exit-code classification matching the message text. func ParseInt(field, value string) (int, error) { n, err := strconv.Atoi(value) if err != nil { - return 0, fmt.Errorf("invalid %s: %q is not a valid whole number", field, value) + return 0, NewValidationError(field, value, "is not a valid whole number") } return n, nil } diff --git a/internal/validation/parse_test.go b/internal/validation/parse_test.go index 88e1a25f..8142db46 100644 --- a/internal/validation/parse_test.go +++ b/internal/validation/parse_test.go @@ -1,10 +1,11 @@ package validation import ( - "strings" + "errors" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestParseInt(t *testing.T) { @@ -25,21 +26,21 @@ func TestParseInt(t *testing.T) { value: "bad-location", wantErr: true, // Friendly message: names the field and value, no strconv internals. - errSubstr: []string{"invalid location ID", "bad-location"}, + errSubstr: []string{"location ID", "bad-location", "not a valid whole number"}, }, { name: "empty string", field: "employee ID", value: "", wantErr: true, - errSubstr: []string{"invalid employee ID"}, + errSubstr: []string{"employee ID", "not a valid whole number"}, }, { name: "float string", field: "term", value: "1.5", wantErr: true, - errSubstr: []string{"invalid term"}, + errSubstr: []string{"term", "not a valid whole number"}, }, } @@ -61,11 +62,12 @@ func TestParseInt(t *testing.T) { } } -// classifyError keys off a lowercase "invalid" + "ID" substring, so the helper -// must keep that wording for ID fields to preserve the Usage exit code. -func TestParseIntPreservesUsageClassification(t *testing.T) { +// TestParseIntReturnsTypedValidationError verifies ParseInt's failure is a +// typed *ValidationError, which is what actually drives the Usage exit code +// in classifyError (not a substring match against the message text). +func TestParseIntReturnsTypedValidationError(t *testing.T) { _, err := ParseInt("location ID", "abc") - assert.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "invalid")) - assert.True(t, strings.Contains(err.Error(), "ID")) + require.Error(t, err) + var validationErr *ValidationError + assert.True(t, errors.As(err, &validationErr), "expected a typed *ValidationError, got %T: %v", err, err) } From 0a09b44d2d0bb591d7f03d1dbb9accc295517385 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:58:25 -0700 Subject: [PATCH 05/19] fix(buy): require --yes for JSON-mode purchase confirmation JSON and json-file input skipped the purchase confirmation prompt entirely, so a JSON-mode buy without --yes went straight through with no interactive prompt and no explicit acknowledgement. Every buy/create command (port, LAG port, VXC, MVE, IX, MCR, NAT Gateway) now requires --yes to proceed in JSON mode, and returns a usage error otherwise. The check runs as early as each function's data flow allows, ahead of login and live validation calls, so a doomed request fails fast instead of spending a network round trip first. --- internal/commands/ix/ix_actions.go | 12 ++++-- internal/commands/ix/ix_actions_test.go | 14 ++++++- internal/commands/mcr/mcr_actions.go | 12 ++++-- internal/commands/mcr/mcr_actions_test.go | 19 +++++++-- internal/commands/mve/mve_actions.go | 12 ++++-- internal/commands/mve/mve_actions_test.go | 40 ++++++++++++++++++- .../nat_gateway/nat_gateway_actions.go | 9 +++-- .../nat_gateway_additional_test.go | 1 + .../commands/nat_gateway/nat_gateway_test.go | 13 ++++++ internal/commands/ports/ports_actions.go | 24 +++++++---- internal/commands/ports/ports_actions_test.go | 12 +++++- internal/commands/vxc/vxc_actions.go | 12 ++++-- internal/commands/vxc/vxc_actions_test.go | 9 ++++- 13 files changed, 153 insertions(+), 36 deletions(-) diff --git a/internal/commands/ix/ix_actions.go b/internal/commands/ix/ix_actions.go index d0125c01..5a15f12f 100644 --- a/internal/commands/ix/ix_actions.go +++ b/internal/commands/ix/ix_actions.go @@ -198,6 +198,13 @@ func BuyIX(cmd *cobra.Command, args []string, noColor bool) error { return err } + jsonStr, _ := cmd.Flags().GetString("json") + jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") + if !yes && (jsonStr != "" || jsonFile != "") { + return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + } + // Flag read errors are intentionally ignored — flags are registered by the command builder. noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK @@ -220,10 +227,7 @@ func BuyIX(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Network Service Type", Value: req.NetworkServiceType}, diff --git a/internal/commands/ix/ix_actions_test.go b/internal/commands/ix/ix_actions_test.go index 98d47790..953a8219 100644 --- a/internal/commands/ix/ix_actions_test.go +++ b/internal/commands/ix/ix_actions_test.go @@ -1556,9 +1556,11 @@ func TestBuyIX_JSONStringMode(t *testing.T) { cmd.Flags().String("promo-code", "", "Promo code") cmd.Flags().String("json", "", "JSON string") cmd.Flags().String("json-file", "", "JSON file") + cmd.Flags().Bool("yes", false, "Skip confirmation") jsonInput := `{"productUid":"port-uid-json","productName":"JSON IX","networkServiceType":"Sydney IX","asn":65100,"macAddress":"AA:BB:CC:DD:EE:FF","rateLimit":2000,"vlan":200}` _ = cmd.Flags().Set("json", jsonInput) + _ = cmd.Flags().Set("yes", "true") var err error var capturedStderr string @@ -1735,15 +1737,25 @@ func TestBuyIX_Confirmation(t *testing.T) { promptShouldBeCalled: false, }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", flags: map[string]string{ "json": `{"productUid":"port-uid-123","productName":"Test IX","networkServiceType":"Los Angeles IX","asn":65000,"macAddress":"00:11:22:33:44:55","rateLimit":1000,"vlan":100}`, + "yes": "true", }, confirmResult: false, expectBuyCalled: true, expectedOutput: "IX created", promptShouldBeCalled: false, }, + { + name: "json input without yes is a usage error", + flags: map[string]string{ + "json": `{"productUid":"port-uid-123","productName":"Test IX","networkServiceType":"Los Angeles IX","asn":65000,"macAddress":"00:11:22:33:44:55","rateLimit":1000,"vlan":100}`, + }, + expectBuyCalled: false, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + promptShouldBeCalled: false, + }, } for _, tt := range tests { diff --git a/internal/commands/mcr/mcr_actions.go b/internal/commands/mcr/mcr_actions.go index 487a3902..4767fcc6 100644 --- a/internal/commands/mcr/mcr_actions.go +++ b/internal/commands/mcr/mcr_actions.go @@ -80,6 +80,13 @@ func BuyMCR(cmd *cobra.Command, args []string, noColor bool) error { }) } + jsonStr, _ := cmd.Flags().GetString("json") + jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") + if !yes && (jsonStr != "" || jsonFile != "") { + return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + } + // Flag read errors are intentionally ignored — flags are registered by the command builder. noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK @@ -102,10 +109,7 @@ func BuyMCR(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/mcr/mcr_actions_test.go b/internal/commands/mcr/mcr_actions_test.go index 28046771..9bb30ece 100644 --- a/internal/commands/mcr/mcr_actions_test.go +++ b/internal/commands/mcr/mcr_actions_test.go @@ -688,7 +688,7 @@ func TestGetMCRPrefixFilterListCmd_WithMockClient(t *testing.T) { mcrUID: "mcr-123", rawPrefixListID: "abc", setupMock: func(m *MockMCRService) {}, - expectedError: "invalid prefix filter list ID", + expectedError: "Invalid prefix filter list ID", }, } @@ -793,7 +793,7 @@ func TestDeleteMCRPrefixFilterListCmd_WithMockClient(t *testing.T) { rawPrefixListID: "abc", force: true, setupMock: func(m *MockMCRService) {}, - expectedError: "invalid prefix filter list ID", + expectedError: "Invalid prefix filter list ID", }, } @@ -1098,6 +1098,7 @@ func TestBuyMCRCmd_WithMockClient(t *testing.T) { name: "JSON string mode success", flags: map[string]string{ "json": `{"name":"JSON MCR","term":24,"portSpeed":10000,"locationId":123,"mcrAsn":65000,"diversityZone":"green","costCentre":"cost-789","promoCode":"JSONPROMO","marketplaceVisibility":true}`, + "yes": "true", }, setupMock: func(m *MockMCRService) { m.BuyMCRResult = &megaport.BuyMCRResponse{ @@ -2683,7 +2684,7 @@ func TestUpdateMCRPrefixFilterList(t *testing.T) { { name: "invalid prefix filter list ID", args: []string{"mcr-123", "abc"}, - expectedError: "invalid prefix filter list ID", + expectedError: "Invalid prefix filter list ID", }, { name: "API error", @@ -3017,15 +3018,25 @@ func TestBuyMCR_Confirmation(t *testing.T) { promptShouldBeCalled: false, }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", flags: map[string]string{ "json": `{"name":"JSON MCR","term":12,"portSpeed":10000,"locationId":123,"mcrAsn":65000}`, + "yes": "true", }, confirmResult: false, expectBuyCalled: true, expectedOutput: "MCR created", promptShouldBeCalled: false, }, + { + name: "json input without yes is a usage error", + flags: map[string]string{ + "json": `{"name":"JSON MCR","term":12,"portSpeed":10000,"locationId":123,"mcrAsn":65000}`, + }, + expectBuyCalled: false, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + promptShouldBeCalled: false, + }, } for _, tt := range tests { diff --git a/internal/commands/mve/mve_actions.go b/internal/commands/mve/mve_actions.go index 16283a32..6a345e35 100644 --- a/internal/commands/mve/mve_actions.go +++ b/internal/commands/mve/mve_actions.go @@ -141,6 +141,13 @@ func BuyMVE(cmd *cobra.Command, args []string, noColor bool) error { return err } + jsonStr, _ := cmd.Flags().GetString("json") + jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") + if !yes && (jsonStr != "" || jsonFile != "") { + return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + } + client, err := config.Login(ctx) if err != nil { output.PrintError("Failed to log in: %v", noColor, err) @@ -165,10 +172,7 @@ func BuyMVE(cmd *cobra.Command, args []string, noColor bool) error { output.PrintInfo("Validation successful", noColor) - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/mve/mve_actions_test.go b/internal/commands/mve/mve_actions_test.go index a2b6d756..39100b86 100644 --- a/internal/commands/mve/mve_actions_test.go +++ b/internal/commands/mve/mve_actions_test.go @@ -939,6 +939,7 @@ func TestBuyMVE(t *testing.T) { name: "json mode success", args: []string{}, flags: map[string]string{ + "yes": "true", "json": `{ "name": "JSON MVE", "term": 12, @@ -1041,10 +1042,36 @@ func TestBuyMVE(t *testing.T) { { name: "invalid JSON returns error", flags: map[string]string{ + "yes": "true", "json": `{bad json}`, }, expectedError: "failed to parse JSON", }, + { + name: "json without yes is a usage error", + flags: map[string]string{ + "json": `{ + "name": "JSON MVE", + "term": 12, + "locationId": 123, + "vendorConfig": { + "vendor": "cisco", + "imageId": 1, + "productSize": "LARGE", + "mveLabel": "json-label", + "manageLocally": true, + "adminSshPublicKey": "admin-ssh", + "sshPublicKey": "ssh-key", + "cloudInit": "cloud-init", + "fmcIpAddress": "fmc-ip", + "fmcRegistrationKey": "fmc-key", + "fmcNatId": "fmc-nat" + }, + "vnics": [{"description": "JSON VNIC", "vlan": 200}] + }`, + }, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + }, { name: "interactive combined with JSON is a usage error", interactive: true, @@ -1109,6 +1136,7 @@ func TestBuyMVE(t *testing.T) { cmd.Flags().Int("location-id", 0, "") cmd.Flags().String("vendor-config", "", "") cmd.Flags().String("vnics", "", "") + cmd.Flags().Bool("yes", false, "") testutil.SetFlags(t, cmd, tt.flags) @@ -2248,15 +2276,25 @@ func TestBuyMVE_Confirmation(t *testing.T) { promptShouldBeCalled: false, }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", flags: map[string]string{ "json": `{"name":"JSON MVE","term":12,"locationId":123,"vendorConfig":{"vendor":"cisco","imageId":1,"productSize":"LARGE","mveLabel":"label-1","manageLocally":true,"adminSshPublicKey":"admin-ssh","sshPublicKey":"ssh-key","cloudInit":"cloud-init","fmcIpAddress":"fmc-ip","fmcRegistrationKey":"fmc-key","fmcNatId":"fmc-nat"},"vnics":[{"description":"VNIC 1","vlan":100}]}`, + "yes": "true", }, confirmResult: false, expectBuyCalled: true, expectedOutput: "MVE created", promptShouldBeCalled: false, }, + { + name: "json input without yes is a usage error", + flags: map[string]string{ + "json": `{"name":"JSON MVE","term":12,"locationId":123,"vendorConfig":{"vendor":"cisco","imageId":1,"productSize":"LARGE","mveLabel":"label-1","manageLocally":true,"adminSshPublicKey":"admin-ssh","sshPublicKey":"ssh-key","cloudInit":"cloud-init","fmcIpAddress":"fmc-ip","fmcRegistrationKey":"fmc-key","fmcNatId":"fmc-nat"},"vnics":[{"description":"VNIC 1","vlan":100}]}`, + }, + expectBuyCalled: false, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + promptShouldBeCalled: false, + }, } for _, tt := range tests { diff --git a/internal/commands/nat_gateway/nat_gateway_actions.go b/internal/commands/nat_gateway/nat_gateway_actions.go index f3e9c235..40ad8845 100644 --- a/internal/commands/nat_gateway/nat_gateway_actions.go +++ b/internal/commands/nat_gateway/nat_gateway_actions.go @@ -23,6 +23,7 @@ func CreateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { interactive, _ := cmd.Flags().GetBool("interactive") jsonStr, _ := cmd.Flags().GetString("json") jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") flagsProvided := cmd.Flags().Changed("name") || cmd.Flags().Changed("term") || cmd.Flags().Changed("speed") || cmd.Flags().Changed("location-id") || cmd.Flags().Changed("session-count") || cmd.Flags().Changed("asn") || @@ -34,6 +35,10 @@ func CreateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { return err } + if !yes && (jsonStr != "" || jsonFile != "") { + return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm creating a NAT Gateway design when using --json or --json-file")) + } + var req *megaport.CreateNATGatewayRequest var err error @@ -53,8 +58,6 @@ func CreateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { return err } - yes, _ := cmd.Flags().GetBool("yes") - client, err := config.Login(ctx) if err != nil { output.PrintError("Failed to log in: %v", noColor, err) @@ -66,7 +69,7 @@ func CreateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { return err } - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.ProductName}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/nat_gateway/nat_gateway_additional_test.go b/internal/commands/nat_gateway/nat_gateway_additional_test.go index 4ff4bb44..fce7214a 100644 --- a/internal/commands/nat_gateway/nat_gateway_additional_test.go +++ b/internal/commands/nat_gateway/nat_gateway_additional_test.go @@ -880,6 +880,7 @@ func TestCreateNATGateway_JSONWithSessionCount(t *testing.T) { cmd := newTestCmd("create") require.NoError(t, cmd.Flags().Set("json", `{"name":"GW","term":12,"speed":1000,"locationId":1,"sessionCount":500,"diversityZone":"blue","autoRenewTerm":true}`)) + require.NoError(t, cmd.Flags().Set("yes", "true")) err := CreateNATGateway(cmd, nil, true) assert.NoError(t, err) diff --git a/internal/commands/nat_gateway/nat_gateway_test.go b/internal/commands/nat_gateway/nat_gateway_test.go index 2673dc5a..75066995 100644 --- a/internal/commands/nat_gateway/nat_gateway_test.go +++ b/internal/commands/nat_gateway/nat_gateway_test.go @@ -80,6 +80,7 @@ func TestCreateNATGateway_JSON(t *testing.T) { cmd := newTestCmd("create") require.NoError(t, cmd.Flags().Set("json", `{"name":"JSON GW","term":12,"speed":2000,"locationId":456}`)) + require.NoError(t, cmd.Flags().Set("yes", "true")) err := CreateNATGateway(cmd, nil, true) assert.NoError(t, err) @@ -88,6 +89,18 @@ func TestCreateNATGateway_JSON(t *testing.T) { assert.Equal(t, 456, mock.CapturedCreateReq.LocationID) } +func TestCreateNATGateway_JSONWithoutYes(t *testing.T) { + mock := &MockNATGatewayService{} + defer setupMockNATGateway(mock)() + + cmd := newTestCmd("create") + require.NoError(t, cmd.Flags().Set("json", `{"name":"JSON GW","term":12,"speed":2000,"locationId":456}`)) + + err := CreateNATGateway(cmd, nil, true) + assert.Error(t, err) + assert.Contains(t, err.Error(), "--yes is required to confirm creating a NAT Gateway design when using --json or --json-file") +} + func TestCreateNATGateway_NoInput(t *testing.T) { mock := &MockNATGatewayService{} defer setupMockNATGateway(mock)() diff --git a/internal/commands/ports/ports_actions.go b/internal/commands/ports/ports_actions.go index bbcfce7f..1b04b8de 100644 --- a/internal/commands/ports/ports_actions.go +++ b/internal/commands/ports/ports_actions.go @@ -73,6 +73,13 @@ func BuyPort(cmd *cobra.Command, args []string, noColor bool) error { return err } + jsonStr, _ := cmd.Flags().GetString("json") + jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") + if !yes && (jsonStr != "" || jsonFile != "") { + return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + } + // Flag read errors are intentionally ignored — flags are registered by the command builder. noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK @@ -95,10 +102,7 @@ func BuyPort(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, @@ -222,6 +226,13 @@ func BuyLAGPort(cmd *cobra.Command, args []string, noColor bool) error { return err } + jsonStr, _ := cmd.Flags().GetString("json") + jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") + if !yes && (jsonStr != "" || jsonFile != "") { + return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + } + noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK // must not also poll for provisioning: a 429 raised during polling would @@ -243,10 +254,7 @@ func BuyLAGPort(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/ports/ports_actions_test.go b/internal/commands/ports/ports_actions_test.go index 783e6f2a..670f8d08 100644 --- a/internal/commands/ports/ports_actions_test.go +++ b/internal/commands/ports/ports_actions_test.go @@ -979,9 +979,11 @@ func TestBuyPort(t *testing.T) { cmd.Flags().Bool("marketplace-visibility", false, "") cmd.Flags().String("diversity-zone", "", "") cmd.Flags().Bool("cost-confirm", true, "") + cmd.Flags().Bool("yes", false, "") if tt.jsonInput != "" { require.NoError(t, cmd.Flags().Set("json", tt.jsonInput)) + require.NoError(t, cmd.Flags().Set("yes", "true")) } for k, v := range tt.flags { require.NoError(t, cmd.Flags().Set(k, v)) @@ -1417,7 +1419,7 @@ func TestCheckPortVLANAvailability(t *testing.T) { name: "invalid VLAN arg", portUID: "port-vlan-3", vlanArg: "abc", - expectedError: "invalid VLAN ID", + expectedError: "Invalid VLAN ID", }, { name: "VLAN out of assignable range", @@ -1862,11 +1864,17 @@ func TestBuyPort_Confirmation(t *testing.T) { expectedContains: "new-port-uid-123", }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", jsonInput: `{"name":"json-port","term":12,"portSpeed":1000,"locationId":1,"marketPlaceVisibility":false}`, + yesFlag: true, expectPromptCalled: false, expectedContains: "new-port-uid-123", }, + { + name: "json input without yes is a usage error", + jsonInput: `{"name":"json-port","term":12,"portSpeed":1000,"locationId":1,"marketPlaceVisibility":false}`, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + }, } for _, tt := range tests { diff --git a/internal/commands/vxc/vxc_actions.go b/internal/commands/vxc/vxc_actions.go index 981a8be1..3da1205c 100644 --- a/internal/commands/vxc/vxc_actions.go +++ b/internal/commands/vxc/vxc_actions.go @@ -276,6 +276,13 @@ func BuyVXC(cmd *cobra.Command, args []string, noColor bool) error { return err } + jsonStr, _ := cmd.Flags().GetString("json") + jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") + if !yes && (jsonStr != "" || jsonFile != "") { + return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + } + noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK // must not also poll for provisioning: a 429 raised during polling would @@ -292,10 +299,7 @@ func BuyVXC(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.VXCName}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/vxc/vxc_actions_test.go b/internal/commands/vxc/vxc_actions_test.go index fb056ea1..495c4b8d 100644 --- a/internal/commands/vxc/vxc_actions_test.go +++ b/internal/commands/vxc/vxc_actions_test.go @@ -78,6 +78,7 @@ func TestBuyVXC_NilResponse(t *testing.T) { testutil.SetFlags(t, cmd, map[string]string{ "json": `{"portUid":"port-aaa-111","vxcName":"JSON VXC","rateLimit":500,"term":12,"bEndConfiguration":{"productUID":"port-bbb-222"}}`, + "yes": "true", }) var err error @@ -2504,11 +2505,17 @@ func TestBuyVXC_Confirmation(t *testing.T) { expectedContains: "vxc-uid-123", }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", jsonInput: `{"portUid":"dcc-12345","vxcName":"JSON VXC","rateLimit":500,"term":12,"aEndConfiguration":{"vlan":100},"bEndConfiguration":{"productUID":"dcc-67890","vlan":200}}`, + yesFlag: true, expectPromptCalled: false, expectedContains: "vxc-uid-123", }, + { + name: "json input without yes is a usage error", + jsonInput: `{"portUid":"dcc-12345","vxcName":"JSON VXC","rateLimit":500,"term":12,"aEndConfiguration":{"vlan":100},"bEndConfiguration":{"productUID":"dcc-67890","vlan":200}}`, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + }, } for _, tt := range tests { From 9f01f38711d0f9fc7ce2fdca1ebe75cbe76aacea Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:58:30 -0700 Subject: [PATCH 06/19] fix(docs): track temp doc files with an explicit flag FindDocFile decided whether to clean up a doc file by testing its path for a "megaport-docs-" substring, which is fragile if a real doc file or working directory happened to contain that string. It now returns an explicit isTemp bool and cleanup is driven by that instead. --- internal/base/cmdbuilder/docs_render.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/base/cmdbuilder/docs_render.go b/internal/base/cmdbuilder/docs_render.go index 849df225..9884851e 100644 --- a/internal/base/cmdbuilder/docs_render.go +++ b/internal/base/cmdbuilder/docs_render.go @@ -7,7 +7,6 @@ import ( "os" "path" "path/filepath" - "strings" "github.com/charmbracelet/glamour" "github.com/spf13/cobra" @@ -16,8 +15,9 @@ import ( // DocsDirectory is the fallback location for markdown documentation files var DocsDirectory = "./docs" -// FindDocFile locates the markdown file for a specific command -func FindDocFile(cmd *cobra.Command) (string, error) { +// FindDocFile locates the markdown file for a specific command. The second +// return value reports whether the path is a temp file the caller must remove. +func FindDocFile(cmd *cobra.Command) (string, bool, error) { cmdPath := getCommandPath(cmd) docName := cmdPath + ".md" @@ -30,18 +30,18 @@ func FindDocFile(cmd *cobra.Command) (string, error) { // by path and the caller removes it after rendering. tempFile, err := os.CreateTemp("", "megaport-docs-*.md") if err != nil { - return "", fmt.Errorf("failed to create temporary file: %w", err) + return "", false, fmt.Errorf("failed to create temporary file: %w", err) } if _, err := tempFile.Write(content); err != nil { _ = tempFile.Close() _ = os.Remove(tempFile.Name()) - return "", fmt.Errorf("failed to write to temporary file: %w", err) + return "", false, fmt.Errorf("failed to write to temporary file: %w", err) } if err := tempFile.Close(); err != nil { _ = os.Remove(tempFile.Name()) - return "", fmt.Errorf("failed to close temporary file: %w", err) + return "", false, fmt.Errorf("failed to close temporary file: %w", err) } - return tempFile.Name(), nil + return tempFile.Name(), true, nil } // If embedded file not found, try local docs directory as fallback @@ -49,10 +49,10 @@ func FindDocFile(cmd *cobra.Command) (string, error) { // Check if the file exists if _, err := os.Stat(docPath); os.IsNotExist(err) { - return "", fmt.Errorf("documentation file not found for %s: %w", cmdPath, err) + return "", false, fmt.Errorf("documentation file not found for %s: %w", cmdPath, err) } - return docPath, nil + return docPath, false, nil } // RenderDocFile reads and renders a markdown file using Glamour @@ -83,13 +83,13 @@ func RenderDocFile(filePath string) (string, error) { // ShowDocumentation displays rendered documentation for a command func ShowDocumentation(cmd *cobra.Command) error { - docPath, err := FindDocFile(cmd) + docPath, isTemp, err := FindDocFile(cmd) if err != nil { return err } // If we created a temporary file, ensure it gets deleted - if strings.Contains(docPath, "megaport-docs-") { + if isTemp { defer os.Remove(docPath) } From 5722af5f1e0e1b96b81932796ee2acc5b8f98582 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:58:34 -0700 Subject: [PATCH 07/19] fix(output): sanitize struct tag names before use as XML elements printXML wrote struct json tag names directly as XML element local names. Go's encoding/xml does not validate element names at encode time, so a tag starting with a digit, containing punctuation, or colliding with another tag after sanitization would silently produce malformed or ambiguous XML. Sanitize each name against the XML Name grammar (with collision disambiguation across a struct's fields) once per printXML call, for both the native and WASM renderers. --- internal/base/output/common.go | 59 ++++++++++++++++++++ internal/base/output/output.go | 6 +- internal/base/output/output_test.go | 86 +++++++++++++++++++++++++++++ internal/base/output/output_wasm.go | 10 +++- 4 files changed, 157 insertions(+), 4 deletions(-) diff --git a/internal/base/output/common.go b/internal/base/output/common.go index 8e97c56c..4121f039 100644 --- a/internal/base/output/common.go +++ b/internal/base/output/common.go @@ -452,6 +452,65 @@ func filterByFields(headers, jsonNames []string, indices []int, selected []strin return outHeaders, outJSONNames, outIndices, nil } +// isXMLNameStartChar reports whether r is legal as the first character of an +// XML element local name. Colon is deliberately excluded even though XML 1.0 +// allows it in a Name: it is namespace-significant, and treating a field name +// like "a:b" as a namespace-qualified name would produce an undeclared-prefix +// error in namespace-aware parsers. +func isXMLNameStartChar(r rune) bool { + return r == '_' || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') +} + +// isXMLNameChar reports whether r is legal as a non-first character of an XML +// element local name (see isXMLNameStartChar for why colon is excluded). +func isXMLNameChar(r rune) bool { + return isXMLNameStartChar(r) || r == '.' || r == '-' || (r >= '0' && r <= '9') +} + +// sanitizeXMLElementName rewrites name so it is safe to use as an XML element +// local name. Struct json tags are written for JSON, not XML, so a name like +// "a/b" or one starting with a digit would otherwise produce malformed XML. +func sanitizeXMLElementName(name string) string { + var b strings.Builder + b.Grow(len(name)) + for _, r := range name { + if isXMLNameChar(r) { + b.WriteRune(r) + } else { + b.WriteRune('_') + } + } + name = b.String() + if name == "" { + return "field" + } + if !isXMLNameStartChar(rune(name[0])) { + name = "_" + name + } + if strings.HasPrefix(strings.ToLower(name), "xml") { + name = "_" + name + } + return name +} + +// sanitizeXMLElementNames sanitizes each name and disambiguates any collisions +// caused by distinct names sanitizing to the same value (e.g. "a/b" and "a b" +// both becoming "a_b"), so no two elements in the same XML item share a name. +func sanitizeXMLElementNames(names []string) []string { + used := make(map[string]bool, len(names)) + out := make([]string, len(names)) + for i, name := range names { + base := sanitizeXMLElementName(name) + candidate := base + for n := 2; used[candidate]; n++ { + candidate = fmt.Sprintf("%s_%d", base, n) + } + used[candidate] = true + out[i] = candidate + } + return out +} + // isOutputCompatibleType checks if a type can be output func isOutputCompatibleType(t reflect.Type) bool { // Handle pointer types by checking the element type diff --git a/internal/base/output/output.go b/internal/base/output/output.go index 7287bec6..5f54881b 100644 --- a/internal/base/output/output.go +++ b/internal/base/output/output.go @@ -120,6 +120,8 @@ func printXML[T OutputFields](data []T, opts printOptions) error { } } + xmlNames := sanitizeXMLElementNames(jsonNames) + encoder := xml.NewEncoder(os.Stdout) encoder.Indent("", " ") @@ -143,8 +145,8 @@ func printXML[T OutputFields](data []T, opts printOptions) error { return err } - for i, name := range jsonNames { - elemStart := xml.StartElement{Name: xml.Name{Local: name}} + for i := range jsonNames { + elemStart := xml.StartElement{Name: xml.Name{Local: xmlNames[i]}} if err := encoder.EncodeToken(elemStart); err != nil { return err } diff --git a/internal/base/output/output_test.go b/internal/base/output/output_test.go index b85c9d83..83d09f21 100644 --- a/internal/base/output/output_test.go +++ b/internal/base/output/output_test.go @@ -52,6 +52,16 @@ type NoTagStruct struct { Name string } +type IllegalTagStruct struct { + ID int `json:"1id"` + Value string `json:"a/b c"` +} + +type CollidingTagStruct struct { + First string `json:"a/b"` + Second string `json:"a b"` +} + func TestPrintCSV_SimpleStruct(t *testing.T) { data := []SimpleStruct{ {ID: 1, Name: "Item 1", Active: true}, @@ -983,6 +993,82 @@ func TestPrintXML_SpecialCharacters(t *testing.T) { } } +func TestPrintXML_IllegalElementName(t *testing.T) { + data := []IllegalTagStruct{ + {ID: 1, Value: "test"}, + } + + output := CaptureOutput(func() { + err := printXML(data, currentPrintOptions()) + assert.NoError(t, err) + }) + + // json tag "1id" starts with a digit, which is illegal as an XML name-start + // character; json tag "a/b c" contains characters illegal in an XML name. + assert.NotContains(t, output, "<1id>") + assert.NotContains(t, output, "") + + decoder := xml.NewDecoder(strings.NewReader(output)) + for { + _, err := decoder.Token() + if err != nil { + break + } + } +} + +func TestSanitizeXMLElementName(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + {"already valid", "name", "name"}, + {"empty", "", "field"}, + {"leading digit", "1id", "_1id"}, + {"leading underscore", "_id", "_id"}, + {"illegal characters", "a/b c", "a_b_c"}, + {"xml prefix", "xmlns", "_xmlns"}, + {"mixed case xml prefix", "XMLThing", "_XMLThing"}, + {"colon is namespace-significant, treated as illegal", "a:b", "a_b"}, + {"other punctuation", "a@b#c(d)e,f;g=h", "a_b_c_d_e_f_g_h"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sanitizeXMLElementName(tt.input)) + }) + } +} + +func TestSanitizeXMLElementNames_DisambiguatesCollisions(t *testing.T) { + // "a/b" and "a b" both sanitize to "a_b"; the second must be renamed so + // no two elements in the same XML item share a name. + got := sanitizeXMLElementNames([]string{"a/b", "a b", "c"}) + assert.Equal(t, []string{"a_b", "a_b_2", "c"}, got) +} + +func TestPrintXML_CollidingElementNames(t *testing.T) { + data := []CollidingTagStruct{ + {First: "one", Second: "two"}, + } + + output := CaptureOutput(func() { + err := printXML(data, currentPrintOptions()) + assert.NoError(t, err) + }) + + assert.Contains(t, output, "one") + assert.Contains(t, output, "two") + + decoder := xml.NewDecoder(strings.NewReader(output)) + for { + _, err := decoder.Token() + if err != nil { + break + } + } +} + func TestPrintOutput_XMLFormat(t *testing.T) { data := []SimpleStruct{ {ID: 1, Name: "Test", Active: true}, diff --git a/internal/base/output/output_wasm.go b/internal/base/output/output_wasm.go index 88af4b34..5b0ec4db 100644 --- a/internal/base/output/output_wasm.go +++ b/internal/base/output/output_wasm.go @@ -289,6 +289,12 @@ func printXML[T OutputFields](data []T, opts printOptions) error { fields = filtered } + fieldNames := make([]string, len(fields)) + for i, f := range fields { + fieldNames[i] = f.name + } + xmlNames := sanitizeXMLElementNames(fieldNames) + encoder := xml.NewEncoder(WasmXMLWriter) encoder.Indent("", " ") @@ -318,11 +324,11 @@ func printXML[T OutputFields](data []T, opts printOptions) error { return err } - for _, f := range fields { + for i, f := range fields { fieldVal := v.Field(f.index) valueStr := formatFieldValue(fieldVal) - elemStart := xml.StartElement{Name: xml.Name{Local: f.name}} + elemStart := xml.StartElement{Name: xml.Name{Local: xmlNames[i]}} if err := encoder.EncodeToken(elemStart); err != nil { return err } From 9cc07289b95ef01044477a50e21a75d43fe345b9 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 14:16:26 -0700 Subject: [PATCH 08/19] fix: address Copilot review feedback on PR #537 - quote the offending value in ParseInt's validation error so an empty or whitespace-only input isn't ambiguous - wrap the --env partial-credential guard in a typed usage error instead of a plain error - assert io.EOF specifically when checking XML output is well-formed, so a malformed document with an early syntax error can't slip past the same as a valid one - add exit-code assertions to the partial-env-var login tests to cover the new typed usage error --- internal/base/output/output_test.go | 57 ++++++++++---------------- internal/commands/config/login.go | 3 +- internal/commands/config/login_test.go | 10 +++++ internal/validation/parse.go | 5 ++- 4 files changed, 38 insertions(+), 37 deletions(-) diff --git a/internal/base/output/output_test.go b/internal/base/output/output_test.go index 83d09f21..6bef2dcc 100644 --- a/internal/base/output/output_test.go +++ b/internal/base/output/output_test.go @@ -7,6 +7,7 @@ import ( "encoding/xml" "errors" "fmt" + "io" "os" "reflect" "regexp" @@ -880,6 +881,21 @@ func TestPrintXML_NilSlice(t *testing.T) { }) } +// assertValidXML decodes output in full and requires it to end in io.EOF, so +// a malformed document (which would otherwise stop the decoder early with a +// syntax error) fails the assertion instead of silently passing. +func assertValidXML(t *testing.T, output string) { + t.Helper() + decoder := xml.NewDecoder(strings.NewReader(output)) + for { + _, err := decoder.Token() + if err != nil { + assert.ErrorIs(t, err, io.EOF, "output is not well-formed XML") + return + } + } +} + func TestPrintXML_ComplexStruct(t *testing.T) { now := time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC) ref := &SimpleStruct{ID: 100, Name: "Referenced", Active: true} @@ -908,13 +924,7 @@ func TestPrintXML_ComplexStruct(t *testing.T) { assert.Contains(t, output, "") // Verify it's parseable XML - decoder := xml.NewDecoder(strings.NewReader(output)) - for { - _, err := decoder.Token() - if err != nil { - break - } - } + assertValidXML(t, output) } func TestPrintXML_PointerStruct(t *testing.T) { @@ -984,13 +994,7 @@ func TestPrintXML_SpecialCharacters(t *testing.T) { assert.Contains(t, output, "&") // Should still be parseable - decoder := xml.NewDecoder(strings.NewReader(output)) - for { - _, err := decoder.Token() - if err != nil { - break - } - } + assertValidXML(t, output) } func TestPrintXML_IllegalElementName(t *testing.T) { @@ -1008,13 +1012,7 @@ func TestPrintXML_IllegalElementName(t *testing.T) { assert.NotContains(t, output, "<1id>") assert.NotContains(t, output, "") - decoder := xml.NewDecoder(strings.NewReader(output)) - for { - _, err := decoder.Token() - if err != nil { - break - } - } + assertValidXML(t, output) } func TestSanitizeXMLElementName(t *testing.T) { @@ -1060,13 +1058,7 @@ func TestPrintXML_CollidingElementNames(t *testing.T) { assert.Contains(t, output, "one") assert.Contains(t, output, "two") - decoder := xml.NewDecoder(strings.NewReader(output)) - for { - _, err := decoder.Token() - if err != nil { - break - } - } + assertValidXML(t, output) } func TestPrintOutput_XMLFormat(t *testing.T) { @@ -1086,13 +1078,7 @@ func TestPrintOutput_XMLFormat(t *testing.T) { assert.Contains(t, output, "") // Verify parseable by xml.Decoder - decoder := xml.NewDecoder(strings.NewReader(output)) - for { - _, err := decoder.Token() - if err != nil { - break - } - } + assertValidXML(t, output) } func TestPrintXML_Parseable(t *testing.T) { @@ -1113,6 +1099,7 @@ func TestPrintXML_Parseable(t *testing.T) { for { tok, err := decoder.Token() if err != nil { + assert.ErrorIs(t, err, io.EOF, "output is not well-formed XML") break } if se, ok := tok.(xml.StartElement); ok && se.Name.Local == "item" { diff --git a/internal/commands/config/login.go b/internal/commands/config/login.go index 3cacd632..f77ad360 100644 --- a/internal/commands/config/login.go +++ b/internal/commands/config/login.go @@ -13,6 +13,7 @@ import ( "sync" "time" + "github.com/megaport/megaport-cli/internal/base/exitcodes" "github.com/megaport/megaport-cli/internal/base/output" "github.com/megaport/megaport-cli/internal/utils" megaport "github.com/megaport/megaportgo" @@ -174,7 +175,7 @@ var loginFuncWithOutput = func(ctx context.Context, outputFormat string) (*megap } } default: - return nil, fmt.Errorf("only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set; with --env, both must come from the environment or neither should be set") + return nil, exitcodes.NewUsageError(fmt.Errorf("only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set; with --env, both must come from the environment or neither should be set")) } } else { // No --env flag, use original priority: profile > env vars diff --git a/internal/commands/config/login_test.go b/internal/commands/config/login_test.go index 2dd71f47..9e088026 100644 --- a/internal/commands/config/login_test.go +++ b/internal/commands/config/login_test.go @@ -5,6 +5,7 @@ package config import ( "bytes" "context" + "errors" "fmt" "io" "log/slog" @@ -15,6 +16,7 @@ import ( "testing" "time" + "github.com/megaport/megaport-cli/internal/base/exitcodes" "github.com/megaport/megaport-cli/internal/utils" megaport "github.com/megaport/megaportgo" "github.com/stretchr/testify/assert" @@ -383,6 +385,10 @@ func TestEnvFlagPartialEnvVarsDoesNotMixWithProfile(t *testing.T) { _, err := LoginWithOutput(context.Background(), "json") assert.Error(t, err) assert.Contains(t, err.Error(), "only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set") + + var cliErr *exitcodes.CLIError + assert.True(t, errors.As(err, &cliErr), "expected a typed usage error") + assert.Equal(t, exitcodes.Usage, cliErr.Code) }) t.Run("only secret key set in env errors instead of mixing with profile", func(t *testing.T) { @@ -392,6 +398,10 @@ func TestEnvFlagPartialEnvVarsDoesNotMixWithProfile(t *testing.T) { _, err := LoginWithOutput(context.Background(), "json") assert.Error(t, err) assert.Contains(t, err.Error(), "only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set") + + var cliErr *exitcodes.CLIError + assert.True(t, errors.As(err, &cliErr), "expected a typed usage error") + assert.Equal(t, exitcodes.Usage, cliErr.Code) }) t.Run("neither env var set falls back fully to profile", func(t *testing.T) { diff --git a/internal/validation/parse.go b/internal/validation/parse.go index 7ef24b5e..5c17fcc3 100644 --- a/internal/validation/parse.go +++ b/internal/validation/parse.go @@ -1,6 +1,7 @@ package validation import ( + "fmt" "strconv" ) @@ -11,7 +12,9 @@ import ( func ParseInt(field, value string) (int, error) { n, err := strconv.Atoi(value) if err != nil { - return 0, NewValidationError(field, value, "is not a valid whole number") + // Quoted so an empty or whitespace-only value renders unambiguously + // in the error message (e.g. "" rather than a blank gap). + return 0, NewValidationError(field, fmt.Sprintf("%q", value), "is not a valid whole number") } return n, nil } From b43df3751e2802ac9629e084b5143408975077f9 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 17:22:57 -0700 Subject: [PATCH 09/19] fix: catch short writes when saving config to temp file os.File.Write can return n < len(data) with a nil error; check the byte count so a short write doesn't get silently renamed into place as the new config file. --- internal/commands/config/manager.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/commands/config/manager.go b/internal/commands/config/manager.go index 19ed4567..ab7a9577 100644 --- a/internal/commands/config/manager.go +++ b/internal/commands/config/manager.go @@ -236,10 +236,15 @@ func (m *ConfigManager) Save() error { tmpPath := tmpFile.Name() defer os.Remove(tmpPath) // no-op once the rename below succeeds - if _, err := tmpFile.Write(configData); err != nil { + n, err := tmpFile.Write(configData) + if err != nil { tmpFile.Close() return fmt.Errorf("failed to write temp config file: %w", err) } + if n != len(configData) { + tmpFile.Close() + return fmt.Errorf("failed to write temp config file: short write (%d of %d bytes)", n, len(configData)) + } if err := tmpFile.Close(); err != nil { return fmt.Errorf("failed to close temp config file: %w", err) } From 15ec46eb4e19e1e783e685405ddb83edc52c69a8 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 17:37:16 -0700 Subject: [PATCH 10/19] fix: keep ValidationError.Value raw in ParseInt Pre-formatting the value with %q before handing it to NewValidationError corrupted the typed field: any caller inspecting ValidationError.Value got a quoted/escaped display string instead of the actual offending input. Pass the raw value through, matching every other validator in the package. --- internal/validation/parse.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/validation/parse.go b/internal/validation/parse.go index 5c17fcc3..7ef24b5e 100644 --- a/internal/validation/parse.go +++ b/internal/validation/parse.go @@ -1,7 +1,6 @@ package validation import ( - "fmt" "strconv" ) @@ -12,9 +11,7 @@ import ( func ParseInt(field, value string) (int, error) { n, err := strconv.Atoi(value) if err != nil { - // Quoted so an empty or whitespace-only value renders unambiguously - // in the error message (e.g. "" rather than a blank gap). - return 0, NewValidationError(field, fmt.Sprintf("%q", value), "is not a valid whole number") + return 0, NewValidationError(field, value, "is not a valid whole number") } return n, nil } From 15e1d61b3fb416b058993d24844338af338d5020 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 17:44:46 -0700 Subject: [PATCH 11/19] test: make partial-env-var login test network-independent The "neither env var set" subtest reached the real Authorize() call and relied on whatever network error came back, making it environment-dependent and possibly slow or flaky under CI egress restrictions. Point utils.BaseURL/utils.TokenURL at a local httptest server returning 401 so it fails fast and deterministically while still proving the code path reaches Authorize. --- internal/commands/config/login_test.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/commands/config/login_test.go b/internal/commands/config/login_test.go index 9e088026..1908a965 100644 --- a/internal/commands/config/login_test.go +++ b/internal/commands/config/login_test.go @@ -358,9 +358,13 @@ func TestEnvFlagPartialEnvVarsDoesNotMixWithProfile(t *testing.T) { // Save and restore non-env-var globals originalEnv := utils.Env originalProfileOverride := utils.ProfileOverride + originalBaseURL := utils.BaseURL + originalTokenURL := utils.TokenURL defer func() { utils.Env = originalEnv utils.ProfileOverride = originalProfileOverride + utils.BaseURL = originalBaseURL + utils.TokenURL = originalTokenURL }() tempDir, err := os.MkdirTemp("", "megaport-login-test") @@ -408,10 +412,18 @@ func TestEnvFlagPartialEnvVarsDoesNotMixWithProfile(t *testing.T) { t.Setenv("MEGAPORT_ACCESS_KEY", "") t.Setenv("MEGAPORT_SECRET_KEY", "") + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer ts.Close() + utils.BaseURL = ts.URL + utils.TokenURL = ts.URL + "/oauth2/token" + _, err := LoginWithOutput(context.Background(), "json") assert.Error(t, err) - // Reaches the Authorize call (network error) rather than failing on - // missing credentials or the partial-env-var mixing guard. + // Reaches the Authorize call (which fails against the local test + // server) rather than failing on missing credentials or the + // partial-env-var mixing guard. assert.NotContains(t, err.Error(), "access key not provided") assert.NotContains(t, err.Error(), "only one of") }) From b88ae29d3ea1792f8401d4f9c7bf514d52519d3b Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 18:06:23 -0700 Subject: [PATCH 12/19] fix(validation): quote empty or whitespace-only values in ValidationError Keeps ValidationError.Value raw (the caller passes the original string unmodified) but quotes it in Error() only when it is empty or whitespace-only, so the message doesn't read as a bare, ambiguous dash. --- internal/validation/common_test.go | 4 +-- internal/validation/errors.go | 10 +++++++- internal/validation/errors_test.go | 2 +- internal/validation/mcr_test.go | 6 ++--- internal/validation/mve_test.go | 40 +++++++++++++++--------------- internal/validation/port_test.go | 4 +-- internal/validation/vxc_test.go | 28 ++++++++++----------- 7 files changed, 51 insertions(+), 43 deletions(-) diff --git a/internal/validation/common_test.go b/internal/validation/common_test.go index bbab5d47..20e754ed 100644 --- a/internal/validation/common_test.go +++ b/internal/validation/common_test.go @@ -229,7 +229,7 @@ func TestValidateMACAddress(t *testing.T) { {"Valid colon-separated uppercase", "AA:BB:CC:DD:EE:FF", false, ""}, {"Valid colon-separated mixed case", "aA:bB:cC:dD:eE:fF", false, ""}, {"Valid hyphen-separated", "00-11-22-33-44-55", false, ""}, - {"Invalid empty", "", true, "Invalid MAC address: - cannot be empty"}, + {"Invalid empty", "", true, "Invalid MAC address: \"\" - cannot be empty"}, {"Invalid too short", "00:11:22:33:44", true, "Invalid MAC address: 00:11:22:33:44 - must be a valid MAC address (e.g. 00:11:22:33:44:55)"}, {"Invalid too long", "00:11:22:33:44:55:66", true, "Invalid MAC address: 00:11:22:33:44:55:66 - must be a valid MAC address (e.g. 00:11:22:33:44:55)"}, {"Invalid EUI-64", "00:11:22:33:44:55:66:77", true, "Invalid MAC address: 00:11:22:33:44:55:66:77 - must be a 6-byte (EUI-48) MAC address"}, @@ -264,7 +264,7 @@ func TestValidateMVEProductSize(t *testing.T) { {"Valid LARGE", "LARGE", false, ""}, {"Invalid lowercase", "small", true, fmt.Sprintf("Invalid product size: small - must be one of: %v", ValidMVEProductSizes)}, {"Invalid value", "XLARGE", true, fmt.Sprintf("Invalid product size: XLARGE - must be one of: %v", ValidMVEProductSizes)}, - {"Empty value", "", true, fmt.Sprintf("Invalid product size: - must be one of: %v", ValidMVEProductSizes)}, + {"Empty value", "", true, fmt.Sprintf("Invalid product size: \"\" - must be one of: %v", ValidMVEProductSizes)}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/validation/errors.go b/internal/validation/errors.go index 8c69bfa3..248c91f4 100644 --- a/internal/validation/errors.go +++ b/internal/validation/errors.go @@ -1,6 +1,9 @@ package validation -import "fmt" +import ( + "fmt" + "strings" +) // ValidationError represents an error that occurs during validation of user input. // It contains information about the field being validated, its value, and the reason for validation failure. @@ -22,5 +25,10 @@ func NewValidationError(field string, value interface{}, reason string) *Validat // Error implements the error interface for ValidationError. // Returns a formatted string with field name, value, and reason for the validation error. func (e *ValidationError) Error() string { + // An empty or whitespace-only string value renders unambiguously when + // quoted; every other value keeps the plain %v rendering. + if s, ok := e.Value.(string); ok && strings.TrimSpace(s) == "" { + return fmt.Sprintf("Invalid %s: %q - %s", e.Field, s, e.Reason) + } return fmt.Sprintf("Invalid %s: %v - %s", e.Field, e.Value, e.Reason) } diff --git a/internal/validation/errors_test.go b/internal/validation/errors_test.go index a0ff0554..c174a44a 100644 --- a/internal/validation/errors_test.go +++ b/internal/validation/errors_test.go @@ -23,7 +23,7 @@ func TestValidationErrorError(t *testing.T) { }{ {"int value", "rate limit", 0, "must be positive", "Invalid rate limit: 0 - must be positive"}, {"string value", "name", "x", "too short", "Invalid name: x - too short"}, - {"empty string value", "name", "", "cannot be empty", "Invalid name: - cannot be empty"}, + {"empty string value", "name", "", "cannot be empty", "Invalid name: \"\" - cannot be empty"}, {"nil value", "peer ASN", nil, "is required", "Invalid peer ASN: - is required"}, {"bool value", "enabled", true, "bad", "Invalid enabled: true - bad"}, {"slice value", "terms", []int{1, 12}, "bad", "Invalid terms: [1 12] - bad"}, diff --git a/internal/validation/mcr_test.go b/internal/validation/mcr_test.go index 43d86b71..205f980a 100644 --- a/internal/validation/mcr_test.go +++ b/internal/validation/mcr_test.go @@ -68,7 +68,7 @@ func TestValidateMCRRequest(t *testing.T) { LocationID: 100, }, wantErr: true, - errText: "Invalid MCR name: - cannot be empty", // Use ValidationError format + errText: "Invalid MCR name: \"\" - cannot be empty", // Use ValidationError format }, { name: "Invalid term", @@ -229,7 +229,7 @@ func TestValidatePrefixFilterListRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid description: - cannot be empty", + errText: "Invalid description: \"\" - cannot be empty", }, { name: "Invalid address family", @@ -259,7 +259,7 @@ func TestValidatePrefixFilterListRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid address family: - cannot be empty", + errText: "Invalid address family: \"\" - cannot be empty", }, { name: "Empty entries", diff --git a/internal/validation/mve_test.go b/internal/validation/mve_test.go index d83754a8..ed35d421 100644 --- a/internal/validation/mve_test.go +++ b/internal/validation/mve_test.go @@ -30,7 +30,7 @@ func TestValidateMVERequest(t *testing.T) { term: 12, locationID: 123, wantErr: true, - errText: "Invalid MVE name: - cannot be empty", + errText: "Invalid MVE name: \"\" - cannot be empty", }, { name: "Invalid term", @@ -105,7 +105,7 @@ func TestValidateMVEVendor(t *testing.T) { name: "Empty vendor", vendor: "", wantErr: true, - errText: fmt.Sprintf("Invalid MVE vendor: - must be one of: %v", ValidMVEVendors), // Adjusted for empty value check if ValidateStringOneOf handles it + errText: fmt.Sprintf("Invalid MVE vendor: \"\" - must be one of: %v", ValidMVEVendors), // Adjusted for empty value check if ValidateStringOneOf handles it }, } @@ -176,7 +176,7 @@ func TestValidateMVENetworkInterfaces(t *testing.T) { {Description: ""}, }, wantErr: true, - errText: "Invalid network interface 2: - description cannot be empty", // Adjusted expected error + errText: "Invalid network interface 2: \"\" - description cannot be empty", // Adjusted expected error }, } @@ -249,7 +249,7 @@ func TestValidateMVENetworkInterfacesTyped(t *testing.T) { {Description: ""}, }, wantErr: true, - errText: "Invalid network interface 2: - description cannot be empty", + errText: "Invalid network interface 2: \"\" - description cannot be empty", }, } @@ -308,7 +308,7 @@ func TestValidateBuyMVERequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid MVE name: - cannot be empty", + errText: "Invalid MVE name: \"\" - cannot be empty", }, { name: "Missing location", @@ -477,7 +477,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { ProductSize: "MEDIUM", }, wantErr: true, - errText: "Invalid SSH public key: - cannot be empty", + errText: "Invalid SSH public key: \"\" - cannot be empty", }, // aruba - valid { @@ -503,7 +503,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { SystemTag: "test-tag", }, wantErr: true, - errText: "Invalid account name: - cannot be empty", + errText: "Invalid account name: \"\" - cannot be empty", }, // aviatrix - valid { @@ -525,7 +525,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { ProductSize: "MEDIUM", }, wantErr: true, - errText: "Invalid cloud init: - cannot be empty", + errText: "Invalid cloud init: \"\" - cannot be empty", }, // cisco - valid { @@ -551,7 +551,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { ManageLocally: true, }, wantErr: true, - errText: "Invalid admin SSH public key: - cannot be empty", + errText: "Invalid admin SSH public key: \"\" - cannot be empty", }, // fortinet - valid { @@ -577,7 +577,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { SSHPublicKey: "ssh-rsa AAAA...", }, wantErr: true, - errText: "Invalid license data: - cannot be empty", + errText: "Invalid license data: \"\" - cannot be empty", }, // paloalto - valid { @@ -603,7 +603,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { LicenseData: "license-data", }, wantErr: true, - errText: "Invalid admin password / admin password hash: - at least one of admin password or admin password hash must be provided", + errText: "Invalid admin password / admin password hash: \"\" - at least one of admin password or admin password hash must be provided", }, // paloalto - valid with plaintext admin password only { @@ -631,7 +631,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { LicenseData: "license-data", }, wantErr: true, - errText: "Invalid admin password / admin password hash: - only one of admin password or admin password hash may be provided, not both", + errText: "Invalid admin password / admin password hash: \"\" - only one of admin password or admin password hash may be provided, not both", }, // prisma - valid { @@ -655,7 +655,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { SecretKey: "secret-key", }, wantErr: true, - errText: "Invalid ION key: - cannot be empty", + errText: "Invalid ION key: \"\" - cannot be empty", }, // versa - valid { @@ -685,7 +685,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { SerialNumber: "SN123456", }, wantErr: true, - errText: "Invalid director address: - cannot be empty", + errText: "Invalid director address: \"\" - cannot be empty", }, // vmware - valid { @@ -713,7 +713,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { VcoActivationCode: "activation-code", }, wantErr: true, - errText: "Invalid VCO address: - cannot be empty", + errText: "Invalid VCO address: \"\" - cannot be empty", }, // meraki - valid { @@ -735,7 +735,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { ProductSize: "MEDIUM", }, wantErr: true, - errText: "Invalid token: - cannot be empty", + errText: "Invalid token: \"\" - cannot be empty", }, } @@ -784,7 +784,7 @@ func TestValidateMVEVendorConfig(t *testing.T) { // SSHPublicKey missing (zero value "") }, wantErr: true, - errText: "Invalid SSH public key: - cannot be empty", // Adjusted error + errText: "Invalid SSH public key: \"\" - cannot be empty", // Adjusted error }, { name: "Valid Cisco config", @@ -813,7 +813,7 @@ func TestValidateMVEVendorConfig(t *testing.T) { ManageLocally: false, // Requires FMC fields (zero value "") }, wantErr: true, - errText: "Invalid FMC IP address: - cannot be empty when not managing locally", // Adjusted error + errText: "Invalid FMC IP address: \"\" - cannot be empty when not managing locally", // Adjusted error }, { name: "Invalid product size", @@ -839,7 +839,7 @@ func TestValidateMVEVendorConfig(t *testing.T) { ManageLocally: true, // Avoid FMC errors }, wantErr: true, - errText: "Invalid SSH public key: - cannot be empty", // Updated based on actual test failure output + errText: "Invalid SSH public key: \"\" - cannot be empty", // Updated based on actual test failure output }, { name: "Missing image ID (zero value)", // Renamed test @@ -861,7 +861,7 @@ func TestValidateMVEVendorConfig(t *testing.T) { ManageLocally: true, // Avoid FMC errors }, wantErr: true, - errText: fmt.Sprintf("Invalid product size: - must be one of: %v", ValidMVEProductSizes), // Updated error message prefix and adjusted for empty string check + errText: fmt.Sprintf("Invalid product size: \"\" - must be one of: %v", ValidMVEProductSizes), // Updated error message prefix and adjusted for empty string check }, } diff --git a/internal/validation/port_test.go b/internal/validation/port_test.go index a8a18feb..1cc99ccd 100644 --- a/internal/validation/port_test.go +++ b/internal/validation/port_test.go @@ -34,7 +34,7 @@ func TestValidatePortRequest(t *testing.T) { portSpeed: 10000, locationID: 100, wantErr: true, - errText: "Invalid port name: - cannot be empty", + errText: "Invalid port name: \"\" - cannot be empty", }, { name: "Invalid term", @@ -146,7 +146,7 @@ func TestValidateLAGPortRequest(t *testing.T) { Term: 12, }, wantErr: true, - errText: "Invalid port name: - cannot be empty", + errText: "Invalid port name: \"\" - cannot be empty", }, { name: "Invalid port speed for LAG", diff --git a/internal/validation/vxc_test.go b/internal/validation/vxc_test.go index d3220fd6..a38c4e09 100644 --- a/internal/validation/vxc_test.go +++ b/internal/validation/vxc_test.go @@ -345,7 +345,7 @@ func TestValidateIPsecTunnelConfig(t *testing.T) { PreSharedKey: "psk", }, wantErr: true, - errText: "Invalid vRouter interface [0] IPsec tunnel source IP address: - cannot be empty", + errText: "Invalid vRouter interface [0] IPsec tunnel source IP address: \"\" - cannot be empty", }, { name: "Source IP not IPv4", @@ -364,7 +364,7 @@ func TestValidateIPsecTunnelConfig(t *testing.T) { PreSharedKey: "psk", }, wantErr: true, - errText: "Invalid vRouter interface [0] IPsec tunnel destination IP address: - cannot be empty", + errText: "Invalid vRouter interface [0] IPsec tunnel destination IP address: \"\" - cannot be empty", }, { name: "Missing pre-shared key", @@ -683,7 +683,7 @@ func TestValidateVXCRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid VXC name: - cannot be empty", + errText: "Invalid VXC name: \"\" - cannot be empty", }, { name: "Invalid term", @@ -725,7 +725,7 @@ func TestValidateVXCRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid A-End UID (PortUID): - cannot be empty", + errText: "Invalid A-End UID (PortUID): \"\" - cannot be empty", }, { name: "Empty B-End UID without partner config", @@ -739,7 +739,7 @@ func TestValidateVXCRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid B-End UID: - cannot be empty when no partner configuration is provided", + errText: "Invalid B-End UID: \"\" - cannot be empty when no partner configuration is provided", }, } @@ -818,7 +818,7 @@ func TestValidateAWSPartnerConfig(t *testing.T) { ownerAccount: "123456789012", asn: 65000, wantErr: true, - errText: "Invalid AWS connect type: - cannot be empty", + errText: "Invalid AWS connect type: \"\" - cannot be empty", }, { connectType: "INVALID", @@ -833,7 +833,7 @@ func TestValidateAWSPartnerConfig(t *testing.T) { ownerAccount: "", asn: 65000, wantErr: true, - errText: "Invalid AWS owner account: - cannot be empty", + errText: "Invalid AWS owner account: \"\" - cannot be empty", }, { name: "Invalid customer IP CIDR", @@ -951,7 +951,7 @@ func TestValidateGooglePartnerConfig(t *testing.T) { errText string }{ {"Valid Google config", "google-pairing-key", false, ""}, - {"Empty pairing key", "", true, "Invalid Google pairing key: - cannot be empty"}, + {"Empty pairing key", "", true, "Invalid Google pairing key: \"\" - cannot be empty"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -979,7 +979,7 @@ func TestValidateOraclePartnerConfig(t *testing.T) { errText string }{ {"Valid Oracle config", "ocid1.virtualcircuit.oc1..example", false, ""}, - {"Empty virtual circuit ID", "", true, "Invalid Oracle virtual circuit ID: - cannot be empty"}, + {"Empty virtual circuit ID", "", true, "Invalid Oracle virtual circuit ID: \"\" - cannot be empty"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1025,7 +1025,7 @@ func TestValidateIBMPartnerConfig(t *testing.T) { name: "Empty account ID", accountID: "", wantErr: true, - errText: "Invalid IBM account ID: - cannot be empty", + errText: "Invalid IBM account ID: \"\" - cannot be empty", }, { name: "Account ID too short", @@ -1179,7 +1179,7 @@ func TestValidateVXCPartnerConfig(t *testing.T) { name: "Missing config details (handled by specific validators)", config: &megaport.VXCPartnerConfigAWS{}, // Empty AWS config wantErr: true, - errText: "Invalid AWS connect type: - cannot be empty", // Error from ValidateAWSPartnerConfig + errText: "Invalid AWS connect type: \"\" - cannot be empty", // Error from ValidateAWSPartnerConfig }, { name: "Invalid AWS config details", @@ -1188,7 +1188,7 @@ func TestValidateVXCPartnerConfig(t *testing.T) { ASN: 65000, }, wantErr: true, - errText: "Invalid AWS connect type: - cannot be empty", + errText: "Invalid AWS connect type: \"\" - cannot be empty", }, { name: "Invalid Azure config details", @@ -1196,7 +1196,7 @@ func TestValidateVXCPartnerConfig(t *testing.T) { ServiceKey: "", // Invalid service key }, wantErr: true, - errText: "Invalid Azure service key: - cannot be empty", + errText: "Invalid Azure service key: \"\" - cannot be empty", }, { name: "Invalid vRouter config details", @@ -1339,7 +1339,7 @@ func TestValidateAzurePartnerConfig(t *testing.T) { ServiceKey: "", }, wantErr: true, - errText: "Invalid Azure service key: - cannot be empty", + errText: "Invalid Azure service key: \"\" - cannot be empty", }, { name: "Nil config", From 88a8c2dc8b1d236008933ad0380f8a0d6a6767d5 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 18:06:28 -0700 Subject: [PATCH 13/19] docs(cli): correct isCobraUsageError comment on RunE error wrapping The comment claimed every RunE in the codebase is wrapped by utils.Wrap*. The cmdbuilder-injected docs subcommand and the --generate-skeleton bypass are unwrapped exceptions; note them and why the substring match is still safe. --- cmd/megaport/megaport.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/cmd/megaport/megaport.go b/cmd/megaport/megaport.go index 7728a630..91b7a68e 100644 --- a/cmd/megaport/megaport.go +++ b/cmd/megaport/megaport.go @@ -337,10 +337,13 @@ func exitCodeFromError(err error) int { // "arg(s)" (from the per-command Args validator) run before // PersistentPreRunE and have no equivalent hook, so they still rely on this // match. Matching here on any of these patterns is safe because every RunE -// in this codebase is wrapped by utils.Wrap*, which always converts its own -// return value to a typed *exitcodes.CLIError before it reaches cobra - so a -// plain, untyped error surfacing all the way to exitCodeFromError can only -// have originated from cobra itself, never from application or API text. +// built from WithRunFunc is wrapped by utils.Wrap*, which always converts its +// return value to a typed *exitcodes.CLIError before it reaches cobra. The +// cmdbuilder-injected `docs` subcommand and the `--generate-skeleton` bypass +// are the two exceptions: they return plain errors (a missing doc file, a +// failed stdout write), but neither can produce text that coincidentally +// matches a cobra usage pattern, so they still fall through safely to +// exitcodes.General below rather than being misclassified as a usage error. func isCobraUsageError(msg string) bool { cobraPatterns := []string{ "unknown command", From 351d7264379c96c044319e20c485a37a8634c135 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 18:06:32 -0700 Subject: [PATCH 14/19] refactor: extract shared --yes/--json-mode purchase guard BuyPort, BuyLAGPort, BuyIX, BuyMCR, BuyMVE, and BuyVXC each repeated the same --yes check for JSON-mode purchases. Extract it into utils.RequireYesForJSONBuy. --- internal/commands/ix/ix_actions.go | 8 +++----- internal/commands/mcr/mcr_actions.go | 8 +++----- internal/commands/mve/mve_actions.go | 8 +++----- internal/commands/ports/ports_actions.go | 16 ++++++---------- internal/commands/vxc/vxc_actions.go | 8 +++----- internal/utils/utils.go | 14 ++++++++++++++ 6 files changed, 32 insertions(+), 30 deletions(-) diff --git a/internal/commands/ix/ix_actions.go b/internal/commands/ix/ix_actions.go index 5a15f12f..e9007c97 100644 --- a/internal/commands/ix/ix_actions.go +++ b/internal/commands/ix/ix_actions.go @@ -198,11 +198,9 @@ func BuyIX(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && (jsonStr != "" || jsonFile != "") { - return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err } // Flag read errors are intentionally ignored — flags are registered by the command builder. diff --git a/internal/commands/mcr/mcr_actions.go b/internal/commands/mcr/mcr_actions.go index 4767fcc6..ab165513 100644 --- a/internal/commands/mcr/mcr_actions.go +++ b/internal/commands/mcr/mcr_actions.go @@ -80,11 +80,9 @@ func BuyMCR(cmd *cobra.Command, args []string, noColor bool) error { }) } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && (jsonStr != "" || jsonFile != "") { - return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err } // Flag read errors are intentionally ignored — flags are registered by the command builder. diff --git a/internal/commands/mve/mve_actions.go b/internal/commands/mve/mve_actions.go index 6a345e35..6d6cc697 100644 --- a/internal/commands/mve/mve_actions.go +++ b/internal/commands/mve/mve_actions.go @@ -141,11 +141,9 @@ func BuyMVE(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && (jsonStr != "" || jsonFile != "") { - return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err } client, err := config.Login(ctx) diff --git a/internal/commands/ports/ports_actions.go b/internal/commands/ports/ports_actions.go index 1b04b8de..5b09f843 100644 --- a/internal/commands/ports/ports_actions.go +++ b/internal/commands/ports/ports_actions.go @@ -73,11 +73,9 @@ func BuyPort(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && (jsonStr != "" || jsonFile != "") { - return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err } // Flag read errors are intentionally ignored — flags are registered by the command builder. @@ -226,11 +224,9 @@ func BuyLAGPort(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && (jsonStr != "" || jsonFile != "") { - return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err } noWait, _ := cmd.Flags().GetBool("no-wait") diff --git a/internal/commands/vxc/vxc_actions.go b/internal/commands/vxc/vxc_actions.go index 3da1205c..01027453 100644 --- a/internal/commands/vxc/vxc_actions.go +++ b/internal/commands/vxc/vxc_actions.go @@ -276,11 +276,9 @@ func BuyVXC(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && (jsonStr != "" || jsonFile != "") { - return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err } noWait, _ := cmd.Flags().GetBool("no-wait") diff --git a/internal/utils/utils.go b/internal/utils/utils.go index c4b0b922..8f774ec7 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -323,6 +323,20 @@ func WrapOutputFormatRunE(fn func(cmd *cobra.Command, args []string, noColor boo } } +// RequireYesForJSONBuy enforces that a JSON-mode purchase (--json or +// --json-file) also passes --yes, since JSON mode has no interactive +// confirmation prompt to fall back on. Returns the resolved --yes value so +// callers can reuse it for their own confirmation-prompt branch. +func RequireYesForJSONBuy(cmd *cobra.Command) (bool, error) { + jsonStr, _ := cmd.Flags().GetString("json") + jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") + if !yes && (jsonStr != "" || jsonFile != "") { + return false, exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + } + return yes, nil +} + // classifyError inspects an error message to determine the appropriate exit code. func classifyError(err error) int { // Preserve exit codes already set by action functions From 4cd6ddc42ee809e08aa940d148ac8b9f64a7ddc9 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 18:18:14 -0700 Subject: [PATCH 15/19] test(config): skip POSIX-permission tests on Windows TestReadOnlyConfigFile and TestSaveRetightensOverPermissiveFile rely on chmod semantics and permission-denied error text that don't hold on Windows, so skip them there rather than let them fail or be meaningless for local Windows contributors. --- internal/commands/config/manager_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/commands/config/manager_test.go b/internal/commands/config/manager_test.go index af17601e..3792a044 100644 --- a/internal/commands/config/manager_test.go +++ b/internal/commands/config/manager_test.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "testing" "github.com/megaport/megaport-cli/internal/utils" @@ -641,6 +642,9 @@ func TestConfigVersionHandling(t *testing.T) { assert.Equal(t, ConfigVersion, manager.config.Version, "Config should be upgraded to current version") } func TestReadOnlyConfigFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("os.Chmod doesn't reliably block directory writes on Windows, and error text differs") + } if os.Geteuid() == 0 { t.Skip("Skipping test when running as root") } @@ -683,6 +687,9 @@ func TestReadOnlyConfigFile(t *testing.T) { } func TestSaveRetightensOverPermissiveFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } if os.Geteuid() == 0 { t.Skip("Skipping test when running as root") } From ffb5a2beb33c5123cd746f4a5675c43cc7d873c6 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 18:28:25 -0700 Subject: [PATCH 16/19] test(cli): isolate TestMissingRequiredFlagIsTypedUsageError from host state This test runs the real rootCmd through PersistentPreRunE, which reads the config dir and mutates shared output config globals. Point it at a temp config dir and reset output state afterward, matching the sibling PersistentPreRunE tests in this file. --- cmd/megaport/megaport_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/megaport/megaport_test.go b/cmd/megaport/megaport_test.go index 726bb6fa..7f5e3053 100644 --- a/cmd/megaport/megaport_test.go +++ b/cmd/megaport/megaport_test.go @@ -374,6 +374,9 @@ func TestUnknownFlagIsTypedUsageError(t *testing.T) { // missing-required-flag error as a typed *exitcodes.CLIError ahead of // cobra's own (redundant) ValidateRequiredFlags call. func TestMissingRequiredFlagIsTypedUsageError(t *testing.T) { + t.Setenv("MEGAPORT_CONFIG_DIR", t.TempDir()) + defer output.ResetState() + rootCmd.SetArgs([]string{"billing-market", "set"}) var execErr error _ = output.CaptureOutput(func() { From 2259229b8e2c787cc6d64b18d6432901c797980d Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Mon, 13 Jul 2026 07:28:25 -0700 Subject: [PATCH 17/19] docs(cli): document --yes requirement for JSON-mode buy/create Flag help text and generated docs for every buy/create command didn't mention that --yes is required in JSON mode, even after the enforcement went in. Update the shared --yes flag description and NAT Gateway's create flag, then regenerate docs/. --- docs/index.md | 2 +- docs/megaport-cli_ix_buy.md | 2 +- docs/megaport-cli_mcr_buy.md | 2 +- docs/megaport-cli_mve_buy.md | 2 +- docs/megaport-cli_nat-gateway_create.md | 2 +- docs/megaport-cli_ports_buy-lag.md | 2 +- docs/megaport-cli_ports_buy.md | 2 +- docs/megaport-cli_vxc_buy.md | 2 +- internal/base/cmdbuilder/flagsets.go | 2 +- internal/commands/nat_gateway/nat_gateway.go | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/index.md b/docs/index.md index 365a8681..344a9180 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1,6 +1,6 @@ # Megaport CLI Documentation -> Generated on July 8, 2026 for version v1.0.0-beta.1 +> Generated on July 13, 2026 for version v1.0.0-beta.1 ## Available Commands diff --git a/docs/megaport-cli_ix_buy.md b/docs/megaport-cli_ix_buy.md index 7c63dfe7..61ee6a5a 100644 --- a/docs/megaport-cli_ix_buy.md +++ b/docs/megaport-cli_ix_buy.md @@ -77,5 +77,5 @@ megaport-cli ix buy [flags] | `--rate-limit` | | `0` | Rate limit in Mbps | true | | `--shutdown` | | `false` | Whether the IX is initially shut down | false | | `--vlan` | | `0` | VLAN ID for the IX connection | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_mcr_buy.md b/docs/megaport-cli_mcr_buy.md index 8c21d630..4878e2ae 100644 --- a/docs/megaport-cli_mcr_buy.md +++ b/docs/megaport-cli_mcr_buy.md @@ -96,5 +96,5 @@ megaport-cli mcr buy [flags] | `--resource-tags` | | | Resource tags as a JSON string (e.g. {"key1":"value1","key2":"value2"}) | false | | `--resource-tags-file` | | | Path to JSON file containing resource tags | false | | `--term` | | `0` | The term of the MCR (1, 12, 24, 36, 48, or 60 months) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_mve_buy.md b/docs/megaport-cli_mve_buy.md index 71d9e205..3a9a26c4 100644 --- a/docs/megaport-cli_mve_buy.md +++ b/docs/megaport-cli_mve_buy.md @@ -100,5 +100,5 @@ megaport-cli mve buy [flags] | `--term` | | `0` | The term of the MVE (1, 12, 24, 36, 48, or 60 months) | true | | `--vendor-config` | | | JSON string with vendor-specific configuration (for flag mode) | true | | `--vnics` | | | JSON array of network interfaces (for flag mode) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_nat-gateway_create.md b/docs/megaport-cli_nat-gateway_create.md index 38b9fbf5..8fef4801 100644 --- a/docs/megaport-cli_nat-gateway_create.md +++ b/docs/megaport-cli_nat-gateway_create.md @@ -84,5 +84,5 @@ megaport-cli nat-gateway create [flags] | `--session-count` | | `0` | The number of NAT sessions (optional) | false | | `--speed` | | `0` | The speed of the NAT Gateway in Mbps | true | | `--term` | | `0` | The contract term in months (1, 12, 24, 36, 48, or 60) | true | -| `--yes` | `-y` | `false` | Skip the confirmation prompt for creating the NAT Gateway design (no charges are incurred until 'nat-gateway buy') | false | +| `--yes` | `-y` | `false` | Skip the confirmation prompt for creating the NAT Gateway design (no charges are incurred until 'nat-gateway buy'); required when using --json or --json-file | false | diff --git a/docs/megaport-cli_ports_buy-lag.md b/docs/megaport-cli_ports_buy-lag.md index 9f229113..eeaadc8e 100644 --- a/docs/megaport-cli_ports_buy-lag.md +++ b/docs/megaport-cli_ports_buy-lag.md @@ -85,5 +85,5 @@ megaport-cli ports buy-lag [flags] | `--resource-tags` | | | Resource tags as a JSON string (e.g. {"key1":"value1","key2":"value2"}) | false | | `--resource-tags-file` | | | Path to JSON file containing resource tags | false | | `--term` | | `0` | The term of the port (1, 12, or 24 months) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_ports_buy.md b/docs/megaport-cli_ports_buy.md index f6f9f597..d9780e54 100644 --- a/docs/megaport-cli_ports_buy.md +++ b/docs/megaport-cli_ports_buy.md @@ -84,5 +84,5 @@ megaport-cli ports buy [flags] | `--resource-tags` | | | Resource tags as a JSON string (e.g. {"key1":"value1","key2":"value2"}) | false | | `--resource-tags-file` | | | Path to JSON file containing resource tags | false | | `--term` | | `0` | The term of the port (1, 12, 24, 36, 48, or 60 months) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_vxc_buy.md b/docs/megaport-cli_vxc_buy.md index 2a131a53..091c25b7 100644 --- a/docs/megaport-cli_vxc_buy.md +++ b/docs/megaport-cli_vxc_buy.md @@ -95,5 +95,5 @@ megaport-cli vxc buy [flags] | `--resource-tags-file` | | | Path to JSON file containing resource tags | false | | `--service-key` | | | Service key | false | | `--term` | | `0` | Contract term in months (1, 12, 24, 36, 48, or 60) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/internal/base/cmdbuilder/flagsets.go b/internal/base/cmdbuilder/flagsets.go index d88aa3fc..02e5ff3b 100644 --- a/internal/base/cmdbuilder/flagsets.go +++ b/internal/base/cmdbuilder/flagsets.go @@ -81,7 +81,7 @@ func (b *CommandBuilder) WithDeferredDeleteFlags() *CommandBuilder { // WithBuyConfirmFlags adds the --yes/-y flag to skip buy confirmation prompts func (b *CommandBuilder) WithBuyConfirmFlags() *CommandBuilder { - b.WithBoolFlagP("yes", "y", false, "Skip confirmation prompt for purchase") + b.WithBoolFlagP("yes", "y", false, "Skip confirmation prompt for purchase; required when using --json or --json-file") return b } diff --git a/internal/commands/nat_gateway/nat_gateway.go b/internal/commands/nat_gateway/nat_gateway.go index 94acbd86..ded8f25d 100644 --- a/internal/commands/nat_gateway/nat_gateway.go +++ b/internal/commands/nat_gateway/nat_gateway.go @@ -63,7 +63,7 @@ func buildNATGatewayCommands(rootCmd *cobra.Command) (get, list, create, update, create = cmdbuilder.NewCommand("create", "Create a new NAT Gateway"). WithColorAwareRunFunc(CreateNATGateway). - WithBoolFlagP("yes", "y", false, "Skip the confirmation prompt for creating the NAT Gateway design (no charges are incurred until 'nat-gateway buy')"). + WithBoolFlagP("yes", "y", false, "Skip the confirmation prompt for creating the NAT Gateway design (no charges are incurred until 'nat-gateway buy'); required when using --json or --json-file"). WithNATGatewayCreateFlags(). WithStandardInputFlags(). WithLongDesc("Create a new NAT Gateway through the Megaport API.\n\nThis command creates a NAT Gateway by providing the necessary details."). From 96e51c19b1026aea3cb41712b23b0121e1a91a38 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Wed, 15 Jul 2026 13:57:51 -0700 Subject: [PATCH 18/19] test: cover the JSON-mode --yes gate and atomic config save Codecov flagged patch coverage below the 80% gate on PR 537. The diff's new error branches and JSON-mode --yes enforcement paths had no tests: RequireYesForJSONBuy, ConfigManager.Save's temp-file/rename/ chmod failure paths, FindDocFile's embedded-temp-file and not-found paths, and the untested confirm-prompt/--yes branches in nat-gateway create and LAG port buy. --- internal/base/cmdbuilder/docs/megaport-cli.md | 3 + .../cmdbuilder/docs_render_native_test.go | 54 ++++++++++++++++++ internal/commands/config/manager_test.go | 57 +++++++++++++++++++ .../commands/nat_gateway/nat_gateway_test.go | 47 +++++++++++++++ internal/commands/ports/ports_actions_test.go | 28 +++++++++ internal/utils/utils_test.go | 50 ++++++++++++++++ 6 files changed, 239 insertions(+) create mode 100644 internal/base/cmdbuilder/docs/megaport-cli.md diff --git a/internal/base/cmdbuilder/docs/megaport-cli.md b/internal/base/cmdbuilder/docs/megaport-cli.md new file mode 100644 index 00000000..4252ad6e --- /dev/null +++ b/internal/base/cmdbuilder/docs/megaport-cli.md @@ -0,0 +1,3 @@ +# Embedded Sample + +Embedded documentation used by docs_render tests. diff --git a/internal/base/cmdbuilder/docs_render_native_test.go b/internal/base/cmdbuilder/docs_render_native_test.go index f35cba44..47e72811 100644 --- a/internal/base/cmdbuilder/docs_render_native_test.go +++ b/internal/base/cmdbuilder/docs_render_native_test.go @@ -12,6 +12,9 @@ import ( "github.com/spf13/cobra" ) +//go:embed docs/megaport-cli.md +var embeddedTestDocsFS embed.FS + // ShowDocumentation must write to the command's configured output writer, not // straight to os.Stdout, so callers (and tests) can capture/redirect docs output. func TestShowDocumentationWritesToCobraWriter(t *testing.T) { @@ -39,3 +42,54 @@ func TestShowDocumentationWritesToCobraWriter(t *testing.T) { t.Fatal("expected rendered docs on the cobra writer, got nothing") } } + +// ShowDocumentation must stage embedded docs in a temp file and clean it up +// afterwards, since embed.FS content can't be read back by path directly. +func TestShowDocumentationEmbeddedTempFile(t *testing.T) { + origFS := embeddedDocsFS + embeddedDocsFS = embeddedTestDocsFS + t.Cleanup(func() { embeddedDocsFS = origFS }) + + root := &cobra.Command{Use: "megaport-cli"} + var buf bytes.Buffer + root.SetOut(&buf) + + docPath, isTemp, err := FindDocFile(root) + if err != nil { + t.Fatalf("FindDocFile: %v", err) + } + if !isTemp { + t.Fatal("expected embedded doc to be staged in a temp file") + } + defer os.Remove(docPath) + + if _, err := os.Stat(docPath); err != nil { + t.Fatalf("expected temp doc file to exist: %v", err) + } + + if err := ShowDocumentation(root); err != nil { + t.Fatalf("ShowDocumentation: %v", err) + } + if buf.Len() == 0 { + t.Fatal("expected rendered docs on the cobra writer, got nothing") + } +} + +func TestFindDocFileNotFound(t *testing.T) { + origFS := embeddedDocsFS + embeddedDocsFS = embed.FS{} // force the on-disk fallback + t.Cleanup(func() { embeddedDocsFS = origFS }) + + origDir := DocsDirectory + DocsDirectory = t.TempDir() // empty, no doc files present + t.Cleanup(func() { DocsDirectory = origDir }) + + root := &cobra.Command{Use: "megaport-cli"} + _, isTemp, err := FindDocFile(root) + if err == nil { + t.Fatal("expected an error for a missing doc file") + } + if isTemp { + t.Fatal("expected isTemp to be false when no doc file is found") + } +} diff --git a/internal/commands/config/manager_test.go b/internal/commands/config/manager_test.go index 3792a044..e8e61c93 100644 --- a/internal/commands/config/manager_test.go +++ b/internal/commands/config/manager_test.go @@ -502,6 +502,63 @@ func TestCorruptedConfigFile_ChmodFailure(t *testing.T) { assert.Empty(t, profiles) } +func TestConfigManagerSave_ChmodTmpFileError(t *testing.T) { + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + + old := chmodFile + defer func() { chmodFile = old }() + chmodFile = func(_ string, _ os.FileMode) error { + return fmt.Errorf("chmod: operation not permitted") + } + + err = manager.Save() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to set permissions on temp config file") +} + +func TestConfigManagerSave_RenameError(t *testing.T) { + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + + old := renameFile + defer func() { renameFile = old }() + renameFile = func(_, _ string) error { + return fmt.Errorf("rename: permission denied") + } + + err = manager.Save() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to write config file") +} + +func TestConfigManagerSave_ChmodFinalError(t *testing.T) { + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + + old := chmodFile + defer func() { chmodFile = old }() + callCount := 0 + chmodFile = func(path string, mode os.FileMode) error { + callCount++ + if callCount == 1 { + // Let the temp file's own chmod succeed; only fail the final one. + return os.Chmod(path, mode) + } + return fmt.Errorf("chmod: operation not permitted") + } + + err = manager.Save() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to set permissions on config file") +} + func TestCorruptedConfigFile_ConcurrentRecovery(t *testing.T) { setupTestConfig(t) diff --git a/internal/commands/nat_gateway/nat_gateway_test.go b/internal/commands/nat_gateway/nat_gateway_test.go index 75066995..f01634cd 100644 --- a/internal/commands/nat_gateway/nat_gateway_test.go +++ b/internal/commands/nat_gateway/nat_gateway_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/megaport/megaport-cli/internal/testutil" + "github.com/megaport/megaport-cli/internal/utils" megaport "github.com/megaport/megaportgo" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -101,6 +102,52 @@ func TestCreateNATGateway_JSONWithoutYes(t *testing.T) { assert.Contains(t, err.Error(), "--yes is required to confirm creating a NAT Gateway design when using --json or --json-file") } +func TestCreateNATGateway_ConfirmationPrompt(t *testing.T) { + mock := &MockNATGatewayService{} + defer setupMockNATGateway(mock)() + + originalDesignConfirmPrompt := utils.GetDesignConfirmPrompt() + defer func() { utils.SetDesignConfirmPrompt(originalDesignConfirmPrompt) }() + + promptCalled := false + utils.SetDesignConfirmPrompt(func(_ string, _ []utils.BuyConfirmDetail, _ bool) bool { + promptCalled = true + return true + }) + + cmd := newTestCmd("create") + require.NoError(t, cmd.Flags().Set("name", "My NAT GW")) + require.NoError(t, cmd.Flags().Set("term", "12")) + require.NoError(t, cmd.Flags().Set("speed", "1000")) + require.NoError(t, cmd.Flags().Set("location-id", "123")) + + err := CreateNATGateway(cmd, nil, true) + assert.NoError(t, err) + assert.True(t, promptCalled) + require.NotNil(t, mock.CapturedCreateReq) + assert.Equal(t, "My NAT GW", mock.CapturedCreateReq.ProductName) +} + +func TestCreateNATGateway_ConfirmationDenied(t *testing.T) { + mock := &MockNATGatewayService{} + defer setupMockNATGateway(mock)() + + originalDesignConfirmPrompt := utils.GetDesignConfirmPrompt() + defer func() { utils.SetDesignConfirmPrompt(originalDesignConfirmPrompt) }() + utils.SetDesignConfirmPrompt(func(_ string, _ []utils.BuyConfirmDetail, _ bool) bool { return false }) + + cmd := newTestCmd("create") + require.NoError(t, cmd.Flags().Set("name", "My NAT GW")) + require.NoError(t, cmd.Flags().Set("term", "12")) + require.NoError(t, cmd.Flags().Set("speed", "1000")) + require.NoError(t, cmd.Flags().Set("location-id", "123")) + + err := CreateNATGateway(cmd, nil, true) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cancelled by user") + assert.Nil(t, mock.CapturedCreateReq) +} + func TestCreateNATGateway_NoInput(t *testing.T) { mock := &MockNATGatewayService{} defer setupMockNATGateway(mock)() diff --git a/internal/commands/ports/ports_actions_test.go b/internal/commands/ports/ports_actions_test.go index 670f8d08..1ee07e63 100644 --- a/internal/commands/ports/ports_actions_test.go +++ b/internal/commands/ports/ports_actions_test.go @@ -2085,6 +2085,34 @@ func TestBuyLAGPort_ConfirmationDenied(t *testing.T) { assert.Contains(t, capturedOutput, "Purchase cancelled") } +func TestBuyLAGPort_JSONWithoutYes(t *testing.T) { + cleanup := testutil.SetupLogin(func(c *megaport.Client) { + c.PortService = &MockPortService{} + }) + defer cleanup() + + cmd := &cobra.Command{Use: "buy-lag"} + cmd.Flags().Bool("interactive", false, "") + cmd.Flags().Bool("no-wait", false, "") + cmd.Flags().Bool("yes", false, "") + cmd.Flags().String("json", "", "") + cmd.Flags().String("json-file", "", "") + cmd.Flags().String("name", "", "") + cmd.Flags().Int("term", 0, "") + cmd.Flags().Int("port-speed", 0, "") + cmd.Flags().Int("location-id", 0, "") + cmd.Flags().Bool("marketplace-visibility", false, "") + cmd.Flags().String("diversity-zone", "", "") + cmd.Flags().Bool("cost-confirm", true, "") + cmd.Flags().Int("lag-count", 0, "") + + require.NoError(t, cmd.Flags().Set("json", `{"name":"json-lag","term":12,"portSpeed":10000,"locationId":1,"lagCount":2,"marketPlaceVisibility":true}`)) + + err := BuyLAGPort(cmd, nil, true) + assert.Error(t, err) + assert.Contains(t, err.Error(), "--yes is required to confirm a purchase when using --json or --json-file") +} + func TestValidatePort(t *testing.T) { cleanup := testutil.SetupLogin(func(c *megaport.Client) {}) defer cleanup() diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go index f3004aa0..f43ebb46 100644 --- a/internal/utils/utils_test.go +++ b/internal/utils/utils_test.go @@ -481,6 +481,56 @@ func TestWrapOutputFormatRunE(t *testing.T) { }) } +func TestRequireYesForJSONBuy(t *testing.T) { + newCmd := func() *cobra.Command { + cmd := &cobra.Command{Use: "buy"} + cmd.Flags().String("json", "", "") + cmd.Flags().String("json-file", "", "") + cmd.Flags().Bool("yes", false, "") + return cmd + } + + t.Run("json without yes is a usage error", func(t *testing.T) { + cmd := newCmd() + require.NoError(t, cmd.Flags().Set("json", `{"name":"test"}`)) + + yes, err := RequireYesForJSONBuy(cmd) + assert.False(t, yes) + require.Error(t, err) + assert.Contains(t, err.Error(), "--yes is required to confirm a purchase when using --json or --json-file") + var cliErr *exitcodes.CLIError + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, exitcodes.Usage, cliErr.Code) + }) + + t.Run("json-file without yes is a usage error", func(t *testing.T) { + cmd := newCmd() + require.NoError(t, cmd.Flags().Set("json-file", "./config.json")) + + yes, err := RequireYesForJSONBuy(cmd) + assert.False(t, yes) + require.Error(t, err) + }) + + t.Run("json with yes succeeds", func(t *testing.T) { + cmd := newCmd() + require.NoError(t, cmd.Flags().Set("json", `{"name":"test"}`)) + require.NoError(t, cmd.Flags().Set("yes", "true")) + + yes, err := RequireYesForJSONBuy(cmd) + assert.True(t, yes) + assert.NoError(t, err) + }) + + t.Run("no json and no yes succeeds, leaving confirmation to the caller", func(t *testing.T) { + cmd := newCmd() + + yes, err := RequireYesForJSONBuy(cmd) + assert.False(t, yes) + assert.NoError(t, err) + }) +} + func TestClassifyError(t *testing.T) { tests := []struct { name string From 51429aa49a9908d3cd1a51ab60432a69437b56d8 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Wed, 22 Jul 2026 05:57:35 -0700 Subject: [PATCH 19/19] docs(cli): trim verbose exit-code and XML-name comments --- cmd/megaport/megaport.go | 21 +++++---------------- internal/base/output/common.go | 10 ++++------ 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/cmd/megaport/megaport.go b/cmd/megaport/megaport.go index 91b7a68e..fdf538e1 100644 --- a/cmd/megaport/megaport.go +++ b/cmd/megaport/megaport.go @@ -328,22 +328,11 @@ func exitCodeFromError(err error) int { return exitcodes.General } -// isCobraUsageError is a defensive fallback for cobra's own error shapes. -// Flag-parse failures and missing-required-flag errors are now tagged as a -// typed usage error at the source (rootCmd.SetFlagErrorFunc and the -// ValidateRequiredFlags check in PersistentPreRunE), so in practice those -// reach exitCodeFromError as a *exitcodes.CLIError and never fall through to -// this substring match. "unknown command" (from cobra.Command.Find) and -// "arg(s)" (from the per-command Args validator) run before -// PersistentPreRunE and have no equivalent hook, so they still rely on this -// match. Matching here on any of these patterns is safe because every RunE -// built from WithRunFunc is wrapped by utils.Wrap*, which always converts its -// return value to a typed *exitcodes.CLIError before it reaches cobra. The -// cmdbuilder-injected `docs` subcommand and the `--generate-skeleton` bypass -// are the two exceptions: they return plain errors (a missing doc file, a -// failed stdout write), but neither can produce text that coincidentally -// matches a cobra usage pattern, so they still fall through safely to -// exitcodes.General below rather than being misclassified as a usage error. +// isCobraUsageError is a defensive fallback matching cobra's usage-error text. +// Flag-parse and missing-required-flag errors are already typed as usage errors +// at the source (SetFlagErrorFunc and the ValidateRequiredFlags check), so the +// matches for those are a backstop for any that arrive untyped; "unknown command" +// and the arg-count validators have no such hook and rely on this match. func isCobraUsageError(msg string) bool { cobraPatterns := []string{ "unknown command", diff --git a/internal/base/output/common.go b/internal/base/output/common.go index 4121f039..0bc5611f 100644 --- a/internal/base/output/common.go +++ b/internal/base/output/common.go @@ -452,17 +452,15 @@ func filterByFields(headers, jsonNames []string, indices []int, selected []strin return outHeaders, outJSONNames, outIndices, nil } -// isXMLNameStartChar reports whether r is legal as the first character of an -// XML element local name. Colon is deliberately excluded even though XML 1.0 -// allows it in a Name: it is namespace-significant, and treating a field name -// like "a:b" as a namespace-qualified name would produce an undeclared-prefix -// error in namespace-aware parsers. +// isXMLNameStartChar reports whether r is legal as the first character of an XML +// element local name. Colon is excluded on purpose: it is namespace-significant, +// so a name like "a:b" would trip an undeclared-prefix error in aware parsers. func isXMLNameStartChar(r rune) bool { return r == '_' || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') } // isXMLNameChar reports whether r is legal as a non-first character of an XML -// element local name (see isXMLNameStartChar for why colon is excluded). +// element local name. func isXMLNameChar(r rune) bool { return isXMLNameStartChar(r) || r == '.' || r == '-' || (r >= '0' && r <= '9') }