fix(auth): rate-limit /v1/auth/extension/session by session, not IP - #6137
Merged
Conversation
The same pre-auth-vs-session-authenticated gap #6117 fixed for /v1/auth/github/token also applies to /v1/auth/extension/session (#556): isPreAuthRateLimitPath's broad /v1/auth/ prefix match classified it as pre-auth, keying its rate limit by client IP. But this endpoint always requires (and validates) a real session bearer token to mint a new extension-scoped session from it -- it is not a pre-auth flow. IP-keying meant a caller with a stolen session token could bypass the strict 10/min cap by rotating source IPs, and unrelated sessions behind a shared IP (office NAT, CI infra) would throttle each other. Generalized the #6117 fix's single-path exclusion into a small Set of session-authenticated /v1/auth/* paths and added this one to it. Falls through to token-based rate-limit keying when a valid bearer is present, falling back to IP-keying only when no valid bearer is supplied, matching every other authenticated route.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6137 +/- ##
=======================================
Coverage 95.33% 95.33%
=======================================
Files 598 598
Lines 47177 47178 +1
Branches 15025 15025
=======================================
+ Hits 44974 44975 +1
Misses 1477 1477
Partials 726 726
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Summary
/v1/auth/github/token.isPreAuthRateLimitPathinsrc/auth/rate-limit.tsclassified every/v1/auth/*path as pre-authentication, keying its rate limit by client IP./v1/auth/extension/session(feat(api): /v1/extension/* contributor-context endpoints + miner auth scope #556) is not actually pre-auth: it always requires (and validates) a real session bearer token, and mints a new extension-scoped session from it.Setof session-authenticated/v1/auth/*paths and added this one. Falls through to token-based rate-limit keying when a valid bearer is present, falling back to IP-keying only when no valid bearer is supplied — matching every other authenticated route.Pre-existing behavior from #556, unrelated to the #6114-#6117 GitHub-auth milestone — shipping as its own standalone fix.
Test plan
test/unit/auth.test.tsmirroring Security review pass: persisted GitHub token storage, retrieval, and revocation #6117's/v1/auth/github/tokentest: same session token from two IPs shares one bucket; a different session's token from the same IP gets an independent bucket; no/invalid bearer still falls back to IP-keyingnpm run test:cigate greennpm run typecheckclean