From 69c0a8541ca8e37fbaee5c05cc078ab2d3421649 Mon Sep 17 00:00:00 2001 From: scotttong Date: Wed, 30 Sep 2026 18:02:31 -0700 Subject: [PATCH] fix(connections): commit a Paperclip page before the OAuth callback's slow work Railway's consent page navigates to the callback and, if it is still on screen ~2s later, replaces itself with its own 'you can close this window' screen. Code exchange plus tool discovery takes longer than that, so the browser never returned to Paperclip even though the connection completed. A cross-site browser navigation to the callback now gets an immediate same-origin interstitial that repeats the request; the repeat does the work. Also updates three callback tests to the armed ask-first write default. Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/tool-access-service.test.ts | 77 ++++++++++++++----- server/src/routes/tool-access.ts | 30 ++++++++ 2 files changed, 87 insertions(+), 20 deletions(-) diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index f2b45f8e3da..85b10d585c8 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -8048,7 +8048,7 @@ describeEmbeddedPostgres("tool access service", () => { } }); - it("activates allowed Drive write actions without approval defaults after a managed callback", async () => { + it("activates Drive write actions behind ask-first defaults after a managed callback", async () => { const company = await createCompany(db); const userId = `drive-write-member-${randomUUID()}`; await grantBoardUser(db, company.id, userId, [], "owner"); @@ -8148,8 +8148,9 @@ describeEmbeddedPostgres("tool access service", () => { const createEntry = callback.body.catalog.find( (entry: { toolName: string }) => entry.toolName === "create_file", ); - await expect( - db + // The armed default: writes ask first, reads stay allowed. + const drivePolicies = JSON.stringify( + await db .select() .from(toolPolicies) .where( @@ -8158,7 +8159,9 @@ describeEmbeddedPostgres("tool access service", () => { eq(toolPolicies.enabled, true), ), ), - ).resolves.toEqual([]); + ); + expect(drivePolicies).toContain(createEntry.id); + expect(drivePolicies).not.toContain(searchEntry.id); await expect( db .select() @@ -8273,12 +8276,20 @@ describeEmbeddedPostgres("tool access service", () => { ).resolves.toEqual([ expect.objectContaining({ targetType: "agent", targetId: agent.id }), ]); - await expect( - db - .select() - .from(toolPolicies) - .where(eq(toolPolicies.companyId, company.id)), - ).resolves.toEqual([]); + // Reconnecting neither adds nor drops the ask-first defaults. + expect( + JSON.stringify( + await db + .select() + .from(toolPolicies) + .where( + and( + eq(toolPolicies.companyId, company.id), + eq(toolPolicies.enabled, true), + ), + ), + ), + ).toBe(drivePolicies); } finally { driveDefinition.ownershipAvailability = previousOwnershipAvailability; } @@ -9495,8 +9506,9 @@ describeEmbeddedPostgres("tool access service", () => { const sendMessageEntry = completed.catalog.find( (entry) => entry.toolName === "send_message", )!; - await expect( - db + // The armed default: writes ask first, reads stay allowed. + const callbackPolicies = JSON.stringify( + await db .select() .from(toolPolicies) .where( @@ -9505,7 +9517,9 @@ describeEmbeddedPostgres("tool access service", () => { eq(toolPolicies.enabled, true), ), ), - ).resolves.toEqual([]); + ); + expect(callbackPolicies).toContain(sendMessageEntry.id); + expect(callbackPolicies).not.toContain(searchMessagesEntry.id); const callbackPolicy = toolAccessPolicyService(db); const decide = ( entry: (typeof completed.catalog)[number], @@ -9521,13 +9535,14 @@ describeEmbeddedPostgres("tool access service", () => { arguments: {}, }, }); - for (const entry of [searchMessagesEntry, sendMessageEntry]) { - await expect(decide(entry)).resolves.toMatchObject( - host === "task" - ? { decision: "deny" } - : { decision: "allow", reasonCode: "allow_profile" }, - ); - } + await expect(decide(searchMessagesEntry)).resolves.toMatchObject( + host === "task" + ? { decision: "deny" } + : { decision: "allow", reasonCode: "allow_profile" }, + ); + await expect(decide(sendMessageEntry)).resolves.toMatchObject( + host === "task" ? { decision: "deny" } : { decision: "require_approval" }, + ); if (host === "task") { await expect( db @@ -10115,9 +10130,31 @@ describeEmbeddedPostgres("tool access service", () => { ); expect(state).toBeTruthy(); + // The provider's redirect is a cross-site navigation: Paperclip commits a + // page at once (Railway's consent page otherwise replaces itself after ~2s) + // and leaves the state unconsumed for the same-origin repeat. + const interstitialRes = await request(app) + .get("/api/tools/oauth/callback") + .set("Accept", "text/html") + .set("Sec-Fetch-Site", "cross-site") + .set("Sec-Fetch-Mode", "navigate") + .query({ state, code: "notion-choice-code" }); + expect(interstitialRes.status).toBe(200); + expect(interstitialRes.headers["cache-control"]).toBe("no-store"); + expect(interstitialRes.text).toContain( + ``, + ); + const [pendingConnection] = await db + .select() + .from(toolConnections) + .where(eq(toolConnections.id, connectRes.body.connectionId)); + expect(pendingConnection?.status).not.toBe("active"); + const callbackRes = await request(app) .get("/api/tools/oauth/callback") .set("Accept", "text/html") + .set("Sec-Fetch-Site", "same-origin") + .set("Sec-Fetch-Mode", "navigate") .query({ state, code: "notion-choice-code" }); expect(callbackRes.status).toBe(303); diff --git a/server/src/routes/tool-access.ts b/server/src/routes/tool-access.ts index 0f4ad7d8b00..f4c51194977 100644 --- a/server/src/routes/tool-access.ts +++ b/server/src/routes/tool-access.ts @@ -191,6 +191,29 @@ export function connectionIntentOAuthOutcomeHtml(input: { return `Connection authorization

Returning to Paperclip…

`; } +// Some providers' consent pages navigate to this callback and, if their own +// page is still on screen ~2s later, replace it with a "you can close this +// window" screen (Railway does exactly this). Exchanging the code and +// discovering the tool catalog routinely takes longer than that, so the +// provider's timer wins and the browser never lands back in Paperclip even +// though the connection completed. For a cross-site browser navigation, commit +// a Paperclip document immediately and repeat the same request from it; the +// repeat is same-origin and does the slow work. +export function isCrossSiteOAuthCallbackNavigation(req: Request): boolean { + return req.get("sec-fetch-site") === "cross-site" + && req.get("sec-fetch-mode") === "navigate"; +} + +export function oauthCallbackInterstitialHtml(continuePath: string): string { + const attribute = continuePath + .replaceAll("&", "&") + .replaceAll("\"", """) + .replaceAll("<", "<"); + // A meta refresh alone, not a script as well: the OAuth code is single-use, so + // two racing follow-ups would let the loser render an expired-state error. + return `Finishing connection

Finishing your connection…

`; +} + function normalizeCloudConnectorEnrollmentReturnTo(returnTo?: string | null): string | null { if (!returnTo || returnTo.length > 2_048) return null; try { @@ -1331,6 +1354,13 @@ function connectorEnrollmentPrincipal(req: Request): string { await assertToolConnectionConfigureAccess(req, pendingConnection); } const acceptsHtml = req.get("accept")?.includes("text/html") === true; + if (acceptsHtml && isCrossSiteOAuthCallbackNavigation(req)) { + // State is only peeked above, so the same-origin repeat still owns it. + res.set("Cache-Control", "no-store"); + res.set("Referrer-Policy", "no-referrer"); + res.type("html").send(oauthCallbackInterstitialHtml(req.originalUrl)); + return; + } let result: Awaited>; try { result = await svc.completeOAuthCallback({