From cfed739f811ed544388f40e86d6357fcdc92db94 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Mon, 26 Oct 2026 16:13:04 -0700 Subject: [PATCH] fix(external-objects): stop reporting GitHub rate limits as unreachable (#17792) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://entire.com/gh/paperclipai/-/trails/11/changes/paperclip/11 ## Thinking Path > - Paperclip is the open source app that people use to manage AI agents for work. > - Paperclip shows the status of external objects, such as GitHub pull requests and issues. > - A background sweep refreshes these objects and resolves a GitHub credential. > - The sweep read a credential only from a company secret with a well-known name. > - An instance without that secret made anonymous GitHub API calls. Anonymous calls are limited to 60 per hour. > - When the limit was reached, the sweep showed "unreachable" for objects that it could not refresh. > - A server environment token is instance-wide, so it must not expose a private repository that a company cannot see. > - This pull request reads a server token for public repositories, reports a rate limit as its own state, and shows a warning on the pill. > - The benefit is an accurate status, fewer misleading failures, and no cross-company private data. ## Linked Issues or Issue Description No public GitHub issue exists. The sections below describe the bug. **What happened?** The board showed "unreachable" for many GitHub links, even after a related fix was deployed. **Expected behavior** The board should show "unreachable" only for real network failures. It should show a rate limit as its own state and retry later. **Steps to reproduce** 1. Use an instance with no GitHub token company secret and no server token. 2. Let the external-object sweep make more than 60 anonymous GitHub requests in one hour. 3. Open a task with GitHub links. **Paperclip version or commit** master (commit a67a18c at report time). **Deployment mode** Self-hosted server. ## What Changed - Add `resolveGitHubApiCredential` in `server/src/services/git-credentials.ts`. It reads a company secret with a well-known name, then falls back to the server environment token (`GITHUB_TOKEN`/`GH_TOKEN`), and it reports the credential `source`. - Use the source in the GitHub external-object provider and in `github-commit-details.ts`. A `server_env` token is limited to public repositories. A private repository resolves only with a company credential; otherwise the object reports `auth_required` and does not fetch the private object. - Add a `rate_limited` liveness state in `packages/shared/src/constants.ts`. - Classify a 403 with `x-ratelimit-remaining: 0`, a 429 response, and a rate-limited repo probe as `rate_limited` instead of `unreachable`. - Show `rate_limited` in the UI filter, icon, label, tone, overlay, and badge maps. - Make `ExternalObjectPill` and `ExternalObjectStatusIcon` prefer a liveness warning icon and tone, so a rate-limited object shows a warning even when the last status was healthy. - Add tests for the credential source, the public-only server token, the company-scoped private repository, and the rate-limited pill tone. ## Verification - `vitest run server/src/__tests__/external-objects-service.test.ts server/src/__tests__/git-credentials.test.ts server/src/__tests__/github-commit-details.test.ts server/src/__tests__/work-products.test.ts` — pass, with the new scoping and credential-source tests. - `vitest run ui/src/lib/external-objects.test.ts ui/src/lib/issue-filters.test.ts ui/src/components/ExternalObjectPill.test.tsx ui/src/components/MarkdownExternalLink.test.tsx` — pass, with the new rate-limited pill test. - `pnpm --filter @paperclipai/shared typecheck` — pass. The full monorepo `tsc` (server and ui) is killed by the 4GB memory cap of this sandbox; the authoritative CI Typecheck gate runs it instead. - One test in `git-credentials.test.ts` fails in this sandbox only. The test "credential helper execution (real git, no network) ... env-carried token" reads a global GitHub credential helper in the sandbox. The test fails the same way on the base without this change. ## Risks - Low risk. The change adds a liveness state and a credential source. It does not change existing terminal verdicts. - A server-environment token cannot resolve a private repository. This is intentional. An operator who needs private repositories must add a GitHub token as a company secret. The object then reports `auth_required` with that instruction. - `rate_limited` is a new value in `EXTERNAL_OBJECT_LIVENESS_STATES`. An old client ignores an unknown value. ## Model Used - Provider: DeepSeek (OpenRouter). - Exact model ID: `openrouter/deepseek/deepseek-v4.1-flash`. - Capabilities: tool use and code execution in the Paperclip runner. ## 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 similar open and closed PRs and confirmed this is not a duplicate - [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) - [ ] 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 - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> --- packages/shared/src/constants.ts | 1 + .../external-objects-service.test.ts | 182 ++++++++++++++++- server/src/__tests__/git-credentials.test.ts | 27 +++ server/src/services/external-objects.ts | 4 +- server/src/services/git-credentials.ts | 48 +++++ server/src/services/github-commit-details.ts | 66 ++++-- .../github-external-object-provider.ts | 191 +++++++++++++----- ui/src/components/ExternalObjectPill.test.tsx | 18 ++ ui/src/components/ExternalObjectPill.tsx | 6 +- .../components/ExternalObjectStatusIcon.tsx | 14 +- ui/src/components/IssueFiltersPopover.tsx | 1 + ui/src/lib/external-objects.test.ts | 10 + ui/src/lib/external-objects.ts | 5 + ui/src/lib/issue-filters.ts | 4 + ui/src/lib/status-colors.ts | 3 + 15 files changed, 502 insertions(+), 78 deletions(-) diff --git a/packages/shared/src/constants.ts b/packages/shared/src/constants.ts index 675c8cede30..0a3d0d21981 100644 --- a/packages/shared/src/constants.ts +++ b/packages/shared/src/constants.ts @@ -497,6 +497,7 @@ export const EXTERNAL_OBJECT_LIVENESS_STATES = [ "fresh", "stale", "auth_required", + "rate_limited", "unreachable", ] as const; export type ExternalObjectLivenessState = (typeof EXTERNAL_OBJECT_LIVENESS_STATES)[number]; diff --git a/server/src/__tests__/external-objects-service.test.ts b/server/src/__tests__/external-objects-service.test.ts index 76555d88dd5..e9022599779 100644 --- a/server/src/__tests__/external-objects-service.test.ts +++ b/server/src/__tests__/external-objects-service.test.ts @@ -326,7 +326,7 @@ describe("GitHub external object provider", () => { status: 403, headers: { "x-ratelimit-remaining": "0", "x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 120) }, }), - { ok: false, liveness: "unreachable", errorCode: "github_rate_limited" }, + { ok: false, liveness: "rate_limited", errorCode: "github_rate_limited" }, ], ])("maps %s responses to provider-safe results", async (_name, githubResponse, expected) => { const provider = createGitHubExternalObjectProvider({} as any, { @@ -498,7 +498,7 @@ describe("GitHub external object provider", () => { expect(JSON.stringify(result)).not.toContain("http"); }); - it("classifies a 404 as unreachable when the repository probe is rate-limited", async () => { + it("classifies a 404 as rate-limited when the repository probe is rate-limited", async () => { const fetch = vi.fn(async (url: string) => { if (url.endsWith("/repos/acme/app")) { return new Response("", { @@ -519,8 +519,8 @@ describe("GitHub external object provider", () => { expect(result).toEqual( expect.objectContaining({ ok: false, - liveness: "unreachable", - errorCode: "github_repo_probe_failed", + liveness: "rate_limited", + errorCode: "github_rate_limited", }), ); expect(result).not.toEqual( @@ -528,6 +528,180 @@ describe("GitHub external object provider", () => { ); expect(JSON.stringify(result)).not.toContain("http"); }); + + it("classifies an authenticated rate limit as rate-limited rather than unreachable", async () => { + const fetch = vi.fn(async () => new Response("", { + status: 403, + headers: { "x-ratelimit-remaining": "0", "x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 120) }, + })); + const provider = createGitHubExternalObjectProvider({} as any, { + fetch, + tokenProvider: async () => "ghp_secret", + }); + const resolver = provider.resolvers.find((entry) => entry.objectType === "pull_request")!; + + const result = await resolver.resolve({ + companyId: "company-1", + object: githubObject("pull/42", "pull_request"), + }); + + expect(result).toEqual( + expect.objectContaining({ ok: false, liveness: "rate_limited", errorCode: "github_rate_limited" }), + ); + expect(JSON.stringify(result)).not.toContain("ghp_secret"); + }); + + it("classifies a 429 as rate-limited", async () => { + const fetch = vi.fn(async () => new Response("", { + status: 429, + headers: { "retry-after": "60" }, + })); + const provider = createGitHubExternalObjectProvider({} as any, { fetch, tokenProvider: null }); + const resolver = provider.resolvers.find((entry) => entry.objectType === "pull_request")!; + + const result = await resolver.resolve({ + companyId: "company-1", + object: githubObject("pull/42", "pull_request"), + }); + + expect(result).toEqual( + expect.objectContaining({ ok: false, liveness: "rate_limited", errorCode: "github_rate_limited" }), + ); + }); + + it("limits a server-environment token to public repositories and never fetches a private object", async () => { + const fetch = vi.fn(async (url: string) => { + if (url.endsWith("/repos/acme/app")) return response({ private: true, full_name: "acme/app" }); + return response({ state: "open", draft: false, merged: false, title: "Private PR" }); + }); + const provider = createGitHubExternalObjectProvider({} as any, { + fetch, + credentialProvider: async () => ({ token: "env-token", source: "server_env", secretName: null }), + }); + const resolver = provider.resolvers.find((entry) => entry.objectType === "pull_request")!; + + const result = await resolver.resolve({ + companyId: "company-1", + object: githubObject("pull/42", "pull_request"), + }); + + expect(result).toEqual( + expect.objectContaining({ + ok: false, + liveness: "auth_required", + errorCode: "github_repo_requires_company_token", + }), + ); + expect(fetch).not.toHaveBeenCalledWith(expect.stringContaining("/pulls/42"), expect.anything()); + expect(JSON.stringify(result)).not.toContain("env-token"); + }); + + it("fails closed when a server-environment token probe does not confirm the repository is public", async () => { + const fetch = vi.fn(async (url: string) => { + // No `private` flag: public visibility is not positively confirmed. + if (url.endsWith("/repos/acme/app")) return response({ full_name: "acme/app" }); + return response({ state: "open", draft: false, merged: false, title: "Unconfirmed PR" }); + }); + const provider = createGitHubExternalObjectProvider({} as any, { + fetch, + credentialProvider: async () => ({ token: "env-token", source: "server_env", secretName: null }), + }); + const resolver = provider.resolvers.find((entry) => entry.objectType === "pull_request")!; + + const result = await resolver.resolve({ + companyId: "company-1", + object: githubObject("pull/42", "pull_request"), + }); + + expect(result).toEqual( + expect.objectContaining({ + ok: false, + liveness: "auth_required", + errorCode: "github_repo_requires_company_token", + }), + ); + expect(fetch).not.toHaveBeenCalledWith(expect.stringContaining("/pulls/42"), expect.anything()); + expect(JSON.stringify(result)).not.toContain("env-token"); + }); + + it("uses a server-environment token to resolve a public repository", async () => { + const fetch = vi.fn(async (url: string) => { + if (url.endsWith("/repos/acme/app")) return response({ private: false }); + return response({ state: "open", draft: false, merged: false, title: "Public PR" }); + }); + const provider = createGitHubExternalObjectProvider({} as any, { + fetch, + credentialProvider: async () => ({ token: "env-token", source: "server_env", secretName: null }), + }); + const resolver = provider.resolvers.find((entry) => entry.objectType === "pull_request")!; + + const result = await resolver.resolve({ + companyId: "company-1", + object: githubObject("pull/42", "pull_request"), + }); + + expect(result.ok).toBe(true); + expect(fetch).toHaveBeenCalledWith("https://api.github.com/repos/acme/app/pulls/42", expect.anything()); + }); + + it("re-verifies public access for a server-environment token instead of reusing a cached verdict", async () => { + let repoPrivate = false; + const fetch = vi.fn(async (url: string) => { + if (url.endsWith("/repos/acme/app")) return response({ private: repoPrivate }); + return response({ state: "open", draft: false, merged: false, title: "acme/app PR" }); + }); + const provider = createGitHubExternalObjectProvider({} as any, { + fetch, + credentialProvider: async () => ({ token: "env-token", source: "server_env", secretName: null }), + }); + const resolver = provider.resolvers.find((entry) => entry.objectType === "pull_request")!; + + // First refresh while the repository is public: the object resolves. + const publicResult = await resolver.resolve({ + companyId: "company-1", + object: githubObject("pull/42", "pull_request"), + }); + expect(publicResult.ok).toBe(true); + + // The repository becomes private inside the probe-cache TTL. The server token + // still has access, but this company did not authorize it, so the second object + // must not reuse the cached "public" verdict to read private details. + repoPrivate = true; + const privateResult = await resolver.resolve({ + companyId: "company-1", + object: githubObject("pull/43", "pull_request"), + }); + + expect(privateResult).toEqual( + expect.objectContaining({ + ok: false, + liveness: "auth_required", + errorCode: "github_repo_requires_company_token", + }), + ); + expect(fetch).not.toHaveBeenCalledWith(expect.stringContaining("/pulls/43"), expect.anything()); + expect(JSON.stringify(privateResult)).not.toContain("env-token"); + }); + + it("lets a company-scoped token resolve a private repository", async () => { + const fetch = vi.fn(async (url: string) => { + if (url.endsWith("/repos/acme/app")) return response({ private: true }); + return response({ state: "open", draft: false, merged: false, title: "Private PR" }); + }); + const provider = createGitHubExternalObjectProvider({} as any, { + fetch, + credentialProvider: async () => ({ token: "company-token", source: "company_secret", secretName: "GITHUB_TOKEN" }), + }); + const resolver = provider.resolvers.find((entry) => entry.objectType === "pull_request")!; + + const result = await resolver.resolve({ + companyId: "company-1", + object: githubObject("pull/42", "pull_request"), + }); + + expect(result.ok).toBe(true); + expect(fetch).toHaveBeenCalledWith("https://api.github.com/repos/acme/app/pulls/42", expect.anything()); + }); }); describeEmbeddedPostgres("externalObjectService", () => { diff --git a/server/src/__tests__/git-credentials.test.ts b/server/src/__tests__/git-credentials.test.ts index e5aee1eb96f..d2fb1cd44bd 100644 --- a/server/src/__tests__/git-credentials.test.ts +++ b/server/src/__tests__/git-credentials.test.ts @@ -12,6 +12,7 @@ import { createIssueGitRemoteAuthProvider, describeGitAuthFailure, isGitHubHttpsRemoteUrl, + resolveGitHubApiCredential, scrubGitCredentialText, } from "../services/git-credentials.ts"; @@ -48,6 +49,32 @@ describe("isGitHubHttpsRemoteUrl", () => { }); }); +describe("resolveGitHubApiCredential", () => { + it("prefers a company secret over the server environment and marks it company-scoped", async () => { + const secrets = buildSecretsFake({ GH_TOKEN: "gh-token" }); + const credential = await resolveGitHubApiCredential(fakeDb, "company-1", { + secrets, + env: { GITHUB_TOKEN: "env-token" }, + }); + expect(credential).toEqual({ token: "gh-token", source: "company_secret", secretName: "GH_TOKEN" }); + }); + + it("falls back to the server environment and marks it server_env so callers can restrict it", async () => { + const secrets = buildSecretsFake({}); + const credential = await resolveGitHubApiCredential(fakeDb, "company-1", { + secrets, + env: { GITHUB_TOKEN: "env-token" }, + }); + expect(credential).toEqual({ token: "env-token", source: "server_env", secretName: null }); + }); + + it("returns null when neither a secret nor the environment provides a token", async () => { + const secrets = buildSecretsFake({}); + const credential = await resolveGitHubApiCredential(fakeDb, "company-1", { secrets, env: {} }); + expect(credential).toBeNull(); + }); +}); + describe("createGitRemoteAuthProvider", () => { const githubUrl = "https://github.com/example/repo.git"; diff --git a/server/src/services/external-objects.ts b/server/src/services/external-objects.ts index 6776cb25305..56a8fb91834 100644 --- a/server/src/services/external-objects.ts +++ b/server/src/services/external-objects.ts @@ -72,7 +72,7 @@ export type ExternalObjectResolveResult = | { ok: true; snapshot: ExternalObjectResolverSnapshot } | { ok: false; - liveness: Extract; + liveness: Extract; errorCode: string; errorMessage?: string | null; retryAfterSeconds?: number; @@ -676,7 +676,7 @@ export function externalObjectService( byLiveness[object.liveness] = (byLiveness[object.liveness] ?? 0) + 1; const livenessTone = object.liveness === "auth_required" || object.liveness === "unreachable" ? "danger" - : object.liveness === "stale" + : object.liveness === "rate_limited" || object.liveness === "stale" ? "warning" : object.statusTone; if (severityRank[livenessTone] > severityRank[highestSeverity]) highestSeverity = livenessTone; diff --git a/server/src/services/git-credentials.ts b/server/src/services/git-credentials.ts index 69bac2c029e..520c03c7628 100644 --- a/server/src/services/git-credentials.ts +++ b/server/src/services/git-credentials.ts @@ -200,6 +200,54 @@ type GitCredentialSecretsDeps = { resolveUserSecretValue?: SecretServiceLike["resolveUserSecretValue"]; }; +export type GitHubApiCredentialSource = "company_secret" | "server_env"; + +export type GitHubApiCredential = { + token: string; + source: GitHubApiCredentialSource; + /** The company-secret name the token came from; null for a server-environment token. */ + secretName: string | null; +}; + +/** + * Resolve a GitHub API credential for company-scoped, non-run work (for example the + * external-object refresh sweep, which has no agent or responsible-user context). + * + * This is the background-safe subset of the git credential order: it reads a company + * secret by well-known name, then the server process environment (`GITHUB_TOKEN`/`GH_TOKEN`) + * that self-hosted operators already set for Git. It deliberately cannot resolve a managed + * GitHub connection, because every managed grant is scoped to one agent or one person and a + * background sweep has neither. Callers that have a run context must use + * `createGitRemoteAuthProvider`/`resolveManagedGitHubCredential` instead so the managed + * identity and its consent boundary are honored. + * + * The `source` matters to callers that show company data. A server-environment token is + * instance-wide, so it can see repositories the company cannot. A caller must treat a + * `server_env` token differently from a `company_secret` token and keep it away from + * private repositories. Without any credential a caller runs anonymous, and anonymous + * GitHub requests are capped at 60/hour, so a busy company is rate-limited into + * misleading failures. + */ +export async function resolveGitHubApiCredential( + db: Db, + companyId: string, + opts: { secretNames?: readonly string[]; env?: NodeJS.ProcessEnv; secrets?: GitCredentialSecretsDeps } = {}, +): Promise { + const secrets = opts.secrets ?? secretService(db); + for (const secretName of opts.secretNames ?? DEFAULT_GITHUB_TOKEN_SECRET_NAMES) { + const secret = await Promise.resolve(secrets.getByName(companyId, secretName)).catch(() => null); + if (!secret) continue; + const token = await secrets + .resolveSecretValue(companyId, secret.id, "latest") + .then((value) => value.trim()) + .catch(() => ""); + if (token) return { token, source: "company_secret", secretName }; + } + const env = opts.env ?? process.env; + const envToken = env.GITHUB_TOKEN?.trim() || env.GH_TOKEN?.trim() || ""; + return envToken ? { token: envToken, source: "server_env", secretName: null } : null; +} + /** * Build the credential provider for one run. Resolution order: the managed GitHub identity * resolver, then a company secret by well-known name, then the server process environment diff --git a/server/src/services/github-commit-details.ts b/server/src/services/github-commit-details.ts index b5e64b55e72..aea3451b1e4 100644 --- a/server/src/services/github-commit-details.ts +++ b/server/src/services/github-commit-details.ts @@ -1,7 +1,10 @@ import type { Db } from "@paperclipai/db"; -import { DEFAULT_GITHUB_TOKEN_SECRET_NAMES } from "./git-credentials.js"; +import { + DEFAULT_GITHUB_TOKEN_SECRET_NAMES, + resolveGitHubApiCredential, + type GitHubApiCredential, +} from "./git-credentials.js"; import { ghFetch, gitHubApiBase } from "./github-fetch.js"; -import { secretService } from "./secrets.js"; type FetchLike = (url: string, init?: RequestInit) => Promise; @@ -26,6 +29,7 @@ export type GitHubCommitDiffDetailsResolver = ( export interface GitHubCommitDetailsResolverOptions { fetch?: FetchLike; tokenProvider?: (companyId: string) => Promise | string | null; + credentialProvider?: (companyId: string) => Promise | GitHubApiCredential | null; secretNames?: readonly string[]; } @@ -42,6 +46,12 @@ function record(value: unknown): Record | null { : null; } +function normalizeCredential(credential: GitHubApiCredential | null | undefined): GitHubApiCredential | null { + const token = credential?.token?.trim(); + if (!token) return null; + return { token, source: credential!.source, secretName: credential!.secretName ?? null }; +} + export function extractGitHubCommitReference(values: readonly unknown[]): GitHubCommitReference | null { for (const value of values) { if (typeof value !== "string" || value.length === 0) continue; @@ -57,16 +67,27 @@ export function extractGitHubCommitReference(values: readonly unknown[]): GitHub return null; } -async function defaultTokenProvider(db: Db, companyId: string, secretNames: readonly string[]) { - const secrets = secretService(db); - for (const secretName of secretNames) { - const secret = await secrets.getByName(companyId, secretName); - if (!secret) continue; - const token = await secrets.resolveSecretValue(companyId, secret.id, "latest"); - const trimmed = token.trim(); - if (trimmed) return trimmed; +async function defaultCredentialProvider(db: Db, companyId: string, secretNames: readonly string[]) { + return resolveGitHubApiCredential(db, companyId, { secretNames }); +} + +/** Return the repo's `private` flag, or null when the probe does not answer. */ +async function probeRepoIsPrivate( + reference: GitHubCommitReference, + headers: Record, + fetchImpl: FetchLike, +): Promise { + try { + const response = await fetchImpl( + `${gitHubApiBase(reference.host)}/repos/${encodeURIComponent(reference.owner)}/${encodeURIComponent(reference.repo)}`, + { headers }, + ); + if (!response.ok) return null; + const body = record(await response.json()); + return typeof body?.private === "boolean" ? body.private : null; + } catch { + return null; } - return null; } function hasNextPage(response: Response): boolean { @@ -85,13 +106,21 @@ export function createGitHubCommitDiffDetailsResolver( ): GitHubCommitDiffDetailsResolver { const fetchImpl = opts.fetch ?? ghFetch; const secretNames = opts.secretNames ?? DEFAULT_GITHUB_TOKEN_SECRET_NAMES; - const tokenProvider = Object.prototype.hasOwnProperty.call(opts, "tokenProvider") && opts.tokenProvider !== undefined - ? opts.tokenProvider - : ((companyId: string) => defaultTokenProvider(db, companyId, secretNames)); + const credentialProvider: (companyId: string) => Promise = + opts.credentialProvider + ? async (companyId: string) => normalizeCredential(await opts.credentialProvider!(companyId)) + : Object.prototype.hasOwnProperty.call(opts, "tokenProvider") && opts.tokenProvider !== undefined + ? async (companyId: string) => { + const raw = typeof opts.tokenProvider === "function" ? await opts.tokenProvider(companyId) : opts.tokenProvider; + const token = raw?.trim(); + return token ? { token, source: "company_secret", secretName: null } : null; + } + : (companyId: string) => defaultCredentialProvider(db, companyId, secretNames); return async (companyId, reference) => { try { - const token = (typeof tokenProvider === "function" ? await tokenProvider(companyId) : tokenProvider)?.trim() || null; + const credential = await credentialProvider(companyId); + const token = credential?.token?.trim() || null; const headers: Record = { accept: "application/vnd.github+json", "user-agent": "paperclip-work-product-resolver", @@ -99,6 +128,13 @@ export function createGitHubCommitDiffDetailsResolver( }; if (token) headers.authorization = `Bearer ${token}`; + // A server-environment token is instance-wide. Do not use it to read a private + // repository on behalf of one company; only a company-scoped credential may. + if (credential?.source === "server_env") { + const isPrivate = await probeRepoIsPrivate(reference, headers, fetchImpl); + if (isPrivate !== false) return null; + } + const commitUrl = `${gitHubApiBase(reference.host)}/repos/${encodeURIComponent(reference.owner)}/${encodeURIComponent(reference.repo)}/commits/${encodeURIComponent(reference.sha)}`; let additions: number | null = null; let deletions: number | null = null; diff --git a/server/src/services/github-external-object-provider.ts b/server/src/services/github-external-object-provider.ts index eeb65ca648d..6dedac97efb 100644 --- a/server/src/services/github-external-object-provider.ts +++ b/server/src/services/github-external-object-provider.ts @@ -1,9 +1,8 @@ import { createHash } from "node:crypto"; import type { Db } from "@paperclipai/db"; import type { ExternalObjectCanonicalUrl } from "@paperclipai/shared"; -import { DEFAULT_GITHUB_TOKEN_SECRET_NAMES } from "./git-credentials.js"; +import { DEFAULT_GITHUB_TOKEN_SECRET_NAMES, resolveGitHubApiCredential, type GitHubApiCredential } from "./git-credentials.js"; import { ghFetch, gitHubApiBase } from "./github-fetch.js"; -import { secretService } from "./secrets.js"; import type { ExternalObjectDetection, ExternalObjectDetector, @@ -17,6 +16,11 @@ type FetchLike = (url: string, init?: RequestInit) => Promise; export interface GitHubExternalObjectProviderOptions { fetch?: FetchLike; tokenProvider?: (companyId: string) => Promise | string | null; + /** + * Resolve the credential and its source. Prefer this over `tokenProvider` when the + * caller must treat a server-environment token differently from a company credential. + */ + credentialProvider?: (companyId: string) => Promise | GitHubApiCredential | null; secretNames?: readonly string[]; } @@ -57,6 +61,12 @@ function asNestedString(record: Record, key: string, nestedKey: return nested ? asString(nested[nestedKey]) : null; } +function normalizeCredential(credential: GitHubApiCredential | null | undefined): GitHubApiCredential | null { + const token = credential?.token?.trim(); + if (!token) return null; + return { token, source: credential!.source, secretName: credential!.secretName ?? null }; +} + function parseGitHubCanonicalUrl(canonical: ExternalObjectCanonicalUrl): GitHubObjectIdentity | null { if (canonical.canonicalIdentity.scheme !== "https") return null; const host = canonical.canonicalIdentity.host.toLowerCase(); @@ -126,7 +136,19 @@ function retryAfterSeconds(response: Response) { return 300; } -function failureFromGitHubResponse(response: Response): ExternalObjectResolveResult | null { +function rateLimitedResult(authenticated: boolean, retryAfterSeconds: number): ExternalObjectResolveResult { + return { + ok: false, + liveness: "rate_limited", + errorCode: "github_rate_limited", + errorMessage: authenticated + ? "GitHub rate limit reached while refreshing this object. It will retry after the limit resets." + : "GitHub rate limit reached while refreshing this object because no GitHub credential is configured, so requests are anonymous. Add a GitHub token to raise the limit.", + retryAfterSeconds, + }; +} + +function failureFromGitHubResponse(response: Response, authenticated: boolean): ExternalObjectResolveResult | null { if (response.status === 401) { return { ok: false, @@ -140,13 +162,7 @@ function failureFromGitHubResponse(response: Response): ExternalObjectResolveRes if (response.status === 403) { const rateLimitRemaining = response.headers.get("x-ratelimit-remaining"); if (rateLimitRemaining === "0") { - return { - ok: false, - liveness: "unreachable", - errorCode: "github_rate_limited", - errorMessage: "GitHub rate limit reached while refreshing this object.", - retryAfterSeconds: retryAfterSeconds(response), - }; + return rateLimitedResult(authenticated, retryAfterSeconds(response)); } return { ok: false, @@ -157,11 +173,15 @@ function failureFromGitHubResponse(response: Response): ExternalObjectResolveRes }; } - if (response.status === 429 || response.status >= 500) { + if (response.status === 429) { + return rateLimitedResult(authenticated, retryAfterSeconds(response)); + } + + if (response.status >= 500) { return { ok: false, liveness: "unreachable", - errorCode: response.status === 429 ? "github_rate_limited" : "github_unreachable", + errorCode: "github_unreachable", errorMessage: `GitHub returned HTTP ${response.status} while refreshing this object.`, retryAfterSeconds: retryAfterSeconds(response), }; @@ -200,40 +220,53 @@ function notFoundSnapshot(identity: GitHubObjectIdentity, etag: string | null): } /** - * Probe `/repos/{owner}/{repo}` to classify a 404 on a pull request or issue. + * Probe `/repos/{owner}/{repo}` to classify a 404 on a pull request or issue, and to + * learn whether the repository is private. * * GitHub returns 404 both when the object does not exist and when the repository * exists but the credentials cannot see it (private repos are deliberately hidden * behind 404). This probes the repo metadata with the same credentials to tell the * two apart: * -* - "visible": the repo returns 2xx — the referenced object really is gone. + * - "visible": the repo returns 2xx — the referenced object really is gone. The + * response also carries the repo's `private` flag. * - "hidden": the repo returns 404/403 — the credentials lack access to the repo, * so the object status cannot be determined and should not be frozen as "Not found". - * - "uncertain": the probe itself failed (network/rate limit) — retry later. + * - "rate_limited": the probe was rejected by a GitHub rate limit — retry later, with a + * distinct state so a quota problem is not reported as a network failure. + * - "uncertain": the probe itself failed (network/other) — retry later. */ +type RepoProbeOutcome = { + visibility: "visible" | "hidden" | "rate_limited" | "uncertain"; + isPrivate: boolean | null; +}; + async function probeRepoVisibility( identity: GitHubObjectIdentity, opts: { fetch: FetchLike; headers?: Record | undefined }, -): Promise<"visible" | "hidden" | "uncertain"> { +): Promise { try { const probe = await opts.fetch( `${gitHubApiBase(identity.host)}/repos/${encodeURIComponent(identity.owner)}/${encodeURIComponent(identity.repo)}`, { headers: opts.headers }, ); - if (probe.ok) return "visible"; - if (probe.status === 404) return "hidden"; + if (probe.ok) { + const body = await safeJson(probe); + return { visibility: "visible", isPrivate: typeof body?.private === "boolean" ? body.private : null }; + } + if (probe.status === 404) return { visibility: "hidden", isPrivate: null }; + if (probe.status === 429) return { visibility: "rate_limited", isPrivate: null }; if (probe.status === 403) { // GitHub can return 403 for both "credentials lack access" and "rate limit // reached". A rate-limited probe cannot tell the two apart, so treat it as - // uncertain (retry later) instead of implying missing access. + // a rate limit (retry later) instead of implying missing access. const rateLimitRemaining = probe.headers.get("x-ratelimit-remaining"); - if (rateLimitRemaining === "0") return "uncertain"; - return "hidden"; + if (rateLimitRemaining === "0") return { visibility: "rate_limited", isPrivate: null }; + return { visibility: "hidden", isPrivate: null }; } - return "uncertain"; + return { visibility: "uncertain", isPrivate: null }; } catch { - return "uncertain"; + return { visibility: "uncertain", isPrivate: null }; } } @@ -362,27 +395,25 @@ async function safeJson(response: Response) { } } -async function defaultTokenProvider(db: Db, companyId: string, secretNames: readonly string[]) { - const secrets = secretService(db); - for (const secretName of secretNames) { - const secret = await secrets.getByName(companyId, secretName); - if (!secret) continue; - const token = await secrets.resolveSecretValue(companyId, secret.id, "latest"); - const trimmed = token.trim(); - if (trimmed) return trimmed; - } - return null; -} - export function createGitHubExternalObjectProvider( db: Db, opts: GitHubExternalObjectProviderOptions = {}, ): { detector: ExternalObjectDetector; resolvers: ExternalObjectResolver[] } { const fetchImpl = opts.fetch ?? ghFetch; const secretNames = opts.secretNames ?? DEFAULT_GITHUB_TOKEN_SECRET_NAMES; - const tokenProvider = Object.prototype.hasOwnProperty.call(opts, "tokenProvider") && opts.tokenProvider !== undefined - ? opts.tokenProvider - : ((companyId: string) => defaultTokenProvider(db, companyId, secretNames)); + // `credentialProvider` is the preferred seam; `tokenProvider` stays for callers that + // only care about the token string. A token from `tokenProvider` is treated as + // company-scoped, because the caller supplied it explicitly. + const credentialProvider: (companyId: string) => Promise = + opts.credentialProvider + ? async (companyId: string) => normalizeCredential(await opts.credentialProvider!(companyId)) + : Object.prototype.hasOwnProperty.call(opts, "tokenProvider") && opts.tokenProvider !== undefined + ? async (companyId: string) => { + const raw = typeof opts.tokenProvider === "function" ? await opts.tokenProvider(companyId) : opts.tokenProvider; + const token = raw?.trim(); + return token ? { token, source: "company_secret", secretName: null } : null; + } + : (companyId: string) => resolveGitHubApiCredential(db, companyId, { secretNames }); // Cache repository-visibility probe results so many objects in the same repo // share a single probe instead of each issuing its own request every refresh // cycle. Only determined outcomes ("visible"/"hidden") are cached; uncertain @@ -391,21 +422,21 @@ export function createGitHubExternalObjectProvider( // reused after the company rotates its credentials — that would freeze a // reference as "probe-verified not-found" using an out-of-date result. - const probeCache = new Map(); + const probeCache = new Map(); async function cachedProbeRepoVisibility( companyId: string, identity: GitHubObjectIdentity, headers: Record, - ): Promise<"visible" | "hidden" | "uncertain"> { + ): Promise { const credentialFingerprint = createHash("sha256").update(headers.authorization ?? "anonymous").digest("hex"); const key = `${companyId}\u0000${credentialFingerprint}\u0000${identity.owner.toLowerCase()}\u0000${identity.repo.toLowerCase()}`; const cached = probeCache.get(key); - if (cached && cached.expiresAt > Date.now()) return cached.result; - const result = await probeRepoVisibility(identity, { fetch: fetchImpl, headers }); - if (result !== "uncertain") { - probeCache.set(key, { result, expiresAt: Date.now() + GITHUB_OBJECT_TTL_SECONDS * 1000 }); + if (cached && cached.expiresAt > Date.now()) return cached.outcome; + const outcome = await probeRepoVisibility(identity, { fetch: fetchImpl, headers }); + if (outcome.visibility === "visible" || outcome.visibility === "hidden") { + probeCache.set(key, { outcome, expiresAt: Date.now() + GITHUB_OBJECT_TTL_SECONDS * 1000 }); } - return result; + return outcome; } const detector: ExternalObjectDetector = { @@ -445,9 +476,9 @@ export function createGitHubExternalObjectProvider( }; } - let token: string | null = null; + let credential: GitHubApiCredential | null = null; try { - token = typeof tokenProvider === "function" ? await tokenProvider(companyId) : tokenProvider; + credential = await credentialProvider(companyId); } catch { return { ok: false, @@ -457,7 +488,12 @@ export function createGitHubExternalObjectProvider( retryAfterSeconds: GITHUB_OBJECT_TTL_SECONDS, }; } - token = token?.trim() || null; + const token = credential?.token?.trim() || null; + const authenticated = Boolean(token); + // A server-environment token is instance-wide, so it can see repositories this + // company cannot. Only a company-scoped credential may resolve a private + // repository for company display; a server token is limited to public repos. + const companyScoped = credential?.source !== "server_env"; const headers: Record = { accept: "application/vnd.github+json", "user-agent": "paperclip-external-object-resolver", @@ -465,6 +501,54 @@ export function createGitHubExternalObjectProvider( }; if (token) headers.authorization = `Bearer ${token}`; + if (!companyScoped) { + // A server-environment token is instance-wide, so it can read repositories + // this company cannot. Before reading with it, verify that the repository is + // public RIGHT NOW. Never serve this authorization decision from the probe + // cache: a repository can become private inside the cache TTL, and a cached + // "public" verdict must not authorize a later read of its private contents. + // The cache is still used below to classify a 404, where a stale verdict can + // only make the object look deleted — it can never expose private data. + const probe = await probeRepoVisibility(identity, { fetch: fetchImpl, headers }); + if (probe.visibility === "hidden") { + return { + ok: false, + liveness: "auth_required", + errorCode: "github_repo_not_accessible", + errorMessage: `GitHub returned not-found for ${identity.owner}/${identity.repo}. The repository may be private, deleted, or the configured GitHub credentials may lack access to it.`, + retryAfterSeconds: GITHUB_OBJECT_TTL_SECONDS, + }; + } + if (probe.visibility === "rate_limited") { + return rateLimitedResult(authenticated, GITHUB_OBJECT_TTL_SECONDS); + } + if (probe.visibility === "uncertain") { + return { + ok: false, + liveness: "unreachable", + errorCode: "github_repo_probe_failed", + errorMessage: "GitHub could not verify this repository while checking this object.", + retryAfterSeconds: GITHUB_OBJECT_TTL_SECONDS, + }; + } + // Require positive proof that the repository is public before reading + // with the instance-wide server token. A private verdict and a probe + // response that does not confirm public visibility both fail closed, so + // the server token can never fetch private details for a company that + // did not authorize it. + if (probe.isPrivate !== false) { + return { + ok: false, + liveness: "auth_required", + errorCode: "github_repo_requires_company_token", + errorMessage: probe.isPrivate === true + ? `The ${identity.owner}/${identity.repo} repository is private. Add a GitHub token for this company to refresh this object.` + : `The ${identity.owner}/${identity.repo} repository visibility could not be confirmed as public. Add a GitHub token for this company to refresh this object.`, + retryAfterSeconds: GITHUB_OBJECT_TTL_SECONDS, + }; + } + } + const apiKind = objectType === "pull_request" ? "pulls" : "issues"; const url = `${gitHubApiBase(identity.host)}/repos/${encodeURIComponent(identity.owner)}/${encodeURIComponent(identity.repo)}/${apiKind}/${identity.number}`; @@ -489,11 +573,11 @@ export function createGitHubExternalObjectProvider( // Probe the repository metadata to classify: if the repo itself is visible // the object truly is gone; if it is not, surface credentials-needed // instead of permanently freezing a reference as "Not found". - const repoVisibility = await cachedProbeRepoVisibility(companyId, identity, headers); - if (repoVisibility === "visible") { + const probe = await cachedProbeRepoVisibility(companyId, identity, headers); + if (probe.visibility === "visible") { return { ok: true, snapshot: notFoundSnapshot(identity, etag) }; } - if (repoVisibility === "hidden") { + if (probe.visibility === "hidden") { return { ok: false, liveness: "auth_required", @@ -502,6 +586,9 @@ export function createGitHubExternalObjectProvider( retryAfterSeconds: GITHUB_OBJECT_TTL_SECONDS, }; } + if (probe.visibility === "rate_limited") { + return rateLimitedResult(authenticated, GITHUB_OBJECT_TTL_SECONDS); + } return { ok: false, liveness: "unreachable", @@ -511,7 +598,7 @@ export function createGitHubExternalObjectProvider( }; } - const failure = failureFromGitHubResponse(response); + const failure = failureFromGitHubResponse(response, authenticated); if (failure) return failure; if (!response.ok) { return { diff --git a/ui/src/components/ExternalObjectPill.test.tsx b/ui/src/components/ExternalObjectPill.test.tsx index 0b19095b904..e6182de2caf 100644 --- a/ui/src/components/ExternalObjectPill.test.tsx +++ b/ui/src/components/ExternalObjectPill.test.tsx @@ -75,6 +75,24 @@ describe("ExternalObjectPill", () => { expect(auth).toContain("[border-style:dashed]"); }); + it("shows the liveness warning tone for a rate-limited object, not the last status tone", () => { + const html = renderToStaticMarkup( + , + ); + expect(html).toContain("text-amber-600"); + expect(html).toContain("[border-style:dashed]"); + expect(html).not.toContain("text-blue-600"); + }); + it("does not show a source count when only a single mention is present", () => { const html = renderToStaticMarkup( { expect(externalObjectIconForKey("unknown-provider-icon")).toBeNull(); }); + it("returns a liveness warning icon only for problem liveness states", () => { + expect(externalObjectIconForLiveness("rate_limited")?.displayName ?? "").toMatch(/AlertCircle|CircleAlert|Alert/); + expect(externalObjectIconForLiveness("unreachable")?.displayName ?? "").toMatch(/CloudOff|Cloud/); + expect(externalObjectIconForLiveness("auth_required")?.displayName ?? "").toMatch(/KeyRound|Key/); + expect(externalObjectIconForLiveness("fresh")).toBeNull(); + expect(externalObjectIconForLiveness("unknown")).toBeNull(); + expect(externalObjectIconForLiveness("stale")).toBeNull(); + }); + it("sorts items by severity first, preserving insertion order within a tone", () => { const items = [ { id: "a", statusTone: "info", providerKey: "github", objectType: "pull_request", displayTitle: null, statusCategory: "running", liveness: "fresh", isTerminal: false }, diff --git a/ui/src/lib/external-objects.ts b/ui/src/lib/external-objects.ts index 382e47ad73d..41c9f0c7253 100644 --- a/ui/src/lib/external-objects.ts +++ b/ui/src/lib/external-objects.ts @@ -39,6 +39,7 @@ export const externalObjectCategoryIcon: Record = { closed: Circle, archived: Archive, auth_required: KeyRound, + rate_limited: AlertCircle, unreachable: CloudOff, }; @@ -70,6 +71,7 @@ export function externalObjectIconForKey(iconKey: string | null | undefined): Lu export function externalObjectIconForLiveness(liveness: string): LucideIcon | null { if (liveness === "auth_required") return KeyRound; + if (liveness === "rate_limited") return AlertCircle; if (liveness === "unreachable") return CloudOff; return null; } @@ -85,6 +87,7 @@ const CATEGORY_LABELS: Record = { closed: "Closed", archived: "Archived", auth_required: "Authorization required", + rate_limited: "Rate limited", unreachable: "Unreachable", }; @@ -97,6 +100,7 @@ const LIVENESS_LABELS: Record = { fresh: "Fresh", stale: "Stale", auth_required: "Requires auth", + rate_limited: "Rate limited", unreachable: "Unreachable", }; @@ -151,6 +155,7 @@ const CATEGORY_TONE_FALLBACK: Record = { closed: "muted", archived: "muted", auth_required: "warning", + rate_limited: "warning", unreachable: "danger", }; diff --git a/ui/src/lib/issue-filters.ts b/ui/src/lib/issue-filters.ts index 0e7c0c16b98..9ef192e814c 100644 --- a/ui/src/lib/issue-filters.ts +++ b/ui/src/lib/issue-filters.ts @@ -57,6 +57,7 @@ export const externalObjectFilterOrder = [ "waiting", "running", "auth_required", + "rate_limited", "unreachable", "stale", "none", @@ -67,6 +68,7 @@ const EXTERNAL_OBJECT_FILTER_LABELS: Record = { waiting: "Any waiting", running: "Any running", auth_required: "Auth required", + rate_limited: "Rate limited", unreachable: "Unreachable", stale: "Stale", none: "No external objects", @@ -193,6 +195,8 @@ function issueMatchesExternalObjectStatusFilter( case "auth_required": return (summary?.authRequiredCount ?? 0) > 0 || summaryRecordCount(summary?.byLiveness, "auth_required") > 0; + case "rate_limited": + return summaryRecordCount(summary?.byLiveness, "rate_limited") > 0; case "unreachable": return (summary?.unreachableCount ?? 0) > 0 || summaryRecordCount(summary?.byLiveness, "unreachable") > 0; diff --git a/ui/src/lib/status-colors.ts b/ui/src/lib/status-colors.ts index 024c5ee4569..4925f30cf38 100644 --- a/ui/src/lib/status-colors.ts +++ b/ui/src/lib/status-colors.ts @@ -344,6 +344,7 @@ export const externalObjectStatusIcon: Record = { closed: "text-neutral-500 border-neutral-500", archived: "text-neutral-500 border-neutral-500", auth_required: "text-amber-600 border-amber-600 dark:text-amber-400 dark:border-amber-400", + rate_limited: "text-amber-600 border-amber-600 dark:text-amber-400 dark:border-amber-400", unreachable: "text-red-600 border-red-600 dark:text-red-400 dark:border-red-400", }; @@ -360,6 +361,7 @@ export const externalObjectStatusBadge: Record = { closed: "bg-muted text-muted-foreground", archived: "bg-muted text-muted-foreground", auth_required: "bg-amber-100 text-amber-700 dark:bg-amber-900/50 dark:text-amber-300", + rate_limited: "bg-amber-100 text-amber-700 dark:bg-amber-900/50 dark:text-amber-300", unreachable: "bg-red-100 text-red-700 dark:bg-red-900/50 dark:text-red-300", }; @@ -378,6 +380,7 @@ export const externalObjectLivenessOverlay: Record = { fresh: "", stale: "opacity-70 [border-style:dashed]", auth_required: "[border-style:dashed]", + rate_limited: "[border-style:dashed]", unreachable: "[border-style:dashed]", };