fix(connect): validate OAuth state nonce server-side to prevent CSRF (#223)

[Ridanshi]

Summary

Closes #223.

The GitHub connect flow (/api/connect/github) accepted userId directly from the unverified state callback parameter and used Math.random() to generate the state value. An attacker could forge a callback with a crafted state, causing the server to associate the victim's account with an attacker-controlled GitHub credential.

The login flow (/auth/github, /auth/google) is addressed separately via the cookie-based double-submit CSRF fix already present on main (#481).

Changes

apps/backend/src/routes/connect.ts

• Generate a 32-byte cryptographically secure nonce on /connect/github initiation
• Store { userId } in Redis under oauth:connect-nonce:<nonce> with a 10-minute TTL
• Pass only the nonce as the state parameter — userId never travels through the callback URL
• On callback: validate nonce format (64 lowercase hex chars), look up Redis, reject unknown or expired entries, fail closed if Redis is unavailable, delete nonce immediately after first use (replay protection)
• Replace Math.random() with randomBytes(32)
• Use dedicated github_follow platform key to prevent auth-token overwrites
• Fail closed — no Redis bypass path

apps/backend/src/__tests__/connect.test.ts

• 14 tests covering the full callback security surface

apps/backend/src/__tests__/oauth-scope.test.ts

• Updated buildConnectApp and makeConnectState to match new nonce-only state format

Security properties

Property	Before	After
State entropy	~44 bits (Math.random)	256 bits (randomBytes(32))
userId source in callback	Trusted from state param	Looked up from Redis by nonce
CSRF protection	None	Server-side nonce validation
Replay protection	None	Nonce deleted on first use
Nonce expiry	None	10 minutes (Redis TTL)
Redis unavailable	N/A	Fails closed (rejects request)

Tests (connect.test.ts — 14 tests)

• Valid connect flow end-to-end with github_follow platform assertion
• Nonce stored in Redis with correct userId and TTL
• State is 64 lowercase hex chars
• Missing code parameter
• Missing state parameter
• Malformed state (too short)
• Malformed state (non-hex characters)
• Forged base64-JSON state rejected as malformed
• Unknown nonce (not in Redis)
• Expired nonce
• Forged nonce (not in Redis)
• Replay attack — second callback with same nonce is rejected
• Corrupt Redis data
• GitHub token exchange failure

Test plan

• npx vitest run src/__tests__/connect.test.ts src/__tests__/oauth-scope.test.ts src/__tests__/follow.test.ts src/__tests__/profiles.test.ts — 52/52 pass
• Pre-existing failures in event.test.ts and analytics.test.ts are unrelated to this change (upstream test infrastructure issue with request.user not being set by the jwtVerify mock)

[vercel]

@Ridanshi is attempting to deploy a commit to the Prashantkumar Khatri's projects Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions]

Hi @Ridanshi,

Thanks for opening this pull request.

This PR has been automatically classified based on the files modified.

Applied Labels

• backend

Primary Review Area

• backend

Reviewer

@Harxhit has been identified as the primary reviewer for this pull request.

If you have any questions regarding the affected area or implementation details, feel free to reach out to the assigned reviewer.

Thank you for your contribution!

[github-actions]

CI — All Checks Passed

Backend — PASS

Check	Result
Lint	PASS
Test	PASS
Typecheck	PASS

Mobile — SKIP

Check	Result
Lint	-
Test	-

Web — SKIP

Check	Result
Build	-

---

Last updated: Wed, 17 Jun 2026 05:53:58 GMT

[Harxhit]

Checked this out and tested locally. The core security fix is correct and the test coverage is genuinely thorough — this closes the account-takeover vector cleanly. I do have a couple of things that should be sorted before merge, the main one being that the branch is built on a stale base.

Verified locally: npm run typecheck clean, and npx vitest run src/__tests__/connect.test.ts src/__tests__/oauth-scope.test.ts src/__tests__/follow.test.ts src/__tests__/profiles.test.ts → 52/52 pass.

Verified ✅

• The CSRF vector is actually closed. userId no longer travels through the callback — it's looked up server-side from Redis by nonce. A forged callback can no longer bind a victim's account to an attacker credential.
• Entropy is sound. randomBytes(32).toString('hex') = 256-bit nonce, replacing Math.random().
• Strict format validation before any Redis I/O. parseNonce enforces ^[0-9a-f]{64}$ and the callback bails before touching Redis on malformed input — confirmed by the "too short", "non-hex", and "forged base64-JSON" tests, all of which assert mockRedis.get is never called.
• No bypass path. Traced every branch: missing code/state → reject; bad format → reject; nonce not in Redis → reject; corrupt JSON → reject; empty userId → reject. Token exchange is only reachable after a valid, stored nonce.
• Single-use / replay protection works. Nonce is deleted after the existence check, and the replay test (second callback, same nonce → connect_failed) passes.
• Expiry via 10-min Redis TTL (EX 600), asserted in the initiation test.
• Fails closed when Redis is down. app.redis is always decorated by the plugin, so the dropped if (app.redis) guards are safe; a genuine outage makes .get() reject → 500 → no connection made.
• github_follow platform key correctly prevents the connect token from clobbering the login token.

Suggestions

1. [blocker] Rebase onto main — the branch is stale and conflicts. This branch forked from an older connect.ts (~221 lines); main has since refactored that file substantially (~383 lines, including its own lint cleanup: node:crypto, catch {}, fixed import order). git merge main currently conflicts in both connect.ts and connect.test.ts. Please rebase and re-resolve so the fix lands on top of the current file rather than reverting main's improvements. Several lint errors below disappear after rebasing.
2. [security – minor] Use atomic getdel to close the replay TOCTOU window. The callback does get then a separate del. Two concurrent callbacks replaying the same nonce can both pass the get before either dels. auth.ts:604 already uses app.redis.getdel(...) for exactly this one-time-code pattern — mirroring it here makes consumption atomic:

   const storedRaw = await app.redis.getdel(`oauth:connect-nonce:${nonce}`);
   if (!storedRaw) { /* reject */ }

3. [lint – should fix] New parseNonce violates the repo's curly rule (brace-less ifs). Wrap them in braces.
4. [lint – should fix] Test-harness lint. The modified harnesses trip import-x/order and no-param-reassign in both connect.test.ts and oauth-scope.test.ts. eslint --fix handles the import ordering.
5. [nit] Redis-outage UX. When Redis is unreachable the callback returns a raw 500 rather than the friendly …/settings?error=server_error redirect used elsewhere in this route. Security is fine (fails closed), but wrapping get/del to land back on settings would be more consistent.

Nice work on the test matrix — the forged-state, replay, corrupt-data, and expiry cases are exactly what I'd want on a CSRF fix. Once it's rebased and the getdel + lint items are in, this is good to go.

_{Reviewed locally on the PR branch: typecheck ✅, 52/52 tests ✅, eslint + merge-base/conflict analysis. No changes pushed.}

[Ridanshi]

Rebased onto main and addressed all review feedback:

[blocker] Rebase — Branch is now on top of main (5fffad9). All improvements from main's connect.ts (autodiscover endpoint, node:crypto, getErrorMessage utils, typed catch blocks) are preserved. The nonce-only state format lands cleanly on the 383-line file.

[security] Atomic getdel — Replaced the get + del pair with app.redis.getdel(...), mirroring auth.ts:604. Replay TOCTOU window is now closed.

[lint] parseNonce curly rule — Both if branches now have braces.

[lint] Test-harness lint — Fixed import-x/order in connect.test.ts and oauth-scope.test.ts. Renamed req → request in authenticate decorator callbacks to satisfy no-param-reassign (only request and reply are in ignorePropertyModificationsFor).

[nit] Redis outage — getdel is now wrapped in try/catch; on Redis failure the callback redirects to settings?error=server_error instead of returning a raw 500, consistent with the rest of the route.

Validation on the updated branch:

• eslint src/routes/connect.ts src/__tests__/connect.test.ts src/__tests__/oauth-scope.test.ts → 0 errors
• vitest run connect.test.ts oauth-scope.test.ts → 34/34 pass (20 connect + 14 oauth-scope; added Redis-outage test)
• vitest run follow.test.ts → 16/16 pass
• The profiles.test.ts P2002 failure (1/8) pre-exists on main and is unrelated to this PR