fix(auth): rate-limit the GitHub token-retrieval endpoint by session, not IP - #6135
Merged
Conversation
… not IP Closes #6117 Security review pass over #6114/#6115/#6116's persisted-GitHub-token storage, retrieval, and revocation surface. Findings: - Encryption at rest: correct. storeSessionGitHubToken/ getDecryptedSessionGitHubTokenBundle reuse encryptSecret/decryptSecret (AES-256-GCM, PBKDF2-derived key, random per-record IV+salt) -- the same primitive and key material as repositoryAiKeys/repositoryLinearKeys. Refresh tokens go through the identical encryptSecret call as access tokens, not a weaker scheme. - No raw-token leakage: verified across storeSessionGitHubToken (its one console.warn logs sessionId/message only), the /v1/auth/github/token route (recordRouteProductUsage carries no token field), and packages/loopover-miner/lib/github-token-resolution.js (no console/log call ever touches the resolved token). Sentry's existing beforeSend scrubber (src/selfhost/sentry.ts) independently catches GitHub token value patterns (gh[opsru]_...) as defense in depth. An existing test already asserts the raw token never appears in the auth.session_created audit event. - Logout/revocation: revokeAuthSession unconditionally calls deleteSessionGitHubToken; already verified by an existing test that checks both the decrypt-returns-null path and a raw row-count query. - Endpoint access control: session-only, already verified by an existing test that explicitly authenticates as the static "api"/"mcp" shared-secret identities and confirms both are rejected with 403. - Rate-limit/abuse posture: REAL GAP, fixed here. isPreAuthRateLimitPath's broad `/v1/auth/` prefix match classified /v1/auth/github/token as pre-auth, keying its rate limit by client IP instead of by session -- unlike the OAuth start/callback/device-poll flows it sits alongside, this endpoint always requires a valid session bearer. IP-keying meant a caller with a stolen session token could exceed the strict 10/min cap by rotating source IPs, and unrelated sessions behind a shared IP (office NAT, CI infra) would throttle each other. Excluded this one path from the pre-auth classification so it falls through to token-based keying when a valid bearer is present (falling back to IP-keying only when no valid bearer is supplied, same as every other authenticated route). Also confirmed the only production caller of the decrypted-token repository functions is the single /v1/auth/github/token route (via getLiveSessionGitHubToken) -- no other route or admin surface reaches the decrypted token. The identical IP-vs-session rate-limit gap exists on the pre-existing (pre- milestone) /v1/auth/extension/session endpoint; that is out of this milestone's scope and tracked separately.
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 #6135 +/- ##
=======================================
Coverage 95.33% 95.33%
=======================================
Files 598 598
Lines 47178 47178
Branches 15026 15026
=======================================
Hits 44975 44975
Misses 1477 1477
Partials 726 726
Flags with carried forward coverage won't be shown. Click here to find out more.
|
3 tasks
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
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
Security review pass over #6114/#6115/#6116's persisted-GitHub-token storage, retrieval, and revocation surface — the final issue in the "Unified AMS/ORB Auth" milestone.
Review findings (pass/fail per the issue's checklist)
storeSessionGitHubToken/getDecryptedSessionGitHubTokenBundlereuseencryptSecret/decryptSecret(AES-256-GCM, PBKDF2-derived key, random per-record IV+salt), the same primitive and key material asrepositoryAiKeys/repositoryLinearKeys.storeSessionGitHubToken's oneconsole.warnlogssessionId/message only; the route'srecordRouteProductUsagecall carries no token field;packages/loopover-miner/lib/github-token-resolution.jsnever logs the resolved token. Sentry's existingbeforeSendscrubber (src/selfhost/sentry.ts) independently catches GitHub token value patterns (gh[opsru]_...) as defense in depth. An existing test asserts the raw token never appears in theauth.session_createdaudit event.revokeAuthSessionunconditionally callsdeleteSessionGitHubToken; an existing test checks both the decrypt-returns-null path and a raw row-count query.api/mcpshared-secret identities and confirms both get 403.encryptSecretcall as access tokens, not a weaker scheme.The fix
isPreAuthRateLimitPathinsrc/auth/rate-limit.tsclassified every/v1/auth/*path as pre-authentication, keying its rate limit by client IP. That's correct for the OAuth start/callback/device-poll flows it sits alongside, but/v1/auth/github/tokenalways requires (and validates) a real session bearer token to do anything useful. IP-keying meant:Excluded
/v1/auth/github/tokenfrom the pre-auth classification so it falls through to token-based keying when a valid bearer is present, falling back to IP-keying only when no valid bearer is supplied — matching every other authenticated route.Also confirmed the only production caller of the decrypted-token repository functions is this single route (via
getLiveSessionGitHubToken) — no other route or admin surface reaches the decrypted token.Out of scope
The identical IP-vs-session gap exists on the pre-existing (pre-milestone, #556)
/v1/auth/extension/sessionendpoint. That predates this milestone and is tracked as a separate follow-up rather than bundled here.Closes #6117
Test plan
test/unit/auth.test.ts: same session token from two IPs shares one rate-limit bucket; a different session's token from the same IP gets an independent bucket; no/invalid bearer still falls back to IP-keyingrouteClassForPath("/v1/auth/github/token")explicitly asserted as"strict"npm run test:cigate green (unsharded coverage, no threshold failures)npm run typecheckclean