From b0df8ba9638738829331bb0cd7a2e708eae67052 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Fri, 22 May 2026 18:54:06 +0200 Subject: [PATCH] fix(setup): correct false-positive migration detection and plan command errors - isXMLMigrated and isResolvConfMigrated now guard against empty hostname (Go's strings.Contains(s, "") is always true, causing any speaker to appear migrated when --service-url has a malformed single-slash scheme) - renderPlanSteps message no longer claims "and paired" when --include-pair=false - validateServiceURL rejects malformed service URLs early with a hint (e.g. "did you mean https://soundtouch.fritz.box?") - Generated plan-step commands move --host before the subcommand name (urfave/cli/v2 requires global flags before the first subcommand token) Co-Authored-By: Claude Sonnet 4.6 --- cmd/soundtouch-cli/cmd_setup.go | 58 ++++++++++++++++--- .../setup/migration_state_telnet_test.go | 44 ++++++++++++++ pkg/service/setup/setup.go | 6 ++ 3 files changed, 100 insertions(+), 8 deletions(-) diff --git a/cmd/soundtouch-cli/cmd_setup.go b/cmd/soundtouch-cli/cmd_setup.go index 2c71ea5..7368915 100644 --- a/cmd/soundtouch-cli/cmd_setup.go +++ b/cmd/soundtouch-cli/cmd_setup.go @@ -549,6 +549,11 @@ func setupInstallCACmd() *cli.Command { cfg := GetClientConfig(c) serviceURL := strings.TrimRight(c.String("service-url"), "/") + if err := validateServiceURL(serviceURL); err != nil { + PrintError(err.Error()) + return err + } + certPEM, err := fetchCACert(serviceURL, c.String("auth")) if err != nil { PrintError(err.Error()) @@ -698,6 +703,11 @@ func setupMigrateCmd() *cli.Command { method := setup.MigrationMethod(c.String("method")) serviceURL := c.String("service-url") + if err := validateServiceURL(serviceURL); err != nil { + PrintError(err.Error()) + return err + } + // For DNS-redirect methods, prove AfterTouch's DNS listener // is alive by sending it a real query — that's the truth, // regardless of what its settings claim. @@ -762,6 +772,24 @@ func preInstallCAForCLI(deviceIP, serviceURL string) error { return nil } +// validateServiceURL returns an error if serviceURL cannot be parsed or has no +// hostname. A common mistake is a single-slash scheme (https:/host instead of +// https://host); the error message hints at the correction in that case. +func validateServiceURL(serviceURL string) error { + parsed, err := url.Parse(serviceURL) + if err != nil { + return fmt.Errorf("invalid --service-url %q: %w", serviceURL, err) + } + if parsed.Hostname() == "" { + hint := "" + if parsed.Scheme != "" && parsed.Opaque != "" { + hint = fmt.Sprintf(" (did you mean %s://%s?)", parsed.Scheme, strings.TrimPrefix(parsed.Opaque, "/")) + } + return fmt.Errorf("invalid --service-url %q: no hostname found%s", serviceURL, hint) + } + return nil +} + // requireAfterTouchDNSReachable sends a real DNS query to AfterTouch's // port-53 listener and confirms it responds. This is the ground-truth // preflight for DNS-redirect migration methods — config inspection (the @@ -822,6 +850,11 @@ func setupVerifyCmd() *cli.Command { cfg := GetClientConfig(c) serviceURL := c.String("service-url") + if err := validateServiceURL(serviceURL); err != nil { + PrintError(err.Error()) + return err + } + m := setup.NewManager(serviceURL, nil, nil) summary, err := m.GetMigrationSummary(cfg.Host, serviceURL, c.String("proxy-url"), nil) @@ -1013,6 +1046,11 @@ func setupPlanCmd() *cli.Command { wifiSSID := c.String("wifi-ssid") includePair := c.Bool("include-pair") + if err := validateServiceURL(serviceURL); err != nil { + PrintError(err.Error()) + return err + } + m := setup.NewManager(serviceURL, nil, nil) fmt.Printf("Probing %s …\n\n", cfg.Host) @@ -1036,7 +1074,7 @@ func setupPlanCmd() *cli.Command { renderPreResetNote() } - renderPlanSteps(steps) + renderPlanSteps(steps, includePair) return nil }, @@ -1146,7 +1184,7 @@ func buildPlanSteps( if includePair && (reset || (summary != nil && !summary.IsPaired)) { steps = append(steps, planStep{ title: "Pair the device with an AfterTouch account", - cmd: fmt.Sprintf("soundtouch-cli setup pair --host=%s --service-url=%s", host, serviceURL), + cmd: fmt.Sprintf("soundtouch-cli --host=%s setup pair --service-url=%s", host, serviceURL), reason: "Required for preset persistence, streaming services, multi-room zones.", }) } @@ -1178,7 +1216,7 @@ func resetSteps(host, wifiSSID string, inspect *setup.InspectReport) []planStep return []planStep{ { title: "Factory-reset the speaker", - cmd: fmt.Sprintf("soundtouch-cli setup factory-reset --host=%s", host), + cmd: fmt.Sprintf("soundtouch-cli --host=%s setup factory-reset", host), reason: "Wipes account pairing, presets, Wi-Fi — gives a clean baseline for the SETUP state machine.", }, { @@ -1236,20 +1274,20 @@ func migrationSteps(host, serviceURL string, summary *setup.MigrationSummary, re if dnsRedirect && summary != nil && !summary.CACertTrusted { steps = append(steps, planStep{ title: "Install AfterTouch's CA cert on the speaker", - cmd: fmt.Sprintf("soundtouch-cli setup install-ca --host=%s --service-url=%s", host, serviceURL), + cmd: fmt.Sprintf("soundtouch-cli --host=%s setup install-ca --service-url=%s", host, serviceURL), reason: "DNS-redirect methods keep using https://*.bose.com URLs — the device needs to trust AfterTouch's cert.", }) } steps = append(steps, planStep{ title: fmt.Sprintf("Apply URL migration using method=%s", method), - cmd: fmt.Sprintf("soundtouch-cli setup migrate --host=%s --service-url=%s --method=%s", host, serviceURL, method), + cmd: fmt.Sprintf("soundtouch-cli --host=%s setup migrate --service-url=%s --method=%s", host, serviceURL, method), reason: methodReason, }) steps = append(steps, planStep{ title: "Reboot the speaker", - cmd: fmt.Sprintf("soundtouch-cli setup reboot --host=%s", host), + cmd: fmt.Sprintf("soundtouch-cli --host=%s setup reboot", host), reason: "The envswitch parallel-persistence layer only fully wins on next boot; reboot now to lock the new URLs in before pairing.", }) @@ -1314,9 +1352,13 @@ func renderPreResetNote() { fmt.Println() } -func renderPlanSteps(steps []planStep) { +func renderPlanSteps(steps []planStep, includePair bool) { if len(steps) == 0 { - PrintSuccess("Speaker is already migrated and paired. No action required.") + if includePair { + PrintSuccess("Speaker is already migrated and paired. No action required.") + } else { + PrintSuccess("Speaker is already migrated. No action required.") + } return } diff --git a/pkg/service/setup/migration_state_telnet_test.go b/pkg/service/setup/migration_state_telnet_test.go index 4b91c58..2b9ee6d 100644 --- a/pkg/service/setup/migration_state_telnet_test.go +++ b/pkg/service/setup/migration_state_telnet_test.go @@ -39,6 +39,50 @@ func TestIsTelnetMigrated_EmptyVerifiedConfig(t *testing.T) { } } +// TestIsXMLMigrated_MalformedServiceURL guards against the false-positive that +// occurs when --service-url is a malformed URL with an empty hostname (e.g. +// "https:/host" instead of "https://host"). url.Parse succeeds but Hostname() +// returns "", and strings.Contains(anything, "") is always true in Go. +func TestIsXMLMigrated_MalformedServiceURL(t *testing.T) { + m := &Manager{ServerURL: "https:/soundtouch.fritz.box"} // single slash — malformed + + summary := &MigrationSummary{ + ParsedCurrentConfig: &PrivateCfg{ + MargeServerUrl: "https://streaming.bose.com", + }, + } + + if m.isXMLMigrated(summary) { + t.Error("isXMLMigrated = true, want false when ServerURL has no resolvable hostname") + } +} + +// TestCheckIsMigrated_MalformedServiceURL ensures a malformed --service-url +// does not produce IsMigrated=true on an unmigrated speaker. +func TestCheckIsMigrated_MalformedServiceURL(t *testing.T) { + m := &Manager{ + ServerURL: "https:/soundtouch.fritz.box", // single slash — malformed + NewSSH: func(string) SSHClient { + return &mockSSH{runFunc: func(string) (string, error) { return "", errors.New("unused") }} + }, + } + + summary := &MigrationSummary{ + SSHSuccess: true, + TelnetVerifiedConfig: "", // empty — not migrated via telnet either + ParsedCurrentConfig: &PrivateCfg{ + MargeServerUrl: "https://streaming.bose.com", + }, + CurrentResolvConf: "nameserver 8.8.8.8\n", + } + + m.checkIsMigrated(summary, "192.0.2.1") + + if summary.IsMigrated { + t.Error("IsMigrated = true, want false when service URL is malformed and speaker still points at streaming.bose.com") + } +} + // TestCheckIsMigrated_TelnetOnlyMigratedDevice covers the gap that motivated // this iteration: SSH is unreachable, but the speaker has been pointed at // our service via telnet (e.g. a firmware that refuses USB unlock). The diff --git a/pkg/service/setup/setup.go b/pkg/service/setup/setup.go index c5b89ef..3dad65d 100644 --- a/pkg/service/setup/setup.go +++ b/pkg/service/setup/setup.go @@ -543,6 +543,9 @@ func (m *Manager) isXMLMigrated(summary *MigrationSummary) bool { } targetHost := parsedTarget.Hostname() + if targetHost == "" { + return false + } return strings.Contains(summary.ParsedCurrentConfig.MargeServerUrl, targetHost) || strings.Contains(summary.ParsedCurrentConfig.StatsServerUrl, targetHost) || @@ -595,6 +598,9 @@ func (m *Manager) isResolvConfMigrated(client SSHClient, summary *MigrationSumma } targetHost := parsedTarget.Hostname() + if targetHost == "" { + return false + } if strings.Contains(summary.CurrentResolvConf, targetHost) && summary.CACertTrusted { return true }