diff --git a/CHANGELOG.md b/CHANGELOG.md index c2382fb1d..4678376f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - [EE] Preserved cached repository permissions when OAuth token refresh fails because of a transient code host outage. [#1481](https://github.com/sourcebot-dev/sourcebot/pull/1481) +- [EE] Classified code host permission sync failures by provider context before clearing cached repository permissions. [#1482](https://github.com/sourcebot-dev/sourcebot/pull/1482) ## [5.1.3] - 2026-07-20 diff --git a/packages/backend/src/ee/accountPermissionSyncer.test.ts b/packages/backend/src/ee/accountPermissionSyncer.test.ts index b2499e6e8..9636b34c9 100644 --- a/packages/backend/src/ee/accountPermissionSyncer.test.ts +++ b/packages/backend/src/ee/accountPermissionSyncer.test.ts @@ -1,5 +1,9 @@ import { describe, expect, test } from 'vitest'; import { classifyPermissionSyncFailure } from './accountPermissionSyncer.js'; +import { + PermissionSyncUpstreamError, + type PermissionSyncUpstreamErrorKind, +} from './permissionSyncError.js'; import { TokenRefreshError, type TokenRefreshErrorKind } from './tokenRefresh.js'; const tokenRefreshError = ( @@ -10,6 +14,14 @@ const tokenRefreshError = ( status, }); +const upstreamError = ( + kind: PermissionSyncUpstreamErrorKind, +): PermissionSyncUpstreamError => new PermissionSyncUpstreamError(`Permission sync failed: ${kind}`, { + kind, + provider: 'github', + operation: 'list_accessible_repositories', +}); + describe('classifyPermissionSyncFailure', () => { test('fails closed for invalid_grant', () => { expect(classifyPermissionSyncFailure(tokenRefreshError('invalid_grant', 400))).toEqual({ @@ -36,19 +48,28 @@ describe('classifyPermissionSyncFailure', () => { }); test.each([ - [401, 'http_unauthorized'], - [403, 'http_forbidden'], - [410, 'http_gone'], - ] as const)('preserves fail-closed behavior for an API HTTP %s response', (status, reason) => { - const error = Object.assign(new Error(reason), { status }); - expect(classifyPermissionSyncFailure(error)).toEqual({ + ['credential_rejected', 'upstream_credential_rejected'], + ['insufficient_scope', 'upstream_insufficient_scope'], + ] as const)('fails closed for a classified %s upstream failure', (kind, reason) => { + expect(classifyPermissionSyncFailure(upstreamError(kind))).toEqual({ action: 'clear_permissions', reason, }); }); - test('keeps permissions for an unrelated API failure', () => { - const error = Object.assign(new Error('Internal Server Error'), { status: 500 }); + test.each([ + 'rate_limited', + 'upstream_unavailable', + 'forbidden', + 'unknown', + ] satisfies PermissionSyncUpstreamErrorKind[])('keeps permissions for a classified %s upstream failure', (kind) => { + expect(classifyPermissionSyncFailure(upstreamError(kind))).toEqual({ + action: 'preserve_permissions', + }); + }); + + test.each([401, 403, 410])('does not fail closed on an unclassified HTTP %s error', (status) => { + const error = Object.assign(new Error(`HTTP ${status}`), { status }); expect(classifyPermissionSyncFailure(error)).toEqual({ action: 'preserve_permissions', }); diff --git a/packages/backend/src/ee/accountPermissionSyncer.ts b/packages/backend/src/ee/accountPermissionSyncer.ts index 382d773fe..b507051d0 100644 --- a/packages/backend/src/ee/accountPermissionSyncer.ts +++ b/packages/backend/src/ee/accountPermissionSyncer.ts @@ -18,7 +18,7 @@ import { import { createBitbucketCloudClient, createBitbucketServerClient, getReposForAuthenticatedBitbucketCloudUser, getReposForAuthenticatedBitbucketServerUser } from "../bitbucket.js"; import { Settings } from "../types.js"; import { setIntervalAsync } from "../utils.js"; -import { isUnauthorized, isForbidden, isGone } from "../errors.js"; +import { PermissionSyncUpstreamError, withPermissionSyncUpstreamError } from "./permissionSyncError.js"; const LOG_TAG = 'user-permission-syncer'; const logger = createLogger(LOG_TAG); @@ -34,9 +34,8 @@ type AccountPermissionSyncJob = { export type PermissionCleanupReason = | 'oauth_invalid_grant' - | 'http_unauthorized' - | 'http_forbidden' - | 'http_gone'; + | 'upstream_credential_rejected' + | 'upstream_insufficient_scope'; export type PermissionCleanupDecision = | { @@ -49,9 +48,8 @@ export type PermissionCleanupDecision = const PERMISSION_CLEANUP_REASON_MESSAGES: Record = { oauth_invalid_grant: 'OAuth invalid_grant', - http_unauthorized: 'HTTP 401 Unauthorized', - http_forbidden: 'HTTP 403 Forbidden', - http_gone: 'HTTP 410 Gone', + upstream_credential_rejected: 'upstream credential rejection', + upstream_insufficient_scope: 'insufficient OAuth scope', }; export const classifyPermissionSyncFailure = (error: unknown): PermissionCleanupDecision => { @@ -64,14 +62,13 @@ export const classifyPermissionSyncFailure = (error: unknown): PermissionCleanup : { action: 'preserve_permissions' }; } - if (isUnauthorized(error)) { - return { action: 'clear_permissions', reason: 'http_unauthorized' }; - } - if (isForbidden(error)) { - return { action: 'clear_permissions', reason: 'http_forbidden' }; - } - if (isGone(error)) { - return { action: 'clear_permissions', reason: 'http_gone' }; + if (error instanceof PermissionSyncUpstreamError) { + if (error.kind === 'credential_rejected') { + return { action: 'clear_permissions', reason: 'upstream_credential_rejected' }; + } + if (error.kind === 'insufficient_scope') { + return { action: 'clear_permissions', reason: 'upstream_insufficient_scope' }; + } } return { action: 'preserve_permissions' }; @@ -235,12 +232,9 @@ export class AccountPermissionSyncer { try { await this.syncAccountPermissions(account, logger); } catch (error) { - // Fail-closed: when the code-host layer signals that the upstream - // account is permanently unauthorized (token revoked, user - // deprovisioned, OAuth grant dead) or that the endpoint we depend - // on is gone (e.g. Bitbucket Cloud's CHANGE-2770), clear the - // account's existing permission rows so the read-side filter stops - // matching through them. + // Clear cached permissions only for classified permanent failures. + // Ambiguous HTTP errors and transient upstream failures preserve the + // last successful permission state. const cleanupDecision = classifyPermissionSyncFailure(error); if (cleanupDecision.action === 'clear_permissions') { @@ -280,19 +274,34 @@ export class AccountPermissionSyncer { url: idpConfig.baseUrl, }); - const scopes = await getGitHubOAuthScopesForAuthenticatedUser(octokit, accessToken); + const scopes = await withPermissionSyncUpstreamError( + 'github', + 'inspect_token_scopes', + () => getGitHubOAuthScopesForAuthenticatedUser(octokit, accessToken), + ); // Token supports scope introspection (classic PAT or OAuth app token) if (scopes !== null) { if (!scopes.includes('repo')) { - throw new Error(`OAuth token with scopes [${scopes.join(', ')}] is missing the 'repo' scope required for permission syncing. Please re-authorize with GitHub to grant the required scope.`); + throw new PermissionSyncUpstreamError( + `OAuth token with scopes [${scopes.join(', ')}] is missing the 'repo' scope required for permission syncing. Please re-authorize with GitHub to grant the required scope.`, + { + kind: 'insufficient_scope', + provider: 'github', + operation: 'inspect_token_scopes', + }, + ); } } // @note: we only care about the private repos since we don't need to build a mapping // for public repos. // @see: packages/web/src/prisma.ts - const githubRepos = await getReposForAuthenticatedUser(/* visibility = */ 'private', octokit); + const githubRepos = await withPermissionSyncUpstreamError( + 'github', + 'list_accessible_repositories', + () => getReposForAuthenticatedUser(/* visibility = */ 'private', octokit), + ); const gitHubRepoIds = githubRepos.map(repo => repo.id.toString()); const repos = await this.db.repo.findMany({ @@ -314,9 +323,20 @@ export class AccountPermissionSyncer { url: idpConfig.baseUrl, }); - const scopes = await getGitLabOAuthScopesForAuthenticatedUser(api); + const scopes = await withPermissionSyncUpstreamError( + 'gitlab', + 'inspect_token_scopes', + () => getGitLabOAuthScopesForAuthenticatedUser(api), + ); if (!scopes.includes('read_api')) { - throw new Error(`OAuth token with scopes [${scopes.join(', ')}] is missing the 'read_api' scope required for permission syncing.`); + throw new PermissionSyncUpstreamError( + `OAuth token with scopes [${scopes.join(', ')}] is missing the 'read_api' scope required for permission syncing.`, + { + kind: 'insufficient_scope', + provider: 'gitlab', + operation: 'inspect_token_scopes', + }, + ); } // @note: we only care about the private repos since we don't need to build a @@ -326,7 +346,11 @@ export class AccountPermissionSyncer { // // @see: packages/web/src/prisma.ts const gitLabProjectIds = ( - await getProjectsForAuthenticatedUser('private', api) + await withPermissionSyncUpstreamError( + 'gitlab', + 'list_accessible_repositories', + () => getProjectsForAuthenticatedUser('private', api), + ) ).map(project => project.id.toString()); const repos = await this.db.repo.findMany({ @@ -346,7 +370,11 @@ export class AccountPermissionSyncer { // @note: we don't pass a user here since we want to use a bearer token // for authentication. const client = createBitbucketCloudClient(/* user = */ undefined, accessToken) - const bitbucketRepos = await getReposForAuthenticatedBitbucketCloudUser(client); + const bitbucketRepos = await withPermissionSyncUpstreamError( + 'bitbucket-cloud', + 'list_accessible_repositories', + () => getReposForAuthenticatedBitbucketCloudUser(client), + ); const bitbucketRepoUuids = bitbucketRepos.map(repo => repo.uuid); const repos = await this.db.repo.findMany({ @@ -364,7 +392,11 @@ export class AccountPermissionSyncer { repos.forEach(repo => aggregatedRepoIds.add(repo.id)); } else if (idpConfig.provider === 'bitbucket-server') { const client = createBitbucketServerClient(idpConfig.baseUrl, /* user = */ undefined, accessToken); - const serverRepos = await getReposForAuthenticatedBitbucketServerUser(client); + const serverRepos = await withPermissionSyncUpstreamError( + 'bitbucket-server', + 'list_accessible_repositories', + () => getReposForAuthenticatedBitbucketServerUser(client), + ); const serverRepoIds = serverRepos.map(r => r.id); const repos = await this.db.repo.findMany({ diff --git a/packages/backend/src/ee/permissionSyncError.test.ts b/packages/backend/src/ee/permissionSyncError.test.ts new file mode 100644 index 000000000..5dffde4ac --- /dev/null +++ b/packages/backend/src/ee/permissionSyncError.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, test } from 'vitest'; +import { + classifyPermissionSyncUpstreamError, + PermissionSyncUpstreamError, + withPermissionSyncUpstreamError, +} from './permissionSyncError.js'; + +describe('classifyPermissionSyncUpstreamError', () => { + test('classifies a 401 from an authenticated operation as a credential rejection', () => { + const cause = Object.assign(new Error('Unauthorized'), { status: 401 }); + + expect(classifyPermissionSyncUpstreamError( + cause, + 'bitbucket-server', + 'list_accessible_repositories', + )).toMatchObject({ + kind: 'credential_rejected', + provider: 'bitbucket-server', + operation: 'list_accessible_repositories', + status: 401, + cause, + }); + }); + + test('classifies a GitHub rate-limit 403 separately from an ambiguous forbidden response', () => { + const rateLimitError = Object.assign(new Error('Rate limited'), { + status: 403, + response: { headers: { 'x-ratelimit-remaining': '0' } }, + }); + const forbiddenError = Object.assign(new Error('Forbidden'), { status: 403 }); + + expect(classifyPermissionSyncUpstreamError( + rateLimitError, + 'github', + 'list_accessible_repositories', + ).kind).toBe('rate_limited'); + expect(classifyPermissionSyncUpstreamError( + forbiddenError, + 'github', + 'list_accessible_repositories', + ).kind).toBe('forbidden'); + }); + + test('classifies HTTP 429 as rate limited', () => { + const cause = Object.assign(new Error('Too Many Requests'), { status: 429 }); + + expect(classifyPermissionSyncUpstreamError( + cause, + 'gitlab', + 'list_accessible_repositories', + ).kind).toBe('rate_limited'); + }); + + test('does not classify Bitbucket Cloud 410 as a removed permission endpoint', () => { + const cause = Object.assign(new Error('Gone'), { status: 410 }); + + expect(classifyPermissionSyncUpstreamError( + cause, + 'bitbucket-cloud', + 'list_accessible_repositories', + ).kind).toBe('unknown'); + }); + + test.each([ + Object.assign(new Error('Internal Server Error'), { status: 500 }), + new TypeError('fetch failed'), + Object.assign(new Error('request timed out'), { name: 'TimeoutError' }), + ])('classifies an unavailable provider as transient', (cause) => { + expect(classifyPermissionSyncUpstreamError( + cause, + 'bitbucket-server', + 'list_accessible_repositories', + ).kind).toBe('upstream_unavailable'); + }); + + test('does not reclassify an existing permission sync error', async () => { + const error = new PermissionSyncUpstreamError('Missing scope', { + kind: 'insufficient_scope', + provider: 'github', + operation: 'inspect_token_scopes', + }); + + const caught = await withPermissionSyncUpstreamError( + 'github', + 'inspect_token_scopes', + () => Promise.reject(error), + ).catch(error => error); + + expect(caught).toBe(error); + }); +}); diff --git a/packages/backend/src/ee/permissionSyncError.ts b/packages/backend/src/ee/permissionSyncError.ts new file mode 100644 index 000000000..cdebeb250 --- /dev/null +++ b/packages/backend/src/ee/permissionSyncError.ts @@ -0,0 +1,112 @@ +import type { IdentityProviderType } from '@sourcebot/shared'; +import { getErrorHeader, getErrorStatus } from '../errors.js'; + +export type PermissionSyncUpstreamErrorKind = + | 'credential_rejected' + | 'insufficient_scope' + | 'rate_limited' + | 'upstream_unavailable' + | 'forbidden' + | 'unknown'; + +export type PermissionSyncOperation = + | 'inspect_token_scopes' + | 'list_accessible_repositories'; + +type PermissionSyncUpstreamErrorOptions = { + kind: PermissionSyncUpstreamErrorKind; + provider: IdentityProviderType; + operation: PermissionSyncOperation; + status?: number; + cause?: unknown; +}; + +export class PermissionSyncUpstreamError extends Error { + public readonly kind: PermissionSyncUpstreamErrorKind; + public readonly provider: IdentityProviderType; + public readonly operation: PermissionSyncOperation; + public readonly status?: number; + + constructor(message: string, options: PermissionSyncUpstreamErrorOptions) { + super(message, { cause: options.cause }); + this.name = 'PermissionSyncUpstreamError'; + this.kind = options.kind; + this.provider = options.provider; + this.operation = options.operation; + this.status = options.status; + } +} + +const isRateLimited = (error: unknown, status: number | null): boolean => + status === 429 || + getErrorHeader(error, 'retry-after') !== undefined || + getErrorHeader(error, 'x-ratelimit-remaining') === '0'; + +const isNetworkOrTimeoutError = (error: unknown): boolean => { + if (!(error instanceof Error)) { + return false; + } + + return error instanceof TypeError || error.name === 'AbortError' || error.name === 'TimeoutError'; +}; + +export const classifyPermissionSyncUpstreamError = ( + error: unknown, + provider: IdentityProviderType, + operation: PermissionSyncOperation, +): PermissionSyncUpstreamError => { + if (error instanceof PermissionSyncUpstreamError) { + return error; + } + + const status = getErrorStatus(error); + + if (status === 401) { + return new PermissionSyncUpstreamError( + `${provider} rejected the permission sync credential.`, + { kind: 'credential_rejected', provider, operation, status, cause: error }, + ); + } + + if (isRateLimited(error, status)) { + return new PermissionSyncUpstreamError( + `${provider} rate limited the permission sync request.`, + { kind: 'rate_limited', provider, operation, status: status ?? undefined, cause: error }, + ); + } + + if (status === 403) { + return new PermissionSyncUpstreamError( + `${provider} forbade the permission sync request.`, + { kind: 'forbidden', provider, operation, status, cause: error }, + ); + } + + if ( + status === 408 || + (status !== null && status >= 500 && status < 600) || + isNetworkOrTimeoutError(error) + ) { + return new PermissionSyncUpstreamError( + `${provider} is temporarily unavailable for permission syncing.`, + { kind: 'upstream_unavailable', provider, operation, status: status ?? undefined, cause: error }, + ); + } + + return new PermissionSyncUpstreamError( + `${provider} permission sync failed with an unclassified upstream error.`, + { kind: 'unknown', provider, operation, status: status ?? undefined, cause: error }, + ); +}; + +export const withPermissionSyncUpstreamError = async ( + provider: IdentityProviderType, + operation: PermissionSyncOperation, + callback: () => Promise, +): Promise => { + try { + return await callback(); + } catch (error) { + throw classifyPermissionSyncUpstreamError(error, provider, operation); + } +}; diff --git a/packages/backend/src/errors.test.ts b/packages/backend/src/errors.test.ts index c96b4f694..8e2dbbc90 100644 --- a/packages/backend/src/errors.test.ts +++ b/packages/backend/src/errors.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from 'vitest'; import { RequestError } from '@octokit/request-error'; import { GitbeakerRequestError } from '@gitbeaker/requester-utils'; -import { isForbidden, isGone, isUnauthorized } from './errors'; +import { getErrorHeader, getErrorStatus, isForbidden, isGone, isUnauthorized } from './errors'; import { throwOnHttpError } from './bitbucket'; // Helper: invoke the openapi-fetch middleware against a synthetic Response and @@ -23,6 +23,59 @@ const invokeMiddleware = async (response: Response): Promise => { } }; +describe('HTTP error metadata', () => { + test('reads status and case-insensitive headers from a direct response', () => { + const error = Object.assign(new Error('Rate limited'), { + response: { + status: 429, + headers: { + 'Retry-After': '30', + }, + }, + }); + + expect(getErrorStatus(error)).toBe(429); + expect(getErrorHeader(error, 'retry-after')).toBe('30'); + }); + + test('reads status and native Headers from a nested cause response', () => { + const error = Object.assign(new Error('Rate limited'), { + cause: { + response: new Response(null, { + status: 429, + headers: { + 'X-RateLimit-Remaining': '0', + }, + }), + }, + }); + + expect(getErrorStatus(error)).toBe(429); + expect(getErrorHeader(error, 'x-ratelimit-remaining')).toBe('0'); + }); + + test('combines a direct status with headers from the direct response', () => { + const error = Object.assign(new Error('Rate limited'), { + status: 403, + response: { + headers: { + 'x-ratelimit-remaining': '0', + }, + }, + }); + + expect(getErrorStatus(error)).toBe(403); + expect(getErrorHeader(error, 'X-RateLimit-Remaining')).toBe('0'); + }); + + test('returns no metadata for a plain error', () => { + const error = new Error('Not an HTTP error'); + + expect(getErrorStatus(error)).toBeNull(); + expect(getErrorHeader(error, 'retry-after')).toBeUndefined(); + }); +}); + describe('isUnauthorized', () => { test('Octokit RequestError with status 401', () => { const err = new RequestError('Unauthorized', 401, { diff --git a/packages/backend/src/errors.ts b/packages/backend/src/errors.ts index e174f0b20..56865e587 100644 --- a/packages/backend/src/errors.ts +++ b/packages/backend/src/errors.ts @@ -1,31 +1,59 @@ +type HttpErrorDetails = { + status: number | null; + headers?: unknown; +}; + /** - * Extract an HTTP status code from a thrown error across the libraries used by - * the code-host clients: - * - Octokit RequestError: { status } - * - openapi-fetch (Bitbucket Cloud / Server): direct throws with { status } - * or errors wrapped via Object.assign(new Error(...), { status }) - * - gitbeaker (GitLab): { cause: { response: { status } } } + * Normalizes HTTP response metadata across the code-host client libraries: + * - Octokit RequestError: { status, response: { headers } } + * - openapi-fetch (Bitbucket Cloud / Server): { status } + * - gitbeaker (GitLab): { cause: { response: { status, headers } } } */ -const getStatus = (err: unknown): number | null => { - if (err === null || typeof err !== 'object') { - return null; +const getHttpErrorDetails = (error: unknown): HttpErrorDetails => { + if (error === null || typeof error !== 'object') { + return { status: null }; } - const direct = (err as { status?: unknown }).status; - if (typeof direct === 'number') { - return direct; + const directError = error as { + status?: unknown; + response?: { status?: unknown; headers?: unknown }; + cause?: { response?: { status?: unknown; headers?: unknown } }; + }; + const directResponse = directError.response; + const nestedResponse = directError.cause?.response; + const status = [ + directError.status, + directResponse?.status, + nestedResponse?.status, + ].find((value): value is number => typeof value === 'number') ?? null; + + return { + status, + headers: directResponse?.headers ?? nestedResponse?.headers, + }; +}; + +export const getErrorStatus = (error: unknown): number | null => + getHttpErrorDetails(error).status; + +export const getErrorHeader = (error: unknown, name: string): string | undefined => { + const { headers } = getHttpErrorDetails(error); + + if (headers instanceof Headers) { + return headers.get(name) ?? undefined; } - const nested = (err as { cause?: { response?: { status?: unknown } } }).cause?.response?.status; - if (typeof nested === 'number') { - return nested; + if (headers !== null && typeof headers === 'object') { + const normalizedName = name.toLowerCase(); + const entry = Object.entries(headers).find(([key]) => key.toLowerCase() === normalizedName); + return typeof entry?.[1] === 'string' ? entry[1] : undefined; } - return null; + return undefined; }; -export const isUnauthorized = (err: unknown): boolean => getStatus(err) === 401; -export const isForbidden = (err: unknown): boolean => getStatus(err) === 403; -export const isNotFound = (err: unknown): boolean => getStatus(err) === 404; -export const isGone = (err: unknown): boolean => getStatus(err) === 410; +export const isUnauthorized = (err: unknown): boolean => getErrorStatus(err) === 401; +export const isForbidden = (err: unknown): boolean => getErrorStatus(err) === 403; +export const isNotFound = (err: unknown): boolean => getErrorStatus(err) === 404; +export const isGone = (err: unknown): boolean => getErrorStatus(err) === 410;