Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,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)
- [EE] Added action-required warnings and guided recovery when permission syncing clears cached repository access. [#1484](https://github.com/sourcebot-dev/sourcebot/pull/1484)

## [5.1.3] - 2026-07-20

Expand Down
132 changes: 130 additions & 2 deletions packages/backend/src/ee/accountPermissionSyncer.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
import { describe, expect, test } from 'vitest';
import { classifyPermissionSyncFailure } from './accountPermissionSyncer.js';
import { beforeEach, describe, expect, test, vi } from 'vitest';

const mocks = vi.hoisted(() => ({
hasEntitlement: vi.fn(),
}));

vi.mock('../entitlements.js', () => ({
hasEntitlement: mocks.hasEntitlement,
}));

import { AccountPermissionSyncer, classifyPermissionSyncFailure } from './accountPermissionSyncer.js';
import {
PermissionSyncUpstreamError,
type PermissionSyncUpstreamErrorKind,
Expand All @@ -22,6 +31,49 @@ const upstreamError = (
operation: 'list_accessible_repositories',
});

const createSyncerHarness = (syncError?: Error, permissionCount = 95) => {
const account = {
id: 'account_1',
providerId: 'bitbucket-server',
user: { email: 'user@example.com' },
};
const db = {
accountPermissionSyncJob: {
update: vi.fn().mockResolvedValue({ account }),
},
accountToRepoPermission: {
deleteMany: vi.fn().mockResolvedValue({ count: permissionCount }),
},
account: {
update: vi.fn().mockResolvedValue(account),
},
$transaction: vi.fn((queries: Array<Promise<unknown>>) => Promise.all(queries)),
};
const syncAccountPermissions = syncError
? vi.fn().mockRejectedValue(syncError)
: vi.fn().mockResolvedValue(undefined);
const syncer = Object.create(AccountPermissionSyncer.prototype) as {
db: typeof db;
syncAccountPermissions: typeof syncAccountPermissions;
runJob(job: { data: { jobId: string } }): Promise<void>;
onJobCompleted(job: { data: { jobId: string } }): Promise<void>;
};
syncer.db = db;
syncer.syncAccountPermissions = syncAccountPermissions;

return {
account,
db,
job: { data: { jobId: 'job_1' } },
syncer,
};
};

beforeEach(() => {
vi.clearAllMocks();
mocks.hasEntitlement.mockResolvedValue(true);
});

describe('classifyPermissionSyncFailure', () => {
test('fails closed for invalid_grant', () => {
expect(classifyPermissionSyncFailure(tokenRefreshError('invalid_grant', 400))).toEqual({
Expand Down Expand Up @@ -75,3 +127,79 @@ describe('classifyPermissionSyncFailure', () => {
});
});
});

describe('permission sync issue lifecycle', () => {
test('atomically records a reauthentication issue when invalid_grant clears permissions', async () => {
const error = tokenRefreshError('invalid_grant', 400);
const { db, job, syncer } = createSyncerHarness(error);

await expect(syncer.runJob(job)).rejects.toBe(error);

expect(db.accountToRepoPermission.deleteMany).toHaveBeenCalledWith({
where: { accountId: 'account_1' },
});
expect(db.account.update).toHaveBeenCalledWith({
where: { id: 'account_1' },
data: {
permissionSyncIssue: 'REAUTHENTICATION_REQUIRED',
permissionSyncIssueAt: expect.any(Date),
},
});
expect(db.$transaction).toHaveBeenCalledOnce();
});

test('records an issue when permissions were already cleared by an earlier attempt', async () => {
const error = tokenRefreshError('invalid_grant', 400);
const { db, job, syncer } = createSyncerHarness(error, 0);

await expect(syncer.runJob(job)).rejects.toBe(error);

expect(db.account.update).toHaveBeenCalledWith(expect.objectContaining({
data: expect.objectContaining({
permissionSyncIssue: 'REAUTHENTICATION_REQUIRED',
}),
}));
expect(db.$transaction).toHaveBeenCalledOnce();
});

test('records an insufficient-scope issue for scope failures', async () => {
const error = upstreamError('insufficient_scope');
const { db, job, syncer } = createSyncerHarness(error);

await expect(syncer.runJob(job)).rejects.toBe(error);

expect(db.account.update).toHaveBeenCalledWith(expect.objectContaining({
data: expect.objectContaining({
permissionSyncIssue: 'INSUFFICIENT_SCOPE',
}),
}));
});

test('does not record an issue or clear permissions for a transient refresh failure', async () => {
const error = tokenRefreshError('transient', 500);
const { db, job, syncer } = createSyncerHarness(error);

await expect(syncer.runJob(job)).rejects.toBe(error);

expect(db.accountToRepoPermission.deleteMany).not.toHaveBeenCalled();
expect(db.account.update).not.toHaveBeenCalled();
expect(db.$transaction).not.toHaveBeenCalled();
});

test('clears the action-required issue after a successful permission sync', async () => {
const { db, job, syncer } = createSyncerHarness();

await syncer.onJobCompleted(job);

expect(db.accountPermissionSyncJob.update).toHaveBeenCalledWith(expect.objectContaining({
data: expect.objectContaining({
account: {
update: expect.objectContaining({
permissionSyncIssue: null,
permissionSyncIssueAt: null,
}),
},
}),
}));
});
});
49 changes: 39 additions & 10 deletions packages/backend/src/ee/accountPermissionSyncer.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import * as Sentry from "@sentry/node";
import { PrismaClient, AccountPermissionSyncJobStatus, Account, PermissionSyncSource} from "@sourcebot/db";
import {
PrismaClient,
AccountPermissionSyncIssue,
AccountPermissionSyncJobStatus,
Account,
PermissionSyncSource,
} from "@sourcebot/db";
import { env, createLogger, getIdentityProviderConfig, PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERS } from "@sourcebot/shared";
import { hasEntitlement } from "../entitlements.js";
import { ensureFreshAccountToken, TokenRefreshError } from "./tokenRefresh.js";
Expand Down Expand Up @@ -46,10 +52,22 @@ export type PermissionCleanupDecision =
action: 'preserve_permissions';
};

const PERMISSION_CLEANUP_REASON_MESSAGES: Record<PermissionCleanupReason, string> = {
oauth_invalid_grant: 'OAuth invalid_grant',
upstream_credential_rejected: 'upstream credential rejection',
upstream_insufficient_scope: 'insufficient OAuth scope',
const PERMISSION_CLEANUP_DETAILS: Record<PermissionCleanupReason, {
message: string;
issue: AccountPermissionSyncIssue;
}> = {
oauth_invalid_grant: {
message: 'OAuth invalid_grant',
issue: AccountPermissionSyncIssue.REAUTHENTICATION_REQUIRED,
},
upstream_credential_rejected: {
message: 'upstream credential rejection',
issue: AccountPermissionSyncIssue.REAUTHENTICATION_REQUIRED,
},
upstream_insufficient_scope: {
message: 'insufficient OAuth scope',
issue: AccountPermissionSyncIssue.INSUFFICIENT_SCOPE,
},
};

export const classifyPermissionSyncFailure = (error: unknown): PermissionCleanupDecision => {
Expand Down Expand Up @@ -238,12 +256,21 @@ export class AccountPermissionSyncer {
const cleanupDecision = classifyPermissionSyncFailure(error);

if (cleanupDecision.action === 'clear_permissions') {
const { count } = await this.db.accountToRepoPermission.deleteMany({
where: { accountId: account.id },
});
const details = PERMISSION_CLEANUP_DETAILS[cleanupDecision.reason];
const [{ count }] = await this.db.$transaction([
this.db.accountToRepoPermission.deleteMany({
where: { accountId: account.id },
}),
this.db.account.update({
where: { id: account.id },
data: {
permissionSyncIssue: details.issue,
permissionSyncIssueAt: new Date(),
},
}),
]);
const message = error instanceof Error ? error.message : String(error);
const reason = PERMISSION_CLEANUP_REASON_MESSAGES[cleanupDecision.reason];
logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${reason}: ${message}`);
logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${details.message}: ${message}`);
Comment on lines +259 to +273

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Avoid logging user email in the fail-closed cleanup log line.

account.user.email is included in the warn log for fail-closed cleanup. This mirrors existing (unchanged) patterns elsewhere in the file, but static analysis flags it as PII exposure in logs (CWE-532). Consider logging account.id only, or a redacted/hashed identifier.

🔒 Suggested fix
-                logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${details.message}: ${message}`);
+                logger.warn(`Cleared ${count} permission row(s) for account ${account.id} — fail-closed cleanup triggered by ${details.message}: ${message}`);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const details = PERMISSION_CLEANUP_DETAILS[cleanupDecision.reason];
const [{ count }] = await this.db.$transaction([
this.db.accountToRepoPermission.deleteMany({
where: { accountId: account.id },
}),
this.db.account.update({
where: { id: account.id },
data: {
permissionSyncIssue: details.issue,
permissionSyncIssueAt: new Date(),
},
}),
]);
const message = error instanceof Error ? error.message : String(error);
const reason = PERMISSION_CLEANUP_REASON_MESSAGES[cleanupDecision.reason];
logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${reason}: ${message}`);
logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${details.message}: ${message}`);
const details = PERMISSION_CLEANUP_DETAILS[cleanupDecision.reason];
const [{ count }] = await this.db.$transaction([
this.db.accountToRepoPermission.deleteMany({
where: { accountId: account.id },
}),
this.db.account.update({
where: { id: account.id },
data: {
permissionSyncIssue: details.issue,
permissionSyncIssueAt: new Date(),
},
}),
]);
const message = error instanceof Error ? error.message : String(error);
logger.warn(`Cleared ${count} permission row(s) for account ${account.id} — fail-closed cleanup triggered by ${details.message}: ${message}`);
🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 272-272: Avoid logging sensitive data
Context: logger.warn(Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${details.message}: ${message})
Note: [CWE-532] Insertion of Sensitive Information into Log File.

(log-sensitive-data-typescript)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/backend/src/ee/accountPermissionSyncer.ts` around lines 259 - 273,
Update the fail-closed cleanup warning in the account permission sync flow to
remove account.user.email from the log message, retaining the account.id and
existing cleanup details and error message. Do not alter the transaction or
cleanup behavior.

Source: Linters/SAST tools

}
throw error;
}
Expand Down Expand Up @@ -451,6 +478,8 @@ export class AccountPermissionSyncer {
account: {
update: {
permissionSyncedAt: new Date(),
permissionSyncIssue: null,
permissionSyncIssueAt: null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed job clears newer issue

High Severity

onJobCompleted always nulls permissionSyncIssue for the account, with no check against a newer fail-closed update. Concurrent jobs for the same account are allowed (schedulePermissionSyncForAccount never dedupes; default concurrency is 8), so a later successful completion can erase an issue after another job already cleared permissions, leaving empty access and no banner.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c149678. Configure here.

},
},
completedAt: new Date(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-- CreateEnum
CREATE TYPE "AccountPermissionSyncIssue" AS ENUM ('REAUTHENTICATION_REQUIRED', 'INSUFFICIENT_SCOPE');

-- AlterTable
ALTER TABLE "Account"
ADD COLUMN "permissionSyncIssue" "AccountPermissionSyncIssue",
ADD COLUMN "permissionSyncIssueAt" TIMESTAMP(3);
12 changes: 11 additions & 1 deletion packages/db/prisma/schema.prisma
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,11 @@ enum PermissionSyncSource {
REPO_DRIVEN
}

enum AccountPermissionSyncIssue {
REAUTHENTICATION_REQUIRED
INSUFFICIENT_SCOPE
}

model AccountToRepoPermission {
createdAt DateTime @default(now())

Expand Down Expand Up @@ -607,7 +612,12 @@ model Account {
permissionSyncJobs AccountPermissionSyncJob[]
permissionSyncedAt DateTime?

/// Set when an OAuth token refresh fails and the account needs to be re-linked by the user.
/// Set when permission syncing fails closed and user action is required.
/// Cleared after a subsequent permission sync completes successfully.
permissionSyncIssue AccountPermissionSyncIssue?
permissionSyncIssueAt DateTime?

/// Last OAuth token refresh failure, retained for diagnostics.
/// Cleared when the user successfully re-authenticates.
tokenRefreshErrorMessage String?

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ const makeContext = (overrides: Partial<BannerContext> = {}): BannerContext => (
offlineLicense: null,
hasPermissionSyncEntitlement: false,
hasPendingFirstSync: false,
permissionSyncIssues: [],
dismissals: {},
today: TODAY,
now: NOW,
Expand All @@ -100,6 +101,22 @@ describe('resolveActiveBanner', () => {
expect(result?.id).toBe('permissionSync');
});

test('shows permission sync banner when an account requires reauthentication', () => {
const result = resolveActiveBanner(makeContext({
hasPermissionSyncEntitlement: true,
permissionSyncIssues: [{
accountId: 'account_1',
providerId: 'bitbucket-server',
providerType: 'bitbucket-server',
reason: 'REAUTHENTICATION_REQUIRED',
occurredAt: NOW.toISOString(),
isSyncing: false,
}],
}));
expect(result?.id).toBe('permissionSync');
expect(result?.dismissible).toBe(false);
});

test('license expired outranks permission sync', () => {
const result = resolveActiveBanner(makeContext({
license: makeLicense({ status: 'canceled' }),
Expand Down
15 changes: 13 additions & 2 deletions packages/web/src/app/(app)/components/banners/bannerResolver.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { InvoicePastDueBanner } from "./invoicePastDueBanner";
import { ServicePingFailedBanner } from "./servicePingFailedBanner";
import { TrialBanner } from "./trialBanner";
import { UpgradeAvailableBanner } from "./upgradeAvailableBanner";
import type { PermissionSyncStatusResponse } from "@/app/api/(server)/ee/permissionSyncStatus/api";

// Mirrors the value in `lighthouse: lambda/serviceError.ts` and the gating
// constant in `packages/shared/src/entitlements.ts`.
Expand All @@ -28,6 +29,7 @@ export interface BannerContext {
offlineLicense: OfflineLicenseMetadata | null;
hasPermissionSyncEntitlement: boolean;
hasPendingFirstSync: boolean;
permissionSyncIssues: PermissionSyncStatusResponse['issues'];
dismissals: Partial<Record<BannerId, string>>;
today: string;
now: Date;
Expand Down Expand Up @@ -155,14 +157,23 @@ function buildCandidates(ctx: BannerContext): BannerDescriptor[] {
});
}

if (ctx.hasPermissionSyncEntitlement && ctx.hasPendingFirstSync) {
if (
ctx.hasPermissionSyncEntitlement &&
(ctx.hasPendingFirstSync || ctx.permissionSyncIssues.length > 0)
) {
banners.push({
id: 'permissionSync',
priority: BannerPriority.PERMISSION_SYNC,
dismissible: false,
audience: 'everyone',
render: (props) => (
<PermissionSyncBanner {...props} initialHasPendingFirstSync={true} />
<PermissionSyncBanner
{...props}
initialStatus={{
hasPendingFirstSync: ctx.hasPendingFirstSync,
issues: ctx.permissionSyncIssues,
}}
/>
),
});
}
Expand Down
Loading
Loading