From f5e0f3c551dc0df40a0a0ac2a4c32d04abdc95ce Mon Sep 17 00:00:00 2001 From: Chinoman10 <8300763+Chinoman10@users.noreply.github.com> Date: Sun, 18 Oct 2026 21:17:41 +0200 Subject: [PATCH 1/5] fix(source-control): opt in to repository-scoped GitHub credentials Reuse existing global Git HTTPS helpers for github.com repository requests, including multiple organisation-scoped PATs belonging to the same login. Keep existing host authentication precedence unchanged by default. Carry validated repository scope through the shared request policy, typed REST and GraphQL transports, native operations, resolver batches, server caches, and media redirects. Preserve original remote spelling for helper matching while retaining canonical identities, and keep fallback credential-cache provenance separate from operation scope. Add real Git, routing, mutation, media, identity and rotation regressions, plus opt-in documentation. Verified 586 focused tests with one explicit pre-existing Bun VM-GC exclusion, scoped typechecks and lint/format checks, fresh independent review, and eight native checks against a clean-built candidate with case-sensitive helper rules. --- .../src/assets/GitHubMediaFetch.test.ts | 230 +++++++ apps/server/src/assets/GitHubMediaFetch.ts | 70 +- .../pullRequest/PullRequestService.test.ts | 310 +++++++++ .../src/pullRequest/PullRequestService.ts | 341 +++++++--- docs/user/source-control.md | 13 + .../src/server/PullRequestProvider.ts | 4 +- .../src/server/GitHubCredentials.ts | 191 +++++- .../src/server/GitHubGitCredentials.test.ts | 524 +++++++++++++++ .../src/server/GitHubPullRequestApi.ts | 308 +++++++-- .../src/server/GitHubPullRequestProvider.ts | 6 +- .../server/GitHubRepositoryRouting.test.ts | 619 ++++++++++++++++++ .../src/server/GitHubRequestPolicy.ts | 66 +- .../src/server/GitHubRest.ts | 12 +- .../src/server/GitHubSourceControlProvider.ts | 101 ++- .../src/server/gitHubConditionalChecks.ts | 1 + .../src/server/gitHubGraphQlClient.test.ts | 43 +- .../src/server/gitHubGraphQlClient.ts | 4 + .../server/gitHubRepositoryResolution.test.ts | 48 ++ .../src/server/gitHubRepositoryResolution.ts | 45 +- .../src/server/githubStackActions.ts | 4 + .../src/server/githubStackRebase.ts | 2 +- 21 files changed, 2739 insertions(+), 203 deletions(-) create mode 100644 apps/server/src/assets/GitHubMediaFetch.test.ts create mode 100644 packages/source-control-github/src/server/GitHubGitCredentials.test.ts create mode 100644 packages/source-control-github/src/server/GitHubRepositoryRouting.test.ts diff --git a/apps/server/src/assets/GitHubMediaFetch.test.ts b/apps/server/src/assets/GitHubMediaFetch.test.ts new file mode 100644 index 000000000000..c4be249fa476 --- /dev/null +++ b/apps/server/src/assets/GitHubMediaFetch.test.ts @@ -0,0 +1,230 @@ +import { describe, expect, it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Redacted from "effect/Redacted"; +import * as HostProcess from "@t3tools/shared/HostProcess"; +import { HttpClient, HttpClientResponse } from "effect/http"; +import * as GitHubCredentials from "@t3tools/source-control-github/server/GitHubCredentials"; +import { githubMediaResponse } from "./GitHubMediaFetch.ts"; + +const asset = (url: string) => ({ + url, + cwd: "/untrusted-checkout", + expiresAt: Number.MAX_SAFE_INTEGER, +}); + +describe("repository GitHub media credentials", () => { + it.effect.each([true, false])( + "keeps legacy fallback redirect behavior only with opt-in disabled: %s", + (enabled) => { + const authorizations: Array = []; + return Effect.gen(function* () { + yield* githubMediaResponse( + asset("https://raw.githubusercontent.com/OwnerA/Repo/main/image.png"), + {}, + ); + expect(authorizations).toEqual([ + "Bearer fake-fallback", + enabled ? undefined : "Bearer fake-fallback", + ]); + }).pipe( + Effect.provide( + Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: (host, repository) => { + expect(repository).toBe("OwnerA/Repo"); + // A repository helper miss returns the legacy credential without helper provenance. + return Effect.succeed({ + host, + source: "settings", + fingerprint: "default", + token: Redacted.make("fake-fallback"), + }); + }, + }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => { + authorizations.push(request.headers.authorization); + return Effect.succeed( + HttpClientResponse.fromWeb( + request, + new Response( + null, + authorizations.length === 1 + ? { + status: 302, + headers: { + location: "https://raw.githubusercontent.com/OwnerB/Repo/main/image.png", + }, + } + : { status: 200, headers: { "content-type": "image/png" } }, + ), + ), + ); + }), + ), + Effect.provideService(HostProcess.Environment, { + T3CODE_GITHUB_USE_GIT_CREDENTIALS: enabled ? "1" : "0", + }), + Effect.scoped, + ); + }, + ); + + it.effect.each([401, 403])( + "invalidates the host cache after opted-in fallback HTTP %s", + (status) => { + const invalidations: Array = []; + let requests = 0; + return Effect.gen(function* () { + expect( + (yield* githubMediaResponse( + asset("https://raw.githubusercontent.com/OwnerA/Repo/main/image.png"), + {}, + )).status, + ).toBe(status); + expect(invalidations).toEqual([["github.com", undefined]]); + expect(requests).toBe(1); + }).pipe( + Effect.provide( + Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: (host) => + Effect.succeed({ + host, + source: "gh", + fingerprint: "default", + token: Redacted.make("fake-fallback"), + }), + invalidate: (host, repository) => + Effect.sync(() => { + invalidations.push([host, repository]); + }), + }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => { + requests++; + return Effect.succeed( + HttpClientResponse.fromWeb(request, new Response(null, { status })), + ); + }), + ), + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + Effect.scoped, + ); + }, + ); + + it.effect( + "uses raw URL repository context and strips a scoped token across repository redirects", + () => { + const scopes: Array = []; + const authorizations: Array = []; + return Effect.gen(function* () { + const response = yield* githubMediaResponse( + asset("https://raw.githubusercontent.com/OwnerA/Repo/main/image.png"), + {}, + ); + expect(response.status).toBe(200); + expect(scopes).toEqual(["OwnerA/Repo"]); + expect(authorizations).toEqual(["Bearer fake-owner-a", undefined]); + }).pipe( + Effect.provide( + Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: (host, repository) => { + scopes.push(repository); + return Effect.succeed({ + host, + repository: "ownera/repo", + source: "git", + fingerprint: "fake-a", + token: Redacted.make("fake-owner-a"), + }); + }, + }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => { + authorizations.push(request.headers.authorization); + return Effect.succeed( + HttpClientResponse.fromWeb( + request, + new Response( + null, + authorizations.length === 1 + ? { + status: 302, + headers: { + location: "https://raw.githubusercontent.com/OwnerB/Repo/main/image.png", + }, + } + : { status: 200, headers: { "content-type": "image/png" } }, + ), + ), + ); + }), + ), + Effect.scoped, + ); + }, + ); + + it.effect("keeps attachments without repository context on the host default", () => { + const scopes: Array = []; + return Effect.gen(function* () { + yield* githubMediaResponse(asset("https://github.com/user-attachments/assets/fake-id"), {}); + expect(scopes).toEqual([undefined]); + }).pipe( + Effect.provide( + Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: (host, repository) => { + scopes.push(repository); + return Effect.succeed({ + host, + source: "settings", + fingerprint: "default", + token: Redacted.make("fake-default"), + }); + }, + }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.succeed(HttpClientResponse.fromWeb(request, new Response(null, { status: 404 }))), + ), + ), + Effect.scoped, + ); + }); + + it.effect("reports helper failures without a host fallback or HTTP request", () => + Effect.gen(function* () { + const error = yield* githubMediaResponse( + asset("https://raw.githubusercontent.com/OwnerA/Repo/main/image.png"), + {}, + ).pipe(Effect.flip); + expect(error._tag).toBe("GitHubGitCredentialError"); + }).pipe( + Effect.provide( + Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: () => + Effect.fail( + new GitHubCredentials.GitHubGitCredentialError({ + host: "github.com", + reason: "process", + }), + ), + }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make(() => Effect.die("Must not fetch after helper failure")), + ), + Effect.scoped, + ), + ); +}); diff --git a/apps/server/src/assets/GitHubMediaFetch.ts b/apps/server/src/assets/GitHubMediaFetch.ts index 668f91b3d78a..434b173ef27e 100644 --- a/apps/server/src/assets/GitHubMediaFetch.ts +++ b/apps/server/src/assets/GitHubMediaFetch.ts @@ -4,6 +4,7 @@ import * as Clock from "effect/Clock"; import * as Effect from "effect/Effect"; import * as Option from "effect/Option"; import * as Redacted from "effect/Redacted"; +import * as HostProcess from "@t3tools/shared/HostProcess"; import { FetchHttpClient, HttpClient, @@ -50,22 +51,66 @@ const SVG_CONTENT_TYPE = "image/svg+xml"; // body cannot run script against this origin. const SVG_CONTENT_SECURITY_POLICY = "default-src 'none'; style-src 'unsafe-inline'; sandbox"; -const githubToken = GitHubCredentials.GitHubCredentials.pipe( - Effect.flatMap((credentials) => credentials.get("github.com")), - Effect.map((credential) => credential.token), - Effect.orElseSucceed(() => null), -); +const repositoryOf = (source: string) => { + const url = new URL(source); + const parts = url.pathname.split("/"); + const repository = `${parts[1]}/${parts[2]}`; + return isCredentialedHost(source) && + parts.length > 3 && + parts[1] !== "user-attachments" && + GitHubCredentials.normalizeCredentialRepository(repository) !== null + ? repository + : null; +}; +type MediaCredential = GitHubCredentials.GitHubCredential & { + readonly operationRepository?: string; +}; +const githubToken = (source: string) => + !isCredentialedHost(source) + ? Effect.succeed(null) + : GitHubCredentials.GitHubCredentials.pipe( + Effect.flatMap((credentials) => + Effect.gen(function* () { + // Raw/LFS URLs and legacy attachments carry an explicit repository. Modern + // user-attachments URLs do not, so they keep the host's default credential. + const repository = repositoryOf(source); + const credential = yield* credentials.get("github.com", repository ?? undefined); + const enabled = + (yield* HostProcess.Environment).T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1"; + // Redirect scope follows the requested repository even on a helper miss. Keep + // credential.repository as the actual cache provenance for invalidation. + const operationRepository = + enabled && repository !== null + ? GitHubCredentials.normalizeCredentialRepository(repository)! + : credential.repository; + return { + ...credential, + ...(operationRepository === undefined ? {} : { operationRepository }), + } satisfies MediaCredential; + }), + ), + Effect.catchIf( + (error) => error._tag !== "GitHubGitCredentialError", + () => Effect.succeed(null), + ), + ); const fetchFollowingRedirects = Effect.fn("GitHubMediaFetch.fetchFollowingRedirects")(function* ( url: string, headers: Record, - token: Redacted.Redacted | null, + credential: MediaCredential | null, ) { const httpClient = HttpClient.withScope(yield* HttpClient.HttpClient); let target = url; for (let hop = 0; ; hop += 1) { const authorization = - token !== null && isCredentialedHost(target) ? `Bearer ${Redacted.value(token)}` : null; + credential !== null && + isCredentialedHost(target) && + (credential.operationRepository === undefined || + GitHubCredentials.normalizeCredentialRepository(repositoryOf(target) ?? "") === + credential.operationRepository) + ? `Bearer ${Redacted.value(credential.token)}` + : null; const response: HttpClientResponse.HttpClientResponse = yield* httpClient .execute( HttpClientRequest.get(target).pipe( @@ -92,13 +137,20 @@ export const githubMediaResponse = Effect.fn("GitHubMediaFetch.githubMediaRespon requestHeaders: Record, ) { // Both media hosts are served by github.com's account, which is the host `gh` stores it under. - const token = yield* githubToken; + const credential = yield* githubToken(asset.url); const forwarded: Record = {}; for (const name of FORWARDED_REQUEST_HEADERS) { const value = requestHeaders[name]; if (value !== undefined) forwarded[name] = value; } - const response = yield* fetchFollowingRedirects(asset.url, forwarded, token); + const response = yield* fetchFollowingRedirects(asset.url, forwarded, credential); + if ( + credential?.operationRepository !== undefined && + (response?.status === 401 || response?.status === 403) + ) { + const credentials = yield* GitHubCredentials.GitHubCredentials; + yield* credentials.invalidate("github.com", credential.repository); + } const remainingSeconds = Math.floor((asset.expiresAt - (yield* Clock.currentTimeMillis)) / 1000); const headers: Record = { "cache-control": diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 21d947dcced8..e4caab438466 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1,5 +1,6 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; import * as NodeServices from "@effect/platform-node/NodeServices"; +import * as NodeCrypto from "@effect/platform-node/NodeCrypto"; import * as NodeV8 from "node:v8"; import * as NodeVM from "node:vm"; import * as ChildProcessSpawner from "effect/process/ChildProcessSpawner"; @@ -7,6 +8,7 @@ import * as KeyValueStore from "effect/persistence/KeyValueStore"; import { assert, it } from "@effect/vitest"; import * as Cause from "effect/Cause"; import * as Clock from "effect/Clock"; +import * as Context from "effect/Context"; import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; import * as Exit from "effect/Exit"; @@ -37,6 +39,8 @@ import * as SourceControlRateLimit from "@t3tools/source-control-core/server/Sou import * as ForgejoCli from "@t3tools/source-control-forgejo/server/ForgejoCli"; import * as ForgejoPullRequestProvider from "@t3tools/source-control-forgejo/server/ForgejoPullRequestProvider"; import * as GitHubRest from "@t3tools/source-control-github/server/GitHubRest"; +import * as GitHubRequestPolicy from "@t3tools/source-control-github/server/GitHubRequestPolicy"; +import * as HostProcess from "@t3tools/shared/HostProcess"; import * as GitHubCredentials from "@t3tools/source-control-github/server/GitHubCredentials"; import * as GitHubPullRequestApi from "@t3tools/source-control-github/server/GitHubPullRequestApi"; import * as GitHubPullRequestProvider from "@t3tools/source-control-github/server/GitHubPullRequestProvider"; @@ -1406,6 +1410,312 @@ it.effect("tries another workspace on the same host for the viewer", () => }), ); +it.effect( + "uses original remote spelling for canonical GitHub identities across native credential paths", + () => + Effect.gen(function* () { + let generation = 1; + const helperPaths: string[] = []; + const lookups: string[] = []; + const requests: Array<{ operation: string; authorization: string | undefined }> = []; + const host = TestSourceControlHost.layer({ + process: { + run: (input) => + Effect.sync(() => { + const path = /path=(.*)\n/.exec(input.stdin ?? "")?.[1]; + assert.ok( + path === "OwnerA/Repo.git" || path === "ownerb/repo.git", + `Unexpected helper selector: ${path}`, + ); + helperPaths.push(path); + return { + exitCode: ChildProcessSpawner.ExitCode(0), + stdout: `protocol=https\nhost=github.com\nusername=Chinoman10\npassword=fake-${path.split("/")[0]}-${generation}\n`, + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + }; + }), + }, + }); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.sync(() => { + const operation = request.url.endsWith("/user") + ? "user" + : request.body._tag === "Uint8Array" + ? new TextDecoder().decode(request.body.body) + : ""; + requests.push({ operation, authorization: request.headers.authorization }); + return HttpClientResponse.fromWeb( + request, + new Response( + JSON.stringify( + operation === "user" + ? { id: 101, login: "Chinoman10" } + : { + data: { + repository: { pullRequest: { id: "PR_1" } }, + _0_markFileAsViewed: { clientMutationId: null }, + }, + }, + ), + { headers: { "content-type": "application/json" } }, + ), + ); + }), + ), + ); + const context = yield* Layer.build( + GitHubPullRequestApi.layer.pipe( + Layer.provideMerge( + GitHubRest.layerWithDependencies.pipe( + Layer.provideMerge(host), + Layer.provideMerge(http), + Layer.provide(NodeCrypto.layer), + ), + ), + Layer.provideMerge(NodeServices.layer), + ), + ); + const native = yield* GitHubPullRequestProvider.make.pipe(Effect.provideContext(context)); + const policy = Context.get(context, GitHubRequestPolicy.GitHubRequestPolicy); + const credentials = Context.get(context, GitHubCredentials.GitHubCredentials); + const lookup = (input: { readonly host: string; readonly repository: string }) => { + lookups.push(input.repository); + return policy + .credential(input.host, input.repository) + .pipe(Effect.mapError(() => requestFailed)); + }; + const provider = fakeProvider(SourceControlProviderKind.make("github"), { + capabilities: { + ...fakeProvider(SourceControlProviderKind.make("github")).capabilities, + viewedFiles: "host", + }, + withVerifiedCredential: native.withVerifiedCredential!, + listChangeRequests: (input) => + lookup(input).pipe( + Effect.as({ + items: [changeRequest(1, "2026-07-02T00:00:00Z")], + truncated: false, + continues: true, + }), + ), + getChangeRequest: (input) => + lookup(input).pipe( + Effect.map((credential) => hostedChangeRequest(Redacted.value(credential.token))), + ), + listChangeRequestStats: (input) => + Effect.forEach(input.changeRequests, (ref) => + lookup({ host: input.host, repository: ref.repository }).pipe( + Effect.as({ ...ref, additions: 1, deletions: 0 }), + ), + ), + getViewerPermissions: (input) => + lookup(input).pipe(Effect.as(hostedChangeRequest("").viewerPermissions)), + comment: (input) => lookup(input).pipe(Effect.asVoid), + listReviewerCandidates: (input) => + lookup(input).pipe(Effect.as({ candidates: [], truncated: false })), + getReviewThreadComments: (input) => + lookup(input).pipe(Effect.as({ comments: [], nextCursor: null })), + setFilesViewed: native.setFilesViewed!, + }); + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "A", + workspaceRoot: "/a", + repository: "ownera/repo", + remoteUrl: "https://github.com/OwnerA/Repo.git", + }), + ], + providers: [provider], + }); + const ref = { projectId: "p1" as ProjectId, repository: "ownera/repo", number: 1 }; + const listed = yield* service.list({ state: "open" }); + assert.deepStrictEqual( + listed.entries.map((entry) => entry.repository), + ["ownera/repo"], + ); + assert.strictEqual((yield* service.detail(ref)).body, "fake-OwnerA-1"); + yield* credentials.invalidate("github.com", "ownera/repo"); + generation = 2; + assert.strictEqual((yield* service.detail(ref)).body, "fake-OwnerA-2"); + assert.strictEqual( + (yield* service.listStats({ refs: [ref] })).stats[0]?.repository, + "ownera/repo", + ); + yield* service.comment({ ...ref, body: "Fake comment" }); + yield* service.reviewerCandidates(ref); + yield* service.threadComments({ ...ref, threadId: "fake-thread", cursor: "fake-cursor" }); + assert.ok(lookups.length >= 7); + assert.ok(lookups.every((repository) => repository === "OwnerA/Repo")); + yield* credentials.invalidate("github.com", "ownera/repo"); + generation = 3; + yield* service.setFilesViewed({ ...ref, files: [{ path: "file.ts", viewed: true }] }); + assert.ok( + requests.some( + (request) => + request.operation.includes("markFileAsViewed") && + request.authorization === "Bearer fake-OwnerA-3", + ), + ); + const cross = yield* service.detail({ + ...ref, + repository: "ownerb/repo", + host: "github.com", + }); + assert.strictEqual(cross.repository, "ownerb/repo"); + assert.strictEqual(cross.body, "fake-ownerb-3"); + assert.strictEqual(lookups.at(-1), "ownerb/repo"); + assert.deepStrictEqual(helperPaths, [ + "OwnerA/Repo.git", + "OwnerA/Repo.git", + "OwnerA/Repo.git", + "ownerb/repo.git", + ]); + }).pipe( + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + ), +); + +it.effect.each([ + { remoteUrl: "https://github.com/OwnerA/Repo.git", enabled: false }, + { remoteUrl: "https://evil.example/OwnerA/Repo.git", enabled: true }, + { remoteUrl: "https://github.com/Other/Repo.git", enabled: true }, +])( + "keeps canonical selectors when remote spelling is inapplicable: $remoteUrl, opt-in $enabled", + ({ remoteUrl, enabled }) => + Effect.gen(function* () { + const asked: string[] = []; + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "A", + workspaceRoot: "/a", + repository: "ownera/repo", + remoteUrl, + }), + ], + providers: [ + fakeProvider(SourceControlProviderKind.make("github"), { + withVerifiedCredential: (input, use) => { + asked.push(input.repository!); + return use({ accountId: "101", viewer: "Chinoman10", credentialFingerprint: "fake" }); + }, + getChangeRequest: (input) => { + asked.push(input.repository); + return Effect.succeed(hostedChangeRequest("fake body")); + }, + }), + ], + }); + yield* service.detail({ projectId: "p1" as ProjectId, repository: "ownera/repo", number: 1 }); + assert.ok(asked.every((repository) => repository === "ownera/repo")); + }).pipe( + Effect.provideService(HostProcess.Environment, { + T3CODE_GITHUB_USE_GIT_CREDENTIALS: enabled ? "1" : "0", + }), + ), +); + +it.effect("keeps opted-in repository viewers and list searches separate for one login", () => + Effect.gen(function* () { + const listed: string[] = []; + let blocked = false; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "A", workspaceRoot: "/a", repository: "OwnerA/Repo" }), + project({ id: "p2", title: "B", workspaceRoot: "/b", repository: "OwnerB/Repo" }), + ], + providers: [ + fakeProvider(SourceControlProviderKind.make("github"), { + getViewer: () => Effect.die("A repository listing must not use the host-wide viewer"), + withVerifiedCredential: (input, use) => + input.repository === "OwnerA/Repo" && blocked + ? Effect.fail(requestFailed) + : use({ + accountId: "101", + viewer: "Chinoman10", + credentialFingerprint: `pat:${input.repository}`, + }), + listChangeRequestsAcross: () => Effect.die("Cannot mix owner credentials in one search"), + listChangeRequests: (input) => + Effect.sync(() => { + listed.push(input.repository); + assert.strictEqual(input.viewer, "Chinoman10"); + return { + items: [changeRequest(1, "2026-07-02T00:00:00Z")], + truncated: false, + continues: true, + }; + }), + }), + ], + }); + assert.strictEqual((yield* service.list({ state: "open" })).entries.length, 2); + assert.deepStrictEqual(listed.sort(), ["OwnerA/Repo", "OwnerB/Repo"]); + blocked = true; + const next = yield* service.list({ state: "open" }); + assert.deepStrictEqual( + next.entries.map((entry) => entry.repository), + ["OwnerB/Repo"], + ); + assert.strictEqual(next.errors.length, 1); + }).pipe( + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + ), +); + +it.effect("namespaces ordinary detail caches by repository PAT without a routing RPC", () => + Effect.gen(function* () { + for (const operation of ["summary", "detail"] as const) { + let credential = "broad"; + let reads = 0; + const read = () => + Effect.suspend(() => { + reads++; + return credential === "broad" + ? Effect.succeed(hostedChangeRequest("private content")) + : Effect.fail(requestFailed); + }); + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "A", workspaceRoot: "/a", repository: "OwnerA/Repo" }), + ], + providers: [ + fakeProvider(SourceControlProviderKind.make("github"), { + withVerifiedCredential: (input, use) => { + assert.strictEqual(input.repository, "OwnerA/Repo"); + return use({ + accountId: "101", + viewer: "Chinoman10", + credentialFingerprint: credential, + }); + }, + getChangeRequest: read, + getChangeRequestSummary: read, + }), + ], + }); + const ref = { projectId: "p1" as ProjectId, repository: "OwnerA/Repo", number: 1 }; + yield* service[operation](ref); + credential = "restricted"; + for (const allowStale of [false, true]) + assert.strictEqual( + (yield* service[operation]({ ...ref, allowStale }).pipe(Effect.flip))._tag, + "PullRequestOperationError", + ); + assert.strictEqual(reads, 3); + } + }).pipe( + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + ), +); + it.effect("routing verifies the current account on the requested host without caching it", () => Effect.gen(function* () { let viewer = "first-account"; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 328e32526c1e..d979cb27bea7 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -6,6 +6,11 @@ import { import { normalizeGitRemoteUrl } from "@t3tools/shared/git"; import { parseDiffFilePaths } from "@t3tools/source-control-core/server/diffFileRevisions"; import * as Cache from "effect/Cache"; +import * as HostProcess from "@t3tools/shared/HostProcess"; +import { + gitHubApiHostForRemote, + parseGitHubRemoteRepositorySelector, +} from "@t3tools/source-control-github/server/gitHubRepositoryResolution"; import * as Cause from "effect/Cause"; import * as Clock from "effect/Clock"; import * as Context from "effect/Context"; @@ -585,6 +590,43 @@ export const make = Effect.gen(function* () { const rateLimits = yield* SourceControlRateLimit.SourceControlRateLimit; const filesViewedStore = yield* PullRequestFilesViewed.PullRequestFilesViewedRepository; const readCache = yield* PullRequestReadCache.PullRequestReadCache; + const useGitCredentials = + (yield* HostProcess.Environment).T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1"; + const hasRepositoryCredential = (project: SupportedProject) => + useGitCredentials && + project.api.kind === "github" && + project.host === "github.com" && + project.api.withVerifiedCredential !== undefined; + const providerRepository = (project: SupportedProject) => { + if (!useGitCredentials || project.api.kind !== "github" || project.host !== "github.com") + return project.repository; + const remote = project.project.repositoryIdentity?.locator.remoteUrl; + if (remote === undefined || gitHubApiHostForRemote(remote) !== project.host) + return project.repository; + const parsed = parseGitHubRemoteRepositorySelector(remote, project.host); + const selector = parsed === null ? null : `${parsed.owner}/${parsed.name}`; + // GitHub identity is case-insensitive; URL-scoped Git helpers are not. A host fallback + // may serve another repository, so only its matching remote can supply original casing. + return parsed?.host === project.host && + selector !== null && + selector.toLowerCase() === project.repository.toLowerCase() + ? selector + : project.repository; + }; + const withProjectCredential = ( + project: SupportedProject, + read: Effect.Effect, + ) => { + if (!hasRepositoryCredential(project)) return read; + return project.api.withVerifiedCredential!( + { + cwd: project.project.workspaceRoot, + host: project.host, + repository: providerRepository(project), + }, + (identity) => read.pipe(Effect.provideService(routingCredential, identity), Effect.result), + ).pipe(Effect.mapError(toPullRequestError("routeIdentity")), Effect.flatMap(Effect.fromResult)); + }; const refineUnknownProjectKinds = ( projects: ReadonlyArray, @@ -878,7 +920,7 @@ export const make = Effect.gen(function* () { project.api .getViewerPermissions({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: ref.number, includeUpdateBranch, @@ -910,6 +952,8 @@ export const make = Effect.gen(function* () { readonly kind: SourceControlProviderKind; readonly viewer: string | null; readonly error: PullRequestProviderError | null; + readonly repository?: string; + readonly credentialFingerprint?: string; }; const viewersByHost = new Map(); const viewerFlights = yield* Cache.makeWith( @@ -953,7 +997,7 @@ export const make = Effect.gen(function* () { }, ); - const resolveViewers = ( + const resolveHostViewers = ( projects: ReadonlyArray, viewerRoots: WorkspaceProjects["viewerRoots"], options?: { readonly allowPaused: boolean }, @@ -1000,6 +1044,57 @@ export const make = Effect.gen(function* () { { concurrency: REPOSITORY_CONCURRENCY }, ); + const resolveViewers = ( + projects: ReadonlyArray, + viewerRoots: WorkspaceProjects["viewerRoots"], + options?: { readonly allowPaused: boolean }, + ): Effect.Effect> => + Effect.gen(function* () { + const ordinary = yield* resolveHostViewers( + projects.filter((project) => !hasRepositoryCredential(project)), + viewerRoots, + options, + ); + const scoped = yield* Effect.forEach( + projects.filter(hasRepositoryCredential), + (project) => + project.api.withVerifiedCredential!( + { + cwd: project.project.workspaceRoot, + host: project.host, + repository: providerRepository(project), + }, + (identity) => + Effect.succeed({ + host: project.host, + kind: project.api.kind, + repository: project.repository, + credentialFingerprint: identity.credentialFingerprint, + viewer: identity.viewer, + error: null, + }), + ).pipe( + Effect.catch((error) => + Effect.succeed({ + host: project.host, + kind: project.api.kind, + repository: project.repository, + viewer: null, + error, + }), + ), + ), + { concurrency: REPOSITORY_CONCURRENCY }, + ); + return [...ordinary, ...scoped]; + }); + const projectViewer = (project: SupportedProject, viewers: ReadonlyArray) => + viewers.find( + (viewer) => + viewer.host === project.host && + (viewer.repository === undefined || viewer.repository === project.repository), + ); + /** * `checks` is absent because no listed row carries its check state: that one filter is * the host's alone, and a row nobody narrowed stays rather than being guessed at. @@ -1096,19 +1191,28 @@ export const make = Effect.gen(function* () { } const providers: ReadonlyArray = [ - ...viewerResults.map((result) => { - const capabilities = projects.find((project) => project.host === result.host)?.api - .capabilities; - return { - host: result.host, - kind: result.kind, - searchesOnHost: capabilities?.search ?? false, - actions: capabilities?.actions ?? [], - projectCount: projectCounts.get(result.host) ?? 1, - configured: result.viewer !== null, - detail: result.error === null ? null : providerDetail(result.error), - }; - }), + ...viewerResults + .filter( + (result, index) => + viewerResults.findIndex((other) => other.host === result.host) === index, + ) + .map((firstResult) => { + const result = + viewerResults.find( + (candidate) => candidate.host === firstResult.host && candidate.viewer !== null, + ) ?? firstResult; + const capabilities = projects.find((project) => project.host === result.host)?.api + .capabilities; + return { + host: result.host, + kind: result.kind, + searchesOnHost: capabilities?.search ?? false, + actions: capabilities?.actions ?? [], + projectCount: projectCounts.get(result.host) ?? 1, + configured: result.viewer !== null, + detail: result.error === null ? null : providerDetail(result.error), + }; + }), ...[...unimplemented].map(([host, { kind, projectCount }]) => ({ host, kind, @@ -1124,9 +1228,11 @@ export const make = Effect.gen(function* () { continuation === null ? projects : projects.filter(({ cursorKey }) => continuation.has(cursorKey)); - const readable = selected.filter(({ host }) => viewers[host] !== undefined); + const readable = selected.filter( + (project) => projectViewer(project, viewerResults)?.viewer != null, + ); const unreadable = selected - .filter(({ host }) => viewers[host] === undefined) + .filter((project) => projectViewer(project, viewerResults)?.viewer == null) .map(({ project, repository }) => ({ projectId: project.id, projectTitle: project.title, @@ -1159,13 +1265,14 @@ export const make = Effect.gen(function* () { const readRepository = (project: SupportedProject): Effect.Effect => { { - const viewer = viewers[project.host]!; + const viewer = projectViewer(project, viewerResults)!.viewer!; const key = project.cursorKey; const cursor = cursorOf(project); - return project.api - .listChangeRequests({ + return withProjectCredential( + project, + project.api.listChangeRequests({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, state: input.state, involvement, @@ -1178,48 +1285,48 @@ export const make = Effect.gen(function* () { : { cursor: { updatedBefore: cursor.updatedBefore, delivered: cursor.delivered }, }), - }) - .pipe( - observeRead, - Effect.map(({ value: page, observedAt }): RepositoryBatch => { - // The boundary instant was asked for inclusively, so the rows already sent at it - // come back with the slice. Dropping them here rather than asking for strictly - // older is what keeps their neighbours at the same instant from being skipped. - const items = - cursor === undefined - ? page.items - : page.items.filter( - (item) => - item.updatedAt !== cursor.updatedBefore || - !cursor.seenAt.includes(item.number), - ); - return { - key, - entries: items - .filter((item) => matchesRowFilters(item, input.filters, viewer)) - .map((item) => toEntry({ project, item, viewer, observedAt })), - errors: [], - truncated: page.truncated, - nextCursor: - page.continues && page.truncated - ? nextListCursor(cursor, page.items, items, page.cursorAdvance) - : null, - }; - }), - Effect.orElseSucceed((): RepositoryBatch => ({ + }), + ).pipe( + observeRead, + Effect.map(({ value: page, observedAt }): RepositoryBatch => { + // The boundary instant was asked for inclusively, so the rows already sent at it + // come back with the slice. Dropping them here rather than asking for strictly + // older is what keeps their neighbours at the same instant from being skipped. + const items = + cursor === undefined + ? page.items + : page.items.filter( + (item) => + item.updatedAt !== cursor.updatedBefore || + !cursor.seenAt.includes(item.number), + ); + return { key, - entries: [], - errors: [ - { - projectId: project.project.id, - projectTitle: project.project.title, - message: `${project.repository} could not be read.`, - }, - ], - truncated: false, - nextCursor: null, - })), - ); + entries: items + .filter((item) => matchesRowFilters(item, input.filters, viewer)) + .map((item) => toEntry({ project, item, viewer, observedAt })), + errors: [], + truncated: page.truncated, + nextCursor: + page.continues && page.truncated + ? nextListCursor(cursor, page.items, items, page.cursorAdvance) + : null, + }; + }), + Effect.orElseSucceed((): RepositoryBatch => ({ + key, + entries: [], + errors: [ + { + projectId: project.project.id, + projectTitle: project.project.title, + message: `${project.repository} could not be read.`, + }, + ], + truncated: false, + nextCursor: null, + })), + ); } }; @@ -1231,12 +1338,12 @@ export const make = Effect.gen(function* () { const separately = () => Effect.forEach(chunk, readRepository, { concurrency: REPOSITORY_CONCURRENCY }); if (readAcross === undefined) return separately(); - const viewer = viewers[first.host]!; + const viewer = projectViewer(first, viewerResults)!.viewer!; const cursor = cursorOf(first); return readAcross({ cwd: first.project.workspaceRoot, host: first.host, - repositories: chunk.map((project) => project.repository), + repositories: chunk.map(providerRepository), state: input.state, involvement, viewer, @@ -1324,7 +1431,10 @@ export const make = Effect.gen(function* () { const together = new Map>(); const separate: Array = []; for (const project of readable) { - if (project.api.listChangeRequestsAcross === undefined) { + if ( + project.api.listChangeRequestsAcross === undefined || + hasRepositoryCredential(project) + ) { separate.push(project); continue; } @@ -1413,7 +1523,11 @@ export const make = Effect.gen(function* () { } const result = yield* api .withVerifiedCredential( - { cwd: project.project.workspaceRoot, host: project.host }, + { + cwd: project.project.workspaceRoot, + host: project.host, + ...(useGitCredentials ? { repository: providerRepository(project) } : {}), + }, (identity) => identity.accountId === input.expectedAccountId ? operation.pipe(Effect.provideService(routingCredential, identity), Effect.result) @@ -1433,6 +1547,7 @@ export const make = Effect.gen(function* () { .getRoutingIdentity({ cwd: project.project.workspaceRoot, host: project.host, + ...(useGitCredentials ? { repository: providerRepository(project) } : {}), }) .pipe(Effect.mapError(toPullRequestError("routeIdentity"))); if (!identity.viewer.trim() || !identity.accountId.trim()) { @@ -1455,7 +1570,7 @@ export const make = Effect.gen(function* () { Effect.flatMap((project) => { const providerInput = { cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, }; @@ -1513,7 +1628,7 @@ export const make = Effect.gen(function* () { if (read === undefined) return Effect.succeed(null); return read({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, includeDetails: options?.includeDetails !== false, @@ -1547,7 +1662,7 @@ export const make = Effect.gen(function* () { project.api .getChangeRequest({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, }) @@ -1625,7 +1740,7 @@ export const make = Effect.gen(function* () { Effect.flatMap((project) => (project.api.getChangeRequestPreview ?? project.api.getChangeRequest)({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, }).pipe( @@ -1647,7 +1762,7 @@ export const make = Effect.gen(function* () { project.api .getChangeRequestActivity({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, }) @@ -1685,7 +1800,7 @@ export const make = Effect.gen(function* () { } return read({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, threadId: input.threadId, @@ -1702,7 +1817,7 @@ export const make = Effect.gen(function* () { ? project.api .getDiff({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, ...(input.cursor === undefined ? {} : { cursor: input.cursor }), @@ -1725,7 +1840,7 @@ export const make = Effect.gen(function* () { return project.api.capabilities.diff && read ? read({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, ...(input.commit === undefined ? {} : { commit: input.commit }), @@ -1767,7 +1882,14 @@ export const make = Effect.gen(function* () { const setFilesViewed: PullRequestService["Service"]["setFilesViewed"] = (input) => canonicalRef(input).pipe( Effect.flatMap((ref) => - viewedFiles.setFilesViewed(input).pipe( + (useGitCredentials + ? requireProject(ref).pipe( + Effect.flatMap((project) => + withProjectCredential(project, viewedFiles.setFilesViewed(input)), + ), + ) + : viewedFiles.setFilesViewed(input) + ).pipe( // Deliberately not `invalidatedByMutation`: ticking a file off says nothing about the // change request, and dropping a 300-file diff on every checkbox is the whole cost of // the feature. @@ -1888,7 +2010,7 @@ export const make = Effect.gen(function* () { project.api .runAction({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, action: input.action, @@ -1920,7 +2042,11 @@ export const make = Effect.gen(function* () { return input.action === "delete-branch" && project.api.withVerifiedCredential !== undefined ? project.api .withVerifiedCredential( - { cwd: project.project.workspaceRoot, host: project.host }, + { + cwd: project.project.workspaceRoot, + host: project.host, + ...(useGitCredentials ? { repository: providerRepository(project) } : {}), + }, () => operation.pipe(Effect.result), ) .pipe( @@ -1964,7 +2090,7 @@ export const make = Effect.gen(function* () { return project.api .comment({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, body: input.body, @@ -2003,7 +2129,7 @@ export const make = Effect.gen(function* () { } return rewrite({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, ...(input.title === undefined ? {} : { title: input.title }), @@ -2034,7 +2160,7 @@ export const make = Effect.gen(function* () { } return rewrite({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, commentId: input.commentId, @@ -2080,7 +2206,7 @@ export const make = Effect.gen(function* () { return project.api .submitReview({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, verdict: input.verdict, @@ -2126,7 +2252,7 @@ export const make = Effect.gen(function* () { return project.api .replyToThread({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, threadId: input.threadId, @@ -2163,7 +2289,7 @@ export const make = Effect.gen(function* () { return project.api .setThreadResolution({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, threadId: input.threadId, @@ -2193,7 +2319,7 @@ export const make = Effect.gen(function* () { return project.api .setReaction({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, ...(input.subjectId === undefined ? {} : { subjectId: input.subjectId }), @@ -2223,7 +2349,7 @@ export const make = Effect.gen(function* () { ? project.api .listReviewerCandidates({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, }) @@ -2264,7 +2390,7 @@ export const make = Effect.gen(function* () { return project.api .setReviewerRequest({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, reviewers: input.reviewers, @@ -2300,7 +2426,7 @@ export const make = Effect.gen(function* () { ) : list({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, }).pipe(Effect.mapError(toPullRequestError("labelCandidates"))), @@ -2332,7 +2458,7 @@ export const make = Effect.gen(function* () { ) : change({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, labels: input.labels, @@ -2369,8 +2495,11 @@ export const make = Effect.gen(function* () { } const byHost = new Map>(); for (const entry of wanted.values()) { - const held = byHost.get(entry.project.host); - if (held === undefined) byHost.set(entry.project.host, [entry]); + const key = hasRepositoryCredential(entry.project) + ? `${entry.project.host}\n${entry.project.repository}` + : entry.project.host; + const held = byHost.get(key); + if (held === undefined) byHost.set(key, [entry]); else held.push(entry); } const stats = yield* Effect.forEach( @@ -2390,7 +2519,7 @@ export const make = Effect.gen(function* () { cwd: first.project.project.workspaceRoot, host: first.project.host, changeRequests: entries.map((entry) => ({ - repository: entry.project.repository, + repository: providerRepository(entry.project), number: entry.number, })), }).pipe( @@ -2735,6 +2864,9 @@ export const make = Effect.gen(function* () { }, ); const list: PullRequestService["Service"]["list"] = (input) => { + // ponytail: aggregate keys are host-wide; restore caching with credential-set keys if + // helper-mode workspaces need it. Per-PR and verified-identity caches stay credential-scoped. + if (useGitCredentials) return listUncached(input); const key = JSON.stringify([ listingsEpoch, input.state, @@ -2776,7 +2908,7 @@ export const make = Effect.gen(function* () { : project.api .getChangeRequestChecks({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, }) @@ -2800,7 +2932,7 @@ export const make = Effect.gen(function* () { : project.api .getChangeRequestWatchFingerprint({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: input.number, }) @@ -3103,6 +3235,7 @@ export const make = Effect.gen(function* () { const listStats: PullRequestService["Service"]["listStats"] = Effect.fn( "PullRequestService.listStats", )(function* (input: PullRequestListStatsInput) { + if (useGitCredentials) return yield* listStatsUncached(input); if (input.refs.length === 0) return { stats: [] }; const now = yield* Clock.currentTimeMillis; const held: PullRequestDiffStat[] = []; @@ -3240,13 +3373,21 @@ export const make = Effect.gen(function* () { (input: I, ...args: Args) => Effect.gen(function* () { const ref = yield* canonicalRef(input); - const credential = yield* routingCredential; - return yield* read( - credential === null - ? ref - : { ...ref, [credentialNamespace]: credential.credentialFingerprint }, - ...args, - ); + const run = Effect.gen(function* () { + const credential = yield* routingCredential; + return yield* read( + credential === null + ? ref + : { + ...ref, + [credentialNamespace]: credential.credentialFingerprint, + }, + ...args, + ); + }); + if (!useGitCredentials) return yield* run; + const project = yield* requireProject(ref); + return yield* withProjectCredential(project, run); }); /** diff --git a/docs/user/source-control.md b/docs/user/source-control.md index 4a222b9102f2..8a4c4449551e 100644 --- a/docs/user/source-control.md +++ b/docs/user/source-control.md @@ -34,6 +34,19 @@ over that choice; a host turned off stays off either way. For GitHub Enterprise, sign in with `gh auth login --hostname YOUR_HOST`. T3 Code treats a custom server name as GitHub once it has a credential for that host. +If you use different GitHub tokens for different organizations, set +`T3CODE_GITHUB_USE_GIT_CREDENTIALS=1` in the **server's** environment and restart it. For +repository operations on github.com, T3 Code then asks the server's trusted global Git HTTPS +credential helpers for that repository's credential before using the token above. Configure +Git's URL/path-specific helpers on that machine so `git credential fill` selects the intended +token for `https://github.com/OWNER/REPOSITORY.git`; T3 Code honors their existing path settings. +There is no additional token to enter in T3 Code. If Git has no credential, the normal token +precedence still applies; helper failures are reported and a refused token is never retried +with a different one. Host-only account discovery and GitHub Enterprise keep the normal +credential, and a host turned off in Settings stays off. +With only repository credentials configured, account discovery shows that access will be resolved +when you enter a repository; host-wide operations still need a default credential. + ### Forgejo and Gitea Install [Forgejo CLI (`fj`)](https://codeberg.org/forgejo-contrib/forgejo-cli) or diff --git a/packages/source-control-core/src/server/PullRequestProvider.ts b/packages/source-control-core/src/server/PullRequestProvider.ts index 882bc73c1153..39a7d2bcae22 100644 --- a/packages/source-control-core/src/server/PullRequestProvider.ts +++ b/packages/source-control-core/src/server/PullRequestProvider.ts @@ -244,7 +244,7 @@ export interface ProviderRepositoryRef { export interface PullRequestProviderApi { readonly withVerifiedCredential?: ( - input: { readonly cwd: string; readonly host: string }, + input: { readonly cwd: string; readonly host: string; readonly repository?: string }, use: (identity: { readonly accountId: string; readonly viewer: string; @@ -254,6 +254,7 @@ export interface PullRequestProviderApi { readonly getRoutingIdentity?: (input: { readonly cwd: string; readonly host: string; + readonly repository?: string; }) => Effect.Effect< { readonly accountId: string; readonly viewer: string }, PullRequestProviderError @@ -273,6 +274,7 @@ export interface PullRequestProviderApi { readonly getViewer: (input: { readonly cwd: string; readonly host?: string; + readonly repository?: string; }) => Effect.Effect; readonly listChangeRequests: ( diff --git a/packages/source-control-github/src/server/GitHubCredentials.ts b/packages/source-control-github/src/server/GitHubCredentials.ts index b2d0b7d0ea34..48873314b3f5 100644 --- a/packages/source-control-github/src/server/GitHubCredentials.ts +++ b/packages/source-control-github/src/server/GitHubCredentials.ts @@ -19,19 +19,47 @@ import type { ServerSettings } from "@t3tools/contracts"; import * as GitHubClient from "../client/definition.ts"; +/** How long a token is reused before `gh` is asked again, so a `gh auth switch` applies soon. */ const TOKEN_TTL = Duration.minutes(5); +/** No credential is retried sooner, so a fresh `gh auth login` takes effect on the next read. */ const MISSING_TTL = Duration.seconds(10); -export const GitHubCredentialSource = Schema.Literals(["settings", "env", "gh"]); +export const GitHubCredentialSource = Schema.Literals(["settings", "env", "gh", "git"]); export type GitHubCredentialSource = typeof GitHubCredentialSource.Type; export interface GitHubCredential { readonly host: string; readonly token: Redacted.Redacted; readonly source: GitHubCredentialSource; + /** A digest of host and token: safe for cache keys and rate-limit scopes, never the token. */ readonly fingerprint: string; + /** Git helpers select a credential for this repository, never for the host generally. */ + readonly repository?: string; } +export class GitHubGitCredentialError extends Schema.TaggedError()( + "GitHubGitCredentialError", + { host: Schema.String, reason: Schema.Literals(["repository", "process", "response"]) }, +) { + override get message(): string { + return "Git could not safely resolve a repository credential. Check the server's global Git credential helpers."; + } +} + +const Repository = Schema.String.check( + Schema.isPattern(/^[a-z0-9](?:[a-z0-9-]{0,38})\/[a-z0-9_.-]{1,100}$/i), +); +const isRepository = Schema.is(Repository); +/** Canonical HTTPS credential path; never let a selector add credential protocol fields. */ +export function normalizeCredentialRepository(repository: string): string | null { + const normalized = repository.replace(/\.git$/i, "").toLowerCase(); + return isRepository(normalized) && + !normalized.split("/").some((part) => part === "." || part === ".." || part.startsWith("-")) + ? normalized + : null; +} + +/** Nothing in the environment, and no `gh` on PATH to ask. */ export class GitHubCliMissingError extends Schema.TaggedError()( "GitHubCliMissingError", { host: Schema.String }, @@ -41,6 +69,7 @@ export class GitHubCliMissingError extends Schema.TaggedError()( "GitHubNotSignedInError", { host: Schema.String, account: Schema.optional(Schema.String) }, @@ -52,6 +81,7 @@ export class GitHubNotSignedInError extends Schema.TaggedError()( "GitHubHostDisabledError", { host: Schema.String }, @@ -61,6 +91,7 @@ export class GitHubHostDisabledError extends Schema.TaggedError()( "GitHubCliFailedError", { host: Schema.String, cause: Schema.Defect() }, @@ -70,19 +101,28 @@ export class GitHubCliFailedError extends Schema.TaggedError Effect.Effect; - readonly invalidate: (host: string) => Effect.Effect; + /** Drops the held token after GitHub refused it, so the next read asks its source again. */ + readonly invalidate: (host: string, repository?: string) => Effect.Effect; + readonly checkEnabled?: (host: string) => Effect.Effect; } >()("@t3tools/source-control-github/server/GitHubCredentials") {} @@ -123,10 +163,105 @@ export const make = Effect.gen(function* () { const crypto = yield* Crypto.Crypto; const environment = yield* HostProcess.Environment; const workingDirectory = yield* HostProcess.WorkingDirectory; + const gitPaths = new Map(); + + const fromGit = Effect.fnUntraced(function* (repository: string) { + const host = "github.com"; + const invalid = () => new GitHubGitCredentialError({ host, reason: "response" }); + const output = yield* process + .run({ + operation: "GitHubCredentials.git", + command: "git", + args: ["-c", "credential.interactive=false", "credential", "fill"], + // Ignore any repository around the server working directory, including inherited Git + // selectors. Helpers come only from the server's system/global configuration. + cwd: workingDirectory, + stdin: `protocol=https\nhost=github.com\npath=${gitPaths.get(repository) ?? repository}.git\n\n`, + env: { + GIT_DIR: "", + GIT_WORK_TREE: "", + GIT_COMMON_DIR: "", + GIT_CONFIG: "", + GIT_CONFIG_COUNT: "0", + GIT_CONFIG_PARAMETERS: "", + GIT_TERMINAL_PROMPT: "0", + GIT_ASKPASS: "", + SSH_ASKPASS: "", + GCM_INTERACTIVE: "never", + GIT_TRACE: "0", + GIT_TRACE_SETUP: "0", + GIT_TRACE_CURL: "0", + GIT_CURL_VERBOSE: "0", + GIT_TRACE2: "0", + GIT_TRACE2_EVENT: "0", + GIT_TRACE2_PERF: "0", + }, + allowNonZeroExit: true, + timeoutMs: 10_000, + maxOutputBytes: 16_384, + outputMode: "error", + }) + .pipe( + // Neither helper stderr nor lower-level errors may retain credential material. + Effect.mapError(() => new GitHubGitCredentialError({ host, reason: "process" })), + ); + if ( + output.stdoutTruncated || + output.stderrTruncated || + output.stdoutInvalidUtf8 || + output.stderrInvalidUtf8 + ) + return yield* invalid(); + if (output.exitCode !== 0) { + // With prompts disabled, Git's specific missing-credential exit is a normal miss. Any + // helper error before it makes this an operational failure, rather than a fallback. + const missing = + /^fatal: could not read (?:Username|Password) for '[^'\n]*': terminal prompts disabled\r?\n?$/; + if (output.stdout === "" && missing.test(output.stderr)) return null; + return yield* new GitHubGitCredentialError({ host, reason: "process" }); + } + if (output.stderr !== "") + return yield* new GitHubGitCredentialError({ host, reason: "process" }); + if (output.stdout === "") return null; + if (!output.stdout.endsWith("\n") || /[\0\r]/.test(output.stdout)) return yield* invalid(); + const fields = new Map(); + let ended = false; + for (const line of output.stdout.split("\n").slice(0, -1)) { + if (line === "") { + ended = true; + continue; + } + const equals = line.indexOf("="); + if (ended || equals <= 0) return yield* invalid(); + const key = line.slice(0, equals); + if (fields.has(key)) return yield* invalid(); + fields.set(key, line.slice(equals + 1)); + } + // `useHttpPath=false` owner-specific helpers legitimately erase path after selection. + const path = fields.get("path"); + if ( + fields.get("protocol") !== "https" || + fields.get("host") !== host || + (path !== undefined && normalizeCredentialRepository(path) !== repository) + ) + return yield* invalid(); + const token = fields.get("password"); + if (!token) return null; + if (!/^[\x21-\x7e]+$/.test(token)) return yield* invalid(); + return { + host, + repository, + source: "git", + token: Redacted.make(token), + fingerprint: yield* fingerprintOf(host, token), + } satisfies GitHubCredential; + }); + /** `host:sha256(token)`, safe for cache keys and rate-limit scopes. */ const fingerprintOf = (host: string, token: string) => crypto.digest("SHA-256", new TextEncoder().encode(token)).pipe( Effect.map((digest) => `${host}:${Hex.encode(digest)}`), + // Hashing a string in memory has no platform failure worth a typed error. Effect.orDie, ); @@ -171,24 +306,30 @@ export const make = Effect.gen(function* () { settings.sourceControlHosts[GitHubClient.definition.kind], ); + /** The Settings choice for a host; unreadable settings fall back to gh's own choice. */ const hostChoice = (host: string) => sourceControlHost.settings.get.pipe( Effect.map((settings) => readSettings(settings).hosts[host]), Effect.orElseSucceed(() => undefined), ); + /** A token saved in Settings for the host, read fresh so a saved or removed one applies at once. */ const savedToken = (host: string) => sourceControlHost.settings.get.pipe( Effect.map((settings) => readSettings(settings).tokens[host]?.trim() || null), Effect.orElseSucceed(() => null), ); + /** Cache key: the host plus its pinned account, so a changed pin misses the cache. */ const cacheKey = (host: string, account: string | undefined) => account === undefined ? host : `${host}\u0000${account}`; const lookup = Effect.fn("GitHubCredentials.lookup")(function* (key: string) { const [host = key, choice] = key.split("\u0000"); + // An environment token wins over a pinned account, exactly as it does in gh. const fromEnv = environmentToken(host, environment); + // A pinned login gh no longer holds (logged out, expired) falls back to the active one, + // which is what discovery reports as the account in use. const token = fromEnv ?? (yield* fromGh(host, choice).pipe( @@ -207,6 +348,7 @@ export const make = Effect.gen(function* () { const cache = yield* Cache.makeWith(lookup, { capacity: 32, + // A transient gh failure (a timeout, a locked keyring) is asked again on the next read. timeToLive: (exit) => Exit.isSuccess(exit) ? TOKEN_TTL @@ -217,13 +359,44 @@ export const make = Effect.gen(function* () { : MISSING_TTL, }); + const gitCache = yield* Cache.makeWith(fromGit, { + capacity: 128, + timeToLive: (exit) => + Exit.isFailure(exit) ? Duration.zero : exit.value === null ? MISSING_TTL : TOKEN_TTL, + }); + return GitHubCredentials.of({ - get: Effect.fn("GitHubCredentials.get")(function* (rawHost) { + checkEnabled: (rawHost) => { + const host = normalizeHost(rawHost); + return hostChoice(host).pipe( + Effect.flatMap((choice) => + choice?.enabled === false + ? Effect.fail(new GitHubHostDisabledError({ host })) + : Effect.void, + ), + ); + }, + get: Effect.fn("GitHubCredentials.get")(function* (rawHost, repository) { const host = normalizeHost(rawHost); const choice = yield* hostChoice(host); if (choice?.enabled === false) { return yield* new GitHubHostDisabledError({ host }); } + if ( + environment.T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1" && + host === "github.com" && + repository !== undefined + ) { + const normalized = normalizeCredentialRepository(repository); + if (normalized === null) + return yield* new GitHubGitCredentialError({ host, reason: "repository" }); + if (gitPaths.size >= 128) gitPaths.delete(gitPaths.keys().next().value!); + gitPaths.set(normalized, repository.replace(/\.git$/i, "")); + const credential = yield* Cache.get(gitCache, normalized); + if (credential !== null) return credential; + } + // A token saved in Settings is the most deliberate choice, so it comes before the + // environment and gh. It is read from the secret store each time, so it needs no cache. const saved = yield* savedToken(host); if (saved !== null) { return { @@ -235,8 +408,16 @@ export const make = Effect.gen(function* () { } return yield* Cache.get(cache, cacheKey(host, choice?.account)); }), - invalidate: (rawHost) => { + invalidate: (rawHost, repository) => { const host = normalizeHost(rawHost); + const normalized = + repository === undefined ? null : normalizeCredentialRepository(repository); + if ( + host === "github.com" && + normalized !== null && + environment.T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1" + ) + return Cache.invalidate(gitCache, normalized); return hostChoice(host).pipe( Effect.flatMap((choice) => Cache.invalidate(cache, cacheKey(host, choice?.account))), ); diff --git a/packages/source-control-github/src/server/GitHubGitCredentials.test.ts b/packages/source-control-github/src/server/GitHubGitCredentials.test.ts new file mode 100644 index 000000000000..2ed293b3d2df --- /dev/null +++ b/packages/source-control-github/src/server/GitHubGitCredentials.test.ts @@ -0,0 +1,524 @@ +import * as NodeCrypto from "@effect/platform-node/NodeCrypto"; +import * as NodeServices from "@effect/platform-node/NodeServices"; +import { describe, expect, it } from "@effect/vitest"; +import * as Deferred from "effect/Deferred"; +import * as Fiber from "effect/Fiber"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Redacted from "effect/Redacted"; +import * as FileSystem from "effect/FileSystem"; +import * as TestClock from "effect/testing/TestClock"; +import * as Stream from "effect/Stream"; +import { HttpClient, HttpClientResponse } from "effect/http"; +import { ChildProcess, ChildProcessSpawner } from "effect/process"; +import { + DEFAULT_SERVER_SETTINGS, + SourceControlProviderKind, + VcsProcessTimeoutError, +} from "@t3tools/contracts"; +import * as HostProcess from "@t3tools/shared/HostProcess"; +import { applyServerSettingsPatch } from "@t3tools/shared/serverSettings"; +import * as SourceControlHost from "@t3tools/source-control-core/server/SourceControlHost"; +import * as TestSourceControlHost from "@t3tools/source-control-testing/TestSourceControlHost"; +import * as GitHubCredentials from "./GitHubCredentials.ts"; +import * as GitHubRest from "./GitHubRest.ts"; +import * as GitHubQuota from "./GitHubQuota.ts"; +import * as GitHubSourceControlProvider from "./GitHubSourceControlProvider.ts"; +import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; + +const output = ( + stdout: string, + extra: Partial = {}, +) => ({ + exitCode: ChildProcessSpawner.ExitCode(0), + stdout, + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + ...extra, +}); +const response = (path: string, token = "fake-token") => + `protocol=https\nhost=github.com\npath=${path}\nusername=Chinoman10\npassword=${token}\n`; + +function harness( + options: { + enabled?: boolean; + disabledHost?: boolean; + cwd?: string; + run?: SourceControlHost.SourceControlHost["Service"]["process"]["run"]; + } = {}, +) { + const calls: SourceControlHost.SourceControlProcessInput[] = []; + const run: SourceControlHost.SourceControlHost["Service"]["process"]["run"] = (input) => { + calls.push(input); + return ( + options.run?.(input) ?? + Effect.succeed( + output( + response(/path=(.*)\n/.exec(input.stdin ?? "")?.[1] ?? "missing", `fake-${calls.length}`), + ), + ) + ); + }; + const layer = GitHubCredentials.layer.pipe( + Layer.provideMerge( + TestSourceControlHost.layer({ + process: { run }, + settings: applyServerSettingsPatch(DEFAULT_SERVER_SETTINGS, { + sourceControlHosts: { + github: { + tokens: { "github.com": "settings-token" }, + hosts: { + "github.com": { enabled: !options.disabledHost }, + }, + }, + }, + }), + }), + ), + Layer.provide(NodeCrypto.layer), + Layer.provide( + Layer.succeed(HostProcess.Environment, { + T3CODE_GITHUB_USE_GIT_CREDENTIALS: options.enabled === false ? "0" : "1", + GH_TOKEN: "env-token", + }), + ), + Layer.provide(Layer.succeed(HostProcess.WorkingDirectory, options.cwd ?? "/trusted-server")), + ); + return { calls, layer }; +} + +describe("repository Git credentials", () => { + it.effect( + "honors real global owner stores with useHttpPath=false and ignores repository helpers", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const spawner = yield* ChildProcessSpawner.ChildProcessSpawner; + const directory = yield* fs.makeTempDirectoryScoped({ + prefix: "t3-git-owner-credentials-", + }); + const globalConfig = `${directory}/global-config`; + const run: SourceControlHost.SourceControlHost["Service"]["process"]["run"] = (input) => + Effect.scoped( + Effect.gen(function* () { + const child = yield* spawner.spawn( + ChildProcess.make(input.command, input.args, { + cwd: input.cwd, + env: { + ...process.env, + HOME: directory, + XDG_CONFIG_HOME: directory, + GIT_CONFIG_GLOBAL: globalConfig, + GIT_CONFIG_SYSTEM: "/dev/null", + ...input.env, + }, + stdin: Stream.encodeText(Stream.make(input.stdin ?? "")), + }), + ); + const text = (stream: typeof child.stdout) => + stream.pipe( + Stream.decodeText, + Stream.runFold( + () => "", + (all, chunk) => all + chunk, + ), + ); + const [stdout, stderr, exitCode] = yield* Effect.all( + [text(child.stdout), text(child.stderr), child.exitCode], + { concurrency: "unbounded" }, + ); + return output(stdout, { stderr, exitCode }); + }), + ).pipe(Effect.orDie); + yield* fs.writeFileString( + globalConfig, + `[credential]\n\thelper =\n\tuseHttpPath = true\n` + + ["OwnerA", "OwnerB"] + .map( + (owner) => + `[credential "https://github.com/${owner}"]\n\thelper = store --file=${directory}/${owner}.store\n\tuseHttpPath = false\n`, + ) + .join(""), + ); + for (const owner of ["OwnerA", "OwnerB"]) + yield* fs.writeFileString( + `${directory}/${owner}.store`, + `https://Chinoman10:fake-${owner}@github.com\n`, + ); + yield* run({ + operation: "fixture", + command: "git", + args: ["init", "--quiet"], + cwd: directory, + }); + yield* fs.writeFileString( + `${directory}/.git/config`, + '[credential]\n\thelper = "!f() { echo username=local; echo password=repo-local; }; f"\n', + ); + let remotes = ""; + let resolved = ""; + let helperReady: Deferred.Deferred | undefined; + const { layer, calls } = harness({ + cwd: directory, + run: (input) => + input.operation === "GitHubSourceControlProvider.resolveRepository" + ? Effect.succeed(output(input.args[0] === "remote" ? remotes : resolved)) + : run(input).pipe( + Effect.tap(() => + helperReady === undefined + ? Effect.void + : Deferred.succeed(helperReady, undefined), + ), + ), + }); + yield* Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + expect(Redacted.value((yield* credentials.get("github.com", "OwnerA/Repo")).token)).toBe( + "fake-OwnerA", + ); + expect(Redacted.value((yield* credentials.get("github.com", "OwnerB/Repo")).token)).toBe( + "fake-OwnerB", + ); + }).pipe(Effect.provide(layer)); + const authorizations: Array = []; + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => { + authorizations.push(request.headers.authorization); + if (request.url.endsWith("/graphql")) { + expect(request.body._tag).toBe("Uint8Array"); + if (request.body._tag === "Uint8Array") { + const payload = JSON.parse(new TextDecoder().decode(request.body.body)) as { + variables: Record; + }; + const names = Object.entries(payload.variables) + .filter(([key]) => key === "name" || key.endsWith("_name")) + .map(([, value]) => value); + expect(names).toEqual(["Repo"]); + } + } else { + expect(new URL(request.url).pathname).toMatch(/^\/repos\/OwnerA\/Repo(?:\/|$)/); + } + const body = request.url.endsWith("/graphql") + ? { + data: { + repository: { pullRequests: { nodes: [] } }, + _0_repository: { pullRequests: { nodes: [] } }, + }, + } + : { + full_name: "OwnerA/Repo", + html_url: "https://github.com/OwnerA/Repo", + ssh_url: "git@github.com:OwnerA/Repo.git", + default_branch: "main", + }; + return Effect.succeed( + HttpClientResponse.fromWeb( + request, + new Response(JSON.stringify(body), { + headers: { "content-type": "application/json" }, + }), + ), + ); + }), + ); + const transport = GitHubRest.layer.pipe( + Layer.provideMerge( + Layer.mergeAll(layer, http, GitHubQuota.layer, SourceControlRateLimit.layer), + ), + ); + yield* Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const provider = yield* GitHubSourceControlProvider.make; + const bodyFile = `${directory}/pr-body`; + yield* fs.writeFileString(bodyFile, "fixture pull request"); + for (const scenario of [ + { remote: "https://github.com/OwnerA/Repo.git", contextOnly: true }, + { remote: "git@github-work:OwnerA/Repo.git", contextOnly: true }, + { remote: "https://github.com/OwnerA/Repo.git" }, + { remote: "ssh://git@github.com/OwnerA/Repo.git" }, + { remote: "git@github.com:OwnerA/Repo.git" }, + { remote: "git@github-work:OwnerA/Repo.git" }, + { remote: "https://github.com/OwnerA/Repo.git/", contextOnly: true }, + { remote: "ssh://git@github.com/OwnerA/Repo.git/" }, + { remote: "git@github.com:OwnerA/Repo.git/" }, + { remote: "git@github-work:OwnerA/Repo.git/" }, + { remote: "git@github.com:OwnerA/Repo.git", resolved: "base" }, + { remote: "git@github.com:Other/Repo.git", resolved: "OwnerA/Repo" }, + ]) { + remotes = scenario.contextOnly ? "" : `origin\t${scenario.remote} (fetch)`; + resolved = + scenario.resolved === undefined + ? "" + : `remote.origin.gh-resolved ${scenario.resolved}`; + const context = { + provider: { + kind: SourceControlProviderKind.make("github"), + name: "GitHub", + baseUrl: "https://github.com", + }, + remoteName: "origin", + remoteUrl: scenario.remote, + }; + yield* credentials.invalidate("github.com", "OwnerA/Repo"); + expect(yield* provider.getDefaultBranch({ cwd: directory, context })).toBe("main"); + yield* credentials.invalidate("github.com", "OwnerA/Repo"); + helperReady = yield* Deferred.make(); + const pending = yield* provider + .listChangeRequests({ + cwd: directory, + context, + headSelector: "feature", + state: "open", + }) + .pipe(Effect.forkChild); + yield* Deferred.await(helperReady); + yield* TestClock.adjust("500 millis"); + expect(yield* Fiber.join(pending)).toEqual([]); + helperReady = undefined; + yield* provider.createChangeRequest({ + cwd: directory, + context, + baseRefName: "main", + headSelector: "feature", + title: "fixture", + bodyFile, + }); + } + expect(authorizations).toHaveLength(36); + expect(authorizations.every((value) => value === "Bearer fake-OwnerA")).toBe(true); + const paths = calls + .filter((input) => input.operation === "GitHubCredentials.git") + .map((input) => /path=(.*)\n/.exec(input.stdin ?? "")?.[1]); + expect(paths.slice(2)).toEqual(Array(24).fill("OwnerA/Repo.git")); + }).pipe( + Effect.provide(transport), + Effect.provideService(HostProcess.Environment, { + T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1", + }), + ); + }).pipe(Effect.provide(NodeServices.layer)), + ); + + it.effect("keeps legacy precedence when off, and host-only operations use Settings", () => { + const { layer, calls } = harness({ enabled: false }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + expect(Redacted.value((yield* credentials.get("github.com", "Owner/Repo")).token)).toBe( + "settings-token", + ); + expect(Redacted.value((yield* credentials.get("github.com")).token)).toBe("settings-token"); + expect(calls).toHaveLength(0); + }).pipe(Effect.provide(layer)); + }); + + it.effect("selects and caches different PATs for the same login by repository", () => { + const { layer, calls } = harness(); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const first = yield* credentials.get("GitHub.com", "OwnerA/Repo.git"); + const second = yield* credentials.get("github.com", "OwnerB/Repo"); + expect(first.fingerprint).not.toBe(second.fingerprint); + expect(first.source).toBe("git"); + expect(first.repository).toBe("ownera/repo"); + expect(Redacted.value(first.token)).toBe("fake-1"); + expect(Redacted.value(second.token)).toBe("fake-2"); + expect((yield* credentials.get("github.com", "ownera/repo")).fingerprint).toBe( + first.fingerprint, + ); + yield* credentials.invalidate("github.com", "OwnerA/Repo"); + expect((yield* credentials.get("github.com", "ownera/repo")).fingerprint).not.toBe( + first.fingerprint, + ); + expect((yield* credentials.get("github.com", "OwnerB/Repo")).fingerprint).toBe( + second.fingerprint, + ); + expect(calls).toHaveLength(3); + expect(calls[0]?.stdin).toBe("protocol=https\nhost=github.com\npath=OwnerA/Repo.git\n\n"); + expect(calls[0]?.cwd).toBe("/trusted-server"); + expect(calls[0]?.env).toMatchObject({ + GIT_DIR: "", + GIT_CONFIG_COUNT: "0", + GIT_TERMINAL_PROMPT: "0", + GIT_ASKPASS: "", + SSH_ASKPASS: "", + GCM_INTERACTIVE: "never", + GIT_TRACE: "0", + }); + expect(calls[0]?.args).toEqual(["-c", "credential.interactive=false", "credential", "fill"]); + expect(calls[0]?.timeoutMs).toBe(10_000); + expect(calls[0]?.maxOutputBytes).toBe(16_384); + expect(calls[0]?.outputMode).toBe("error"); + }).pipe(Effect.provide(layer)); + }); + + it.effect("does not run helpers for a disabled host or arbitrary host", () => { + const { layer, calls } = harness({ disabledHost: true }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + expect((yield* credentials.get("github.com", "owner/repo").pipe(Effect.flip))._tag).toBe( + "GitHubHostDisabledError", + ); + yield* credentials.get("enterprise.example", "owner/repo"); + expect(calls.every((input) => input.command !== "git")).toBe(true); + }).pipe(Effect.provide(layer)); + }); + + it.effect( + "uses the default credential without a repository and accepts path-free helper stores", + () => { + const { layer, calls } = harness({ + run: () => + Effect.succeed( + output("protocol=https\nhost=github.com\nusername=Chinoman10\npassword=owner-token\n"), + ), + }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + expect((yield* credentials.get("github.com")).source).toBe("settings"); + expect(Redacted.value((yield* credentials.get("github.com", "Owner/Repo")).token)).toBe( + "owner-token", + ); + expect(calls).toHaveLength(1); + }).pipe(Effect.provide(layer)); + }, + ); + + it.effect("falls back only on missing credentials", () => { + const { layer } = harness({ + run: () => + Effect.succeed( + output("", { + exitCode: ChildProcessSpawner.ExitCode(128), + stderr: + "fatal: could not read Username for 'https://github.com/owner/repo.git': terminal prompts disabled\n", + }), + ), + }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + expect((yield* credentials.get("github.com", "owner/repo")).source).toBe("settings"); + }).pipe(Effect.provide(layer)); + }); + + it.effect("rejects unsafe owner/path selectors before invoking Git", () => { + const { layer, calls } = harness(); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + for (const repository of [ + "-owner/repo", + "owner/-repo", + "owner/..", + "owner/../repo", + "owner/repo\npassword=secret", + "owner/repo\0", + "owner/repo/extra", + "https://github.com/owner/repo", + " owner/repo", + ]) { + expect((yield* credentials.get("github.com", repository).pipe(Effect.flip))._tag).toBe( + "GitHubGitCredentialError", + ); + } + expect(calls).toHaveLength(0); + }).pipe(Effect.provide(layer)); + }); + + it.effect.each([ + ["wrong host", response("owner/repo.git").replace("host=github.com", "host=evil.example")], + ["wrong protocol", response("owner/repo.git").replace("protocol=https", "protocol=http")], + ["wrong path", response("other/repo.git")], + ["encoded path", response("owner%2Frepo.git")], + ["duplicate host", `${response("owner/repo.git")}host=github.com\n`], + ["duplicate password", `${response("owner/repo.git")}password=second\n`], + ["truncated record", response("owner/repo.git").trimEnd()], + ["NUL", response("owner/repo.git", "fake\0secret")], + ] as const)("rejects %s without retaining helper output", ([_name, answer]) => { + const { layer, calls } = harness({ run: () => Effect.succeed(output(answer)) }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const error = yield* credentials.get("github.com", "owner/repo").pipe(Effect.flip); + expect(error._tag).toBe("GitHubGitCredentialError"); + expect(JSON.stringify(error)).not.toContain("fake-token"); + expect(calls).toHaveLength(1); + }).pipe(Effect.provide(layer)); + }); + + it.effect("fails safely on helper timeout, nonzero failure, and truncated output", () => { + let attempt = 0; + const { layer, calls } = harness({ + run: () => { + attempt++; + if (attempt === 1) + return Effect.fail( + new VcsProcessTimeoutError({ + operation: "git", + command: "git", + cwd: "/trusted-server", + argumentCount: 2, + timeoutMs: 10_000, + }), + ); + if (attempt === 2) + return Effect.succeed( + output("", { + exitCode: ChildProcessSpawner.ExitCode(1), + stderr: "fake-secret helper failed", + }), + ); + return Effect.succeed(output(response("owner/repo.git"), { stdoutTruncated: true })); + }, + }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + for (let i = 0; i < 3; i++) { + const error = yield* credentials.get("github.com", "owner/repo").pipe(Effect.flip); + expect(error._tag).toBe("GitHubGitCredentialError"); + expect(JSON.stringify(error)).not.toContain("fake-secret"); + } + expect(calls).toHaveLength(3); + }).pipe(Effect.provide(layer)); + }); + it.effect("refreshes positive helper results after five minutes", () => { + const { layer, calls } = harness(); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + yield* credentials.get("github.com", "Owner/Repo"); + yield* TestClock.adjust("4 minutes"); + yield* credentials.get("github.com", "Owner/Repo"); + expect(calls).toHaveLength(1); + yield* TestClock.adjust("1 minute"); + yield* credentials.get("github.com", "Owner/Repo"); + expect(calls).toHaveLength(2); + }).pipe(Effect.provide(layer)); + }); + + it.effect("caches normal helper misses for ten seconds only", () => { + const { layer, calls } = harness({ run: () => Effect.succeed(output("")) }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + yield* credentials.get("github.com", "Owner/Repo"); + yield* credentials.get("github.com", "Owner/Repo"); + expect(calls).toHaveLength(1); + yield* TestClock.adjust("10 seconds"); + yield* credentials.get("github.com", "Owner/Repo"); + expect(calls).toHaveLength(2); + }).pipe(Effect.provide(layer)); + }); + it.effect.each(["stdoutInvalidUtf8", "stderrInvalidUtf8", "stderrTruncated"] as const)( + "rejects %s without retaining helper output", + (flag) => { + const { layer } = harness({ + run: () => + Effect.succeed(output(response("owner/repo.git", "fake-secret"), { [flag]: true })), + }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const error = yield* credentials.get("github.com", "owner/repo").pipe(Effect.flip); + expect(error._tag).toBe("GitHubGitCredentialError"); + expect(JSON.stringify(error)).not.toContain("fake-secret"); + }).pipe(Effect.provide(layer)); + }, + ); +}); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 0e73babffc08..cbd1c2e6f8a8 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -42,6 +42,7 @@ import { type PullRequestPreview, } from "@t3tools/contracts"; +import * as HostProcess from "@t3tools/shared/HostProcess"; import * as GitHubRest from "./GitHubRest.ts"; import * as GitHubRequestPolicy from "./GitHubRequestPolicy.ts"; import * as GitHubGraphQlClient from "./gitHubGraphQlClient.ts"; @@ -396,7 +397,7 @@ export class GitHubPullRequestApi extends Context.Service< GitHubPullRequestApi, { readonly withVerifiedCredential: ( - input: { readonly cwd: string; readonly host: string }, + input: { readonly cwd: string; readonly host: string; readonly repository?: string }, use: (identity: { readonly accountId: string; readonly viewer: string; @@ -406,6 +407,7 @@ export class GitHubPullRequestApi extends Context.Service< readonly getRoutingIdentity: (input: { readonly cwd: string; readonly host: string; + readonly repository?: string; }) => Effect.Effect< { readonly accountId: string; readonly viewer: string }, GitHubPullRequestApiError @@ -413,6 +415,7 @@ export class GitHubPullRequestApi extends Context.Service< readonly getViewerLogin: (input: { readonly cwd: string; readonly host: string; + readonly repository?: string; }) => Effect.Effect; readonly listPullRequests: (input: { @@ -882,6 +885,27 @@ export const make = Effect.gen(function* () { const sourceControlHost = yield* SourceControlHost.SourceControlHost; const fileSystem = yield* FileSystem.FileSystem; const revalidateChecks = yield* makeChecksRevalidator; + const useGitCredentials = + (yield* HostProcess.Environment).T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1"; + const withRepositoryCredential = ( + input: { readonly host: string; readonly repository: string }, + read: Effect.Effect, + ) => + Effect.gen(function* () { + if (!useGitCredentials) return yield* read; + const credential = yield* api.credential(input.host, input.repository); + return yield* read.pipe( + Effect.provideService(GitHubRequestPolicy.PinnedGitHubCredential, { + host: input.host.toLowerCase(), + token: credential.token, + credentialFingerprint: credential.fingerprint, + ...(credential.repository === undefined ? {} : { repository: credential.repository }), + ...(credential.credentialRepository === undefined + ? {} + : { credentialRepository: credential.credentialRepository }), + }), + ); + }); const routingIdentities = new Map< string, { @@ -899,11 +923,29 @@ export const make = Effect.gen(function* () { ), ); const captureVerifiedCredential = Effect.fn("GitHubPullRequestApi.captureVerifiedCredential")( - function* (input: { readonly cwd: string; readonly host: string }) { + function* (input: { + readonly cwd: string; + readonly host: string; + readonly repository?: string; + }) { const unavailable = () => new GitHubViewerLoginUnavailableError({ cwd: input.cwd }); const host = input.host.toLowerCase(); - const { token, fingerprint: key } = yield* api.credential(host); - const credential = { host, token, credentialFingerprint: key }; + // A missing or signed-out credential keeps its own error, so the page can say which. + const { + token, + fingerprint: key, + repository, + credentialRepository, + } = yield* api.credential(host, input.repository); + const credential = { + host, + token, + credentialFingerprint: key, + ...(repository === undefined ? {} : { repository }), + ...(credentialRepository === undefined ? {} : { credentialRepository }), + }; + // A cold page may ask several times. Wait per credential and check again after the + // first verification; cancellation releases the next waiter without losing its request. return yield* Effect.acquireUseRelease( Effect.sync(() => { const lock = identityLocks.get(key) ?? { gate: Semaphore.makeUnsafe(1), users: 0 }; @@ -920,7 +962,13 @@ export const make = Effect.gen(function* () { return { ...credential, ...cached.value }; // Pin this read so an auth switch cannot poison its cache entry. const response = yield* api - .rest({ host, operation: "getRoutingIdentity", path: "user", allowReserve: true }) + .rest({ + host, + ...(input.repository === undefined ? {} : { repository: input.repository }), + operation: "getRoutingIdentity", + path: "user", + allowReserve: true, + }) .pipe( Effect.provideService(GitHubRequestPolicy.PinnedGitHubCredential, credential), Effect.provideService(SourceControlRateLimit.CredentialScope, key), @@ -948,15 +996,26 @@ export const make = Effect.gen(function* () { use, ) => captureVerifiedCredential(input).pipe( - Effect.flatMap(({ host, token, accountId, viewer, credentialFingerprint }) => - use({ accountId, viewer, credentialFingerprint }).pipe( - Effect.provideService(SourceControlRateLimit.CredentialScope, credentialFingerprint), - Effect.provideService(GitHubRequestPolicy.PinnedGitHubCredential, { - host, - token, - credentialFingerprint, - }), - ), + Effect.flatMap( + ({ + host, + token, + accountId, + viewer, + credentialFingerprint, + repository, + credentialRepository, + }) => + use({ accountId, viewer, credentialFingerprint }).pipe( + Effect.provideService(SourceControlRateLimit.CredentialScope, credentialFingerprint), + Effect.provideService(GitHubRequestPolicy.PinnedGitHubCredential, { + host, + token, + credentialFingerprint, + ...(repository === undefined ? {} : { repository }), + ...(credentialRepository === undefined ? {} : { credentialRepository }), + }), + ), ), ); const getRoutingIdentity: GitHubPullRequestApi["Service"]["getRoutingIdentity"] = (input) => @@ -980,6 +1039,7 @@ export const make = Effect.gen(function* () { (input: { readonly cwd: string; readonly host: string; + readonly repository?: string | undefined; readonly operation: string; readonly allowReserve?: boolean | undefined; }) => @@ -996,6 +1056,7 @@ export const make = Effect.gen(function* () { effect.pipe( GitHubGraphQlClient.call({ host: input.host, + repository: input.repository, operation: input.operation, ...(input.allowReserve === true ? { allowReserve: true } : {}), }), @@ -1076,30 +1137,34 @@ export const make = Effect.gen(function* () { readonly host: string; readonly number: number; readonly operation: string; - }): Effect.Effect => { - const { owner, name } = parseRepositorySelector(input.repository); - const key = `${input.host} ${owner}/${name} ${input.number}`; - const held = nodeIds.get(key); - if (held !== undefined) { - nodeIds.delete(key); - nodeIds.set(key, held); - return Effect.succeed(held); - } - return client.PullRequestNodeId({ owner, name, number: input.number }).pipe( - graphql({ ...input, allowReserve: true }), - Effect.map(toPullRequestNodeId), - Effect.flatMap(found(input)), - Effect.tap((nodeId) => - Effect.sync(() => { - if (nodeIds.size >= NODE_ID_CACHE_CAPACITY) { - const oldest = nodeIds.keys().next().value; - if (oldest !== undefined) nodeIds.delete(oldest); - } - nodeIds.set(key, nodeId); - }), - ), - ); - }; + }): Effect.Effect => + Effect.gen(function* () { + const { owner, name } = parseRepositorySelector(input.repository); + const fingerprint = useGitCredentials + ? ((yield* GitHubRequestPolicy.PinnedGitHubCredential)?.credentialFingerprint ?? "") + : ""; + const key = `${fingerprint} ${input.host} ${owner}/${name} ${input.number}`; + const held = nodeIds.get(key); + if (held !== undefined) { + nodeIds.delete(key); + nodeIds.set(key, held); + return held; + } + return yield* client.PullRequestNodeId({ owner, name, number: input.number }).pipe( + graphql({ ...input, allowReserve: true }), + Effect.map(toPullRequestNodeId), + Effect.flatMap(found(input)), + Effect.tap((nodeId) => + Effect.sync(() => { + if (nodeIds.size >= NODE_ID_CACHE_CAPACITY) { + const oldest = nodeIds.keys().next().value; + if (oldest !== undefined) nodeIds.delete(oldest); + } + nodeIds.set(key, nodeId); + }), + ), + ); + }); /** * A subject id is trusted to be whatever node it names, and that node can hang off any pull @@ -1137,12 +1202,18 @@ export const make = Effect.gen(function* () { const restRead = (input: { readonly cwd: string; readonly host: string; + readonly repository?: string | undefined; readonly operation: string; readonly path: string; readonly decode: (raw: string) => Result.Result; }): Effect.Effect => api - .rest({ host: input.host, operation: input.operation, path: input.path }) + .rest({ + host: input.host, + repository: input.repository, + operation: input.operation, + path: input.path, + }) .pipe( Effect.flatMap((response) => decodeWith(input.cwd, input.operation, input.decode, response.body), @@ -1162,6 +1233,7 @@ export const make = Effect.gen(function* () { return api .rest({ host: input.host, + repository: input.repository, operation: "getPullRequestDiff", path: input.commit === undefined @@ -1215,6 +1287,7 @@ export const make = Effect.gen(function* () { const { owner, name } = parseRepositorySelector(input.repository); const refsResponse = yield* api.rest({ host: input.host, + repository: input.repository, operation: "getPullRequestDiffFileContents", path: input.commit === undefined @@ -1251,6 +1324,7 @@ export const make = Effect.gen(function* () { api .rest({ host: input.host, + repository: input.repository, operation: "getPullRequestDiffFileContents", accept: "application/vnd.github.raw+json", path: `repos/${owner}/${name}/contents/${filePath @@ -1456,6 +1530,7 @@ export const make = Effect.gen(function* () { const read: GitHubWorkflowRunPage = yield* restRead({ cwd: input.cwd, host: input.host, + repository: input.repository, operation: "listWorkflowRunsRequiringApproval", path: `repos/${owner}/${name}/actions/runs?head_sha=${encodeURIComponent(input.headSha)}&branch=${encodeURIComponent(input.headBranch)}&event=pull_request&status=action_required&per_page=100&page=${page}`, decode: decodeWorkflowRunsJson, @@ -1552,7 +1627,12 @@ export const make = Effect.gen(function* () { * refused credential) passes through as itself. */ const batchedRead = ( - input: { readonly cwd: string; readonly host: string; readonly operation: string }, + input: { + readonly cwd: string; + readonly host: string; + readonly repository?: string | undefined; + readonly operation: string; + }, effect: Effect.Effect, ): Effect.Effect< A, @@ -1564,6 +1644,7 @@ export const make = Effect.gen(function* () { effect.pipe( Effect.provideService(GitHubGraphQlClient.GitHubGraphQlTarget, { host: input.host, + repository: input.repository, operation: input.operation, }), Effect.mapError((error) => @@ -1649,7 +1730,7 @@ export const make = Effect.gen(function* () { batchKey: GitHubGraphQlClient.batchKey, }).pipe(RequestResolver.batchN(FILES_VIEWED_PER_REQUEST)); - return GitHubPullRequestApi.of({ + const service = GitHubPullRequestApi.of({ withVerifiedCredential, revalidateChecks, getRoutingIdentity, @@ -1668,7 +1749,12 @@ export const make = Effect.gen(function* () { }), ); } - const target = { cwd: input.cwd, host: input.host, operation: "listPullRequests" }; + const target = { + cwd: input.cwd, + host: input.host, + repository: input.repository, + operation: "listPullRequests", + }; const toPage = ( page: ReturnType, ): GitHubPullRequestListPage & { readonly pageInfo: GraphQLClient.PageInfo } => ({ @@ -1817,6 +1903,7 @@ export const make = Effect.gen(function* () { ).pipe( Effect.provideService(GitHubGraphQlClient.GitHubGraphQlTarget, { host: input.host, + repository: input.repository, operation: "listPullRequestStackMemberships", }), Effect.map((items) => ({ ...batch, items })), @@ -1826,6 +1913,33 @@ export const make = Effect.gen(function* () { }, searchPullRequests: (input) => { + // Scoped pins are deliberately repository-local, including when two helpers return the + // same token. A search may never silently borrow the first repository's credential. + if ( + useGitCredentials && + input.host.toLowerCase() === "github.com" && + input.repositories.length > 1 + ) { + return Effect.forEach( + input.repositories, + (repository) => + service.searchPullRequests({ + ...input, + repositories: [repository], + }), + { concurrency: 4 }, + ).pipe( + Effect.map((pages) => { + const items = pages + .flatMap((page) => page.items) + .toSorted((a, b) => b.updatedAt.localeCompare(a.updatedAt)); + return { + items: items.slice(0, input.limit), + truncated: items.length > input.limit || pages.some((page) => page.truncated), + }; + }), + ); + } const query = searchQuery(input); if (query === null) { return Effect.fail( @@ -1844,7 +1958,17 @@ export const make = Effect.gen(function* () { stats: false, }) .pipe( - graphql({ ...input, operation: "searchPullRequests" }), + (effect) => + withRepositoryCredential( + { host: input.host, repository: input.repositories[0]! }, + effect.pipe( + graphql({ + ...input, + repository: input.repositories[0]!, + operation: "searchPullRequests", + }), + ), + ), Effect.map(({ search }) => { const page = toPullRequestRows(search); // A search row names its repository; one that does not cannot be placed on a page. @@ -1859,8 +1983,21 @@ export const make = Effect.gen(function* () { ); }, - listPullRequestStats: (input) => - Effect.gen(function* () { + listPullRequestStats: (input) => { + if (useGitCredentials && input.host.toLowerCase() === "github.com") { + const repositories = [...new Set(input.changeRequests.map((ref) => ref.repository))]; + if (repositories.length > 1) + return Effect.forEach( + repositories, + (repository) => + service.listPullRequestStats({ + ...input, + changeRequests: input.changeRequests.filter((ref) => ref.repository === repository), + }), + { concurrency: 4 }, + ).pipe(Effect.map((pages) => pages.flat())); + } + const read = Effect.gen(function* () { const lookups = input.changeRequests.map((changeRequest) => ({ changeRequest, lookup: pullRequestLookup(changeRequest), @@ -1888,7 +2025,23 @@ export const make = Effect.gen(function* () { { concurrency: "unbounded" }, ); return stats.flat(); - }).pipe((effect) => batchedRead({ ...input, operation: "listPullRequestStats" }, effect)), + }).pipe((effect) => + batchedRead( + { + ...input, + repository: input.changeRequests[0]?.repository, + operation: "listPullRequestStats", + }, + effect, + ), + ); + return input.changeRequests.length === 0 + ? Effect.succeed([]) + : withRepositoryCredential( + { host: input.host, repository: input.changeRequests[0]!.repository }, + read, + ); + }, getPullRequestSummary, getPullRequestWatchFingerprint, @@ -1912,6 +2065,7 @@ export const make = Effect.gen(function* () { return restRead({ cwd: input.cwd, host: input.host, + repository: input.repository, operation: "getPullRequestStack", path: `repos/${owner}/${name}/stacks?pull_request=${input.number}`, decode: decodePullRequestStacksJson, @@ -1922,6 +2076,7 @@ export const make = Effect.gen(function* () { return restRead({ cwd: input.cwd, host: input.host, + repository: input.repository, operation: "getPullRequestStack", path: `repos/${owner}/${name}/stacks/${stack.number}`, decode: (raw) => decodePullRequestStacksJson(`[${raw}]`), @@ -2009,6 +2164,7 @@ export const make = Effect.gen(function* () { return api .rest({ host: input.host, + repository: input.repository, operation: "getPullRequestDiff", path: `repos/${owner}/${name}/pulls/${input.number}`, accept: "application/vnd.github.diff", @@ -2197,6 +2353,7 @@ export const make = Effect.gen(function* () { return api .rest({ host: input.host, + repository: input.repository, operation: "setReviewerRequest", method: input.requested ? "POST" : "DELETE", path: `repos/${owner}/${name}/pulls/${input.number}/requested_reviewers`, @@ -2224,6 +2381,7 @@ export const make = Effect.gen(function* () { return api .rest({ host: input.host, + repository: input.repository, operation: "setLabels", method: "POST", path: issue, @@ -2236,6 +2394,7 @@ export const make = Effect.gen(function* () { (label) => api.rest({ host: input.host, + repository: input.repository, operation: "setLabels", method: "DELETE", path: `${issue}/${encodeURIComponent(label)}`, @@ -2407,6 +2566,7 @@ export const make = Effect.gen(function* () { ? api .rest({ host: input.host, + repository: input.repository, operation: "approveWorkflowRun", method: "POST", path: `repos/${owner}/${name}/actions/runs/${run.id}/approve`, @@ -2518,6 +2678,7 @@ export const make = Effect.gen(function* () { return api .rest({ host: input.host, + repository: input.repository, operation: "submitReview", method: "POST", path: `repos/${owner}/${name}/pulls/${input.number}/reviews`, @@ -2668,6 +2829,61 @@ export const make = Effect.gen(function* () { }), ), }); + // Capture once at each repository operation boundary. A later refusal invalidates the + // source cache for the next operation; it cannot switch credentials midway through this one. + return GitHubPullRequestApi.of({ + ...service, + listPullRequests: (input) => withRepositoryCredential(input, service.listPullRequests(input)), + getPullRequestSummary: (input) => + withRepositoryCredential(input, service.getPullRequestSummary(input)), + getPullRequestWatchFingerprint: (input) => + withRepositoryCredential(input, service.getPullRequestWatchFingerprint(input)), + getPullRequestDetail: (input) => + withRepositoryCredential(input, service.getPullRequestDetail(input)), + getPullRequestPreview: (input) => + withRepositoryCredential(input, service.getPullRequestPreview(input)), + listWorkflowRunsRequiringApproval: (input) => + withRepositoryCredential(input, service.listWorkflowRunsRequiringApproval(input)), + getPullRequestStack: (input) => + withRepositoryCredential(input, service.getPullRequestStack(input)), + getPullRequestActivity: (input) => + withRepositoryCredential(input, service.getPullRequestActivity(input)), + getPullRequestDiff: (input) => + withRepositoryCredential(input, service.getPullRequestDiff(input)), + getPullRequestDiffFileContents: (input) => + withRepositoryCredential(input, service.getPullRequestDiffFileContents(input)), + getPullRequestFilesViewed: (input) => + withRepositoryCredential(input, service.getPullRequestFilesViewed(input)), + setPullRequestFilesViewed: (input) => + withRepositoryCredential(input, service.setPullRequestFilesViewed(input)), + listReviewThreadComments: (input) => + withRepositoryCredential(input, service.listReviewThreadComments(input)), + listActorAvatars: (input) => withRepositoryCredential(input, service.listActorAvatars(input)), + getReviewThreadComments: (input) => + withRepositoryCredential(input, service.getReviewThreadComments(input)), + getViewerAccess: (input) => withRepositoryCredential(input, service.getViewerAccess(input)), + listReviewerCandidates: (input) => + withRepositoryCredential(input, service.listReviewerCandidates(input)), + setReviewerRequest: (input) => + withRepositoryCredential(input, service.setReviewerRequest(input)), + listLabelCandidates: (input) => + withRepositoryCredential(input, service.listLabelCandidates(input)), + setLabels: (input) => withRepositoryCredential(input, service.setLabels(input)), + runPullRequestAction: (input) => + withRepositoryCredential(input, service.runPullRequestAction(input)), + commentOnPullRequest: (input) => + withRepositoryCredential(input, service.commentOnPullRequest(input)), + submitReview: (input) => withRepositoryCredential(input, service.submitReview(input)), + replyToReviewThread: (input) => + withRepositoryCredential(input, service.replyToReviewThread(input)), + setReviewThreadResolution: (input) => + withRepositoryCredential(input, service.setReviewThreadResolution(input)), + setReaction: (input) => withRepositoryCredential(input, service.setReaction(input)), + updatePullRequest: (input) => withRepositoryCredential(input, service.updatePullRequest(input)), + updateComment: (input) => withRepositoryCredential(input, service.updateComment(input)), + revalidateChecks: (input, read) => + withRepositoryCredential(input, service.revalidateChecks(input, read)), + }); }); export const layer = Layer.effect(GitHubPullRequestApi, make); diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts index 7ab10f263949..53a2307b9dfa 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts @@ -246,7 +246,11 @@ export const make = Effect.gen(function* () { getViewer: (input) => cli - .getViewerLogin({ cwd: input.cwd, host: input.host ?? "github.com" }) + .getViewerLogin({ + cwd: input.cwd, + host: input.host ?? "github.com", + ...(input.repository === undefined ? {} : { repository: input.repository }), + }) .pipe(Effect.mapError(fail("getViewer"))), listChangeRequests: (input) => diff --git a/packages/source-control-github/src/server/GitHubRepositoryRouting.test.ts b/packages/source-control-github/src/server/GitHubRepositoryRouting.test.ts new file mode 100644 index 000000000000..2506e6d958ef --- /dev/null +++ b/packages/source-control-github/src/server/GitHubRepositoryRouting.test.ts @@ -0,0 +1,619 @@ +import * as NodeCrypto from "@effect/platform-node/NodeCrypto"; +import * as NodeServices from "@effect/platform-node/NodeServices"; +import { describe, expect, it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Redacted from "effect/Redacted"; +import * as Fiber from "effect/Fiber"; +import * as TestClock from "effect/testing/TestClock"; +import * as Option from "effect/Option"; +import { ChildProcessSpawner } from "effect/process"; +import { HttpClient, HttpClientResponse, type HttpClientRequest } from "effect/http"; +import { DEFAULT_SERVER_SETTINGS, SourceControlProviderKind } from "@t3tools/contracts"; +import * as HostProcess from "@t3tools/shared/HostProcess"; +import { applyServerSettingsPatch } from "@t3tools/shared/serverSettings"; +import * as TestSourceControlHost from "@t3tools/source-control-testing/TestSourceControlHost"; +import * as GitHubApi from "./GitHubRest.ts"; +import * as GitHubPullRequestApi from "./GitHubPullRequestApi.ts"; +import * as GitHubSourceControlProvider from "./GitHubSourceControlProvider.ts"; + +function harness( + refusalStatus = 401, + successfulGraphql = false, + options: { + readonly missingOwners?: ReadonlyArray; + readonly fallbackGh?: boolean; + readonly sameToken?: boolean; + } = {}, +) { + const requests: HttpClientRequest.HttpClientRequest[] = []; + const helperPaths: string[] = []; + const refused = new Set(); + let generation = 1; + const json = (value: unknown, status = 200) => + new Response(JSON.stringify(value), { + status, + headers: { "content-type": "application/json" }, + }); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => { + requests.push(request); + const token = request.headers.authorization ?? ""; + const pathname = new URL(request.url).pathname; + const result = refused.has(token) + ? json({ message: "Bad credentials" }, refusalStatus) + : pathname === "/user" + ? json({ id: 123, login: "Chinoman10" }) + : pathname.startsWith("/repos/") + ? json({ + full_name: pathname.slice("/repos/".length), + html_url: `https://github.com/${pathname.slice(7)}`, + ssh_url: `git@github.com:${pathname.slice(7)}.git`, + }) + : successfulGraphql + ? json({ + data: { + repository: { pullRequest: { id: "PR_1" }, pullRequests: { nodes: [] } }, + addComment: { clientMutationId: null }, + addReaction: { reaction: { content: "THUMBS_UP" } }, + _0_markFileAsViewed: { clientMutationId: null }, + _1_unmarkFileAsViewed: { clientMutationId: null }, + _0_repository: { pullRequests: { nodes: [] } }, + }, + }) + : json({ message: "Not Found" }, 404); + return Effect.succeed(HttpClientResponse.fromWeb(request, result)); + }), + ); + const host = TestSourceControlHost.layer({ + settings: applyServerSettingsPatch(DEFAULT_SERVER_SETTINGS, { + sourceControlHosts: { + github: { tokens: options.fallbackGh ? {} : { "github.com": "host-default" } }, + }, + }), + process: { + run: (input) => + Effect.sync(() => { + if (input.command === "gh") + return { + exitCode: ChildProcessSpawner.ExitCode(0), + stdout: `fake-gh-${generation}\n`, + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + }; + const path = /path=(.*)\n/.exec(input.stdin ?? "")?.[1] ?? ""; + if (path) helperPaths.push(path); + if (options.missingOwners?.includes(path.split("/")[0] ?? "")) + return { + exitCode: ChildProcessSpawner.ExitCode(128), + stdout: "", + stderr: `fatal: could not read Username for 'https://github.com/${path}': terminal prompts disabled\n`, + stdoutTruncated: false, + stderrTruncated: false, + }; + return { + exitCode: ChildProcessSpawner.ExitCode(0), + stdout: `protocol=https\nhost=github.com\nusername=Chinoman10\npassword=fake-${options.sameToken ? "shared" : path.split("/")[0]}-${generation}\n`, + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + }; + }), + }, + }); + const api = GitHubApi.layerWithDependencies.pipe( + Layer.provideMerge(host), + Layer.provideMerge(http), + Layer.provide(NodeCrypto.layer), + ); + const layer = GitHubPullRequestApi.layer.pipe( + Layer.provideMerge(api), + Layer.provideMerge(NodeServices.layer), + Layer.provide( + Layer.succeed(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + ), + Layer.provide(Layer.succeed(HostProcess.WorkingDirectory, "/trusted")), + ); + return { + layer, + requests, + helperPaths, + refused, + rotate: () => { + generation++; + }, + }; +} + +describe("native GitHub repository routing", () => { + it.effect("a verified fallback credential stays scoped to the requested repository", () => { + const { layer, requests, helperPaths } = harness(401, false, { missingOwners: ["OwnerA"] }); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const api = yield* GitHubApi.GitHubRest; + const error = yield* prs.withVerifiedCredential( + { cwd: "/w", host: "github.com", repository: "OwnerA/Repo" }, + () => + api + .rest({ + host: "github.com", + repository: "OwnerB/Repo", + operation: "test", + path: "repos/OwnerB/Repo", + }) + .pipe(Effect.flip), + ); + expect(error._tag).toBe("GitHubApiAuthenticationError"); + expect(requests).toHaveLength(1); + expect(requests[0]?.headers.authorization).toBe("Bearer host-default"); + const ownB = yield* api.credential("github.com", "OwnerB/Repo"); + expect(Redacted.value(ownB.token)).toBe("fake-OwnerB-1"); + expect(helperPaths).toEqual(["OwnerA/Repo.git", "OwnerB/Repo.git"]); + }).pipe(Effect.provide(layer)); + }); + + it.effect("an operation wrapper cannot reuse repository A's fallback credential for B", () => { + const { layer, requests, helperPaths, refused } = harness(404, false, { + missingOwners: ["OwnerA"], + }); + refused.add("Bearer host-default"); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const api = yield* GitHubApi.GitHubRest; + const error = yield* prs + .revalidateChecks( + { cwd: "/w", host: "github.com", repository: "OwnerA/Repo", number: 1 }, + api + .rest({ + host: "github.com", + repository: "OwnerB/Repo", + operation: "test", + path: "repos/OwnerB/Repo", + }) + .pipe(Effect.as({ state: "open" as const, checks: [], headSha: "a".repeat(40) })), + ) + .pipe(Effect.flip); + expect(error._tag).toBe("GitHubApiAuthenticationError"); + expect(requests).toHaveLength(1); + expect(requests[0]?.url).toContain("/repos/OwnerA/Repo/"); + expect(helperPaths).toEqual(["OwnerA/Repo.git"]); + }).pipe(Effect.provide(layer)); + }); + + it.effect("validates requested repository context before returning a host-only pin", () => { + const { layer, requests, helperPaths } = harness(); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const api = yield* GitHubApi.GitHubRest; + const error = yield* prs.withVerifiedCredential({ cwd: "/w", host: "github.com" }, () => + api.credential("github.com", "owner/repo\npassword=unsafe").pipe(Effect.flip), + ); + expect(error._tag).toBe("GitHubGitCredentialError"); + expect(requests).toHaveLength(1); + expect(helperPaths).toHaveLength(0); + }).pipe(Effect.provide(layer)); + }); + + it.effect.each([401, 403])( + "invalidates a scoped gh fallback after HTTP %s while retaining B's helper token", + (status) => { + const { layer, requests, helperPaths, refused, rotate } = harness(status, false, { + missingOwners: ["OwnerA"], + fallbackGh: true, + }); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const api = yield* GitHubApi.GitHubRest; + yield* api.credential("github.com", "OwnerB/Repo"); + const ref = { cwd: "/w", host: "github.com", repository: "OwnerA/Repo", number: 1 }; + const error = yield* prs + .withVerifiedCredential(ref, () => + Effect.sync(() => refused.add("Bearer fake-gh-1")).pipe( + Effect.andThen( + prs.submitReview({ ...ref, verdict: "comment", body: "Fake review", comments: [] }), + ), + ), + ) + .pipe(Effect.flip); + expect(error._tag).toBe( + status === 401 ? "GitHubApiAuthenticationError" : "GitHubApiResponseError", + ); + expect(requests.map((request) => request.headers.authorization)).toEqual([ + "Bearer fake-gh-1", + "Bearer fake-gh-1", + ]); + rotate(); + expect(Redacted.value((yield* api.credential("github.com", "OwnerA/Repo")).token)).toBe( + "fake-gh-2", + ); + expect(Redacted.value((yield* api.credential("github.com", "OwnerB/Repo")).token)).toBe( + "fake-OwnerB-1", + ); + expect(helperPaths).toEqual(["OwnerB/Repo.git", "OwnerA/Repo.git"]); + }).pipe(Effect.provide(layer)); + }, + ); + + it.effect("routes native PR writes through the repository PAT", () => { + const { layer, requests } = harness(401, true); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const ref = { cwd: "/w", host: "github.com", repository: "OwnerA/Repo", number: 1 }; + yield* prs.commentOnPullRequest({ ...ref, body: "Fake test comment" }); + yield* prs.submitReview({ ...ref, verdict: "comment", body: "Fake review", comments: [] }); + yield* prs.setLabels({ ...ref, labels: ["bug"], applied: true }); + yield* prs.setReaction({ ...ref, content: "thumbs-up", reacted: true }); + yield* prs.setPullRequestFilesViewed({ + ...ref, + files: [ + { path: "file.ts", viewed: true }, + { path: "other.ts", viewed: false }, + ], + }); + expect(requests).toHaveLength(6); + expect( + requests.every((request) => request.headers.authorization === "Bearer fake-OwnerA-1"), + ).toBe(true); + expect(requests.every((request) => request.method === "POST")).toBe(true); + const viewed = requests.at(-1)?.body; + expect(viewed?._tag).toBe("Uint8Array"); + if (viewed?._tag === "Uint8Array") { + const payload = JSON.parse(new TextDecoder().decode(viewed.body)); + expect(payload.query).toContain("_0_markFileAsViewed"); + expect(payload.query).toContain("_1_unmarkFileAsViewed"); + } + }).pipe(Effect.provide(layer)); + }); + + it.effect("a newly disabled host rejects a captured pin before another request", () => { + const { layer, requests, helperPaths } = harness(); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const api = yield* GitHubApi.GitHubRest; + const settings = yield* TestSourceControlHost.TestSourceControlHostSettings; + const error = yield* prs.withVerifiedCredential( + { cwd: "/w", host: "github.com", repository: "OwnerA/Repo" }, + () => + settings + .update({ + sourceControlHosts: { github: { hosts: { "github.com": { enabled: false } } } }, + }) + .pipe( + Effect.andThen( + api.rest({ + host: "github.com", + repository: "OwnerA/Repo", + operation: "test", + path: "repos/OwnerA/Repo", + }), + ), + Effect.flip, + ), + ); + expect(error._tag).toBe("GitHubHostDisabledError"); + expect(requests).toHaveLength(1); + expect(helperPaths).toHaveLength(1); + }).pipe(Effect.provide(layer)); + }); + + it.effect("invalidates a non-rate-limited 403 without retrying with another token", () => { + const { layer, requests, refused, helperPaths, rotate } = harness(403); + return Effect.gen(function* () { + const api = yield* GitHubApi.GitHubRest; + refused.add("Bearer fake-OwnerA-1"); + const error = yield* api + .rest({ + host: "github.com", + repository: "OwnerA/Repo", + operation: "test", + path: "repos/OwnerA/Repo", + }) + .pipe(Effect.flip); + expect(error._tag).toBe("GitHubApiResponseError"); + expect(requests).toHaveLength(1); + rotate(); + expect(Redacted.value((yield* api.credential("github.com", "OwnerA/Repo")).token)).toBe( + "fake-OwnerA-2", + ); + expect(helperPaths).toHaveLength(2); + }).pipe(Effect.provide(layer)); + }); + + it.effect( + "offers repository lookup without claiming a host-wide login in helper-only mode", + () => { + const commands: string[] = []; + return Effect.gen(function* () { + const discovery = yield* GitHubSourceControlProvider.makeDiscovery; + const available = yield* discovery.probe("/untrusted-checkout"); + expect(available.status).toBe("available"); + expect(available.auth.status).toBe("unknown"); + expect(Option.isNone(available.auth.account)).toBe(true); + expect(Option.getOrNull(available.auth.detail)).toContain("per repository"); + const settings = yield* TestSourceControlHost.TestSourceControlHostSettings; + yield* settings.update({ + sourceControlHosts: { github: { hosts: { "github.com": { enabled: false } } } }, + }); + yield* discovery.probe("/untrusted-checkout"); + expect(commands.filter((command) => command === "git")).toHaveLength(1); + }).pipe( + Effect.provide( + Layer.mergeAll( + Layer.mock(GitHubApi.GitHubRest)({}), + TestSourceControlHost.layer({ + process: { + run: (input) => + Effect.sync(() => { + commands.push(input.command); + return { + exitCode: ChildProcessSpawner.ExitCode(input.command === "git" ? 0 : 1), + stdout: input.command === "git" ? "git version 2.43.0\n" : "", + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, + }; + }), + }, + }), + ), + ), + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + Effect.provideService(HostProcess.WorkingDirectory, "/trusted"), + ); + }, + ); + + it.effect("routes repository lookup before cloning for shorthand and full URLs", () => { + const { layer, requests, helperPaths } = harness(); + return Effect.gen(function* () { + const provider = yield* GitHubSourceControlProvider.make; + for (const repository of ["OwnerA/Repo", "https://github.com/OwnerB/Repo.git"]) { + yield* provider.getRepositoryCloneUrls({ cwd: "/checkout-does-not-exist", repository }); + } + expect(requests.map((request) => request.headers.authorization)).toEqual([ + "Bearer fake-OwnerA-1", + "Bearer fake-OwnerB-1", + ]); + expect(helperPaths).toEqual(["OwnerA/Repo.git", "OwnerB/Repo.git"]); + }).pipe(Effect.provide(layer)); + }); + + it.effect( + "keeps host-only discovery on the default and repository viewers distinct by PAT", + () => { + const { layer, requests, helperPaths } = harness(); + return Effect.gen(function* () { + const api = yield* GitHubApi.GitHubRest; + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + yield* api.rest({ host: "github.com", operation: "discovery", path: "user" }); + for (const repository of ["OwnerA/Repo", "OwnerB/Repo", "OwnerA/Repo"]) { + expect( + yield* prs.getRoutingIdentity({ cwd: "/w", host: "github.com", repository }), + ).toEqual({ accountId: "123", viewer: "Chinoman10" }); + } + expect(requests.map((request) => request.headers.authorization)).toEqual([ + "Bearer host-default", + "Bearer fake-OwnerA-1", + "Bearer fake-OwnerB-1", + ]); + expect(helperPaths).toHaveLength(2); + }).pipe(Effect.provide(layer)); + }, + ); + + it.effect("rejects a repository-scoped pin for another repository or host before HTTP", () => { + const { layer, requests } = harness(); + return Effect.gen(function* () { + const api = yield* GitHubApi.GitHubRest; + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const errors = yield* prs.withVerifiedCredential( + { cwd: "/w", host: "github.com", repository: "OwnerA/Repo" }, + () => + Effect.all([ + api + .rest({ + host: "github.com", + repository: "OwnerB/Repo", + operation: "test", + path: "repos/OwnerB/Repo", + }) + .pipe(Effect.flip), + api + .rest({ + host: "evil.example", + repository: "OwnerA/Repo", + operation: "test", + path: "user", + }) + .pipe(Effect.flip), + ]), + ); + expect(errors.map((error) => error._tag)).toEqual([ + "GitHubApiAuthenticationError", + "GitHubApiAuthenticationError", + ]); + expect(requests).toHaveLength(1); + }).pipe(Effect.provide(layer)); + }); + + it.effect("invalidates only the refused repository with no default-token retry", () => { + const { layer, requests, helperPaths, refused, rotate } = harness(); + return Effect.gen(function* () { + const api = yield* GitHubApi.GitHubRest; + yield* api.credential("github.com", "OwnerB/Repo"); + refused.add("Bearer fake-OwnerA-1"); + const error = yield* api + .rest({ + host: "github.com", + repository: "OwnerA/Repo", + operation: "test", + path: "repos/OwnerA/Repo", + }) + .pipe(Effect.flip); + expect(error._tag).toBe("GitHubApiAuthenticationError"); + expect(requests).toHaveLength(1); + expect(requests[0]?.headers.authorization).toBe("Bearer fake-OwnerA-1"); + rotate(); + expect(Redacted.value((yield* api.credential("github.com", "OwnerA/Repo")).token)).toBe( + "fake-OwnerA-2", + ); + expect(Redacted.value((yield* api.credential("github.com", "OwnerB/Repo")).token)).toBe( + "fake-OwnerB-1", + ); + expect(helperPaths).toEqual(["OwnerB/Repo.git", "OwnerA/Repo.git", "OwnerA/Repo.git"]); + }).pipe(Effect.provide(layer)); + }); + + it.effect("separates summary/watch GraphQL batches even for PATs with the same login", () => { + const { layer, requests } = harness(); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + for (const read of [prs.getPullRequestSummary, prs.getPullRequestWatchFingerprint]) { + const pending = yield* Effect.all( + ["OwnerA/Repo", "OwnerB/Repo"].map((repository) => + Effect.gen(function* () { + yield* read({ cwd: "/w", host: "github.com", repository, number: 1 }); + }).pipe(Effect.flip), + ), + { concurrency: "unbounded" }, + ).pipe(Effect.forkChild); + yield* TestClock.adjust("10 millis"); + const errors = yield* Fiber.join(pending); + expect(errors.map((error) => error._tag)).toEqual([ + "GitHubApiNotFoundError", + "GitHubApiNotFoundError", + ]); + } + const queries = requests.filter((request) => new URL(request.url).pathname === "/graphql"); + expect(queries).toHaveLength(4); + expect(queries.map((request) => request.headers.authorization).sort()).toEqual([ + "Bearer fake-OwnerA-1", + "Bearer fake-OwnerA-1", + "Bearer fake-OwnerB-1", + "Bearer fake-OwnerB-1", + ]); + }).pipe(Effect.provide(layer)); + }); + it.effect("keeps the same token scoped to each repository in summary batches", () => { + const { layer, requests, helperPaths } = harness(404, false, { sameToken: true }); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const pending = yield* Effect.forEach( + ["OwnerA/Repo", "OwnerB/Repo"], + (repository) => + prs + .getPullRequestSummary({ cwd: "/w", host: "github.com", repository, number: 1 }) + .pipe(Effect.flip), + { concurrency: "unbounded" }, + ).pipe(Effect.forkChild); + yield* TestClock.adjust("10 millis"); + yield* Fiber.join(pending); + expect(requests).toHaveLength(2); + expect(requests.map((request) => request.headers.authorization)).toEqual([ + "Bearer fake-shared-1", + "Bearer fake-shared-1", + ]); + expect(helperPaths).toHaveLength(2); + }).pipe(Effect.provide(layer)); + }); + + it.effect.each(["open", "all"] as const)( + "separates source head resolver credentials for %s lookups", + (state) => { + const { layer, requests } = harness(401, true); + return Effect.gen(function* () { + const provider = yield* GitHubSourceControlProvider.make; + const pending = yield* Effect.forEach( + ["OwnerA", "OwnerB"], + (owner) => + provider.listChangeRequests({ + cwd: "/w", + state, + headSelector: "feature", + context: { + provider: { + kind: SourceControlProviderKind.make("github"), + name: "GitHub", + baseUrl: "https://github.com", + }, + remoteName: "origin", + remoteUrl: `https://github.com/${owner}/Repo.git`, + }, + }), + { concurrency: "unbounded" }, + ).pipe(Effect.forkChild); + yield* TestClock.adjust("500 millis"); + expect(yield* Fiber.join(pending)).toEqual([[], []]); + expect(requests).toHaveLength(2); + expect(requests.map((request) => request.headers.authorization).sort()).toEqual([ + "Bearer fake-OwnerA-1", + "Bearer fake-OwnerB-1", + ]); + }).pipe( + Effect.provide(layer), + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + ); + }, + ); + + it.effect.each([401, 403, 404])("makes only one GraphQL attempt after HTTP %s", (status) => { + const { layer, requests, refused } = harness(status); + refused.add("Bearer fake-OwnerA-1"); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const pending = yield* prs + .getPullRequestSummary({ + cwd: "/w", + host: "github.com", + repository: "OwnerA/Repo", + number: 1, + }) + .pipe(Effect.flip, Effect.forkChild); + yield* TestClock.adjust("10 millis"); + expect((yield* Fiber.join(pending))._tag).toBe( + status === 401 + ? "GitHubApiAuthenticationError" + : status === 404 + ? "GitHubApiNotFoundError" + : "GitHubApiResponseError", + ); + expect(requests).toHaveLength(1); + expect(requests[0]?.headers.authorization).toBe("Bearer fake-OwnerA-1"); + }).pipe(Effect.provide(layer)); + }); + + it.effect("rejects context-free requests under a repository pin", () => { + const { layer, requests } = harness(); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const api = yield* GitHubApi.GitHubRest; + const error = yield* prs.withVerifiedCredential( + { cwd: "/w", host: "github.com", repository: "OwnerA/Repo" }, + () => api.rest({ host: "github.com", operation: "test", path: "user" }).pipe(Effect.flip), + ); + expect(error._tag).toBe("GitHubApiAuthenticationError"); + expect(requests).toHaveLength(1); + }).pipe(Effect.provide(layer)); + }); + + it.effect("does not share the node-id cache after a repository token rotates", () => { + const { layer, requests, rotate } = harness(401, true); + return Effect.gen(function* () { + const prs = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const ref = { cwd: "/w", host: "github.com", repository: "OwnerA/Repo", number: 1 }; + yield* prs.commentOnPullRequest({ ...ref, body: "first" }); + rotate(); + yield* TestClock.adjust("5 minutes"); + yield* prs.commentOnPullRequest({ ...ref, body: "second" }); + expect(requests).toHaveLength(4); + expect(requests.map((request) => request.headers.authorization)).toEqual([ + "Bearer fake-OwnerA-1", + "Bearer fake-OwnerA-1", + "Bearer fake-OwnerA-2", + "Bearer fake-OwnerA-2", + ]); + }).pipe(Effect.provide(layer)); + }); +}); diff --git a/packages/source-control-github/src/server/GitHubRequestPolicy.ts b/packages/source-control-github/src/server/GitHubRequestPolicy.ts index 831ba010b9c8..7bdb35ba29a3 100644 --- a/packages/source-control-github/src/server/GitHubRequestPolicy.ts +++ b/packages/source-control-github/src/server/GitHubRequestPolicy.ts @@ -10,6 +10,7 @@ import * as Schema from "effect/Schema"; import * as Semaphore from "effect/Semaphore"; import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; +import * as HostProcess from "@t3tools/shared/HostProcess"; import * as GitHubCredentials from "./GitHubCredentials.ts"; import * as GitHubQuota from "./GitHubQuota.ts"; @@ -28,6 +29,9 @@ export const PinnedGitHubCredential = Context.Reference<{ readonly host: string; readonly token: Redacted.Redacted; readonly credentialFingerprint: string; + /** Operation scope is independent of the selected credential cache. */ + readonly repository?: string; + readonly credentialRepository?: string; } | null>("@t3tools/source-control-github/server/GitHubRequestPolicy/PinnedGitHubCredential", { defaultValue: () => null, }); @@ -200,6 +204,7 @@ export type GitHubAnswer = { export interface GitHubRequest { readonly host: string; + readonly repository?: string | undefined; readonly operation: string; /** The quota the request spends: `core` for REST, `graphql` for GraphQL. */ readonly resource: string; @@ -227,8 +232,14 @@ export class GitHubRequestPolicy extends Context.Service< /** The credential a request to `host` would carry right now. */ readonly credential: ( host: string, + repository?: string, ) => Effect.Effect< - { readonly token: Redacted.Redacted; readonly fingerprint: string }, + { + readonly token: Redacted.Redacted; + readonly fingerprint: string; + readonly repository?: string; + readonly credentialRepository?: string; + }, GitHubApiError >; } @@ -241,23 +252,54 @@ export const make = Effect.gen(function* () { const limits = yield* SourceControlRateLimit.SourceControlRateLimit; const slots = yield* Semaphore.make(CONCURRENCY); + const useGitCredentials = + (yield* HostProcess.Environment).T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1"; const credential: GitHubRequestPolicy["Service"]["credential"] = Effect.fn( - "GitHubRest.credential", - )(function* (host) { + "GitHubRequestPolicy.credential", + )(function* (host, repository) { const normalized = normalizeHost(host); + const requestedRepository = + useGitCredentials && normalized === "github.com" && repository !== undefined + ? GitHubCredentials.normalizeCredentialRepository(repository) + : undefined; const pinned = yield* PinnedGitHubCredential; if (pinned !== null) { + yield* credentials.checkEnabled?.(normalized) ?? Effect.void; + if (requestedRepository === null) { + return yield* new GitHubCredentials.GitHubGitCredentialError({ + host: normalized, + reason: "repository", + }); + } // A pinned page only ever talks to the host it verified. - if (pinned.host !== normalized) { + if ( + pinned.host !== normalized || + (pinned.repository !== undefined && + GitHubCredentials.normalizeCredentialRepository(repository ?? "") !== pinned.repository) + ) { return yield* new GitHubApiAuthenticationError({ host: normalized, operation: "credential", }); } - return { token: pinned.token, fingerprint: pinned.credentialFingerprint }; + const operationRepository = requestedRepository ?? pinned.repository; + return { + token: pinned.token, + fingerprint: pinned.credentialFingerprint, + ...(operationRepository === undefined ? {} : { repository: operationRepository }), + ...(pinned.credentialRepository === undefined + ? {} + : { credentialRepository: pinned.credentialRepository }), + }; } - const held = yield* credentials.get(normalized); - return { token: held.token, fingerprint: held.fingerprint }; + const held = yield* credentials.get(normalized, repository); + const operationRepository = requestedRepository ?? held.repository; + return { + token: held.token, + fingerprint: held.fingerprint, + ...(operationRepository === undefined ? {} : { repository: operationRepository }), + ...(held.repository === undefined ? {} : { credentialRepository: held.repository }), + }; }); const request: GitHubRequestPolicy["Service"]["request"] = ( @@ -271,7 +313,10 @@ export const make = Effect.gen(function* () { "github.host": host, "github.operation": input.operation, }); - const { token, fingerprint } = yield* credential(host); + const { token, fingerprint, repository, credentialRepository } = yield* credential( + host, + input.repository, + ); const scope = yield* SourceControlRateLimit.CredentialScope; // A refusal pauses the whole host, whichever quota it spent: the key PullRequestService // records its own backoff under. @@ -284,6 +329,9 @@ export const make = Effect.gen(function* () { ...(answer.status === undefined ? {} : { "http.response.status_code": answer.status }), ...rateLimitAttributes(answer.headers), }); + if (repository !== undefined && answer.status === 403 && answer._tag !== "RateLimited") { + yield* credentials.invalidate(host, credentialRepository); + } switch (answer._tag) { case "Ok": yield* limits.recordSuccess({ ...key, lease }); @@ -302,7 +350,7 @@ export const make = Effect.gen(function* () { } case "Unauthorized": // The source may hold a newer token than the one that was refused. - yield* credentials.invalidate(host); + yield* credentials.invalidate(host, credentialRepository); return yield* new GitHubApiAuthenticationError(context); case "NotFound": return yield* new GitHubApiNotFoundError(context); diff --git a/packages/source-control-github/src/server/GitHubRest.ts b/packages/source-control-github/src/server/GitHubRest.ts index 6ed65eebabf7..18f275a39015 100644 --- a/packages/source-control-github/src/server/GitHubRest.ts +++ b/packages/source-control-github/src/server/GitHubRest.ts @@ -3,7 +3,6 @@ import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; -import * as Redacted from "effect/Redacted"; import * as Schema from "effect/Schema"; import { HttpClient, HttpClientRequest, type HttpClientResponse } from "effect/http"; @@ -28,6 +27,7 @@ export interface GitHubRestResponse { export interface GitHubRestInput { readonly host: string; + readonly repository?: string | undefined; readonly operation: string; readonly method?: "GET" | "POST" | "PUT" | "PATCH" | "DELETE"; /** Relative to the API root, e.g. `repos/acme/web/pulls/7`; query string included. */ @@ -49,12 +49,7 @@ export class GitHubRest extends Context.Service< { readonly rest: (input: GitHubRestInput) => Effect.Effect; /** The credential a request to `host` would carry right now. */ - readonly credential: ( - host: string, - ) => Effect.Effect< - { readonly token: Redacted.Redacted; readonly fingerprint: string }, - GitHubApiError - >; + readonly credential: GitHubRequestPolicy.GitHubRequestPolicy["Service"]["credential"]; } >()("@t3tools/source-control-github/server/GitHubRest") {} @@ -127,6 +122,7 @@ export const make = Effect.gen(function* () { */ const send = Effect.fn("GitHubRest.send")(function* (input: { readonly host: string; + readonly repository?: string | undefined; readonly operation: string; readonly request: HttpClientRequest.HttpClientRequest; readonly maxResponseBytes: number; @@ -144,6 +140,7 @@ export const make = Effect.gen(function* () { return yield* policy.request( { host, + repository: input.repository, operation: input.operation, // Every REST path this reads today spends `core`; a `search/` read would need its own // resource here. @@ -209,6 +206,7 @@ export const make = Effect.gen(function* () { Effect.flatMap((interactive) => send({ host: input.host, + repository: input.repository, operation: input.operation, request, maxResponseBytes: input.maxResponseBytes ?? DEFAULT_MAX_RESPONSE_BYTES, diff --git a/packages/source-control-github/src/server/GitHubSourceControlProvider.ts b/packages/source-control-github/src/server/GitHubSourceControlProvider.ts index 8c8aa06cd9a0..7a6ae75cd570 100644 --- a/packages/source-control-github/src/server/GitHubSourceControlProvider.ts +++ b/packages/source-control-github/src/server/GitHubSourceControlProvider.ts @@ -38,6 +38,7 @@ import { gitHubApiHostForRemote, parseFetchRemotes, parseGitHubRepositorySelector, + parseGitHubRemoteRepositorySelector, parsePullRequestReference, resolveGitHubRepository, type GitHubRepositoryLocator, @@ -219,6 +220,7 @@ export const makeDiscovery = Effect.gen(function* () { const sourceControlHost = yield* SourceControlHost.SourceControlHost; const process = sourceControlHost.process; const environment = yield* HostProcess.Environment; + const workingDirectory = yield* HostProcess.WorkingDirectory; return { type: "managed-cli", @@ -241,7 +243,42 @@ export const makeDiscovery = Effect.gen(function* () { const savedToken = (settings.tokens["github.com"] ?? "").trim() !== ""; const variable = environmentTokenVariable(environment); const tokenSource = savedToken ? "the token saved in Settings" : variable; - if (tokenSource === null || settings.hosts["github.com"]?.enabled === false) return cli; + const repositoryFallback = (item: SourceControlProviderDiscoveryItem) => + Effect.gen(function* () { + if ( + environment.T3CODE_GITHUB_USE_GIT_CREDENTIALS !== "1" || + item.auth.status === "authenticated" + ) + return item; + const available = yield* process + .run({ + operation: "GitHubSourceControlProvider.gitCredentialsDiscovery", + command: "git", + args: ["--version"], + cwd: workingDirectory, + timeoutMs: 5_000, + maxOutputBytes: 1_024, + }) + .pipe( + Effect.as(true), + Effect.orElseSucceed(() => false), + ); + return available + ? { + ...item, + status: "available" as const, + auth: providerAuth({ + status: "unknown", + host: "github.com", + detail: + "Git credentials are resolved per repository. Enter an owner/repository or GitHub URL; host-wide operations still need a default GitHub credential.", + }), + } + : item; + }); + // A host turned off in Settings stays off even with a token. + if (settings.hosts["github.com"]?.enabled === false) return cli; + if (tokenSource === null) return yield* repositoryFallback(cli); const viewer = yield* api .rest({ host: "github.com", operation: "discovery", path: "user" }) .pipe(Effect.result); @@ -249,7 +286,7 @@ export const makeDiscovery = Effect.gen(function* () { ? Option.getOrUndefined(decodeViewer(viewer.success.body))?.login : undefined; const accounts = cli.auth.accounts === undefined ? {} : { accounts: cli.auth.accounts }; - return { + return yield* repositoryFallback({ ...cli, status: "available" as const, auth: { @@ -277,7 +314,7 @@ export const makeDiscovery = Effect.gen(function* () { })), ...accounts, }, - } satisfies SourceControlProviderDiscoveryItem; + } satisfies SourceControlProviderDiscoveryItem); }), /** * A host turned off in Settings is claimed too, so its error says so @@ -345,6 +382,7 @@ function fromGitHubApiError( case "GitHubApiNotFoundError": return failure(notFound, error); case "GitHubCliFailedError": + case "GitHubGitCredentialError": case "GitHubApiResponseError": case "GitHubApiRequestError": return failure(error.message.trim() || "GitHub request failed.", error); @@ -488,14 +526,18 @@ export const make = Effect.gen(function* () { resolved: resolved?.exitCode === 0 ? resolved.stdout : "", hostHint, defaultHost, + preserveRepositoryCase: environment.T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1", }); if (locator !== null) return locator; if (input.context) { const remote = normalizeGitRemoteUrl(input.context.remoteUrl); - const fallback = parseGitHubRepositorySelector( - `${host}/${remote.slice(remote.indexOf("/") + 1)}`, - host, - ); + const fallback = + environment.T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1" && host === "github.com" + ? parseGitHubRemoteRepositorySelector(input.context.remoteUrl, host) + : parseGitHubRepositorySelector( + `${host}/${remote.slice(remote.indexOf("/") + 1)}`, + host, + ); if (fallback !== null) return fallback; } return yield* failure( @@ -519,8 +561,23 @@ export const make = Effect.gen(function* () { decodeDetail: string, effect: Effect.Effect, ) => - effect.pipe( - GitHubGraphQlClient.call(target), + Effect.gen(function* () { + if (environment.T3CODE_GITHUB_USE_GIT_CREDENTIALS !== "1") + return yield* effect.pipe(GitHubGraphQlClient.call(target)); + const credential = yield* api.credential(target.host, target.repository); + return yield* effect.pipe( + GitHubGraphQlClient.call(target), + Effect.provideService(GitHubRequestPolicy.PinnedGitHubCredential, { + host: target.host.toLowerCase(), + token: credential.token, + credentialFingerprint: credential.fingerprint, + ...(credential.repository === undefined ? {} : { repository: credential.repository }), + ...(credential.credentialRepository === undefined + ? {} + : { credentialRepository: credential.credentialRepository }), + }), + ); + }).pipe( Effect.mapError((error) => GraphQLClientError.isGraphQLClientError(error) ? failure(decodeDetail, error) @@ -565,6 +622,7 @@ export const make = Effect.gen(function* () { const result = yield* graphqlRead( { host: locator.host, + repository: `${locator.owner}/${locator.name}`, operation: "listPullRequestsByHead", allowReserve: input.allowReserve, // Up to 50 heads of 100 rows each. A default branch such as `main` can match a hundred @@ -624,7 +682,12 @@ export const make = Effect.gen(function* () { const locator = parsed.kind === "url" ? parsed.locator : yield* resolveRepository(input); const decodeDetail = "GitHub returned an invalid pull request."; const result = yield* graphqlRead( - { host: locator.host, operation: "getPullRequest", allowReserve: true }, + { + host: locator.host, + repository: `${locator.owner}/${locator.name}`, + operation: "getPullRequest", + allowReserve: true, + }, decodeDetail, client.PullRequestByNumber({ owner: locator.owner, @@ -647,6 +710,7 @@ export const make = Effect.gen(function* () { const response = yield* rest( { host: locator.host, + repository: `${locator.owner}/${locator.name}`, operation: "getRepository", path: `repos/${encodeURIComponent(locator.owner)}/${encodeURIComponent(locator.name)}`, allowReserve: true, @@ -662,9 +726,11 @@ export const make = Effect.gen(function* () { const readViewerLogin = Effect.fn("GitHubSourceControlProvider.readViewerLogin")(function* ( host: string, + repository?: string, ) { const response = yield* rest({ host, + ...(repository === undefined ? {} : { repository }), operation: "getViewer", path: "user", allowReserve: true, @@ -844,10 +910,12 @@ export const make = Effect.gen(function* () { const readLinkSubject = Effect.fn("GitHubSourceControlProvider.readLinkSubject")(function* ( input: { readonly cwd: string; readonly url: URL }, endpoint: string, + repository: string, ) { const result = yield* api .rest({ host: input.url.host, + repository, operation: "resolveLink", path: endpoint, maxResponseBytes: 1_000_000, @@ -900,7 +968,11 @@ export const make = Effect.gen(function* () { input.url.pathname, ); if (!match) return undefined; - return readLinkSubject(input, `repos/${match[1]}/${match[2]}/issues/${match[3]}`); + return readLinkSubject( + input, + `repos/${match[1]}/${match[2]}/issues/${match[3]}`, + `${match[1]}/${match[2]}`, + ); }, listChangeRequests: (input) => (input.state === "open" ? Effect.succeed(true) : SourceControlRateLimit.Interactive).pipe( @@ -939,6 +1011,7 @@ export const make = Effect.gen(function* () { ); yield* rest({ host: locator.host, + repository: `${locator.owner}/${locator.name}`, operation: "createPullRequest", method: "POST", allowReserve: true, @@ -983,7 +1056,10 @@ export const make = Effect.gen(function* () { input.repository, (environment.GH_HOST ?? "github.com").toLowerCase(), ); - const viewer = locator === null ? null : yield* readViewerLogin(locator.host); + const viewer = + locator === null + ? null + : yield* readViewerLogin(locator.host, `${locator.owner}/${locator.name}`); const owner = locator?.owner ?? viewer; const name = locator?.name ?? input.repository.trim(); const host = locator?.host ?? (environment.GH_HOST?.trim().toLowerCase() || "github.com"); @@ -991,6 +1067,7 @@ export const make = Effect.gen(function* () { const response = yield* rest( { host, + ...(locator === null ? {} : { repository: `${locator.owner}/${locator.name}` }), operation: "createRepository", method: "POST", allowReserve: true, diff --git a/packages/source-control-github/src/server/gitHubConditionalChecks.ts b/packages/source-control-github/src/server/gitHubConditionalChecks.ts index ebef5ba5f7b1..0a9e536f0627 100644 --- a/packages/source-control-github/src/server/gitHubConditionalChecks.ts +++ b/packages/source-control-github/src/server/gitHubConditionalChecks.ts @@ -91,6 +91,7 @@ export const makeChecksRevalidator = Effect.gen(function* () { const response = yield* api .rest({ host: input.host, + repository: input.repository, operation: "revalidateChecks", path: endpoint, ...(previous?.etag ? { ifNoneMatch: previous.etag } : {}), diff --git a/packages/source-control-github/src/server/gitHubGraphQlClient.test.ts b/packages/source-control-github/src/server/gitHubGraphQlClient.test.ts index 785ae1ac98ba..43a5dc313fd6 100644 --- a/packages/source-control-github/src/server/gitHubGraphQlClient.test.ts +++ b/packages/source-control-github/src/server/gitHubGraphQlClient.test.ts @@ -16,17 +16,25 @@ import { PullRequestBatchesGraphQLGroup } from "./pullRequestBatches.graphql.ts" const NOW = Date.parse("2026-10-05T12:00:00.000Z"); -function harness(respond: (request: HttpClientRequest.HttpClientRequest) => Response) { +function harness( + respond: (request: HttpClientRequest.HttpClientRequest) => Response, + tokenForRepository?: (repository: string | undefined) => string, +) { + const selections: Array = []; const requests: Array = []; let invalidations = 0; const services = Layer.mergeAll( Layer.succeed(GitHubCredentials.GitHubCredentials, { - get: (host) => - Effect.succeed({ - host, - token: Redacted.make("secret"), - source: "gh" as const, - fingerprint: `${host}:secret`, + get: (host, repository) => + Effect.sync(() => { + selections.push(repository); + const token = tokenForRepository?.(repository) ?? "secret"; + return { + host, + token: Redacted.make(token), + source: "gh" as const, + fingerprint: `${host}:${token}`, + }; }), invalidate: () => Effect.sync(() => { @@ -44,7 +52,7 @@ function harness(respond: (request: HttpClientRequest.HttpClientRequest) => Resp SourceControlRateLimit.layer, ); const layer = GitHubRest.layer.pipe(Layer.provideMerge(services)); - return { layer, requests, invalidations: () => invalidations }; + return { layer, requests, selections, invalidations: () => invalidations }; } const json = (body: unknown, init?: ResponseInit) => @@ -54,7 +62,7 @@ const json = (body: unknown, init?: ResponseInit) => }); /** One stats lookup on `host`, through the client and middleware the pull request reads use. */ -const readStats = (host: string, number = 7) => +const readStats = (host: string, number = 7, repository?: string) => Effect.gen(function* () { const client = yield* GraphQLClient.make( PullRequestBatchesGraphQLGroup.middleware(GitHubGraphQlClient.GitHubGraphQlAuth), @@ -67,6 +75,7 @@ const readStats = (host: string, number = 7) => ).pipe( Effect.provideService(GitHubGraphQlClient.GitHubGraphQlTarget, { host, + repository, operation: "stats", }), ); @@ -75,6 +84,22 @@ const readStats = (host: string, number = 7) => const stats = { additions: 3, deletions: 1 }; describe("GitHubGraphQlAuth", () => { + it.effect("passes the explicit repository through the generated GraphQL request policy", () => { + const { layer, requests, selections } = harness( + () => json({ data: { _0_repository: { pullRequest: stats } } }), + (repository) => (repository === "acme/web" ? "repo-token" : "host-token"), + ); + return readStats("github.com", 7, "acme/web").pipe( + Effect.tap(() => + Effect.sync(() => { + expect(selections).toEqual(["acme/web"]); + expect(requests[0]?.headers.authorization).toBe("Bearer repo-token"); + }), + ), + Effect.provide(layer), + ); + }); + it.effect( "sends each host's lookups to its own endpoint with its token, never in one batch", () => { diff --git a/packages/source-control-github/src/server/gitHubGraphQlClient.ts b/packages/source-control-github/src/server/gitHubGraphQlClient.ts index f1f6ee8944e0..b702c1f51c21 100644 --- a/packages/source-control-github/src/server/gitHubGraphQlClient.ts +++ b/packages/source-control-github/src/server/gitHubGraphQlClient.ts @@ -22,6 +22,7 @@ export class GitHubGraphQlTarget extends Context.Service< GitHubGraphQlTarget, { readonly host: string; + readonly repository?: string | undefined; readonly operation: string; /** Overrides `SourceControlRateLimit.Interactive` for this one call. */ readonly allowReserve?: boolean | undefined; @@ -75,6 +76,7 @@ export class GitHubGraphQlAuth extends GraphQLMiddleware.Service< return yield* policy.request( { host, + repository: target.repository, operation: target.operation, resource: "graphql", allowReserve: target.allowReserve ?? (yield* SourceControlRateLimit.Interactive), @@ -225,6 +227,8 @@ export const batchKey = (context: Context.Context) => JSON.stringify([ Context.getOrElse(context, GitHubRequestPolicy.PinnedGitHubCredential, () => null) ?.credentialFingerprint ?? null, + Context.getOrElse(context, GitHubRequestPolicy.PinnedGitHubCredential, () => null) + ?.repository ?? null, Context.getOrElse(context, SourceControlRateLimit.CredentialScope, () => ""), ]); diff --git a/packages/source-control-github/src/server/gitHubRepositoryResolution.test.ts b/packages/source-control-github/src/server/gitHubRepositoryResolution.test.ts index 7c6f754192bb..8c830a06afc9 100644 --- a/packages/source-control-github/src/server/gitHubRepositoryResolution.test.ts +++ b/packages/source-control-github/src/server/gitHubRepositoryResolution.test.ts @@ -2,6 +2,7 @@ import { assert, describe, it } from "@effect/vitest"; import { gitHubApiHostForRemote, + resolveGitHubRepository, selectGitHubBaseRepository, } from "./gitHubRepositoryResolution.ts"; @@ -84,3 +85,50 @@ describe("gitHubApiHostForRemote", () => { assert.strictEqual(gitHubApiHostForRemote("git@gitlab.com:a/b.git"), null); }); }); + +describe("repository credential selector spelling", () => { + it.each([ + "https://github.com/OwnerA/Repo.git", + "ssh://git@github.com/OwnerA/Repo.git", + "git@github.com:OwnerA/Repo.git", + "git@github-work:OwnerA/Repo.git", + "https://github.com/OwnerA/Repo.git/", + "ssh://git@github.com/OwnerA/Repo.git/", + "git@github.com:OwnerA/Repo.git/", + "git@github-work:OwnerA/Repo.git/", + ])("preserves %s only when requested", (remote) => { + const input = { + remotes: `origin\t${remote} (fetch)`, + resolved: "", + hostHint: undefined, + defaultHost: "github.com", + }; + assert.deepStrictEqual(resolveGitHubRepository(input).locator, { + host: "github.com", + owner: "ownera", + name: "repo", + }); + assert.deepStrictEqual( + resolveGitHubRepository({ ...input, preserveRepositoryCase: true }).locator, + { host: "github.com", owner: "OwnerA", name: "Repo" }, + ); + }); + + it("preserves an explicit gh-resolved selector independently of its checkout", () => { + const input = { + remotes: "origin\tgit@github.com:Other/Repo.git (fetch)", + resolved: "remote.origin.gh-resolved OwnerA/Repo", + hostHint: undefined, + defaultHost: "github.com", + }; + assert.deepStrictEqual(resolveGitHubRepository(input).locator, { + host: "github.com", + owner: "ownera", + name: "repo", + }); + assert.deepStrictEqual( + resolveGitHubRepository({ ...input, preserveRepositoryCase: true }).locator, + { host: "github.com", owner: "OwnerA", name: "Repo" }, + ); + }); +}); diff --git a/packages/source-control-github/src/server/gitHubRepositoryResolution.ts b/packages/source-control-github/src/server/gitHubRepositoryResolution.ts index bd36758605e1..a69f008b7f26 100644 --- a/packages/source-control-github/src/server/gitHubRepositoryResolution.ts +++ b/packages/source-control-github/src/server/gitHubRepositoryResolution.ts @@ -32,13 +32,21 @@ export function selectGitHubBaseRepository(input: { readonly remotes: string; readonly resolved: string; readonly host: string; + readonly preserveRepositoryCase?: boolean; }): { readonly owner: string; readonly name: string } | null { const host = input.host.toLowerCase(); const repositories = new Map(); for (const remote of parseFetchRemotes(input.remotes)) { const [remoteHost, owner, name, ...rest] = normalizeGitRemoteUrl(remote.url).split("/"); if (remoteHost !== host || !owner || !name || rest.length > 0) return null; - repositories.set(remote.name, { owner, name }); + const original = + input.preserveRepositoryCase && host === "github.com" + ? parseGitHubRemoteRepositorySelector(remote.url, host) + : null; + repositories.set(remote.name, { + owner: original?.owner ?? owner, + name: original?.name ?? name, + }); } const marks = input.resolved .split("\n") @@ -48,7 +56,9 @@ export function selectGitHubBaseRepository(input: { const [mark] = marks; if (mark) { if (mark[2] === "base") return repositories.get(mark[1]!) ?? null; - const [owner, name, ...rest] = mark[2]!.toLowerCase().split("/"); + const [owner, name, ...rest] = ( + input.preserveRepositoryCase && host === "github.com" ? mark[2]! : mark[2]!.toLowerCase() + ).split("/"); return owner && name && rest.length === 0 ? { owner, name } : null; } // gh sorts remotes by these names, case-insensitively, and takes the first. A tie for the @@ -73,6 +83,7 @@ export function resolveGitHubRepository(input: { readonly resolved: string; readonly hostHint: string | undefined; readonly defaultHost: string; + readonly preserveRepositoryCase?: boolean; }): { readonly host: string; readonly locator: GitHubRepositoryLocator | null } { const fetchRemotes = parseFetchRemotes(input.remotes).map((remote) => ({ ...remote, @@ -87,6 +98,9 @@ export function resolveGitHubRepository(input: { remotes: input.remotes, resolved: input.resolved, host, + ...(input.preserveRepositoryCase === undefined + ? {} + : { preserveRepositoryCase: input.preserveRepositoryCase }), }); if (selected !== null) return { host, locator: { host, ...selected } }; // gh's own order without its prompt: upstream, github, origin, then the first remote, among @@ -103,7 +117,16 @@ export function resolveGitHubRepository(input: { }); for (const remote of candidates) { const [owner, name, ...rest] = normalizeGitRemoteUrl(remote.url).split("/").slice(1); - if (owner && name && rest.length === 0) return { host, locator: { host, owner, name } }; + if (owner && name && rest.length === 0) { + const original = + input.preserveRepositoryCase && host === "github.com" + ? parseGitHubRemoteRepositorySelector(remote.url, host) + : null; + return { + host, + locator: { host, owner: original?.owner ?? owner, name: original?.name ?? name }, + }; + } } return { host, locator: null }; } @@ -131,6 +154,22 @@ export function parseGitHubRepositorySelector( return null; } +/** Keep credential path matching case-sensitive while the chosen API host stays canonical. */ +export function parseGitHubRemoteRepositorySelector( + remoteUrl: string, + host: string, +): GitHubRepositoryLocator | null { + const remote = remoteUrl.trim().replace(/\/+$/g, ""); + const url = URL.parse(remote); + const selector = + url !== null + ? `https://${host}${url.pathname}` + : isSshRemoteUrl(remote) + ? remote.slice(remote.indexOf(":") + 1) + : remote; + return parseGitHubRepositorySelector(selector, host); +} + export function parsePullRequestReference( reference: string, ): diff --git a/packages/source-control-github/src/server/githubStackActions.ts b/packages/source-control-github/src/server/githubStackActions.ts index 1f2e52b4ff43..79527e636647 100644 --- a/packages/source-control-github/src/server/githubStackActions.ts +++ b/packages/source-control-github/src/server/githubStackActions.ts @@ -129,6 +129,7 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* const endpoint = `repos/${input.repository}`; const read = yield* api.rest({ host: input.host, + repository: input.repository, operation: "runGitHubStackAction", path: `${endpoint}/stacks?pull_request=${input.number}`, }); @@ -183,6 +184,7 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* ).pipe( GitHubGraphQlClient.call({ host: input.host, + repository: input.repository, operation: "runGitHubStackAction", allowReserve: true, }), @@ -237,6 +239,7 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* ); const request = yield* api.rest({ host: input.host, + repository: input.repository, operation: "runGitHubStackAction", method: "PUT", path: `${endpoint}/pulls/${input.number}/merge-async`, @@ -258,6 +261,7 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* yield* Effect.sleep(Math.min(1_000 * 2 ** attempt, 10_000)); const poll = yield* api.rest({ host: input.host, + repository: input.repository, operation: "runGitHubStackAction", path: `${endpoint}/pulls/${input.number}/merge-async/${encodeURIComponent(uuid)}`, }); diff --git a/packages/source-control-github/src/server/githubStackRebase.ts b/packages/source-control-github/src/server/githubStackRebase.ts index 2b854dec12e4..a097bd14bb06 100644 --- a/packages/source-control-github/src/server/githubStackRebase.ts +++ b/packages/source-control-github/src/server/githubStackRebase.ts @@ -45,7 +45,7 @@ export const cascadeRebaseStack = Effect.fn("cascadeRebaseStack")(function* (inp const api = yield* GitHubRest.GitHubRest; const { process } = yield* SourceControlHost.SourceControlHost; const fileSystem = yield* FileSystem.FileSystem; - const { token } = yield* api.credential(input.host); + const { token } = yield* api.credential(input.host, input.repository); const first = input.layers[0]; if (first === undefined) return 0; const directory = yield* fileSystem.makeTempDirectoryScoped({ prefix: "t3-stack-rebase-" }).pipe( From dda54a01e173fa3def5131f3186c06f7694a67c6 Mon Sep 17 00:00:00 2001 From: Chinoman10 <8300763+Chinoman10@users.noreply.github.com> Date: Sun, 18 Oct 2026 21:39:39 +0200 Subject: [PATCH 2/5] fix(github): preserve repository viewers and LFS credential scope Carry repository-specific viewers through the existing server result and web/mobile filters, preserving host and environment fallback. Keep Authored, grouping, me filters and mobile local search correct when organisations use different accounts. Parse the media-host path prefix before selecting a Git credential and test same-repository, rejected redirects and malformed paths. Independent review passes all 26 frozen source hashes and 429 focused tests, with one existing Bun GC exclusion. Server/web types and focused lint/formatting pass; full mobile typechecking remains blocked by absent Expo dependencies. --- .../pull-requests/pullRequestList.test.ts | 123 ++++++++++++++++++ .../features/pull-requests/pullRequestList.ts | 2 +- .../src/assets/GitHubMediaFetch.test.ts | 114 ++++++++++++++++ apps/server/src/assets/GitHubMediaFetch.ts | 4 + .../pullRequest/PullRequestService.test.ts | 67 ++++++++++ .../src/pullRequest/PullRequestService.ts | 6 +- .../pullRequest/pullRequestList.logic.test.ts | 112 ++++++++++++++++ .../pullRequest/pullRequestList.logic.ts | 9 +- packages/contracts/src/pullRequest.ts | 3 +- 9 files changed, 436 insertions(+), 4 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts index af0d64c08268..ebbe280557d4 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.test.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.test.ts @@ -240,6 +240,103 @@ describe("all-environment PR list", () => { [1, mbp], ]); }); + + it("keeps repository viewers through head pages and filters each environment before deduplication", () => { + const rows = [ + entry(1, { + repository: "OwnerA/Repo", + author: { login: "Alice", name: null, avatarUrl: null }, + }), + entry(2, { + repository: "OwnerB/Repo", + author: { login: "Bob", name: null, avatarUrl: null }, + }), + ]; + const merged = mergeEnvironmentPullRequestPages([ + { + environmentId: cups, + pages: pullRequestListHead(null, { + ...page(rows), + viewers: { + "github.com": "Alice", + "github.com ownera/repo": "Other", + "github.com ownerb/repo": "Other", + }, + }), + }, + { + environmentId: mbp, + pages: pullRequestListHead(null, { + ...page(rows), + viewers: { + "github.com": "Other", + "github.com ownera/repo": "Alice", + "github.com ownerb/repo": "Bob", + }, + }), + }, + ]); + expect(merged.viewers[`${mbp} github.com ownerb/repo`]).toBe("Bob"); + for (const [controls, query] of [ + [{ ...DEFAULT_PR_LIST_CONTROLS, involvement: "authored" as const }, ""], + [{ ...DEFAULT_PR_LIST_CONTROLS, filters: { author: "me" } }, ""], + [DEFAULT_PR_LIST_CONTROLS, "author:@me"], + ] as const) { + expect( + filterPullRequestList( + merged.entries, + controls, + merged.viewers, + query, + merged.localSearchHosts, + ).map((row) => [row.environmentId, row.number]), + ).toEqual([ + [mbp, 2], + [mbp, 1], + ]); + } + }); + + it("keeps local full-text searches keyed by host with repository viewers", () => { + const rows = [ + entry(1, { title: "Needle", repository: "OwnerA/Repo" }), + entry(2, { title: "Other", repository: "OwnerB/Repo" }), + ]; + const merged = mergeEnvironmentPullRequestPages([ + { + environmentId: cups, + pages: pullRequestListHead(null, { + ...page(rows), + viewers: { + "github.com": "Other", + "github.com ownera/repo": "octocat", + "github.com ownerb/repo": "octocat", + }, + providers: [ + { + host: "github.com", + kind: SourceControlProviderKind.make("github"), + searchesOnHost: false, + projectCount: 2, + configured: true, + detail: null, + }, + ], + }), + }, + ]); + expect([...merged.localSearchHosts]).toEqual([`${cups} github.com`]); + expect( + filterPullRequestList( + merged.entries, + { ...DEFAULT_PR_LIST_CONTROLS, involvement: "authored" }, + merged.viewers, + "needle", + merged.localSearchHosts, + ).map((row) => row.number), + ).toEqual([1]); + }); + it("scopes a project to its owning environment even if another has the same project id", () => { const merged = mergeEnvironmentPullRequestPages([ { environmentId: cups, pages: pullRequestListHead(null, page([entry(1)])) }, @@ -299,6 +396,32 @@ describe("mobile PR scan and filters", () => { ), ).toEqual([1]); }); + + it("prefers repository viewers for unscoped entries and retains the host fallback", () => { + const rows = [ + entry(1, { repository: "OwnerA/Repo" }), + entry(2, { + repository: "OwnerB/Repo", + author: { login: "Bob", name: null, avatarUrl: null }, + }), + entry(3, { repository: "legacy/repo" }), + ]; + const viewers = { + "github.com": "octocat", + "github.com ownera/repo": "octocat", + "github.com ownerb/repo": "Bob", + }; + expect( + filterPullRequestList( + rows, + { ...DEFAULT_PR_LIST_CONTROLS, involvement: "authored" }, + viewers, + "", + new Set(), + ).map((row) => row.number), + ).toEqual([3, 2, 1]); + }); + it("combines label groups, exclusions, draft and review filters", () => { const rows = [ entry(1, { diff --git a/apps/mobile/src/features/pull-requests/pullRequestList.ts b/apps/mobile/src/features/pull-requests/pullRequestList.ts index 976d62e10689..0608c574a564 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestList.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestList.ts @@ -215,7 +215,7 @@ export function filterPullRequestList< if (controls.projectEnvironmentId && entry.environmentId !== controls.projectEnvironmentId) return false; const viewerKey = entry.environmentId ? `${entry.environmentId} ${entry.host}` : entry.host; - const viewer = viewers[viewerKey]; + const viewer = viewers[`${viewerKey} ${entry.repository.toLowerCase()}`] ?? viewers[viewerKey]; if (controls.involvement === "reviewing" && !entry.viewerReviewRequested) return false; if ( controls.involvement === "authored" && diff --git a/apps/server/src/assets/GitHubMediaFetch.test.ts b/apps/server/src/assets/GitHubMediaFetch.test.ts index c4be249fa476..5ecd273ed21b 100644 --- a/apps/server/src/assets/GitHubMediaFetch.test.ts +++ b/apps/server/src/assets/GitHubMediaFetch.test.ts @@ -14,6 +14,120 @@ const asset = (url: string) => ({ }); describe("repository GitHub media credentials", () => { + it.effect.each([ + { fallback: false, enabled: true, unsupportedPrefix: false }, + { fallback: true, enabled: true, unsupportedPrefix: false }, + { fallback: true, enabled: false, unsupportedPrefix: false }, + { fallback: true, enabled: true, unsupportedPrefix: true }, + ])( + "uses LFS repository context through redirects: fallback $fallback, opt-in $enabled, unsupported prefix $unsupportedPrefix", + ({ fallback, enabled, unsupportedPrefix }) => { + const scopes: Array = []; + const authorizations: Array = []; + const redirects = [ + "https://raw.githubusercontent.com/OwnerA/Repo/main/image.png", + "https://media.githubusercontent.com/media/ownera/repo/main/image.png", + unsupportedPrefix + ? "https://media.githubusercontent.com/OwnerA/Repo/main/image.png" + : "https://media.githubusercontent.com/media/OwnerB/Repo/main/image.png", + ]; + return Effect.gen(function* () { + const response = yield* githubMediaResponse( + asset("https://media.githubusercontent.com/media/OwnerA/Repo/main/image.png"), + {}, + ); + expect(response.status).toBe(200); + expect(scopes).toEqual(["OwnerA/Repo"]); + expect(authorizations).toEqual([ + "Bearer fake-owner-a", + "Bearer fake-owner-a", + "Bearer fake-owner-a", + enabled ? undefined : "Bearer fake-owner-a", + ]); + }).pipe( + Effect.provide( + Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: (host, repository) => { + scopes.push(repository); + return Effect.succeed({ + host, + ...(fallback ? {} : { repository: "ownera/repo" }), + source: fallback ? "settings" : "git", + fingerprint: "fake-a", + token: Redacted.make("fake-owner-a"), + }); + }, + }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => { + const location = redirects[authorizations.length]; + authorizations.push(request.headers.authorization); + return Effect.succeed( + HttpClientResponse.fromWeb( + request, + new Response(null, { + status: location === undefined ? 200 : 302, + headers: location === undefined ? { "content-type": "image/png" } : { location }, + }), + ), + ); + }), + ), + Effect.provideService(HostProcess.Environment, { + T3CODE_GITHUB_USE_GIT_CREDENTIALS: enabled ? "1" : "0", + }), + Effect.scoped, + ); + }, + ); + + it.effect.each([ + { path: "raw.githubusercontent.com/media/Repo/main/image.png", repository: "media/Repo" }, + { path: "media.githubusercontent.com/OwnerA/Repo/main/image.png", repository: undefined }, + { path: "media.githubusercontent.com/Media/OwnerA/Repo/main/image.png", repository: undefined }, + { path: "media.githubusercontent.com/media/OwnerA/Repo", repository: undefined }, + { path: "media.githubusercontent.com/media//Repo/main/image.png", repository: undefined }, + { + path: "media.githubusercontent.com/media/Owner%2FA/Repo/main/image.png", + repository: undefined, + }, + { + path: "media.githubusercontent.com/media/OwnerA/-Repo/main/image.png", + repository: undefined, + }, + ])("requires the LFS prefix only on its own host: $path", ({ path, repository }) => + Effect.gen(function* () { + yield* githubMediaResponse(asset(`https://${path}`), {}); + }).pipe( + Effect.provide( + Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: (host, selected) => { + expect(selected).toBe(repository); + return Effect.succeed({ + host, + source: "settings", + fingerprint: "default", + token: Redacted.make("fake-default"), + }); + }, + }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => { + expect(request.headers.authorization).toBe("Bearer fake-default"); + return Effect.succeed( + HttpClientResponse.fromWeb(request, new Response(null, { status: 404 })), + ); + }), + ), + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + Effect.scoped, + ), + ); + it.effect.each([true, false])( "keeps legacy fallback redirect behavior only with opt-in disabled: %s", (enabled) => { diff --git a/apps/server/src/assets/GitHubMediaFetch.ts b/apps/server/src/assets/GitHubMediaFetch.ts index 434b173ef27e..0bf18b707d1d 100644 --- a/apps/server/src/assets/GitHubMediaFetch.ts +++ b/apps/server/src/assets/GitHubMediaFetch.ts @@ -54,6 +54,10 @@ const SVG_CONTENT_SECURITY_POLICY = "default-src 'none'; style-src 'unsafe-inlin const repositoryOf = (source: string) => { const url = new URL(source); const parts = url.pathname.split("/"); + if (url.hostname.toLowerCase() === "media.githubusercontent.com") { + if (parts[1] !== "media") return null; + parts.splice(1, 1); + } const repository = `${parts[1]}/${parts[2]}`; return isCredentialedHost(source) && parts.length > 3 && diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index e4caab438466..52ac91a14224 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1665,11 +1665,78 @@ it.effect("keeps opted-in repository viewers and list searches separate for one ["OwnerB/Repo"], ); assert.strictEqual(next.errors.length, 1); + assert.deepStrictEqual(next.viewers, { + "github.com": "Chinoman10", + "github.com ownerb/repo": "Chinoman10", + }); }).pipe( Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), ), ); +it.effect.each([true, false])( + "returns repository viewer keys only with Git credentials enabled: %s", + (enabled) => + Effect.gen(function* () { + let hostLookups = 0; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "A", workspaceRoot: "/a", repository: "OwnerA/Repo" }), + project({ id: "p2", title: "B", workspaceRoot: "/b", repository: "OwnerB/Repo" }), + ], + providers: [ + fakeProvider(SourceControlProviderKind.make("github"), { + getViewer: () => + Effect.sync(() => { + hostLookups++; + return "host-default"; + }), + withVerifiedCredential: (input, use) => + use({ + accountId: input.repository === "OwnerA/Repo" ? "1" : "2", + viewer: input.repository === "OwnerA/Repo" ? "Alice" : "Bob", + credentialFingerprint: `pat:${input.repository}`, + }), + listChangeRequests: (input) => + Effect.sync(() => { + assert.strictEqual( + input.viewer, + enabled ? (input.repository === "OwnerA/Repo" ? "Alice" : "Bob") : "host-default", + ); + return { + items: [ + { + ...changeRequest(1, "2026-07-02T00:00:00Z"), + author: { login: input.viewer, name: null, avatarUrl: null }, + }, + ], + truncated: false, + continues: true, + }; + }), + }), + ], + }); + const result = yield* service.list({ state: "open", involvement: "authored" }); + assert.strictEqual(result.entries.length, 2); + assert.deepStrictEqual( + result.viewers, + enabled + ? { + "github.com": "Alice", + "github.com ownera/repo": "Alice", + "github.com ownerb/repo": "Bob", + } + : { "github.com": "host-default" }, + ); + assert.strictEqual(hostLookups, enabled ? 0 : 1); + }).pipe( + Effect.provideService(HostProcess.Environment, { + T3CODE_GITHUB_USE_GIT_CREDENTIALS: enabled ? "1" : "0", + }), + ), +); + it.effect("namespaces ordinary detail caches by repository PAT without a routing RPC", () => Effect.gen(function* () { for (const operation of ["summary", "detail"] as const) { diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index d979cb27bea7..10d74ff48ced 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -1187,7 +1187,11 @@ export const make = Effect.gen(function* () { const viewerResults = yield* resolveViewers(projects, viewerRoots); const viewers: Record = {}; for (const result of viewerResults) { - if (result.viewer !== null) viewers[result.host] = result.viewer; + if (result.viewer === null) continue; + if (result.repository !== undefined) + viewers[listCursorKey(result.host, result.repository)] = result.viewer; + if (result.repository === undefined || viewers[result.host] === undefined) + viewers[result.host] = result.viewer; } const providers: ReadonlyArray = [ diff --git a/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts index 172e64062f89..1208a8b77385 100644 --- a/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestList.logic.test.ts @@ -15,6 +15,7 @@ import { findScopedProject, mergePullRequestLists, pullRequestEntryKey, + pullRequestEntryViewer, pullRequestEnvironmentSetKey, groupPullRequestsByInvolvement, matchesPullRequestFilters, @@ -1317,6 +1318,117 @@ describe('who "I" am, per server', () => { ).toEqual([1]); }); + it("keeps each repository's viewer through environment merging, grouping, filters and snapshots", () => { + const rows = [ + entry({ + number: 1, + repository: "OwnerA/Repo", + author: { login: "Alice", name: null, avatarUrl: null }, + }), + entry({ + number: 2, + repository: "OwnerB/Repo", + author: { login: "Bob", name: null, avatarUrl: null }, + }), + ]; + const merged = mergePullRequestLists([ + [ + ENV_1, + answer( + { + "github.com": "Alice", + "github.com ownera/repo": "Alice", + "github.com ownerb/repo": "Bob", + }, + rows, + ), + ], + [ + ENV_2, + answer( + { + "github.com": "Alice", + "github.com ownera/repo": "Carol", + "github.com ownerb/repo": "Dave", + }, + rows, + ), + ], + ])!; + const storage = new Map(); + const snapshotStorage = { + getItem: (key: string) => storage.get(key) ?? null, + setItem: (key: string, value: string) => { + storage.set(key, value); + }, + }; + writePullRequestListSnapshot(snapshotStorage, "both", { scope: "both", data: merged }); + const restored = readPullRequestListSnapshot(snapshotStorage, "both")!.data; + expect(restored.viewers).toEqual(merged.viewers); + expect( + filterPullRequestsByInvolvement(restored.entries, restored.viewers, "authored").map((row) => [ + row.environmentId, + row.number, + ]), + ).toEqual([ + [ENV_1, 1], + [ENV_1, 2], + ]); + expect( + groupPullRequestsByInvolvement(restored.entries, restored.viewers) + .find((group) => group.key === "authored") + ?.entries.map((row) => [row.environmentId, row.number]), + ).toEqual([ + [ENV_1, 1], + [ENV_1, 2], + ]); + for (const author of ["me", "@me"]) { + expect( + restored.entries + .filter((row) => + matchesPullRequestFilters( + row, + { author }, + pullRequestEntryViewer(row, restored.viewers), + ), + ) + .map((row) => [row.environmentId, row.number]), + ).toEqual([ + [ENV_1, 1], + [ENV_1, 2], + ]); + } + }); + + it.each([ + [ + { + [`${ENV_1} github.com ownera/repo`]: "Scoped", + [`${ENV_1} github.com`]: "Environment", + "github.com ownera/repo": "Repository", + "github.com": "Host", + }, + "scoped", + ], + [ + { + [`${ENV_1} github.com`]: "Environment", + "github.com ownera/repo": "Repository", + "github.com": "Host", + }, + "environment", + ], + [{ "github.com ownera/repo": "Repository", "github.com": "Host" }, "repository"], + [{ "github.com": "Host" }, "host"], + ] as const)("prefers the most specific available viewer: %s", (viewers, expected) => { + expect( + pullRequestEntryViewer( + { ...entry({ number: 1, repository: "OwnerA/Repo" }), environmentId: ENV_1 }, + viewers, + ), + ).toBe(expected); + }); + it("names the servers with more rows and no cursor to reach them by", () => { const merged = mergePullRequestLists([ [ diff --git a/apps/web/src/components/pullRequest/pullRequestList.logic.ts b/apps/web/src/components/pullRequest/pullRequestList.logic.ts index c64254d58f05..d2f9b0c9eec0 100644 --- a/apps/web/src/components/pullRequest/pullRequestList.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestList.logic.ts @@ -128,7 +128,14 @@ export function pullRequestEntryViewer( entry: ScopedEntry, viewers: PullRequestViewers, ): string | null { - return normalize(viewers[pullRequestViewerKey(entry)] ?? viewers[entry.host]); + const key = pullRequestViewerKey(entry); + const repository = entry.repository.toLowerCase(); + return normalize( + viewers[`${key} ${repository}`] ?? + viewers[key] ?? + viewers[`${entry.host} ${repository}`] ?? + viewers[entry.host], + ); } function isAuthoredByViewer(entry: ScopedEntry, viewers: PullRequestViewers): boolean { diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 2415c59f056e..f4a23ac38093 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -385,7 +385,8 @@ export type PullRequestListProjectError = typeof PullRequestListProjectError.Typ export const PullRequestListResult = Schema.Struct({ /** * A host that could not be read is absent rather than present-and-undefined, because an - * open-keyed record cannot carry an optional value through the JSON codec. + * open-keyed record cannot carry an optional value through the JSON codec. Repository-scoped + * logins also use `host owner/repo` keys, with the repository lowercased. */ viewers: Schema.Record(TrimmedNonEmptyString, TrimmedNonEmptyString), providers: Schema.Array(PullRequestProviderSummary), From c6a818ef98686436448cf6a163b00c7219500a46 Mon Sep 17 00:00:00 2001 From: Chinoman10 <8300763+Chinoman10@users.noreply.github.com> Date: Sun, 18 Oct 2026 21:53:44 +0200 Subject: [PATCH 3/5] fix(github): isolate credential cache selectors and recognize Git misses Key Git credentials by the exact validated helper path and remove mutable spelling state. Keep canonical repository provenance, and invalidate every cached spelling through the native cache API, including pending entries and clone-suffix inputs. Recognize only complete normal missing-credential stderr variants across Git versions; preserve sanitized failures for diagnostics, partial output and malformed responses. Cover expiry, concurrent invalidation, HTTP 401/403, case-sensitive real Git stores and fallback behavior. Focused and independent checks pass 98 tests and GitHub types; lint, formatting and frozen source verification pass. --- .../src/server/GitHubCredentials.ts | 34 +- .../src/server/GitHubGitCredentials.test.ts | 325 +++++++++++++++++- 2 files changed, 339 insertions(+), 20 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubCredentials.ts b/packages/source-control-github/src/server/GitHubCredentials.ts index 48873314b3f5..d1eb3d4ce251 100644 --- a/packages/source-control-github/src/server/GitHubCredentials.ts +++ b/packages/source-control-github/src/server/GitHubCredentials.ts @@ -163,10 +163,9 @@ export const make = Effect.gen(function* () { const crypto = yield* Crypto.Crypto; const environment = yield* HostProcess.Environment; const workingDirectory = yield* HostProcess.WorkingDirectory; - const gitPaths = new Map(); - const fromGit = Effect.fnUntraced(function* (repository: string) { const host = "github.com"; + const normalized = repository.toLowerCase(); const invalid = () => new GitHubGitCredentialError({ host, reason: "response" }); const output = yield* process .run({ @@ -176,7 +175,7 @@ export const make = Effect.gen(function* () { // Ignore any repository around the server working directory, including inherited Git // selectors. Helpers come only from the server's system/global configuration. cwd: workingDirectory, - stdin: `protocol=https\nhost=github.com\npath=${gitPaths.get(repository) ?? repository}.git\n\n`, + stdin: `protocol=https\nhost=github.com\npath=${repository}.git\n\n`, env: { GIT_DIR: "", GIT_WORK_TREE: "", @@ -215,9 +214,10 @@ export const make = Effect.gen(function* () { if (output.exitCode !== 0) { // With prompts disabled, Git's specific missing-credential exit is a normal miss. Any // helper error before it makes this an operational failure, rather than a fallback. - const missing = - /^fatal: could not read (?:Username|Password) for '[^'\n]*': terminal prompts disabled\r?\n?$/; - if (output.stdout === "" && missing.test(output.stderr)) return null; + const missing = output.stderr.match( + /^(?:fatal: could not read (?:Username|Password) for '[^'\r\n]*': terminal prompts disabled|fatal: unable to get password from user)(?:\r?\n)?$/, + ); + if (output.stdout === "" && missing?.[0] === output.stderr) return null; return yield* new GitHubGitCredentialError({ host, reason: "process" }); } if (output.stderr !== "") @@ -242,7 +242,7 @@ export const make = Effect.gen(function* () { if ( fields.get("protocol") !== "https" || fields.get("host") !== host || - (path !== undefined && normalizeCredentialRepository(path) !== repository) + (path !== undefined && normalizeCredentialRepository(path) !== normalized) ) return yield* invalid(); const token = fields.get("password"); @@ -250,7 +250,7 @@ export const make = Effect.gen(function* () { if (!/^[\x21-\x7e]+$/.test(token)) return yield* invalid(); return { host, - repository, + repository: normalized, source: "git", token: Redacted.make(token), fingerprint: yield* fingerprintOf(host, token), @@ -387,12 +387,9 @@ export const make = Effect.gen(function* () { host === "github.com" && repository !== undefined ) { - const normalized = normalizeCredentialRepository(repository); - if (normalized === null) + if (normalizeCredentialRepository(repository) === null) return yield* new GitHubGitCredentialError({ host, reason: "repository" }); - if (gitPaths.size >= 128) gitPaths.delete(gitPaths.keys().next().value!); - gitPaths.set(normalized, repository.replace(/\.git$/i, "")); - const credential = yield* Cache.get(gitCache, normalized); + const credential = yield* Cache.get(gitCache, repository.replace(/\.git$/i, "")); if (credential !== null) return credential; } // A token saved in Settings is the most deliberate choice, so it comes before the @@ -417,7 +414,16 @@ export const make = Effect.gen(function* () { normalized !== null && environment.T3CODE_GITHUB_USE_GIT_CREDENTIALS === "1" ) - return Cache.invalidate(gitCache, normalized); + return Effect.gen(function* () { + const keys = Array.from(yield* Cache.keys(gitCache)); + // Accept canonical helper provenance and selectors carrying a clone .git suffix. + const literal = repository?.toLowerCase(); + yield* Effect.forEach( + keys.filter((key) => key.toLowerCase() === literal || key.toLowerCase() === normalized), + (key) => Cache.invalidate(gitCache, key), + { discard: true }, + ); + }); return hostChoice(host).pipe( Effect.flatMap((choice) => Cache.invalidate(cache, cacheKey(host, choice?.account))), ); diff --git a/packages/source-control-github/src/server/GitHubGitCredentials.test.ts b/packages/source-control-github/src/server/GitHubGitCredentials.test.ts index 2ed293b3d2df..28d56fe21e0a 100644 --- a/packages/source-control-github/src/server/GitHubGitCredentials.test.ts +++ b/packages/source-control-github/src/server/GitHubGitCredentials.test.ts @@ -44,6 +44,7 @@ function harness( options: { enabled?: boolean; disabledHost?: boolean; + fallback?: "settings" | "env" | "gh"; cwd?: string; run?: SourceControlHost.SourceControlHost["Service"]["process"]["run"]; } = {}, @@ -67,7 +68,10 @@ function harness( settings: applyServerSettingsPatch(DEFAULT_SERVER_SETTINGS, { sourceControlHosts: { github: { - tokens: { "github.com": "settings-token" }, + tokens: + options.fallback === undefined || options.fallback === "settings" + ? { "github.com": "settings-token" } + : {}, hosts: { "github.com": { enabled: !options.disabledHost }, }, @@ -80,7 +84,7 @@ function harness( Layer.provide( Layer.succeed(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: options.enabled === false ? "0" : "1", - GH_TOKEN: "env-token", + GH_TOKEN: options.fallback === "gh" ? "" : "env-token", }), ), Layer.provide(Layer.succeed(HostProcess.WorkingDirectory, options.cwd ?? "/trusted-server")), @@ -180,6 +184,10 @@ describe("repository Git credentials", () => { expect(Redacted.value((yield* credentials.get("github.com", "OwnerB/Repo")).token)).toBe( "fake-OwnerB", ); + expect((yield* credentials.get("github.com", "ownera/repo")).source).toBe("settings"); + expect(Redacted.value((yield* credentials.get("github.com", "OwnerA/Repo")).token)).toBe( + "fake-OwnerA", + ); }).pipe(Effect.provide(layer)); const authorizations: Array = []; const http = Layer.succeed( @@ -291,7 +299,7 @@ describe("repository Git credentials", () => { const paths = calls .filter((input) => input.operation === "GitHubCredentials.git") .map((input) => /path=(.*)\n/.exec(input.stdin ?? "")?.[1]); - expect(paths.slice(2)).toEqual(Array(24).fill("OwnerA/Repo.git")); + expect(paths.slice(3)).toEqual(Array(24).fill("OwnerA/Repo.git")); }).pipe( Effect.provide(transport), Effect.provideService(HostProcess.Environment, { @@ -324,17 +332,25 @@ describe("repository Git credentials", () => { expect(first.repository).toBe("ownera/repo"); expect(Redacted.value(first.token)).toBe("fake-1"); expect(Redacted.value(second.token)).toBe("fake-2"); + const lowercase = yield* credentials.get("github.com", "ownera/repo"); + expect(lowercase.fingerprint).not.toBe(first.fingerprint); + expect((yield* credentials.get("github.com", "OwnerA/Repo")).fingerprint).toBe( + first.fingerprint, + ); expect((yield* credentials.get("github.com", "ownera/repo")).fingerprint).toBe( + lowercase.fingerprint, + ); + yield* credentials.invalidate("github.com", "ownera/repo"); + expect((yield* credentials.get("github.com", "OwnerA/Repo")).fingerprint).not.toBe( first.fingerprint, ); - yield* credentials.invalidate("github.com", "OwnerA/Repo"); expect((yield* credentials.get("github.com", "ownera/repo")).fingerprint).not.toBe( - first.fingerprint, + lowercase.fingerprint, ); expect((yield* credentials.get("github.com", "OwnerB/Repo")).fingerprint).toBe( second.fingerprint, ); - expect(calls).toHaveLength(3); + expect(calls).toHaveLength(5); expect(calls[0]?.stdin).toBe("protocol=https\nhost=github.com\npath=OwnerA/Repo.git\n\n"); expect(calls[0]?.cwd).toBe("/trusted-server"); expect(calls[0]?.env).toMatchObject({ @@ -365,6 +381,198 @@ describe("repository Git credentials", () => { }).pipe(Effect.provide(layer)); }); + it.effect("preserves each selector's spelling through repeated cache expiry", () => { + let generation = 1; + const { layer, calls } = harness({ + run: (input) => { + const path = /path=(.*)\n/.exec(input.stdin ?? "")?.[1] ?? ""; + return Effect.succeed(output(response(path, `fake-${path}-${generation}`))); + }, + }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + for (; generation <= 3; generation++) { + for (const selector of ["OwnerA/Repo", "ownera/repo"]) { + const credential = yield* credentials.get("github.com", selector); + expect(credential.repository).toBe("ownera/repo"); + expect(Redacted.value(credential.token)).toBe(`fake-${selector}.git-${generation}`); + expect((yield* credentials.get("github.com", `${selector}.git`)).fingerprint).toBe( + credential.fingerprint, + ); + } + yield* TestClock.adjust("5 minutes"); + } + expect(calls.map((input) => /path=(.*)\n/.exec(input.stdin ?? "")?.[1])).toEqual( + Array.from({ length: 3 }, () => ["OwnerA/Repo.git", "ownera/repo.git"]).flat(), + ); + }).pipe(Effect.provide(layer)); + }); + + it.effect("strips one transport suffix while preserving a repository's .git ending", () => { + const { layer, calls } = harness(); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const credential = yield* credentials.get("github.com", "Owner/Repo.git.git"); + const unrelated = yield* credentials.get("github.com", "OwnerB/Repo"); + expect(credential.repository).toBe("owner/repo.git"); + expect(calls[0]?.stdin).toBe("protocol=https\nhost=github.com\npath=Owner/Repo.git.git\n\n"); + expect((yield* credentials.get("github.com", "Owner/Repo.git.git")).fingerprint).toBe( + credential.fingerprint, + ); + yield* credentials.invalidate("github.com", credential.repository); + const refreshed = yield* credentials.get("github.com", "Owner/Repo.git.git"); + expect(refreshed.fingerprint).not.toBe(credential.fingerprint); + yield* credentials.invalidate("github.com", "Owner/Repo.git.git"); + expect((yield* credentials.get("github.com", "Owner/Repo.git.git")).fingerprint).not.toBe( + refreshed.fingerprint, + ); + const ordinary = yield* credentials.get("github.com", "Owner/Repo"); + yield* credentials.invalidate("github.com", "Owner/Repo.git"); + expect((yield* credentials.get("github.com", "Owner/Repo")).fingerprint).not.toBe( + ordinary.fingerprint, + ); + expect((yield* credentials.get("github.com", "OwnerB/Repo")).fingerprint).toBe( + unrelated.fingerprint, + ); + expect(calls).toHaveLength(6); + }).pipe(Effect.provide(layer)); + }); + + it.effect("invalidates pending spelling variants without reviving their completed entries", () => + Effect.gen(function* () { + const startedUpper = yield* Deferred.make(); + const startedLower = yield* Deferred.make(); + const release = yield* Deferred.make(); + let pending = true; + const { layer, calls } = harness({ + run: (input) => + Effect.gen(function* () { + const path = /path=(.*)\n/.exec(input.stdin ?? "")?.[1] ?? ""; + const hold = pending && path.toLowerCase() === "ownera/repo.git"; + if (hold) { + // Cache.get installs its pending entry after launching the lookup fiber. + yield* Effect.yieldNow; + yield* Deferred.succeed( + path.startsWith("OwnerA/") ? startedUpper : startedLower, + undefined, + ); + yield* Deferred.await(release); + } + return output(response(path, hold ? "fake-before" : "fake-after")); + }), + }); + yield* Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const unrelated = yield* credentials.get("github.com", "OwnerB/Repo"); + const waiting = yield* Effect.all( + ["OwnerA/Repo", "ownera/repo"].map((repository) => + credentials.get("github.com", repository), + ), + { concurrency: "unbounded" }, + ).pipe(Effect.forkChild); + yield* Deferred.await(startedUpper); + yield* Deferred.await(startedLower); + yield* credentials.invalidate("github.com", "ownera/repo"); + pending = false; + yield* Deferred.succeed(release, undefined); + expect( + (yield* Fiber.join(waiting)).map((credential) => Redacted.value(credential.token)), + ).toEqual(["fake-before", "fake-before"]); + for (const repository of ["OwnerA/Repo", "ownera/repo"]) + expect(Redacted.value((yield* credentials.get("github.com", repository)).token)).toBe( + "fake-after", + ); + expect((yield* credentials.get("github.com", "OwnerB/Repo")).fingerprint).toBe( + unrelated.fingerprint, + ); + expect(calls).toHaveLength(5); + }).pipe(Effect.provide(layer)); + }), + ); + + it.effect.each([401, 403])( + "evicts all spelling variants after canonical HTTP %s refusal", + (status) => { + let generation = 1; + let requests = 0; + const { layer, calls } = harness({ + run: (input) => { + const path = /path=(.*)\n/.exec(input.stdin ?? "")?.[1] ?? ""; + return Effect.succeed(output(response(path, `fake-${path}-${generation}`))); + }, + }); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => { + requests++; + return Effect.succeed( + HttpClientResponse.fromWeb(request, new Response(null, { status })), + ); + }), + ); + const transport = GitHubRest.layer.pipe( + Layer.provideMerge( + Layer.mergeAll(layer, http, GitHubQuota.layer, SourceControlRateLimit.layer), + ), + ); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const api = yield* GitHubRest.GitHubRest; + for (const repository of ["OwnerA/Repo", "ownera/repo", "OwnerB/Repo"]) + yield* credentials.get("github.com", repository); + const error = yield* api + .rest({ + host: "github.com", + repository: "OwnerA/Repo", + operation: "test", + path: "repos/OwnerA/Repo", + }) + .pipe(Effect.flip); + expect(error._tag).toBe( + status === 401 ? "GitHubApiAuthenticationError" : "GitHubApiResponseError", + ); + generation++; + for (const repository of ["OwnerA/Repo", "ownera/repo"]) + expect(Redacted.value((yield* credentials.get("github.com", repository)).token)).toBe( + `fake-${repository}.git-2`, + ); + expect(Redacted.value((yield* credentials.get("github.com", "OwnerB/Repo")).token)).toBe( + "fake-OwnerB/Repo.git-1", + ); + expect(calls).toHaveLength(5); + expect(requests).toBe(1); + }).pipe( + Effect.provide(transport), + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + ); + }, + ); + + it.effect("does not let a lowercase helper miss poison the original selector", () => { + const { layer, calls } = harness({ + run: (input) => + Effect.succeed( + input.stdin?.includes("path=OwnerA/Repo.git\n") + ? output(response("ownera/repo.git", "fake-owner-a")) + : output("", { + exitCode: ChildProcessSpawner.ExitCode(128), + stderr: "fatal: unable to get password from user\n", + }), + ), + }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + for (let round = 0; round < 3; round++) { + expect((yield* credentials.get("github.com", "ownera/repo")).source).toBe("settings"); + expect(Redacted.value((yield* credentials.get("github.com", "OwnerA/Repo")).token)).toBe( + "fake-owner-a", + ); + yield* TestClock.adjust("10 seconds"); + } + expect(calls).toHaveLength(4); + }).pipe(Effect.provide(layer)); + }); + it.effect( "uses the default credential without a repository and accepts path-free helper stores", () => { @@ -402,6 +610,111 @@ describe("repository Git credentials", () => { }).pipe(Effect.provide(layer)); }); + it.effect.each( + [ + "fatal: unable to get password from user", + "fatal: could not read Username for 'https://github.com/OwnerA/Repo.git': terminal prompts disabled", + "fatal: could not read Password for 'https://github.com/OwnerA/Repo.git': terminal prompts disabled", + ].flatMap((message) => + ["", "\n", "\r\n"].flatMap((ending) => + (["settings", "env", "gh"] as const).map((fallback) => ({ message, ending, fallback })), + ), + ), + )( + "accepts only an exact normal miss with $fallback fallback: $message, ending $ending", + ({ message, ending, fallback }) => { + const { layer, calls } = harness({ + fallback, + run: (input) => + Effect.succeed( + input.command === "gh" + ? output("fake-gh\n") + : output("", { + exitCode: ChildProcessSpawner.ExitCode(128), + stderr: message + ending, + }), + ), + }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const credential = yield* credentials.get("github.com", "OwnerA/Repo"); + expect(credential.source).toBe(fallback); + expect(Redacted.value(credential.token)).toBe( + fallback === "settings" ? "settings-token" : fallback === "env" ? "env-token" : "fake-gh", + ); + yield* credentials.get("github.com", "OwnerA/Repo"); + expect(calls.filter((input) => input.command === "git")).toHaveLength(1); + expect(calls.filter((input) => input.command === "gh")).toHaveLength( + fallback === "gh" ? 1 : 0, + ); + }).pipe(Effect.provide(layer)); + }, + ); + + it.effect.each([ + { + name: "prefixed helper diagnostic", + stderr: "fake-secret helper failed\nfatal: unable to get password from user\n", + }, + { + name: "suffixed helper diagnostic", + stderr: "fatal: unable to get password from user\nfake-secret helper failed\n", + }, + { name: "extra LF", stderr: "fatal: unable to get password from user\n\n" }, + { name: "extra CRLF", stderr: "fatal: unable to get password from user\r\n\r\n" }, + { name: "bare CR", stderr: "fatal: unable to get password from user\r" }, + { name: "trailing space", stderr: "fatal: unable to get password from user \n" }, + { name: "partial fatal", stderr: "fatal: unable to get password from us" }, + { + name: "old fatal with extra LF", + stderr: + "fatal: could not read Username for 'https://github.com': terminal prompts disabled\n\n", + }, + { + name: "old fatal with helper diagnostic", + stderr: + "fake-secret\nfatal: could not read Username for 'https://github.com': terminal prompts disabled\n", + }, + { + name: "partial stdout", + stdout: "protocol=https\nhost=github.com\n", + stderr: "fatal: unable to get password from user\n", + }, + { + name: "credential stdout", + stdout: response("OwnerA/Repo.git", "fake-secret"), + stderr: "fatal: unable to get password from user\n", + }, + { + name: "zero exit", + exitCode: ChildProcessSpawner.ExitCode(0), + stderr: "fatal: unable to get password from user\n", + }, + { + name: "truncated stderr", + stderrTruncated: true, + stderr: "fatal: unable to get password from user\n", + }, + { + name: "invalid UTF-8", + stderrInvalidUtf8: true, + stderr: "fatal: unable to get password from user\n", + }, + ])("does not fall back on a malformed missing-credential result: $name", (answer) => { + const { layer, calls } = harness({ + run: () => + Effect.succeed(output("", { exitCode: ChildProcessSpawner.ExitCode(128), ...answer })), + }); + return Effect.gen(function* () { + const credentials = yield* GitHubCredentials.GitHubCredentials; + const error = yield* credentials.get("github.com", "OwnerA/Repo").pipe(Effect.flip); + expect(error._tag).toBe("GitHubGitCredentialError"); + expect(JSON.stringify(error)).not.toContain("fake-secret"); + expect(calls).toHaveLength(1); + expect(calls[0]?.command).toBe("git"); + }).pipe(Effect.provide(layer)); + }); + it.effect("rejects unsafe owner/path selectors before invoking Git", () => { const { layer, calls } = harness(); return Effect.gen(function* () { From df30bbd79e223c83adaf24304e438020dfaf2bdb Mon Sep 17 00:00:00 2001 From: Chinoman10 <8300763+Chinoman10@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:27:03 +0200 Subject: [PATCH 4/5] fix(github): respect credential pauses during PR listing Let verified credential captures opt out of the interactive pause override. Apply that policy to scoped viewer discovery and both credential captures in the PR-list flow, including identity expiry between discovery and the read. Keep existing interactive and other helper defaults, cache reuse, credential fingerprint isolation, and pinned routing unchanged. Add real request-policy regressions for cold and expired identities, cached reuse, credential-specific pauses, explicit overrides, and the second-capture expiry race. Fresh independent review passed 371 tests (one unchanged Bun GC exclusion), three scoped typechecks, lint, formatting, and whitespace checks. --- .../pullRequest/PullRequestService.test.ts | 121 ++++++++++++++++++ .../src/pullRequest/PullRequestService.ts | 4 + .../src/server/PullRequestProvider.ts | 7 +- .../src/server/GitHubPullRequestApi.test.ts | 102 +++++++++++++++ .../src/server/GitHubPullRequestApi.ts | 10 +- 5 files changed, 241 insertions(+), 3 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 52ac91a14224..c498326f2a05 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1581,6 +1581,127 @@ it.effect( ), ); +it.effect("pauses repository viewer discovery while preserving interactive credential paths", () => + Effect.gen(function* () { + const limits = yield* SourceControlRateLimit.make; + const requests: string[] = []; + const pauseModes: Array = []; + let expireAfterViewer = false; + const key = { provider: SourceControlProviderKind.make("github"), host: "github.com" }; + const pause = Effect.gen(function* () { + const now = yield* Clock.currentTimeMillis; + const lease = yield* limits.check(key, { allowPaused: true }); + yield* limits.recordRateLimit({ ...key, lease, retryAt: now + 60 * 60_000 }); + }).pipe( + Effect.provideService(SourceControlRateLimit.CredentialScope, "pat:owner/repo"), + Effect.orDie, + ); + const credentials = Layer.succeed(GitHubCredentials.GitHubCredentials, { + get: (host) => + Effect.succeed({ + host, + token: Redacted.make("test-credential"), + source: "git" as const, + repository: "owner/repo", + credentialRepository: "Owner/Repo", + fingerprint: "pat:owner/repo", + }), + invalidate: () => Effect.void, + }); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.sync(() => { + requests.push(request.url); + return HttpClientResponse.fromWeb( + request, + new Response('{"id":123,"login":"octocat"}', { + headers: { "content-type": "application/json" }, + }), + ); + }), + ), + ); + const native = yield* GitHubPullRequestProvider.make.pipe( + Effect.provide( + GitHubPullRequestApi.layer.pipe( + Layer.provide( + GitHubRest.layer.pipe( + Layer.provideMerge(Layer.mergeAll(credentials, http)), + Layer.provideMerge(GitHubQuota.layer), + Layer.provideMerge( + Layer.succeed(SourceControlRateLimit.SourceControlRateLimit, limits), + ), + ), + ), + Layer.provide(TestSourceControlHost.layer()), + Layer.provide(NodeServices.layer), + ), + ), + ); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "A", workspaceRoot: "/a", repository: "Owner/Repo" })], + providers: [ + fakeProvider(SourceControlProviderKind.make("github"), { + capabilities: { ...native.capabilities, viewedFiles: "environment" }, + withVerifiedCredential: (input, use) => { + pauseModes.push(input.allowPaused); + return native.withVerifiedCredential!(input, (identity) => + use(identity).pipe( + Effect.tap(() => { + if (!expireAfterViewer) return Effect.void; + expireAfterViewer = false; + return TestClock.adjust("10 minutes").pipe(Effect.andThen(pause)); + }), + ), + ); + }, + listChangeRequests: () => + Effect.succeed({ + items: [changeRequest(1, "2026-07-02T00:00:00Z")], + truncated: false, + continues: true, + }), + getChangeRequest: () => Effect.succeed(hostedChangeRequest("interactive detail")), + }), + ], + }); + yield* pause; + + const cold = yield* service.list({ state: "open" }).pipe(Effect.flip); + assert.strictEqual(cold._tag, "PullRequestOperationError"); + assert.match(cold.message, /paused until the rate limit resets/u); + assert.deepStrictEqual(requests, []); + assert.deepStrictEqual(pauseModes, [false]); + const ref = { projectId: "p1" as ProjectId, repository: "Owner/Repo", number: 1 }; + assert.strictEqual((yield* service.detail(ref)).body, "interactive detail"); + assert.deepStrictEqual(pauseModes, [false, true]); + assert.deepStrictEqual(requests, ["https://api.github.com/user"]); + assert.strictEqual((yield* service.list({ state: "open" })).entries.length, 1); + assert.strictEqual(requests.length, 1); + + yield* TestClock.adjust("10 minutes"); + const expired = yield* service.list({ state: "open" }).pipe(Effect.flip); + assert.strictEqual(expired._tag, "PullRequestOperationError"); + assert.match(expired.message, /paused until the rate limit resets/u); + assert.strictEqual(requests.length, 1); + pauseModes.length = 0; + assert.deepStrictEqual(yield* service.filesViewed(ref), { files: [], truncated: false }); + assert.deepStrictEqual(pauseModes, [true, true]); + assert.strictEqual(requests.length, 2); + + pauseModes.length = 0; + expireAfterViewer = true; + const queued = yield* service.list({ state: "open" }); + assert.strictEqual(queued.entries.length, 0); + assert.strictEqual(queued.errors.length, 1); + assert.deepStrictEqual(pauseModes, [false, false]); + assert.strictEqual(requests.length, 2); + }).pipe( + Effect.provideService(HostProcess.Environment, { T3CODE_GITHUB_USE_GIT_CREDENTIALS: "1" }), + ), +); + it.effect.each([ { remoteUrl: "https://github.com/OwnerA/Repo.git", enabled: false }, { remoteUrl: "https://evil.example/OwnerA/Repo.git", enabled: true }, diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 10d74ff48ced..dced171193aa 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -616,6 +616,7 @@ export const make = Effect.gen(function* () { const withProjectCredential = ( project: SupportedProject, read: Effect.Effect, + allowPaused = true, ) => { if (!hasRepositoryCredential(project)) return read; return project.api.withVerifiedCredential!( @@ -623,6 +624,7 @@ export const make = Effect.gen(function* () { cwd: project.project.workspaceRoot, host: project.host, repository: providerRepository(project), + allowPaused, }, (identity) => read.pipe(Effect.provideService(routingCredential, identity), Effect.result), ).pipe(Effect.mapError(toPullRequestError("routeIdentity")), Effect.flatMap(Effect.fromResult)); @@ -1063,6 +1065,7 @@ export const make = Effect.gen(function* () { cwd: project.project.workspaceRoot, host: project.host, repository: providerRepository(project), + allowPaused: options?.allowPaused === true, }, (identity) => Effect.succeed({ @@ -1290,6 +1293,7 @@ export const make = Effect.gen(function* () { cursor: { updatedBefore: cursor.updatedBefore, delivered: cursor.delivered }, }), }), + false, ).pipe( observeRead, Effect.map(({ value: page, observedAt }): RepositoryBatch => { diff --git a/packages/source-control-core/src/server/PullRequestProvider.ts b/packages/source-control-core/src/server/PullRequestProvider.ts index 39a7d2bcae22..a19c511a7468 100644 --- a/packages/source-control-core/src/server/PullRequestProvider.ts +++ b/packages/source-control-core/src/server/PullRequestProvider.ts @@ -244,7 +244,12 @@ export interface ProviderRepositoryRef { export interface PullRequestProviderApi { readonly withVerifiedCredential?: ( - input: { readonly cwd: string; readonly host: string; readonly repository?: string }, + input: { + readonly cwd: string; + readonly host: string; + readonly repository?: string; + readonly allowPaused?: boolean; + }, use: (identity: { readonly accountId: string; readonly viewer: string; diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 705be8fb556b..48b0bcf4316c 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -10,6 +10,7 @@ import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; import * as TestClock from "effect/testing/TestClock"; import * as Redacted from "effect/Redacted"; +import { SourceControlProviderKind } from "@t3tools/contracts"; import { HttpClient, HttpClientResponse } from "effect/http"; @@ -703,6 +704,107 @@ it.effect( }), ); +it.effect.each([ + { expired: false, allowPaused: false }, + { expired: true, allowPaused: false }, + { expired: false, allowPaused: true }, + { expired: true, allowPaused: true }, + { expired: false, allowPaused: undefined }, + { expired: true, allowPaused: undefined }, +])( + "honors credential pauses for identity lookups: expired $expired, allowPaused $allowPaused", + ({ expired, allowPaused }) => + Effect.gen(function* () { + const limits = yield* SourceControlRateLimit.make; + const requests: string[] = []; + const credentials = Layer.succeed(GitHubCredentials.GitHubCredentials, { + get: (host, repository) => + Effect.succeed({ + host, + token: Redacted.make(`test-${repository}`), + source: "git" as const, + fingerprint: `pat:${repository}`, + }), + invalidate: () => Effect.void, + }); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.sync(() => { + requests.push(request.headers.authorization!); + return HttpClientResponse.fromWeb( + request, + new Response('{"id":123,"login":"same-account"}', { + headers: { "content-type": "application/json" }, + }), + ); + }), + ), + ); + const cli = yield* GitHubPullRequestApi.make.pipe( + Effect.provide( + GitHubRest.layer.pipe( + Layer.provideMerge(Layer.mergeAll(credentials, http)), + Layer.provideMerge(GitHubQuota.layer), + Layer.provideMerge( + Layer.succeed(SourceControlRateLimit.SourceControlRateLimit, limits), + ), + Layer.merge(TestSourceControlHost.layer()), + Layer.provideMerge(NodeServices.layer), + ), + ), + ); + const input = { cwd: "/repo", host: "github.com", repository: "owner-a/repo" }; + if (expired) { + yield* cli.withVerifiedCredential(input, Effect.succeed); + yield* TestClock.adjust("10 minutes"); + } + requests.length = 0; + const key = { provider: SourceControlProviderKind.make("github"), host: input.host }; + const now = yield* Clock.currentTimeMillis; + yield* Effect.gen(function* () { + const lease = yield* limits.check(key); + yield* limits.recordRateLimit({ ...key, lease, retryAt: now + 60 * 60_000 }); + }).pipe(Effect.provideService(SourceControlRateLimit.CredentialScope, "pat:owner-a/repo")); + + const result = yield* cli + .withVerifiedCredential( + { ...input, ...(allowPaused === undefined ? {} : { allowPaused }) }, + Effect.succeed, + ) + .pipe(Effect.result); + if (allowPaused === false) { + assert.ok(Result.isFailure(result)); + expect(result.failure._tag).toBe("SourceControlRateLimitPausedError"); + expect(requests).toEqual([]); + const other = yield* cli.withVerifiedCredential( + { ...input, repository: "owner-b/repo", allowPaused: false }, + Effect.succeed, + ); + expect(other.credentialFingerprint).toBe("pat:owner-b/repo"); + expect(requests).toEqual(["Bearer test-owner-b/repo"]); + } else { + assert.ok(Result.isSuccess(result)); + expect(result.success.viewer).toBe("same-account"); + expect(requests).toEqual(["Bearer test-owner-a/repo"]); + requests.length = 0; + const cached = yield* cli.withVerifiedCredential( + { ...input, allowPaused: false }, + Effect.succeed, + ); + expect(cached).toEqual(result.success); + expect(requests).toEqual([]); + const paused = yield* limits + .check(key) + .pipe( + Effect.provideService(SourceControlRateLimit.CredentialScope, "pat:owner-a/repo"), + Effect.flip, + ); + expect(paused._tag).toBe("SourceControlRateLimitPausedError"); + } + }), +); + layer("GitHubPullRequestApi.layer", (it) => { it.effect("holds background previews once an answer reports the reserve, until it resets", () => Effect.gen(function* () { diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index cbd1c2e6f8a8..bd7645ed7d48 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -397,7 +397,12 @@ export class GitHubPullRequestApi extends Context.Service< GitHubPullRequestApi, { readonly withVerifiedCredential: ( - input: { readonly cwd: string; readonly host: string; readonly repository?: string }, + input: { + readonly cwd: string; + readonly host: string; + readonly repository?: string; + readonly allowPaused?: boolean; + }, use: (identity: { readonly accountId: string; readonly viewer: string; @@ -927,6 +932,7 @@ export const make = Effect.gen(function* () { readonly cwd: string; readonly host: string; readonly repository?: string; + readonly allowPaused?: boolean; }) { const unavailable = () => new GitHubViewerLoginUnavailableError({ cwd: input.cwd }); const host = input.host.toLowerCase(); @@ -967,7 +973,7 @@ export const make = Effect.gen(function* () { ...(input.repository === undefined ? {} : { repository: input.repository }), operation: "getRoutingIdentity", path: "user", - allowReserve: true, + allowReserve: input.allowPaused !== false, }) .pipe( Effect.provideService(GitHubRequestPolicy.PinnedGitHubCredential, credential), From 268faa80dcd2770221d76405098c496e0adf90d7 Mon Sep 17 00:00:00 2001 From: Chinoman10 <8300763+Chinoman10@users.noreply.github.com> Date: Wed, 21 Oct 2026 08:16:51 +0200 Subject: [PATCH 5/5] fix(server): preserve mixed-case change breakdown selectors Pass the matching original remote selector through the existing providerRepository helper when loading PR change breakdowns. Extend ordinary and routed fingerprint-cache tests for mixed-case selectors, warm hits, credential rotation and switching back. Exercise cold/warm breakdown requests through the native credential and HTTP layers, retaining normalized provenance and the existing token pin. The native fixture already preserved the correct token before this change; the reproduced regression was the provider selector spelling. --- .../pullRequest/PullRequestService.test.ts | 91 ++++++++++++++++--- .../src/pullRequest/PullRequestService.ts | 2 +- 2 files changed, 78 insertions(+), 15 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 0bf123c4b7f0..f62a38855a7b 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -875,7 +875,14 @@ it.effect( let generation = 1; const helperPaths: string[] = []; const lookups: string[] = []; - const requests: Array<{ operation: string; authorization: string | undefined }> = []; + const requests: Array<{ + operation: string; + authorization: string | undefined; + repository: string | undefined; + credentialRepository: string | undefined; + fingerprint: string | undefined; + scope: string; + }> = []; const host = TestSourceControlHost.layer({ process: { run: (input) => @@ -899,25 +906,48 @@ it.effect( const http = Layer.succeed( HttpClient.HttpClient, HttpClient.make((request) => - Effect.sync(() => { + Effect.gen(function* () { const operation = request.url.endsWith("/user") ? "user" : request.body._tag === "Uint8Array" ? new TextDecoder().decode(request.body.body) : ""; - requests.push({ operation, authorization: request.headers.authorization }); + const pin = yield* GitHubRequestPolicy.PinnedGitHubCredential; + requests.push({ + operation, + authorization: request.headers.authorization, + repository: pin?.repository, + credentialRepository: pin?.credentialRepository, + fingerprint: pin?.credentialFingerprint, + scope: yield* SourceControlRateLimit.CredentialScope, + }); return HttpClientResponse.fromWeb( request, new Response( JSON.stringify( operation === "user" ? { id: 101, login: "Chinoman10" } - : { - data: { - repository: { pullRequest: { id: "PR_1" } }, - _0_markFileAsViewed: { clientMutationId: null }, + : operation.includes("PullRequestFileStats") + ? { + data: { + repository: { + pullRequest: { + files: { + nodes: [ + { path: "src/private.ts", additions: generation, deletions: 1 }, + ], + pageInfo: { hasNextPage: false, endCursor: null }, + }, + }, + }, + }, + } + : { + data: { + repository: { pullRequest: { id: "PR_1" } }, + _0_markFileAsViewed: { clientMutationId: null }, + }, }, - }, ), { headers: { "content-type": "application/json" } }, ), @@ -978,6 +1008,7 @@ it.effect( getReviewThreadComments: (input) => lookup(input).pipe(Effect.as({ comments: [], nextCursor: null })), setFilesViewed: native.setFilesViewed!, + getFileStats: native.getFileStats!, }); const service = yield* makeService({ projects: [ @@ -998,9 +1029,30 @@ it.effect( ["ownera/repo"], ); assert.strictEqual((yield* service.detail(ref)).body, "fake-OwnerA-1"); + const firstBreakdown = yield* service.changeBreakdown(ref); + assert.deepStrictEqual(firstBreakdown?.code, { additions: 1, deletions: 1 }); + assert.deepStrictEqual(yield* service.changeBreakdown(ref), firstBreakdown); yield* credentials.invalidate("github.com", "ownera/repo"); generation = 2; assert.strictEqual((yield* service.detail(ref)).body, "fake-OwnerA-2"); + const secondBreakdown = yield* service.changeBreakdown(ref); + assert.deepStrictEqual(secondBreakdown?.code, { additions: 2, deletions: 1 }); + assert.deepStrictEqual(yield* service.changeBreakdown(ref), secondBreakdown); + const fileStatsRequests = requests.filter((request) => + request.operation.includes("PullRequestFileStats"), + ); + assert.strictEqual(fileStatsRequests.length, 2); + assert.deepStrictEqual( + fileStatsRequests.map((request) => request.authorization), + ["Bearer fake-OwnerA-1", "Bearer fake-OwnerA-2"], + ); + assert.notStrictEqual(fileStatsRequests[0]?.fingerprint, fileStatsRequests[1]?.fingerprint); + for (const request of fileStatsRequests) { + assert.strictEqual(request.repository, "ownera/repo"); + assert.strictEqual(request.credentialRepository, "ownera/repo"); + assert.ok(request.fingerprint); + assert.strictEqual(request.scope, request.fingerprint); + } assert.strictEqual( (yield* service.listStats({ refs: [ref] })).stats[0]?.repository, "ownera/repo", @@ -1370,19 +1422,30 @@ it.effect.each([false, true])( const reads: string[] = []; const service = yield* makeService({ projects: [ - project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" }), + project({ + id: "p1", + title: "web", + workspaceRoot: "/a", + repository: "acme/web", + remoteUrl: "https://github.com/Acme/Web.git", + }), ], providers: [ fakeProvider(SourceControlProviderKind.make("github"), { - withVerifiedCredential: (_, use) => - Effect.suspend(() => - use({ accountId: "101", viewer: "octocat", credentialFingerprint: credential }), - ), + withVerifiedCredential: (input, use) => + Effect.suspend(() => { + assert.strictEqual(input.repository, "Acme/Web"); + return use({ + accountId: "101", + viewer: "octocat", + credentialFingerprint: credential, + }); + }), getFileStats: (input) => Effect.sync(() => { assert.deepStrictEqual(input, { cwd: "/a", - repository: "acme/web", + repository: "Acme/Web", host: "github.com", number: 1, }); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index e68a93b14a83..0a663b5f4e07 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -3111,7 +3111,7 @@ export const make = Effect.gen(function* () { if (read === undefined) return Effect.succeed(null); return read({ cwd: project.project.workspaceRoot, - repository: project.repository, + repository: providerRepository(project), host: project.host, number: ref.number, }).pipe(