Skip to content

Commit 3a87681

Browse files
fix(gitcafe): PR panel follows renumbered pull requests (#26079)
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 9ff0597 commit 3a87681

2 files changed

Lines changed: 101 additions & 24 deletions

File tree

‎packages/source-control-gitcafe/src/server/GitCafeApi.ts‎

Lines changed: 66 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import * as GitCafeCredentials from "./GitCafeCredentials.ts";
1313
import * as GitCafeHosts from "./gitCafeHosts.ts";
1414

1515
const NO_REDIRECT: RequestInit = { redirect: "manual" };
16+
const MAX_REDIRECTS = 3;
1617
const TIMEOUT = Duration.seconds(30);
1718

1819
export const GitCafeApiFailure = Schema.Literals([
@@ -85,6 +86,28 @@ const credentialFailure = (
8586
? "unauthenticated"
8687
: "failed";
8788

89+
const REDIRECTS = new Set([301, 302, 303, 307, 308]);
90+
// 307 and 308 keep the method and body; the others only safely repeat a read.
91+
const METHOD_PRESERVING = new Set([307, 308]);
92+
93+
const redirectTarget = (
94+
method: string,
95+
origin: string,
96+
from: string,
97+
response: { readonly status: number; readonly location: string | undefined },
98+
): string | null => {
99+
if (!REDIRECTS.has(response.status) || response.location === undefined) return null;
100+
if (method !== "GET" && !METHOD_PRESERVING.has(response.status)) return null;
101+
const target = URL.parse(response.location, from);
102+
return target !== null &&
103+
target.origin === origin &&
104+
target.username === "" &&
105+
target.password === "" &&
106+
target.pathname.startsWith("/api/")
107+
? target.href
108+
: null;
109+
};
110+
88111
/** @public */
89112
export const make = Effect.gen(function* () {
90113
const httpClient = yield* HttpClient.HttpClient;
@@ -122,30 +145,49 @@ export const make = Effect.gen(function* () {
122145
fail({ status: null, reason: credentialFailure(cause), detail: cause.message, cause }),
123146
),
124147
);
125-
const base = HttpClientRequest.make(input.method ?? "GET")(
126-
`https://${host}/api${input.path}`,
127-
).pipe(
128-
HttpClientRequest.bearerToken(Redacted.value(credential.token)),
129-
HttpClientRequest.setHeaders({ accept: "application/json", "user-agent": "t3code" }),
130-
);
131-
const { status, text } = yield* Effect.gen(function* () {
132-
const response = yield* httpClient
133-
.execute(
134-
input.body === undefined ? base : base.pipe(HttpClientRequest.bodyJsonUnsafe(input.body)),
135-
)
136-
.pipe(
137-
// Client spans expose query strings containing branch names.
138-
Effect.provideService(HttpClient.TracerDisabledWhen, () => true),
139-
// GitCafe does not redirect; reject 3xx to protect bearer credentials.
140-
Effect.provideService(FetchHttpClient.RequestInit, NO_REDIRECT),
141-
);
142-
return { status: response.status, text: yield* response.text };
143-
}).pipe(
144-
Effect.timeout(TIMEOUT),
145-
Effect.mapError((cause) =>
146-
fail({ status: null, reason: "failed", detail: "Could not reach GitCafe.", cause }),
147-
),
148-
);
148+
const origin = `https://${host}`;
149+
const method = input.method ?? "GET";
150+
const send = (url: string) => {
151+
const base = HttpClientRequest.make(method)(url).pipe(
152+
HttpClientRequest.bearerToken(Redacted.value(credential.token)),
153+
HttpClientRequest.setHeaders({ accept: "application/json", "user-agent": "t3code" }),
154+
);
155+
return Effect.gen(function* () {
156+
const response = yield* httpClient
157+
.execute(
158+
input.body === undefined
159+
? base
160+
: base.pipe(HttpClientRequest.bodyJsonUnsafe(input.body)),
161+
)
162+
.pipe(
163+
// Client spans expose query strings containing branch names.
164+
Effect.provideService(HttpClient.TracerDisabledWhen, () => true),
165+
// fetch would carry the bearer token to any redirect target.
166+
Effect.provideService(FetchHttpClient.RequestInit, NO_REDIRECT),
167+
);
168+
return {
169+
status: response.status,
170+
location: response.headers.location,
171+
text: yield* response.text,
172+
};
173+
}).pipe(
174+
Effect.timeout(TIMEOUT),
175+
Effect.mapError((cause) =>
176+
fail({ status: null, reason: "failed", detail: "Could not reach GitCafe.", cause }),
177+
),
178+
);
179+
};
180+
// A renumbered pull request (provisional number replaced by the synced GitHub one)
181+
// answers with a redirect; follow it only within this host's API.
182+
let url = `${origin}/api${input.path}`;
183+
let response = yield* send(url);
184+
for (let hop = 0; hop < MAX_REDIRECTS; hop++) {
185+
const next = redirectTarget(method, origin, url, response);
186+
if (next === null) break;
187+
url = next;
188+
response = yield* send(url);
189+
}
190+
const { status, text } = response;
149191
yield* Effect.annotateCurrentSpan({ "http.response.status_code": status });
150192
if (status >= 200 && status < 300) return text;
151193
// The source may hold a newer token than the one that was refused.

‎packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,41 @@ describe("GitCafePullRequestProvider", () => {
534534
);
535535
});
536536

537+
it.effect("follows a renumbered pull request's redirect only within the host's API", () =>
538+
Effect.gen(function* () {
539+
const moved = (location: string) =>
540+
new Response(null, { status: 301, headers: { location } });
541+
const renumbered = fakeGitCafe({
542+
"GET /repos/owner/repo/pulls/7/stack": moved("/api/repos/owner/repo/pulls/8/stack"),
543+
"GET /repos/owner/repo/pulls/8/stack": { stack: null },
544+
});
545+
yield* GitCafePullRequestProvider.make.pipe(
546+
Effect.flatMap((provider) => provider.getChangeRequestStack!(target)),
547+
Effect.provide(renumbered.layer),
548+
);
549+
assert.deepStrictEqual(
550+
renumbered.sent.map((r) => `${r.host}${r.path}`),
551+
["git.cafe/repos/owner/repo/pulls/7/stack", "git.cafe/repos/owner/repo/pulls/8/stack"],
552+
);
553+
554+
for (const location of [
555+
"https://evil.example/api/repos/owner/repo/pulls/8/stack",
556+
"//evil.example/api/repos/owner/repo/pulls/8/stack",
557+
"https://staging.git.cafe/api/repos/owner/repo/pulls/8/stack",
558+
"/repos/owner/repo/pulls/8/stack",
559+
]) {
560+
const elsewhere = fakeGitCafe({ "GET /repos/owner/repo/pulls/7/stack": moved(location) });
561+
const error = yield* GitCafePullRequestProvider.make.pipe(
562+
Effect.flatMap((provider) => provider.getChangeRequestStack!(target)),
563+
Effect.flip,
564+
Effect.provide(elsewhere.layer),
565+
);
566+
assert.strictEqual(error.detail, "GitCafe answered HTTP 301.");
567+
assert.strictEqual(elsewhere.sent.length, 1);
568+
}
569+
}),
570+
);
571+
537572
it.effect("reads the stack a pull request belongs to, with each open layer's head", () => {
538573
const { "GET /repos/owner/repo/pulls/6": _merged, ...routes } = stack([
539574
{ number: 6, headOid: baseOid, state: "merged" },

0 commit comments

Comments
 (0)