fix(dashboard): restrict admin_token query-param to GET only (PILOT-238)#87
Closed
matthew-pilot wants to merge 1 commit into
Closed
fix(dashboard): restrict admin_token query-param to GET only (PILOT-238)#87matthew-pilot wants to merge 1 commit into
matthew-pilot wants to merge 1 commit into
Conversation
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
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
Superseded by PR #89 — broader fix at requireAdminToken level |
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 changed
The
requireAdminTokenwrapper acceptedadmin_token=via query parameter for all HTTP methods, not just GET. This meant a leaked dashboard URL containing?admin_token=...(e.g. pasted into Slack, email, or browser history) could be used for cross-site mutation attacks on any admin endpoint wrapped by it.The
/api/bannerand/api/breakers/handlers already had inline restrictions (header-only for mutations), butrequireAdminTokenitself — used by/api/admin/runtime/profile-rates,/api/admin/runtime/log-level,/api/admin/whitelist,/api/admin/runtime/gc, and others — still accepted query-param tokens for writes.Why
Query parameters are a "simple" part of an HTTP request (like URL paths or body content). An attacker-controlled page can submit a form or script that sends a POST/PUT/DELETE with
?admin_token=...appended — the browser sends the token without triggering a CORS preflight. By contrast, theX-Admin-Tokenheader is a non-simple header that triggers preflight, blocking cross-origin requests from attacker pages.Verification
go build ./...✅go vet ./...✅go test ./...— all packages green ✅TestRequireAdminToken_RejectsQueryParamOnWrite— new test that confirms query-param token is accepted for GET but rejected with 401 for POST/PUT/DELETE ✅Files changed
dashboard/dashboard.go(+7/-4) — restrict query-param fallback to GET inrequireAdminTokendashboard/zz_stats_auth_test.go(+56/-0) — new test for the restrictionCloses https://vulturelabs.atlassian.net/browse/PILOT-238