Skip to content

Commit c80ee96

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
fix(bitbucket): accept provider diff redirect specs
1 parent 3ed291f commit c80ee96

4 files changed

Lines changed: 34 additions & 16 deletions

File tree

apps/sim/tools/bitbucket/pull-requests.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -713,9 +713,9 @@ describe('Bitbucket pull request diff safety', () => {
713713

714714
it('fetches an already-validated repository diffstat cursor directly', async () => {
715715
const nextUrl =
716-
'https://api.bitbucket.org/2.0/repositories/acme%20team/sdk%2Fcore/diffstat/main..feature?page=2'
716+
'https://api.bitbucket.org/2.0/repositories/acme%20team/sdk%2Fcore/diffstat/source-team/source-repo:6315b3bac849%0Decdc2efc4f27?page=2'
717717
serverMocks.resolveBitbucketPullRequestRedirect.mockResolvedValueOnce(
718-
'https://api.bitbucket.org/2.0/repositories/acme%20team/sdk%2Fcore/diffstat/main..feature'
718+
'https://api.bitbucket.org/2.0/repositories/acme%20team/sdk%2Fcore/diffstat/source-team/source-repo:6315b3bac849%0Decdc2efc4f27'
719719
)
720720
serverMocks.secureBitbucketRead.mockResolvedValueOnce(
721721
Response.json({ values: [RAW_DIFFSTAT], page: 2 })
@@ -739,14 +739,14 @@ describe('Bitbucket pull request diff safety', () => {
739739

740740
it('rejects a diffstat cursor for a different pull request revspec', async () => {
741741
serverMocks.resolveBitbucketPullRequestRedirect.mockResolvedValueOnce(
742-
'https://api.bitbucket.org/2.0/repositories/acme%20team/sdk%2Fcore/diffstat/main..feature'
742+
'https://api.bitbucket.org/2.0/repositories/acme%20team/sdk%2Fcore/diffstat/source-team/source-repo:6315b3bac849%0Decdc2efc4f27'
743743
)
744744

745745
await expect(
746746
bitbucketGetPullRequestDiffstatTool.directExecution!({
747747
...PULL_REQUEST_PARAMS,
748748
nextUrl:
749-
'https://api.bitbucket.org/2.0/repositories/acme%20team/sdk%2Fcore/diffstat/main..unrelated?page=2',
749+
'https://api.bitbucket.org/2.0/repositories/acme%20team/sdk%2Fcore/diffstat/source-team/source-repo:6315b3bac849%0Dunrelated?page=2',
750750
})
751751
).rejects.toThrow(/does not belong to this Bitbucket pull request diffstat/)
752752
expect(serverMocks.secureBitbucketRead).not.toHaveBeenCalled()

apps/sim/tools/bitbucket/utils.server.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ describe('resolveBitbucketPullRequestRedirect', () => {
161161
headers: {
162162
get: (name: string) =>
163163
name.toLowerCase() === 'location'
164-
? 'https://api.bitbucket.org/2.0/repositories/acme/demo/diff/main..feature'
164+
? 'https://api.bitbucket.org/2.0/repositories/acme/demo/diff/source-team/source-repo:6315b3bac849%0Decdc2efc4f27?from_pullrequest_id=7&topic=true'
165165
: null,
166166
},
167167
body: { cancel },
@@ -181,7 +181,7 @@ describe('resolveBitbucketPullRequestRedirect', () => {
181181
{ targetQuery: { path: 'src/index.ts', binary: 'false' } }
182182
)
183183
).resolves.toBe(
184-
'https://api.bitbucket.org/2.0/repositories/acme/demo/diff/main..feature?path=src%2Findex.ts&binary=false'
184+
'https://api.bitbucket.org/2.0/repositories/acme/demo/diff/source-team/source-repo:6315b3bac849%0Decdc2efc4f27?from_pullrequest_id=7&topic=true&path=src%2Findex.ts&binary=false'
185185
)
186186
expect(order).toEqual(['cancel', 'close'])
187187
})

apps/sim/tools/bitbucket/utils.test.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,11 @@ describe('Bitbucket path and pagination safety', () => {
163163
it('accepts PR redirects only for the requested repository and endpoint kind', () => {
164164
const diff = 'https://api.bitbucket.org/2.0/repositories/acme/demo/diff/main..feature'
165165
expect(validateBitbucketPullRequestRedirect(diff, 'acme', 'demo', 'diff')).toBe(diff)
166+
const providerQualified =
167+
'https://api.bitbucket.org/2.0/repositories/acme/demo/diff/source-team/source-repo:6315b3bac849%0Decdc2efc4f27?from_pullrequest_id=7&topic=true'
168+
expect(validateBitbucketPullRequestRedirect(providerQualified, 'acme', 'demo', 'diff')).toBe(
169+
providerQualified
170+
)
166171
expect(() =>
167172
validateBitbucketPullRequestRedirect(
168173
'https://api.bitbucket.org/2.0/repositories/acme/other/diff/main..feature',
@@ -173,12 +178,12 @@ describe('Bitbucket path and pagination safety', () => {
173178
).toThrow(/did not target/)
174179
expect(() =>
175180
validateBitbucketPullRequestRedirect(
176-
'https://api.bitbucket.org/2.0/repositories/acme/demo/diff/main..feature/extra',
181+
'https://api.bitbucket.org/2.0/repositories/acme/demo/diff/main..feature//extra',
177182
'acme',
178183
'demo',
179184
'diff'
180185
)
181-
).toThrow(/did not target/)
186+
).toThrow(/empty spec path segment/)
182187
expect(() =>
183188
validateBitbucketPullRequestRedirect(
184189
'https://api.bitbucket.org/2.0/repositories/acme/demo/diffstat/main..feature',

apps/sim/tools/bitbucket/utils.ts

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -321,18 +321,31 @@ export function validateBitbucketPullRequestRedirect(
321321
const validated = validateBitbucketOpaqueUrl(value)
322322
const parsed = new URL(validated)
323323
const expectedPrefix = `/2.0${bitbucketRepositoryPath(workspaceSlug, repoSlug)}/${kind}/`
324-
const encodedRevspec = parsed.pathname.startsWith(expectedPrefix)
324+
const encodedSpec = parsed.pathname.startsWith(expectedPrefix)
325325
? parsed.pathname.slice(expectedPrefix.length)
326326
: ''
327-
let decodedRevspec = ''
328-
try {
329-
decodedRevspec = decodeURIComponent(encodedRevspec)
330-
} catch {
331-
throw new Error(`Bitbucket ${kind} redirect contained invalid path encoding`)
332-
}
333-
if (!encodedRevspec || encodedRevspec.includes('/') || decodedRevspec.includes('/')) {
327+
if (!encodedSpec) {
334328
throw new Error(`Bitbucket ${kind} redirect did not target this repository's ${kind} endpoint`)
335329
}
330+
331+
/**
332+
* Bitbucket documents the redirect target as the repository `{kind}/{spec}`
333+
* endpoint. In live responses that opaque spec is repository-qualified, for
334+
* example `source-workspace/source-repo:sourceHash%0DdestinationHash`, so it
335+
* legitimately spans multiple URL path segments. The exact API origin and
336+
* destination repository remain bound above; only the trailing spec is
337+
* treated as provider-owned opaque path data.
338+
*/
339+
for (const segment of encodedSpec.split('/')) {
340+
if (!segment) {
341+
throw new Error(`Bitbucket ${kind} redirect contained an empty spec path segment`)
342+
}
343+
try {
344+
decodeURIComponent(segment)
345+
} catch {
346+
throw new Error(`Bitbucket ${kind} redirect contained invalid path encoding`)
347+
}
348+
}
336349
return parsed.toString()
337350
}
338351

0 commit comments

Comments
 (0)