diff --git a/doc/connections/GITHUB-REVIEW-BOT.md b/doc/connections/GITHUB-REVIEW-BOT.md index c3c2f036108..888a85fc35c 100644 --- a/doc/connections/GITHUB-REVIEW-BOT.md +++ b/doc/connections/GITHUB-REVIEW-BOT.md @@ -134,6 +134,13 @@ choose an active sponsor, and use the restricted guest profile. Guests receive n sponsor credentials. Authority is checked again before tool calls and publication, so revocation also affects queued or ongoing work. +The username lookup uses the bot's own GitHub App installation with a short-lived, +metadata-only token. It does not use a personal account or the shared unauthenticated +IP quota. An active connection with a verified installation is required. GitHub +supports installation tokens for [Get a user](https://docs.github.com/en/rest/users/users#get-a-user) +without additional permissions. Lookup errors distinguish unavailable accounts, +rate limits, and installation authentication or access failures. + Choose automatic events and author filters in **Settings** to control which authored PRs and issues start work. Authorized mentions remain available with automatic events off and bypass automatic author filters. The shared external- diff --git a/maps/agent-access-map/chat-channels.md b/maps/agent-access-map/chat-channels.md index fda30a0e5fb..ce1399bb7b4 100644 --- a/maps/agent-access-map/chat-channels.md +++ b/maps/agent-access-map/chat-channels.md @@ -170,6 +170,7 @@ Gaps: This individual operation has source review but no recorded HTTP verificat API: `present` — `POST /api/chat-endpoints/{endpointId}/github/people/lookup`; JSON REST. Agent access: `available` — agent REST and default native `search_api` / `call_api`. Policy: `tools:manage_connections`; [shared management policy](../../doc/plans/2026-10-13-connection-api-parity.md#authorization-contract). Resolved resources and every affected target are checked; personal ownership remains the responsible user. +Provider identity: the requested endpoint's active GitHub App installation issues a metadata-only token. The lookup does not use a personal grant or fall back to anonymous access. Inactive connections and unverified installation IDs are rejected. [Service regressions](../../server/src/services/chat-github-management.test.ts) cover authentication and sanitized lookup failures with provider fixtures; they do not prove the live HTTP journey. Evidence: `source-only` — [route](../../server/src/routes/chat-channels.ts); reviewed in the working tree based on the source commit above. Verification: `not-run` — individual operation not executed in this audit; shared authorization and representative family flows are exercised by the authenticated integration suite. Follow the route schema, inspect persistence/audit, and assert owner, scope and revoked-authority denial. Gaps: This individual operation has source review but no recorded HTTP verification. Provider availability and live setup remain unqualified. diff --git a/maps/feature-map/chat-channels.md b/maps/feature-map/chat-channels.md index d9d90e23599..41b6bb81dc2 100644 --- a/maps/feature-map/chat-channels.md +++ b/maps/feature-map/chat-channels.md @@ -33,6 +33,8 @@ Delegated setup can save ordinary bot tokens and API keys. Provider verification Open `/apps/chat/:endpointId/settings` and the endpoint’s other available tabs; identity confirmation uses `/chat-identity/confirm`. +For GitHub external contributors, open the connection's Access tab and look up a username. The lookup uses that endpoint's active GitHub App installation with a metadata-only token. Verify that a person or bot resolves to its numeric identity and that unavailable accounts, rate limits, and installation failures show distinct errors. [Lookup regressions](../../server/src/services/chat-github-management.test.ts) use provider fixtures; live installation lookup remains separate evidence. + For delegated Slack test messages and identity invitations, apply a Block or Ask first rule to the calling agent while allowing the endpoint's assigned agent. Verify no message is sent. Exercise caller, target and shared rate limits: one shared quota admits one send and the next distinct request is denied. For a saved GitHub identity handoff on an otherwise healthy endpoint, verify the card stays pending before identity confirmation and after an OAuth callback alone. Confirm the addressed user's identity through their human session, then verify continuation becomes available. Revoked, expired, wrong-user and wrong-installation links must keep the handoff blocked. These cases pass through the assembled API with real key/JWT/session middleware and deterministic providers; live Slack/GitHub consent remains separate evidence. ### `outbound-agentmail` diff --git a/server/src/routes/chat-channels.ts b/server/src/routes/chat-channels.ts index 7ab30abea8b..947e1a7ada1 100644 --- a/server/src/routes/chat-channels.ts +++ b/server/src/routes/chat-channels.ts @@ -346,7 +346,7 @@ export function chatChannelRoutes(db: Db, options: ChatChannelRouteOptions) { }); router.post("/chat-endpoints/:endpointId/github/people/lookup", managementGuard("connections:configure", "POST /chat-endpoints/:endpointId/github/people/lookup"), validate(z.object({ login: z.string().min(1).max(44) }).strict()), async (req, res) => { if (!(await assertEndpointManagementAccess(req, res))) return; - res.json(await github.lookupPerson(req.body.login)); + res.json(await github.lookupPerson(endpointId(req), req.body.login)); }); router.post("/chat-endpoints/:endpointId/github/registration", managementGuard("connections:configure", "POST /chat-endpoints/:endpointId/github/registration"), validate(z.object({ name: z.string().trim().min(1).max(34), ownerType: z.enum(["personal", "organization"]).optional(), ownerLogin: z.string().regex(/^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$/).optional() }).strict().refine(value => value.ownerType !== "organization" || !!value.ownerLogin, "Enter the GitHub organization")), async (req, res) => { if (!(await assertEndpointManagementAccess(req, res))) return; diff --git a/server/src/services/chat-github-management.test.ts b/server/src/services/chat-github-management.test.ts new file mode 100644 index 00000000000..4facd0df0f9 --- /dev/null +++ b/server/src/services/chat-github-management.test.ts @@ -0,0 +1,155 @@ +import type { Db } from "@paperclipai/db"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { githubBotCredentials } from "./chat-github-client.js"; +import { githubChatManagementService } from "./chat-github-management.js"; + +vi.mock("./chat-github-client.js", async (importOriginal) => ({ + ...await importOriginal(), + githubBotCredentials: vi.fn(), +})); +vi.mock("./secrets.js", () => ({ secretService: vi.fn() })); +vi.mock("./tool-access-policy.js", () => ({ toolAccessPolicyService: vi.fn() })); +vi.mock("./chat-github-tools.js", () => ({ GITHUB_BOT_TOOLS: [], syncGitHubBotTools: vi.fn() })); +vi.mock("./tool-access.js", () => ({ toolAccessService: vi.fn() })); +vi.mock("./activity-log.js", () => ({ logActivity: vi.fn() })); +vi.mock("./environments.js", () => ({ environmentService: vi.fn() })); +vi.mock("./instance-settings.js", () => ({ instanceSettingsService: vi.fn() })); +vi.mock("./execution-workspace-policy.js", () => ({ resolveExecutionWorkspaceEnvironmentId: vi.fn() })); + +const endpoint = { id: "bot-endpoint", companyId: "bot-company", status: "active" }; +const credentials = { + endpoint, + connection: { enabled: true, status: "active" }, + credentials: { installationId: "12345" }, + appJwt: "app-jwt-not-for-users", +}; + +function fixture(user: unknown = { id: 1, login: "octocat", type: "User" }) { + const where = vi.fn().mockResolvedValue([endpoint]); + const db = { select: vi.fn(() => ({ from: vi.fn(() => ({ where })) })) } as unknown as Db; + const fetchImpl = vi.fn() + .mockResolvedValueOnce(Response.json({ token: "installation-token-not-for-output" }, { status: 201 })) + .mockResolvedValueOnce(Response.json(user)); + const service = githubChatManagementService(db, fetchImpl); + return { db, where, fetchImpl, service }; +} + +describe("GitHub external contributor lookup", () => { + beforeEach(() => { + vi.mocked(githubBotCredentials).mockReset(); + vi.mocked(githubBotCredentials).mockResolvedValue(credentials as never); + }); + + it("uses only the requested endpoint's installation with metadata permission", async () => { + const { db, fetchImpl, service } = fixture(); + expect(await service.lookupPerson(endpoint.id, "octocat")).toEqual({ githubUserId: "1", login: "octocat" }); + expect(githubBotCredentials).toHaveBeenCalledExactlyOnceWith(db, endpoint.companyId, endpoint.id); + expect(fetchImpl).toHaveBeenCalledTimes(2); + expect(fetchImpl.mock.calls[0]).toEqual([ + "https://api.github.com/app/installations/12345/access_tokens", + expect.objectContaining({ + method: "POST", + redirect: "error", + headers: expect.objectContaining({ authorization: "Bearer app-jwt-not-for-users" }), + body: JSON.stringify({ permissions: { metadata: "read" } }), + }), + ]); + expect(fetchImpl.mock.calls[1]).toEqual([ + "https://api.github.com/users/octocat", + expect.objectContaining({ + method: "GET", + redirect: "error", + headers: expect.objectContaining({ authorization: "Bearer installation-token-not-for-output" }), + }), + ]); + }); + + it("accepts bot accounts and keeps the numeric identity", async () => { + const { fetchImpl, service } = fixture({ id: 9001, login: "reviewer[bot]", type: "Bot" }); + expect(await service.lookupPerson(endpoint.id, "reviewer[bot]")).toEqual({ githubUserId: "9001", login: "reviewer[bot]" }); + expect(fetchImpl.mock.calls[1][0]).toBe("https://api.github.com/users/reviewer%5Bbot%5D"); + }); + + it.each(["../someone", "https://github.com/someone", "", "-name"])("rejects invalid login %j before credentials or network access", async (login) => { + const { fetchImpl, service } = fixture(); + await expect(service.lookupPerson(endpoint.id, login)).rejects.toMatchObject({ status: 400 }); + expect(githubBotCredentials).not.toHaveBeenCalled(); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it("does not use another endpoint when the requested bot is absent", async () => { + const { fetchImpl, where, service } = fixture(); + where.mockResolvedValue([]); + await expect(service.lookupPerson(endpoint.id, "octocat")).rejects.toMatchObject({ status: 404 }); + expect(githubBotCredentials).not.toHaveBeenCalled(); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it.each([ + { connection: { enabled: false, status: "active" } }, + { connection: { enabled: true, status: "revoked" } }, + { endpoint: { ...endpoint, status: "draft" } }, + { endpoint: { ...endpoint, status: "paused" } }, + ])("rejects inactive installation state %j without anonymous fallback", async (state) => { + const { fetchImpl, service } = fixture(); + vi.mocked(githubBotCredentials).mockResolvedValue({ ...credentials, ...state } as never); + await expect(service.lookupPerson(endpoint.id, "octocat")).rejects.toMatchObject({ status: 409 }); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it.each(["", "not-an-installation", "123/../../users"])("requires a valid saved installation ID %j", async (installationId) => { + const { fetchImpl, service } = fixture(); + vi.mocked(githubBotCredentials).mockResolvedValue({ ...credentials, credentials: { installationId } } as never); + await expect(service.lookupPerson(endpoint.id, "octocat")).rejects.toMatchObject({ status: 409 }); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it("requires an issued token before it looks up a person", async () => { + const { fetchImpl, service } = fixture(); + fetchImpl.mockReset().mockResolvedValue(Response.json({})); + await expect(service.lookupPerson(endpoint.id, "octocat")).rejects.toMatchObject({ status: 422, message: expect.stringContaining("did not issue") }); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it.each([ + { id: 1, login: "org", type: "Organization" }, + { id: "1", login: "octocat", type: "User" }, + { id: 0, login: "octocat", type: "User" }, + { id: 1, type: "User" }, + ])("rejects accounts without a valid person identity: %j", async (user) => { + const { service } = fixture(user); + await expect(service.lookupPerson(endpoint.id, "octocat")).rejects.toMatchObject({ status: 400, message: "Choose a GitHub person or bot account" }); + }); + + it.each([403, 429])("reports HTTP %s rate limits and preserves retry metadata", async (status) => { + const { fetchImpl, service } = fixture(); + fetchImpl.mockReset() + .mockResolvedValueOnce(Response.json({ token: "installation-token-not-for-output" })) + .mockResolvedValueOnce(Response.json({ message: "secret-provider-response" }, { status, headers: { "retry-after": "60", "x-ratelimit-remaining": "0" } })); + await expect(service.lookupPerson(endpoint.id, "octocat")).rejects.toMatchObject({ + message: expect.stringContaining("rate limit"), + details: { code: "github_bot_rate_limited", providerStatus: status, retryable: true, retryAfterMs: 60000 }, + }); + expect(fetchImpl).toHaveBeenCalledTimes(2); + }); + + it("distinguishes an unavailable account from an invalid installation", async () => { + const { fetchImpl, service } = fixture(); + fetchImpl.mockReset() + .mockResolvedValueOnce(Response.json({ token: "installation-token-not-for-output" })) + .mockResolvedValueOnce(Response.json({ message: "secret-provider-response" }, { status: 404 })); + await expect(service.lookupPerson(endpoint.id, "octocat")).rejects.toMatchObject({ + status: 400, message: expect.stringContaining("Check the username"), details: { providerStatus: 404 }, + }); + }); + + it.each([401, 403, 404])("reports installation HTTP %s without a user lookup or provider response text", async (status) => { + const { fetchImpl, service } = fixture(); + fetchImpl.mockReset().mockResolvedValue(Response.json({ message: "secret-provider-response" }, { status })); + const error = await service.lookupPerson(endpoint.id, "octocat").catch((error: Error) => error); + expect(error).toMatchObject({ status: 422, message: expect.stringContaining("installation"), details: { providerStatus: status, retryable: false } }); + expect(JSON.stringify(error)).not.toContain("secret-provider-response"); + expect(JSON.stringify(error)).not.toContain("app-jwt-not-for-users"); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/src/services/chat-github-management.ts b/server/src/services/chat-github-management.ts index c4bf27e1330..9d30b793ab1 100644 --- a/server/src/services/chat-github-management.ts +++ b/server/src/services/chat-github-management.ts @@ -21,7 +21,7 @@ import { updateGitHubChatConfigurationSchema, type GitHubChatConfiguration, } from "@paperclipai/shared"; -import { badRequest, conflict, forbidden, notFound } from "../errors.js"; +import { HttpError, badRequest, conflict, forbidden, notFound, unprocessable } from "../errors.js"; import { toolAccessService } from "./tool-access.js"; import { logActivity } from "./activity-log.js"; import { githubBotRequest } from "./chat-github-client.js"; @@ -615,22 +615,57 @@ export function githubChatManagementService(db: Db, fetchImpl = fetch) { saveConfiguration, personalConnections, identity, - lookupPerson: async (login: string) => { + lookupPerson: async (endpointId: string, login: string) => { if (!/^[a-zA-Z0-9][a-zA-Z0-9-]{0,38}(?:\[bot\])?$/.test(login)) throw badRequest("Enter a GitHub username"); - const user = await githubBotRequest<{ - id?: number; - login?: string; - type?: string; - }>(fetchImpl, null, `/users/${encodeURIComponent(login)}`); + const bot = await endpoint(endpointId); + const material = await githubBotCredentials(db, bot.companyId, endpointId); if ( - !Number.isSafeInteger(user.id) || - !user.id || - !["User", "Bot"].includes(user.type ?? "") || - !user.login + !material.connection.enabled || + material.connection.status !== "active" || + !["active", "verifying"].includes(material.endpoint.status) ) - throw badRequest("Choose a GitHub person or bot account"); - return { githubUserId: String(user.id), login: user.login }; + throw conflict("GitHub bot connection is not active"); + const installationId = material.credentials.installationId; + if (!installationId || !/^[1-9][0-9]*$/.test(installationId)) + throw conflict("Verify the GitHub App installation before looking up people"); + let lookingUpPerson = false; + try { + const issued = await githubBotRequest<{ token?: string }>( + fetchImpl, + material.appJwt, + `/app/installations/${installationId}/access_tokens`, + { method: "POST", body: { permissions: { metadata: "read" } } }, + ); + if (!issued.token) + throw unprocessable("GitHub did not issue an installation token. Reconnect the bot and try again."); + lookingUpPerson = true; + const user = await githubBotRequest<{ + id?: number; + login?: string; + type?: string; + }>(fetchImpl, issued.token, `/users/${encodeURIComponent(login)}`); + if ( + !Number.isSafeInteger(user.id) || + !user.id || + !["User", "Bot"].includes(user.type ?? "") || + !user.login + ) + throw badRequest("Choose a GitHub person or bot account"); + return { githubUserId: String(user.id), login: user.login }; + } catch (error) { + if (!(error instanceof HttpError)) throw error; + const details = error.details as { code?: string; providerStatus?: number } | undefined; + if (details?.code === "github_bot_rate_limited") + throw unprocessable("GitHub's rate limit was reached. Wait and try the lookup again.", details); + if (details?.providerStatus === 404 && lookingUpPerson) + throw badRequest("GitHub could not find this account. Check the username and the bot's access to the account.", details); + if (details?.providerStatus === 401 || details?.providerStatus === 404) + throw unprocessable("GitHub could not authenticate this bot installation. Reconnect the bot and try again.", details); + if (details?.providerStatus === 403) + throw unprocessable("GitHub denied this bot installation access. Check its installation on GitHub and try again.", details); + throw error; + } }, review: async (endpointId: string, reviewId: string) => { const bot = await endpoint(endpointId);