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({