From 4442153af1acbd53884c4f3fa0caa12abbf2f9cf Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 15 Jul 2026 05:23:31 -0700 Subject: [PATCH] fix(auth): rate-limit the GitHub token-retrieval endpoint by session, 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. --- src/auth/rate-limit.ts | 11 ++++++++++- test/unit/auth.test.ts | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/auth/rate-limit.ts b/src/auth/rate-limit.ts index 6b4e36e268..825d1261e5 100644 --- a/src/auth/rate-limit.ts +++ b/src/auth/rate-limit.ts @@ -221,6 +221,15 @@ function isValidIpv6(value: string): boolean { return hasHexSegment; } +// /v1/auth/github/token (#6114/#6115/#6117) is excluded from the broad /v1/auth/ prefix match below: unlike +// the OAuth start/callback/device-poll flows it sits alongside, it always requires (and validates) a real +// session bearer token to do anything useful, so it should rate-limit per SESSION like any other authenticated +// route -- not per IP, which would let a caller with a stolen session token bypass the strict 10/min cap by +// rotating source IPs, and would let unrelated sessions behind one NAT (a shared office network, CI infra) +// throttle each other. function isPreAuthRateLimitPath(path: string): boolean { - return path === "/health" || path === "/v1/mcp/compatibility" || path === "/openapi.json" || path === "/mcp" || path.startsWith("/v1/auth/") || path === "/v1/github/webhook"; + return ( + (path === "/health" || path === "/v1/mcp/compatibility" || path === "/openapi.json" || path === "/mcp" || path.startsWith("/v1/auth/") || path === "/v1/github/webhook") && + path !== "/v1/auth/github/token" + ); } diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index c826b43f1a..5478e247ba 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -112,6 +112,7 @@ describe("private-beta auth and rate limiting", () => { expect(routeClassForPath("/v1/github/webhook")).toBe("strict"); expect(routeClassForPath("/v1/orb/ingest")).toBe("strict"); // open telemetry ingest — abuse-capped per IP expect(routeClassForPath("/v1/auth/github/device/start")).toBe("strict"); + expect(routeClassForPath("/v1/auth/github/token")).toBe("strict"); // #6117: same strict cap as the rest of /v1/auth/* expect(routeClassForPath("/v1/local/branch-analysis")).toBe("expensive"); expect(routeClassForPath("/loopover/shot")).toBe("expensive"); expect(routeClassForPath("/v1/scoring/preview")).toBe("expensive"); @@ -180,6 +181,42 @@ describe("private-beta auth and rate limiting", () => { expect(observedKeys[0]).toMatch(/^normal:\/v1\/public\/github\/repos\/:owner\/:repo\/stats:ip:/); }); + it("keys /v1/auth/github/token by SESSION, not by IP (#6117) -- unlike its pre-auth OAuth-flow siblings", async () => { + const observedKeys: string[] = []; + const env = rateLimitTestEnv({}, observedKeys); + const { token: sessionToken } = await createSessionForGitHubUser(env, { login: "jsonbored", id: 42 }); + + // The same session's token from two DIFFERENT IPs shares one bucket -- a stolen token can't be used to + // bypass the strict cap by rotating source IPs. + await expect( + enforceRateLimit(fakeContext(env, "/v1/auth/github/token", { authorization: `Bearer ${sessionToken}`, "cf-connecting-ip": "203.0.113.9" }), "strict"), + ).resolves.toBeNull(); + await expect( + enforceRateLimit(fakeContext(env, "/v1/auth/github/token", { authorization: `Bearer ${sessionToken}`, "cf-connecting-ip": "198.51.100.50" }), "strict"), + ).resolves.toBeNull(); + expect(observedKeys).toHaveLength(2); + expect(observedKeys[0]).toBe(observedKeys[1]); + expect(observedKeys[0]).toMatch(/^strict:\/v1\/auth\/github\/token:token:/); + const firstSessionKey = observedKeys[0]; + + // A DIFFERENT session's token from the SAME IP gets its own independent bucket -- unrelated sessions + // behind one NAT/CI-runner IP don't throttle each other. + observedKeys.length = 0; + const { token: otherSessionToken } = await createSessionForGitHubUser(env, { login: "other-user", id: 43 }); + await expect( + enforceRateLimit(fakeContext(env, "/v1/auth/github/token", { authorization: `Bearer ${otherSessionToken}`, "cf-connecting-ip": "203.0.113.9" }), "strict"), + ).resolves.toBeNull(); + expect(observedKeys[0]).toMatch(/^strict:\/v1\/auth\/github\/token:token:/); + expect(observedKeys[0]).not.toBe(firstSessionKey); + + // No/invalid bearer still falls back to IP-keying (the pre-auth default), matching every other route. + observedKeys.length = 0; + await expect( + enforceRateLimit(fakeContext(env, "/v1/auth/github/token", { "cf-connecting-ip": "203.0.113.9" }), "strict"), + ).resolves.toBeNull(); + expect(observedKeys[0]).toMatch(/^strict:\/v1\/auth\/github\/token:ip:/); + }); + it("ignores proxy fallback headers when cf-connecting-ip is absent", async () => { const observedKeys: string[] = []; const env = rateLimitTestEnv({}, observedKeys);