Skip to content

Commit e36563c

Browse files
committed
fix(github): guard backfill GraphQL helpers' repoFullName parsing
fetchLiveCiAggregateViaGraphQl, fetchLivePullRequestReviewDecision, and fetchLiveReviewThreadBlockers parsed repoFullName with a bare split/truthiness check, so extra segments and whitespace-padded slugs reached GraphQL queries. Add a local parseBackfillRepoFullName helper mirroring #8311's segment-count and whitespace guard, preserving each call site's fail-soft return contract. Closes #9317
1 parent d158034 commit e36563c

3 files changed

Lines changed: 48 additions & 6 deletions

File tree

src/github/backfill.ts

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3276,6 +3276,19 @@ export function isStatusRollupGraphQlEnabled(env: { GITHUB_STATUS_ROLLUP_GRAPHQL
32763276
return /^(1|true|yes|on)$/i.test(env.GITHUB_STATUS_ROLLUP_GRAPHQL ?? "");
32773277
}
32783278

3279+
// Mirrors parseRepoFullName in labels.ts / assignees.ts (#8311): backfill keeps its own local copy rather than
3280+
// importing a shared one. Returns null for malformed input so each GraphQL read helper preserves its existing
3281+
// fail-soft return contract (null / undefined / []).
3282+
function parseBackfillRepoFullName(repoFullName: string): { owner: string; name: string } | null {
3283+
const parts = repoFullName.split("/");
3284+
const owner = parts[0];
3285+
const name = parts[1];
3286+
if (parts.length !== 2 || !owner || !name || /\s/.test(repoFullName)) {
3287+
return null;
3288+
}
3289+
return { owner, name };
3290+
}
3291+
32793292
/**
32803293
* GraphQL equivalent of {@link fetchLiveCiAggregate}: ONE bounded query returns the head commit's statusCheckRollup
32813294
* (check-runs AND classic statuses, unified) plus its check-suites — replacing the paginated /check-runs + /status
@@ -3295,8 +3308,9 @@ export async function fetchLiveCiAggregateViaGraphQl(
32953308
advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null,
32963309
): Promise<LiveCiAggregate | null> {
32973310
if (!headSha || !token) return null;
3298-
const [owner, name] = repoFullName.split("/");
3299-
if (!owner || !name) return null;
3311+
const parsed = parseBackfillRepoFullName(repoFullName);
3312+
if (!parsed) return null;
3313+
const { owner, name } = parsed;
33003314
const query = `query LoopOverLiveCiRollup { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { object(oid: ${JSON.stringify(headSha)}) { ... on Commit { statusCheckRollup { contexts(first: 100) { nodes { __typename ... on CheckRun { name conclusion status startedAt detailsUrl title summary checkSuite { databaseId app { slug } } } ... on StatusContext { context state description targetUrl } } pageInfo { hasNextPage } } } checkSuites(first: 100) { nodes { status app { slug } } pageInfo { hasNextPage } } } } } }`;
33013315
const result = await githubGraphQl<{
33023316
data?: {
@@ -4058,8 +4072,9 @@ export async function fetchLivePullRequestReviewDecision(
40584072
admissionKey?: GitHubRateLimitAdmissionKey,
40594073
): Promise<string | undefined> {
40604074
if (!token) return undefined;
4061-
const [owner, name] = repoFullName.split("/");
4062-
if (!owner || !name) return undefined;
4075+
const parsed = parseBackfillRepoFullName(repoFullName);
4076+
if (!parsed) return undefined;
4077+
const { owner, name } = parsed;
40634078
const query = `query { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { pullRequest(number: ${prNumber}) { reviewDecision } } }`;
40644079
const result = await githubGraphQl<{ data?: { repository?: { pullRequest?: { reviewDecision?: string | null } | null } | null }; errors?: unknown[] }>(
40654080
env,
@@ -4130,8 +4145,9 @@ export async function fetchLiveReviewThreadBlockers(
41304145
admissionKey?: GitHubRateLimitAdmissionKey,
41314146
): Promise<ReviewThreadBlocker[]> {
41324147
if (!token) return [];
4133-
const [owner, name] = repoFullName.split("/");
4134-
if (!owner || !name) return [];
4148+
const parsed = parseBackfillRepoFullName(repoFullName);
4149+
if (!parsed) return [];
4150+
const { owner, name } = parsed;
41354151
const threads: Array<GitHubReviewThreadNode | null> = [];
41364152
let cursor: string | null = null;
41374153
const seenCursors = new Set<string>();

test/unit/backfill-2.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1660,6 +1660,18 @@ describe("GitHub backfill", () => {
16601660
// (`liveReviewDecision ?? pr.reviewDecision`), so a PR approved at backfill time and later flipped to
16611661
// CHANGES_REQUESTED still read as APPROVED -> approvalsSatisfied -> merge. The sentinel is a real VALUE
16621662
// precisely so it survives that `??` and the stale stored decision can never be substituted.
1663+
describe("fetchLivePullRequestReviewDecision — repoFullName guard (#9317)", () => {
1664+
it("returns undefined for extra-segment and whitespace-padded slugs before any GraphQL call", async () => {
1665+
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
1666+
const fetchSpy = vi.fn(async () => Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }));
1667+
vi.stubGlobal("fetch", fetchSpy);
1668+
for (const repoFullName of ["owner/repo/extra", "owner/ repo", " owner/repo"]) {
1669+
expect(await fetchLivePullRequestReviewDecision(env, repoFullName, 7, "public-token")).toBeUndefined();
1670+
}
1671+
expect(fetchSpy).not.toHaveBeenCalled();
1672+
});
1673+
});
1674+
16631675
describe("fetchLivePullRequestReviewDecision — partial-response guard (#9052)", () => {
16641676
it("returns the unreadable sentinel on a 200-with-errors response, so the stale stored decision cannot win the ?? fallback", async () => {
16651677
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
@@ -2528,6 +2540,10 @@ describe("GitHub backfill", () => {
25282540
expect(fetchSpy).not.toHaveBeenCalled();
25292541
await expect(fetchLiveReviewThreadBlockers(env, "malformed", 1, "public-token")).resolves.toEqual([]);
25302542
expect(fetchSpy).not.toHaveBeenCalled();
2543+
for (const repoFullName of ["owner/repo/extra", "owner/ repo", " owner/repo"]) {
2544+
await expect(fetchLiveReviewThreadBlockers(env, repoFullName, 1, "public-token")).resolves.toEqual([]);
2545+
}
2546+
expect(fetchSpy).not.toHaveBeenCalled();
25312547
await expect(fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1, "public-token")).resolves.toEqual([]);
25322548
expect(fetchSpy).toHaveBeenCalledTimes(1);
25332549
});

test/unit/graphql-status-rollup.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,16 @@ describe("fetchLiveCiAggregateViaGraphQl — verdicts", () => {
7676
expect(await fetchLiveCiAggregateViaGraphQl(env, "no-slash", SHA, TOKEN)).toBeNull();
7777
});
7878

79+
// #9317: segment-count + whitespace guard, matching pr-actions.ts/assignees.ts/labels.ts (#8311).
80+
it("returns null for extra-segment and whitespace-padded repo slugs before any GraphQL call (#9317)", async () => {
81+
const fetchSpy = vi.fn(async () => Response.json(graphqlBody({ runs: [{ name: "build", conclusion: "SUCCESS", status: "COMPLETED" }] })));
82+
vi.stubGlobal("fetch", fetchSpy);
83+
for (const repoFullName of ["owner/repo/extra", "owner/ repo", " owner/repo"]) {
84+
expect(await fetchLiveCiAggregateViaGraphQl(env, repoFullName, SHA, TOKEN)).toBeNull();
85+
}
86+
expect(fetchSpy).not.toHaveBeenCalled();
87+
});
88+
7989
it("returns null on a GraphQL error or an unexpected/absent commit (→ REST fallback)", async () => {
8090
stubGraphql(graphqlBody(), { status: 500 });
8191
expect(await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN)).toBeNull();

0 commit comments

Comments
 (0)