feat(auth): add Zod validation for OAuth callback endpoints - #593
Conversation
Validates code and state query params in /auth/github/callback and /auth/google/callback before any token exchange or DB work happens. Adds oauthCallbackSchema to validators.ts and tests covering missing/ empty code, missing/empty state, and state cookie mismatch scenarios.
|
@ramnnn2006 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. |
|
Hi @ramnnn2006, Thanks for opening this pull request. This PR has been automatically classified based on the files modified. Applied Labels
Primary Review Area
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! |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Zod-based validation for OAuth callback query parameters (GitHub + Google) and introduces tests to ensure invalid callback requests fail early with consistent 400 responses.
Changes:
- Introduced
oauthCallbackSchemato validatecodeandstateon OAuth callbacks. - Updated GitHub/Google callback routes to use
safeParse()and return structured validation errors. - Added Vitest coverage for invalid callback parameter and OAuth state-cookie scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/backend/src/validations/auth.validation.ts | Adds a Zod schema for OAuth callback query params. |
| apps/backend/src/routes/auth.ts | Uses the new schema to validate callback querystrings and standardize 400 responses. |
| apps/backend/src/tests/auth-callback.test.ts | Adds regression tests for Zod validation + state cookie enforcement. |
Comments suppressed due to low confidence (1)
apps/backend/src/validations/auth.validation.ts:1
- Schema naming is inconsistent (
oAuthStartSchemavsoauthCallbackSchema). Standardizing on a single convention (e.g.,oauthStartSchema/oauthCallbackSchemaoroAuthStartSchema/oAuthCallbackSchema) will make imports and discoverability more predictable.
import { z } from 'zod';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const oauthCallbackSchema = z.object({ | ||
| code: z.string().min(1, 'Authorization code is required'), | ||
| state: z.string().min(1, 'State parameter is required'), | ||
| }); No newline at end of file |
| app.get('/github/callback', async (request: FastifyRequest<{ Querystring: OAuthCallbackQuery }>, reply: FastifyReply) => { | ||
| //TODO: Add zod validation here | ||
| const { code, state } = request.query; | ||
| const parsed = oauthCallbackSchema.safeParse(request.query); |
| const parsed = oauthCallbackSchema.safeParse(request.query); | ||
| if (!parsed.success) { | ||
| return reply.status(400).send({ error: 'Invalid callback parameters', details: parsed.error.flatten() }); | ||
| } | ||
| const { code, state } = parsed.data; |
CI — All Checks PassedBackend — PASS
Mobile — SKIP
Web — SKIP
Last updated: |
- rename oauthCallbackSchema to oAuthCallbackSchema to match naming convention - add .trim() to code and state fields to reject whitespace-only values - export OAuthCallbackQuery type from auth.validation.ts and remove duplicate local interface
|
@Harxhit could u check , ive resolved the conflictsnow! |
Harxhit
left a comment
There was a problem hiding this comment.
Review — adds Zod validation to the OAuth callbacks.
Clean, well-targeted change that does exactly what the title says: replaces the two //TODO: Add zod validation here placeholders with a shared oAuthCallbackSchema, drops the local OAuthCallbackQuery interface in favor of z.infer, and adds solid rejection-path tests (14 cases, ran green locally).
Verdict: approve with minor changes. The only item I'd actually act on is the CSRF cookie not being cleared on the validation-failure branch (inline below). Everything else is optional polish.
Inline comments follow.
| const { code, state } = request.query; | ||
| const parsed = oAuthCallbackSchema.safeParse(request.query); | ||
| if (!parsed.success) { | ||
| return reply.status(400).send({ error: 'Invalid callback parameters', details: parsed.error.flatten() }); |
There was a problem hiding this comment.
CSRF cookie not cleared on validation failure (behavior change).
Previously the !code check ran after reply.clearCookie('oauth_state', ...) (line 106). Now validation returns early here, so a request with a valid+matching state but missing/empty code returns 400 without clearing oauth_state. The single-use CSRF token then lingers in the browser until the next successful callback or expiry.
Not exploitable (it's still compared on the next attempt), but a small hygiene regression. Consider clearing the cookie before returning on this branch, or moving clearCookie ahead of both checks once storedState is read.
Same applies to the Google callback below (line ~316).
Separately: this path also catches the user-denial redirect (?error=access_denied, no code), which now surfaces as a generic "Invalid callback parameters". Pre-existing, but since you're here it could be worth handling request.query.error explicitly. Optional.
| const { code, state } = request.query; | ||
| const parsed = oAuthCallbackSchema.safeParse(request.query); | ||
| if (!parsed.success) { | ||
| return reply.status(400).send({ error: 'Invalid callback parameters', details: parsed.error.flatten() }); |
There was a problem hiding this comment.
Same cookie-not-cleared-on-validation-failure point as the GitHub callback applies here.
Also: the parse + state-check preamble is now byte-identical across both callbacks. Optional, but a small shared helper (or a preHandler) like validateOAuthCallback(request, reply) returning { code, state } would keep the two providers from drifting.
| state: z.string().trim().min(1, 'State parameter is required'), | ||
| }); | ||
|
|
||
| export type OAuthCallbackQuery = z.infer<typeof oAuthCallbackSchema>; No newline at end of file |
There was a problem hiding this comment.
Schema looks good — z.string().trim().min(1) is appropriately defensive and the messages are clear.
Nit: file is missing a trailing newline (diff shows \ No newline at end of file). Add one for consistency / to avoid lint noise.
| await app.close(); | ||
| }); | ||
|
|
||
| it('400 — missing code rejects with validation error', async () => { |
There was a problem hiding this comment.
Rejection coverage is strong and nicely symmetric across both providers (missing/empty code, missing/empty state, no-cookie, mismatched-cookie, field-level details).
Gap: there's no happy-path assertion — valid code + matching state proceeding past validation (e.g. mocking fetch to assert clearCookie fires / token exchange is attempted). Without it the suite can't catch a regression that wrongly rejects valid callbacks. buildTestApp already wires the prisma/redis mocks, so it's set up for this. Optional for this PR's scope.
| if (!storedState || state !== storedState) { | ||
| return reply.status(400).send({ error: 'Invalid or missing OAuth state — possible CSRF attack' }); | ||
| } | ||
| reply.clearCookie('oauth_state', { path: '/' }); |
| reply.clearCookie('oauth_state', { path: '/' }); | ||
| return reply.status(400).send({ error: 'Invalid callback parameters', details: parsed.error.flatten() }); |
…ails from 400 response
|
Congratulations @ramnnn2006 on getting PR #593 merged! Thank you for your contribution to the project. To receive the appropriate GSSoC labels and recognition, please mention @Harxhit in the #get-labels channel on our Discord server and share your merged PR link. |
…#593) * feat(auth): add Zod validation for OAuth callback endpoints Validates code and state query params in /auth/github/callback and /auth/google/callback before any token exchange or DB work happens. Adds oauthCallbackSchema to validators.ts and tests covering missing/ empty code, missing/empty state, and state cookie mismatch scenarios. * fix(auth): address review feedback on OAuth callback validation - rename oauthCallbackSchema to oAuthCallbackSchema to match naming convention - add .trim() to code and state fields to reject whitespace-only values - export OAuthCallbackQuery type from auth.validation.ts and remove duplicate local interface * fix(auth): clear oauth_state cookie on validation failure and add trailing newline * fix(auth): clear oauth_state cookie on all failure paths and drop details from 400 response
…#593) * feat(auth): add Zod validation for OAuth callback endpoints Validates code and state query params in /auth/github/callback and /auth/google/callback before any token exchange or DB work happens. Adds oauthCallbackSchema to validators.ts and tests covering missing/ empty code, missing/empty state, and state cookie mismatch scenarios. * fix(auth): address review feedback on OAuth callback validation - rename oauthCallbackSchema to oAuthCallbackSchema to match naming convention - add .trim() to code and state fields to reject whitespace-only values - export OAuthCallbackQuery type from auth.validation.ts and remove duplicate local interface * fix(auth): clear oauth_state cookie on validation failure and add trailing newline * fix(auth): clear oauth_state cookie on all failure paths and drop details from 400 response
Summary
Adds Zod validation to
/auth/github/callbackand/auth/google/callbacksocodeandstateare validated before any token exchange or DB calls happen. Previously there were scattered manualif (!code)checks, this replaces them with a singlesafeParseat the top of each handler.Closes #539
Type of Change
What Changed
oauthCallbackSchematoauth.validation.tswithcodeandstateas required non-empty stringsauth.tsto use `safnual guardsauth-callback.test.tswith 14 tests covering missing/empty code, missing/empty state, no cookie, and cookie mismatch for bothendpoints
How to Test
pnpm -r run test— 14 new tests in `auth-callbacGET /auth/github/callbackwith nocodeparam — expect 400 withInvalid callback parameterscodeandstatebut nooauth_statevalid or missing OAuth state — possible CSRF attack`Checklist
pnpm -r run typecheck).pnpm -r run test).console.logor debug statements left in the code.Screenshots / Recordings
N/A