Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions src/github/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<GitHubRepositoryCollaboratorPermission | null> {
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`,
Expand Down Expand Up @@ -617,8 +633,9 @@ export async function cancelInFlightWorkflowRunsForHeadSha(
headSha: string,
pullNumber: number,
): Promise<CancelWorkflowRunsOutcome> {
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);
Expand Down Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions src/github/comments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
42 changes: 42 additions & 0 deletions test/unit/github-app.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
}
});
});
16 changes: 16 additions & 0 deletions test/unit/github-comments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -519,3 +519,19 @@ async function generatePrivateKeyPem(): Promise<string> {
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}`);
}
});
});