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

[hariom888]

Description:

apps/backend/src/routes/connect.ts stores a one-time nonce in Redis during the /connect/github initiation flow. The callback at /connect/github/callback is supposed to verify this nonce to prevent CSRF attacks. The verification logic is:

const storedUserId = app.redis
  ? await app.redis.get(`oauth:nonce:${decodedState.nonce}`)
  : null;

if (app.redis && (!storedUserId || storedUserId !== decodedState.userId)) {
  // reject
}

Both the read and the guard are conditional on app.redis being truthy. If Redis is unavailable (connection failure, misconfiguration, Redis crash), app.redis is falsy and both branches are skipped entirely. The callback then proceeds to exchange the attacker-supplied code for a token and stores it under the userId decoded from the attacker-controlled state parameter — with zero server-side verification that the state was ever issued by this server.

An attacker who can observe or predict the base64-encoded state format (which is not a secret — it is { userId, nonce } base64-encoded) can construct a valid-looking 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.

The initiation endpoint (GET /connect/github) unconditionally calls await app.redis.set(...) without a null-guard, meaning if Redis is actually absent it would throw before a nonce is stored — but the callback does not mirror this strictness and degrades silently.

Affected file: apps/backend/src/routes/connect.ts

[hariom888]

Hi, requesting assignment under GSSoC'26. Technical breakdown:

Root cause:
connect.ts L97 wraps the Redis GET in a ternary (app.redis ? ... : null) and L99 wraps the enforcement in if (app.redis && ...). This means a Redis absence removes the CSRF guard entirely while still allowing the rest of the OAuth exchange to proceed, because the userId is taken directly from the attacker-controlled, base64-decoded state parameter (L107: const userId = decodedState.userId). No database check confirms that userId corresponds to a real, currently-authenticated session.

Affected modules/files:

• apps/backend/src/routes/connect.ts — L56–160 (initiate and callback)
• apps/backend/src/plugins/redis.ts — Redis plugin registration
• apps/backend/src/__tests__/connect.test.ts — CSRF test coverage

Proposed fix approach:

1. Hard-fail when Redis is unavailable: At the top of the callback, if !app.redis, return a 503 (Service unavailable — cannot verify OAuth state) rather than proceeding without the CSRF check. This makes the security boundary explicit and observable.
2. Alternatively, use a stateless HMAC-signed state: Replace Redis nonce storage with an HMAC-SHA256 signature over { userId, nonce, exp } using JWT_SECRET or a dedicated signing key. The callback verifies the signature without any external dependency. This also eliminates the Redis round-trip on the hot path.
3. Remove the asymmetry between the initiator (which unconditionally calls app.redis.set) and the callback (which treats Redis as optional).

Regression/unit test considerations:

• Test: initiate connect, inject a crafted state with a valid userId but a nonce that was never stored → callback must return 400/503, not store any token.
• Test: simulate Redis unavailability during callback → must not proceed to token exchange.
• Test: valid full round-trip (nonce issued and verified) continues to work.
• Existing connect.test.ts likely mocks Redis as present; add a test fixture where app.redis is null/falsy.

Requesting assignment. Thank you!

[Harxhit]

@hariom888 I have assigned this issue to you.