Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #772 +/- ##
==========================================
- Coverage 92.27% 92.25% -0.02%
==========================================
Files 1305 1305
Lines 47938 47957 +19
Branches 1628 1636 +8
==========================================
+ Hits 44233 44245 +12
- Misses 3396 3401 +5
- Partials 309 311 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…eover Generate a cryptographically random state on redirect, store it in a signed httponly cookie, and validate it matches before exchanging the authorization code. Also updates tests for the OAuth 2.0 flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove OAuth 1.0 query params (oauth_consumer_key, oauth_token, oauth_version) from cassette URIs to match the new Bearer token request format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
thomasrockhu-codecov
left a comment
There was a problem hiding this comment.
Please test on staging or another environment first before merging into main
Access tokens expire in ~2h; refresh using stored refresh_token. Removes the no-op early returns in the token refresh callbacks for both API and worker that were left over from OAuth 1.0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…h errors - Bitbucket Server still uses OAuth 1.0 so restore the return None guard that was incorrectly removed alongside the BITBUCKET guard - Catch TorngitClientGeneralError/5xx from refresh_token() in api() so an expired/revoked refresh token raises the original 401 instead of a confusing refresh error Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Set secure=settings.SESSION_COOKIE_SECURE on _bb_oauth_state cookie so it isn't transmitted over plain HTTP - Remove unused original_url param from refresh_token() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| log.warning( | ||
| "Bitbucket token refresh failed, raising original 401", | ||
| extra=log_dict, | ||
| ) |
There was a problem hiding this comment.
Unhandled network errors during token refresh propagate raw
Low Severity
The refresh_token method makes an HTTP request via client.request() at line 145 which can raise httpx.NetworkError or httpx.TimeoutException. The except block at line 102 only catches TorngitClientGeneralError and TorngitServer5xxCodeError, so raw httpx exceptions propagate unhandled. This is inconsistent with the main request path (lines 90–93) which wraps these into TorngitServerUnreachableError. Callers catching TorngitServerFailureError would miss these.
Additional Locations (1)
There was a problem hiding this comment.
This is existing behavior. Will be handled in a later PR.
Prevents state cookie from being sent on cross-site requests when SameSite=None is configured (e.g. staging). Also caps cookie lifetime at 300s to limit the window for state reuse. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| if service == "bitbucket_server": | ||
| return None | ||
|
|
||
| async def callback(new_token: dict) -> None: |
There was a problem hiding this comment.
Bug: The worker's token refresh callback for Bitbucket updates owner.oauth_token in memory but doesn't commit the change to the database, causing the new token to be lost.
Severity: HIGH
Suggested Fix
After updating the owner.oauth_token in the callback within apps/worker/helpers/token_refresh.py, ensure the change is persisted to the database. This can be done by calling owner.get_db_session().commit() or an equivalent persistence method on the SQLAlchemy session.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/worker/helpers/token_refresh.py#L22-L25
Potential issue: The pull request enables a token refresh callback for Bitbucket in the
worker context. When this callback is invoked, it successfully refreshes the token and
updates the `owner.oauth_token` attribute on the SQLAlchemy `Owner` model instance.
However, it fails to call `owner.save()` or `db_session.commit()` to persist this change
to the database. As a result, the refreshed token is lost after the task completes.
Subsequent tasks will use the old, expired token, triggering an unnecessary and
continuous refresh cycle without ever saving the valid new token.
michelletran-sentry
left a comment
There was a problem hiding this comment.
Just one comment about the while loop. Otherwise looks OK to me.
| ) | ||
| elif res.status_code >= 500: | ||
| tried_refresh = False | ||
| while True: |
There was a problem hiding this comment.
This while loop is kind of confusing, and it's not clear when it's going to "break" out of the loop. It looks like we only use it to retry the request after we perform a token refresh. It would be easier to understand if the logic was more explicit (i.e. do an attempt to call the request, if it fails, do token refresh then attempt again).
Extract HTTP call into _send_request() helper and make the 401 token refresh retry explicit instead of using while/continue. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| httponly=True, | ||
| secure=settings.SESSION_COOKIE_SECURE, | ||
| samesite=settings.COOKIE_SAME_SITE, | ||
| max_age=300, |
There was a problem hiding this comment.
nit: is this in seconds? Might be worth setting to a const or adding a comment for clarity
| ) | ||
| expected_state = request.get_signed_cookie("_bb_oauth_state", default=None) | ||
| if not expected_state or request.GET.get("state") != expected_state: | ||
| log.warning("Bitbucket OAuth state mismatch — possible CSRF attempt") |
There was a problem hiding this comment.
nit: would it be worth it to add exc_info to this log? Or maybe some other identifying things
| new_token = await self.refresh_token(client) | ||
| except (TorngitClientGeneralError, TorngitServer5xxCodeError): | ||
| log.warning( | ||
| "Bitbucket token refresh failed, raising original 401", |
There was a problem hiding this comment.
am I missing something, but I don't think we're re-raising in this block, right?
ajay-sentry
left a comment
There was a problem hiding this comment.
couple small things to look at


Fixes API-DQ3. Bitbucket's 401 response to the OAuth 1.0 request token endpoint caused a
KeyErrordue to missing error handling.Changes
oauthlibdependencystateparameter to the OAuth 2.0 authorization URL and validates it on callback via signed cookie, preventing OAuth CSRF / account takeoverCredentials
Existing
BITBUCKET_CLIENT_ID/BITBUCKET_CLIENT_SECRETcredentials are reusable — no new OAuth consumer needed. The Bitbucket consumer must be configured as a private consumer in Bitbucket settings (required for the authorization code grant). Verifycodecov_production_bitbucket_client_secretin GCP Secret Manager is populated.Note
High Risk
High risk because it changes Bitbucket login/authentication from OAuth1 to OAuth2, adds new CSRF-state validation, and introduces automatic access-token refresh on 401s which affects all Bitbucket API calls.
Overview
Bitbucket OAuth is migrated from OAuth 1.0 to OAuth 2.0. The login flow now redirects via
generate_redirect_url, exchanges an OAuthcodefor tokens, and signs API calls withAuthorization: Bearer ...instead of OAuth1 query signing.Adds OAuth CSRF protection and better error handling. The login redirect sets a short-lived signed
_bb_oauth_statecookie (secure/httponly/samesite) and the callback requires a matchingstate, plus the view now handles Bitbucket 4xx OAuth errors (TorngitClientGeneralError) by redirecting back to login.Enables Bitbucket token refresh and persistence. The Bitbucket adapter can refresh tokens using the stored refresh token on 401 responses and replays the request, and both API and worker token-refresh callback wiring now allow Bitbucket (but still disable Bitbucket Server); tests and Bitbucket VCR cassettes are updated accordingly.
Written by Cursor Bugbot for commit 7c1c15f. This will update automatically on new commits. Configure here.