From 58d1b210984d1c639ee3014a2c314b0ee4e231b9 Mon Sep 17 00:00:00 2001 From: Michael Nguyen <13559011+nguyenm7@users.noreply.github.com> Date: Tue, 3 Nov 2026 17:51:29 -0800 Subject: [PATCH] fix: separate fresh setup from archived personal connections (#18902) ## Thinking Path > - Paperclip helps people manage AI agents and their service access. > - Each personal connection belongs to one user. > - Setup can recover an archived connection by its name and provider. > - A fresh setup can therefore select another user's old identity and fail its owner check. > - This change creates a separate connection for fresh setup after personal archival. > - Explicit reconnect still requires the existing owner and preserves its history. ## Linked Issues or Issue Description **What happened?** A fresh GitHub setup can return 403 after a personal connection with the same name was archived. The request does not include a reconnect target. Setup still selects the archived identity by name and provider. **Expected behavior** Fresh setup creates a separate personal identity. It leaves the archived connection, credentials, grants, and activity unchanged. Explicit reconnect rejects a different user. **Steps to reproduce** 1. Create a personal GitHub connection in a test company. 2. Archive that connection and its application. 3. As another authenticated member, start fresh personal setup with the same name and provider. 4. Observe the owner-check failure before this change. **Paperclip version or commit** Base: db9be4c5a. Tests use isolated database and HTTP fixtures. **Deployment mode** Authenticated. The legacy owner can be a local board identity. ## What Changed - Exclude implicitly recovered archived personal connections from fresh setup. - Use the existing company-scoped name allocator to create a separate application and connection. - Test three owners: a legacy board identity, another user, and the current user. - Check credential isolation, unchanged archived records and activity, missing-key rejection, and explicit reconnect denial. - Keep managed revival and generic MCP rollback/reconnect fixtures on explicit reconnect targets. - Verify generic personal URL setup creates an independent identity and preserves the archived records. - Document fresh setup and reconnect behavior. ## Verification - Final head: `96184e3179aab2eeb88df87d442691741a4875a2`. Full CI recovered and completed successfully: [run 41752164958, attempt 2](https://github.com/paperclipai/paperclip/actions/runs/41752164958/attempts/2). PR rollup: 53 successful checks, 3 skipped, none failing or pending. Typecheck, build, all server shards, Runner lanes, and E2E gates pass. - Exact-head Greptile: 5/5, zero inline comments or review threads. Independent engineering/security reviews approved the production implementation; engineering approved the test-only follow-up. Browser acceptance review approved with the timing qualification below. - Final-head local generic MCP suite: 84/84 passed (23.26 s); server typecheck passed. Reviewer independently reran the three changed scenarios: 3/3 passed (15.74 s). - Production head `587631230`: local tool-access suite 504/504, recursive typecheck, and full build passed. Broad local checks were not repeated for the test-only delta. Full local `pnpm test:run` remains incomplete after interruption during the first chat shard; no full-local-suite pass is claimed. - Initial full CI at `587631230` failed three generic MCP fixtures that still relied on implicit archived personal recovery. All three reproduced locally (15.01 s). The test-only follow-up gives owner reconnect/rollback scenarios explicit targets and verifies independent fresh URL setup without relaxing their assertions. Corrected server shard 12/12 passes on final head. The unfinished earlier run was cancelled after the follow-up push. - Initial current-head E2E shard 3/8 failed a 5000 ms task-heading wait at `tests/e2e/cloud-auth.spec.ts:46` (40 passed, one failed). Its screenshot/accessibility snapshot contains the expected heading. The single authorized no-code rerun passed 41/41 with zero test retries, including all URL, handoff, and login-form checks. The first-heading assertion took 3774 ms on rerun. Both attempts tested the same merge commit for this PR head. These observations support a timing concern; the original cause remains unknown. The unchanged Cloud-auth path does not invoke archived-connection setup. Recovery is established; an infrastructure cause is not. If it recurs, capture an instrumented first-navigation trace and compare an unchanged baseline under comparable conditions. - PR remains draft. Merge and deployment are outside this request. Synthetic provider/browser fixtures do not prove live GitHub authorization or a deployed fix. ## Risks - Fresh setup after personal archival now creates a new ID and a suffixed name, including for the same owner. Clients must supply an explicit target to reconnect the old identity. - The guard applies to all personal gallery connections, not only GitHub. - No schema migration is required. Explicit owner checks remain in place. - Security review approved the unchanged production implementation. Merge and deployment are outside this change request. ## Model Used OpenAI GPT-6 through Codex. The runtime does not expose a more specific model snapshot or context window. Used reasoning, code editing, and shell tools. ## 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 duplicate or related PRs and linked them above - [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) - [x] 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 - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- doc/connections/GITHUB.md | 6 + .../__tests__/generic-mcp-connection.test.ts | 34 ++++- .../src/__tests__/tool-access-service.test.ts | 126 +++++++++++++++++- server/src/services/tool-access.ts | 21 ++- 4 files changed, 171 insertions(+), 16 deletions(-) diff --git a/doc/connections/GITHUB.md b/doc/connections/GITHUB.md index 0f93999ab86..96826dfddfb 100644 --- a/doc/connections/GITHUB.md +++ b/doc/connections/GITHUB.md @@ -77,6 +77,12 @@ repeat an existing instance approval. ## Identity resolution +Fresh setup does not reuse an archived personal connection found by name and +provider. It creates a separate application and connection with an available +name, such as `GitHub (2)`. The archived identity, credentials, grants, and +history stay unchanged. An explicit reconnect or application-targeted setup +continues the existing identity and requires its personal owner. + Every MCP call, `gh` invocation, native Git operation, checkout, health check, and webhook binding uses the same order: diff --git a/server/src/__tests__/generic-mcp-connection.test.ts b/server/src/__tests__/generic-mcp-connection.test.ts index 2978db9bbe1..78bf314dc4f 100644 --- a/server/src/__tests__/generic-mcp-connection.test.ts +++ b/server/src/__tests__/generic-mcp-connection.test.ts @@ -1445,7 +1445,7 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { ]); }); - it("does not let another user take over an archived personal URL connection", async () => { + it("creates a separate fresh personal URL connection and keeps explicit reconnect owner-bound", async () => { const fixture = installMcpOAuthFixture({ auth: "oauth" }); const company = await createCompany(db); const service = toolAccessService(db); @@ -1456,18 +1456,36 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { await db.insert(companyMemberships).values({ companyId: company.id, principalType: "user", principalId: "other-user", status: "active", membershipRole: "admin", }); + const archivedConnection = await db.select().from(toolConnections).where(eq(toolConnections.id, first.connectionId)); + const archivedApplication = await db.select().from(toolApplications).where(eq(toolApplications.id, first.application.id)); fixture.fetchMock.mockClear(); - await expect(service.connectGalleryApp(company.id, input, { - actorType: "user", actorId: "other-user", - })).rejects.toMatchObject({ status: 403, message: "Only the existing personal identity can reconnect this connection" }); + for (const target of [{ reconnectConnectionId: first.connectionId }, { applicationId: first.application.id }]) { + await expect(service.connectGalleryApp(company.id, { ...input, ...target }, { + actorType: "user", actorId: "other-user", + })).rejects.toMatchObject({ status: 403, message: "Only the existing personal identity can reconnect this connection" }); + } expect(fixture.fetchMock).not.toHaveBeenCalled(); + const fresh = await service.connectGalleryApp(company.id, input, { actorType: "user", actorId: "other-user" }); + expect(fresh.connectionId).not.toBe(first.connectionId); + expect(fresh.application.id).not.toBe(first.application.id); + expect(fresh.auth).toMatchObject({ kind: "oauth" }); + await expect(service.getConnection(fresh.connectionId)).resolves.toMatchObject({ + createdByUserId: "other-user", credentialPolicy: "per_user", credentialSecretRefs: [], + }); + await expect(db.select().from(toolConnections).where(eq(toolConnections.id, first.connectionId))) + .resolves.toEqual(archivedConnection); + await expect(db.select().from(toolApplications).where(eq(toolApplications.id, first.application.id))) + .resolves.toEqual(archivedApplication); + // OAuth grants are created only after authorization; discovery is not a grant. await expect(db.select().from(connectionGrants)).resolves.toHaveLength(0); await expect(service.getConnection(first.connectionId)).resolves.toMatchObject({ status: "archived", createdByUserId: "board-user", }); - const resumed = await service.connectGalleryApp(company.id, input, { actorType: "user", actorId: "board-user" }); + const resumed = await service.connectGalleryApp(company.id, { + ...input, reconnectConnectionId: first.connectionId, + }, { actorType: "user", actorId: "board-user" }); expect(resumed.connectionId).toBe(first.connectionId); expect(resumed.auth).toMatchObject({ kind: "oauth" }); }); @@ -1544,7 +1562,7 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { return jsonResponse({ jsonrpc: "2.0", id: "paperclip-catalog-refresh", result: { tools: FIXTURE_TOOLS } }); }); const failure = service.connectGalleryApp(company.id, { - link: MCP_URL, name: "Personal retry rollback", grantKind: "user", + link: MCP_URL, name: "Personal retry rollback", grantKind: "user", reconnectConnectionId: first.connectionId, }, actor).then(() => null, (error: unknown) => error); await Promise.race([catalogPending, failure.then((error) => { throw error ?? new Error("Retry finished before the catalog probe"); })]); try { @@ -1587,7 +1605,9 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { FOR EACH ROW EXECUTE FUNCTION test_personal_profile_failure() `); try { - await expect(service.connectGalleryApp(company.id, input, actor)).rejects.toThrow(); + await expect(service.connectGalleryApp(company.id, { + ...input, reconnectConnectionId: first.connectionId, + }, actor)).rejects.toThrow(); await expect(db.select().from(toolCatalogEntries).where(eq(toolCatalogEntries.connectionId, first.connectionId))) .resolves.toEqual(catalogBefore); await expect(db.select().from(toolProfiles).where(eq(toolProfiles.companyId, company.id))).resolves.toHaveLength(0); diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index 5839a505ec2..166ec8f212a 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -9795,6 +9795,7 @@ describeEmbeddedPostgres("tool access service", () => { const revived = await service.connectGalleryApp( company.id, { + reconnectConnectionId: first.connectionId, galleryKey: "google-drive", connectionMethodKey: "paperclip-write", grantKind: "user", @@ -15997,7 +15998,7 @@ describeEmbeddedPostgres("tool access service", () => { ).rejects.toMatchObject({ status: 404 }); }); - it("reuses a removed gallery app while applying the omitted organization identity default", async () => { + it("creates a separate gallery app for the omitted organization default after personal removal", async () => { const company = await createCompany(db); const service = createTestToolAccessService(db); const actor = { actorType: "user" as const, actorId: "local-board" }; @@ -16022,8 +16023,10 @@ describeEmbeddedPostgres("tool access service", () => { actor, ); - expect(second.application.id).toBe(first.application.id); - expect(second.connectionId).toBe(first.connectionId); + expect(second.application.id).not.toBe(first.application.id); + expect(second.connectionId).not.toBe(first.connectionId); + await expect(service.getConnection(first.connectionId, company.id)) + .resolves.toMatchObject({ status: "archived", credentialPolicy: "per_user" }); expect(second.application.status).toBe("draft"); expect(second.connection.status).toBe("draft"); expect(second.connection.credentialPolicy).toBe("shared"); @@ -16038,8 +16041,8 @@ describeEmbeddedPostgres("tool access service", () => { isDefault: true, }), ]); - await expect(db.select().from(toolApplications)).resolves.toHaveLength(1); - await expect(db.select().from(toolConnections)).resolves.toHaveLength(1); + await expect(db.select().from(toolApplications)).resolves.toHaveLength(2); + await expect(db.select().from(toolConnections)).resolves.toHaveLength(2); }); it("restores grants and credential policy when an identity-changing revival fails", async () => { @@ -18414,6 +18417,119 @@ describeEmbeddedPostgres("tool access service", () => { }); }, 15000); + it.each(["local-board", "another-user", "current-user"])( + "creates an independent fresh GitHub identity after archiving a personal connection owned by %s", + async (ownerUserId) => { + const company = await createCompany(db); + const service = createTestToolAccessService(db); + const currentUserId = "current-user"; + await grantBoardUser(db, company.id, currentUserId, [], "member"); + mockToolsList([readTool("get_me", "Read current user")]); + await withGalleryServerUrl("github", PUBLIC_MCP_FIXTURE_URL, async () => { + const first = await service.connectGalleryApp(company.id, { + galleryKey: "github", + connectionMethodKey: "mcp-key", + name: "GitHub", + grantKind: "user", + credentialValues: { "credentials.authorization": "archived-fixture-key" }, + }, { actorType: "user", actorId: ownerUserId }); + // Model a legacy archived row that still has personal credential refs. + // Fresh setup must leave even this retained material untouched. + await db.update(toolConnections).set({ status: "archived", enabled: false }) + .where(eq(toolConnections.id, first.connectionId)); + await db.update(toolApplications).set({ status: "archived", archivedAt: new Date() }) + .where(eq(toolApplications.id, first.application.id)); + await db.insert(activityLog).values({ + companyId: company.id, actorType: "user", actorId: ownerUserId, + action: "tool_connection.archived", entityType: "tool_connection", + entityId: first.connectionId, + }); + const retained = async () => ({ + applications: await db.select().from(toolApplications) + .where(eq(toolApplications.id, first.application.id)), + connections: await db.select().from(toolConnections) + .where(eq(toolConnections.id, first.connectionId)), + grants: await db.select().from(connectionGrants) + .where(eq(connectionGrants.connectionId, first.connectionId)), + secrets: await db.select().from(companySecrets), + versions: await db.select().from(companySecretVersions), + activity: await db.select().from(activityLog) + .where(eq(activityLog.entityId, first.connectionId)), + }); + const before = await retained(); + const app = createRouteApp(db, + boardSessionActor(company.id, "member", currentUserId), + undefined, { deploymentMode: "authenticated" }); + const freshInput = { + galleryKey: "github", + connectionMethodKey: "mcp-key", + name: "GitHub", + grantKind: "user", + credentialValues: { "credentials.authorization": "fresh-fixture-key" }, + }; + + if (ownerUserId !== currentUserId) { + for (const target of [ + { reconnectConnectionId: first.connectionId }, + { applicationId: first.application.id }, + ]) { + await request(app) + .post(`/api/companies/${company.id}/tools/apps/connect`) + .send({ ...freshInput, ...target }).expect(403); + await expect(service.connectGalleryApp(company.id, { + ...freshInput, grantKind: "user", ...target, + }, { actorType: "user", actorId: currentUserId, actorSource: "session" })) + .rejects.toMatchObject({ + status: 403, + message: "Only the existing personal identity can reconnect this connection", + }); + } + expect(await retained()).toEqual(before); + } + + // An omitted key must not borrow the archived personal credential. + await request(app) + .post(`/api/companies/${company.id}/tools/apps/connect`) + .send({ ...freshInput, credentialValues: {} }).expect(400); + expect(await retained()).toEqual(before); + + const fresh = await request(app) + .post(`/api/companies/${company.id}/tools/apps/connect`) + .send(freshInput).expect(201); + expect(fresh.body.connectionId).not.toBe(first.connectionId); + expect(fresh.body.application.id).not.toBe(first.application.id); + expect(fresh.body.application.name).toBe("GitHub (2)"); + expect(fresh.body.connection).toMatchObject({ + credentialPolicy: "per_user", credentialSecretRefs: [], + }); + const after = await retained(); + expect(after.applications).toEqual(before.applications); + expect(after.connections).toEqual(before.connections); + expect(after.grants).toEqual(before.grants); + expect(after.activity).toEqual(before.activity); + for (const secret of before.secrets) expect(after.secrets).toContainEqual(secret); + for (const version of before.versions) expect(after.versions).toContainEqual(version); + const freshGrants = await db.select().from(connectionGrants) + .where(eq(connectionGrants.connectionId, fresh.body.connectionId)); + expect(freshGrants).toEqual([expect.objectContaining({ + kind: "user", subjectUserId: currentUserId, status: "active", + })]); + const oldSecretIds = before.grants.flatMap((grant) => + grant.credentialSecretRefs.map((ref) => ref.secretId)); + expect(freshGrants[0]!.credentialSecretRefs).toHaveLength(1); + expect(oldSecretIds).toHaveLength(1); + const freshSecretId = freshGrants[0]!.credentialSecretRefs[0]!.secretId; + expect(oldSecretIds).not.toContain(freshSecretId); + expect(after.secrets.find((secret) => secret.id === freshSecretId)) + .toMatchObject({ scope: "user", ownerUserId: currentUserId }); + await expect(db.select().from(activityLog) + .where(and(eq(activityLog.entityId, fresh.body.connectionId), + eq(activityLog.action, "tool_app.connected")))) + .resolves.toEqual([expect.objectContaining({ actorId: currentUserId })]); + }, "mcp-key"); + }, + ); + it("reconnects a personal key on the existing user grant without creating an organization credential", async () => { const company = await createCompany(db); const service = createTestToolAccessService(db); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 3e3f6292565..5b61834c94c 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -1692,8 +1692,8 @@ export function toolAccessService( // application/connection rows. A fresh gallery click has no // applicationId, so recover that retained identity by its company-unique // name and source instead of inserting a duplicate application that the - // unique index rejects. Active applications are excluded: connecting a - // second account still requires its own name/application identity. + // unique index rejects. Archived personal identities are excluded below. + // Active applications also require a separate application identity. const requestedName = input.name ?? galleryEntry?.name ?? defaultLinkName(input.link ?? ""); const recoverableApplicationStatuses = galleryEntry @@ -1841,6 +1841,19 @@ export function toolAccessService( retainedConnection = undefined; existingApplication = null; } + // A fresh setup must not revive a personal identity found only by name. + // Keep its application, grants, credentials, and history archived, and let + // normal name allocation create a separate application/connection below. + // Explicit reconnects still use the fixed personal-owner check. + if ( + !requestedConnectionId && + !input.applicationId && + retainedConnection?.status === "archived" && + retainedConnection.credentialPolicy === "per_user" + ) { + existingApplication = null; + retainedConnection = undefined; + } let applicationName = existingApplication?.name ?? requestedName; let name = retainedConnection?.name ?? requestedName; if (!existingApplication) { @@ -1876,8 +1889,8 @@ export function toolAccessService( : "organization" : null; // An explicit resume/application reconnect continues the retained identity. - // A fresh gallery connect may still reuse an archived row for stable history - // and company-unique naming, but an explicit Access choice is a new identity + // A fresh gallery connect may still reuse an archived non-personal row for + // stable history, but an explicit Access choice is a new identity // decision and must replace the archived policy. Without this distinction, // removing a dedicated-agent connection and reconnecting the default personal // account leaves `per_agent` behind and the OAuth callback cannot persist its