From e5b9151e05692c8deb208c8d8f0949718d448548 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Fri, 9 Oct 2026 12:48:54 +0200 Subject: [PATCH] hub (unreleased, SECURITY): /preferences and /notify refuse a per-customer key acting for another household (403); red-proved Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS --- hub/CHANGELOG.md | 7 +++ hub/internal/api/crosscustomer_prefs_test.go | 63 ++++++++++++++++++++ hub/internal/api/handler.go | 18 +++++- 3 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 hub/internal/api/crosscustomer_prefs_test.go diff --git a/hub/CHANGELOG.md b/hub/CHANGELOG.md index ca17a95d..886d984e 100644 --- a/hub/CHANGELOG.md +++ b/hub/CHANGELOG.md @@ -2,6 +2,13 @@ Not released: ships with the next hub release. No version bump here. +- **SECURITY — one box's key can no longer act for another household** (found 2026-10-09 by a security review of the + R-922 change; fixed without a row). `POST /api/v1/preferences` and `POST /api/v1/notify` checked only that the key + was valid and trusted the body's `customer_id`: box B could rewrite (with R-922: delete) household A's notification + address, or make the hub mail household A any text. Both now refuse a per-customer key whose customer differs (403), + as `/event`, `/status` and `/offsite` already did; the global key still acts for any. `crosscustomer_prefs_test.go` + (red before: `status = 200, want 403` on both; green after). + - **Two log lines no longer print a household's mail address** (found while building R-922, fixed without a row): the preferences push logs `address set=` and the dispatcher logs `Customer email sent for /`. diff --git a/hub/internal/api/crosscustomer_prefs_test.go b/hub/internal/api/crosscustomer_prefs_test.go new file mode 100644 index 00000000..c8d34af0 --- /dev/null +++ b/hub/internal/api/crosscustomer_prefs_test.go @@ -0,0 +1,63 @@ +package api + +import ( + "net/http" + "testing" + + "gitea.dooplex.hu/admin/felhom-hub/internal/store" +) + +// One box's key must not act for another household (found 2026-10-09 by a security review of the R-922 change): +// POST /preferences and POST /notify checked only that the key was valid, then trusted the body's customer_id — so +// box B could rewrite (since R-922: delete) household A's notification address, or make the hub mail household A any +// text. Every other customer-scoped controller route already refuses a mismatch (/event, /status, /offsite …). +// Red-proof: with the checkAuthCustomer mismatch test removed from either handler, its test below fails. + +func newTwoCustomerHandler(t *testing.T) (*Handler, *store.Store) { + t.Helper() + h, st := newEventTestHandler(t) // c1 / ckey + if err := st.SaveCustomerConfig(&store.CustomerConfig{CustomerID: "c2", APIKey: "c2key", RetrievalPassword: "p"}); err != nil { + t.Fatalf("SaveCustomerConfig c2: %v", err) + } + if err := st.SaveNotificationPrefs("c1", "seeded@example.hu", []string{"backup_failed"}, 6); err != nil { + t.Fatalf("stored prefs: %v", err) + } + return h, st +} + +func TestSavePreferences_OtherHouseholdsKeyIsRefused(t *testing.T) { + h, st := newTwoCustomerHandler(t) + rr := do(h, http.MethodPost, "/preferences", "c2key", + `{"customer_id":"c1","email":"","enabled_events":[],"cooldown_hours":6,"email_cleared":true}`) + if rr.Code != http.StatusForbidden { + t.Errorf("c2's key acting for c1: status = %d, want 403", rr.Code) + } + prefs, _ := st.GetNotificationPrefs("c1") + if prefs == nil || prefs.Email != "seeded@example.hu" || len(prefs.EnabledEvents) != 1 { + t.Fatalf("another household's key changed c1's preferences: %+v", prefs) + } +} + +func TestSavePreferences_OwnAndGlobalKeyStillWork(t *testing.T) { + h, st := newTwoCustomerHandler(t) + if rr := do(h, http.MethodPost, "/preferences", "ckey", + `{"customer_id":"c1","email":"own@example.hu","enabled_events":["node_down"],"cooldown_hours":6}`); rr.Code != http.StatusOK { + t.Fatalf("own key: status = %d", rr.Code) + } + if rr := do(h, http.MethodPost, "/preferences", globalKey, + `{"customer_id":"c1","email":"global@example.hu","enabled_events":["node_down"],"cooldown_hours":6}`); rr.Code != http.StatusOK { + t.Fatalf("global key: status = %d", rr.Code) + } + if prefs, _ := st.GetNotificationPrefs("c1"); prefs == nil || prefs.Email != "global@example.hu" { + t.Fatalf("own/global updates must apply: %+v", prefs) + } +} + +func TestNotify_OtherHouseholdsKeyIsRefused(t *testing.T) { + h, _ := newTwoCustomerHandler(t) + rr := do(h, http.MethodPost, "/notify", "c2key", + `{"customer_id":"c1","event_type":"backup_failed","severity":"error","message":"spoofed"}`) + if rr.Code != http.StatusForbidden { + t.Fatalf("c2's key asking the hub to mail c1: status = %d, want 403 (body %s)", rr.Code, rr.Body.String()) + } +} diff --git a/hub/internal/api/handler.go b/hub/internal/api/handler.go index e1626919..e481f529 100644 --- a/hub/internal/api/handler.go +++ b/hub/internal/api/handler.go @@ -2516,7 +2516,10 @@ func (h *Handler) handleCustomerHistory(w http.ResponseWriter, r *http.Request, // handleNotify processes notification events from customer controllers. func (h *Handler) handleNotify(w http.ResponseWriter, r *http.Request) { - if !h.checkAuth(r) { + // A per-customer key acts only for its own customer (the body's customer_id is checked below); the global key for + // any. Found 2026-10-09: this route checked only that the key was valid (crosscustomer_prefs_test.go). + authCustomerID, isGlobal, ok := h.checkAuthCustomer(r) + if !ok { http.Error(w, "Unauthorized", http.StatusUnauthorized) return } @@ -2538,6 +2541,10 @@ func (h *Handler) handleNotify(w http.ResponseWriter, r *http.Request) { http.Error(w, "Invalid payload: customer_id and event_type required", http.StatusBadRequest) return } + if !isGlobal && authCustomerID != payload.CustomerID { + http.Error(w, "Forbidden: customer_id does not match the key", http.StatusForbidden) + return + } h.logger.Printf("[INFO] Notification from %s: %s (%s) — %s", payload.CustomerID, payload.EventType, payload.Severity, payload.Message) @@ -2618,7 +2625,10 @@ func (h *Handler) handleNotify(w http.ResponseWriter, r *http.Request) { // handleSavePreferences stores notification preferences pushed from a customer controller. func (h *Handler) handleSavePreferences(w http.ResponseWriter, r *http.Request) { - if !h.checkAuth(r) { + // A per-customer key acts only for its own customer (the body's customer_id is checked below); the global key for + // any. Found 2026-10-09: this route checked only that the key was valid (crosscustomer_prefs_test.go). + authCustomerID, isGlobal, ok := h.checkAuthCustomer(r) + if !ok { http.Error(w, "Unauthorized", http.StatusUnauthorized) return } @@ -2643,6 +2653,10 @@ func (h *Handler) handleSavePreferences(w http.ResponseWriter, r *http.Request) http.Error(w, "Invalid payload: customer_id required", http.StatusBadRequest) return } + if !isGlobal && authCustomerID != payload.CustomerID { + http.Error(w, "Forbidden: customer_id does not match the key", http.StatusForbidden) + return + } // Empty-email no-clobber guard (v0.71.0, audit F12): a controller push with an empty email // (e.g. an unconfigured box) must never wipe a stored non-empty address — the seeded/edited