From 9983f8c8951ffc14f2251133c1521cf5c8fc86fe Mon Sep 17 00:00:00 2001 From: "omegent-app[bot]" <306514130+omegent-app[bot]@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:50:36 +0000 Subject: [PATCH] feat(github): require identity-map membership for agent turns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When T3_IDENTITY_MAP_PATH is enabled, GitHub mention actors must resolve via github.id or github.login before a turn starts. Repo write access alone is no longer enough — including public repos and outside collaborators. Map off keeps the previous permission-floor-only gate. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com> --- apps/server/src/github/GitHubPrBridge.ts | 39 +++++++- .../src/github/githubActorTrust.test.ts | 90 +++++++++++++++++++ apps/server/src/github/githubActorTrust.ts | 80 +++++++++++++++++ docs/architecture/source-and-identity.md | 1 + docs/integrations/github-pr-conversations.md | 33 ++++--- 5 files changed, 228 insertions(+), 15 deletions(-) create mode 100644 apps/server/src/github/githubActorTrust.test.ts create mode 100644 apps/server/src/github/githubActorTrust.ts diff --git a/apps/server/src/github/GitHubPrBridge.ts b/apps/server/src/github/GitHubPrBridge.ts index 25802481bbc7..9a8f6db0e629 100644 --- a/apps/server/src/github/GitHubPrBridge.ts +++ b/apps/server/src/github/GitHubPrBridge.ts @@ -54,6 +54,7 @@ import { type GitHubPullRequestStackContext, stackBranchesForMatching, } from "./GitHubPullRequestStack.ts"; +import { classifyGitHubActorTrust } from "./githubActorTrust.ts"; const BUSY_RESPONSE = "This T3 thread is already working. Try again after the current turn finishes."; @@ -69,6 +70,8 @@ const PROVISION_FAILED_RESPONSE = "T3 could not open a thread for this pull request. Check the server logs for details."; const EMPTY_PROMPT_RESPONSE = "Provide a prompt after the mention. Conversation comments use the PR work thread; inline review reuses that discussion's session (first tag creates it). Override with `main-thread` or `sibling-thread`."; +const IDENTITY_DENIED_RESPONSE = + "Not authorized to run agent turns from this GitHub account. When the T3 identity map is enabled, only mapped operators (`github.login` / `github.id`) can invoke the bot — repository write access alone is not enough (including on public repos)."; const MAX_GITHUB_COMMENT_LENGTH = 65_536; const PERMISSION_RANK: Readonly> = { @@ -1215,6 +1218,38 @@ export const make = Effect.gen(function* () { return; } + // When the closed-set identity map is on, map membership is required in + // addition to the GitHub permission floor. Public-repo write / outside + // collaborators cannot drive the host unless listed. + const mapEnabled = yield* identity.isMapEnabled(); + const mapPeople = yield* identity.listMapPeople(); + const trust = classifyGitHubActorTrust({ + identityMapEnabled: mapEnabled, + actorId: input.invocation.actorId, + actorLogin: input.invocation.actorLogin, + people: mapPeople, + }); + if (trust.mode === "denied") { + yield* Effect.logWarning("Rejected GitHub PR invocation from unmapped identity", { + deliveryId: input.deliveryId, + repository: input.invocation.repository, + pullRequestNumber: input.invocation.pullRequestNumber, + actorId: input.invocation.actorId, + actorLogin: input.invocation.actorLogin, + reason: trust.reason, + }); + yield* finishDelivery(initial, IDENTITY_DENIED_RESPONSE, "rejected"); + return; + } + yield* Effect.logInfo("Classified GitHub actor trust", { + deliveryId: input.deliveryId, + actorLogin: input.invocation.actorLogin, + actorId: input.invocation.actorId, + mode: trust.mode, + reason: trust.reason, + personId: trust.person?.personId ?? null, + }); + const addAckReaction = input.invocation.commentSurface === "review" ? github.addReviewCommentReaction({ @@ -1360,10 +1395,10 @@ export const make = Effect.gen(function* () { const turnModelSelection = hasExplicitModelSelection ? yield* resolveGitHubModelSelection(turnInvocation, thread.modelSelection) : thread.modelSelection; - const mapPeople = yield* identity.listMapPeople(); + const sourcePeople = yield* identity.listMapPeople(); const [repoOwner, repoName] = turnInvocation.repository.split("/"); const source = buildIntegrationSourceRef({ - people: mapPeople, + people: sourcePeople, channel: "github", platformId: String(turnInvocation.actorId), displayName: turnInvocation.actorLogin, diff --git a/apps/server/src/github/githubActorTrust.test.ts b/apps/server/src/github/githubActorTrust.test.ts new file mode 100644 index 000000000000..0f6696609e15 --- /dev/null +++ b/apps/server/src/github/githubActorTrust.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from "@effect/vitest"; + +import { classifyGitHubActorTrust, resolvePersonByGitHubActor } from "./githubActorTrust.ts"; + +const people = [ + { + personId: "patroza", + username: "patroza", + name: "Patrick Roza", + github: { login: "patroza", id: "42661" }, + }, + { + personId: "julius", + username: "julius", + github: { login: "juliusmarminge" }, + }, +] as const; + +describe("resolvePersonByGitHubActor", () => { + it("prefers github id over login", () => { + const hit = resolvePersonByGitHubActor(people, { + actorId: 42661, + actorLogin: "someone-else", + }); + expect(hit?.person.username).toBe("patroza"); + expect(hit?.reason).toBe("mapped_github_id"); + }); + + it("falls back to login (case-insensitive)", () => { + const hit = resolvePersonByGitHubActor(people, { + actorId: 999, + actorLogin: "JuliusMarminge", + }); + expect(hit?.person.username).toBe("julius"); + expect(hit?.reason).toBe("mapped_github_login"); + }); + + it("returns null when unmapped", () => { + expect( + resolvePersonByGitHubActor(people, { actorId: 1, actorLogin: "random-user" }), + ).toBeNull(); + }); +}); + +describe("classifyGitHubActorTrust", () => { + it("allows full access when the identity map is disabled", () => { + expect( + classifyGitHubActorTrust({ + identityMapEnabled: false, + actorId: 1, + actorLogin: "stranger", + people: [], + }), + ).toEqual({ mode: "full", person: null, reason: "identity_map_disabled" }); + }); + + it("trusts mapped github accounts for full agent turns", () => { + const byId = classifyGitHubActorTrust({ + identityMapEnabled: true, + actorId: "42661", + actorLogin: "patroza", + people, + }); + expect(byId.mode).toBe("full"); + expect(byId.reason).toBe("mapped_github_id"); + expect(byId.person?.username).toBe("patroza"); + }); + + it("denies unmapped actors when the map is on (public write is not enough)", () => { + expect( + classifyGitHubActorTrust({ + identityMapEnabled: true, + actorId: 42, + actorLogin: "drive-by-collaborator", + people, + }), + ).toMatchObject({ mode: "denied", reason: "unmapped_github_actor", person: null }); + }); + + it("denies missing actor fields when the map is on", () => { + expect( + classifyGitHubActorTrust({ + identityMapEnabled: true, + actorId: null, + actorLogin: "", + people, + }), + ).toMatchObject({ mode: "denied", reason: "missing_github_actor" }); + }); +}); diff --git a/apps/server/src/github/githubActorTrust.ts b/apps/server/src/github/githubActorTrust.ts new file mode 100644 index 000000000000..9a2dd150ba18 --- /dev/null +++ b/apps/server/src/github/githubActorTrust.ts @@ -0,0 +1,80 @@ +/** + * GitHub actor trust relative to the closed-set identity map. + * + * When the map is off, collaborator permission alone gates agent turns + * (legacy behaviour). When the map is on, the actor must also resolve to a + * mapped person (github id or login) — so public-repo write access or + * outside collaborators cannot drive the host unless listed. + */ +import { + findPersonByGithubId, + findPersonByGithubLogin, + type IdentityMapPerson, +} from "@t3tools/shared/identityMap"; + +export type GitHubActorTrustMode = "full" | "denied"; + +export type GitHubActorTrustDecision = { + readonly mode: GitHubActorTrustMode; + readonly person: IdentityMapPerson | null; + readonly reason: + | "identity_map_disabled" + | "mapped_github_id" + | "mapped_github_login" + | "unmapped_github_actor" + | "missing_github_actor"; +}; + +export function resolvePersonByGitHubActor( + people: ReadonlyArray, + input: { + readonly actorId: number | string | null | undefined; + readonly actorLogin: string | null | undefined; + }, +): { + readonly person: IdentityMapPerson; + readonly reason: "mapped_github_id" | "mapped_github_login"; +} | null { + if (input.actorId !== null && input.actorId !== undefined && String(input.actorId).length > 0) { + const byId = findPersonByGithubId(people, input.actorId); + if (byId !== null) return { person: byId, reason: "mapped_github_id" }; + } + const login = input.actorLogin?.trim() ?? ""; + if (login.length > 0) { + const byLogin = findPersonByGithubLogin(people, login); + if (byLogin !== null) return { person: byLogin, reason: "mapped_github_login" }; + } + return null; +} + +/** + * Classify a GitHub mention actor for agent execution. + * + * - Map off → full (permission floor still enforced separately) + * - Map on + id/login in map → full + * - Map on + unmapped/missing → denied (no agent turn) + */ +export function classifyGitHubActorTrust(input: { + readonly identityMapEnabled: boolean; + readonly actorId: number | string | null | undefined; + readonly actorLogin: string | null | undefined; + readonly people: ReadonlyArray; +}): GitHubActorTrustDecision { + if (!input.identityMapEnabled) { + return { mode: "full", person: null, reason: "identity_map_disabled" }; + } + const login = input.actorLogin?.trim() ?? ""; + const hasId = + input.actorId !== null && input.actorId !== undefined && String(input.actorId).length > 0; + if (!hasId && login.length === 0) { + return { mode: "denied", person: null, reason: "missing_github_actor" }; + } + const hit = resolvePersonByGitHubActor(input.people, { + actorId: input.actorId, + actorLogin: input.actorLogin, + }); + if (hit === null) { + return { mode: "denied", person: null, reason: "unmapped_github_actor" }; + } + return { mode: "full", person: hit.person, reason: hit.reason }; +} diff --git a/docs/architecture/source-and-identity.md b/docs/architecture/source-and-identity.md index 474acf3239af..e89624df1f81 100644 --- a/docs/architecture/source-and-identity.md +++ b/docs/architecture/source-and-identity.md @@ -203,6 +203,7 @@ type SessionIdentityClaim = { | Web / desktop / mobile | After pairing: **typeahead claim** against map usernames; must claim before operate | | Discord bot | Auto-resolve sender snowflake → person; stamp on turn; no UI | | Jira bot | Auto-resolve accountId → person; **mapped = full agent turn**, unmapped = Discord context-only note when issue is Discord-linked (no agent) | +| GitHub bot | Auto-resolve login/id → person; **mapped = full agent turn** (+ GH permission floor); unmapped when map on = reject (no agent) | | Headless CLI / admin bootstrap | Optional claim; if identity-on and operate without claim → reject operate RPCs | ### Gate diff --git a/docs/integrations/github-pr-conversations.md b/docs/integrations/github-pr-conversations.md index 8a47575d2730..dae8123eba93 100644 --- a/docs/integrations/github-pr-conversations.md +++ b/docs/integrations/github-pr-conversations.md @@ -143,7 +143,13 @@ orchestration projection and command engine as the web application. `X-GitHub-Delivery` id. - Ignore bot actors and require an explicit configured mention. - Require the repository to be enabled and the actor to meet the configured permission floor - (`write` by default). + (`write` by default). Collaborator write alone is **not** enough when the identity map is on. +- **Identity map trust gate** (when `T3_IDENTITY_MAP_PATH` has people): + - **Trusted** — actor resolves via `github.id` or `github.login` on a map person → full agent turn + (still subject to repo allowlist + permission floor). + - **Untrusted** — map on but actor unmapped → reject with a short authorization reply; **no** agent + turn. Closes the public-repo hole where outside collaborators with write could drive the host. + - Map **off** / empty → legacy behaviour (permission floor only). - Treat all GitHub fields and comment text as untrusted user input in the generated T3 prompt. - Use a GitHub App installation token for permission checks and PR comment writes. - Keep the webhook secret and private key out of prompts, logs, persisted deliveries, and git config. @@ -197,18 +203,19 @@ The route returns 404 unless all four required variables are configured. ## Failure semantics -| Condition | Result | -| ---------------------------------------------- | -------------------------------------------------------- | -| No unique live PR/branch/worktree/thread match | Exactly `not yet linked/checked out.` | -| Missing/deleted worktree or T3 thread | Exactly `not yet linked/checked out.` | -| Repository or PR mismatch | Exactly `not yet linked/checked out.` | -| Unauthorized repository | Silently ignored; no response, no turn | -| Unauthorized actor | Neutral authorization response; no link-state disclosure | -| Thread already running | Busy response; no queue and no turn | -| Duplicate delivery | Reuse persisted classification; no new comment or turn | -| Turn completes | Replace working/progress comment with final answer | -| Turn errors or is interrupted | Replace comment with a stable failure response | -| Server restarts during turn | Resume the persisted response bridge | +| Condition | Result | +| ---------------------------------------------- | ------------------------------------------------------ | +| No unique live PR/branch/worktree/thread match | Exactly `not yet linked/checked out.` | +| Missing/deleted worktree or T3 thread | Exactly `not yet linked/checked out.` | +| Repository or PR mismatch | Exactly `not yet linked/checked out.` | +| Unauthorized repository | Silently ignored; no response, no turn | +| Unauthorized actor (GH permission) | Silently ignored; no response, no turn | +| Unmapped actor (identity map on) | Short authorization reply; no agent turn | +| Thread already running | Busy response; no queue and no turn | +| Duplicate delivery | Reuse persisted classification; no new comment or turn | +| Turn completes | Replace working/progress comment with final answer | +| Turn errors or is interrupted | Replace comment with a stable failure response | +| Server restarts during turn | Resume the persisted response bridge | ## Tests and acceptance criteria