mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 12:04:41 +01:00
fix(github): authenticate external contributor lookups (#17902)
## Thinking Path > - Paperclip manages AI agents and their access to external services. > - A GitHub bot connection can add external contributors by username. > - The lookup sent an anonymous request even when the bot had an App installation. > - Anonymous requests share the server IP quota and can fail for a valid username. > - This pull request uses the selected bot installation for the lookup. > - It also gives users clear errors without exposing credentials or provider response text. ## Linked Issues or Issue Description **What happened?** The Access tab could reject a valid external contributor username with a generic GitHub error. The lookup did not use the connected bot's identity. **Expected behavior** The lookup uses the selected bot's active App installation. It returns the numeric user identity or a clear account, rate limit, or installation error. **Steps to reproduce** 1. Configure a GitHub review bot and open its Access tab. 2. Exhaust the server IP's anonymous GitHub quota. 3. Look up a valid external contributor username. 4. Observe the generic rejection even though the bot has an installation. The original provider HTTP status was not retained. Source inspection proves the anonymous request. The historical cause remains unconfirmed. **Paperclip version or commit** The defect is present at `eda2a03bfe5213ffef2fbbdefdf572be775f1e14`. **Deployment mode** Server deployment with an active GitHub App bot connection. ## What Changed - Pass the endpoint ID from the existing authorized lookup route. - Issue a short-lived installation token with metadata read permission. - Reject inactive connections and invalid installation IDs without an anonymous fallback. - Distinguish missing accounts, rate limits, and installation access failures. - Add 25 lookup tests and update the connector guide and both access maps. The GitHub management service owns the provider behavior. The shared route only passes its authorized endpoint ID. The request body and response shape stay the same. ## Verification - Passed: [full CI](https://github.com/paperclipai/paperclip/actions/runs/40535979115) on `38d1ebcebb08553f159b3b81ee1bb31d1fbbba8e`, including typecheck, build, server and workspace tests, Runner checks, serialized suites, eight E2E shards, and the final verification gate. - Passed: Greptile 5/5 for the same head, with no open review threads. - Passed: 25 lookup regression tests on the final test fixtures. - Passed: 91 tests across five GitHub service suites before the fixture-only change. - Passed: `node scripts/check-maps.mjs` and `git diff --check`. - The tests exercise actual GitHub request construction with provider response fixtures. They assert the installation identity, metadata-only token request, request count, and sanitized error details. - `pnpm build` and `pnpm -r typecheck` could not complete because this environment has no Cargo executable. - Two standalone server TypeScript attempts were killed. The second ran after the full suite was stopped, with a 5120 MiB Node heap limit. The 8 GiB container recorded two OOM kills. Neither attempt produced a typecheck verdict. - `pnpm test:run` was started, then interrupted after resource exhaustion. Its first server phase did not produce a final verdict. These local attempts remain incomplete. Full CI later passed for the same head. A live authenticated bot lookup remains unverified. - The initial frozen install stopped on an existing patch configuration mismatch. Installation then succeeded with lockfile writes disabled. No dependency or lockfile changes are included. - GitHub documents installation-token support for [Get a user](https://docs.github.com/en/rest/users/users#get-a-user) without extra user permissions. ## Risks - The lookup now requires an active bot connection and a saved installation ID. - GitHub rate limits can still occur. The error keeps the retry metadata and tells the user to retry. - No database migration or permission-policy change is included. - This PR does not prove that a deployment contains the fix. ## Model Used OpenAI Codex, GPT-6 family, with code editing, tool use, and test execution. The recovered implementation was assisted by `gpt-6.1-sol`. This session does not expose its exact model ID or context window limit. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
ac22b1b1f9
commit
2f3516f6fd
6 files changed
+214
-14
No files matched your search
@@ -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-
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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`
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<typeof import("./chat-github-client.js")>(),
|
||||
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<typeof fetch>()
|
||||
.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);
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
|
||||
Reference in new issue
Block a user