From 0de408c467629d1676318ad994deaea59611224f Mon Sep 17 00:00:00 2001 From: Dave Date: Tue, 21 Jul 2026 13:05:35 +0200 Subject: [PATCH] fix(server): don't overwrite shared *arr host/token on empty-credential polls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The qBittorrent and SABnzbd auth middleware overwrote a shared in-memory *arr.Arr's Host/Token in place whenever a.Source == "auto", unconditionally — even when the incoming credentials were empty. When Sonarr/Radarr polled GET /torrents/info (or the SAB equivalent) with blank download-client credentials, they wiped the shared arr's Host and Token to "", corrupting the arr for every other consumer of the shared arr map — notably the repair service, which then rejects the arr with "arr not configured". Guard the mutation so it only overwrites when BOTH incoming credentials are non-empty, mirroring the empty-guard these functions already apply before their AddOrUpdate call. A poll with valid creds still populates/updates an auto arr; an empty-cred poll no longer wipes an already-populated one. Adds regression + positive tests to both the qbit and sabnzbd packages. --- pkg/server/qbit/auth_test.go | 100 +++++++++++++++++++++++++++++ pkg/server/qbit/context.go | 2 +- pkg/server/sabnzbd/context.go | 2 +- pkg/server/sabnzbd/context_test.go | 93 +++++++++++++++++++++++++++ 4 files changed, 195 insertions(+), 2 deletions(-) create mode 100644 pkg/server/qbit/auth_test.go create mode 100644 pkg/server/sabnzbd/context_test.go diff --git a/pkg/server/qbit/auth_test.go b/pkg/server/qbit/auth_test.go new file mode 100644 index 000000000..77c673ba2 --- /dev/null +++ b/pkg/server/qbit/auth_test.go @@ -0,0 +1,100 @@ +package qbit + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/sirrobot01/decypharr/internal/config" + "github.com/sirrobot01/decypharr/pkg/arr" + "github.com/sirrobot01/decypharr/pkg/manager" +) + +func newQBitTestManager(t *testing.T) *manager.Manager { + t.Helper() + config.Reset() + config.SetConfigPath(t.TempDir()) + t.Cleanup(config.Reset) + config.Get().UseAuth = false + m := manager.New() + t.Cleanup(func() { + if err := m.Stop(); err != nil { + t.Errorf("Stop manager: %v", err) + } + }) + return m +} + +// healthOKServer answers 200 to the arr health probe so Arr.Validate() +// completes quickly and deterministically, without any real network +// dependency, when authenticate runs its validation pass. +func healthOKServer(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + return srv +} + +// TestAuthenticateDoesNotWipeAutoArrOnEmptyCredentials is the regression guard +// for the shared-arr corruption bug. When Sonarr/Radarr poll GET /torrents/info +// with blank download-client credentials, authenticate must NOT overwrite an +// already-populated auto arr's Host/Token with empty strings. +// +// Pre-fix, the unconditional `if a.Source == "auto" { a.Host = username; +// a.Token = password }` wiped Host/Token to "" on every empty-cred poll, +// corrupting the shared arr map that other consumers rely on — notably the +// repair service, which then rejects the arr with "arr not configured". +func TestAuthenticateDoesNotWipeAutoArrOnEmptyCredentials(t *testing.T) { + srv := healthOKServer(t) + m := newQBitTestManager(t) + + const category = "sonarr" + seeded := arr.New(category, srv.URL, "seed-token", false, nil, "", string(arr.SourceAuto)) + m.Arr().AddOrUpdate(seeded) + + q := New(m) + got, err := q.authenticate(category, "", "") + if err != nil { + t.Fatalf("authenticate with empty creds returned error: %v", err) + } + if got.Host != srv.URL { + t.Fatalf("Host wiped by empty-cred poll: got %q, want %q", got.Host, srv.URL) + } + if got.Token != "seed-token" { + t.Fatalf("Token wiped by empty-cred poll: got %q, want %q", got.Token, "seed-token") + } + + // The shared map entry every other consumer reads must stay intact. + shared := m.Arr().Get(category) + if shared == nil { + t.Fatalf("shared arr disappeared from the map") + } + if shared.Host != srv.URL || shared.Token != "seed-token" { + t.Fatalf("shared arr corrupted: Host=%q Token=%q", shared.Host, shared.Token) + } +} + +// TestAuthenticatePopulatesAutoArrWithValidCredentials proves the legitimate +// path is unchanged: a poll carrying valid credentials still populates an auto +// arr's Host/Token and registers it in the shared map. +func TestAuthenticatePopulatesAutoArrWithValidCredentials(t *testing.T) { + srv := healthOKServer(t) + m := newQBitTestManager(t) + + const category = "radarr" + q := New(m) + got, err := q.authenticate(category, srv.URL, "valid-token") + if err != nil { + t.Fatalf("authenticate with valid creds returned error: %v", err) + } + if got.Host != srv.URL || got.Token != "valid-token" { + t.Fatalf("auto arr not populated: got Host=%q Token=%q", got.Host, got.Token) + } + + shared := m.Arr().Get(category) + if shared == nil || shared.Host != srv.URL || shared.Token != "valid-token" { + t.Fatalf("valid creds not registered in shared map: %+v", shared) + } +} diff --git a/pkg/server/qbit/context.go b/pkg/server/qbit/context.go index 7bf08073f..02869610a 100644 --- a/pkg/server/qbit/context.go +++ b/pkg/server/qbit/context.go @@ -162,7 +162,7 @@ func (q *QBit) authenticate(category, username, password string) (*arr.Arr, erro if (username == "" || password == "") && cfg.UseAuth { return nil, fmt.Errorf("unauthorized: Host and token are required for authentication(you've enabled authentication)") } - if a.Source == "auto" { + if a.Source == "auto" && username != "" && password != "" { a.Host = username a.Token = password } diff --git a/pkg/server/sabnzbd/context.go b/pkg/server/sabnzbd/context.go index 884acb92b..d3bb9a1f4 100644 --- a/pkg/server/sabnzbd/context.go +++ b/pkg/server/sabnzbd/context.go @@ -123,7 +123,7 @@ func (s *SABnzbd) authenticate(category, username, password string) (*arr.Arr, e if (username == "" || password == "") && cfg.UseAuth { return nil, fmt.Errorf("unauthorized: Host and token are required for authentication(you've enabled authentication)") } - if a.Source == "auto" { + if a.Source == "auto" && username != "" && password != "" { a.Host = username a.Token = password } diff --git a/pkg/server/sabnzbd/context_test.go b/pkg/server/sabnzbd/context_test.go new file mode 100644 index 000000000..c9462e221 --- /dev/null +++ b/pkg/server/sabnzbd/context_test.go @@ -0,0 +1,93 @@ +package sabnzbd + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/sirrobot01/decypharr/internal/config" + "github.com/sirrobot01/decypharr/pkg/arr" + "github.com/sirrobot01/decypharr/pkg/manager" +) + +// newSABAuthHarness wires a bare Manager into the SABnzbd shim. The auth path +// never touches usenet — it only reads and mutates the shared arr map. +func newSABAuthHarness(t *testing.T) (*SABnzbd, *manager.Manager) { + t.Helper() + config.Reset() + config.SetConfigPath(t.TempDir()) + t.Cleanup(config.Reset) + config.Get().UseAuth = false + m := manager.New() + t.Cleanup(func() { + if err := m.Stop(); err != nil { + t.Errorf("Stop manager: %v", err) + } + }) + return New(m), m +} + +// sabHealthOKServer answers 200 to the arr health probe so Arr.Validate() +// completes quickly and deterministically during authenticate. +func sabHealthOKServer(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + return srv +} + +// TestAuthenticateDoesNotWipeAutoArrOnEmptyCredentials is the SABnzbd-side +// regression guard for the shared-arr corruption bug. A SAB poll with empty +// ma_username/ma_password must NOT wipe an already-populated auto arr's +// Host/Token, which would corrupt the shared arr map for every other consumer. +func TestAuthenticateDoesNotWipeAutoArrOnEmptyCredentials(t *testing.T) { + srv := sabHealthOKServer(t) + s, m := newSABAuthHarness(t) + + const category = "sonarr" + seeded := arr.New(category, srv.URL, "seed-token", false, nil, "", string(arr.SourceAuto)) + m.Arr().AddOrUpdate(seeded) + + got, err := s.authenticate(category, "", "") + if err != nil { + t.Fatalf("authenticate with empty creds returned error: %v", err) + } + if got.Host != srv.URL { + t.Fatalf("Host wiped by empty-cred poll: got %q, want %q", got.Host, srv.URL) + } + if got.Token != "seed-token" { + t.Fatalf("Token wiped by empty-cred poll: got %q, want %q", got.Token, "seed-token") + } + + shared := m.Arr().Get(category) + if shared == nil { + t.Fatalf("shared arr disappeared from the map") + } + if shared.Host != srv.URL || shared.Token != "seed-token" { + t.Fatalf("shared arr corrupted: Host=%q Token=%q", shared.Host, shared.Token) + } +} + +// TestAuthenticatePopulatesAutoArrWithValidCredentials proves the legitimate +// path is unchanged: a poll carrying valid credentials still populates an auto +// arr's Host/Token and registers it in the shared map. +func TestAuthenticatePopulatesAutoArrWithValidCredentials(t *testing.T) { + srv := sabHealthOKServer(t) + s, m := newSABAuthHarness(t) + + const category = "radarr" + got, err := s.authenticate(category, srv.URL, "valid-token") + if err != nil { + t.Fatalf("authenticate with valid creds returned error: %v", err) + } + if got.Host != srv.URL || got.Token != "valid-token" { + t.Fatalf("auto arr not populated: got Host=%q Token=%q", got.Host, got.Token) + } + + shared := m.Arr().Get(category) + if shared == nil || shared.Host != srv.URL || shared.Token != "valid-token" { + t.Fatalf("valid creds not registered in shared map: %+v", shared) + } +}