From b69d55dc9e624465cba5789bdbffca3dee736022 Mon Sep 17 00:00:00 2001 From: Ridanshi Date: Tue, 9 Jun 2026 01:20:48 +0530 Subject: [PATCH 1/2] fix(auth): validate mobile OAuth redirect URI against scheme allowlist Add isSafeMobileRedirectUri() to validate the embedded redirect URI against an explicit scheme allowlist before embedding in or extracting from OAuth state. Resolves lint and typecheck errors in cards route and auth service. --- apps/backend/package.json | 2 +- .../backend/src/__tests__/authService.test.ts | 146 ++++++++++++++++++ apps/backend/src/routes/cards.ts | 3 +- apps/backend/src/services/authService.ts | 39 ++++- 4 files changed, 185 insertions(+), 5 deletions(-) create mode 100644 apps/backend/src/__tests__/authService.test.ts diff --git a/apps/backend/package.json b/apps/backend/package.json index d71b0777..22a1f833 100644 --- a/apps/backend/package.json +++ b/apps/backend/package.json @@ -52,4 +52,4 @@ "typescript-eslint": "^8.59.3", "vitest": "^2.0.0" } -} \ No newline at end of file +} diff --git a/apps/backend/src/__tests__/authService.test.ts b/apps/backend/src/__tests__/authService.test.ts new file mode 100644 index 00000000..804ca9a2 --- /dev/null +++ b/apps/backend/src/__tests__/authService.test.ts @@ -0,0 +1,146 @@ +import { describe, it, expect } from 'vitest'; + +import { + isSafeMobileRedirectUri, + buildOAuthState, + getMobileRedirectUri, +} from '../services/authService.js'; + +// ─── isSafeMobileRedirectUri ────────────────────────────────────────────────── + +describe('isSafeMobileRedirectUri', () => { + it('accepts devcard:// URIs', () => { + expect(isSafeMobileRedirectUri('devcard://oauth/callback')).toBe(true); + expect(isSafeMobileRedirectUri('devcard://')).toBe(true); + }); + + it('accepts exp:// URIs (Expo Go development)', () => { + expect(isSafeMobileRedirectUri('exp://192.168.1.1:8081')).toBe(true); + expect(isSafeMobileRedirectUri('exp://localhost')).toBe(true); + }); + + it('rejects plain https:// URIs', () => { + expect(isSafeMobileRedirectUri('https://attacker.com/steal')).toBe(false); + expect(isSafeMobileRedirectUri('https://devcard.dev/auth')).toBe(false); + }); + + it('rejects http:// URIs', () => { + expect(isSafeMobileRedirectUri('http://localhost:3000')).toBe(false); + }); + + it('rejects empty strings', () => { + expect(isSafeMobileRedirectUri('')).toBe(false); + }); + + it('rejects URIs that embed a safe scheme in a path component', () => { + // An attacker-crafted URI that includes "devcard://" somewhere other + // than the start must not be treated as safe. + expect(isSafeMobileRedirectUri('https://evil.com?redirect=devcard://x')).toBe(false); + }); + + it('rejects javascript: URIs', () => { + expect(isSafeMobileRedirectUri('javascript:alert(1)')).toBe(false); + }); +}); + +// ─── buildOAuthState ────────────────────────────────────────────────────────── + +describe('buildOAuthState', () => { + it('returns a random hex string when clientState is empty', () => { + const state = buildOAuthState('', ''); + expect(state).toMatch(/^[0-9a-f]{64}$/); + }); + + it('appends a random nonce to a non-mobile clientState', () => { + const state = buildOAuthState('web_flow', ''); + const parts = state.split('.'); + expect(parts[0]).toBe('web_flow'); + expect(parts[1]).toMatch(/^[0-9a-f]{64}$/); + }); + + it('embeds a safe mobile redirect URI in the state', () => { + const uri = 'devcard://oauth/callback'; + const state = buildOAuthState('mobile_github', uri); + const parts = state.split('.'); + expect(parts[0]).toBe('mobile_github'); + // Decode the second segment and verify it matches the original URI + const decoded = Buffer.from(parts[1], 'base64url').toString('utf8'); + expect(decoded).toBe(uri); + }); + + it('drops an unsafe mobile redirect URI and omits the embedded segment', () => { + // When the caller supplies an https:// URI the state must not contain + // the encoded form of that URI — the URI segment is skipped entirely. + const state = buildOAuthState('mobile_github', 'https://attacker.com/steal'); + const parts = state.split('.'); + // With no embedded URI the state is: . + expect(parts).toHaveLength(2); + expect(parts[0]).toBe('mobile_github'); + // The second part must be the random nonce, not a base64url of the bad URI + expect(parts[1]).toMatch(/^[0-9a-f]{64}$/); + }); + + it('drops an empty mobile redirect URI', () => { + const state = buildOAuthState('mobile_github', ''); + const parts = state.split('.'); + expect(parts).toHaveLength(2); + expect(parts[0]).toBe('mobile_github'); + }); + + it('generates a unique nonce on every call', () => { + const a = buildOAuthState('mobile_github', 'devcard://oauth/callback'); + const b = buildOAuthState('mobile_github', 'devcard://oauth/callback'); + // The random nonce component (last segment) must differ + expect(a.split('.').at(-1)).not.toBe(b.split('.').at(-1)); + }); +}); + +// ─── getMobileRedirectUri ───────────────────────────────────────────────────── + +describe('getMobileRedirectUri', () => { + it('returns null for non-mobile state strings', () => { + expect(getMobileRedirectUri('web_flow.abc123')).toBeNull(); + expect(getMobileRedirectUri(undefined)).toBeNull(); + expect(getMobileRedirectUri('')).toBeNull(); + }); + + it('returns null when the state has no embedded URI segment', () => { + // A mobile state without an embedded redirect: mobile_x. + const nonce = 'a'.repeat(64); + const state = `mobile_github.${nonce}`; + // The second segment is the nonce, not a base64url-encoded URI — + // decoding it yields a non-devcard string, so null is expected. + const result = getMobileRedirectUri(state); + // Either null (failed decode or not a safe scheme) is correct + expect(result === null || !result.startsWith('devcard://')).toBe(true); + }); + + it('returns the decoded URI for a state built with a safe redirect', () => { + const uri = 'devcard://oauth/callback'; + const state = buildOAuthState('mobile_github', uri); + expect(getMobileRedirectUri(state)).toBe(uri); + }); + + it('returns null when the embedded URI is an https:// URL', () => { + // Simulate a tampered state that encodes a forbidden URI directly, + // bypassing buildOAuthState's validation. + const forbiddenUri = 'https://attacker.com/steal'; + const encoded = Buffer.from(forbiddenUri, 'utf8').toString('base64url'); + const nonce = 'b'.repeat(64); + const tamperedState = `mobile_github.${encoded}.${nonce}`; + expect(getMobileRedirectUri(tamperedState)).toBeNull(); + }); + + it('returns null when the embedded segment cannot be decoded', () => { + const state = 'mobile_github.!!!invalid_base64!!!.abc'; + expect(getMobileRedirectUri(state)).toBeNull(); + }); + + it('returns null for an exp:// URI embedded in a tampered state', () => { + // exp:// is allowed, but this test checks the allowlist works end-to-end + // for Expo Go URIs constructed via buildOAuthState. + const uri = 'exp://192.168.1.42:8081'; + const state = buildOAuthState('mobile_github', uri); + expect(getMobileRedirectUri(state)).toBe(uri); + }); +}); diff --git a/apps/backend/src/routes/cards.ts b/apps/backend/src/routes/cards.ts index e5f98762..4281df20 100644 --- a/apps/backend/src/routes/cards.ts +++ b/apps/backend/src/routes/cards.ts @@ -3,7 +3,6 @@ import { handleDbError } from '../utils/error.util.js'; import { createCardSchema, updateCardSchema } from '../utils/validators.js'; import type { CardResponse } from '../services/cardService'; -import type { Card } from '@devcard/shared'; import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify'; interface CreateCardBody { @@ -69,7 +68,7 @@ export async function cardRoutes(app: FastifyInstance): Promise { // ─── Create Card ─── - app.post('/', async (request: FastifyRequest<{ Body: CreateCardBody }>, reply: FastifyReply): Promise => { + app.post('/', async (request: FastifyRequest<{ Body: CreateCardBody }>, reply: FastifyReply): Promise => { const userId = (request.user as { id: string }).id; const parsed = createCardSchema.safeParse(request.body); diff --git a/apps/backend/src/services/authService.ts b/apps/backend/src/services/authService.ts index 9af718c5..9cf3192a 100644 --- a/apps/backend/src/services/authService.ts +++ b/apps/backend/src/services/authService.ts @@ -1,15 +1,37 @@ -import { randomBytes } from 'crypto'; +import { randomBytes } from 'node:crypto'; + +// Schemes that are permitted as mobile OAuth redirect targets. +// The devcard:// custom scheme is the only registered scheme for the +// DevCard mobile app; exp:// covers Expo Go during local development. +// Any URI that does not start with one of these prefixes is rejected +// before it is embedded in the OAuth state or used as a redirect target. +const ALLOWED_MOBILE_SCHEMES = ['devcard://', 'exp://']; export function generateState(): string { return randomBytes(32).toString('hex'); } +/** + * Returns true only when the supplied URI begins with one of the + * registered mobile app schemes. An empty string, a plain HTTPS URL, + * or any other value returns false. + */ +export function isSafeMobileRedirectUri(uri: string): boolean { + return ALLOWED_MOBILE_SCHEMES.some((scheme) => uri.startsWith(scheme)); +} + export function buildOAuthState(clientState: string, mobileRedirectUri: string): string { if (!clientState) { return generateState(); } if (clientState.startsWith('mobile_') && mobileRedirectUri) { + // Only embed the redirect URI when it targets a registered app scheme. + // An attacker-supplied https:// URI is silently dropped; the callback + // will fall back to the server-configured MOBILE_REDIRECT_URI instead. + if (!isSafeMobileRedirectUri(mobileRedirectUri)) { + return `${clientState}.${generateState()}`; + } const encodedRedirect = Buffer.from(mobileRedirectUri, 'utf8').toString('base64url'); return `${clientState}.${encodedRedirect}.${generateState()}`; } @@ -17,6 +39,12 @@ export function buildOAuthState(clientState: string, mobileRedirectUri: string): return `${clientState}.${generateState()}`; } +/** + * Decodes the mobile redirect URI from the OAuth state string and + * validates it against the scheme allowlist. Returns null when the + * state is not a mobile flow, when the embedded URI is absent, or + * when the decoded URI does not pass the allowlist check. + */ export function getMobileRedirectUri(state?: string): string | null { if (!state?.startsWith('mobile_')) { return null; @@ -28,7 +56,14 @@ export function getMobileRedirectUri(state?: string): string | null { } try { - return Buffer.from(encodedRedirect, 'base64url').toString('utf8'); + const decoded = Buffer.from(encodedRedirect, 'base64url').toString('utf8'); + // Re-validate on the way out so that a tampered state string (e.g. + // one constructed outside buildOAuthState) cannot slip a forbidden + // URI past the initial check at flow-initiation time. + if (!isSafeMobileRedirectUri(decoded)) { + return null; + } + return decoded; } catch { return null; } From b307e326bd65cdb1e689282e7f1a9dabf38b0983 Mon Sep 17 00:00:00 2001 From: Ridanshi Date: Wed, 10 Jun 2026 00:58:28 +0530 Subject: [PATCH 2/2] chore: replace stale pnpm references with npm equivalents The repository migrated from pnpm workspaces to npm in 4071cbc but several files were not updated at that time: - .github/pull_request_template.md: checklist items still referenced `pnpm -r run lint/typecheck/test`; replaced with the npm --prefix equivalents used by CI. - .gitignore: removed pnpm-debug.log* (pnpm is no longer installed). - apps/web/.gitignore: same pnpm-debug.log* removal. - apps/mobile/jest.config.js: removed the \.pnpm/ virtual-store branch from the transformIgnorePatterns regex (dead code under npm). --- .github/pull_request_template.md | 6 +++--- .gitignore | 1 - apps/mobile/jest.config.js | 2 +- apps/web/.gitignore | 1 - 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 6c10ac89..1816af86 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -43,10 +43,10 @@ Closes # ## Checklist -- [ ] My code follows the project's coding style (`pnpm -r run lint` passes). -- [ ] TypeScript compiles without errors (`pnpm -r run typecheck`). +- [ ] My code follows the project's coding style (`npm --prefix apps/backend run lint` passes). +- [ ] TypeScript compiles without errors (`npm --prefix apps/backend run typecheck`). - [ ] I have added or updated tests for the changes I made. -- [ ] All tests pass locally (`pnpm -r run test`). +- [ ] All tests pass locally (`npm --prefix apps/backend run test`). - [ ] I have updated documentation where necessary. - [ ] No new `console.log` or debug statements left in the code. - [ ] Breaking changes are documented in this PR description. diff --git a/.gitignore b/.gitignore index a276f12a..67853e88 100644 --- a/.gitignore +++ b/.gitignore @@ -45,6 +45,5 @@ coverage/ *.log npm-debug.log* yarn-debug.log* -pnpm-debug.log* .cache/ tmp/ diff --git a/apps/mobile/jest.config.js b/apps/mobile/jest.config.js index 59cb29d8..56ed8283 100644 --- a/apps/mobile/jest.config.js +++ b/apps/mobile/jest.config.js @@ -2,6 +2,6 @@ module.exports = { preset: 'react-native', setupFiles: ['/jest.setup.js'], transformIgnorePatterns: [ - 'node_modules/(?!((react-native|@react-native|@react-navigation|@gorhom)/|\\.pnpm/(react-native|@react-native|@react-navigation|@gorhom)[^/]*))', + 'node_modules/(?!((react-native|@react-native|@react-navigation|@gorhom)/))', ], }; diff --git a/apps/web/.gitignore b/apps/web/.gitignore index a547bf36..7a181037 100644 --- a/apps/web/.gitignore +++ b/apps/web/.gitignore @@ -4,7 +4,6 @@ logs npm-debug.log* yarn-debug.log* yarn-error.log* -pnpm-debug.log* lerna-debug.log* node_modules