Skip to content

feat: add Bitbucket SCM provider support#328

Closed
Abhinandan-Khurana wants to merge 6 commits intoColeMurray:mainfrom
Abhinandan-Khurana:addition/bitbucket-feat
Closed

feat: add Bitbucket SCM provider support#328
Abhinandan-Khurana wants to merge 6 commits intoColeMurray:mainfrom
Abhinandan-Khurana:addition/bitbucket-feat

Conversation

@Abhinandan-Khurana
Copy link
Copy Markdown

Summary

Add Bitbucket as a supported SCM provider across the web app, control plane, and deployment config, with follow-up fixes to harden auth and workspace-scoped behavior.

Included

  • add Bitbucket provider support in the control plane and web auth flow
  • add Bitbucket-aware provider selection and deployment wiring
  • enforce Bitbucket workspace access during branch listing
  • add retry backoff and request-level deduping for Bitbucket token refresh in the web layer
  • avoid duplicate Bitbucket JWT refresh attempts in a single request path

Verification

  • node node_modules/vitest/vitest.mjs run src/lib/auth.test.ts --config vitest.config.ts
  • npm test -w @open-inspect/control-plane -- src/source-control/providers/bitbucket-provider.test.ts
  • npm test -w @open-inspect/control-plane -- src/source-control/config.test.ts
  • npm test -w @open-inspect/control-plane -- src/session/participant-service.test.ts

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 10, 2026

Greptile Summary

This PR adds Bitbucket Cloud as a fully-implemented SCM provider alongside GitHub, covering the control plane, web auth flow, sandbox clone-token wiring, and Terraform/Modal deployment config. The abstraction is well-structured: provider selection, token refresh, workspace enforcement, and repo/branch listing all follow consistent patterns and are backed by new unit tests.

Issues found:

  • Memory leak in Bitbucket JWT cache (packages/web/src/lib/auth.ts): The module-level cachedBitbucketJwtStates Map adds entries for each token rotation but never removes old refresh-token keys. In long-lived server processes this accumulates indefinitely for every active Bitbucket user.

  • Client-credentials token fetched on every app-level API call (bitbucket-provider.ts): buildAppHeaders calls getBitbucketClientCredentialsToken on every invocation without caching. This triggers a new network round-trip to Bitbucket per pagination page during listRepositories, per repo in the scheduler's Promise.all, and on every generatePushAuth call. Client-credentials tokens are valid for ~1 hour and should be cached.

  • getBranchHeadSha skips workspace enforcement (bitbucket-provider.ts): Every other repo-fetching method calls ensureRepositoryInWorkspace before trusting the API response. getBranchHeadSha omits this check, creating a gap in the workspace security boundary and inconsistency with the rest of the provider.

  • Workspace-mismatch 403 swallowed as 500 (router.ts, repos.ts): The catch block in handleCreateSession (and the equivalent in handleListBranches) converts all exceptions into a generic 500, including the SourceControlProviderError with httpStatus: 403 thrown by workspace enforcement. An authorization failure is indistinguishable from an infrastructure error. The corresponding router test asserts status: 500 for this case, documenting the misclassified response.

Comments Outside Diff (1)

  1. packages/control-plane/src/router.ts, line 638-645 (link)

    Workspace-enforcement 403 is swallowed and returned as a generic 500

    ensureRepositoryInWorkspace (inside checkRepositoryAccess) throws a SourceControlProviderError with httpStatus: 403 when a repo is outside the configured workspace. This error propagates through resolveInstalledRepo unchanged (because toSourceControlProviderError preserves SourceControlProviderError instances). However, the catch block here converts every exception, including that 403, into a generic 500:

    } catch (e) {
      logger.error("Failed to resolve repository", { ... });
      return error("Failed to resolve repository", 500); // masks the 403
    }

    As a result, a client that supplies a valid token but targets a repo in a different Bitbucket workspace receives 500 "Failed to resolve repository" instead of 403 Forbidden. This also makes the error indistinguishable from a real infrastructure failure.

    The same pattern affects handleListBranches in repos.ts — the same workspace-enforcement 403 from listBranches falls through to return error("Failed to list branches", 500).

    The catch blocks should inspect the error and propagate provider-level HTTP status codes:

    } catch (e) {
      if (e instanceof SourceControlProviderError && e.httpStatus) {
        return error(e.message, e.httpStatus);
      }
      logger.error("Failed to resolve repository", { ... });
      return error("Failed to resolve repository", 500);
    }

    The router test at line 1185 also asserts expect(response.status).toBe(500) for this scenario — that assertion would need updating when the status is corrected.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/control-plane/src/router.ts
    Line: 638-645
    
    Comment:
    **Workspace-enforcement 403 is swallowed and returned as a generic 500**
    
    `ensureRepositoryInWorkspace` (inside `checkRepositoryAccess`) throws a `SourceControlProviderError` with `httpStatus: 403` when a repo is outside the configured workspace. This error propagates through `resolveInstalledRepo` unchanged (because `toSourceControlProviderError` preserves `SourceControlProviderError` instances). However, the catch block here converts *every* exception, including that 403, into a generic 500:
    
    ```typescript
    } catch (e) {
      logger.error("Failed to resolve repository", { ... });
      return error("Failed to resolve repository", 500); // masks the 403
    }
    ```
    
    As a result, a client that supplies a valid token but targets a repo in a different Bitbucket workspace receives `500 "Failed to resolve repository"` instead of `403 Forbidden`. This also makes the error indistinguishable from a real infrastructure failure.
    
    The same pattern affects `handleListBranches` in `repos.ts` — the same workspace-enforcement 403 from `listBranches` falls through to `return error("Failed to list branches", 500)`.
    
    The catch blocks should inspect the error and propagate provider-level HTTP status codes:
    
    ```typescript
    } catch (e) {
      if (e instanceof SourceControlProviderError && e.httpStatus) {
        return error(e.message, e.httpStatus);
      }
      logger.error("Failed to resolve repository", { ... });
      return error("Failed to resolve repository", 500);
    }
    ```
    
    The router test at line 1185 also asserts `expect(response.status).toBe(500)` for this scenario — that assertion would need updating when the status is corrected.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Codex Fix in Cursor Fix in Claude Code

