From 1e24a280c1a54d6dce969bc984bf698625f6576b Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 24 Jul 2026 19:40:34 +0800 Subject: [PATCH] fix(github): reject malformed repoFullName in app.ts and comments.ts app.ts's three GitHub-write call sites (getRepositoryCollaboratorPermission, cancelInFlightWorkflowRunsForHeadSha, createOrUpdateNamedCheckRun) did a bare two-variable destructure with only a truthiness check, so "owner/repo/extra" silently dropped the extra segment and issued a call against a different repo, and a padded "owner/ repo" was encodeURIComponent-ed straight into a GitHub URL. comments.ts had the segment-count guard but not the whitespace one. Adds a local parseRepoFullNameStrict helper in app.ts (per this directory's house convention of a small per-module copy rather than a shared export) used by all three call sites, each preserving its existing failure contract, and adds the whitespace condition to comments.ts's existing check. Regression tests cover the extra-segment and whitespace-padded shapes at all four call sites. Closes #8311 --- src/github/app.ts | 31 +++++++++++++++++------ src/github/comments.ts | 7 ++++-- test/unit/github-app.test.ts | 42 +++++++++++++++++++++++++++++++ test/unit/github-comments.test.ts | 16 ++++++++++++ 4 files changed, 87 insertions(+), 9 deletions(-) diff --git a/src/github/app.ts b/src/github/app.ts index 47ceae6916..410fbe6824 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -437,14 +437,30 @@ export type GitHubRepositoryCollaboratorPermission = | "none" | string; +// Parse `owner/repo` into its two segments, rejecting any shape that is not exactly two non-empty, +// whitespace-free segments -- the identical guard every sibling GitHub-write module in this directory keeps +// its own local copy of (assignees.ts / labels.ts / issues.ts / milestones.ts, per this dir's house +// convention). "owner/repo/extra" would otherwise silently drop the extra segment and hit a different repo; +// "owner/ repo" / " owner/repo" would get encodeURIComponent-ed straight into a GitHub URL. Returns null so +// each caller can map a malformed value to its own established failure contract (return null / error object / +// throw) rather than sharing one. +function parseRepoFullNameStrict(repoFullName: string): { owner: string; repo: string } | null { + const parts = repoFullName.split("/"); + const owner = parts[0]; + const repo = parts[1]; + if (parts.length !== 2 || !owner || !repo || /\s/.test(repoFullName)) return null; + return { owner, repo }; +} + export async function getRepositoryCollaboratorPermission( env: Env, installationId: number, repoFullName: string, login: string, ): Promise { - const [owner, name] = repoFullName.split("/"); - if (!owner || !name || !login) return null; + const parsed = parseRepoFullNameStrict(repoFullName); + if (!parsed || !login) return null; + const { owner, repo: name } = parsed; const token = await createInstallationToken(env, installationId); const response = await timeoutFetch( `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/collaborators/${encodeURIComponent(login)}/permission`, @@ -617,8 +633,9 @@ export async function cancelInFlightWorkflowRunsForHeadSha( headSha: string, pullNumber: number, ): Promise { - const [owner, repo] = repoFullName.split("/"); - if (!owner || !repo) return { kind: "error", warning: `Invalid repository full name: ${repoFullName}` }; + const parsed = parseRepoFullNameStrict(repoFullName); + if (!parsed) return { kind: "error", warning: `Invalid repository full name: ${repoFullName}` }; + const { owner, repo } = parsed; const repoPath = `${encodeURIComponent(owner)}/${encodeURIComponent(repo)}`; try { const token = await createInstallationToken(env, installationId); @@ -908,9 +925,9 @@ async function createOrUpdateNamedCheckRun( if (!advisory.headSha) return null; // Narrow once into a const so the postNewCheckRun closure below sees a string, not string | undefined. const headSha = advisory.headSha; - const [owner, repo] = repoFullName.split("/"); - if (!owner || !repo) - throw new Error(`Invalid repository full name: ${repoFullName}`); + const parsed = parseRepoFullNameStrict(repoFullName); + if (!parsed) throw new Error(`Invalid repository full name: ${repoFullName}`); + const { owner, repo } = parsed; return await withInstallationTokenRetry(env, installationId, async (token) => { // makeInstallationOctokit injects the shared per-request timeout (a stalled PATCH can never orphan the diff --git a/src/github/comments.ts b/src/github/comments.ts index f19fb55a57..eeb8227713 100644 --- a/src/github/comments.ts +++ b/src/github/comments.ts @@ -86,8 +86,11 @@ async function createOrUpdateIssueCommentWithMarker( const repo = parts[1]; // Reject anything that is not exactly two non-empty segments -- "owner/repo/extra" would otherwise pass // (the destructure silently drops the extra segment), issuing a call against a repo the caller never - // specified. Matches the segment-count guard in parseRepoFullName (assignees.ts / labels.ts). - if (parts.length !== 2 || !owner || !repo) throw new Error(`Invalid repository full name: ${repoFullName}`); + // specified -- and additionally reject whitespace (`owner/ repo`, ` owner/repo`) so a padded slug can never + // reach a GitHub call. Matches the full segment-count + whitespace guard in parseRepoFullName + // (assignees.ts / labels.ts, #6613). + if (parts.length !== 2 || !owner || !repo || /\s/.test(repoFullName)) + throw new Error(`Invalid repository full name: ${repoFullName}`); return await withInstallationTokenRetry(env, installationId, async (token) => { // Non-live mode suppresses the comment create/update writes; the GET marker-search probe below still runs. diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 7d0ec4affa..512b1b55b0 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -2855,3 +2855,45 @@ describe("GitHub rate-limit handling (#ratelimit-resilience)", () => { expect(calls).toBe(4); // initial + GITHUB_RATE_LIMIT_MAX_RETRIES (3) }); }); + +describe("repoFullName segment-count + whitespace guard (#8311)", () => { + // Each of these malformed shapes must be rejected at every app.ts call site the same way the existing + // "invalid" (no-slash) case already is, matching the guard pr-actions.ts/assignees.ts/labels.ts share. + // The rejects happen before any GitHub call, so no fetch stub is needed. Inputs collectively exercise all + // four operands of the guard: parts.length !== 2 ("owner/repo/extra"), !owner ("/repo"), !repo ("owner/"), + // and the whitespace check ("owner/ repo", " owner/repo"). + const MALFORMED = ["owner/repo/extra", "owner/ repo", " owner/repo", "/repo", "owner/"]; + + it("getRepositoryCollaboratorPermission returns null for extra-segment and whitespace-padded slugs", async () => { + const privateKey = await generatePrivateKeyPem(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + for (const repoFullName of MALFORMED) { + await expect( + getRepositoryCollaboratorPermission(env, 123, repoFullName, "maintainer"), + ).resolves.toBeNull(); + } + // A well-formed slug still passes the guard (and only then fails downstream on the un-stubbed fetch). + await expect( + getRepositoryCollaboratorPermission(env, 123, "JSONbored/gittensory", "maintainer"), + ).rejects.toThrow(); + }); + + it("cancelInFlightWorkflowRunsForHeadSha returns an error outcome for extra-segment and whitespace-padded slugs", async () => { + const privateKey = await generatePrivateKeyPem(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + for (const repoFullName of ["owner/repo/extra", "owner/ repo"]) { + const outcome = await cancelInFlightWorkflowRunsForHeadSha(env, 123, repoFullName, "abc123", 55); + expect(outcome).toEqual({ kind: "error", warning: `Invalid repository full name: ${repoFullName}` }); + } + }); + + it("createOrUpdateCheckRun (createOrUpdateNamedCheckRun) throws for extra-segment and whitespace-padded slugs", async () => { + const privateKey = await generatePrivateKeyPem(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + for (const repoFullName of ["owner/repo/extra", "owner/ repo"]) { + await expect( + createOrUpdateCheckRun(env, 123, repoFullName, gateAdvisory("abc123")), + ).rejects.toThrow(`Invalid repository full name: ${repoFullName}`); + } + }); +}); diff --git a/test/unit/github-comments.test.ts b/test/unit/github-comments.test.ts index 59999b3e08..f4152846a6 100644 --- a/test/unit/github-comments.test.ts +++ b/test/unit/github-comments.test.ts @@ -519,3 +519,19 @@ async function generatePrivateKeyPem(): Promise { const base64 = Buffer.from(exported as ArrayBuffer).toString("base64").replace(/(.{64})/g, "$1\n"); return `-----BEGIN PRIVATE KEY-----\n${base64}\n-----END PRIVATE KEY-----`; } + +describe("createOrUpdateIssueCommentWithMarker repoFullName guard (#8311)", () => { + // The existing segment-count guard now also rejects whitespace, matching pr-actions.ts/assignees.ts/ + // labels.ts (#6613). These malformed shapes reject before any GitHub call (no fetch stub needed) and + // collectively exercise all four operands: parts.length !== 2 ("owner/repo/extra"), !owner ("/repo"), + // !repo ("owner/"), and the newly-added whitespace check ("owner/ repo", " owner/repo"). + it("throws for extra-segment and whitespace-padded slugs", async () => { + const privateKey = await generatePrivateKeyPem(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + for (const repoFullName of ["owner/repo/extra", "/repo", "owner/", "owner/ repo", " owner/repo"]) { + await expect( + createOrUpdatePrIntelligenceComment(env, 123, repoFullName, 12, `${PR_INTELLIGENCE_COMMENT_MARKER}\nbody`), + ).rejects.toThrow(`Invalid repository full name: ${repoFullName}`); + } + }); +});