From 03b434a7005721b77de24053d15894efefe8d7de Mon Sep 17 00:00:00 2001 From: Dotta Date: Mon, 26 Oct 2026 16:55:53 -0500 Subject: [PATCH] Reconcile failed secret replacements and document enrollment boundaries Keep a current one-time secret after a rejected replacement, verify its committed generation after uncertain responses, and provide an explicit saved-status check. Record Cloud enrollment administrator requirements and HTML or redirect callbacks. Co-Authored-By: Paperclip --- doc/plans/2026-10-13-connection-api-parity.md | 4 ++ maps/agent-access-map/connection-setup.md | 8 ++-- packages/shared/src/types/chat-channels.ts | 1 + ui/src/api/chatEndpoints.ts | 1 + .../pages/apps/chat/GitHubChatSetup.test.tsx | 35 ++++++++++++++ .../apps/chat/GitHubConnectionRecovery.tsx | 46 ++++++++++++++++--- 6 files changed, 84 insertions(+), 11 deletions(-) diff --git a/doc/plans/2026-10-13-connection-api-parity.md b/doc/plans/2026-10-13-connection-api-parity.md index 61c263b2fe8..e53647ae0ab 100644 --- a/doc/plans/2026-10-13-connection-api-parity.md +++ b/doc/plans/2026-10-13-connection-api-parity.md @@ -161,3 +161,7 @@ The implementation was rebased onto `a1f77821b9cd5c9c846fdf7a0fa5cc620ad2b6bb`. - Implementation `35585448ef` hit a 30-second module-load hook timeout in `company-skills-routes`; that suite passed in a focused local run. The combined route/contract selection passed **90 tests**, followed by all **27 OpenAPI contract tests** after the final server contract update. The companion’s previous complete CI was green at `d84538f8a3`. Fresh CI on the credential-handoff changes remains required. Entire was removed from the expected gates by the maintainer and is no longer a completion dependency. - Final review found that the rotation UI tied reconnect to the one-time plaintext. Saved links now bind an opaque, nonsecret generation baseline, and the endpoint projection exposes only the committed generation ID. Reloading resumes that rotation without generating another secret; an unchanged generation remains blocked. Both new resume cases were red before the UI fix. All **38 GitHub wizard tests** and **69 API/OpenAPI tests** passed; token gates passed. Prior full CI passed at implementation `f99e8aa6d5` and companion `e22d2a0eba`; their final successor checks must complete independently. + +- Current-head review found an AgentMail collection scope leak and agent-keyed Arcade discovery ownership. Both new actual-auth regressions were red before the fixes. Email accounts now pass the shared resource evaluator before metadata or credential discovery; aggregator settings, credentials, inventory, and sync leases use the responsible user, while audit and secret-access records preserve the actual agent. The Arcade fixture initially created a draft and omitted provider authorization metadata; it was corrected to represent an active provider account. Its guarded HTTP transport also required an explicit deterministic fixture; a prior real transport attempt returned 401. No provider authentication was weakened. +- A reused test database retained a Slack bot identity and caused a 409 during the full suite. In a fresh PostgreSQL database, all **70 API/OpenAPI tests passed**, including 43 assembled-server cases and binding the human-only secret response to its committed nonsecret generation. Server typecheck passed. Earlier fixture and contract-update failures remain accounted for here. +- Failed GitHub secret replacements retain the prior copy only after its committed generation is confirmed unchanged. A changed or unreadable generation blocks copying and reconnecting; an explicit status check can restore the earlier copy when the server confirms it. All three new replacement cases were red before the fix; all **82 GitHub UI tests passed** afterward. Cloud enrollment map entries now name instance-admin creation/callback checks and distinguish the HTML/303 browser return. Maps and UI token gates passed. Fresh successor CI and reviews remain required before merging. diff --git a/maps/agent-access-map/connection-setup.md b/maps/agent-access-map/connection-setup.md index ba8365d63b9..beb87d8923b 100644 --- a/maps/agent-access-map/connection-setup.md +++ b/maps/agent-access-map/connection-setup.md @@ -202,7 +202,7 @@ Review: `operation-reviewed`. API: `present` — `GET /api/tools/oauth/cloud-connector/enrollment`; JSON REST. Agent access: `operator-only` — existing human setup or approval surface; agent can identify the saved setup and inspect the result. -Policy: Human consent, identity confirmation, action approval, or trusted-host administration. This endpoint does not delegate human identity or self-approval to an agent. +Policy: Board actor (`assertBoard`) for instance enrollment status; agents cannot read this operator surface. Evidence: `source-only` — [route](../../server/src/routes/tool-access.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: Agent invocation is denied by the stated policy. The authorized human or operator must perform this step; this entry does not claim delegated execution. @@ -211,16 +211,16 @@ Gaps: Agent invocation is denied by the stated policy. The authorized human or o API: `present` — `POST /api/tools/oauth/cloud-connector/enrollment`; JSON REST. Agent access: `operator-only` — existing human setup or approval surface; agent can identify the saved setup and inspect the result. -Policy: Human consent, identity confirmation, action approval, or trusted-host administration. This endpoint does not delegate human identity or self-approval to an agent. +Policy: Instance administrator (`assertInstanceAdmin`) and access to the requested company. The saved enrollment binds the initiating human principal and return destination. Evidence: `source-only` — [route](../../server/src/routes/tool-access.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: Agent invocation is denied by the stated policy. The authorized human or operator must perform this step; this entry does not claim delegated execution. ### `get-tools-oauth-cloud-connector-enrollment-callback` -API: `present` — `GET /api/tools/oauth/cloud-connector/enrollment-callback`; JSON REST. +API: `present` — `GET /api/tools/oauth/cloud-connector/enrollment-callback`; browser return — HTML completion page or HTTP 303 redirect. Agent access: `operator-only` — existing human setup or approval surface; agent can identify the saved setup and inspect the result. -Policy: Human consent, identity confirmation, action approval, or trusted-host administration. This endpoint does not delegate human identity or self-approval to an agent. +Policy: Instance administrator (`assertInstanceAdmin`), access to the saved company, and the same initiating principal. The saved enrollment ID, approval code, state, and expiry must validate before completion. Evidence: `source-only` — [route](../../server/src/routes/tool-access.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: Agent invocation is denied by the stated policy. The authorized human or operator must perform this step; this entry does not claim delegated execution. diff --git a/packages/shared/src/types/chat-channels.ts b/packages/shared/src/types/chat-channels.ts index 795a4012475..103eb76c5c5 100644 --- a/packages/shared/src/types/chat-channels.ts +++ b/packages/shared/src/types/chat-channels.ts @@ -247,6 +247,7 @@ export interface ChatEndpointSetupState { export interface ChatEndpointSetupSecret { webhookSecret: string; + credentialRotationId?: string; } export type ChannelPublicationMode = "automatic" | "explicit"; diff --git a/ui/src/api/chatEndpoints.ts b/ui/src/api/chatEndpoints.ts index e43eb89b44b..572eeacdc64 100644 --- a/ui/src/api/chatEndpoints.ts +++ b/ui/src/api/chatEndpoints.ts @@ -169,6 +169,7 @@ export interface ChatCallbackSurfaceState { export interface ChatEndpointSetupSecret { webhookSecret: string; + credentialRotationId?: string; } type ListResponse = diff --git a/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx b/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx index ec5165bbf08..b92bc0fa6ed 100644 --- a/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx +++ b/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx @@ -295,6 +295,41 @@ describe("GitHub App wizard", () => { expect(githubChatApi.connectApp).not.toHaveBeenCalled(); expect(chatEndpointsApi.create).not.toHaveBeenCalled(); }); + it.each(["unchanged", "changed", "unavailable"])("reconciles a failed replacement without using an uncertain secret (%s)", async (outcome) => { + const previous = "11111111-1111-4111-8111-111111111111"; + const generated = "22222222-2222-4222-8222-222222222222"; + const replacement = "33333333-3333-4333-8333-333333333333"; + const secret = "b".repeat(64); + fixture.endpoint.status = "active"; + fixture.endpoint.botExternalId = "1234"; + fixture.endpoint.setup.credentialRotationId = previous; + vi.mocked(chatEndpointsApi.get).mockReset().mockImplementation(async () => fixture.endpoint); + vi.mocked(chatEndpointsApi.generateSetupSecret).mockReset().mockResolvedValueOnce({ webhookSecret: secret, credentialRotationId: generated }).mockRejectedValueOnce(new Error("Replacement request failed")); + await render(`resume=draft-1&stage=credentials&reconnect=1&rotateSecret=1&afterRotationId=${previous}`); + const button = (label: string) => [...container.querySelectorAll("button")].find(item => item.textContent === label)!; + const reconnect = () => [...container.querySelectorAll("button")].find(item => item.getAttribute("aria-label") === "Reconnect saved App")!; + await act(async () => button("Generate webhook secret").click()); + const before = vi.mocked(chatEndpointsApi.get).mock.calls.length; + if (outcome === "unavailable") vi.mocked(chatEndpointsApi.get).mockRejectedValueOnce(new Error("Status unavailable")); + else vi.mocked(chatEndpointsApi.get).mockResolvedValueOnce({ ...fixture.endpoint, setup: { ...fixture.endpoint.setup, credentialRotationId: outcome === "unchanged" ? generated : replacement } }); + await act(async () => button("Generate another secret").click()); + expect(chatEndpointsApi.get).toHaveBeenCalledTimes(before + 1); + if (outcome === "unchanged") { + expect((container.querySelector("#github-rotation-secret") as HTMLInputElement).value).toBe(secret); + expect(reconnect().disabled).toBe(false); + await act(async () => (container.querySelector('[aria-label="Copy webhook secret"]') as HTMLButtonElement).click()); + expect(copyTextToClipboard).toHaveBeenLastCalledWith(secret); + } else { + expect(container.querySelector('[aria-label="Copy webhook secret"]')).toBeNull(); + expect(reconnect().disabled).toBe(true); + if (outcome === "unavailable") { + vi.mocked(chatEndpointsApi.get).mockResolvedValueOnce({ ...fixture.endpoint, setup: { ...fixture.endpoint.setup, credentialRotationId: generated } }); + await act(async () => button("Check saved secret status").click()); + expect((container.querySelector("#github-rotation-secret") as HTMLInputElement).value).toBe(secret); + expect(reconnect().disabled).toBe(false); + } + } + }); it.each([ { generation: "22222222-2222-4222-8222-222222222222", verified: true, canResume: true }, { generation: "22222222-2222-4222-8222-222222222222", verified: false, canResume: true }, diff --git a/ui/src/pages/apps/chat/GitHubConnectionRecovery.tsx b/ui/src/pages/apps/chat/GitHubConnectionRecovery.tsx index 4ee3cbaa497..0e30e67fc1f 100644 --- a/ui/src/pages/apps/chat/GitHubConnectionRecovery.tsx +++ b/ui/src/pages/apps/chat/GitHubConnectionRecovery.tsx @@ -57,6 +57,8 @@ export function GitHubAppRecovery({ endpoint, reconnect, rotateWebhookSecret = f const [error, setError] = useState(""); const [fileError, setFileError] = useState(""); const [rotationSecret, setRotationSecret] = useState(""); + const [rotationGeneration, setRotationGeneration] = useState(null); + const [rotationUncertain, setRotationUncertain] = useState(false); const needsWebhookSecret = !reconnect && !["active", "paused", "revoked"].includes(endpoint.status); const appName = endpoint.botLabel ?? endpoint.setup?.github?.appName ?? "your existing App"; const appIdInvalid = !!credentials.appId.trim() && !/^\d+$/.test(credentials.appId.trim()); @@ -71,12 +73,36 @@ export function GitHubAppRecovery({ endpoint, reconnect, rotateWebhookSecret = f catch (cause) { setError(cause instanceof Error ? cause.message : "Could not reconnect GitHub. Try again."); } finally { setPending(false); } } + async function checkSavedRotation(expected: string | null | undefined) { + try { + const saved = await chatEndpointsApi.get(endpoint.id); + if (expected !== undefined && (saved.setup?.credentialRotationId ?? null) === expected) { + setRotationUncertain(false); + return true; + } + setRotationSecret(""); + setRotationGeneration(null); + setError("The webhook secret changed, but this page did not receive it. Generate another secret and update GitHub."); + } catch { + // A failed status read cannot establish that the earlier secret is current. + } + setRotationUncertain(true); + return false; + } async function generateSecret() { + const previousGeneration = rotationGeneration ?? (rotationSecret ? undefined : endpoint.setup?.credentialRotationId ?? null); setPending(true); setError(""); - setRotationSecret(""); - try { setRotationSecret((await chatEndpointsApi.generateSetupSecret(endpoint.id)).webhookSecret); } - catch (cause) { setError(cause instanceof Error ? cause.message : "Could not generate the webhook secret. Try again."); } + try { + const result = await chatEndpointsApi.generateSetupSecret(endpoint.id); + setRotationSecret(result.webhookSecret); + setRotationGeneration(result.credentialRotationId ?? null); + setRotationUncertain(false); + } catch (cause) { + setError(cause instanceof Error ? cause.message : "Could not generate the webhook secret. Try again."); + setRotationUncertain(true); + await checkSavedRotation(previousGeneration); + } finally { setPending(false); } } return
@@ -89,10 +115,16 @@ export function GitHubAppRecovery({ endpoint, reconnect, rotateWebhookSecret = f

{rotationAlreadyCommitted ? "The rotated secret is saved. Reconnect using that value; Paperclip will check GitHub’s signed delivery." : <>Generate a new secret, then replace the webhook secret in GitHub App settings. GitHub must verify the new secret before you reconnect.}

Generating immediately invalidates the previous secret. This value is shown only here; keep this page open until you have copied it.

- {rotationSecret &&
+ {rotationUncertain &&

The active secret is not confirmed. Check its saved status before using an earlier value.

} + {rotationUncertain && rotationSecret && } + {rotationSecret && !rotationUncertain &&
- - Copy webhook secret + + {!pending && Copy webhook secret}
} } {error &&
@@ -147,7 +179,7 @@ export function GitHubAppRecovery({ endpoint, reconnect, rotateWebhookSecret = f
}
} -