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 <noreply@anthropic.com>
This commit is contained in:
Tobias Gesellchen
2026-05-22 18:58:57 +02:00
co-authored by Claude Sonnet 4.6
parent a684c88325
commit b0df8ba963
3 changed files with 100 additions and 8 deletions
+50 -8
View File
@@ -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
}
@@ -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
+6
View File
@@ -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
}