Skip to content

Commit 4d6cdd0

Browse files
brendan-kellambrendan-kellam
committed
Handle OAuth errors returned with successful HTTP status
1 parent e6e530b commit 4d6cdd0

4 files changed

Lines changed: 114 additions & 55 deletions

File tree

‎packages/backend/src/ee/accountPermissionSyncer.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@ const tokenRefreshError = (
1111
});
1212

1313
describe('classifyPermissionSyncFailure', () => {
14-
test('fails closed for invalid_grant', () => {
15-
expect(classifyPermissionSyncFailure(tokenRefreshError('invalid_grant', 400))).toEqual({
14+
test('fails closed when the refresh token is rejected', () => {
15+
expect(classifyPermissionSyncFailure(tokenRefreshError('refresh_token_rejected', 400))).toEqual({
1616
action: 'clear_permissions',
17-
reason: 'oauth_invalid_grant',
17+
reason: 'oauth_refresh_token_rejected',
1818
});
1919
});
2020

‎packages/backend/src/ee/accountPermissionSyncer.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ type AccountPermissionSyncJob = {
3333
}
3434

3535
export type PermissionCleanupReason =
36-
| 'oauth_invalid_grant'
36+
| 'oauth_refresh_token_rejected'
3737
| 'http_unauthorized'
3838
| 'http_forbidden'
3939
| 'http_gone';
@@ -48,19 +48,19 @@ export type PermissionCleanupDecision =
4848
};
4949

5050
const PERMISSION_CLEANUP_REASON_MESSAGES: Record<PermissionCleanupReason, string> = {
51-
oauth_invalid_grant: 'OAuth invalid_grant',
51+
oauth_refresh_token_rejected: 'OAuth refresh token rejection',
5252
http_unauthorized: 'HTTP 401 Unauthorized',
5353
http_forbidden: 'HTTP 403 Forbidden',
5454
http_gone: 'HTTP 410 Gone',
5555
};
5656

5757
export const classifyPermissionSyncFailure = (error: unknown): PermissionCleanupDecision => {
5858
// Token refresh failures have their own classification. Do not fall through
59-
// to the generic HTTP checks because a non-invalid_grant response may also
60-
// carry a 401 or 403 status.
59+
// to the generic HTTP checks because another token endpoint failure may
60+
// also carry a 401 or 403 status.
6161
if (error instanceof TokenRefreshError) {
62-
return error.kind === 'invalid_grant'
63-
? { action: 'clear_permissions', reason: 'oauth_invalid_grant' }
62+
return error.kind === 'refresh_token_rejected'
63+
? { action: 'clear_permissions', reason: 'oauth_refresh_token_rejected' }
6464
: { action: 'preserve_permissions' };
6565
}
6666

‎packages/backend/src/ee/tokenRefresh.test.ts‎

Lines changed: 55 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ describe('exchangeRefreshToken', () => {
103103

104104
expect(error).toBeInstanceOf(TokenRefreshError);
105105
expect(error).toMatchObject({
106-
kind: 'invalid_grant',
106+
kind: 'refresh_token_rejected',
107107
status: 400,
108108
oauthError: 'invalid_grant',
109109
errorDescription: 'The provided refresh_token is invalid',
@@ -139,7 +139,7 @@ describe('exchangeRefreshToken', () => {
139139

140140
expect(error).toBeInstanceOf(TokenRefreshError);
141141
expect(error).toMatchObject({
142-
kind: 'invalid_grant',
142+
kind: 'refresh_token_rejected',
143143
status: 400,
144144
oauthError: 'invalid_grant',
145145
});
@@ -152,6 +152,33 @@ describe('exchangeRefreshToken', () => {
152152
});
153153
});
154154

155+
test('classifies a GitHub bad_refresh_token payload returned with HTTP 200 as a credential rejection', async () => {
156+
const fetchMock = vi.fn().mockResolvedValue(new Response(JSON.stringify({
157+
error: 'bad_refresh_token',
158+
error_description: 'The refresh token passed is incorrect or expired.',
159+
}), {
160+
status: 200,
161+
headers: { 'Content-Type': 'application/json' },
162+
}));
163+
vi.stubGlobal('fetch', fetchMock);
164+
165+
const error = await exchangeRefreshToken(
166+
'github',
167+
'old-refresh-token',
168+
credentials,
169+
).catch(error => error);
170+
171+
expect(error).toBeInstanceOf(TokenRefreshError);
172+
expect(error).toMatchObject({
173+
kind: 'refresh_token_rejected',
174+
status: 200,
175+
oauthError: 'bad_refresh_token',
176+
errorDescription: 'The refresh token passed is incorrect or expired.',
177+
isRetryable: false,
178+
});
179+
expect(fetchMock).toHaveBeenCalledOnce();
180+
});
181+
155182
test('retries a transient HTTP 500 response', async () => {
156183
const fetchMock = vi.fn()
157184
.mockResolvedValueOnce(new Response(JSON.stringify({
@@ -202,6 +229,29 @@ describe('exchangeRefreshToken', () => {
202229
expect(fetchMock).toHaveBeenCalledTimes(3);
203230
});
204231

232+
test('retries a transient OAuth error payload returned with HTTP 200', async () => {
233+
const fetchMock = vi.fn()
234+
.mockResolvedValueOnce(new Response(JSON.stringify({
235+
error: 'server_error',
236+
error_description: 'The token service is temporarily unavailable.',
237+
}), { status: 200 }))
238+
.mockResolvedValueOnce(tokenResponse());
239+
vi.stubGlobal('fetch', fetchMock);
240+
241+
const resultPromise = exchangeRefreshToken(
242+
'github',
243+
'old-refresh-token',
244+
credentials,
245+
);
246+
247+
await vi.advanceTimersByTimeAsync(3000);
248+
249+
await expect(resultPromise).resolves.toMatchObject({
250+
access_token: 'new-access-token',
251+
});
252+
expect(fetchMock).toHaveBeenCalledTimes(2);
253+
});
254+
205255
test('retries a transient network error', async () => {
206256
const fetchMock = vi.fn()
207257
.mockRejectedValueOnce(new TypeError('fetch failed'))
@@ -222,12 +272,12 @@ describe('exchangeRefreshToken', () => {
222272
expect(fetchMock).toHaveBeenCalledTimes(2);
223273
});
224274

225-
test('does not retry a non-invalid_grant OAuth rejection', async () => {
275+
test('does not retry a configuration error payload returned with HTTP 200', async () => {
226276
const fetchMock = vi.fn().mockResolvedValue(new Response(JSON.stringify({
227277
error: 'invalid_client',
228278
error_description: 'Client authentication failed',
229279
}), {
230-
status: 400,
280+
status: 200,
231281
headers: { 'Content-Type': 'application/json' },
232282
}));
233283
vi.stubGlobal('fetch', fetchMock);
@@ -241,7 +291,7 @@ describe('exchangeRefreshToken', () => {
241291
expect(error).toBeInstanceOf(TokenRefreshError);
242292
expect(error).toMatchObject({
243293
kind: 'configuration',
244-
status: 400,
294+
status: 200,
245295
oauthError: 'invalid_client',
246296
isRetryable: false,
247297
});

‎packages/backend/src/ee/tokenRefresh.ts‎

Lines changed: 50 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@ const OAuthErrorResponseSchema = z.object({
4545
error_description: z.string().optional(),
4646
});
4747
type OAuthTokenResponse = z.infer<typeof OAuthTokenResponseSchema>;
48+
type OAuthErrorResponse = z.infer<typeof OAuthErrorResponseSchema>;
4849

4950
export type TokenRefreshErrorKind =
50-
| 'invalid_grant'
51+
| 'refresh_token_rejected'
5152
| 'transient'
5253
| 'configuration'
5354
| 'invalid_response'
@@ -304,10 +305,9 @@ export const exchangeRefreshToken = async (
304305
bodyParams.redirect_uri = new URL('/api/auth/callback/gitlab', env.AUTH_URL).toString();
305306
}
306307

307-
let response: Response | undefined;
308308
for (let attempt = 1; attempt <= TOKEN_REFRESH_MAX_ATTEMPTS; attempt++) {
309309
try {
310-
response = await fetch(url, {
310+
const response = await fetch(url, {
311311
method: 'POST',
312312
headers: {
313313
'Content-Type': 'application/x-www-form-urlencoded',
@@ -320,11 +320,7 @@ export const exchangeRefreshToken = async (
320320
signal: AbortSignal.timeout(TOKEN_REFRESH_TIMEOUT_MS),
321321
});
322322

323-
if (!response.ok) {
324-
throw await classifyTokenRefreshErrorResponse(response, providerType);
325-
}
326-
327-
break;
323+
return await parseTokenRefreshResponse(response, providerType);
328324
} catch (error) {
329325
const classifiedError = error instanceof TokenRefreshError
330326
? error
@@ -342,22 +338,43 @@ export const exchangeRefreshToken = async (
342338
}
343339
}
344340

345-
if (!response) {
346-
throw new TokenRefreshError(`${providerType} token refresh produced no response.`, {
347-
kind: 'invalid_response',
348-
});
349-
}
341+
throw new TokenRefreshError(`${providerType} token refresh produced no response.`, {
342+
kind: 'invalid_response',
343+
});
344+
};
350345

346+
const parseTokenRefreshResponse = async (
347+
response: Response,
348+
providerType: SupportedProviderType,
349+
): Promise<OAuthTokenResponse> => {
350+
const responseText = await response.text();
351351
let json: unknown;
352352
try {
353-
json = await response.json();
353+
json = JSON.parse(responseText);
354354
} catch (error) {
355+
if (!response.ok) {
356+
throw classifyTokenRefreshErrorResponse(response.status, providerType);
357+
}
358+
355359
throw new TokenRefreshError(`${providerType} returned a non-JSON token response.`, {
356360
kind: 'invalid_response',
357361
cause: error,
358362
});
359363
}
360364

365+
const oauthErrorResult = OAuthErrorResponseSchema.safeParse(json);
366+
if (oauthErrorResult.success) {
367+
throw classifyTokenRefreshErrorResponse(
368+
response.status,
369+
providerType,
370+
oauthErrorResult.data,
371+
);
372+
}
373+
374+
if (!response.ok) {
375+
throw classifyTokenRefreshErrorResponse(response.status, providerType);
376+
}
377+
361378
const result = OAuthTokenResponseSchema.safeParse(json);
362379

363380
if (!result.success) {
@@ -387,58 +404,50 @@ const classifyTokenRefreshFetchError = (
387404
);
388405
};
389406

390-
const classifyTokenRefreshErrorResponse = async (
391-
response: Response,
407+
const classifyTokenRefreshErrorResponse = (
408+
status: number,
392409
providerType: SupportedProviderType,
393-
): Promise<TokenRefreshError> => {
394-
let oauthError: string | undefined;
395-
let errorDescription: string | undefined;
396-
397-
try {
398-
const responseText = await response.text();
399-
const result = OAuthErrorResponseSchema.safeParse(JSON.parse(responseText));
400-
if (result.success) {
401-
oauthError = result.data.error;
402-
errorDescription = result.data.error_description;
403-
}
404-
} catch {
405-
// Non-JSON and malformed OAuth errors are still classified by HTTP status.
406-
}
407-
410+
oauthErrorResponse?: OAuthErrorResponse,
411+
): TokenRefreshError => {
412+
const oauthError = oauthErrorResponse?.error;
413+
const errorDescription = oauthErrorResponse?.error_description;
408414
const details = errorDescription ? `: ${errorDescription}` : '';
415+
const isRefreshTokenRejected =
416+
oauthError === 'invalid_grant' ||
417+
(providerType === 'github' && oauthError === 'bad_refresh_token');
409418

410-
if (oauthError === 'invalid_grant') {
419+
if (isRefreshTokenRejected) {
411420
return new TokenRefreshError(`${providerType} rejected the OAuth refresh token${details}`, {
412-
kind: 'invalid_grant',
413-
status: response.status,
421+
kind: 'refresh_token_rejected',
422+
status,
414423
oauthError,
415424
errorDescription,
416425
});
417426
}
418427

419428
if (
420-
response.status === 408 ||
421-
response.status === 429 ||
422-
response.status >= 500 ||
429+
status === 408 ||
430+
status === 429 ||
431+
status >= 500 ||
423432
oauthError === 'server_error' ||
424433
oauthError === 'temporarily_unavailable'
425434
) {
426435
return new TokenRefreshError(
427-
`${providerType} token endpoint is temporarily unavailable (HTTP ${response.status})${details}`,
436+
`${providerType} token endpoint is temporarily unavailable (HTTP ${status})${details}`,
428437
{
429438
kind: 'transient',
430-
status: response.status,
439+
status,
431440
oauthError,
432441
errorDescription,
433442
},
434443
);
435444
}
436445

437446
return new TokenRefreshError(
438-
`${providerType} token endpoint rejected the refresh request (HTTP ${response.status})${details}`,
447+
`${providerType} token endpoint rejected the refresh request (HTTP ${status})${details}`,
439448
{
440449
kind: 'configuration',
441-
status: response.status,
450+
status,
442451
oauthError,
443452
errorDescription,
444453
},

0 commit comments

Comments
 (0)