fix(worker): Fix issue with stale permissions#1215
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
WalkthroughAdds error-normalization utilities, defines/registers Bitbucket middleware that throws on non-2xx responses, updates Bitbucket call sites to rely on thrown errors and numeric ChangesError handling and fail-closed permission sync
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/backend/src/bitbucket.ts`:
- Around line 55-58: The current errorconstruction reads the full response with
response.clone().text() and interpolates it into the Error.message via
Object.assign(new Error(`Bitbucket API ${response.status}: ${body}`), { status:
response.status }), which can leak payloads and create huge logs; change this to
keep the Error.message concise (e.g. `Bitbucket API ${response.status}`), attach
the full body to a non-message property (e.g. error.body or error.responseBody)
and if needed store a truncated/redacted excerpt (limit to a small length)
rather than embedding the entire body, while still preserving the status
property; update the throw site where Object.assign and new Error are used so
logs capture the short message and developers can inspect error.body if
necessary.
In `@packages/backend/src/ee/accountPermissionSyncer.ts`:
- Around line 196-199: The fail-closed branch checking
isUnauthorized/isForbidden/RefreshTokenError is bypassed because
syncAccountPermissions() wraps GitHub 401/403 into a plain Error; update the
error handling in syncAccountPermissions to either preserve the original HTTP
status when creating the wrapped error (e.g., copy status/statusCode onto the
new Error) or avoid wrapping and rethrow the original error after adding
context, so isUnauthorized/isForbidden will detect revoked GitHub tokens; ensure
the change touches the error creation/throwing site and keeps
isUnauthorized/isForbidden/RefreshTokenError checks working as-is.
- Around line 188-211: The try/catch around this.syncAccountPermissions
currently swallows non-auth errors so runJob()/onJobCompleted() mark the job
COMPLETED even when sync failed; modify the catch to rethrow any error that is
not handled by the auth checks by adding a throw error after the
unauthorized/forbidden/RefreshTokenError handling (or remove the catch and only
handle auth errors explicitly), ensuring syncAccountPermissions failures
propagate to runJob() and trigger onJobFailed().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3701deaa-f10f-42e0-a483-ebef83618bd2
📒 Files selected for processing (4)
packages/backend/src/bitbucket.tspackages/backend/src/ee/accountPermissionSyncer.tspackages/backend/src/errors.test.tspackages/backend/src/errors.ts
msukkari
previously approved these changes
May 21, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/backend/src/ee/accountPermissionSyncer.ts (2)
223-237:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't treat every refresh-path failure as token revocation.
The catch at Line 235 rewrites every
ensureFreshAccountToken()failure intoRefreshTokenError, so transient issues in that path will also hit the fail-closed delete inrunJob(). That can wipe valid permissions for accounts whose OAuth grant is still fine.Only convert refresh responses that prove the grant is invalid to
RefreshTokenError; let other refresh-path failures bubble as ordinary job failures.🤖 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 223 - 237, The current catch around ensureFreshAccountToken(account, this.db) indiscriminately wraps all errors as RefreshTokenError, causing transient failures to trigger permission deletion in runJob; change this so you only convert to RefreshTokenError when the underlying error clearly indicates an invalid grant (e.g., inspect error.response?.data?.error === 'invalid_grant', error.message contains 'invalid_grant', or a specific OAuth error code), and rethrow the original error for all other cases so ordinary job failures bubble up; update the catch in accountPermissionSyncer.ts accordingly (referencing ensureFreshAccountToken and RefreshTokenError and how runJob treats RefreshTokenError).
191-211:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on scope-loss errors too.
This branch only clears permissions for
401/403/RefreshTokenError, butsyncAccountPermissions()still throws plainErrorwhen a GitHub token lacksrepoor a GitLab token lacksread_api. Those are permanent authorization failures too, so the job will still leave staleaccessibleReposbehind.Consider rethrowing those scope checks as a typed auth error here, or classifying them in this branch as fail-closed conditions.
🤖 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 191 - 211, The catch block in accountPermissionSyncer currently only clears accessibleRepos for isUnauthorized, isForbidden, or RefreshTokenError; extend the fail-closed logic to also treat token-scope errors from syncAccountPermissions as permanent authorization failures by including the scope-check error classification here (or by mapping those scope errors to a typed auth error earlier). Update the condition that checks isUnauthorized/isForbidden/RefreshTokenError to also check the scope-loss predicate (or the new AuthScopeError type) so that this.db.account.update({ where: { id: account.id }, data: { accessibleRepos: { deleteMany: {} } } }) runs for scope-loss cases as well before rethrowing the error. Ensure you reference the same symbols: isUnauthorized, isForbidden, RefreshTokenError, syncAccountPermissions, accessibleRepos and preserve rethrowing the original error after clearing.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@packages/backend/src/ee/accountPermissionSyncer.ts`:
- Around line 223-237: The current catch around ensureFreshAccountToken(account,
this.db) indiscriminately wraps all errors as RefreshTokenError, causing
transient failures to trigger permission deletion in runJob; change this so you
only convert to RefreshTokenError when the underlying error clearly indicates an
invalid grant (e.g., inspect error.response?.data?.error === 'invalid_grant',
error.message contains 'invalid_grant', or a specific OAuth error code), and
rethrow the original error for all other cases so ordinary job failures bubble
up; update the catch in accountPermissionSyncer.ts accordingly (referencing
ensureFreshAccountToken and RefreshTokenError and how runJob treats
RefreshTokenError).
- Around line 191-211: The catch block in accountPermissionSyncer currently only
clears accessibleRepos for isUnauthorized, isForbidden, or RefreshTokenError;
extend the fail-closed logic to also treat token-scope errors from
syncAccountPermissions as permanent authorization failures by including the
scope-check error classification here (or by mapping those scope errors to a
typed auth error earlier). Update the condition that checks
isUnauthorized/isForbidden/RefreshTokenError to also check the scope-loss
predicate (or the new AuthScopeError type) so that this.db.account.update({
where: { id: account.id }, data: { accessibleRepos: { deleteMany: {} } } }) runs
for scope-loss cases as well before rethrowing the error. Ensure you reference
the same symbols: isUnauthorized, isForbidden, RefreshTokenError,
syncAccountPermissions, accessibleRepos and preserve rethrowing the original
error after clearing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a3cd1d3-1806-4ca8-ae2e-be75dda34819
📒 Files selected for processing (2)
CHANGELOG.mdpackages/backend/src/ee/accountPermissionSyncer.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Changelog