fix(dashboard): restrict requireAdminToken query-param to GET only (PILOT-238)#89
Open
matthew-pilot wants to merge 2 commits into
Open
fix(dashboard): restrict requireAdminToken query-param to GET only (PILOT-238)#89matthew-pilot wants to merge 2 commits into
matthew-pilot wants to merge 2 commits into
Conversation
…on endpoints TestAdminToken_CSRF_MutationRejectsQueryParam verifies that POST /api/admin/runtime/gc with ?admin_token=... returns 401, while X-Admin-Token header continues to work. This is the repro case for PILOT-238 — the requireAdminToken middleware accepts query-param tokens on all HTTP methods, creating a CSRF vector for any password-leak (URL in logs, referer, etc). The test is written to fail before the fix and pass after.
…ILOT-238) The requireAdminToken middleware accepted admin_token=<token> as a query parameter for all HTTP methods — GET, POST, PUT, DELETE, etc. This creates a CSRF vector: if an operator copies a URL containing admin_token=... into chat/browser-history/logs, an attacker can auto-submit a hidden form pointing at that URL and trigger mutations like /api/admin/runtime/gc?admin_token=..., /api/admin/runtime/log-level, or /api/admin/whitelist mutations. Fix: the query-param fallback is restricted to GET requests only. POST/PUT/DELETE must present the admin token via the X-Admin-Token header, which cannot be set by cross-site form submissions. The /api/banner handler already had an inline guard for this (merged in PR #7), but requireAdminToken — used by /api/stats, /api/pulse, /api/badge/*, /api/admin/runtime/*, /api/admin/whitelist, /api/admin/ networks/*, and /api/admin/backups — was still unconditionally accepting query-param tokens on all methods. Closes PILOT-238
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What failed
The
requireAdminTokenmiddleware acceptedadmin_token=<token>as a query parameter for all HTTP methods — GET, POST, PUT, DELETE, etc. This creates a CSRF vector: if an operator copies a URL containingadmin_token=...into chat, browser history, or logs, an attacker can auto-submit a hidden form targeting mutation endpoints like/api/admin/runtime/gc?admin_token=...,/api/admin/runtime/log-level, or/api/admin/whitelist.The
/api/bannerhandler already had an inline guard (merged in PR #7), butrequireAdminToken— used by/api/stats,/api/pulse,/api/badge/*,/api/admin/runtime/*,/api/admin/whitelist,/api/admin/networks/*,/api/admin/backups, and/api/admin/audit/*— was still unconditionally accepting query-param tokens on all methods.Why this fix
Restrict the query-param fallback to GET requests only. POST/PUT/DELETE must present the admin token via the
X-Admin-Tokenheader, which cannot be set by cross-site form submissions (HTML forms cannot send custom headers).Files changed
dashboard/dashboard.go:requireAdminTokennow checksr.Method == http.MethodGetbefore falling back to query-param token (+8/-4)dashboard/zz_stats_auth_test.go: newTestAdminToken_CSRF_MutationRejectsQueryParamthat verifies POST with?admin_token=...returns 401, while the header token continues to work (+37/-0)Verification
Closes PILOT-238