Skip to content

Commit c1aa435

Browse files
heskewheskew
authored
Merge pull request #28 from HarperFast/callback-bugfix
better handle provider errors on callback
2 parents 1ea0809 + 8eb2dc3 commit c1aa435

File tree

4 files changed

+402
-31
lines changed

4 files changed

+402
-31
lines changed

‎src/lib/OAuthProvider.ts‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,20 @@ export class OAuthProvider implements IOAuthProvider {
109109
});
110110

111111
if (!response.ok) {
112-
const errorText = await response.text();
113-
throw new Error(`Token exchange failed: ${errorText}`);
112+
const contentType = response.headers.get('content-type');
113+
if (contentType?.includes('application/json')) {
114+
try {
115+
const errorBody = await response.json();
116+
const detail = errorBody.error_description || errorBody.error || response.statusText;
117+
throw new Error(`Token exchange failed: ${detail}`);
118+
} catch (parseError) {
119+
if (parseError instanceof Error && parseError.message.startsWith('Token exchange failed')) throw parseError;
120+
// JSON parse failed despite content-type header — fall through to generic error
121+
}
122+
}
123+
// Drain unconsumed body to free the underlying socket (undici connection pool)
124+
await response.body?.cancel();
125+
throw new Error(`Token exchange failed: provider returned ${response.status} ${response.statusText}`);
114126
}
115127

116128
const contentType = response.headers.get('content-type');
@@ -366,13 +378,24 @@ export class OAuthProvider implements IOAuthProvider {
366378
});
367379

368380
if (!response.ok) {
369-
const error = await response.text();
381+
const contentType = response.headers.get('content-type');
382+
let detail: string = `${response.status} ${response.statusText}`;
383+
if (contentType?.includes('application/json')) {
384+
try {
385+
const errorBody = await response.json();
386+
detail = errorBody.error_description || errorBody.error || response.statusText;
387+
} catch {
388+
// JSON parse failed despite content-type header — use status/statusText
389+
}
390+
}
391+
// Drain unconsumed body to free the underlying socket (undici connection pool)
392+
await response.body?.cancel();
370393
this.logger?.error?.('Token refresh HTTP error:', {
371394
status: response.status,
372395
statusText: response.statusText,
373-
body: error,
396+
detail,
374397
});
375-
throw new Error(`Token refresh failed: ${error}`);
398+
throw new Error(`Token refresh failed: ${detail}`);
376399
}
377400

378401
const contentType = response.headers.get('content-type');

‎src/lib/handlers.ts‎

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,21 @@ export function sanitizeRedirect(redirectParam: string): string {
5555
}
5656
}
5757

58+
/**
59+
* Build a safe error redirect URL
60+
*
61+
* Sanitizes the redirect path, then appends error query params using the URL API
62+
* so params are always placed before any hash fragment.
63+
*/
64+
function buildErrorRedirect(rawUrl: string, params: Record<string, string>): string {
65+
const safePath = sanitizeRedirect(rawUrl);
66+
const url = new URL(safePath, 'http://localhost');
67+
for (const [key, value] of Object.entries(params)) {
68+
url.searchParams.set(key, value);
69+
}
70+
return url.pathname + url.search + url.hash;
71+
}
72+
5873
/**
5974
* Handle OAuth login initiation
6075
*/
@@ -74,7 +89,8 @@ export async function handleLogin(
7489
redirectParam = sanitizeRedirect(redirectParam);
7590
}
7691

77-
const originalUrl = redirectParam || request.headers?.referer || config.postLoginRedirect || '/';
92+
const referer = request.headers?.referer ? sanitizeRedirect(request.headers.referer) : undefined;
93+
const originalUrl = redirectParam || referer || config.postLoginRedirect || '/';
7894

