From 585cf0c1bc904b7726e27d56db8be68d7a284ec8 Mon Sep 17 00:00:00 2001 From: matthew-pilot Date: Mon, 29 Jun 2026 17:20:56 +0000 Subject: [PATCH] fix(dashboard): restrict admin_token query-param to GET only (PILOT-238) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The requireAdminToken wrapper accepted admin_token= via query parameter for ALL HTTP methods, not just GET. A leaked dashboard URL containing ?admin_token=... could be used for cross-site mutation attacks against any admin endpoint wrapped with requireAdminToken — the browser would send the token as a query parameter on POST/PUT/DELETE requests with no CORS preflight barrier (query params are a simple request part). Fix: restrict query-param fallback to GET only. POST/PUT/DELETE must use the X-Admin-Token header, which triggers CORS preflight and blocks cross-origin requests from attacker-controlled pages. This is consistent with the existing patterns in the /api/banner and /api/breakers/ handlers that already had this restriction inlined. Add TestRequireAdminToken_RejectsQueryParamOnWrite to pin the behavior: query-param token is accepted for GET (read-only convenience) but rejected with 401 for POST, PUT, and DELETE. Closes PILOT-238 --- dashboard/dashboard.go | 12 ++++--- dashboard/zz_stats_auth_test.go | 55 +++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/dashboard/dashboard.go b/dashboard/dashboard.go index 7bd2be0..2b35d68 100644 --- a/dashboard/dashboard.go +++ b/dashboard/dashboard.go @@ -739,9 +739,10 @@ func (l *ipRateLimiter) middleware(maxReqs int, window time.Duration, next http. // requireAdminToken wraps next so it returns 401 Unauthorized to anyone // that doesn't present the operator-configured admin token. The token // may be supplied via X-Admin-Token header (preferred for write paths) -// or admin_token=... query parameter (read-only convenience). When no -// admin token is configured on the server the wrapped path is locked -// shut entirely — operators must set -admin-token to enable access. +// or admin_token=... query parameter (GET only — read-only convenience; +// mutations require the header to prevent CSRF from leaked URLs). +// When no admin token is configured on the server the wrapped path is +// locked shut entirely — operators must set -admin-token to enable access. // // Used to lock down the previously-public stats / pulse / badge / // snapshot endpoints so polo.pilotprotocol.network can expose only the @@ -750,7 +751,10 @@ func (l *ipRateLimiter) middleware(maxReqs int, window time.Duration, next http. func (h *Handler) requireAdminToken(next http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { token := r.Header.Get("X-Admin-Token") - if token == "" { + // Query-param fallback only for GET (read-only, no CSRF risk). + // POST/PUT/DELETE require the header so a leaked URL cannot + // be used for cross-site mutation attacks. + if token == "" && r.Method == http.MethodGet { token = r.URL.Query().Get("admin_token") } adminToken := h.cb.GetAdminToken() diff --git a/dashboard/zz_stats_auth_test.go b/dashboard/zz_stats_auth_test.go index 4187ef7..b7cf0a6 100644 --- a/dashboard/zz_stats_auth_test.go +++ b/dashboard/zz_stats_auth_test.go @@ -3,6 +3,7 @@ package dashboard import ( + "bytes" "net/http" "net/http/httptest" "testing" @@ -99,3 +100,57 @@ func TestStatsAuth_UnconfiguredLocksShut(t *testing.T) { t.Errorf("unconfigured: status %d, want 401", resp.StatusCode) } } + +// TestRequireAdminToken_RejectsQueryParamOnWrite confirms that +// requireAdminToken rejects admin_token= query-param for POST/PUT/DELETE, +// preventing CSRF and referer leaks from a leaked URL. The token must +// be sent via X-Admin-Token header for mutating requests. +func TestRequireAdminToken_RejectsQueryParamOnWrite(t *testing.T) { + t.Parallel() + cb := minimalCallbacks() + cb.GetAdminToken = func() string { return "op-secret" } + h := NewHandler(cb) + + hit := false + wrapped := h.requireAdminToken(func(w http.ResponseWriter, r *http.Request) { + hit = true + w.WriteHeader(http.StatusOK) + }) + + cases := []struct { + name string + method string + }{ + {"POST", http.MethodPost}, + {"PUT", http.MethodPut}, + {"DELETE", http.MethodDelete}, + } + for _, tc := range cases { + t.Run(tc.method, func(t *testing.T) { + hit = false + req := httptest.NewRequest(tc.method, "/?admin_token=op-secret", bytes.NewReader(nil)) + rec := httptest.NewRecorder() + wrapped(rec, req) + if rec.Code != http.StatusUnauthorized { + t.Errorf("%s with query-param token: status %d, want 401", tc.method, rec.Code) + } + if hit { + t.Errorf("%s with query-param token: next handler was called unexpectedly", tc.method) + } + }) + } + + // Confirm GET still accepts query-param token (read-only convenience). + t.Run("GET", func(t *testing.T) { + hit = false + req := httptest.NewRequest(http.MethodGet, "/?admin_token=op-secret", nil) + rec := httptest.NewRecorder() + wrapped(rec, req) + if rec.Code != http.StatusOK { + t.Errorf("GET with query-param token: status %d, want 200", rec.Code) + } + if !hit { + t.Errorf("GET with query-param token: next handler was not called") + } + }) +}