Skip to content

fix(connect): hard-fail OAuth callback when Redis unavailable #478

Open
hariom888 wants to merge 6 commits into
Dev-Card:mainfrom
hariom888:fix/438-oauth-csrf-bypass-redis-unavailable
Open

fix(connect): hard-fail OAuth callback when Redis unavailable #478
hariom888 wants to merge 6 commits into
Dev-Card:mainfrom
hariom888:fix/438-oauth-csrf-bypass-redis-unavailable

Conversation

@hariom888

Copy link
Copy Markdown
Contributor

Summary

Fixes #438.

/connect/github/callback guarded its CSRF nonce check behind if (app.redis && ...). A Redis outage silently removed the entire guard while still allowing the OAuth exchange to proceed — with userId taken directly from the attacker-controlled base64 state parameter. An attacker who could predict or observe the state format could store a token under any arbitrary user's account.

Root cause

// Before — Redis absence silently skips the entire CSRF check
const storedUserId = app.redis
  ? await app.redis.get(`oauth:nonce:${decodedState.nonce}`)
  : null;

if (app.redis && (!storedUserId || storedUserId !== decodedState.userId)) {
  // ...reject
}
// Redis down → both branches skipped → falls through to token exchange

Fix

Hard-fail immediately if Redis is not ready. The callback cannot safely proceed without nonce verification.

// After — explicit 503 gate before any token exchange
if (!app.redis || app.redis.status !== 'ready') {
  return reply.status(503).send({ error: 'Service temporarily unavailable. Please try again.' });
}

const storedUserId = await app.redis.get(`oauth:nonce:${decodedState.nonce}`);
if (!storedUserId || storedUserId !== decodedState.userId) {
  return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=invalid_state`);
}

await app.redis.del(`oauth:nonce:${decodedState.nonce}`);

This mirrors the strictness of the initiator (GET /github), which already unconditionally calls app.redis.set — making Redis a hard dependency for both ends of the flow.

Test coverage

Scenario Expected
Redis status !== 'ready' 503, no token written
app.redis is null 503, no token written
Crafted state, nonce never issued 302 → invalid_state
Nonce exists but userId mismatch 302 → invalid_state
Valid round-trip 302 → connected=github, nonce consumed
Nonce replay (second request) 302 → invalid_state
Missing code or state 302 → missing_params
Malformed base64 state 302 → connect_failed

Files changed

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

The CSRF nonce check in /connect/github/callback was guarded by if (app.redis && ...), meaning a Redis outage silently bypassed all CSRF verification. An attacker could craft a base64 state for any userId, submit a stolen OAuth code to the callback, and have a github_follow token stored under an arbitrary user's account.

Changes:
- Replace the conditional CSRF guard with an explicit hard-fail: if !app.redis || app.redis.status !== 'ready', return 503 immediately. The callback never reaches token exchange or DB writes without a verified nonce.
- Remove all �pp.redis ? ... : null ternaries from the callback path — every Redis access is now unconditional, matching the initiator's strictness.
- Nonce deletion (
edis.del) is now unconditional after a successful verification, preventing replay attacks.

Tests added (connect.test.ts):
- Redis unavailable (status !== 'ready') → 503, no upsert
- Redis is null/falsy → 503, no upsert
- Crafted state with unknown nonce → invalid_state redirect, no upsert
- Nonce present but userId mismatch → invalid_state redirect, no upsert
- Valid round-trip → connected=github redirect, nonce consumed
- Nonce replay → second request rejected after first consumes the nonce
- Missing code/state params → missing_params redirect
- Malformed base64 state → connect_failed redirect
@vercel

vercel Bot commented Jun 5, 2026

Copy link
Copy Markdown

@hariom888 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

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

CI — Checks Failed

Backend — FAIL

Check Result
Lint PASS
Test PASS
Typecheck FAIL

Mobile — SKIP

Check Result
Lint -
Test -

Web — SKIP

Check Result
Check -
Build -

Last updated: Fri, 05 Jun 2026 11:16:33 GMT

@hariom888

Copy link
Copy Markdown
Contributor Author

@Harxhit

Typecheck failure is pre-existing on main — not introduced by this PR

All typecheck errors are in files I did not touch:

  • src/routes/team.ts — 6 errors (missing TeamRole export, implicit any, PrismaClientKnownRequestError)
  • src/__tests__/team.test.ts — 1 error (missing TeamRole export)
  • src/services/publicService.ts — 1 error (implicit any)
  • src/utils/error.util.ts — 7 errors (PrismaClientKnownRequestError, PrismaClientValidationError, unknown type)

To confirm, run tsc --noEmit on main directly — the same 15 errors appear before any changes from this PR.

Lint: ✅ PASS
Test: ✅ PASS
Typecheck: ❌ pre-existing failure unrelated to this PR

@Harxhit Harxhit added the gssoc:approved Required label for every approved PR. Gives the base +50 points and enables contribution tracking. label Jun 6, 2026
@ShantKhatri ShantKhatri requested a review from Harxhit June 6, 2026 17:43

@Harxhit Harxhit left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix lint error in connect.ts file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gssoc:approved Required label for every approved PR. Gives the base +50 points and enables contribution tracking.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: GitHub Connect OAuth callback silently skips CSRF nonce verification when Redis is unavailable — account takeover vector

2 participants