diff --git a/apps/api/src/lib/git-forwarding/relay.ts b/apps/api/src/lib/git-forwarding/relay.ts index 87b854228..ae5096b45 100644 --- a/apps/api/src/lib/git-forwarding/relay.ts +++ b/apps/api/src/lib/git-forwarding/relay.ts @@ -181,7 +181,11 @@ function handleConnection( void getLocalGhToken() .then((token) => { if (!token) return finish(null, "no-token", meta); - // GitHub HTTPS token auth: token as password, any non-empty username. + // GitHub HTTPS token auth: it reads the token from either Basic-auth + // slot and ignores the other value, so the username here is a label. + // URL-form auth picks the pair GitHub documents per token type — + // see gitCredentialPair (packages/adapters/src/runtime/git-clone.ts); + // this is the credential-helper protocol, not a URL, so it stays put. finish(`username=x-access-token\npassword=${token}\n\n`, "granted", meta); }) .catch(() => finish(null, "token-error", meta)); diff --git a/apps/api/src/modules/deployments/build-log-sanitize.ts b/apps/api/src/modules/deployments/build-log-sanitize.ts index e75912c8f..ab067ef5b 100644 --- a/apps/api/src/modules/deployments/build-log-sanitize.ts +++ b/apps/api/src/modules/deployments/build-log-sanitize.ts @@ -43,7 +43,11 @@ export const MAX_TOTAL_CHARS = 4_000_000; * Credentials embedded in a URL's userinfo. The shape that matters is the one * `injectGitToken` builds for a private clone: * - * https://x-access-token:@github.com/owner/repo.git + * https://:x-oauth-basic@github.com/owner/repo.git (PAT / OAuth) + * https://x-access-token:@github.com/owner/repo.git (App) + * + * Both are matched by userinfo position, not by which slot holds the secret, so a + * change to the credential pair cannot quietly stop the redaction from firing. * * That URL is interpolated into a shell command whose git output is streamed into * the persisted build log, so without this the token lands in `build_session.logs` diff --git a/apps/api/test/modules/deployments/build-log-sanitize.test.ts b/apps/api/test/modules/deployments/build-log-sanitize.test.ts index 991e55a73..fe3d0941b 100644 --- a/apps/api/test/modules/deployments/build-log-sanitize.test.ts +++ b/apps/api/test/modules/deployments/build-log-sanitize.test.ts @@ -237,9 +237,10 @@ describe("sanitizeLogsForPersistence", () => { }); /** - * The deploy pipeline builds `https://x-access-token:@github.com/o/r.git` - * (injectGitToken, packages/adapters/src/runtime/git-clone.ts:30) and interpolates it - * into a shell command whose git output is persisted. So the credential is one the + * The deploy pipeline builds `https://:x-oauth-basic@github.com/o/r.git` for a + * user token and `https://x-access-token:@…` for an App installation token + * (injectGitToken / gitCredentialPair, packages/adapters/src/runtime/git-clone.ts) and + * interpolates it into a shell command whose git output is persisted. So the credential is one the * pipeline manufactures itself — masking project env upstream does nothing for it, * and `build_session.logs` outlives the ~60-minute token in backups and exports. */ @@ -253,6 +254,15 @@ describe("redactCredentials", () => { expect(out).toContain("https://***@github.com/acme/app.git"); }); + it("redacts the user-token pair — secret in the USERNAME slot", () => { + const out = redactCredentials( + "Cloning into 'app'... https://ghp_0123456789abcdefghij:x-oauth-basic@github.com/acme/app.git", + ); + expect(out).not.toContain("ghp_0123456789abcdefghij"); + expect(out).not.toContain("x-oauth-basic"); + expect(out).toContain("https://***@github.com/acme/app.git"); + }); + it("redacts a token used as the whole userinfo (no colon)", () => { const out = redactCredentials("remote: https://ghp_0123456789abcdefghij@github.com/a/b"); expect(out).not.toContain("ghp_0123456789abcdefghij"); diff --git a/packages/adapters/src/index.ts b/packages/adapters/src/index.ts index c874e850a..1e7c2395b 100644 --- a/packages/adapters/src/index.ts +++ b/packages/adapters/src/index.ts @@ -47,6 +47,7 @@ export { sq, assembleGitClone, injectGitToken, + gitCredentialPair, toGitHubSshUrl, type GitCloneAuth, type GitCloneInvocation, diff --git a/packages/adapters/src/runtime/build-pipeline.ts b/packages/adapters/src/runtime/build-pipeline.ts index dd5a98c54..91b5f2166 100644 --- a/packages/adapters/src/runtime/build-pipeline.ts +++ b/packages/adapters/src/runtime/build-pipeline.ts @@ -17,7 +17,13 @@ import { sq, injectGitToken, assembleGitClone } from "./git-clone"; import { materializeGitSsh, shellGitSshWriter, type GitSshMaterial } from "./git-ssh-material"; // Re-exported for the docker adapters that import these from here. -export { sq, injectGitToken, toGitHubSshUrl, assembleGitClone } from "./git-clone"; +export { + sq, + injectGitToken, + gitCredentialPair, + toGitHubSshUrl, + assembleGitClone, +} from "./git-clone"; // ─── BuildLogger - single source of truth for step + log events ───────────── diff --git a/packages/adapters/src/runtime/git-clone.test.ts b/packages/adapters/src/runtime/git-clone.test.ts index 8c54cc2ed..257dbcf68 100644 --- a/packages/adapters/src/runtime/git-clone.test.ts +++ b/packages/adapters/src/runtime/git-clone.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect } from "vitest"; -import { sq, injectGitToken, toGitHubSshUrl, assembleGitClone } from "./git-clone"; +import { + sq, + injectGitToken, + gitCredentialPair, + toGitHubSshUrl, + assembleGitClone, +} from "./git-clone"; describe("sq (POSIX single-quote)", () => { it("wraps a plain value", () => { @@ -22,17 +28,23 @@ describe("injectGitToken", () => { }); it("rides a classic PAT in the username slot on github.com", () => { expect(injectGitToken("https://github.com/owner/repo.git", "ghp_1234")).toBe( - "https://ghp_1234@github.com/owner/repo.git", + "https://ghp_1234:x-oauth-basic@github.com/owner/repo.git", ); }); it("rides a fine-grained PAT in the username slot on github.com", () => { expect(injectGitToken("https://github.com/owner/repo.git", "github_pat_1234")).toBe( - "https://github_pat_1234@github.com/owner/repo.git", + "https://github_pat_1234:x-oauth-basic@github.com/owner/repo.git", ); }); it("rides an OAuth token in the username slot on github.com", () => { expect(injectGitToken("https://github.com/owner/repo.git", "gho_1234")).toBe( - "https://gho_1234@github.com/owner/repo.git", + "https://gho_1234:x-oauth-basic@github.com/owner/repo.git", + ); + }); + it("treats a legacy prefix-less PAT as a user token", () => { + const legacy = "a".repeat(40); + expect(injectGitToken("https://github.com/owner/repo.git", legacy)).toBe( + `https://${legacy}:x-oauth-basic@github.com/owner/repo.git`, ); }); it("keeps x-access-token on non-GitHub hosts (arbitrary username accepted)", () => { @@ -40,6 +52,21 @@ describe("injectGitToken", () => { "https://x-access-token:glpat-1@gitlab.com/owner/repo.git", ); }); + it("keeps x-access-token on GitHub Enterprise (own domain, not github.com)", () => { + expect(injectGitToken("https://github.acme-corp.com/owner/repo.git", "ghp_1234")).toBe( + "https://x-access-token:ghp_1234@github.acme-corp.com/owner/repo.git", + ); + }); + it("trims a pasted token instead of percent-encoding the whitespace", () => { + expect(injectGitToken("https://github.com/owner/repo.git", " ghp_1234\n")).toBe( + "https://ghp_1234:x-oauth-basic@github.com/owner/repo.git", + ); + }); + it("returns the URL unchanged when the token is only whitespace", () => { + expect(injectGitToken("https://github.com/owner/repo.git", " ")).toBe( + "https://github.com/owner/repo.git", + ); + }); it("returns the URL unchanged when no token", () => { expect(injectGitToken("https://github.com/owner/repo.git")).toBe( "https://github.com/owner/repo.git", @@ -50,6 +77,43 @@ describe("injectGitToken", () => { "git@github.com:owner/repo.git", ); }); + + // A URL missing the password is not a complete credential: git sends an empty + // password, then on the 401 asks GIT_ASKPASS / the credential helper / the tty + // for the real one. Every clone path sets GIT_ASKPASS=/bin/echo, so a path that + // forgets it dies with "unable to get password from user" and never surfaces + // GitHub's reason. Both slots filled = self-contained on success and failure. + it("always emits BOTH Basic-auth slots, whatever the token or host", () => { + const tokens = ["ghs_1", "ghp_1", "github_pat_1", "gho_1", "ghu_1", "a".repeat(40), "glpat-1"]; + const hosts = ["github.com", "github.acme-corp.com", "gitlab.com", "git.example.org"]; + for (const host of hosts) { + for (const token of tokens) { + const out = injectGitToken(`https://${host}/owner/repo.git`, token); + const { username, password } = new URL(out); + expect(username, `${host} / ${token}`).not.toBe(""); + expect(password, `${host} / ${token}`).not.toBe(""); + expect(out).toContain(`@${host}/`); + } + } + }); +}); + +describe("gitCredentialPair", () => { + it("carries the token in exactly one slot and a fixed literal in the other", () => { + const user = gitCredentialPair("github.com", "ghp_1234"); + expect(user).toEqual({ username: "ghp_1234", password: "x-oauth-basic" }); + const app = gitCredentialPair("github.com", "ghs_1234"); + expect(app).toEqual({ username: "x-access-token", password: "ghs_1234" }); + }); + // Suffix-matching the host would pull gist./raw. subdomains into the github.com + // branch. Neither is a clone source, and the general form works on both, so the + // test is an exact host match on purpose. + it("matches github.com exactly — subdomains take the general form", () => { + expect(gitCredentialPair("gist.github.com", "ghp_1234")).toEqual({ + username: "x-access-token", + password: "ghp_1234", + }); + }); }); describe("toGitHubSshUrl", () => { @@ -74,7 +138,7 @@ describe("assembleGitClone — token / public mode", () => { gitToken: "ghp_1234", }); it("injects the token into the clone URL (PAT as username on github.com)", () => { - expect(inv.cloneUrl).toBe("https://ghp_1234@github.com/owner/repo.git"); + expect(inv.cloneUrl).toBe("https://ghp_1234:x-oauth-basic@github.com/owner/repo.git"); }); it("fails fast instead of prompting (no interactive credential path)", () => { expect(inv.gitEnv).toContain("GIT_TERMINAL_PROMPT=0"); diff --git a/packages/adapters/src/runtime/git-clone.ts b/packages/adapters/src/runtime/git-clone.ts index 8e6b4bbac..e8ca541fc 100644 --- a/packages/adapters/src/runtime/git-clone.ts +++ b/packages/adapters/src/runtime/git-clone.ts @@ -23,31 +23,52 @@ export function sq(value: string): string { } /** - * Inject a token into an HTTPS git URL for private repo access. + * The Basic-auth pair for a git HTTPS credential. * - * On GitHub the username is not free-form: `x-access-token` is accepted only - * for GitHub App installation tokens (ghs_…). A classic or fine-grained PAT - * (ghp_…, github_pat_…, gho_…) behind a fixed `x-access-token` username is - * rejected with "Invalid username or token", so those credentials ride in - * the username slot instead: - * https://github.com/owner/repo.git → https://ghp_@github.com/owner/repo.git - * Other hosts keep the previous form (x-access-token:@), which they - * accept with an arbitrary username. - * Unchanged when no token or the URL isn't HTTPS. + * Both slots are ALWAYS filled. `https://@host` is not a complete + * credential to git: it sends an empty password, and on the 401 goes looking for + * the real one — GIT_ASKPASS, then the credential helper, then the tty. Every + * clone path here sets `GIT_ASKPASS=/bin/echo`, so today that detour ends in a + * retry with garbage; a path that forgets to set it fails with git's "unable to + * get password from user" before it ever authenticates, hiding GitHub's actual + * reason. Filling both slots keeps the URL self-contained on success AND failure. + * + * Which slot holds the token does not matter to GitHub — it reads the token from + * either and ignores the other value. So each token type gets the pair GitHub + * itself documents: `x-access-token:` for App installation tokens, + * `:x-oauth-basic` for user tokens (PAT classic, fine-grained, OAuth). + */ +export function gitCredentialPair( + hostname: string, + token: string, +): { username: string; password: string } { + // Exact host, not a suffix match. Every other host — GitHub Enterprise on its + // own domain, GitLab, Gitea — takes the x-access-token form, which works + // wherever an arbitrary username is accepted. Narrowing this test can only + // route a host to the general form, never strand one without a credential. + if (hostname === "github.com" && !token.startsWith("ghs_")) { + return { username: token, password: "x-oauth-basic" }; + } + return { username: "x-access-token", password: token }; +} + +/** + * Inject a token into an HTTPS git URL for private repo access: + * https://github.com/owner/repo.git + * → https://:x-oauth-basic@github.com/owner/repo.git + * Unchanged when there is no token or the URL isn't HTTPS. */ export function injectGitToken(repoUrl: string, token?: string): string { - if (!token) return repoUrl; + // Trim first: a token pasted with surrounding whitespace percent-encodes into + // the URL (%20…%20) and fails auth for a reason no log makes visible. + const secret = token?.trim(); + if (!secret) return repoUrl; try { const url = new URL(repoUrl); if (url.protocol !== "https:") return repoUrl; - const isGitHub = /(^|\.)github\.com$/.test(url.hostname); - if (isGitHub && !token.startsWith("ghs_")) { - url.username = token; - url.password = ""; - } else { - url.username = "x-access-token"; - url.password = token; - } + const { username, password } = gitCredentialPair(url.hostname, secret); + url.username = username; + url.password = password; return url.toString(); } catch { return repoUrl;