Fix All in Codex Fix All in Cursor Fix All in Claude Code

Last reviewed commit: 6de97dc

Comment on lines +73 to +97
Accept: "application/json",
};
}

throw new SourceControlProviderError(
"Bitbucket app credentials not configured",
"permanent"
);
}

private async buildApiHeaders(auth?: SourceControlAuthContext): Promise<HeadersInit> {
if (auth?.token) {
return {
Authorization: `Bearer ${auth.token}`,
Accept: "application/json",
};
}

return this.buildAppHeaders();
}

private async fetchJson<T>(url: string, init?: RequestInit): Promise<T> {
const response = await fetch(url, init);
if (!response.ok) {
const text = await response.text();
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.

Client-credentials token fetched on every app-level API call

buildAppHeaders calls getBitbucketClientCredentialsToken unconditionally on every invocation — there is no token caching:

if (this.config.clientId && this.config.clientSecret) {
  const token = await getBitbucketClientCredentialsToken({ ... }); // network call every time
  return { Authorization: `Bearer ${token.access_token}`, ... };
}

Because buildApiHeaders falls through to buildAppHeaders whenever no user auth is supplied, this means:

  • listRepositories (pagination loop): a new token is fetched from Bitbucket for each page.
  • generatePushAuth: also calls getBitbucketClientCredentialsToken directly — another separate token fetch per sandbox creation.
  • Repo-image scheduler: getBranchHeadSha is called in a Promise.all over all enabled repos, each triggering a fresh token fetch.

Client-credentials tokens are valid for at least one hour. The token should be cached inside the provider instance (or at module scope with an expiry check) so that multiple calls within the same request — or across requests within the same Worker instance — reuse it rather than hitting Bitbucket's token endpoint on every call.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/control-plane/src/source-control/providers/bitbucket-provider.ts
Line: 73-97

Comment:
**Client-credentials token fetched on every app-level API call**

`buildAppHeaders` calls `getBitbucketClientCredentialsToken` unconditionally on every invocation — there is no token caching:

```typescript
if (this.config.clientId && this.config.clientSecret) {
  const token = await getBitbucketClientCredentialsToken({ ... }); // network call every time
  return { Authorization: `Bearer ${token.access_token}`, ... };
}
```

Because `buildApiHeaders` falls through to `buildAppHeaders` whenever no user `auth` is supplied, this means:
- **`listRepositories`** (pagination loop): a new token is fetched from Bitbucket for *each page*.
- **`generatePushAuth`**: also calls `getBitbucketClientCredentialsToken` directly — another separate token fetch per sandbox creation.
- **Repo-image scheduler**: `getBranchHeadSha` is called in a `Promise.all` over all enabled repos, each triggering a fresh token fetch.

Client-credentials tokens are valid for at least one hour. The token should be cached inside the provider instance (or at module scope with an expiry check) so that multiple calls within the same request — or across requests within the same Worker instance — reuse it rather than hitting Bitbucket's token endpoint on every call.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Codex Fix in Cursor Fix in Claude Code

Comment on lines +214 to +229
targetBranch: data.destination?.branch?.name ?? config.targetBranch,
};
}

