Skip to content

Commit a661f17

Browse files
committed
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.
1 parent e243c8c commit a661f17

4 files changed

Lines changed: 187 additions & 7 deletions

File tree

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
import { describe, it, expect } from 'vitest';
2+
3+
import {
4+
isSafeMobileRedirectUri,
5+
buildOAuthState,
6+
getMobileRedirectUri,
7+
} from '../services/authService.js';
8+
9+
// ─── isSafeMobileRedirectUri ──────────────────────────────────────────────────
10+
11+
describe('isSafeMobileRedirectUri', () => {
12+
it('accepts devcard:// URIs', () => {
13+
expect(isSafeMobileRedirectUri('devcard://oauth/callback')).toBe(true);
14+
expect(isSafeMobileRedirectUri('devcard://')).toBe(true);
15+
});
16+
17+
it('accepts exp:// URIs (Expo Go development)', () => {
18+
expect(isSafeMobileRedirectUri('exp://192.168.1.1:8081')).toBe(true);
19+
expect(isSafeMobileRedirectUri('exp://localhost')).toBe(true);
20+
});
21+
22+
it('rejects plain https:// URIs', () => {
23+
expect(isSafeMobileRedirectUri('https://attacker.com/steal')).toBe(false);
24+
expect(isSafeMobileRedirectUri('https://devcard.dev/auth')).toBe(false);
25+
});
26+
27+
it('rejects http:// URIs', () => {
28+
expect(isSafeMobileRedirectUri('http://localhost:3000')).toBe(false);
29+
});
30+
31+
it('rejects empty strings', () => {
32+
expect(isSafeMobileRedirectUri('')).toBe(false);
33+
});
34+
35+
it('rejects URIs that embed a safe scheme in a path component', () => {
36+
// An attacker-crafted URI that includes "devcard://" somewhere other
37+
// than the start must not be treated as safe.
38+
expect(isSafeMobileRedirectUri('https://evil.com?redirect=devcard://x')).toBe(false);
39+
});
40+
41+
it('rejects javascript: URIs', () => {
42+
expect(isSafeMobileRedirectUri('javascript:alert(1)')).toBe(false);
43+
});
44+
});
45+
46+
// ─── buildOAuthState ──────────────────────────────────────────────────────────
47+
48+
describe('buildOAuthState', () => {
49+
it('returns a random hex string when clientState is empty', () => {
50+
const state = buildOAuthState('', '');
51+
expect(state).toMatch(/^[0-9a-f]{64}$/);
52+
});
53+
54+
it('appends a random nonce to a non-mobile clientState', () => {
55+
const state = buildOAuthState('web_flow', '');
56+
const parts = state.split('.');
57+
expect(parts[0]).toBe('web_flow');
58+
expect(parts[1]).toMatch(/^[0-9a-f]{64}$/);
59+
});
60+
61+
it('embeds a safe mobile redirect URI in the state', () => {
62+
const uri = 'devcard://oauth/callback';
63+
const state = buildOAuthState('mobile_github', uri);
64+
const parts = state.split('.');
65+
expect(parts[0]).toBe('mobile_github');
66+
// Decode the second segment and verify it matches the original URI
67+
const decoded = Buffer.from(parts[1], 'base64url').toString('utf8');
68+
expect(decoded).toBe(uri);
69+
});
70+
71+
it('drops an unsafe mobile redirect URI and omits the embedded segment', () => {
72+
// When the caller supplies an https:// URI the state must not contain
73+
// the encoded form of that URI — the URI segment is skipped entirely.
74+
const state = buildOAuthState('mobile_github', 'https://attacker.com/steal');
75+
const parts = state.split('.');
76+
// With no embedded URI the state is: <clientState>.<nonce>
77+
expect(parts).toHaveLength(2);
78+
expect(parts[0]).toBe('mobile_github');
79+
// The second part must be the random nonce, not a base64url of the bad URI
80+
expect(parts[1]).toMatch(/^[0-9a-f]{64}$/);
81+
});
82+
83+
it('drops an empty mobile redirect URI', () => {
84+
const state = buildOAuthState('mobile_github', '');
85+
const parts = state.split('.');
86+
expect(parts).toHaveLength(2);
87+
expect(parts[0]).toBe('mobile_github');
88+
});
89+
90+
it('generates a unique nonce on every call', () => {
91+
const a = buildOAuthState('mobile_github', 'devcard://oauth/callback');
92+
const b = buildOAuthState('mobile_github', 'devcard://oauth/callback');
93+
// The random nonce component (last segment) must differ
94+
expect(a.split('.').at(-1)).not.toBe(b.split('.').at(-1));
95+
});
96+
});
97+
98+
// ─── getMobileRedirectUri ─────────────────────────────────────────────────────
99+
100+
describe('getMobileRedirectUri', () => {
101+
it('returns null for non-mobile state strings', () => {
102+
expect(getMobileRedirectUri('web_flow.abc123')).toBeNull();
103+
expect(getMobileRedirectUri(undefined)).toBeNull();
104+
expect(getMobileRedirectUri('')).toBeNull();
105+
});
106+
107+
it('returns null when the state has no embedded URI segment', () => {
108+
// A mobile state without an embedded redirect: mobile_x.<nonce>
109+
const nonce = 'a'.repeat(64);
110+
const state = `mobile_github.${nonce}`;
111+
// The second segment is the nonce, not a base64url-encoded URI —
112+
// decoding it yields a non-devcard string, so null is expected.
113+
const result = getMobileRedirectUri(state);
114+
// Either null (failed decode or not a safe scheme) is correct
115+
expect(result === null || !result.startsWith('devcard://')).toBe(true);
116+
});
117+
118+
it('returns the decoded URI for a state built with a safe redirect', () => {
119+
const uri = 'devcard://oauth/callback';
120+
const state = buildOAuthState('mobile_github', uri);
121+
expect(getMobileRedirectUri(state)).toBe(uri);
122+
});
123+
124+
it('returns null when the embedded URI is an https:// URL', () => {
125+
// Simulate a tampered state that encodes a forbidden URI directly,
126+
// bypassing buildOAuthState's validation.
127+
const forbiddenUri = 'https://attacker.com/steal';
128+
const encoded = Buffer.from(forbiddenUri, 'utf8').toString('base64url');
129+
const nonce = 'b'.repeat(64);
130+
const tamperedState = `mobile_github.${encoded}.${nonce}`;
131+
expect(getMobileRedirectUri(tamperedState)).toBeNull();
132+
});
133+
134+
it('returns null when the embedded segment cannot be decoded', () => {
135+
const state = 'mobile_github.!!!invalid_base64!!!.abc';
136+
expect(getMobileRedirectUri(state)).toBeNull();
137+
});
138+
139+
it('returns null for an exp:// URI embedded in a tampered state', () => {
140+
// exp:// is allowed, but this test checks the allowlist works end-to-end
141+
// for Expo Go URIs constructed via buildOAuthState.
142+
const uri = 'exp://192.168.1.42:8081';
143+
const state = buildOAuthState('mobile_github', uri);
144+
expect(getMobileRedirectUri(state)).toBe(uri);
145+
});
146+
});

