-
Notifications
You must be signed in to change notification settings - Fork 325
fix: surface permission sync issues requiring user action #1484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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"; | ||
|
|
@@ -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 => { | ||
|
|
@@ -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}`); | ||
| } | ||
| throw error; | ||
| } | ||
|
|
@@ -451,6 +478,8 @@ export class AccountPermissionSyncer { | |
| account: { | ||
| update: { | ||
| permissionSyncedAt: new Date(), | ||
| permissionSyncIssue: null, | ||
| permissionSyncIssueAt: null, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completed job clears newer issueHigh Severity
Additional Locations (2)Reviewed by Cursor Bugbot for commit c149678. Configure here. |
||
| }, | ||
| }, | ||
| completedAt: new Date(), | ||
|
|
||
| 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); |


There was a problem hiding this comment.
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.emailis 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 loggingaccount.idonly, or a redacted/hashed identifier.🔒 Suggested fix
📝 Committable suggestion
🧰 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
Source: Linters/SAST tools