From 909a85883a1e99e4ac3fc6f68c9bceca2bcbfc8c Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 10 May 2026 19:27:27 +0200 Subject: [PATCH] feat(handlers): allow per-field telnet URL keys in migration options Extracts the migration-options query-string parsing into a single parseMigrationOptions helper used by both HandleGetMigrationSummary and HandleMigrateDevice. The allow-list now covers two families: - marge / stats / sw_update / bmx (XML method's per-field self|proxied|original implementation selectors, unchanged) - marge_url / stats_url / sw_update_url / bmx_url (telnet method's per-field URL overrides; empty values fall back to the canonical derivation in setup.telnetURLsFromOptions) Unknown keys are still dropped, so the manager only sees parameters the handler explicitly opted into. Tests cover the allow-list, the noise filter, and the empty-query case. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/service/handlers/handlers_setup.go | 16 +---- pkg/service/handlers/migration_options.go | 45 ++++++++++++ .../handlers/migration_options_test.go | 71 +++++++++++++++++++ 3 files changed, 118 insertions(+), 14 deletions(-) create mode 100644 pkg/service/handlers/migration_options.go create mode 100644 pkg/service/handlers/migration_options_test.go diff --git a/pkg/service/handlers/handlers_setup.go b/pkg/service/handlers/handlers_setup.go index e853879..cabb984 100644 --- a/pkg/service/handlers/handlers_setup.go +++ b/pkg/service/handlers/handlers_setup.go @@ -411,13 +411,7 @@ func (s *Server) HandleGetMigrationSummary(w http.ResponseWriter, r *http.Reques targetURL := r.URL.Query().Get("target_url") proxyURL := r.URL.Query().Get("proxy_url") - options := make(map[string]string) - - for k, v := range r.URL.Query() { - if len(v) > 0 && (k == "marge" || k == "stats" || k == "sw_update" || k == "bmx") { - options[k] = v[0] - } - } + options := parseMigrationOptions(r.URL.Query()) summary, err := s.sm.GetMigrationSummary(deviceIP, targetURL, proxyURL, options) if err != nil { @@ -465,13 +459,7 @@ func (s *Server) HandleMigrateDevice(w http.ResponseWriter, r *http.Request) { proxyURL := r.URL.Query().Get("proxy_url") method := setup.MigrationMethod(r.URL.Query().Get("method")) - options := make(map[string]string) - - for k, v := range r.URL.Query() { - if len(v) > 0 && (k == "marge" || k == "stats" || k == "sw_update" || k == "bmx") { - options[k] = v[0] - } - } + options := parseMigrationOptions(r.URL.Query()) output, err := s.sm.MigrateSpeaker(deviceIP, targetURL, proxyURL, options, method) if err != nil { diff --git a/pkg/service/handlers/migration_options.go b/pkg/service/handlers/migration_options.go new file mode 100644 index 0000000..22fcc14 --- /dev/null +++ b/pkg/service/handlers/migration_options.go @@ -0,0 +1,45 @@ +package handlers + +import "net/url" + +// migrationOptionKeys is the allow-list of query parameters carried into +// the migration manager's options map. Two families coexist: +// +// - marge / stats / sw_update / bmx — the XML method's per-field +// "self | proxied | original" implementation selectors. +// - marge_url / stats_url / sw_update_url / bmx_url — the telnet +// method's per-field URL overrides (default: derive from target_url). +// +// Unrecognised keys are dropped so the manager never sees query +// parameters it did not opt into. +var migrationOptionKeys = map[string]struct{}{ + "marge": {}, + "stats": {}, + "sw_update": {}, + "bmx": {}, + "marge_url": {}, + "stats_url": {}, + "sw_update_url": {}, + "bmx_url": {}, +} + +// parseMigrationOptions copies the recognised keys from query into a +// fresh map. Empty values are preserved as empty strings so the caller +// can distinguish "explicitly cleared" from "not set" if it ever needs +// to; the setup package's telnetURLsFromOptions treats empty as "use +// default", which is the desired UI behaviour today. +func parseMigrationOptions(query url.Values) map[string]string { + out := make(map[string]string, len(migrationOptionKeys)) + + for k, v := range query { + if _, ok := migrationOptionKeys[k]; !ok { + continue + } + + if len(v) > 0 { + out[k] = v[0] + } + } + + return out +} diff --git a/pkg/service/handlers/migration_options_test.go b/pkg/service/handlers/migration_options_test.go new file mode 100644 index 0000000..c8dfff9 --- /dev/null +++ b/pkg/service/handlers/migration_options_test.go @@ -0,0 +1,71 @@ +package handlers + +import ( + "net/url" + "reflect" + "testing" +) + +func TestParseMigrationOptions_AllowsXMLAndTelnetKeys(t *testing.T) { + q := url.Values{ + "marge": []string{"self"}, + "stats": []string{"proxied"}, + "sw_update": []string{"original"}, + "bmx": []string{"self"}, + "marge_url": []string{"http://example:8000/marge"}, + "stats_url": []string{"http://example:8000"}, + "sw_update_url": []string{"http://example:8000/updates/soundtouch"}, + "bmx_url": []string{"http://example:8000/bmx/registry/v1/services"}, + } + + got := parseMigrationOptions(q) + + want := map[string]string{ + "marge": "self", + "stats": "proxied", + "sw_update": "original", + "bmx": "self", + "marge_url": "http://example:8000/marge", + "stats_url": "http://example:8000", + "sw_update_url": "http://example:8000/updates/soundtouch", + "bmx_url": "http://example:8000/bmx/registry/v1/services", + } + + if !reflect.DeepEqual(got, want) { + t.Errorf("parseMigrationOptions = %v\nwant %v", got, want) + } +} + +func TestParseMigrationOptions_DropsUnknownKeys(t *testing.T) { + q := url.Values{ + "marge": []string{"self"}, + "target_url": []string{"http://example:8000"}, // not an option + "method": []string{"telnet"}, // not an option + "random": []string{"value"}, // attacker-controlled noise + } + + got := parseMigrationOptions(q) + + if _, ok := got["target_url"]; ok { + t.Errorf("target_url leaked into options map: %v", got) + } + + if _, ok := got["method"]; ok { + t.Errorf("method leaked into options map: %v", got) + } + + if _, ok := got["random"]; ok { + t.Errorf("random key leaked into options map: %v", got) + } + + if got["marge"] != "self" { + t.Errorf("marge = %q, want self", got["marge"]) + } +} + +func TestParseMigrationOptions_EmptyQueryReturnsEmptyMap(t *testing.T) { + got := parseMigrationOptions(url.Values{}) + if len(got) != 0 { + t.Errorf("got %v, want empty map", got) + } +}