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 <noreply@paperclip.ing>
This commit is contained in:
DottaandPaperclip committed 2026-10-26 16:55:53 -05:00
1 parent c08aa4585e
commit 03b434a700
6 files changed
+84 -11

No files matched your search

@@ -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.
+4 -4
View File
@@ -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.
@@ -247,6 +247,7 @@ export interface ChatEndpointSetupState {
export interface ChatEndpointSetupSecret {
webhookSecret: string;
credentialRotationId?: string;
}
export type ChannelPublicationMode = "automatic" | "explicit";
+1
View File
@@ -169,6 +169,7 @@ export interface ChatCallbackSurfaceState {
export interface ChatEndpointSetupSecret {
webhookSecret: string;
credentialRotationId?: string;
}
type ListResponse<T> =
@@ -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 },
@@ -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<string | null>(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 <div className="space-y-6">
@@ -89,10 +115,16 @@ export function GitHubAppRecovery({ endpoint, reconnect, rotateWebhookSecret = f
<p className="text-sm">{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 <a href={gitHubAppSettingsUrl(endpoint)} target="_blank" rel="noreferrer" className="underline">GitHub App settings</a>. GitHub must verify the new secret before you reconnect.</>}</p>
<p className="text-sm text-muted-foreground">Generating immediately invalidates the previous secret. This value is shown only here; keep this page open until you have copied it.</p>
<Button type="button" variant="outline" disabled={pending} onClick={() => void generateSecret()}>{rotationSecret || rotationAlreadyCommitted ? "Generate another secret" : "Generate webhook secret"}</Button>
{rotationSecret && <div className="space-y-2">
{rotationUncertain && <p role="status" className="text-sm">The active secret is not confirmed. Check its saved status before using an earlier value.</p>}
{rotationUncertain && rotationSecret && <Button type="button" variant="outline" disabled={pending} onClick={async () => {
setPending(true);
try { if (await checkSavedRotation(rotationGeneration ?? undefined)) setError(""); }
finally { setPending(false); }
}}>Check saved secret status</Button>}
{rotationSecret && !rotationUncertain && <div className="space-y-2">
<Label htmlFor="github-rotation-secret">New webhook secret</Label>
<Input id="github-rotation-secret" type="password" autoComplete="off" readOnly value={rotationSecret} />
<CopyText text={rotationSecret} ariaLabel="Copy webhook secret" className="text-sm underline">Copy webhook secret</CopyText>
<Input id="github-rotation-secret" type="password" autoComplete="off" disabled={pending} readOnly value={rotationSecret} />
{!pending && <CopyText text={rotationSecret} ariaLabel="Copy webhook secret" className="text-sm underline">Copy webhook secret</CopyText>}
</div>}
</section>}
{error && <div role="alert" className="space-y-2 rounded-lg border border-destructive/40 bg-destructive/5 p-4 text-sm">
@@ -147,7 +179,7 @@ export function GitHubAppRecovery({ endpoint, reconnect, rotateWebhookSecret = f
</div>}
</div>}
<SetupWizardFooter onSaveExit={onExit} disabled={pending}>
<Button aria-label={actionLabel} disabled={pending || incomplete || (rotateWebhookSecret && !rotationSecret && !rotationAlreadyCommitted)} onClick={() => void connect()}>
<Button aria-label={actionLabel} disabled={pending || incomplete || rotationUncertain || (rotateWebhookSecret && !rotationSecret && !rotationAlreadyCommitted)} onClick={() => void connect()}>
{pending && <Loader2 className="size-4 animate-spin" aria-hidden="true" />}
<span className="sm:hidden" aria-hidden="true">{pending ? "Checking…" : reconnect ? "Reconnect" : "Connect"}</span>
<span className="hidden sm:inline" aria-hidden="true">{actionLabel}</span>