From ef6a024513bd967c2672d3d2e3962bf2f9fbb4bd Mon Sep 17 00:00:00 2001 From: Alexander Adam Date: Thu, 24 Sep 2026 15:44:19 +0200 Subject: [PATCH] refactor(forge): share fork branch naming with github moved the owner normalizer and the branch name builder to utils/change-request-checkout.ts, gitea and github both use it. github changes a bit: a fork PR with an unknown or unusable owner (eg deleted fork) now gets pr-N/ instead of the bare branch name, which could collide with a local branch the same way #3515 did. --- packages/server/src/services/forge-service.ts | 8 +- .../server/src/services/gitea-service.test.ts | 95 ------------------- packages/server/src/services/gitea-service.ts | 21 +--- .../server/src/services/github-service.ts | 11 +-- .../src/utils/change-request-checkout.test.ts | 64 +++++++++++++ .../src/utils/change-request-checkout.ts | 27 ++++++ 6 files changed, 99 insertions(+), 127 deletions(-) create mode 100644 packages/server/src/utils/change-request-checkout.test.ts create mode 100644 packages/server/src/utils/change-request-checkout.ts diff --git a/packages/server/src/services/forge-service.ts b/packages/server/src/services/forge-service.ts index fba9833cd8..a3e70cd5d1 100644 --- a/packages/server/src/services/forge-service.ts +++ b/packages/server/src/services/forge-service.ts @@ -441,9 +441,9 @@ export interface ForgeService { getPullRequestCheckoutTarget(options: GetPullRequestOptions): Promise; /** * Refs to fetch for a change-request checkout when the resolved checkout - * target carries none. Adapters that expose a universal change-request head - * ref (GitHub's refs/pull/N/head) return it here; others let the shell fall - * back to the head branch. + * target carries none. Adapters with a universal change-request head ref + * (refs/pull/N/head) return it here; others let the shell fall back to the + * head branch. */ defaultCheckoutRefs?(params: { changeRequestNumber: number; @@ -451,7 +451,7 @@ export interface ForgeService { }): PullRequestCheckoutRef[]; /** * Local branch name for a checked-out change request when the adapter - * disambiguates cross-repository heads (GitHub prefixes the fork owner). + * disambiguates cross-repository heads (prefixes the fork owner). * Returns undefined to keep the head ref name as-is. */ buildPrLocalBranchName?(params: { diff --git a/packages/server/src/services/gitea-service.test.ts b/packages/server/src/services/gitea-service.test.ts index b0484b3cb5..bc7b28a06c 100644 --- a/packages/server/src/services/gitea-service.test.ts +++ b/packages/server/src/services/gitea-service.test.ts @@ -2359,101 +2359,6 @@ describe("createGiteaService", () => { expect(localBranchName).toBe("contributor/patch-1"); }); - it("keeps the head ref name as-is for a same-repository checkout", () => { - const { service } = makeService(() => ok("")); - - const localBranchName = service.buildPrLocalBranchName?.({ - headRef: "patch-1", - checkoutTarget: { - number: 5, - baseRefName: "main", - headRefName: "patch-1", - headOwnerLogin: null, - headRepositorySshUrl: null, - headRepositoryUrl: null, - isCrossRepository: false, - }, - }); - - expect(localBranchName).toBe("patch-1"); - }); - - it("falls back to a PR-number prefix when the fork owner login is not a safe branch path segment", () => { - const { service } = makeService(() => ok("")); - - const localBranchName = service.buildPrLocalBranchName?.({ - headRef: "patch-1", - checkoutTarget: { - number: 5, - baseRefName: "main", - headRefName: "patch-1", - headOwnerLogin: "../evil", - headRepositorySshUrl: null, - headRepositoryUrl: null, - isCrossRepository: true, - }, - }); - - expect(localBranchName).toBe("pr-5/patch-1"); - }); - - it("falls back to a PR-number prefix when the fork owner login would produce an invalid ref", () => { - const { service } = makeService(() => ok("")); - - const localBranchName = service.buildPrLocalBranchName?.({ - headRef: "patch-1", - checkoutTarget: { - number: 5, - baseRefName: "main", - headRefName: "patch-1", - headOwnerLogin: "contributor.lock", - headRepositorySshUrl: null, - headRepositoryUrl: null, - isCrossRepository: true, - }, - }); - - expect(localBranchName).toBe("pr-5/patch-1"); - }); - - it("falls back to a PR-number prefix when the fork owner login is unknown", () => { - const { service } = makeService(() => ok("")); - - const localBranchName = service.buildPrLocalBranchName?.({ - headRef: "patch-1", - checkoutTarget: { - number: 5, - baseRefName: "main", - headRefName: "patch-1", - headOwnerLogin: null, - headRepositorySshUrl: null, - headRepositoryUrl: null, - isCrossRepository: true, - }, - }); - - expect(localBranchName).toBe("pr-5/patch-1"); - }); - - it("lowercases the fork owner login for the branch prefix", () => { - const { service } = makeService(() => ok("")); - - const localBranchName = service.buildPrLocalBranchName?.({ - headRef: "patch-1", - checkoutTarget: { - number: 5, - baseRefName: "main", - headRefName: "patch-1", - headOwnerLogin: "Contributor", - headRepositorySshUrl: null, - headRepositoryUrl: null, - isCrossRepository: true, - }, - }); - - expect(localBranchName).toBe("contributor/patch-1"); - }); - it("returns the pull head ref for default checkout refs", () => { const { service } = makeService(() => ok("")); diff --git a/packages/server/src/services/gitea-service.ts b/packages/server/src/services/gitea-service.ts index 281b5357d8..0e975ca1da 100644 --- a/packages/server/src/services/gitea-service.ts +++ b/packages/server/src/services/gitea-service.ts @@ -5,6 +5,7 @@ import pLimit from "p-limit"; import { parseGitHubRemoteIdentity, parseGitRemoteLocation } from "@getpaseo/protocol/git-remote"; import { findExecutable } from "../executable-resolution/executable-resolution.js"; import { runGitCommand } from "../utils/run-git-command.js"; +import { buildForkLocalBranchName } from "../utils/change-request-checkout.js"; import { execCommand } from "../utils/spawn.js"; import { createCachedCliPathResolver, @@ -1377,19 +1378,6 @@ function throwFirstNonTeaAuthSearchRejection(results: PromiseSettledResult { + it("lowercases a valid login", () => { + expect(normalizeForgeOwnerForBranch("Contributor")).toBe("contributor"); + }); + + it("allows dots and underscores", () => { + expect(normalizeForgeOwnerForBranch("a.b_c")).toBe("a.b_c"); + }); + + it("rejects a login starting with a dash", () => { + expect(normalizeForgeOwnerForBranch("-abc")).toBeNull(); + }); + + it("rejects a login starting with a dot", () => { + expect(normalizeForgeOwnerForBranch(".hidden")).toBeNull(); + }); + + it("rejects a login containing '..'", () => { + expect(normalizeForgeOwnerForBranch("a..b")).toBeNull(); + }); + + it("rejects a login ending in .lock", () => { + expect(normalizeForgeOwnerForBranch("contributor.lock")).toBeNull(); + }); + + it("rejects a login with a path traversal segment", () => { + expect(normalizeForgeOwnerForBranch("../evil")).toBeNull(); + }); + + it("rejects an empty or null login", () => { + expect(normalizeForgeOwnerForBranch(null)).toBeNull(); + expect(normalizeForgeOwnerForBranch("")).toBeNull(); + expect(normalizeForgeOwnerForBranch(" ")).toBeNull(); + }); +}); + +describe("buildForkLocalBranchName", () => { + const fork = { headRef: "patch-1", number: 42, isCrossRepository: true }; + + it("keeps the head ref for a same-repository head", () => { + expect( + buildForkLocalBranchName({ ...fork, isCrossRepository: false, headOwnerLogin: "arthur" }), + ).toBe("patch-1"); + }); + + it("prefixes a fork head with the lowercased owner", () => { + expect(buildForkLocalBranchName({ ...fork, headOwnerLogin: "Arthur" })).toBe("arthur/patch-1"); + }); + + it("falls back to the pr number when the owner is unknown", () => { + expect(buildForkLocalBranchName({ ...fork, headOwnerLogin: null })).toBe("pr-42/patch-1"); + }); + + it("falls back to the pr number when the owner is not a safe ref segment", () => { + expect(buildForkLocalBranchName({ ...fork, headOwnerLogin: "-zaphod" })).toBe("pr-42/patch-1"); + }); +}); diff --git a/packages/server/src/utils/change-request-checkout.ts b/packages/server/src/utils/change-request-checkout.ts new file mode 100644 index 0000000000..cf5dcaa8fa --- /dev/null +++ b/packages/server/src/utils/change-request-checkout.ts @@ -0,0 +1,27 @@ +// forgejo username pattern, covers github logins too +export function normalizeForgeOwnerForBranch(owner: string | null): string | null { + const normalized = owner?.trim().toLowerCase() ?? ""; + if (!/^[a-z0-9][a-z0-9_.-]*$/.test(normalized)) { + return null; + } + // git refuses .. and .lock, trailing dot too + if (normalized.includes("..") || normalized.endsWith(".") || normalized.endsWith(".lock")) { + return null; + } + return normalized; +} + +export interface ForkLocalBranchNameInput { + headRef: string; + number: number; + isCrossRepository: boolean; + headOwnerLogin: string | null; +} + +export function buildForkLocalBranchName(params: ForkLocalBranchNameInput): string { + if (!params.isCrossRepository) { + return params.headRef; + } + const owner = normalizeForgeOwnerForBranch(params.headOwnerLogin); + return `${owner ?? `pr-${params.number}`}/${params.headRef}`; +}