apps/backend/src/routes/cards.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import { handleDbError } from '../utils/error.util.js';
33
import { createCardSchema, updateCardSchema } from '../utils/validators.js';
44

55
import type { CardResponse } from '../services/cardService';
6-
import type { Card } from '@devcard/shared';
76
import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify';
87

98
interface CreateCardBody {
@@ -69,7 +68,7 @@ export async function cardRoutes(app: FastifyInstance): Promise<void> {
6968

7069
// ─── Create Card ───
7170

72-
app.post('/', async (request: FastifyRequest<{ Body: CreateCardBody }>, reply: FastifyReply): Promise<Card | void> => {
71+
app.post('/', async (request: FastifyRequest<{ Body: CreateCardBody }>, reply: FastifyReply): Promise<CardResponse | void> => {
7372
const userId = (request.user as { id: string }).id;
7473
const parsed = createCardSchema.safeParse(request.body);
7574

@@ -141,4 +140,4 @@ export async function cardRoutes(app: FastifyInstance): Promise<void> {
141140
return handleDbError(error, request, reply)
142141
}
143142
});
144-
}
143+
}

apps/backend/src/services/authService.ts

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,50 @@
1-
import { randomBytes } from 'crypto';
1+
import { randomBytes } from 'node:crypto';
2+
3+
// Schemes that are permitted as mobile OAuth redirect targets.
4+
// The devcard:// custom scheme is the only registered scheme for the
5+
// DevCard mobile app; exp:// covers Expo Go during local development.
6+
// Any URI that does not start with one of these prefixes is rejected
7+
// before it is embedded in the OAuth state or used as a redirect target.
8+
const ALLOWED_MOBILE_SCHEMES = ['devcard://', 'exp://'];
29

310
export function generateState(): string {
411
return randomBytes(32).toString('hex');
512
}
613

14+
/**
15+
* Returns true only when the supplied URI begins with one of the
16+
* registered mobile app schemes. An empty string, a plain HTTPS URL,
17+
* or any other value returns false.
18+
*/
19+
export function isSafeMobileRedirectUri(uri: string): boolean {
20+
return ALLOWED_MOBILE_SCHEMES.some((scheme) => uri.startsWith(scheme));
21+
}
22+
723
export function buildOAuthState(clientState: string, mobileRedirectUri: string): string {
824
if (!clientState) {
925
return generateState();
1026
}
1127

1228
if (clientState.startsWith('mobile_') && mobileRedirectUri) {
29+
// Only embed the redirect URI when it targets a registered app scheme.
30+
// An attacker-supplied https:// URI is silently dropped; the callback
31+
// will fall back to the server-configured MOBILE_REDIRECT_URI instead.
32+
if (!isSafeMobileRedirectUri(mobileRedirectUri)) {
33+
return `${clientState}.${generateState()}`;
34+
}
1335
const encodedRedirect = Buffer.from(mobileRedirectUri, 'utf8').toString('base64url');
1436
return `${clientState}.${encodedRedirect}.${generateState()}`;
1537
}
1638

1739
return `${clientState}.${generateState()}`;
1840
}
1941

42+
/**
43+
* Decodes the mobile redirect URI from the OAuth state string and
44+
* validates it against the scheme allowlist. Returns null when the
45+
* state is not a mobile flow, when the embedded URI is absent, or
46+
* when the decoded URI does not pass the allowlist check.
47+
*/
2048
export function getMobileRedirectUri(state?: string): string | null {
2149
if (!state?.startsWith('mobile_')) {
2250
return null;
@@ -28,8 +56,15 @@ export function getMobileRedirectUri(state?: string): string | null {
2856
}
2957

3058
try {
31-
return Buffer.from(encodedRedirect, 'base64url').toString('utf8');
59+
const decoded = Buffer.from(encodedRedirect, 'base64url').toString('utf8');
60+
// Re-validate on the way out so that a tampered state string (e.g.
61+
// one constructed outside buildOAuthState) cannot slip a forbidden
62+
// URI past the initial check at flow-initiation time.
63+
if (!isSafeMobileRedirectUri(decoded)) {
64+
return null;
65+
}
66+
return decoded;
3267
} catch {
3368
return null;
3469
}
35-
}
70+
}

apps/backend/src/services/cardService.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,4 +171,4 @@ export async function setDefaultCard(app: FastifyInstance, userId: string, id: s
171171
});
172172

173173
return { message: 'Default card updated' };
174-
}
174+
}

0 commit comments

Comments
 (0)