async checkRepositoryAccess(
config: GetRepositoryConfig,
auth?: SourceControlAuthContext
): Promise<RepositoryAccessResult | null> {
try {
const data = await this.fetchJson<BitbucketRepositoryResponse>(
`${BITBUCKET_API_BASE}/repositories/${encodeURIComponent(config.owner)}/${encodeURIComponent(config.name)}`,
{
headers: await this.buildApiHeaders(auth),
}
);
const repository = this.mapRepository(data);
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.

getBranchHeadSha skips the workspace enforcement check

Every other method that fetches repo data (checkRepositoryAccess, listBranches) calls ensureRepositoryInWorkspace before trusting the Bitbucket response. getBranchHeadSha fetches directly without that guard:

async getBranchHeadSha(config: GetBranchHeadShaConfig): Promise<string | null> {
  const data = await this.fetchJson<...>(
    `.../${config.owner}/${config.name}/refs/branches/...`,
    { headers: await this.buildAppHeaders() }   // no workspace check
  );
  return data.target?.hash ?? null;
}

While the repos passed to getBranchHeadSha today are sourced from resolveInstalledRepo (which already validates the workspace), the omission means:

  1. The contract documented on other methods is silently broken here.
  2. If getBranchHeadSha is called through the interface in a future code path, or if a cross-workspace repo somehow ends up in the DB, the branch SHA of an out-of-workspace repo would be resolved and returned without any error.

Adding the same workspace check that checkRepositoryAccess performs (by fetching and calling ensureRepositoryInWorkspace on the repo response first, or by reusing checkRepositoryAccess) would make the method consistent with the rest of the provider.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/control-plane/src/source-control/providers/bitbucket-provider.ts
Line: 214-229

Comment:
**`getBranchHeadSha` skips the workspace enforcement check**

Every other method that fetches repo data (`checkRepositoryAccess`, `listBranches`) calls `ensureRepositoryInWorkspace` before trusting the Bitbucket response. `getBranchHeadSha` fetches directly without that guard:

```typescript
async getBranchHeadSha(config: GetBranchHeadShaConfig): Promise<string | null> {
  const data = await this.fetchJson<...>(
    `.../${config.owner}/${config.name}/refs/branches/...`,
    { headers: await this.buildAppHeaders() }   // no workspace check
  );
  return data.target?.hash ?? null;
}
```

While the repos passed to `getBranchHeadSha` today are sourced from `resolveInstalledRepo` (which already validates the workspace), the omission means:
1. The contract documented on other methods is silently broken here.
2. If `getBranchHeadSha` is called through the interface in a future code path, or if a cross-workspace repo somehow ends up in the DB, the branch SHA of an out-of-workspace repo would be resolved and returned without any error.

Adding the same workspace check that `checkRepositoryAccess` performs (by fetching and calling `ensureRepositoryInWorkspace` on the repo response first, or by reusing `checkRepositoryAccess`) would make the method consistent with the rest of the provider.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Codex Fix in Cursor Fix in Claude Code

Comment on lines +207 to +211
}

function cacheBitbucketJwtState(keys: string[], state: CachedBitbucketJwtState): void {
for (const key of keys) {
cachedBitbucketJwtStates.set(key, state);
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.

Unbounded memory growth in cachedBitbucketJwtStates

The cacheBitbucketJwtState function only ever adds entries to the Map — it never removes stale or rotated keys. After every successful token rotation inside getRefreshableScmJwtToken, the new refresh token is registered as a new map key but the previous key is never deleted.

In a long-lived server process (self-hosted Next.js, warm Vercel function instances, Cloudflare Workers module workers), every token rotation leaves an orphaned entry behind. For a deployment with many active Bitbucket users refreshing tokens hourly, the Map grows without bound over time.

The fix is to delete the old initialCacheKeys from the map inside the .then() callback after the new keys have been registered, so only the current refresh-token key remains active per user state.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/web/src/lib/auth.ts
Line: 207-211

Comment:
**Unbounded memory growth in `cachedBitbucketJwtStates`**

The `cacheBitbucketJwtState` function only ever adds entries to the Map — it never removes stale or rotated keys. After every successful token rotation inside `getRefreshableScmJwtToken`, the new refresh token is registered as a new map key but the previous key is never deleted.

In a long-lived server process (self-hosted Next.js, warm Vercel function instances, Cloudflare Workers module workers), every token rotation leaves an orphaned entry behind. For a deployment with many active Bitbucket users refreshing tokens hourly, the Map grows without bound over time.

The fix is to delete the old `initialCacheKeys` from the map inside the `.then()` callback after the new keys have been registered, so only the current refresh-token key remains active per user state.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Codex Fix in Cursor Fix in Claude Code

@ColeMurray
Copy link
Copy Markdown
Owner

closing due to inactivity

@ColeMurray ColeMurray closed this Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants