Assert the retired proof headers stay off the CORS allowlist - #537
Assert the retired proof headers stay off the CORS allowlist#537seribaymadina wants to merge 2 commits into
Conversation
A browser only sends a request whose headers the preflight approved, so putting X-EA-Token or X-Internal-Key back on the allowlist is not a narrow early-access regression: any client still sending one has its request cancelled, every leaderboard renders empty, and nothing reaches a server to log it. #521 left the positive case for Authorization; this is its negative twin.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe CORS API tests now import Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coval-cale
left a comment
There was a problem hiding this comment.
Does this involve network calls or is this locally mocked? Cause if it's network calls we should refactor this test suite to not use them.
The parametrized negative only caught headers being added. The outage came from one being removed: #521 dropped X-EA-Token and X-Internal-Key while the dashboard still sent them, and that test would have passed throughout. Asserting the exact set fails either way, so narrowing the allowlist has to answer what still sends the header.
|
Locally mocked — no network. The Separately, you prompted a rethink of the test itself and I've replaced it in It now asserts the allowlist is exactly Worth being blunt about the limit: this pins the server's side only. It can't see what the dashboard sends, so it turns a silent narrowing into a failing test that asks "what still sends this?" rather than actually verifying the contract. The real guard is a cross-repo check that the client's header set is a subset of this one — happy to file that separately if it's worth it. Client-side fix is coval-ai/benchmarks-web#47. |
A browser only sends a request whose headers the preflight approved, so putting
X-EA-TokenorX-Internal-Keyback on the allowlist is not a narrow early-access regression: any client still sending one has its request cancelled before it leaves the browser, every leaderboard renders empty including the public rows, and nothing reaches a server to log it. That is how the last one went unnoticed.#521 left the positive case for
Authorization; this is its negative twin. Test-only, and it can fail only if CORS is re-widened.Verified by mutation: re-adding both headers to
allow_headersfails both cases, reverting passes. Fixes the client side in coval-ai/benchmarks-web#47.Greptile Summary
The PR adds regression coverage ensuring the retired
X-Internal-KeyandX-EA-Tokenheaders remain excluded from CORS preflight approval.Access-Control-Allow-Headers.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Reviews (2): Last reviewed commit: "Assert the retired proof headers stay of..." | Re-trigger Greptile