hub (unreleased, SECURITY): /preferences and /notify refuse a per-customer key acting for another household (403); red-proved
gates / gates (push) Successful in 5m49s
gates / gates (push) Successful in 5m49s
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS
This commit is contained in:
@@ -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=<true|false>` and the dispatcher logs `Customer email sent for <customer>/<event>`.
|
||||
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user