7995
// Generate CSRF token with metadata
8096
// Bind token to provider to prevent cross-provider CSRF attacks
@@ -118,9 +134,7 @@ export async function handleCallback(
118134
// Handle OAuth errors from provider
119135
if (error) {
120136
logger?.error?.(`OAuth error: ${error} - ${errorDescription}`);
121-
// Redirect to original URL or fallback with error
122-
const fallbackUrl = config.postLoginRedirect || '/';
123-
const errorUrl = `${fallbackUrl}${fallbackUrl.includes('?') ? '&' : '?'}error=oauth_failed&reason=${encodeURIComponent(error)}`;
137+
const errorUrl = buildErrorRedirect(config.postLoginRedirect || '/', { error: 'oauth_failed', reason: error });
124138
return {
125139
status: 302,
126140
headers: {
@@ -132,8 +146,7 @@ export async function handleCallback(
132146
// Validate parameters
133147
if (!code || !state) {
134148
logger?.warn?.('Missing required OAuth callback parameters');
135-
const fallbackUrl = config.postLoginRedirect || '/';
136-
const errorUrl = `${fallbackUrl}${fallbackUrl.includes('?') ? '&' : '?'}error=invalid_request`;
149+
const errorUrl = buildErrorRedirect(config.postLoginRedirect || '/', { error: 'invalid_request' });
137150
return {
138151
status: 302,
139152
headers: {
@@ -163,8 +176,10 @@ export async function handleCallback(
163176
);
164177
// This could be an attack - redirect back to original URL with error
165178
// Do NOT redirect to login endpoint as that would restart OAuth flow
166-
const redirectUrl = tokenData.originalUrl || config.postLoginRedirect || '/';
167-
const errorUrl = `${redirectUrl}${redirectUrl.includes('?') ? '&' : '?'}error=auth_failed&reason=csrf`;
179+
const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', {
180+
error: 'auth_failed',
181+
reason: 'csrf',
182+
});
168183
return {
169184
status: 302,
170185
headers: {
@@ -257,20 +272,30 @@ export async function handleCallback(
257272
logger?.warn?.('No session available for OAuth user');
258273
}
259274

260-
// Redirect to original URL or default
275+
// Redirect to original URL or default (sanitize to prevent open redirect)
261276
return {
262277
status: 302,
263278
headers: {
264-
Location: tokenData.originalUrl || config.postLoginRedirect || '/',
279+
Location: sanitizeRedirect(tokenData.originalUrl || config.postLoginRedirect || '/'),
265280
},
266281
};
267282
} catch (error) {
268283
logger?.error?.('OAuth callback error:', error);
284+
// Use a safe, generic reason code — details are in the server log
285+
const message = (error as Error).message || '';
286+
let reason = 'unknown';
287+
if (message.startsWith('Token exchange failed')) reason = 'token_exchange';
288+
else if (message.includes('claim')) reason = 'user_mapping';
289+
else if (message.includes('user info') || message.includes('userinfo')) reason = 'user_info';
290+
else if (message.includes('hook') || message.includes('onLogin')) reason = 'login_hook';
291+
const errorUrl = buildErrorRedirect(tokenData.originalUrl || config.postLoginRedirect || '/', {
292+
error: 'auth_failed',
293+
reason,
294+
});
269295
return {
270-
status: 500,
271-
body: {
272-
error: 'Authentication failed',
273-
message: (error as Error).message,
296+
status: 302,
297+
headers: {
298+
Location: errorUrl,
274299
},
275300
};
276301
}

‎test/lib/OAuthProvider.test.js‎

Lines changed: 175 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -331,17 +331,137 @@ describe('OAuthProvider', () => {
331331
}
332332
});
333333

334-
it('should throw on token exchange failure', async () => {
334+
it('should throw with sanitized message on token exchange failure', async () => {
335335
const originalFetch = global.fetch;
336336

337337
global.fetch = async () => ({
338338
ok: false,
339-
text: async () => 'Invalid client credentials',
339+
status: 401,
340+
statusText: 'Unauthorized',
341+
headers: { get: () => null },
342+
});
343+
344+
try {
345+
await assert.rejects(async () => await provider.exchangeCodeForToken('code', 'https://callback'), {
346+
message: /Token exchange failed.*401 Unauthorized/i,
347+
});
348+
} finally {
349+
global.fetch = originalFetch;
350+
}
351+
});
352+
353+
it('should extract error_description from JSON error response', async () => {
354+
const originalFetch = global.fetch;
355+
356+
global.fetch = async () => ({
357+
ok: false,
358+
status: 400,
359+
statusText: 'Bad Request',
360+
headers: { get: (h) => (h === 'content-type' ? 'application/json' : null) },
361+
json: async () => ({
362+
error: 'invalid_client',
363+
error_description: 'The client credentials are invalid',
364+
}),
365+
});
366+
367+
try {
368+
await assert.rejects(async () => await provider.exchangeCodeForToken('code', 'https://callback'), {
369+
message: /Token exchange failed.*The client credentials are invalid/i,
370+
});
371+
} finally {
372+
global.fetch = originalFetch;
373+
}
374+
});
375+
376+
it('should not leak HTML error pages in token exchange errors', async () => {
377+
const originalFetch = global.fetch;
378+
379+
global.fetch = async () => ({
380+
ok: false,
381+
status: 500,
382+
statusText: 'Internal Server Error',
383+
headers: { get: (h) => (h === 'content-type' ? 'text/html' : null) },
384+
});
385+
386+
try {
387+
await assert.rejects(async () => await provider.exchangeCodeForToken('code', 'https://callback'), {
388+
message: /Token exchange failed.*500 Internal Server Error/i,
389+
});
390+
} finally {
391+
global.fetch = originalFetch;
392+
}
393+
});
394+
395+
it('should drain response body on non-JSON error to prevent socket leak', async () => {
396+
const originalFetch = global.fetch;
397+
let bodyCancelled = false;
398+
399+
global.fetch = async () => ({
400+
ok: false,
401+
status: 500,
402+
statusText: 'Internal Server Error',
403+
headers: { get: (h) => (h === 'content-type' ? 'text/html' : null) },
404+
body: {
405+
cancel: async () => {
406+
bodyCancelled = true;
407+
},
408+
},
409+
});
410+
411+
try {
412+
await assert.rejects(async () => await provider.exchangeCodeForToken('code', 'https://callback'), {
413+
message: /Token exchange failed/,
414+
});
415+
assert.ok(bodyCancelled, 'response.body.cancel() should have been called');
416+
} finally {
417+
global.fetch = originalFetch;
418+
}
419+
});
420+
});
421+
422+
describe('JSON Parse Safety', () => {
423+
beforeEach(() => {
424+
provider = new OAuthProvider(mockConfig, mockLogger);
425+
});
426+
427+
it('should fall back to status when JSON parse fails in token exchange', async () => {
428+
const originalFetch = global.fetch;
429+
430+
global.fetch = async () => ({
431+
ok: false,
432+
status: 502,
433+
statusText: 'Bad Gateway',
434+
headers: { get: (h) => (h === 'content-type' ? 'application/json' : null) },
435+
json: async () => {
436+
throw new SyntaxError('Unexpected end of JSON input');
437+
},
340438
});
341439

342440
try {
343441
await assert.rejects(async () => await provider.exchangeCodeForToken('code', 'https://callback'), {
344-
message: /Token exchange failed.*Invalid client credentials/i,
442+
message: /Token exchange failed.*502 Bad Gateway/i,
443+
});
444+
} finally {
445+
global.fetch = originalFetch;
446+
}
447+
});
448+
449+
it('should fall back to status when JSON parse fails in token refresh', async () => {
450+
const originalFetch = global.fetch;
451+
452+
global.fetch = async () => ({
453+
ok: false,
454+
status: 502,
455+
statusText: 'Bad Gateway',
456+
headers: { get: (h) => (h === 'content-type' ? 'application/json' : null) },
457+
json: async () => {
458+
throw new SyntaxError('Unexpected end of JSON input');
459+
},
460+
});
461+
462+
try {
463+
await assert.rejects(async () => await provider.refreshAccessToken('bad-token'), {
464+
message: /Token refresh failed.*502 Bad Gateway/i,
345465
});
346466
} finally {
347467
global.fetch = originalFetch;
@@ -478,19 +598,68 @@ describe('OAuthProvider', () => {
478598
}
479599
});
480600

481-
it('should throw on HTTP error during token refresh', async () => {
601+
it('should throw with sanitized message on HTTP error during token refresh', async () => {
482602
const originalFetch = global.fetch;
483603

484604
global.fetch = async () => ({
485605
ok: false,
486606
status: 400,
487607
statusText: 'Bad Request',
488-
text: async () => 'invalid_grant: The refresh token is invalid',
608+
headers: { get: () => null },
609+
});
610+
611+
try {
612+
await assert.rejects(async () => await provider.refreshAccessToken('bad-token'), {
613+
message: /Token refresh failed.*400 Bad Request/i,
614+
});
615+
} finally {
616+
global.fetch = originalFetch;
617+
}
618+
});
619+
620+
it('should drain response body on error to prevent socket leak', async () => {
621+
const originalFetch = global.fetch;
622+
let bodyCancelled = false;
623+
624+
global.fetch = async () => ({
625+
ok: false,
626+
status: 502,
627+
statusText: 'Bad Gateway',
628+
headers: { get: () => null },
629+
body: {
630+
cancel: async () => {
631+
bodyCancelled = true;
632+
},
633+
},
634+
});
635+
636+
try {
637+
await assert.rejects(async () => await provider.refreshAccessToken('bad-token'), {
638+
message: /Token refresh failed/,
639+
});
640+
assert.ok(bodyCancelled, 'response.body.cancel() should have been called');
641+
} finally {
642+
global.fetch = originalFetch;
643+
}
644+
});
645+
646+
it('should extract error_description from JSON error during token refresh', async () => {
647+
const originalFetch = global.fetch;
648+
649+
global.fetch = async () => ({
650+
ok: false,
651+
status: 400,
652+
statusText: 'Bad Request',
653+
headers: { get: (h) => (h === 'content-type' ? 'application/json' : null) },
654+
json: async () => ({
655+
error: 'invalid_grant',
656+
error_description: 'The refresh token is invalid',
657+
}),
489658
});
490659

491660
try {
492661
await assert.rejects(async () => await provider.refreshAccessToken('bad-token'), {
493-
message: /Token refresh failed.*invalid_grant/i,
662+
message: /Token refresh failed.*The refresh token is invalid/i,
494663
});
495664
} finally {
496665
global.fetch = originalFetch;

0 commit comments

Comments
 (0)