From d013f3965fd86e4965c61be6ae7f21538369439b Mon Sep 17 00:00:00 2001 From: fen Date: Wed, 9 Sep 2026 23:23:10 -0500 Subject: [PATCH] Require secrets via headers only: drop ?key= and ?password= query fallbacks (#137, #141) Query strings leak into Traefik access logs, browser history, and Referer headers. Admin key is now accepted only via X-Admin-Key; paste and can passwords only via X-Paste-Password (or the POST unlock form). Tests updated; new negative cases assert 401 for the query paths. --- internal/api/admin.go | 9 ++++----- internal/api/admin_test.go | 5 +++-- internal/api/cans.go | 6 ------ internal/api/cans_flow_test.go | 3 ++- internal/api/cans_test.go | 6 ++++-- internal/api/issue81_password_ratelimit_test.go | 12 +++++++----- internal/api/main_test.go | 6 ++++-- internal/api/server.go | 11 +++++------ 8 files changed, 29 insertions(+), 29 deletions(-) diff --git a/internal/api/admin.go b/internal/api/admin.go index b88d9dc..01c3246 100644 --- a/internal/api/admin.go +++ b/internal/api/admin.go @@ -160,13 +160,12 @@ func HandleResetAdminKey(dbPath string) { } // adminKeyOK reports whether the request carries the correct admin key via -// X-Admin-Key header or ?key=. Constant-time compare; failures and successes -// are both logged (#40). +// the X-Admin-Key header only. The ?key= query fallback was removed (#137): +// query strings land in access logs, browser history, and Referer headers, +// so accepting the key there leaked the admin secret. Constant-time compare; +// failures and successes are both logged (#40). func (a *apiServer) adminKeyOK(r *http.Request, key string) bool { given := r.Header.Get("X-Admin-Key") - if given == "" { - given = r.URL.Query().Get("key") - } return subtle.ConstantTimeCompare([]byte(given), []byte(key)) == 1 } diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go index 8dc2dbc..67900d0 100644 --- a/internal/api/admin_test.go +++ b/internal/api/admin_test.go @@ -37,11 +37,12 @@ func TestAdminAuth(t *testing.T) { t.Fatalf("wrong key: expected 401, got %d", rec.Code) } + // #137: the ?key= query fallback was removed; keys must go via header. req = httptest.NewRequest("GET", "/admin/api/settings?key=test-admin-key", nil) rec = httptest.NewRecorder() h.ServeHTTP(rec, req) - if rec.Code != 200 { - t.Fatalf("query key: expected 200, got %d", rec.Code) + if rec.Code != 401 { + t.Fatalf("query key: expected 401 after #137 removal, got %d", rec.Code) } req = httptest.NewRequest("GET", "/admin/api/settings", nil) diff --git a/internal/api/cans.go b/internal/api/cans.go index 9107bc3..0583dfd 100644 --- a/internal/api/cans.go +++ b/internal/api/cans.go @@ -206,9 +206,6 @@ func (a *apiServer) handleGetCan(w http.ResponseWriter, r *http.Request) { } if can.PasswordHash.Valid { pw := r.Header.Get("X-Paste-Password") - if pw == "" { - pw = r.URL.Query().Get("password") - } if pw == "" || !store.CheckPassword(can.PasswordHash.String, pw) { writeErr(w, 401, "password required") return @@ -256,9 +253,6 @@ func (a *apiServer) handleCanItem(w http.ResponseWriter, r *http.Request) { can, _ := a.store.GetCan(row.CanID.String) if can != nil && can.PasswordHash.Valid { pw := r.Header.Get("X-Paste-Password") - if pw == "" { - pw = r.URL.Query().Get("password") - } if pw == "" || !store.CheckPassword(can.PasswordHash.String, pw) { // fall back to the browser's unlock cookie for this can c, cerr := r.Cookie("pw_" + can.ID) diff --git a/internal/api/cans_flow_test.go b/internal/api/cans_flow_test.go index 308587d..44c3c8f 100644 --- a/internal/api/cans_flow_test.go +++ b/internal/api/cans_flow_test.go @@ -294,7 +294,8 @@ func TestCanItemCookieParity(t *testing.T) { } // item id from API (with password query) - req = httptest.NewRequest("GET", "/api/cans/"+created.ID+"?password=pw123", nil) + req = httptest.NewRequest("GET", "/api/cans/"+created.ID, nil) + req.Header.Set("X-Paste-Password", "pw123") rec = httptest.NewRecorder() h.ServeHTTP(rec, req) var can struct { diff --git a/internal/api/cans_test.go b/internal/api/cans_test.go index e6ac0ec..4723f4e 100644 --- a/internal/api/cans_test.go +++ b/internal/api/cans_test.go @@ -112,7 +112,8 @@ func TestCanPasswordInheritedByItems(t *testing.T) { } // get item id with pw - req = httptest.NewRequest("GET", "/api/cans/"+created.ID+"?password=pw123", nil) + req = httptest.NewRequest("GET", "/api/cans/"+created.ID, nil) + req.Header.Set("X-Paste-Password", "pw123") rec = httptest.NewRecorder() h.ServeHTTP(rec, req) var can struct { @@ -130,7 +131,8 @@ func TestCanPasswordInheritedByItems(t *testing.T) { } // item with pw -> 200 - req = httptest.NewRequest("GET", "/api/cans/"+created.ID+"/items/"+itemID+"?password=pw123", nil) + req = httptest.NewRequest("GET", "/api/cans/"+created.ID+"/items/"+itemID, nil) + req.Header.Set("X-Paste-Password", "pw123") rec = httptest.NewRecorder() h.ServeHTTP(rec, req) if rec.Code != 200 { diff --git a/internal/api/issue81_password_ratelimit_test.go b/internal/api/issue81_password_ratelimit_test.go index f33d91f..8ec49bf 100644 --- a/internal/api/issue81_password_ratelimit_test.go +++ b/internal/api/issue81_password_ratelimit_test.go @@ -2,7 +2,7 @@ package api // #81: ALL password verification attempts (GET query param, header, POST // form) must go through the per-IP unlock limiter. Regression: N wrong -// passwords via GET ?password= must eventually yield 429. +// passwords via X-Paste-Password must eventually yield 429. import ( "encoding/json" @@ -26,7 +26,7 @@ func createPasswordPaste(t *testing.T, s *apiServer, pw string) string { } // TestRateLimitGetPasswordQuery: repeated wrong passwords via GET -// ?password= must eventually return 429 (unlock limiter: burst 5). +// X-Paste-Password wrong attempts must eventually return 429 (unlock limiter: burst 5). func TestRateLimitGetPasswordQuery(t *testing.T) { s := testServer(t) h := s.routes() @@ -35,7 +35,8 @@ func TestRateLimitGetPasswordQuery(t *testing.T) { var saw429 bool // more attempts than the unlock burst (5) for i := 0; i < 10; i++ { - req := httptest.NewRequest("GET", "/api/pastes/"+id+"?password=wrong"+string(rune('a'+i)), nil) + req := httptest.NewRequest("GET", "/api/pastes/"+id, nil) + req.Header.Set("X-Paste-Password", "wrong"+string(rune('a'+i))) rec := httptest.NewRecorder() h.ServeHTTP(rec, req) if rec.Code == 429 { @@ -47,7 +48,7 @@ func TestRateLimitGetPasswordQuery(t *testing.T) { } } if !saw429 { - t.Fatal("expected 429 after repeated wrong ?password= attempts, never got one") + t.Fatal("expected 429 after repeated wrong password attempts, never got one") } } @@ -83,7 +84,8 @@ func TestRateLimitGetPasswordCorrectStillAllowed(t *testing.T) { h := s.routes() id := createPasswordPaste(t, s, "hunter2") - req := httptest.NewRequest("GET", "/api/pastes/"+id+"?password=hunter2", nil) + req := httptest.NewRequest("GET", "/api/pastes/"+id, nil) + req.Header.Set("X-Paste-Password", "hunter2") rec := httptest.NewRecorder() h.ServeHTTP(rec, req) if rec.Code != 200 { diff --git a/internal/api/main_test.go b/internal/api/main_test.go index 55011d2..e3548ce 100644 --- a/internal/api/main_test.go +++ b/internal/api/main_test.go @@ -89,7 +89,8 @@ func TestPasswordProtection(t *testing.T) { } // with password -> 200 - req = httptest.NewRequest("GET", "/api/pastes/"+created.ID+"?password=hunter2", nil) + req = httptest.NewRequest("GET", "/api/pastes/"+created.ID, nil) + req.Header.Set("X-Paste-Password", "hunter2") rec = httptest.NewRecorder() h.ServeHTTP(rec, req) if rec.Code != 200 { @@ -97,7 +98,8 @@ func TestPasswordProtection(t *testing.T) { } // wrong password -> 401 - req = httptest.NewRequest("GET", "/api/pastes/"+created.ID+"?password=nope", nil) + req = httptest.NewRequest("GET", "/api/pastes/"+created.ID, nil) + req.Header.Set("X-Paste-Password", "nope") rec = httptest.NewRecorder() h.ServeHTTP(rec, req) if rec.Code != 401 { diff --git a/internal/api/server.go b/internal/api/server.go index a1a2ae4..ba67bd4 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -267,18 +267,17 @@ func (a *apiServer) handleGetPaste(w http.ResponseWriter, r *http.Request) { return } if row.PasswordHash.Valid { - // #81: every password verification (header, query param, or empty) - // goes through the same per-IP+paste unlock limiter as the POST form - // path, so brute-force via GET ?password= or X-Paste-Password gets 429. + // #81/#141: every password verification (header or empty) goes + // through the same per-IP+paste unlock limiter as the POST form + // path, so brute-force via X-Paste-Password gets 429. The + // ?password= query fallback was removed (#141): query strings + // leak into access logs, browser history, and Referer headers. if !rateLimitUnlock(row.ID, r) { writeRateLimited(w, 60) return } // require password via header or query pw := r.Header.Get("X-Paste-Password") - if pw == "" { - pw = r.URL.Query().Get("password") - } if pw == "" || !store.CheckPassword(row.PasswordHash.String, pw) { writeErr(w, 401, "password required") return