From d949314707bd65ab9d53ea5ae2d74e1e0a421deb Mon Sep 17 00:00:00 2001 From: 3mdistal <86723305+3mdistal@users.noreply.github.com> Date: Fri, 23 Oct 2026 01:44:07 -0400 Subject: [PATCH 1/7] Enforce comment access and workspace lookup rules --- .changeset/quiet-org-lookup-denial.md | 5 + packages/core/src/org/context.spec.ts | 13 +- packages/core/src/org/context.ts | 3 + ...credential-provider-org-resolution.spec.ts | 96 +++++++++++++ .../clips/actions/delete-comment.test.ts | 128 ++++++++++++------ templates/clips/actions/delete-comment.ts | 30 +--- templates/clips/actions/list-comments.test.ts | 24 ++++ ...letion-follow-recording-permissions-and.md | 6 + templates/clips/server/lib/comment-access.ts | 4 +- 9 files changed, 239 insertions(+), 70 deletions(-) create mode 100644 .changeset/quiet-org-lookup-denial.md create mode 100644 packages/core/src/server/credential-provider-org-resolution.spec.ts create mode 100644 templates/clips/changelog/2026-10-23-comment-access-and-deletion-follow-recording-permissions-and.md diff --git a/.changeset/quiet-org-lookup-denial.md b/.changeset/quiet-org-lookup-denial.md new file mode 100644 index 00000000000..bc19e18206b --- /dev/null +++ b/.changeset/quiet-org-lookup-denial.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Reject unavailable organization scope instead of treating failed membership reads as Personal access during credential resolution. diff --git a/packages/core/src/org/context.spec.ts b/packages/core/src/org/context.spec.ts index 945ac873ec8..43a6a113b04 100644 --- a/packages/core/src/org/context.spec.ts +++ b/packages/core/src/org/context.spec.ts @@ -1303,11 +1303,16 @@ describe("resolveOrgIdForEmail", () => { expect(mockExecute.mock.calls[0][0].args).toEqual(["mixed@case.com"]); }); - it("returns null on a DB error (missing tables) rather than throwing", async () => { - mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), + it.each([ + new Error('relation "org_members" does not exist'), + Object.assign(new Error("permission denied for table org_members"), { + code: "42501", + }), + ])("throws when organization scope is unavailable: %s", async (error) => { + mockExecute.mockRejectedValueOnce(error); + await expect(resolveOrgIdForEmail("a@b.com")).rejects.toThrow( + "Organization scope is unavailable", ); - expect(await resolveOrgIdForEmail("a@b.com")).toBeNull(); }); it("throws when org_members is unreadable, so callers cannot mistake it for 'no org'", async () => { diff --git a/packages/core/src/org/context.ts b/packages/core/src/org/context.ts index 7163847b379..115a13bd90d 100644 --- a/packages/core/src/org/context.ts +++ b/packages/core/src/org/context.ts @@ -727,6 +727,9 @@ export async function resolveOrgIdForEmail( email: string, ): Promise { const resolution = await resolveOrgScopeForEmail(email); + if (resolution.status === "unknown") { + throw new Error("Organization scope is unavailable"); + } return resolution.status === "organization" ? resolution.orgId : null; } diff --git a/packages/core/src/server/credential-provider-org-resolution.spec.ts b/packages/core/src/server/credential-provider-org-resolution.spec.ts new file mode 100644 index 00000000000..3d515e6c5c8 --- /dev/null +++ b/packages/core/src/server/credential-provider-org-resolution.spec.ts @@ -0,0 +1,96 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const fixture = vi.hoisted(() => ({ + membershipUnavailable: false, + restricted: true, + email: "member@example.test", + orgId: "restricted-org", + personalKey: "fake-personal-provider-key", +})); + +vi.mock("../db/client.js", async (importOriginal) => ({ + ...(await importOriginal()), + getDbExec: () => ({ + execute: async ({ sql }: { sql: string }) => { + if (sql.includes("SELECT org_id FROM org_members")) { + if (fixture.membershipUnavailable) { + throw Object.assign( + new Error("permission denied for table org_members"), + { code: "42501" }, + ); + } + return { rows: [{ org_id: fixture.orgId }] }; + } + if (sql.includes("SELECT role FROM org_members")) { + return { rows: [{ role: "member" }] }; + } + throw new Error(`Unexpected query: ${sql}`); + }, + }), +})); + +vi.mock("../settings/store.js", async (importOriginal) => ({ + ...(await importOriginal()), + getSetting: async (key: string) => { + if (key === `u:${fixture.email}:active-org-id`) return null; + if (key === `o:${fixture.orgId}:restrict-personal-provider-keys`) { + return { restricted: fixture.restricted }; + } + throw new Error(`Unexpected setting: ${key}`); + }, +})); + +vi.mock("../secrets/storage.js", () => ({ + readAppSecret: async (ref: { key: string; scope: string; scopeId: string }) => + ref.key === "ANTHROPIC_API_KEY" && + ref.scope === "user" && + ref.scopeId === fixture.email + ? { value: fixture.personalKey } + : null, +})); + +import { resolveSecretDetailed } from "./credential-provider.js"; +import { runWithRequestContext } from "./request-context.js"; + +function resolvePersonalProviderKey() { + return runWithRequestContext({ userEmail: fixture.email }, () => + resolveSecretDetailed("ANTHROPIC_API_KEY"), + ); +} + +describe("credential resolution with real org scope and personal-key policy", () => { + beforeEach(() => { + fixture.membershipUnavailable = false; + fixture.restricted = true; + vi.stubEnv("ANTHROPIC_API_KEY", ""); + vi.stubEnv("AGENT_VAULT_ORG_ID", ""); + }); + + afterEach(() => vi.unstubAllEnvs()); + + it("reports a failed lookup without returning a personal key when membership access is denied", async () => { + fixture.membershipUnavailable = true; + + await expect(resolvePersonalProviderKey()).resolves.toMatchObject({ + value: null, + lookupFailed: true, + }); + }); + + it("skips the restricted member's readable personal key when membership is readable", async () => { + await expect(resolvePersonalProviderKey()).resolves.toMatchObject({ + value: null, + lookupFailed: false, + }); + }); + + it("returns the same readable personal key when the organization allows it", async () => { + fixture.restricted = false; + + await expect(resolvePersonalProviderKey()).resolves.toMatchObject({ + value: fixture.personalKey, + lookupFailed: false, + source: "user", + }); + }); +}); diff --git a/templates/clips/actions/delete-comment.test.ts b/templates/clips/actions/delete-comment.test.ts index f4d2175c05c..9fc221fce97 100644 --- a/templates/clips/actions/delete-comment.test.ts +++ b/templates/clips/actions/delete-comment.test.ts @@ -96,7 +96,10 @@ import action from "./delete-comment"; beforeEach(() => { vi.resetAllMocks(); - mockAssertAccess.mockResolvedValue({ role: "viewer", resource: {} }); + mockAssertAccess.mockResolvedValue({ + role: "viewer", + resource: { enableComments: true }, + }); mockGetUserEmail.mockReturnValue("author@example.com"); state.rows = [ { @@ -158,47 +161,94 @@ describe("delete-comment", () => { }, ); - it.each([true, false])( - "preserves owner cleanup in Trash with enableComments=%s", - async (enableComments) => { - mockGetUserEmail.mockReturnValue("owner@example.com"); - mockAssertAccess.mockResolvedValue({ - role: "owner", - resource: { enableComments, trashedAt: "2026-01-01T00:00:00Z" }, - }); - const result = await action.run(action.schema.parse({ id: "comment-1" })); - expect(result.deletedCommentIds).toEqual([ - "comment-1", - "comment-1-reply", - "comment-1-reply-2", - ]); - }, - ); + it("preserves owner cleanup in Trash with comments enabled", async () => { + mockGetUserEmail.mockReturnValue("owner@example.com"); + mockAssertAccess.mockResolvedValue({ + role: "owner", + resource: { enableComments: true, trashedAt: "2026-01-01T00:00:00Z" }, + }); + const result = await action.run(action.schema.parse({ id: "comment-1" })); + expect(result.deletedCommentIds).toEqual([ + "comment-1", + "comment-1-reply", + "comment-1-reply-2", + ]); + }); + + it("deletes a comment and descendants with comments enabled", async () => { + mockAssertAccess.mockResolvedValue({ + role: "viewer", + resource: { enableComments: true, trashedAt: null }, + }); + const result = await action.run(action.schema.parse({ id: "comment-1" })); - it.each([true, false])( - "deletes a comment and descendants with enableComments=%s", - async (enableComments) => { + expect(result).toEqual({ + id: "comment-1", + deletedCommentIds: ["comment-1", "comment-1-reply", "comment-1-reply-2"], + }); + expect(state.rows.map((row) => row.id)).toEqual([ + "comment-2", + "comment-1-cross-record-reply", + ]); + expect(mockWriteAppState).toHaveBeenCalledWith("refresh-signal", { + ts: expect.any(Number), + }); + }); + + it.each(["viewer", "editor", "owner"])( + "refuses deletion with comments disabled for %s", + async (role) => { mockAssertAccess.mockResolvedValue({ - role: "viewer", - resource: { enableComments, trashedAt: null }, - }); - const result = await action.run(action.schema.parse({ id: "comment-1" })); - - expect(result).toEqual({ - id: "comment-1", - deletedCommentIds: [ - "comment-1", - "comment-1-reply", - "comment-1-reply-2", - ], - }); - expect(state.rows.map((row) => row.id)).toEqual([ - "comment-2", - "comment-1-cross-record-reply", - ]); - expect(mockWriteAppState).toHaveBeenCalledWith("refresh-signal", { - ts: expect.any(Number), + role, + resource: { enableComments: false }, }); + const originalRows = [...state.rows]; + await expect(action.run({ id: "comment-1" })).rejects.toBeInstanceOf( + ForbiddenError, + ); + expect(state.rows).toEqual(originalRows); + expect(mockWriteAppState).not.toHaveBeenCalled(); }, ); + + it("recognizes the author regardless of email case", async () => { + state.rows[0].authorEmail = "Author@Example.com"; + mockGetUserEmail.mockReturnValue("author@example.COM"); + mockAssertAccess.mockImplementation(async (_type, _id, role) => { + if (role === "editor") throw new ForbiddenError(); + return { role: "viewer", resource: { enableComments: true } }; + }); + expect(await action.run({ id: "comment-1" })).toMatchObject({ + id: "comment-1", + }); + expect(state.rows.map((row) => row.id)).not.toContain("comment-1"); + }); + + it("still refuses deletion by an unrelated viewer", async () => { + mockGetUserEmail.mockReturnValue("unrelated@example.com"); + mockAssertAccess.mockImplementation(async (_type, _id, role) => { + if (role === "editor") throw new ForbiddenError(); + return { role: "viewer", resource: { enableComments: true } }; + }); + const originalRows = [...state.rows]; + await expect(action.run({ id: "comment-1" })).rejects.toBeInstanceOf( + ForbiddenError, + ); + expect(state.rows).toEqual(originalRows); + }); + + it("preserves deletion by a recording editor with comments enabled", async () => { + mockGetUserEmail.mockReturnValue("editor@example.com"); + mockAssertAccess.mockResolvedValue({ + role: "editor", + resource: { enableComments: true }, + }); + expect(await action.run({ id: "comment-1" })).toMatchObject({ + id: "comment-1", + }); + expect(state.rows.map((row) => row.id)).toEqual([ + "comment-2", + "comment-1-cross-record-reply", + ]); + }); }); diff --git a/templates/clips/actions/delete-comment.ts b/templates/clips/actions/delete-comment.ts index a28ff6b931b..10bcba12dc6 100644 --- a/templates/clips/actions/delete-comment.ts +++ b/templates/clips/actions/delete-comment.ts @@ -6,10 +6,8 @@ import { and, eq, inArray } from "drizzle-orm"; import { z } from "zod"; import { getDb, schema } from "../server/db/index.js"; -import { - isRecordingExpiredForViewer, - isRecordingTrashedForViewer, -} from "../server/lib/recording-page-access.js"; +import { assertCommentAccess } from "../server/lib/comment-access.js"; +import { sameOwnerEmail } from "../server/lib/recordings.js"; export default defineAction({ description: @@ -27,29 +25,9 @@ export default defineAction({ if (!existing) throw new Error(`Comment not found: ${args.id}`); const userEmail = getRequestUserEmail(); - const isAuthor = !!userEmail && existing.authorEmail === userEmail; + const isAuthor = sameOwnerEmail(existing.authorEmail, userEmail); - const access = await assertAccess( - "recording", - existing.recordingId, - "viewer", - ); - if ( - isRecordingTrashedForViewer({ - trashedAt: (access.resource as { trashedAt?: string | null }).trashedAt, - viewerIsOwner: access.role === "owner", - }) - ) { - throw new ForbiddenError(); - } - if ( - isRecordingExpiredForViewer({ - expiresAt: (access.resource as { expiresAt?: string }).expiresAt, - viewerIsOwner: access.role === "owner", - }) - ) { - throw new ForbiddenError("Recording has expired"); - } + await assertCommentAccess(existing.recordingId); if (!isAuthor) { try { diff --git a/templates/clips/actions/list-comments.test.ts b/templates/clips/actions/list-comments.test.ts index db3590d4dab..0116fc7924a 100644 --- a/templates/clips/actions/list-comments.test.ts +++ b/templates/clips/actions/list-comments.test.ts @@ -42,6 +42,22 @@ beforeEach(() => { describe("list-comments recording policy", () => { it.each([ { role: "viewer", resource: { enableComments: false } }, + { + role: "viewer", + resource: { + enableComments: true, + visibility: "public", + password: "fake-share-password", + }, + }, + { + role: "editor", + resource: { + enableComments: true, + visibility: "public", + password: "fake-share-password", + }, + }, { role: "viewer", resource: { enableComments: true, trashedAt: "2026-10-22T12:00:00.000Z" }, @@ -67,6 +83,14 @@ describe("list-comments recording policy", () => { it.each([ { role: "owner", resource: { enableComments: false } }, + { + role: "owner", + resource: { + enableComments: false, + visibility: "public", + password: "fake-share-password", + }, + }, { role: "owner", resource: { enableComments: true, trashedAt: "2026-10-22T12:00:00.000Z" }, diff --git a/templates/clips/changelog/2026-10-23-comment-access-and-deletion-follow-recording-permissions-and.md b/templates/clips/changelog/2026-10-23-comment-access-and-deletion-follow-recording-permissions-and.md new file mode 100644 index 00000000000..756366d9e6f --- /dev/null +++ b/templates/clips/changelog/2026-10-23-comment-access-and-deletion-follow-recording-permissions-and.md @@ -0,0 +1,6 @@ +--- +type: fixed +date: 2026-10-23 +--- + +Comment access and deletion follow recording permissions and settings consistently. diff --git a/templates/clips/server/lib/comment-access.ts b/templates/clips/server/lib/comment-access.ts index 371281c5383..f2884c55516 100644 --- a/templates/clips/server/lib/comment-access.ts +++ b/templates/clips/server/lib/comment-access.ts @@ -13,6 +13,7 @@ export async function assertCommentAccess( const access = await assertAccess("recording", recordingId, "viewer"); const recording = access.resource as { enableComments: boolean; + password?: string | null; expiresAt?: string | null; trashedAt?: string | null; }; @@ -36,7 +37,8 @@ export async function assertCommentAccess( } if ( operation === "read" - ? !canReadRecordingComments({ + ? (Boolean(recording.password) && access.role !== "owner") || + !canReadRecordingComments({ enableComments: recording.enableComments, viewerIsOwner: access.role === "owner", }) From 2179302d8a220afcaa2841b5a78ec75b5a9651fe Mon Sep 17 00:00:00 2001 From: 3mdistal <86723305+3mdistal@users.noreply.github.com> Date: Fri, 23 Oct 2026 02:21:15 -0400 Subject: [PATCH 2/7] Preserve organization lookup failures at callers --- .changeset/quiet-org-lookup-denial.md | 2 +- packages/core/src/mcp/build-server.ts | 4 +- .../src/mcp/build-server.verify-auth.spec.ts | 43 +++++ packages/core/src/mcp/server.spec.ts | 55 +++++++ packages/core/src/mcp/server.ts | 8 +- packages/core/src/org/context.spec.ts | 10 +- packages/core/src/org/context.ts | 23 ++- .../core/src/org/request-org-cache.spec.ts | 37 +++-- packages/core/src/org/request-org-cache.ts | 11 +- .../core/src/server/action-routes.spec.ts | 71 ++++---- .../agent-teams-delegation-depth.spec.ts | 154 +++++++++++++++++- packages/core/src/server/agent-teams.ts | 12 +- 12 files changed, 341 insertions(+), 89 deletions(-) diff --git a/.changeset/quiet-org-lookup-denial.md b/.changeset/quiet-org-lookup-denial.md index bc19e18206b..5b7dc1e261d 100644 --- a/.changeset/quiet-org-lookup-denial.md +++ b/.changeset/quiet-org-lookup-denial.md @@ -2,4 +2,4 @@ "@agent-native/core": patch --- -Reject unavailable organization scope instead of treating failed membership reads as Personal access during credential resolution. +Reject unavailable organization scope instead of treating failed membership reads as Personal access during credential resolution, MCP identity setup, or background task creation. Preserve the narrow first-boot missing-membership-table exception for action dispatch. diff --git a/packages/core/src/mcp/build-server.ts b/packages/core/src/mcp/build-server.ts index ce4ab533548..9a07ba13c53 100644 --- a/packages/core/src/mcp/build-server.ts +++ b/packages/core/src/mcp/build-server.ts @@ -4473,7 +4473,7 @@ export async function resolveMcpIdentityOrgId( try { const { resolveOrgIdForEmail } = await import("../org/context.js"); return (await resolveOrgIdForEmail(userEmail)) ?? undefined; - } catch { - return undefined; + } catch (cause) { + throw new McpIdentityVerificationUnavailableError(cause); } } diff --git a/packages/core/src/mcp/build-server.verify-auth.spec.ts b/packages/core/src/mcp/build-server.verify-auth.spec.ts index d88668912d9..f61b523492e 100644 --- a/packages/core/src/mcp/build-server.verify-auth.spec.ts +++ b/packages/core/src/mcp/build-server.verify-auth.spec.ts @@ -1668,6 +1668,8 @@ describe("verifyAuth — a credential for an address an email change retired", ( }); describe("resolveMcpIdentityOrgId", () => { + afterEach(() => vi.restoreAllMocks()); + beforeEach(() => { vi.clearAllMocks(); lookupOrgByDomainMock.mockResolvedValue({ status: "not-found" }); @@ -1767,4 +1769,45 @@ describe("resolveMcpIdentityOrgId", () => { expect(resolveOrgIdForEmailMock).toHaveBeenCalledWith("alice@example.com"); }); + + it.each([ + ['relation "org_members" does not exist', "42P01"], + ["permission denied for table org_members", "42501"], + ["membership lookup failed", undefined], + ] as const)( + "keeps real email membership failures unavailable: %s", + async (message, code) => { + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(org.resolveOrgIdForEmail); + const db = await import("../db/client.js"); + const execute = vi + .fn() + .mockRejectedValue(Object.assign(new Error(message), { code })); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + + await expect( + resolveMcpIdentityOrgId({ userEmail: "alice@example.test" }), + ).rejects.toBeInstanceOf(McpIdentityVerificationUnavailableError); + expect(execute).toHaveBeenCalledOnce(); + }, + ); + + it("preserves Personal after a successful real membership read without rows", async () => { + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(org.resolveOrgIdForEmail); + const db = await import("../db/client.js"); + const execute = vi.fn().mockResolvedValue({ rows: [] }); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + + await expect( + resolveMcpIdentityOrgId({ userEmail: "alice@example.test" }), + ).resolves.toBeUndefined(); + expect(execute).toHaveBeenCalledOnce(); + }); }); diff --git a/packages/core/src/mcp/server.spec.ts b/packages/core/src/mcp/server.spec.ts index 9093db8895e..4ccd5f58ef8 100644 --- a/packages/core/src/mcp/server.spec.ts +++ b/packages/core/src/mcp/server.spec.ts @@ -10195,6 +10195,61 @@ describe("handleMcpRequest — web-standard runtime fallback (no Node req/res)", expect(result).toMatchObject({ error: "Service Unavailable" }); }); + it.each(["unavailable", "personal"] as const)( + "preserves the email-only MCP scope result over HTTP: %s", + async (scope) => { + process.env.BETTER_AUTH_SECRET = + "oauth-secret-at-least-32-characters-long"; + const { signMcpOAuthAccessToken } = await import("./oauth-token.js"); + const token = await signMcpOAuthAccessToken({ + ownerEmail: "email-only@example.test", + clientId: "client-123", + scope: "mcp:read", + resource: "https://mail.agent-native.com/mcp", + issuer: "https://mail.agent-native.com", + }); + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(org.resolveOrgIdForEmail); + const db = await import("../db/client.js"); + const realDb = db.getDbExec(); + const execute = vi.fn(async (query: { sql: string; args: unknown[] }) => { + if (!query.sql.includes("SELECT org_id FROM org_members")) + return realDb.execute(query); + if (scope === "unavailable") + throw Object.assign( + new Error("permission denied for table org_members"), + { code: "42501" }, + ); + return { rows: [] }; + }); + const dbSpy = vi + .spyOn(db, "getDbExec") + .mockReturnValue({ execute } as any); + const event = makeWebEvent({ + method: "POST", + body: { jsonrpc: "2.0", id: 15, method: "tools/list", params: {} }, + headers: { authorization: `Bearer ${token}` }, + }); + try { + const result = await handleMcpRequest(event, config as any); + expect(result instanceof Response ? result.status : event._status).toBe( + scope === "unavailable" ? 503 : 200, + ); + expect(event._responseHeaders?.["www-authenticate"]).toBeUndefined(); + if (scope === "unavailable") { + expect(event._responseHeaders?.["retry-after"]).toBe("5"); + expect(result).toMatchObject({ error: "Service Unavailable" }); + } + expect(resolveOrgIdForEmailMock).toHaveBeenCalledOnce(); + } finally { + dbSpy.mockRestore(); + } + }, + ); + it.each([ ["unknown", { status: "not-found" }], ["ambiguous", { status: "ambiguous" }], diff --git a/packages/core/src/mcp/server.ts b/packages/core/src/mcp/server.ts index dd70032e7c8..ff45e58d776 100644 --- a/packages/core/src/mcp/server.ts +++ b/packages/core/src/mcp/server.ts @@ -423,14 +423,16 @@ async function handleMcpRequestInternal( } if ( - authResult.identity?.orgId === undefined && - authResult.identity?.orgDomain !== undefined + authResult.identity && + authResult.identity.orgId === undefined && + (authResult.identity.orgDomain !== undefined || + Boolean(authResult.identity.userEmail)) ) { try { const orgId = await resolveMcpIdentityOrgId(authResult.identity); authResult = { ...authResult, - identity: { ...authResult.identity, orgId }, + identity: { ...authResult.identity, orgId: orgId ?? null }, }; } catch (error) { if (error instanceof McpIdentityVerificationUnavailableError) { diff --git a/packages/core/src/org/context.spec.ts b/packages/core/src/org/context.spec.ts index 43a6a113b04..5bdc5c67ff2 100644 --- a/packages/core/src/org/context.spec.ts +++ b/packages/core/src/org/context.spec.ts @@ -1255,11 +1255,11 @@ describe("resolveOrgIdForEmail", () => { }); it("keeps scope unknown when the membership query is unavailable", async () => { - mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), - ); + const cause = new Error('relation "org_members" does not exist'); + mockExecute.mockRejectedValueOnce(cause); await expect(resolveOrgScopeForEmail("nobody@b.com")).resolves.toEqual({ status: "unknown", + cause, }); }); @@ -1310,9 +1310,7 @@ describe("resolveOrgIdForEmail", () => { }), ])("throws when organization scope is unavailable: %s", async (error) => { mockExecute.mockRejectedValueOnce(error); - await expect(resolveOrgIdForEmail("a@b.com")).rejects.toThrow( - "Organization scope is unavailable", - ); + await expect(resolveOrgIdForEmail("a@b.com")).rejects.toBe(error); }); it("throws when org_members is unreadable, so callers cannot mistake it for 'no org'", async () => { diff --git a/packages/core/src/org/context.ts b/packages/core/src/org/context.ts index 115a13bd90d..e5d04d918ea 100644 --- a/packages/core/src/org/context.ts +++ b/packages/core/src/org/context.ts @@ -682,7 +682,7 @@ async function loadMembershipsUncached( export type OrgScopeResolution = | { status: "organization"; orgId: string } | { status: "personal" } - | { status: "unknown" }; + | { status: "unknown"; cause?: unknown }; export async function resolveOrgScopeForEmail( email: string, @@ -698,18 +698,22 @@ export async function resolveOrgScopeForEmail( if (getRequestContext()?.orgScope === "personal") { return { status: "personal" }; } - const idsPromise = requestMemberOrgIds(email, async () => { - const rows = await queryOrgMembers({ - sql: `SELECT org_id FROM org_members + let ids: string[]; + try { + ids = await requestMemberOrgIds(email, async () => { + const { rows } = await getDbExec().execute({ + sql: `SELECT org_id FROM org_members WHERE LOWER(email) = ? AND federation_removal_pending_at IS NULL ${MEMBERSHIP_FALLBACK_ORDER_BY}`, - args: [email.toLowerCase()], + args: [email.toLowerCase()], + }); + return rows.map((r: any) => String(r.org_id)); }); - return rows?.map((r: any) => String(r.org_id)) ?? null; - }); - const ids = await idsPromise; - if (ids === null) return { status: "unknown" }; + } catch (cause) { + if (isTransientDatabaseError(cause)) throw cause; + return { status: "unknown", cause }; + } if (ids.length === 0) return { status: "personal" }; const activeOrgSetting = (await getUserSetting( @@ -728,6 +732,7 @@ export async function resolveOrgIdForEmail( ): Promise { const resolution = await resolveOrgScopeForEmail(email); if (resolution.status === "unknown") { + if (resolution.cause !== undefined) throw resolution.cause; throw new Error("Organization scope is unavailable"); } return resolution.status === "organization" ? resolution.orgId : null; diff --git a/packages/core/src/org/request-org-cache.spec.ts b/packages/core/src/org/request-org-cache.spec.ts index 0cfef6f898e..8dda800309f 100644 --- a/packages/core/src/org/request-org-cache.spec.ts +++ b/packages/core/src/org/request-org-cache.spec.ts @@ -131,25 +131,28 @@ describe("per-request org membership memo", () => { expect(results).toEqual(["org1", "org2"]); }); - it("evicts a transient failure instead of memoizing it", async () => { - mockExecute.mockRejectedValueOnce( - Object.assign(new Error("connection failure"), { code: "08006" }), - ); + it.each(["08006", "42501", undefined])( + "evicts a failed read instead of memoizing it: %s", + async (code) => { + mockExecute.mockRejectedValueOnce( + Object.assign(new Error("membership lookup failed"), { code }), + ); - const result = await runWithRequestContext( - { userEmail: "alice@builder.io" }, - async () => { - await expect( - resolveOrgIdForEmail("alice@builder.io"), - ).rejects.toThrow(); - mockExecute.mockResolvedValue({ rows: [{ org_id: "org1" }] }); - return resolveOrgIdForEmail("alice@builder.io"); - }, - ); + const result = await runWithRequestContext( + { userEmail: "alice@builder.io" }, + async () => { + await expect( + resolveOrgIdForEmail("alice@builder.io"), + ).rejects.toThrow(); + mockExecute.mockResolvedValue({ rows: [{ org_id: "org1" }] }); + return resolveOrgIdForEmail("alice@builder.io"); + }, + ); - expect(result).toBe("org1"); - expect(memberRowQueries()).toHaveLength(2); - }); + expect(result).toBe("org1"); + expect(memberRowQueries()).toHaveLength(2); + }, + ); }); describe("cross-request membership cache", () => { diff --git a/packages/core/src/org/request-org-cache.ts b/packages/core/src/org/request-org-cache.ts index 8790c29cdad..774efe3a5c3 100644 --- a/packages/core/src/org/request-org-cache.ts +++ b/packages/core/src/org/request-org-cache.ts @@ -20,14 +20,11 @@ import { createTtlCache } from "../shared/ttl-cache.js"; * run owner or credential subject), so a context-only key would answer one * identity with another's memberships. */ -const requestOrgIds = new WeakMap< - object, - Map> ->(); +const requestOrgIds = new WeakMap>>(); function cacheForRequest( create: boolean, -): Map> | null { +): Map> | null { const ctx = getRequestContext(); if (!ctx || typeof ctx !== "object") return null; let cache = requestOrgIds.get(ctx); @@ -40,8 +37,8 @@ function cacheForRequest( export function requestMemberOrgIds( email: string, - load: () => Promise, -): Promise { + load: () => Promise, +): Promise { const cache = cacheForRequest(true); if (!cache) return load(); const key = email.trim().toLowerCase(); diff --git a/packages/core/src/server/action-routes.spec.ts b/packages/core/src/server/action-routes.spec.ts index 065cefde274..c6a0043299c 100644 --- a/packages/core/src/server/action-routes.spec.ts +++ b/packages/core/src/server/action-routes.spec.ts @@ -6074,42 +6074,55 @@ describe("mountActionRoutes", () => { expect(received.orgId).toBeNull(); }); - it.each(['relation "org_members" does not exist'])( - "suppresses the verified first-boot missing org table error: %s", - async (message) => { + it.each([ + ["first boot", 'relation "org_members" does not exist', "42P01", true], + ["denied", "permission denied for table org_members", "42501", false], + ["unknown", "membership lookup failed", undefined, false], + ["other table", 'relation "settings" does not exist', "42P01", false], + ] as const)( + "dispatches only the real first-boot resolver exception: %s", + async (_label, message, code, allowed) => { + const org = + await vi.importActual( + "../org/context.js", + ); + mockResolveOrgIdForEmail.mockImplementation(org.resolveOrgIdForEmail); + mockResolveOrgScopeForEmail.mockImplementation( + org.resolveOrgScopeForEmail, + ); + const db = await import("../db/client.js"); + const failure = Object.assign(new Error(message), { code }); + const execute = vi.fn().mockRejectedValue(failure); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); const { mountActionRoutes } = await import("./action-routes.js"); - mockResolveOrgIdForEmail.mockRejectedValue(new Error(message)); - const mounted: Array<{ path: string; handler: any }> = []; - const nitroApp = { - use: vi.fn((path: string, handler: any) => - mounted.push({ path, handler }), - ), - }; - let received: any; + const mounted: Array<{ handler: any }> = []; + const run = vi.fn(async () => getRequestContext()); mountActionRoutes( - nitroApp, - { - "do-thing": { - run: vi.fn(async (_params, ctx) => { - received = ctx; - return { ok: true }; - }), - } as any, - }, + { use: (_path: string, handler: any) => mounted.push({ handler }) }, + { "do-thing": { run } as any }, { - getOwnerFromEvent: async () => "steve@example.com", + getOwnerFromEvent: async () => "first-boot@example.test", resolveOrgId: async () => null, }, ); + const response = mounted[0]!.handler({ + _method: "POST", + _headers: { "x-agent-native-frontend": "1" }, + context: {}, + req: { json: async () => ({}) }, + }); - await expect( - mounted[0].handler({ - _method: "POST", - _headers: { "x-agent-native-frontend": "1" }, - req: { json: async () => ({}) }, - }), - ).resolves.toEqual({ ok: true }); - expect(received.orgId).toBeNull(); + if (allowed) { + await expect(response).resolves.toMatchObject({ + userEmail: "first-boot@example.test", + }); + expect(await response).not.toHaveProperty("orgScope", "personal"); + expect(run).toHaveBeenCalledOnce(); + } else { + await expect(response).rejects.toBe(failure); + expect(run).not.toHaveBeenCalled(); + } + expect(execute).toHaveBeenCalled(); }, ); diff --git a/packages/core/src/server/agent-teams-delegation-depth.spec.ts b/packages/core/src/server/agent-teams-delegation-depth.spec.ts index fb2f2882db9..60ed3b2398f 100644 --- a/packages/core/src/server/agent-teams-delegation-depth.spec.ts +++ b/packages/core/src/server/agent-teams-delegation-depth.spec.ts @@ -58,7 +58,8 @@ vi.mock("../progress/registry.js", () => ({ completeRun: vi.fn(async () => ({})), })); -vi.mock("./request-context.js", () => ({ +vi.mock("./request-context.js", async (importOriginal) => ({ + ...(await importOriginal()), getRequestOrgId: () => requestContextState.orgId, getRequestRunContext: () => ({ allowedActionNames: requestContextState.allowedActionNames, @@ -98,6 +99,7 @@ describe("agent-teams delegation-depth guardrail", () => { }); afterEach(() => { + vi.restoreAllMocks(); delete process.env.AGENT_NATIVE_MAX_SUBAGENT_DEPTH; }); @@ -188,16 +190,158 @@ describe("agent-teams delegation-depth guardrail", () => { }); it("preserves an explicitly org-less parent request", async () => { + const request = await import("./request-context.js"); + const actualRequest = await vi.importActual( + "./request-context.js", + ); + vi.spyOn(request, "hasRequestContext").mockImplementation( + actualRequest.hasRequestContext, + ); + vi.spyOn(request, "getRequestOrgId").mockImplementation( + actualRequest.getRequestOrgId, + ); + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(() => + org.resolveOrgIdForEmail(OWNER), + ); + const db = await import("../db/client.js"); + const execute = vi + .fn() + .mockRejectedValue( + new Error("Personal scope must not query memberships"), + ); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); const { spawnTask } = await import("./agent-teams.js"); - requestContextState.active = true; - requestContextState.orgId = undefined; - await spawnTask(baseSpawnOptions()); + await actualRequest.runWithRequestContext( + { userEmail: OWNER, orgScope: "personal" }, + () => spawnTask(baseSpawnOptions()), + ); expect(enqueueAgentTeamRunMock).toHaveBeenCalledWith( expect.objectContaining({ orgId: null }), ); - expect(resolveOrgIdForEmailMock).not.toHaveBeenCalled(); + expect(execute).not.toHaveBeenCalled(); + }); + + it("rejects an unresolved real parent request before creating resources", async () => { + const request = await import("./request-context.js"); + const actualRequest = await vi.importActual( + "./request-context.js", + ); + vi.spyOn(request, "hasRequestContext").mockImplementation( + actualRequest.hasRequestContext, + ); + vi.spyOn(request, "getRequestOrgId").mockImplementation( + actualRequest.getRequestOrgId, + ); + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(() => + org.resolveOrgIdForEmail(OWNER), + ); + const db = await import("../db/client.js"); + const execute = vi.fn().mockRejectedValue( + Object.assign(new Error("permission denied for table org_members"), { + code: "42501", + }), + ); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + const { spawnTask } = await import("./agent-teams.js"); + const parentSend = vi.fn(); + const [result] = await Promise.allSettled([ + actualRequest.runWithRequestContext({ userEmail: OWNER }, () => + spawnTask({ ...baseSpawnOptions(), parentSend }), + ), + ]); + expect({ + result: result.status, + threads: createThreadMock.mock.calls.length, + state: appState.size, + queued: enqueueAgentTeamRunMock.mock.calls.length, + dispatched: fireInternalDispatchMock.mock.calls.length, + notified: parentSend.mock.calls.length, + }).toEqual({ + result: "rejected", + threads: 0, + state: 0, + queued: 0, + dispatched: 0, + notified: 0, + }); + expect(execute).toHaveBeenCalledOnce(); + }); + + it.each([ + ['relation "org_members" does not exist', "42P01"], + ["permission denied for table org_members", "42501"], + ["membership lookup failed", undefined], + ] as const)( + "rejects real background org lookup failure before creating resources: %s", + async (message, code) => { + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(() => + org.resolveOrgIdForEmail(OWNER), + ); + const db = await import("../db/client.js"); + const execute = vi + .fn() + .mockRejectedValue(Object.assign(new Error(message), { code })); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + const { spawnTask } = await import("./agent-teams.js"); + const parentSend = vi.fn(); + const [result] = await Promise.allSettled([ + spawnTask({ ...baseSpawnOptions(), parentSend }), + ]); + + expect({ + result: result.status, + threads: createThreadMock.mock.calls.length, + state: appState.size, + queued: enqueueAgentTeamRunMock.mock.calls.length, + dispatched: fireInternalDispatchMock.mock.calls.length, + notified: parentSend.mock.calls.length, + }).toEqual({ + result: "rejected", + threads: 0, + state: 0, + queued: 0, + dispatched: 0, + notified: 0, + }); + expect(execute).toHaveBeenCalledOnce(); + }, + ); + + it("persists Personal after a successful real background membership read without rows", async () => { + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(() => + org.resolveOrgIdForEmail(OWNER), + ); + const db = await import("../db/client.js"); + const execute = vi.fn().mockResolvedValue({ rows: [] }); + vi.spyOn(db, "getDbExec").mockReturnValue({ execute } as any); + const { spawnTask } = await import("./agent-teams.js"); + + const task = await spawnTask(baseSpawnOptions()); + + expect(task.orgId).toBeNull(); + expect(appState.get(`agent-task:${task.taskId}`)?.orgId).toBeNull(); + expect(enqueueAgentTeamRunMock).toHaveBeenCalledWith( + expect.objectContaining({ orgId: null }), + ); + expect(execute).toHaveBeenCalledOnce(); }); it("scopes a spawned thread to the parent app", async () => { diff --git a/packages/core/src/server/agent-teams.ts b/packages/core/src/server/agent-teams.ts index df800487efa..0cfe62b2bb2 100644 --- a/packages/core/src/server/agent-teams.ts +++ b/packages/core/src/server/agent-teams.ts @@ -1864,6 +1864,8 @@ export async function spawnTask(opts: SpawnTaskOptions): Promise { throw new SubagentDelegationDepthError(decision); } const childDepth = decision.childDepth; + const requestOrgId = hasRequestContext() ? getRequestOrgId() : undefined; + const orgId = requestOrgId ?? (await resolveOrgIdForEmail(opts.ownerEmail)); const taskId = generateTaskId(); const userMsgId = `msg-${taskId}-user`; @@ -1895,16 +1897,6 @@ export async function spawnTask(opts: SpawnTaskOptions): Promise { const runId = taskRunId(taskId); const createdAt = Date.now(); - let orgId: string | null; - if (hasRequestContext()) { - orgId = getRequestOrgId() ?? null; - } else { - try { - orgId = await resolveOrgIdForEmail(opts.ownerEmail); - } catch { - orgId = null; - } - } const task: AgentTask = { taskId, threadId: thread.id, From eb0f827a6e46a72b244fb7be4dd8b20fc4c990a5 Mon Sep 17 00:00:00 2001 From: 3mdistal <86723305+3mdistal@users.noreply.github.com> Date: Fri, 23 Oct 2026 02:49:32 -0400 Subject: [PATCH 3/7] Reject unavailable organization scope during credential reads --- .changeset/quiet-org-lookup-denial.md | 2 +- .../credentials/index.org-resolution.spec.ts | 133 ++++++++++++++++++ packages/core/src/credentials/index.ts | 6 +- .../core/src/mcp/connector-catalog.spec.ts | 36 +++++ 4 files changed, 174 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/credentials/index.org-resolution.spec.ts diff --git a/.changeset/quiet-org-lookup-denial.md b/.changeset/quiet-org-lookup-denial.md index 5b7dc1e261d..1fb08c63f9b 100644 --- a/.changeset/quiet-org-lookup-denial.md +++ b/.changeset/quiet-org-lookup-denial.md @@ -2,4 +2,4 @@ "@agent-native/core": patch --- -Reject unavailable organization scope instead of treating failed membership reads as Personal access during credential resolution, MCP identity setup, or background task creation. Preserve the narrow first-boot missing-membership-table exception for action dispatch. +Reject unavailable organization scope instead of treating failed membership reads as Personal access during credential resolution, MCP identity setup, or background task creation. Generic credential reads also reject non-transient membership failures before returning a stored Personal credential whose organization priority could not be checked. Preserve the narrow first-boot missing-membership-table exception for action dispatch. diff --git a/packages/core/src/credentials/index.org-resolution.spec.ts b/packages/core/src/credentials/index.org-resolution.spec.ts new file mode 100644 index 00000000000..f27033965d9 --- /dev/null +++ b/packages/core/src/credentials/index.org-resolution.spec.ts @@ -0,0 +1,133 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const fixture = vi.hoisted(() => ({ + email: "owner@example.test", + orgId: "credential-org", + membershipError: null as Error | null, + membershipIds: ["credential-org"], + role: "owner", + personalSelected: false, + personalValue: "fake-personal-credential" as string | null, +})); + +vi.mock("../db/client.js", async (importOriginal) => ({ + ...(await importOriginal()), + getDbExec: () => ({ + execute: async ({ sql }: { sql: string }) => { + if (sql.includes("SELECT org_id FROM org_members")) { + if (fixture.membershipError) throw fixture.membershipError; + return { rows: fixture.membershipIds.map((org_id) => ({ org_id })) }; + } + if (sql.includes("SELECT role FROM org_members")) { + return { rows: [{ role: fixture.role }] }; + } + throw new Error(`Unexpected query: ${sql}`); + }, + }), +})); + +vi.mock("../settings/store.js", async (importOriginal) => ({ + ...(await importOriginal()), + getSetting: async (key: string) => { + if (key === `u:${fixture.email}:active-org-id`) { + return fixture.personalSelected ? { orgId: null } : null; + } + if (key === `u:${fixture.email}:credential:GENERIC_SERVICE_TOKEN`) { + return fixture.personalValue ? { value: fixture.personalValue } : null; + } + throw new Error(`Unexpected setting: ${key}`); + }, +})); + +vi.mock("../secrets/storage.js", () => ({ + readAppSecret: async (ref: { key: string; scope: string; scopeId: string }) => + ref.key === "GENERIC_SERVICE_TOKEN" && + ref.scope === "org" && + ref.scopeId === fixture.orgId + ? { value: "fake-org-credential" } + : null, +})); + +import { CredentialStoreUnavailableError } from "../server/credential-provider.js"; +import { runWithRequestContext } from "../server/request-context.js"; +import { resolveCredentialDetailed } from "./index.js"; + +function resolveGenericCredential() { + return runWithRequestContext({ userEmail: fixture.email }, () => + resolveCredentialDetailed("GENERIC_SERVICE_TOKEN", { + userEmail: fixture.email, + }), + ); +} + +describe("generic credentials with real organization resolution", () => { + beforeEach(() => { + fixture.membershipError = null; + fixture.membershipIds = [fixture.orgId]; + fixture.role = "owner"; + fixture.personalSelected = false; + fixture.personalValue = "fake-personal-credential"; + }); + + it.each(["42501", "08006"])( + "rejects a readable personal credential when membership lookup fails with %s", + async (code) => { + fixture.membershipError = Object.assign( + new Error( + code === "42501" + ? "permission denied for table org_members" + : "connection reset", + ), + { code }, + ); + + const lookup = resolveGenericCredential(); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup).rejects.toMatchObject({ + cause: fixture.membershipError, + retryable: true, + }); + }, + ); + + it("returns a readable personal credential when membership confirms no organization", async () => { + fixture.membershipIds = []; + + await expect(resolveGenericCredential()).resolves.toEqual({ + value: fixture.personalValue, + scope: "user", + scopeId: fixture.email, + }); + }); + + it("keeps an explicit Personal selection even when the user has an organization", async () => { + fixture.personalSelected = true; + + await expect(resolveGenericCredential()).resolves.toMatchObject({ + value: fixture.personalValue, + scope: "user", + }); + }); + + it("preserves genuine credential absence in Personal scope", async () => { + fixture.membershipIds = []; + fixture.personalValue = null; + + await expect(resolveGenericCredential()).resolves.toBeUndefined(); + }); + + it.each([ + ["owner", "fake-org-credential", "org"], + ["admin", "fake-org-credential", "org"], + ["member", "fake-personal-credential", "user"], + ])("preserves %s credential priority", async (role, value, scope) => { + fixture.role = role; + + await expect(resolveGenericCredential()).resolves.toMatchObject({ + value, + scope, + }); + }); +}); diff --git a/packages/core/src/credentials/index.ts b/packages/core/src/credentials/index.ts index 57d2f929dc3..0934e5c6363 100644 --- a/packages/core/src/credentials/index.ts +++ b/packages/core/src/credentials/index.ts @@ -10,7 +10,7 @@ import { readAppSecret, type SecretRef, } from "../secrets/storage.js"; -import { assertCredentialStoreReadable } from "../server/credential-provider.js"; +import { CredentialStoreUnavailableError } from "../server/credential-provider.js"; import { isPersonalCredentialScope, readsOrgCredentialFirst, @@ -257,7 +257,9 @@ export async function resolveCredentialDetailed( if (ctx.credentialScope === "org" && !ctx.orgId) return undefined; const orgLookup = await resolveEffectiveOrgId(ctx); // Before the personal answer: without the org, an owner's role is unknown. - assertCredentialStoreReadable(orgLookup); + if (orgLookup.lookupFailed) { + throw new CredentialStoreUnavailableError(orgLookup.cause); + } const { orgId } = orgLookup; // An owner or admin runs on the organization's credential ahead of their own. if (personal && !(await readsOrgCredentialFirst(orgId, ctx.userEmail))) { diff --git a/packages/core/src/mcp/connector-catalog.spec.ts b/packages/core/src/mcp/connector-catalog.spec.ts index 0f4401141db..9fe2ec88f90 100644 --- a/packages/core/src/mcp/connector-catalog.spec.ts +++ b/packages/core/src/mcp/connector-catalog.spec.ts @@ -13,6 +13,10 @@ const issuedConnectTokens = vi.hoisted( >(), ); +const resolveOrgIdForEmail = vi.hoisted(() => + vi.fn(async (_email: string): Promise => null), +); + vi.mock("./builtin-tools.js", () => ({ getBuiltinCrossAppTools: () => ({ list_apps: { @@ -79,6 +83,7 @@ vi.mock("./builtin-tools.js", () => ({ vi.mock("../org/context.js", () => ({ resolveOrgByDomain: vi.fn(async () => null), getA2ASecretByDomain: vi.fn(async () => null), + resolveOrgIdForEmail, })); vi.mock("../server/better-auth-instance.js", async (importOriginal) => ({ @@ -102,6 +107,7 @@ vi.mock("./connect-store.js", () => ({ beforeEach(() => { issuedConnectTokens.clear(); + resolveOrgIdForEmail.mockReset().mockResolvedValue(null); }); vi.mock("../server/embed-session.js", () => ({ @@ -980,10 +986,40 @@ describe("connector-catalog tier", () => { const names: string[] = out.result.tools.map((t: any) => t.name); expect(names).toContain("db-exec"); expect(names).toContain("seed-kitchen-sink"); + expect(resolveOrgIdForEmail).toHaveBeenCalledExactlyOnceWith( + "alice@example.com", + ); } finally { delete process.env.BETTER_AUTH_SECRET; } }); + + it("keeps a full-catalog OAuth scope lookup failure retryable", async () => { + process.env.BETTER_AUTH_SECRET = OAUTH_SECRET; + resolveOrgIdForEmail.mockRejectedValueOnce( + Object.assign(new Error("permission denied for table org_members"), { + code: "42501", + }), + ); + const token = await signOAuthToken({ + catalogScope: "full", + clientId: "catalog-test-client", + }); + const event = makeEvent({ + headers: { authorization: `Bearer ${token}` }, + body: { jsonrpc: "2.0", id: 8, method: "tools/list", params: {} }, + }); + + const result = await handleMcpRequest(event, connectorConfig as any); + + expect(result).toMatchObject({ error: "Service Unavailable" }); + expect(event._status).toBe(503); + expect(event._responseHeaders?.["retry-after"]).toBe("5"); + expect(event._responseHeaders?.["www-authenticate"]).toBeUndefined(); + expect(resolveOrgIdForEmail).toHaveBeenCalledExactlyOnceWith( + "alice@example.com", + ); + }); }); describe("AGENT_NATIVE_MCP_FULL_CATALOG=1 overrides connector tier", () => { From 0a0b989211c95de74b7093ec9c6f6ee4ad92a3ea Mon Sep 17 00:00:00 2001 From: 3mdistal <86723305+3mdistal@users.noreply.github.com> Date: Fri, 23 Oct 2026 03:27:52 -0400 Subject: [PATCH 4/7] Preserve scope refusals and credential lookup errors --- .changeset/quiet-org-lookup-denial.md | 2 + .../credentials/index.org-resolution.spec.ts | 185 +++++++++++++++++- packages/core/src/credentials/index.ts | 47 +++-- .../credentials/personal-key-policy.spec.ts | 17 +- packages/core/src/mcp/build-server.ts | 4 + packages/core/src/mcp/server.spec.ts | 75 +++++++ packages/core/src/org/context.spec.ts | 76 ++++++- packages/core/src/org/context.ts | 35 ++-- .../core/src/server/agent-run-context.spec.ts | 12 ++ packages/core/src/server/agent-run-context.ts | 10 +- packages/core/src/server/auth.ts | 6 +- .../core/src/server/credential-provider.ts | 9 + 12 files changed, 422 insertions(+), 56 deletions(-) diff --git a/.changeset/quiet-org-lookup-denial.md b/.changeset/quiet-org-lookup-denial.md index 1fb08c63f9b..265008eed4f 100644 --- a/.changeset/quiet-org-lookup-denial.md +++ b/.changeset/quiet-org-lookup-denial.md @@ -3,3 +3,5 @@ --- Reject unavailable organization scope instead of treating failed membership reads as Personal access during credential resolution, MCP identity setup, or background task creation. Generic credential reads also reject non-transient membership failures before returning a stored Personal credential whose organization priority could not be checked. Preserve the narrow first-boot missing-membership-table exception for action dispatch. + +Keep revoked or mismatched delegated organization access a definitive authorization refusal, including MCP authentication and credential lookup wrappers. Resolve generic credentials' organization scope before provider policy checks and report failed policy or role reads as credential-store unavailability with the original cause. diff --git a/packages/core/src/credentials/index.org-resolution.spec.ts b/packages/core/src/credentials/index.org-resolution.spec.ts index f27033965d9..66c2f3d4241 100644 --- a/packages/core/src/credentials/index.org-resolution.spec.ts +++ b/packages/core/src/credentials/index.org-resolution.spec.ts @@ -6,6 +6,8 @@ const fixture = vi.hoisted(() => ({ membershipError: null as Error | null, membershipIds: ["credential-org"], role: "owner", + roleError: null as Error | null, + restricted: false, personalSelected: false, personalValue: "fake-personal-credential" as string | null, })); @@ -19,7 +21,8 @@ vi.mock("../db/client.js", async (importOriginal) => ({ return { rows: fixture.membershipIds.map((org_id) => ({ org_id })) }; } if (sql.includes("SELECT role FROM org_members")) { - return { rows: [{ role: fixture.role }] }; + if (fixture.roleError) throw fixture.roleError; + return { rows: fixture.role ? [{ role: fixture.role }] : [] }; } throw new Error(`Unexpected query: ${sql}`); }, @@ -32,7 +35,13 @@ vi.mock("../settings/store.js", async (importOriginal) => ({ if (key === `u:${fixture.email}:active-org-id`) { return fixture.personalSelected ? { orgId: null } : null; } - if (key === `u:${fixture.email}:credential:GENERIC_SERVICE_TOKEN`) { + if (key === `o:${fixture.orgId}:restrict-personal-provider-keys`) { + return { restricted: fixture.restricted }; + } + if ( + key === `u:${fixture.email}:credential:GENERIC_SERVICE_TOKEN` || + key === `u:${fixture.email}:credential:ANTHROPIC_API_KEY` + ) { return fixture.personalValue ? { value: fixture.personalValue } : null; } throw new Error(`Unexpected setting: ${key}`); @@ -41,7 +50,7 @@ vi.mock("../settings/store.js", async (importOriginal) => ({ vi.mock("../secrets/storage.js", () => ({ readAppSecret: async (ref: { key: string; scope: string; scopeId: string }) => - ref.key === "GENERIC_SERVICE_TOKEN" && + ["GENERIC_SERVICE_TOKEN", "ANTHROPIC_API_KEY"].includes(ref.key) && ref.scope === "org" && ref.scopeId === fixture.orgId ? { value: "fake-org-credential" } @@ -49,7 +58,12 @@ vi.mock("../secrets/storage.js", () => ({ })); import { CredentialStoreUnavailableError } from "../server/credential-provider.js"; +import { + resolveSecretDetailed, + resolveBuilderGatewayAuth, +} from "../server/credential-provider.js"; import { runWithRequestContext } from "../server/request-context.js"; +import { ForbiddenError } from "../sharing/access.js"; import { resolveCredentialDetailed } from "./index.js"; function resolveGenericCredential() { @@ -65,10 +79,175 @@ describe("generic credentials with real organization resolution", () => { fixture.membershipError = null; fixture.membershipIds = [fixture.orgId]; fixture.role = "owner"; + fixture.roleError = null; + fixture.restricted = false; fixture.personalSelected = false; fixture.personalValue = "fake-personal-credential"; }); + it.each([ + ["GENERIC_SERVICE_TOKEN", undefined], + ["ANTHROPIC_API_KEY", undefined], + ["GENERIC_SERVICE_TOKEN", "credential-org"], + ["ANTHROPIC_API_KEY", "credential-org"], + ])( + "preserves revoked delegation as forbidden for %s with explicit org %s", + async (key, orgId) => { + fixture.role = ""; + await expect( + runWithRequestContext( + { + userEmail: fixture.email, + orgId: fixture.orgId, + a2aIdentityAssurance: "org-member", + }, + () => + resolveCredentialDetailed(key!, { + userEmail: fixture.email, + orgId, + }), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }, + ); + + it("preserves delegated refusal through the detailed provider resolver", async () => { + await expect( + runWithRequestContext( + { userEmail: fixture.email, a2aIdentityAssurance: "org-member" }, + () => resolveSecretDetailed("GENERIC_SERVICE_TOKEN"), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("preserves delegated refusal through Builder gateway authentication", async () => { + await expect( + runWithRequestContext( + { userEmail: fixture.email, a2aIdentityAssurance: "org-member" }, + () => resolveBuilderGatewayAuth(), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("refuses an explicit organization outside the valid delegation", async () => { + await expect( + runWithRequestContext( + { + userEmail: fixture.email, + orgId: fixture.orgId, + a2aIdentityAssurance: "org-member", + }, + () => + resolveCredentialDetailed("GENERIC_SERVICE_TOKEN", { + userEmail: fixture.email, + orgId: "other-org", + credentialScope: "org", + }), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("resolves the explicit organization for a valid delegation", async () => { + await expect( + runWithRequestContext( + { + userEmail: fixture.email, + orgId: fixture.orgId, + a2aIdentityAssurance: "org-member", + }, + () => + resolveCredentialDetailed("GENERIC_SERVICE_TOKEN", { + userEmail: fixture.email, + orgId: fixture.orgId, + credentialScope: "org", + }), + ), + ).resolves.toMatchObject({ value: "fake-org-credential", scope: "org" }); + }); + + it.each(["42501", "08006"])( + "types provider-policy membership failures with %s as unavailable", + async (code) => { + fixture.membershipError = Object.assign( + new Error("fake membership lookup failure"), + { code }, + ); + const lookup = runWithRequestContext({ userEmail: fixture.email }, () => + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + }), + ); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup).rejects.toMatchObject({ + cause: fixture.membershipError, + retryable: true, + }); + }, + ); + + it.each(["42501", "08006"])( + "types provider-policy role lookup failures with %s as unavailable", + async (code) => { + fixture.restricted = true; + fixture.roleError = Object.assign(new Error("fake role lookup failure"), { + code, + }); + const lookup = runWithRequestContext({ userEmail: fixture.email }, () => + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + }), + ); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup).rejects.toMatchObject({ + cause: fixture.roleError, + retryable: true, + }); + }, + ); + + it.each([ + ["personal", "member", true, "fake-personal-credential", "user"], + ["organization", "member", true, "fake-org-credential", "org"], + ["organization", "member", false, "fake-personal-credential", "user"], + ["organization", "owner", true, "fake-org-credential", "org"], + ["organization", "admin", true, "fake-org-credential", "org"], + ])( + "preserves provider policy for %s %s restricted=%s", + async (scope, role, restricted, value, resultScope) => { + fixture.membershipIds = scope === "personal" ? [] : [fixture.orgId]; + fixture.role = role; + fixture.restricted = restricted; + await expect( + runWithRequestContext({ userEmail: fixture.email }, () => + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + }), + ), + ).resolves.toMatchObject({ value, scope: resultScope }); + }, + ); + + it("keeps org-only provider lookup bound to the explicit organization", async () => { + fixture.membershipError = new Error("must not infer scope"); + await expect( + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + orgId: fixture.orgId, + credentialScope: "org", + }), + ).resolves.toMatchObject({ value: "fake-org-credential", scope: "org" }); + await expect( + resolveCredentialDetailed("ANTHROPIC_API_KEY", { + userEmail: fixture.email, + credentialScope: "org", + }), + ).resolves.toBeUndefined(); + }); + it.each(["42501", "08006"])( "rejects a readable personal credential when membership lookup fails with %s", async (code) => { diff --git a/packages/core/src/credentials/index.ts b/packages/core/src/credentials/index.ts index 0934e5c6363..f82cc8f3f3a 100644 --- a/packages/core/src/credentials/index.ts +++ b/packages/core/src/credentials/index.ts @@ -22,6 +22,7 @@ import { } from "../server/personal-provider-key-policy.js"; import { getRequestContext } from "../server/request-context.js"; import { getSetting, putSetting, deleteSetting } from "../settings/store.js"; +import { ForbiddenError } from "../sharing/access.js"; const SETTING_PREFIX = "credential:"; @@ -193,14 +194,22 @@ export async function resolveCredentialForScope( async function resolveEffectiveOrgId( ctx: CredentialContext, ): Promise<{ orgId: string | null; lookupFailed: boolean; cause?: unknown }> { - if (ctx.orgId) return { orgId: ctx.orgId, lookupFailed: false }; try { + const delegated = + getRequestContext()?.a2aIdentityAssurance === "org-member"; + if (ctx.orgId && !delegated) + return { orgId: ctx.orgId, lookupFailed: false }; const { resolveOrgIdForEmail } = await import("../org/context.js"); + const orgId = await resolveOrgIdForEmail(ctx.userEmail); + if (delegated && ctx.orgId && ctx.orgId !== orgId) { + throw new ForbiddenError(); + } return { - orgId: await resolveOrgIdForEmail(ctx.userEmail), + orgId, lookupFailed: false, }; } catch (cause) { + if (cause instanceof ForbiddenError) throw cause; return { orgId: null, lookupFailed: true, cause }; } } @@ -236,15 +245,26 @@ export async function resolveCredentialDetailed( ): Promise { if (!ctx?.userEmail) return undefined; + if (ctx.credentialScope === "org" && !ctx.orgId) return undefined; + const orgLookup = await resolveEffectiveOrgId(ctx); + if (orgLookup.lookupFailed) { + throw new CredentialStoreUnavailableError(orgLookup.cause); + } + const { orgId } = orgLookup; + // Stored but unused while the org restricts a member's provider keys. - const personalRestricted = - ctx.credentialScope !== "org" && - isPersonalProviderPolicyKey(key) && - (await isPersonalProviderKeyUseRestricted( - ctx.orgId - ? { email: ctx.userEmail, orgId: ctx.orgId } - : { email: ctx.userEmail }, - )); + let personalRestricted = false; + if (ctx.credentialScope !== "org" && isPersonalProviderPolicyKey(key)) { + try { + personalRestricted = await isPersonalProviderKeyUseRestricted({ + email: ctx.userEmail, + orgId, + }); + } catch (cause) { + if (cause instanceof ForbiddenError) throw cause; + throw new CredentialStoreUnavailableError(cause); + } + } let personal: ResolvedCredential | undefined; if (ctx.credentialScope !== "org" && !personalRestricted) { @@ -254,13 +274,6 @@ export async function resolveCredentialDetailed( if (value) personal = { value, scope: "user", scopeId: ctx.userEmail }; } - if (ctx.credentialScope === "org" && !ctx.orgId) return undefined; - const orgLookup = await resolveEffectiveOrgId(ctx); - // Before the personal answer: without the org, an owner's role is unknown. - if (orgLookup.lookupFailed) { - throw new CredentialStoreUnavailableError(orgLookup.cause); - } - const { orgId } = orgLookup; // An owner or admin runs on the organization's credential ahead of their own. if (personal && !(await readsOrgCredentialFirst(orgId, ctx.userEmail))) { return personal; diff --git a/packages/core/src/credentials/personal-key-policy.spec.ts b/packages/core/src/credentials/personal-key-policy.spec.ts index 8eb046a4f23..582e706d653 100644 --- a/packages/core/src/credentials/personal-key-policy.spec.ts +++ b/packages/core/src/credentials/personal-key-policy.spec.ts @@ -28,6 +28,7 @@ vi.mock( }), ); +import { CredentialStoreUnavailableError } from "../server/credential-provider.js"; import { resolveCredential } from "./index.js"; const EMAIL = "member@example.com"; @@ -82,7 +83,10 @@ describe("resolveCredential with personal API keys restricted", () => { await expect( resolveCredential("OPENAI_API_KEY", { userEmail: EMAIL }), ).resolves.toBeUndefined(); - expect(mocks.isRestricted).toHaveBeenCalledWith({ email: EMAIL }); + expect(mocks.isRestricted).toHaveBeenCalledWith({ + email: EMAIL, + orgId: "org-1", + }); }); it("leaves other credentials and unrestricted callers alone", async () => { @@ -101,10 +105,13 @@ describe("resolveCredential with personal API keys restricted", () => { }); it("fails loudly when the restriction can't be read", async () => { - mocks.isRestricted.mockRejectedValue(new Error("settings unavailable")); + const cause = new Error("settings unavailable"); + mocks.isRestricted.mockRejectedValue(cause); rows({ [`user:${EMAIL}:ANTHROPIC_API_KEY`]: "sk-ant-personal" }); - await expect( - resolveCredential("ANTHROPIC_API_KEY", { userEmail: EMAIL }), - ).rejects.toThrow("settings unavailable"); + const lookup = resolveCredential("ANTHROPIC_API_KEY", { userEmail: EMAIL }); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup).rejects.toMatchObject({ cause, retryable: true }); }); }); diff --git a/packages/core/src/mcp/build-server.ts b/packages/core/src/mcp/build-server.ts index 9a07ba13c53..985c697047c 100644 --- a/packages/core/src/mcp/build-server.ts +++ b/packages/core/src/mcp/build-server.ts @@ -74,6 +74,7 @@ import { type McpDirectoryWidgetReadArgument, renewMcpDirectoryWidgetCapabilityScope, } from "../shared/embed-auth.js"; +import { ForbiddenError } from "../sharing/access.js"; import { type McpAnalyticsContext, describeMcpError, @@ -4474,6 +4475,9 @@ export async function resolveMcpIdentityOrgId( const { resolveOrgIdForEmail } = await import("../org/context.js"); return (await resolveOrgIdForEmail(userEmail)) ?? undefined; } catch (cause) { + if (cause instanceof ForbiddenError) { + throw new McpIdentityVerificationRefusedError(); + } throw new McpIdentityVerificationUnavailableError(cause); } } diff --git a/packages/core/src/mcp/server.spec.ts b/packages/core/src/mcp/server.spec.ts index 4ccd5f58ef8..032b007e83c 100644 --- a/packages/core/src/mcp/server.spec.ts +++ b/packages/core/src/mcp/server.spec.ts @@ -24,6 +24,7 @@ import { listResourceSuggestions } from "../review/suggestions/actions.js"; import { loadActionsFromStaticRegistry } from "../server/action-discovery.js"; import { registerErrorCaptureProvider } from "../server/capture-error.js"; import { filesystemActionEntry } from "../server/filesystem-action-entry.js"; +import { runWithRequestContext } from "../server/request-context.js"; import { isMcpDirectoryWidgetReadCapabilityScope, isMcpDirectoryWidgetWriteCapabilityScope, @@ -10195,6 +10196,80 @@ describe("handleMcpRequest — web-standard runtime fallback (no Node req/res)", expect(result).toMatchObject({ error: "Service Unavailable" }); }); + it.each([ + "revoked", + "email-mismatch", + "missing-org", + "valid", + "unavailable", + ] as const)( + "preserves delegated organization resolution over HTTP: %s", + async (scope) => { + process.env.BETTER_AUTH_SECRET = + "oauth-secret-at-least-32-characters-long"; + const { signMcpOAuthAccessToken } = await import("./oauth-token.js"); + const token = await signMcpOAuthAccessToken({ + ownerEmail: "delegate@example.test", + clientId: "client-123", + scope: "mcp:read", + resource: "https://mail.agent-native.com/mcp", + issuer: "https://mail.agent-native.com", + }); + const org = + await vi.importActual( + "../org/context.js", + ); + resolveOrgIdForEmailMock.mockImplementation(org.resolveOrgIdForEmail); + const db = await import("../db/client.js"); + const realDb = db.getDbExec(); + const dbSpy = vi.spyOn(db, "getDbExec").mockReturnValue({ + execute: async (query: { sql: string; args: unknown[] }) => { + if (!query.sql.includes("SELECT role FROM org_members")) + return realDb.execute(query); + if (scope === "unavailable") + throw Object.assign(new Error("fake membership database failure"), { + code: "42501", + }); + return { rows: scope === "revoked" ? [] : [{ role: "member" }] }; + }, + } as any); + const event = makeWebEvent({ + method: "POST", + body: { jsonrpc: "2.0", id: 15, method: "tools/list", params: {} }, + headers: { authorization: `Bearer ${token}` }, + }); + try { + const result = await runWithRequestContext( + { + userEmail: + scope === "email-mismatch" + ? "other@example.test" + : "delegate@example.test", + orgId: scope === "missing-org" ? undefined : "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => handleMcpRequest(event, config as any), + ); + expect(result instanceof Response ? result.status : event._status).toBe( + scope === "valid" ? 200 : scope === "unavailable" ? 503 : 401, + ); + if (scope === "unavailable") { + expect(event._responseHeaders?.["retry-after"]).toBe("5"); + expect(event._responseHeaders?.["www-authenticate"]).toBeUndefined(); + expect(result).toMatchObject({ error: "Service Unavailable" }); + } else if (scope !== "valid") { + expect(event._responseHeaders?.["retry-after"]).toBeUndefined(); + expect(event._responseHeaders?.["www-authenticate"]).toContain( + 'error="invalid_token"', + ); + expect(result).toMatchObject({ reason: "invalid" }); + } + } finally { + dbSpy.mockRestore(); + } + }, + ); + it.each(["unavailable", "personal"] as const)( "preserves the email-only MCP scope result over HTTP: %s", async (scope) => { diff --git a/packages/core/src/org/context.spec.ts b/packages/core/src/org/context.spec.ts index 5bdc5c67ff2..f9edad6fcfe 100644 --- a/packages/core/src/org/context.spec.ts +++ b/packages/core/src/org/context.spec.ts @@ -30,6 +30,7 @@ vi.mock("../application-state/store.js", () => ({ import { markVerifiedServiceIdentityForEvent } from "../server/request-context.js"; import { runWithRequestContext } from "../server/request-context.js"; +import { ForbiddenError } from "../sharing/access.js"; import { setActiveOrgId } from "./active-org.js"; import { __resetDomainMatchCacheForTests } from "./auto-join-domain.js"; import { @@ -270,7 +271,7 @@ describe("getOrgContext", () => { orgRole: "owner", }); - const ctx = await runWithRequestContext( + const lookup = runWithRequestContext( { a2aIdentityAssurance: "org-member", userEmail: "owner@example.test", @@ -279,12 +280,7 @@ describe("getOrgContext", () => { () => getOrgContext(EVENT), ); - expect(ctx).toEqual({ - email: "other@example.test", - orgId: null, - orgName: null, - role: null, - }); + await expect(lookup).rejects.toBeInstanceOf(ForbiddenError); expect(mockExecute).not.toHaveBeenCalled(); }); @@ -1247,6 +1243,72 @@ describe("resolveOrgIdForEmail", () => { expect(await resolveOrgIdForEmail("nobody@b.com")).toBeNull(); }); + it.each(["missing-member", "email-mismatch", "missing-org"])( + "refuses delegated %s instead of reporting unavailable or Personal scope", + async (scenario) => { + const lookup = runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: scenario === "missing-org" ? undefined : "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => + resolveOrgIdForEmail( + scenario === "email-mismatch" + ? "other@example.test" + : "delegate@example.test", + ), + ); + await expect(lookup).rejects.toBeInstanceOf(ForbiddenError); + await expect(lookup).rejects.toMatchObject({ statusCode: 403 }); + }, + ); + + it("refuses a revoked delegation at the scope result boundary", async () => { + await expect( + runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => resolveOrgScopeForEmail("delegate@example.test"), + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("keeps a valid delegation pinned to its organization", async () => { + queueSelect([{ role: "owner" }]); + await expect( + runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => resolveOrgIdForEmail("delegate@example.test"), + ), + ).resolves.toBe("delegated-org"); + }); + + it("preserves the actual database failure while checking delegation", async () => { + const cause = Object.assign( + new Error("permission denied for table org_members"), + { code: "42501" }, + ); + mockExecute.mockRejectedValueOnce(cause); + await expect( + runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => resolveOrgIdForEmail("delegate@example.test"), + ), + ).rejects.toBe(cause); + }); + it("confirms personal scope when the membership query succeeds without rows", async () => { queueSelect([]); await expect(resolveOrgScopeForEmail("nobody@b.com")).resolves.toEqual({ diff --git a/packages/core/src/org/context.ts b/packages/core/src/org/context.ts index e5d04d918ea..3532a642bdb 100644 --- a/packages/core/src/org/context.ts +++ b/packages/core/src/org/context.ts @@ -21,6 +21,7 @@ import { import { getSetting } from "../settings/store.js"; import { getUserSetting } from "../settings/user-settings.js"; import { FIRST_RUN_ONBOARDING_ELIGIBLE_KEY } from "../shared/first-run-onboarding.js"; +import { ForbiddenError } from "../sharing/access.js"; import { setActiveOrgId } from "./active-org.js"; import { autoJoinDomainMatchingOrgs } from "./auto-join-domain.js"; import { isFreeEmailProvider } from "./free-email-providers.js"; @@ -79,9 +80,7 @@ function normalizeOrgRole(value: unknown): OrgRole | null { : null; } -function delegatedOrgMemberScope( - email?: string | null, -): string | null | undefined { +function delegatedOrgMemberScope(email?: string | null): string | undefined { const requestContext = getRequestContext(); if (requestContext?.a2aIdentityAssurance !== "org-member") return undefined; const requestEmail = requestContext.userEmail?.trim().toLowerCase(); @@ -91,7 +90,7 @@ function delegatedOrgMemberScope( !requestOrgId || (email && email.trim().toLowerCase() !== requestEmail) ) { - return null; + throw new ForbiddenError(); } return requestOrgId; } @@ -102,12 +101,13 @@ function scopeMembershipsForDelegatedIdentity( ): MembershipRow[] | null { const scopedOrgId = delegatedOrgMemberScope(email); if (scopedOrgId === undefined || memberships === null) return memberships; - if (!scopedOrgId) return []; - return memberships.flatMap((membership) => { + const scopedMemberships = memberships.flatMap((membership) => { if (membership.orgId !== scopedOrgId) return []; const role = effectiveOrgMemberRole(membership.role, scopedOrgId, email); return role ? [{ ...membership, role }] : []; }); + if (scopedMemberships.length === 0) throw new ForbiddenError(); + return scopedMemberships; } function emailDomainOf(email: string): string | null { @@ -280,7 +280,6 @@ function loadMembershipsForEvent( email: string, ): Promise { const scopedOrgId = delegatedOrgMemberScope(email); - if (scopedOrgId === null) return Promise.resolve([]); const ctx = event.context as Record; const cache = ((ctx[MEMBERSHIPS_CACHE_KEY] as | Map> @@ -326,9 +325,6 @@ async function resolveOrgContextUncached(event: H3Event): Promise { const isDelegatedOrgMember = requestContext?.a2aIdentityAssurance === "org-member"; const delegatedOrgId = delegatedOrgMemberScope(email); - if (isDelegatedOrgMember && !delegatedOrgId) { - return { email, orgId: null, orgName: null, role: null }; - } if (isAnonymousWaitlistSessionEmail(email)) { return { email, orgId: null, orgName: null, role: null }; } @@ -462,6 +458,7 @@ async function resolveOrgContextUncached(event: H3Event): Promise { selectedOrgId && !memberships.some((membership) => membership.orgId === selectedOrgId) ) { + if (isDelegatedOrgMember) throw new ForbiddenError(); return { email, orgId: null, orgName: null, role: null }; } if (selectedOrgId && memberships.some((m) => m.orgId === selectedOrgId)) { @@ -689,11 +686,9 @@ export async function resolveOrgScopeForEmail( ): Promise { const delegatedOrgId = delegatedOrgMemberScope(email); if (delegatedOrgId !== undefined) { - if (!delegatedOrgId) return { status: "unknown" }; const role = await readOrgMemberRole(delegatedOrgId, email); - return role - ? { status: "organization", orgId: delegatedOrgId } - : { status: "unknown" }; + if (!role) throw new ForbiddenError(); + return { status: "organization", orgId: delegatedOrgId }; } if (getRequestContext()?.orgScope === "personal") { return { status: "personal" }; @@ -746,9 +741,9 @@ export async function resolveActiveOrgIdForStorageCredential( const delegatedOrgId = delegatedOrgMemberScope(normalizedEmail); if (delegatedOrgId !== undefined) { - if (!delegatedOrgId) return null; const role = await readOrgMemberRole(delegatedOrgId, normalizedEmail); - return role ? delegatedOrgId : null; + if (!role) throw new ForbiddenError(); + return delegatedOrgId; } const { rows } = await getDbExec().execute({ @@ -781,7 +776,6 @@ export async function resolveOrgIdForEmailViaEvent( ): Promise { const delegatedOrgId = delegatedOrgMemberScope(email); if (delegatedOrgId !== undefined) { - if (!delegatedOrgId) return null; const memberships = await loadMembershipsForEvent(event, email); if (!memberships || !memberships.some((m) => m.orgId === delegatedOrgId)) { return null; @@ -795,9 +789,10 @@ export async function resolveOrgIdForEmailViaEvent( if (refreshed !== memberships) { updateMembershipsForEvent(event, email, refreshed); } - return refreshed.some((m) => m.orgId === delegatedOrgId) - ? delegatedOrgId - : null; + if (!refreshed.some((m) => m.orgId === delegatedOrgId)) { + throw new ForbiddenError(); + } + return delegatedOrgId; } if (getRequestContext()?.orgScope === "personal") return null; const settingPromise = loadActiveOrgSettingForEvent(event, email); diff --git a/packages/core/src/server/agent-run-context.spec.ts b/packages/core/src/server/agent-run-context.spec.ts index b2822556698..4649c8a3384 100644 --- a/packages/core/src/server/agent-run-context.spec.ts +++ b/packages/core/src/server/agent-run-context.spec.ts @@ -36,6 +36,7 @@ import { AGENT_CHAT_BROWSER_SESSION_ID_FIELD, AGENT_CHAT_REQUEST_CONTEXT_FIELD, } from "../agent/durable-background.js"; +import { ForbiddenError } from "../sharing/access.js"; import { resolveAgentRunOrgId, resolveAgentRunOrgScope, @@ -211,6 +212,17 @@ describe("server/agent-run-context", () => { ).resolves.toEqual({ orgScope: "personal" }); }); + it("preserves a definitive organization refusal during background scope resolution", async () => { + const refusal = new ForbiddenError(); + resolveOrgScopeForEmailMock.mockRejectedValue(refusal); + await expect( + resolveAgentRunOrgScope({ + event: makeEvent(), + ownerContext: { owner: "delegate@example.test", anonymous: false }, + }), + ).rejects.toBe(refusal); + }); + it("keeps organization scope unknown when an organization lookup fails", async () => { getSessionMock.mockRejectedValue(new Error("session unavailable")); getOrgContextMock.mockRejectedValue(new Error("org store unavailable")); diff --git a/packages/core/src/server/agent-run-context.ts b/packages/core/src/server/agent-run-context.ts index 69c54ae3fc0..1e3eea739d3 100644 --- a/packages/core/src/server/agent-run-context.ts +++ b/packages/core/src/server/agent-run-context.ts @@ -13,6 +13,7 @@ import { SYNTHETIC_TRAFFIC_HEADER, isSyntheticTrafficValue, } from "../shared/test-traffic.js"; +import { ForbiddenError } from "../sharing/access.js"; import { CREDENTIAL_MEMBERSHIP_UNAVAILABLE_MESSAGE, isCredentialMembershipUnavailable, @@ -313,7 +314,8 @@ export async function resolveAgentRunOrgScope(options: { const { getSession } = await import("./auth.js"); const session = await getSession(options.event); resolvedOrgId = normalizeId(session?.orgId); - } catch { + } catch (error) { + if (error instanceof ForbiddenError) throw error; orgLookupFailed = true; } @@ -336,7 +338,8 @@ export async function resolveAgentRunOrgScope(options: { ) { return { orgScope: "personal" }; } - } catch { + } catch (error) { + if (error instanceof ForbiddenError) throw error; orgLookupFailed = true; } } @@ -352,7 +355,8 @@ export async function resolveAgentRunOrgScope(options: { } else if (!orgLookupFailed) { return { orgScope: "personal" }; } - } catch { + } catch (error) { + if (error instanceof ForbiddenError) throw error; orgLookupFailed = true; } } diff --git a/packages/core/src/server/auth.ts b/packages/core/src/server/auth.ts index 426f913fdb4..d4761d62cd8 100644 --- a/packages/core/src/server/auth.ts +++ b/packages/core/src/server/auth.ts @@ -43,6 +43,7 @@ import { MCP_EMBED_CORS_ALLOW_HEADERS, shouldAllowMcpEmbedCredentials, } from "../shared/mcp-embed-headers.js"; +import { ForbiddenError } from "../sharing/access.js"; import { createConsentHandler, createConsentRegionHandler, @@ -4759,7 +4760,10 @@ async function backfillSessionOrg( if (session.orgId || hasExplicitPersonalOrgScope(event)) return session; const { resolveOrgIdForEmailViaEvent } = await import("../org/context.js"); const orgId = await resolveOrgIdForEmailViaEvent(event, session.email).catch( - () => null, + (error) => { + if (error instanceof ForbiddenError) throw error; + return null; + }, ); return orgId ? { ...session, orgId } : session; } diff --git a/packages/core/src/server/credential-provider.ts b/packages/core/src/server/credential-provider.ts index 006c8b32be8..6fac533fc8b 100644 --- a/packages/core/src/server/credential-provider.ts +++ b/packages/core/src/server/credential-provider.ts @@ -11,6 +11,7 @@ import { isTransientDatabaseError, } from "../db/client.js"; import { getOrgSetting } from "../settings/org-settings.js"; +import { ForbiddenError } from "../sharing/access.js"; import { BUILDER_CREDENTIAL_KEYS } from "./builder-credential-keys.js"; import { BuilderOAuthScopeError, @@ -154,6 +155,7 @@ export function assertCredentialStoreReadable(result: { lookupFailed: boolean; cause?: unknown; }): void { + if (result.cause instanceof ForbiddenError) throw result.cause; if (result.lookupFailed && isTransientDatabaseError(result.cause)) { throw new CredentialStoreUnavailableError(result.cause); } @@ -443,6 +445,7 @@ async function resolveOrgIdForRequestEmail( const { resolveOrgIdForEmail } = await import("../org/context.js"); return { orgId: await resolveOrgIdForEmail(email) }; } catch (err) { + if (err instanceof ForbiddenError) throw err; return { orgId: null, cause: err }; } } @@ -704,6 +707,7 @@ async function resolveScopedBuilderCredential( ); } } catch (err) { + if (err instanceof ForbiddenError) throw err; if (traceLookup) { console.log( `[builder-credential] key=${key} email=${email} scope=${scopeAttempted} error=${(err as Error)?.message ?? err}`, @@ -887,6 +891,7 @@ async function resolveScopedBuilderCredentials( } } } catch (err) { + if (err instanceof ForbiddenError) throw err; if (traceLookup) { console.log( `[builder-credential] email=${email} scope=${scopeAttempted} credentials error=${(err as Error)?.message ?? err}`, @@ -1311,6 +1316,7 @@ export async function resolveBuilderGatewayAuth( ? await hasBuilderOAuthSession(ownerEmail, orgId, { orgOnly: true }) : await hasBuilderOAuthSession(ownerEmail, orgId); } catch (error) { + if (error instanceof ForbiddenError) throw error; throw new BuilderCredentialLookupError(error); } } @@ -1329,6 +1335,7 @@ export async function resolveBuilderGatewayAuth( } : null; } catch (error) { + if (error instanceof ForbiddenError) throw error; if (error instanceof BuilderOAuthScopeError) return null; throw new BuilderCredentialLookupError(error); } @@ -2104,6 +2111,7 @@ export async function resolveSecretPairs( } } } catch (error) { + if (error instanceof ForbiddenError) throw error; lookupFailed = true; cause = error; } @@ -2312,6 +2320,7 @@ export async function resolveSecretDetailed( } } } catch (err) { + if (err instanceof ForbiddenError) throw err; if (traceLookup) { console.log( `[resolve-secret] key=${key} email=${email} scope=error err=${(err as Error)?.message ?? err}`, From 6c5963dfe065ef8e595d408802292b90cc9a4adf Mon Sep 17 00:00:00 2001 From: 3mdistal <86723305+3mdistal@users.noreply.github.com> Date: Fri, 23 Oct 2026 04:06:42 -0400 Subject: [PATCH 5/7] Preserve credential and session lookup errors --- .changeset/quiet-org-lookup-denial.md | 2 + packages/core/src/credentials/index.ts | 2 +- packages/core/src/mcp/build-server.ts | 2 +- packages/core/src/org/context.spec.ts | 67 ++++++++ packages/core/src/org/context.ts | 13 +- packages/core/src/server/agent-run-context.ts | 2 +- packages/core/src/server/auth.spec.ts | 38 +++++ packages/core/src/server/auth.ts | 8 +- ...credential-provider-org-resolution.spec.ts | 154 +++++++++++++++--- .../core/src/server/credential-provider.ts | 13 +- packages/core/src/sharing/access.ts | 10 +- packages/core/src/sharing/errors.ts | 7 + 12 files changed, 265 insertions(+), 53 deletions(-) create mode 100644 packages/core/src/sharing/errors.ts diff --git a/.changeset/quiet-org-lookup-denial.md b/.changeset/quiet-org-lookup-denial.md index 265008eed4f..5574ac4e050 100644 --- a/.changeset/quiet-org-lookup-denial.md +++ b/.changeset/quiet-org-lookup-denial.md @@ -5,3 +5,5 @@ Reject unavailable organization scope instead of treating failed membership reads as Personal access during credential resolution, MCP identity setup, or background task creation. Generic credential reads also reject non-transient membership failures before returning a stored Personal credential whose organization priority could not be checked. Preserve the narrow first-boot missing-membership-table exception for action dispatch. Keep revoked or mismatched delegated organization access a definitive authorization refusal, including MCP authentication and credential lookup wrappers. Resolve generic credentials' organization scope before provider policy checks and report failed policy or role reads as credential-store unavailability with the original cause. + +Keep failed non-transient credential lookups distinct from missing credentials in the secret and secret-pair wrappers, preserving the original failure. Propagate unavailable delegated membership reads through event-based organization resolution and session backfill instead of returning Personal scope. Keep authorization error classification independent of sharing database initialization. diff --git a/packages/core/src/credentials/index.ts b/packages/core/src/credentials/index.ts index f82cc8f3f3a..7197818fe95 100644 --- a/packages/core/src/credentials/index.ts +++ b/packages/core/src/credentials/index.ts @@ -22,7 +22,7 @@ import { } from "../server/personal-provider-key-policy.js"; import { getRequestContext } from "../server/request-context.js"; import { getSetting, putSetting, deleteSetting } from "../settings/store.js"; -import { ForbiddenError } from "../sharing/access.js"; +import { ForbiddenError } from "../sharing/errors.js"; const SETTING_PREFIX = "credential:"; diff --git a/packages/core/src/mcp/build-server.ts b/packages/core/src/mcp/build-server.ts index 985c697047c..afdd1d555c6 100644 --- a/packages/core/src/mcp/build-server.ts +++ b/packages/core/src/mcp/build-server.ts @@ -74,7 +74,7 @@ import { type McpDirectoryWidgetReadArgument, renewMcpDirectoryWidgetCapabilityScope, } from "../shared/embed-auth.js"; -import { ForbiddenError } from "../sharing/access.js"; +import { ForbiddenError } from "../sharing/errors.js"; import { type McpAnalyticsContext, describeMcpError, diff --git a/packages/core/src/org/context.spec.ts b/packages/core/src/org/context.spec.ts index f9edad6fcfe..47ef1a46dd9 100644 --- a/packages/core/src/org/context.spec.ts +++ b/packages/core/src/org/context.spec.ts @@ -1231,6 +1231,73 @@ describe("getOrgContext", () => { }); }); +describe("resolveOrgIdForEmailViaEvent delegated membership", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockExecute.mockResolvedValue({ rows: [] }); + mockGetUserSetting.mockResolvedValue(null); + }); + + const resolveDelegated = (event: ReturnType) => + runWithRequestContext( + { + userEmail: "delegate@example.test", + orgId: "delegated-org", + a2aIdentityAssurance: "org-member", + }, + () => resolveOrgIdForEmailViaEvent(event, "delegate@example.test"), + ); + + it.each(["42501", "08006"])( + "rejects unavailable delegated membership (%s) instead of returning Personal", + async (code) => { + const cause = Object.assign(new Error("membership read failed"), { + code, + }); + mockExecute.mockRejectedValueOnce(cause); + const lookup = resolveDelegated(makeEvent()); + await expect(lookup).rejects.toThrow(); + await expect(lookup).rejects.not.toBeInstanceOf(ForbiddenError); + await expect(lookup).rejects.toBe(cause); + }, + ); + + it("preserves definitive refusal when membership is absent", async () => { + await expect(resolveDelegated(makeEvent())).rejects.toBeInstanceOf( + ForbiddenError, + ); + }); + + it("returns a healthy delegated organization", async () => { + queueSelect([ + { orgId: "delegated-org", role: "member", orgName: "Example" }, + ]); + await expect(resolveDelegated(makeEvent())).resolves.toBe("delegated-org"); + }); + + it("keeps the nondelegated first-boot missing-membership-table handling", async () => { + mockExecute.mockRejectedValueOnce( + Object.assign(new Error('relation "org_members" does not exist'), { + code: "42P01", + }), + ); + await expect( + resolveOrgIdForEmailViaEvent(makeEvent(), "personal@example.test"), + ).resolves.toBeNull(); + }); + + it("keeps an explicit Personal scope org-less without a membership read", async () => { + await expect( + runWithRequestContext( + { userEmail: "personal@example.test", orgScope: "personal" }, + () => + resolveOrgIdForEmailViaEvent(makeEvent(), "personal@example.test"), + ), + ).resolves.toBeNull(); + expect(mockExecute).not.toHaveBeenCalled(); + }); +}); + describe("resolveOrgIdForEmail", () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/packages/core/src/org/context.ts b/packages/core/src/org/context.ts index 3532a642bdb..441096be35a 100644 --- a/packages/core/src/org/context.ts +++ b/packages/core/src/org/context.ts @@ -21,7 +21,7 @@ import { import { getSetting } from "../settings/store.js"; import { getUserSetting } from "../settings/user-settings.js"; import { FIRST_RUN_ONBOARDING_ELIGIBLE_KEY } from "../shared/first-run-onboarding.js"; -import { ForbiddenError } from "../sharing/access.js"; +import { ForbiddenError } from "../sharing/errors.js"; import { setActiveOrgId } from "./active-org.js"; import { autoJoinDomainMatchingOrgs } from "./auto-join-domain.js"; import { isFreeEmailProvider } from "./free-email-providers.js"; @@ -69,7 +69,12 @@ export async function queryOrgMembers(query: { const { rows } = await getDbExec().execute(query); return rows as Record[]; } catch (err) { - if (isTransientDatabaseError(err)) throw err; + if ( + getRequestContext()?.a2aIdentityAssurance === "org-member" || + isTransientDatabaseError(err) + ) { + throw err; + } return null; } } @@ -777,8 +782,8 @@ export async function resolveOrgIdForEmailViaEvent( const delegatedOrgId = delegatedOrgMemberScope(email); if (delegatedOrgId !== undefined) { const memberships = await loadMembershipsForEvent(event, email); - if (!memberships || !memberships.some((m) => m.orgId === delegatedOrgId)) { - return null; + if (memberships === null) { + throw new Error("Organization scope is unavailable"); } const refreshed = await refreshFederatedMemberships( event, diff --git a/packages/core/src/server/agent-run-context.ts b/packages/core/src/server/agent-run-context.ts index 1e3eea739d3..34c3e3dda1c 100644 --- a/packages/core/src/server/agent-run-context.ts +++ b/packages/core/src/server/agent-run-context.ts @@ -13,7 +13,7 @@ import { SYNTHETIC_TRAFFIC_HEADER, isSyntheticTrafficValue, } from "../shared/test-traffic.js"; -import { ForbiddenError } from "../sharing/access.js"; +import { ForbiddenError } from "../sharing/errors.js"; import { CREDENTIAL_MEMBERSHIP_UNAVAILABLE_MESSAGE, isCredentialMembershipUnavailable, diff --git a/packages/core/src/server/auth.spec.ts b/packages/core/src/server/auth.spec.ts index 3ea19bf9e5f..a0fe67581da 100644 --- a/packages/core/src/server/auth.spec.ts +++ b/packages/core/src/server/auth.spec.ts @@ -8976,6 +8976,44 @@ describe("server/auth", () => { expect(sessionLookups).toHaveBeenCalledTimes(2); }); + it.each(["42501", "08006", "forbidden"])( + "preserves an org backfill failure instead of returning an org-less session (%s)", + async (code) => { + vi.stubEnv("NODE_ENV", "production"); + delete process.env.ACCESS_TOKEN; + delete process.env.ACCESS_TOKENS; + const { ForbiddenError } = await import("../sharing/access.js"); + const cause = + code === "forbidden" + ? new ForbiddenError() + : Object.assign(new Error("membership read failed"), { code }); + vi.doMock("./better-auth-instance.js", () => ({ + getBetterAuth: vi.fn(async () => undefined), + getBetterAuthSync: () => ({ + api: { + getSession: async () => ({ + user: { id: "fake-auth-user", email: "delegate@example.test" }, + session: { token: "fake-session-token" }, + }), + }, + }), + })); + vi.doMock("../org/context.js", () => ({ + resolveOrgIdForEmailViaEvent: async () => { + throw cause; + }, + })); + const { getSession } = await import("./auth.js"); + await expect( + getSession( + createMockEvent({ + headers: { cookie: "an_session=fake-session-token" }, + }), + ), + ).rejects.toBe(cause); + }, + ); + it("records identity resolution start before asynchronous credential validation", async () => { vi.stubEnv("NODE_ENV", "production"); delete process.env.ACCESS_TOKEN; diff --git a/packages/core/src/server/auth.ts b/packages/core/src/server/auth.ts index d4761d62cd8..acc59069494 100644 --- a/packages/core/src/server/auth.ts +++ b/packages/core/src/server/auth.ts @@ -43,7 +43,6 @@ import { MCP_EMBED_CORS_ALLOW_HEADERS, shouldAllowMcpEmbedCredentials, } from "../shared/mcp-embed-headers.js"; -import { ForbiddenError } from "../sharing/access.js"; import { createConsentHandler, createConsentRegionHandler, @@ -4759,12 +4758,7 @@ async function backfillSessionOrg( ): Promise { if (session.orgId || hasExplicitPersonalOrgScope(event)) return session; const { resolveOrgIdForEmailViaEvent } = await import("../org/context.js"); - const orgId = await resolveOrgIdForEmailViaEvent(event, session.email).catch( - (error) => { - if (error instanceof ForbiddenError) throw error; - return null; - }, - ); + const orgId = await resolveOrgIdForEmailViaEvent(event, session.email); return orgId ? { ...session, orgId } : session; } diff --git a/packages/core/src/server/credential-provider-org-resolution.spec.ts b/packages/core/src/server/credential-provider-org-resolution.spec.ts index 3d515e6c5c8..7b80432bb26 100644 --- a/packages/core/src/server/credential-provider-org-resolution.spec.ts +++ b/packages/core/src/server/credential-provider-org-resolution.spec.ts @@ -2,6 +2,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const fixture = vi.hoisted(() => ({ membershipUnavailable: false, + membershipMissing: false, + personalAbsent: false, + storageFailure: undefined as Error | undefined, + membershipFailure: Object.assign( + new Error("permission denied for table org_members"), + { code: "42501" }, + ), restricted: true, email: "member@example.test", orgId: "restricted-org", @@ -14,12 +21,11 @@ vi.mock("../db/client.js", async (importOriginal) => ({ execute: async ({ sql }: { sql: string }) => { if (sql.includes("SELECT org_id FROM org_members")) { if (fixture.membershipUnavailable) { - throw Object.assign( - new Error("permission denied for table org_members"), - { code: "42501" }, - ); + throw fixture.membershipFailure; } - return { rows: [{ org_id: fixture.orgId }] }; + return { + rows: fixture.membershipMissing ? [] : [{ org_id: fixture.orgId }], + }; } if (sql.includes("SELECT role FROM org_members")) { return { rows: [{ role: "member" }] }; @@ -40,16 +46,48 @@ vi.mock("../settings/store.js", async (importOriginal) => ({ }, })); -vi.mock("../secrets/storage.js", () => ({ - readAppSecret: async (ref: { key: string; scope: string; scopeId: string }) => - ref.key === "ANTHROPIC_API_KEY" && - ref.scope === "user" && - ref.scopeId === fixture.email - ? { value: fixture.personalKey } - : null, -})); +vi.mock("../secrets/storage.js", () => { + const readAppSecret = async (ref: { + key: string; + scope: string; + scopeId: string; + }) => { + if (fixture.storageFailure) throw fixture.storageFailure; + return !fixture.personalAbsent && + ref.scope === "user" && + ref.scopeId === fixture.email + ? { + value: + ref.key === "ANTHROPIC_API_KEY" + ? fixture.personalKey + : `fake-${ref.key}`, + } + : null; + }; + return { + readAppSecret, + readAppSecrets: async (ref: { + keys: readonly string[]; + scope: string; + scopeId: string; + }) => { + const rows = new Map(); + for (const key of ref.keys) { + const row = await readAppSecret({ ...ref, key }); + if (row) rows.set(key, row); + } + return rows; + }, + }; +}); -import { resolveSecretDetailed } from "./credential-provider.js"; +import { ForbiddenError } from "../sharing/access.js"; +import { + CredentialStoreUnavailableError, + resolveSecret, + resolveSecretDetailed, + resolveSecretPairs, +} from "./credential-provider.js"; import { runWithRequestContext } from "./request-context.js"; function resolvePersonalProviderKey() { @@ -58,16 +96,19 @@ function resolvePersonalProviderKey() { ); } -describe("credential resolution with real org scope and personal-key policy", () => { - beforeEach(() => { - fixture.membershipUnavailable = false; - fixture.restricted = true; - vi.stubEnv("ANTHROPIC_API_KEY", ""); - vi.stubEnv("AGENT_VAULT_ORG_ID", ""); - }); +beforeEach(() => { + fixture.membershipUnavailable = false; + fixture.membershipMissing = false; + fixture.personalAbsent = false; + fixture.storageFailure = undefined; + fixture.restricted = true; + vi.stubEnv("ANTHROPIC_API_KEY", ""); + vi.stubEnv("AGENT_VAULT_ORG_ID", ""); +}); - afterEach(() => vi.unstubAllEnvs()); +afterEach(() => vi.unstubAllEnvs()); +describe("credential resolution with real org scope and personal-key policy", () => { it("reports a failed lookup without returning a personal key when membership access is denied", async () => { fixture.membershipUnavailable = true; @@ -94,3 +135,72 @@ describe("credential resolution with real org scope and personal-key policy", () }); }); }); + +describe.each([ + { + name: "resolveSecret", + resolve: () => resolveSecret("ANTHROPIC_API_KEY"), + personal: fixture.personalKey, + }, + { + name: "resolveSecretPairs", + resolve: () => + resolveSecretPairs([["EXAMPLE_CLIENT_ID", "EXAMPLE_CLIENT_SECRET"]]), + personal: ["fake-EXAMPLE_CLIENT_ID", "fake-EXAMPLE_CLIENT_SECRET"], + }, +])("$name with real organization resolution", ({ resolve, personal }) => { + it("throws with the original nontransient membership lookup failure", async () => { + fixture.membershipUnavailable = true; + const lookup = runWithRequestContext({ userEmail: fixture.email }, resolve); + await expect(lookup).rejects.toBeInstanceOf( + CredentialStoreUnavailableError, + ); + await expect(lookup.catch((error: Error) => error.cause)).resolves.toBe( + fixture.membershipFailure, + ); + }); + + it("preserves a definitive refusal's identity", async () => { + const refusal = new ForbiddenError(); + fixture.storageFailure = refusal; + await expect( + runWithRequestContext({ userEmail: fixture.email }, resolve), + ).rejects.toBe(refusal); + }); + + it("refuses an incomplete delegated identity", async () => { + await expect( + runWithRequestContext( + { + userEmail: fixture.email, + a2aIdentityAssurance: "org-member", + }, + resolve, + ), + ).rejects.toBeInstanceOf(ForbiddenError); + }); + + it("returns null when readable Personal storage has no credential", async () => { + fixture.personalAbsent = true; + await expect( + runWithRequestContext( + { userEmail: fixture.email, orgScope: "personal" }, + resolve, + ), + ).resolves.toBeNull(); + }); + + it("returns the readable Personal credential without an organization", async () => { + fixture.membershipMissing = true; + await expect( + runWithRequestContext({ userEmail: fixture.email }, resolve), + ).resolves.toEqual(personal); + }); + + it("returns the readable credential when the organization permits it", async () => { + fixture.restricted = false; + await expect( + runWithRequestContext({ userEmail: fixture.email }, resolve), + ).resolves.toEqual(personal); + }); +}); diff --git a/packages/core/src/server/credential-provider.ts b/packages/core/src/server/credential-provider.ts index 6fac533fc8b..295f46f49dc 100644 --- a/packages/core/src/server/credential-provider.ts +++ b/packages/core/src/server/credential-provider.ts @@ -5,13 +5,9 @@ import { GATEWAY_UNAVAILABLE_VISITOR_MESSAGE, } from "../agent/engine/credential-errors.js"; import { getAppConfig } from "../app-config/index.js"; -import { - getDbExec, - isLocalDatabase, - isTransientDatabaseError, -} from "../db/client.js"; +import { getDbExec, isLocalDatabase } from "../db/client.js"; import { getOrgSetting } from "../settings/org-settings.js"; -import { ForbiddenError } from "../sharing/access.js"; +import { ForbiddenError } from "../sharing/errors.js"; import { BUILDER_CREDENTIAL_KEYS } from "./builder-credential-keys.js"; import { BuilderOAuthScopeError, @@ -156,7 +152,7 @@ export function assertCredentialStoreReadable(result: { cause?: unknown; }): void { if (result.cause instanceof ForbiddenError) throw result.cause; - if (result.lookupFailed && isTransientDatabaseError(result.cause)) { + if (result.lookupFailed) { throw new CredentialStoreUnavailableError(result.cause); } } @@ -2051,8 +2047,7 @@ export async function resolveSecretPairs( if (lookupFailed) { const environmentPair = readEnvironmentPairs(); if (environmentPair) return environmentPair; - assertCredentialStoreReadable({ lookupFailed, cause }); - return null; + throw cause; } if (personal && !(await readsOrgCredentialFirst(orgId, email))) { diff --git a/packages/core/src/sharing/access.ts b/packages/core/src/sharing/access.ts index 197b6a71175..7872714662b 100644 --- a/packages/core/src/sharing/access.ts +++ b/packages/core/src/sharing/access.ts @@ -16,11 +16,13 @@ import { workspaceUserGroupsIncludeUser, workspaceUserGroupsTable, } from "../workspace-connections/groups.js"; +import { ForbiddenError } from "./errors.js"; import { listShareableResources, requireShareableResource, type ShareableResourceRegistration, } from "./registry.js"; +export { ForbiddenError } from "./errors.js"; import { ROLE_RANK, type ShareRole, type Visibility } from "./schema.js"; function findRegistrationByTable( @@ -32,14 +34,6 @@ function findRegistrationByTable( return undefined; } -export class ForbiddenError extends Error { - statusCode = 403; - constructor(message = "Forbidden") { - super(message); - this.name = "ForbiddenError"; - } -} - export interface AccessContext { transaction?: DbExec; userEmail?: string; diff --git a/packages/core/src/sharing/errors.ts b/packages/core/src/sharing/errors.ts new file mode 100644 index 00000000000..fd769e388bb --- /dev/null +++ b/packages/core/src/sharing/errors.ts @@ -0,0 +1,7 @@ +export class ForbiddenError extends Error { + statusCode = 403; + constructor(message = "Forbidden") { + super(message); + this.name = "ForbiddenError"; + } +} From 9c6016c7629a44a486a7f23cab1356cabf3de40f Mon Sep 17 00:00:00 2001 From: 3mdistal <86723305+3mdistal@users.noreply.github.com> Date: Fri, 23 Oct 2026 04:39:41 -0400 Subject: [PATCH 6/7] Preserve unavailable workspace and engine states --- .changeset/quiet-org-lookup-denial.md | 4 +- packages/core/src/org/context.spec.ts | 76 ++++---- packages/core/src/org/context.ts | 16 +- .../agent-engines/list-agent-engines.spec.ts | 43 +++++ .../agent-engines/list-agent-engines.ts | 165 ++++++++++-------- packages/core/src/server/auth.spec.ts | 18 +- ...d-content-database-source-field.db.test.ts | 2 +- 7 files changed, 199 insertions(+), 125 deletions(-) diff --git a/.changeset/quiet-org-lookup-denial.md b/.changeset/quiet-org-lookup-denial.md index 5574ac4e050..e6e14d7696c 100644 --- a/.changeset/quiet-org-lookup-denial.md +++ b/.changeset/quiet-org-lookup-denial.md @@ -6,4 +6,6 @@ Reject unavailable organization scope instead of treating failed membership read Keep revoked or mismatched delegated organization access a definitive authorization refusal, including MCP authentication and credential lookup wrappers. Resolve generic credentials' organization scope before provider policy checks and report failed policy or role reads as credential-store unavailability with the original cause. -Keep failed non-transient credential lookups distinct from missing credentials in the secret and secret-pair wrappers, preserving the original failure. Propagate unavailable delegated membership reads through event-based organization resolution and session backfill instead of returning Personal scope. Keep authorization error classification independent of sharing database initialization. +Keep failed non-transient credential lookups distinct from missing credentials in the secret and secret-pair wrappers, preserving the original failure. Propagate unavailable membership reads through event-based organization resolution and session backfill for ordinary and delegated requests instead of returning Personal scope. Keep authorization error classification independent of sharing database initialization. + +Keep the engine catalog available when current-engine discovery cannot read a provider credential, reporting an unavailable current selection with its error alongside per-provider diagnostics. Runtime engine detection and definitive authorization refusals still reject. diff --git a/packages/core/src/org/context.spec.ts b/packages/core/src/org/context.spec.ts index 47ef1a46dd9..85da1953702 100644 --- a/packages/core/src/org/context.spec.ts +++ b/packages/core/src/org/context.spec.ts @@ -108,17 +108,13 @@ describe("getOrgContext", () => { expect(mockExecute).not.toHaveBeenCalled(); }); - it("marks a swallowed membership read failure as unavailable org scope", async () => { + it("rejects a missing membership table instead of marking Personal scope", async () => { mockGetSession.mockResolvedValue({ email: "a@b.com" }); - mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), - ); + const cause = new Error('relation "org_members" does not exist'); + mockExecute.mockRejectedValueOnce(cause); - await expect(getOrgContext(EVENT)).resolves.toMatchObject({ - email: "a@b.com", - orgId: null, - }); - expect(hasOrgScopeResolutionUnavailable(EVENT)).toBe(true); + await expect(getOrgContext(EVENT)).rejects.toBe(cause); + expect(hasOrgScopeResolutionUnavailable(EVENT)).toBe(false); }); it("resolves a verified service identity for an existing unlinked org", async () => { @@ -717,17 +713,27 @@ describe("getOrgContext", () => { }); describe("membership-lookup failure (tables missing before migrations)", () => { - it("returns the session orgId when present", async () => { + it("rejects the lookup even when a session orgId is present", async () => { + mockGetSession.mockResolvedValue({ + email: "a@b.com", + orgId: "sess-org", + orgRole: "admin", + }); + const cause = new Error('relation "org_members" does not exist'); + mockExecute.mockRejectedValueOnce(cause); + await expect(getOrgContext(EVENT)).rejects.toBe(cause); + }); + + it("preserves a claimed session org during a transient lookup failure", async () => { mockGetSession.mockResolvedValue({ email: "a@b.com", orgId: "sess-org", orgRole: "admin", }); mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), + Object.assign(new Error("db query timed out"), { code: "57014" }), ); - const ctx = await getOrgContext(EVENT); - expect(ctx).toEqual({ + await expect(getOrgContext(EVENT)).resolves.toEqual({ email: "a@b.com", orgId: "sess-org", orgName: null, @@ -735,18 +741,11 @@ describe("getOrgContext", () => { }); }); - it("returns a null-org context when there is no session orgId", async () => { + it("rejects the lookup when there is no session orgId", async () => { mockGetSession.mockResolvedValue({ email: "a@b.com" }); - mockExecute.mockRejectedValueOnce( - new Error('relation "org_members" does not exist'), - ); - const ctx = await getOrgContext(EVENT); - expect(ctx).toEqual({ - email: "a@b.com", - orgId: null, - orgName: null, - role: null, - }); + const cause = new Error('relation "org_members" does not exist'); + mockExecute.mockRejectedValueOnce(cause); + await expect(getOrgContext(EVENT)).rejects.toBe(cause); }); }); @@ -1231,7 +1230,7 @@ describe("getOrgContext", () => { }); }); -describe("resolveOrgIdForEmailViaEvent delegated membership", () => { +describe("resolveOrgIdForEmailViaEvent membership", () => { beforeEach(() => { vi.clearAllMocks(); mockExecute.mockResolvedValue({ rows: [] }); @@ -1275,12 +1274,27 @@ describe("resolveOrgIdForEmailViaEvent delegated membership", () => { await expect(resolveDelegated(makeEvent())).resolves.toBe("delegated-org"); }); - it("keeps the nondelegated first-boot missing-membership-table handling", async () => { - mockExecute.mockRejectedValueOnce( - Object.assign(new Error('relation "org_members" does not exist'), { - code: "42P01", - }), - ); + it.each(["42501", "08006", "42P01"])( + "rejects unavailable ordinary membership (%s) without caching Personal scope", + async (code) => { + const cause = Object.assign(new Error("membership read failed"), { + code, + }); + mockExecute.mockRejectedValueOnce(cause); + const event = makeEvent(); + const resolve = () => + runWithRequestContext({ userEmail: "member@example.test" }, () => + resolveOrgIdForEmailViaEvent(event, "member@example.test"), + ); + await expect(resolve()).rejects.toBe(cause); + queueSelect([ + { orgId: "ordinary-org", role: "member", orgName: "Example" }, + ]); + await expect(resolve()).resolves.toBe("ordinary-org"); + }, + ); + + it("returns Personal when the ordinary membership read succeeds without rows", async () => { await expect( resolveOrgIdForEmailViaEvent(makeEvent(), "personal@example.test"), ).resolves.toBeNull(); diff --git a/packages/core/src/org/context.ts b/packages/core/src/org/context.ts index 441096be35a..73643aaf885 100644 --- a/packages/core/src/org/context.ts +++ b/packages/core/src/org/context.ts @@ -64,19 +64,9 @@ function markOrgScopeResolutionUnavailable(event: H3Event): void { export async function queryOrgMembers(query: { sql: string; args: unknown[]; -}): Promise[] | null> { - try { - const { rows } = await getDbExec().execute(query); - return rows as Record[]; - } catch (err) { - if ( - getRequestContext()?.a2aIdentityAssurance === "org-member" || - isTransientDatabaseError(err) - ) { - throw err; - } - return null; - } +}): Promise[]> { + const { rows } = await getDbExec().execute(query); + return rows as Record[]; } function normalizeOrgRole(value: unknown): OrgRole | null { diff --git a/packages/core/src/scripts/agent-engines/list-agent-engines.spec.ts b/packages/core/src/scripts/agent-engines/list-agent-engines.spec.ts index 2b2233aadbe..3f02f8157f5 100644 --- a/packages/core/src/scripts/agent-engines/list-agent-engines.spec.ts +++ b/packages/core/src/scripts/agent-engines/list-agent-engines.spec.ts @@ -12,6 +12,7 @@ describe("list-agent-engines", () => { let defaultAuthority: { allowed: boolean }; beforeEach(() => { + vi.doUnmock("../../agent/engine/index.js"); vi.doUnmock("../../server/credential-provider.js"); vi.resetModules(); vi.doUnmock("../../settings/store.js"); @@ -305,6 +306,48 @@ describe("list-agent-engines", () => { state: "default", scope: "org", }); + expect(byName("ai-sdk:google")).toMatchObject({ + configuredError: expect.stringContaining("Could not read"), + }); + expect(byName("ai-sdk:google")?.configured).toBeUndefined(); + expect(byName("ai-sdk:openai")?.configured).toBe(false); + expect(byName("ai-sdk:openai")?.configuredError).toBeUndefined(); + expect(result.current).toBeNull(); + expect(result.currentError).toContain("Could not read"); + }); + + it.each(["forbidden", "unexpected"])( + "preserves a %s failure from current-engine discovery", + async (kind) => { + const { ForbiddenError } = await import("../../sharing/access.js"); + const cause = + kind === "forbidden" + ? new ForbiddenError() + : new Error("discovery failed"); + vi.doMock("../../agent/engine/index.js", async (importOriginal) => ({ + ...(await importOriginal< + typeof import("../../agent/engine/index.js") + >()), + detectEngineFromUserSecrets: async () => { + throw cause; + }, + })); + const { run } = await import("./list-agent-engines.js"); + await expect(run()).rejects.toBe(cause); + }, + ); + + it("preserves a definitive refusal during per-engine readiness checks", async () => { + const { ForbiddenError } = await import("../../sharing/access.js"); + const cause = new ForbiddenError(); + vi.doMock("../../agent/engine/index.js", async (importOriginal) => ({ + ...(await importOriginal()), + isStoredEngineUsableForRequest: async () => { + throw cause; + }, + })); + const { run } = await import("./list-agent-engines.js"); + await expect(run()).rejects.toBe(cause); }); it("does not report AGENT_ENGINE as current when its optional package is missing", async () => { diff --git a/packages/core/src/scripts/agent-engines/list-agent-engines.ts b/packages/core/src/scripts/agent-engines/list-agent-engines.ts index 05f09e90873..dfb539efa3c 100644 --- a/packages/core/src/scripts/agent-engines/list-agent-engines.ts +++ b/packages/core/src/scripts/agent-engines/list-agent-engines.ts @@ -29,12 +29,14 @@ import { getAppConfig } from "../../app-config/index.js"; import { CHATGPT_SUBSCRIPTION_LAB } from "../../labs/core-labs.js"; import { getUserLabEnabled } from "../../labs/store.js"; import { + CredentialStoreUnavailableError, prefetchSecrets, readProviderCredentialRejections, resolveSecretDetailed, type ProviderCredentialRejection, } from "../../server/credential-provider.js"; import { getRequestUserEmail } from "../../server/request-context.js"; +import { ForbiddenError } from "../../sharing/errors.js"; export const tool: ActionTool = { description: @@ -167,73 +169,94 @@ export async function run(args: Record = {}): Promise { ? (defaultSetting.value as { engine?: string; model?: string }) : null; - const storedEntry = - typeof current?.engine === "string" ? engineFor(current.engine) : undefined; - const storedUsable = - !!storedEntry && - (await isStoredEngineUsableForRequest(current, storedEntry)); - const appDefault = await getAgentAppModelDefaultForCurrentRequest(args.appId); - const appDefaultEntry = - typeof appDefault?.engine === "string" - ? engineFor(appDefault.engine) - : undefined; - const appDefaultUsable = - !!appDefault && - !!appDefaultEntry && - (await isStoredEngineUsableForRequest(appDefault, appDefaultEntry)); - const detectedFromUser = await detectEngineFromUserSecrets(); - const configuredEngine = getAppConfig().agent.engine; - const envEntry = configuredEngine ? engineFor(configuredEngine) : undefined; - const envUsable = - !!envEntry && - (await isStoredEngineUsableForRequest({ engine: envEntry.name }, envEntry)); - const envUnavailable = !!envEntry && !envUsable; - const detectedFromEnv = detectEngineFromEnv(); - const envSelectedEntry = envUsable ? envEntry : undefined; + let currentEngine: { engine: string; model: string | undefined } | null = + null; + let currentError: string | undefined; + try { + const storedEntry = + typeof current?.engine === "string" + ? engineFor(current.engine) + : undefined; + const storedUsable = + !!storedEntry && + (await isStoredEngineUsableForRequest(current, storedEntry)); + const appDefault = await getAgentAppModelDefaultForCurrentRequest( + args.appId, + ); + const appDefaultEntry = + typeof appDefault?.engine === "string" + ? engineFor(appDefault.engine) + : undefined; + const appDefaultUsable = + !!appDefault && + !!appDefaultEntry && + (await isStoredEngineUsableForRequest(appDefault, appDefaultEntry)); + const detectedFromUser = await detectEngineFromUserSecrets(); + const configuredEngine = getAppConfig().agent.engine; + const envEntry = configuredEngine ? engineFor(configuredEngine) : undefined; + const envUsable = + !!envEntry && + (await isStoredEngineUsableForRequest( + { engine: envEntry.name }, + envEntry, + )); + const envUnavailable = !!envEntry && !envUsable; + const detectedFromEnv = detectEngineFromEnv(); + const envSelectedEntry = envUsable ? envEntry : undefined; - const currentEntry = envUnavailable - ? undefined - : (envSelectedEntry ?? - (appDefaultUsable ? appDefaultEntry : undefined) ?? - (storedUsable ? storedEntry : undefined) ?? - detectedFromUser ?? - detectedFromEnv ?? - getAgentEngineEntry("anthropic")); - const configuredModel = getAppConfig().agent.model; - const currentModelCandidate = - configuredModel && configuredModel !== "auto" - ? configuredModel - : appDefaultUsable && currentEntry?.name === appDefault?.engine - ? appDefault?.model - : storedUsable && currentEntry?.name === current?.engine - ? current?.model - : undefined; - const acceptsCustomModels = currentEntry - ? await resolveEngineAcceptsCustomModels(currentEntry) - : false; - const preserveCustomModels = currentEntry - ? await resolveEnginePreservesCustomModels(currentEntry) - : false; - // Mirrors the runtime: an engine default nobody picked yields to the first - // checked model once it's unchecked. - const currentSelection = - currentEntry && !currentModelCandidate - ? await selectionFor(currentEntry.name) - : null; - const checkedDefault = - currentSelection?.state === "selected" && - currentEntry && - !currentSelection.models.includes(currentEntry.defaultModel) - ? currentSelection.models[0] - : undefined; - const currentModel = - currentEntry && !envUnavailable - ? normalizeModelForEngine( - currentEntry, - currentModelCandidate ?? checkedDefault ?? currentEntry.defaultModel, - { acceptsCustomModels, preserveCustomModels }, - ) - : undefined; + const currentEntry = envUnavailable + ? undefined + : (envSelectedEntry ?? + (appDefaultUsable ? appDefaultEntry : undefined) ?? + (storedUsable ? storedEntry : undefined) ?? + detectedFromUser ?? + detectedFromEnv ?? + getAgentEngineEntry("anthropic")); + const configuredModel = getAppConfig().agent.model; + const currentModelCandidate = + configuredModel && configuredModel !== "auto" + ? configuredModel + : appDefaultUsable && currentEntry?.name === appDefault?.engine + ? appDefault?.model + : storedUsable && currentEntry?.name === current?.engine + ? current?.model + : undefined; + const acceptsCustomModels = currentEntry + ? await resolveEngineAcceptsCustomModels(currentEntry) + : false; + const preserveCustomModels = currentEntry + ? await resolveEnginePreservesCustomModels(currentEntry) + : false; + // Mirrors the runtime: an engine default nobody picked yields to the first + // checked model once it's unchecked. + const currentSelection = + currentEntry && !currentModelCandidate + ? await selectionFor(currentEntry.name) + : null; + const checkedDefault = + currentSelection?.state === "selected" && + currentEntry && + !currentSelection.models.includes(currentEntry.defaultModel) + ? currentSelection.models[0] + : undefined; + const currentModel = + currentEntry && !envUnavailable + ? normalizeModelForEngine( + currentEntry, + currentModelCandidate ?? + checkedDefault ?? + currentEntry.defaultModel, + { acceptsCustomModels, preserveCustomModels }, + ) + : undefined; + currentEngine = + !currentEntry || envUnavailable + ? null + : { engine: currentEntry.name, model: currentModel }; + } catch (error) { + if (!(error instanceof CredentialStoreUnavailableError)) throw error; + currentError = error.message; + } const engineEntries = await Promise.all( engines.map(async (e) => { // Resolved per engine, not across the set: one provider whose credential @@ -253,6 +276,7 @@ export async function run(args: Record = {}): Promise { e, ); } catch (error) { + if (error instanceof ForbiddenError) throw error; configuredError = error instanceof Error ? error.message : String(error); } @@ -318,13 +342,8 @@ export async function run(args: Record = {}): Promise { ); const result = { engines: engineEntries, - current: - !currentEntry || envUnavailable - ? null - : { - engine: currentEntry.name, - model: currentModel, - }, + current: currentEngine, + ...(currentError ? { currentError } : {}), canUpdateDefault: defaultAuthority.allowed, defaultSource: defaultSetting.source, }; diff --git a/packages/core/src/server/auth.spec.ts b/packages/core/src/server/auth.spec.ts index a0fe67581da..9e0dbd91c92 100644 --- a/packages/core/src/server/auth.spec.ts +++ b/packages/core/src/server/auth.spec.ts @@ -3629,7 +3629,10 @@ describe("server/auth", () => { const app = createMockApp(); await autoMountAuth(app, { - getSession: async () => ({ email: "member@example.com" }), + getSession: async () => ({ + email: "member@example.com", + orgId: "fixture-org", + }), }); const guard = app.use.mock.calls @@ -8992,16 +8995,19 @@ describe("server/auth", () => { getBetterAuthSync: () => ({ api: { getSession: async () => ({ - user: { id: "fake-auth-user", email: "delegate@example.test" }, + user: { id: "fake-auth-user", email: "member@example.test" }, session: { token: "fake-session-token" }, }), }, }), })); - vi.doMock("../org/context.js", () => ({ - resolveOrgIdForEmailViaEvent: async () => { - throw cause; - }, + const execute = vi.fn(async ({ sql }: { sql: string }) => { + if (sql.includes("FROM org_members")) throw cause; + return { rows: [] }; + }); + vi.doMock("../db/client.js", async (importOriginal) => ({ + ...(await importOriginal()), + getDbExec: () => ({ execute }), })); const { getSession } = await import("./auth.js"); await expect( diff --git a/templates/content/actions/bind-content-database-source-field.db.test.ts b/templates/content/actions/bind-content-database-source-field.db.test.ts index 5cabeed660a..64916514f41 100644 --- a/templates/content/actions/bind-content-database-source-field.db.test.ts +++ b/templates/content/actions/bind-content-database-source-field.db.test.ts @@ -58,7 +58,7 @@ afterAll(() => { let counter = 0; async function asOwner(fn: () => Promise): Promise { - return runWithRequestContext({ userEmail: OWNER }, fn); + return runWithRequestContext({ userEmail: OWNER, orgScope: "personal" }, fn); } async function seedRowUnion() { From 69be50bbc9eb8005c6a76a6b81ae4313778dcd12 Mon Sep 17 00:00:00 2001 From: 3mdistal <86723305+3mdistal@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:00:02 -0400 Subject: [PATCH 7/7] Preserve automation history when membership lookup is unavailable --- .changeset/quiet-org-lookup-denial.md | 2 + .../jobs/background-automation-runner.spec.ts | 101 ++++++++++++++++++ .../src/jobs/background-automation-runner.ts | 99 +++++++++++------ .../actions/database-setup-mcp.db.test.ts | 1 + .../document-description-mcp.db.test.ts | 1 + .../actions/document-trash-mcp.db.test.ts | 1 + 6 files changed, 175 insertions(+), 30 deletions(-) diff --git a/.changeset/quiet-org-lookup-denial.md b/.changeset/quiet-org-lookup-denial.md index e6e14d7696c..54d5ed06cd9 100644 --- a/.changeset/quiet-org-lookup-denial.md +++ b/.changeset/quiet-org-lookup-denial.md @@ -9,3 +9,5 @@ Keep revoked or mismatched delegated organization access a definitive authorizat Keep failed non-transient credential lookups distinct from missing credentials in the secret and secret-pair wrappers, preserving the original failure. Propagate unavailable membership reads through event-based organization resolution and session backfill for ordinary and delegated requests instead of returning Personal scope. Keep authorization error classification independent of sharing database initialization. Keep the engine catalog available when current-engine discovery cannot read a provider credential, reporting an unavailable current selection with its error alongside per-provider diagnostics. Runtime engine detection and definitive authorization refusals still reject. + +Record a scoped failed background automation attempt when creator-membership or organization-admin alert lookup is unreadable. Preserve the typed failure and original cause, stop before credential or agent execution, and keep a proven former creator's same-organization admin fallback. diff --git a/packages/core/src/jobs/background-automation-runner.spec.ts b/packages/core/src/jobs/background-automation-runner.spec.ts index 3a251d3d57f..e7a49f3c30c 100644 --- a/packages/core/src/jobs/background-automation-runner.spec.ts +++ b/packages/core/src/jobs/background-automation-runner.spec.ts @@ -2334,6 +2334,107 @@ describe("runBackgroundAutomation — preconditions fail before any thread or ru return history.notification_email; } + it.each(["membership", "admin"] as const)( + "records a failed attempt when the %s recipient lookup is unreadable", + async (deniedAt) => { + await seedOrg([["bob@agent-native.test", "owner"]]); + const { ensureTable } = await import("./run-history.js"); + await ensureTable(); + const { resolveOwnerEngineApiKey } = + await import("../agent/production-agent.js"); + const { runAgentLoopDirectWithSoftTimeout } = + await import("../agent/run-loop-with-resume.js"); + const name = `recipient-denied-${deniedAt}`; + const automation = precondition(name, { + runAs: "shared", + orgId: "acme", + createdBy: "alice@agent-native.test", + }); + automation.resource.owner = "__organization__:acme"; + const denied = Object.assign( + new Error("permission denied for table org_members"), + { + code: "42501", + }, + ); + const execute = rawClient.execute.getMockImplementation()!; + const queries: string[] = []; + rawClient.execute.mockImplementation(async (input) => { + if (typeof input !== "string" && /FROM org_members/.test(input.sql)) { + queries.push(input.sql); + if ( + deniedAt === "membership" || + input.sql.includes("role IN ('owner', 'admin')") + ) + throw denied; + } + return execute(input); + }); + createThreadMock.mockClear(); + vi.mocked(resolveOwnerEngineApiKey).mockClear(); + vi.mocked(runAgentLoopDirectWithSoftTimeout).mockClear(); + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const result = await runBackgroundAutomation( + runOptions(automation, { + ownerEmail: "__organization__:acme", + orgId: "acme", + }), + { ...standardDeps, appId: "calendar" }, + ).then( + () => ({ error: undefined }), + (error: unknown) => ({ error }), + ); + const history = await pglite + .prepare( + `SELECT owner, scope, org_id, app_id, path, status, error, error_code, + notification_email, run_id, thread_id, started_at, finished_at + FROM automation_runs WHERE automation = ?`, + ) + .all(name); + expect(history).toHaveLength(1); + const errorCode = + deniedAt === "membership" + ? "owner_unverifiable" + : "automation_notification_recipient_unavailable"; + expect(history[0]).toMatchObject({ + owner: "__organization__:acme", + scope: "organization", + org_id: "acme", + app_id: "calendar", + path: `jobs/${name}.md`, + status: "error", + error_code: errorCode, + error: expect.stringContaining( + "permission denied for table org_members", + ), + notification_email: null, + run_id: null, + thread_id: null, + started_at: expect.any(Number), + finished_at: expect.any(Number), + }); + expect(Number(history[0].finished_at)).toBeGreaterThanOrEqual( + Number(history[0].started_at), + ); + expect(result.error).toMatchObject({ errorCode, cause: denied }); + expect( + errors.mock.calls.some((call) => call.includes(result.error)), + ).toBe(true); + expect(createThreadMock).not.toHaveBeenCalled(); + expect(resolveOwnerEngineApiKey).not.toHaveBeenCalled(); + expect(runAgentLoopDirectWithSoftTimeout).not.toHaveBeenCalled(); + await expect(countRowsWithPrefix(`job-${name}`)).resolves.toBe(0); + if (deniedAt === "membership") { + expect(queries).toHaveLength(1); + } + } finally { + rawClient.execute.mockImplementation(execute); + errors.mockRestore(); + } + }, + ); + it("alerts an org owner, not a creator who has left the organization", async () => { await seedOrg([["bob@agent-native.test", "owner"]]); expect(await orgJobAlertRecipient("org-creator-left")).toBe( diff --git a/packages/core/src/jobs/background-automation-runner.ts b/packages/core/src/jobs/background-automation-runner.ts index 7bf22001192..cd218638691 100644 --- a/packages/core/src/jobs/background-automation-runner.ts +++ b/packages/core/src/jobs/background-automation-runner.ts @@ -154,8 +154,13 @@ export const BACKGROUND_RUN_HARD_TIMEOUT_MS = 10 * 60_000; export class BackgroundAutomationRunError extends Error { readonly errorCode: string; readonly deliveryNote?: string; - constructor(message: string, errorCode: string, deliveryNote?: string) { - super(message); + constructor( + message: string, + errorCode: string, + deliveryNote?: string, + options?: ErrorOptions, + ) { + super(message, options); this.name = "BackgroundAutomationRunError"; this.errorCode = errorCode; this.deliveryNote = deliveryNote; @@ -242,7 +247,12 @@ export type AutomationIdentityFailureCode = export type AutomationIdentityValidation = | { ok: true } - | { ok: false; reason: string; code: AutomationIdentityFailureCode }; + | { + ok: false; + reason: string; + code: AutomationIdentityFailureCode; + cause?: unknown; + }; export type BackgroundAutomationIdentityResult = | { ok: true; identity: AutomationExecutionIdentity } @@ -289,6 +299,7 @@ export async function validateAutomationRunIdentity( ok: false, reason: `could not verify user "${ownerEmail}" for this run`, code: "owner_unverifiable", + cause: error, }; } @@ -302,13 +313,6 @@ export async function validateAutomationRunIdentity( LIMIT 1`, args: [orgId, ownerEmail], }); - if (memberRows === null) { - return { - ok: false, - reason: `could not verify membership in org "${orgId}"`, - code: "owner_unverifiable", - }; - } if (memberRows.length === 0) { return { ok: false, @@ -317,12 +321,13 @@ export async function validateAutomationRunIdentity( }; } return { ok: true }; - } catch { + } catch (cause) { // coercion-ok: reported as `owner_unverifiable`, which the scheduler retries instead of treating the owner as gone. return { ok: false, reason: `could not verify membership in org "${orgId}"`, code: "owner_unverifiable", + cause, }; } } @@ -542,12 +547,17 @@ async function notificationEmailFor( return owner; } const creator = createdBy?.trim(); - if ( - creator?.includes("@") && - automationOwnerKind(creator) === "user" && - (await validateAutomationRunIdentity(creator, orgId)).ok - ) { - return creator; + if (creator?.includes("@") && automationOwnerKind(creator) === "user") { + const validity = await validateAutomationRunIdentity(creator, orgId); + if (validity.ok) return creator; + if (validity.code === "owner_unverifiable") { + throw new BackgroundAutomationRunError( + `${validity.reason}${validity.cause instanceof Error ? `: ${validity.cause.message}` : ""}`, + validity.code, + undefined, + { cause: validity.cause }, + ); + } } const admin = orgId ? await orgAdminEmail(orgId) : undefined; if (!admin) { @@ -567,12 +577,6 @@ async function orgAdminEmail(orgId: string): Promise { LIMIT 1`, args: [orgId], }); - if (rows === null) { - console.error( - `[automations] automation_alert_no_recipient: could not read the owners of org "${orgId}".`, - ); - return undefined; - } const email = rows[0]?.email; return typeof email === "string" && email ? email : undefined; } @@ -589,21 +593,55 @@ export async function startBackgroundAutomationHistory( appId?: string, options?: StartAutomationRunOptions, ): Promise { - return startAutomationRun( + let notificationEmail: string | undefined; + let lookupFailure: BackgroundAutomationRunError | undefined; + try { + notificationEmail = await notificationEmailFor( + automation.name, + ownerEmail, + automation.meta.createdBy, + orgId, + ); + } catch (cause) { + lookupFailure = + cause instanceof BackgroundAutomationRunError + ? cause + : new BackgroundAutomationRunError( + `Could not resolve the alert recipient for automation "${automation.name}"${orgId ? ` in org "${orgId}"` : ""}: ${cause instanceof Error ? cause.message : String(cause)}`, + "automation_notification_recipient_unavailable", + undefined, + { cause }, + ); + } + const historyId = await startAutomationRun( { ...automationRunOwnership(automation.resource.owner, ownerEmail, orgId), automation: automation.name, path: automation.resource.path, appId, - notificationEmail: await notificationEmailFor( - automation.name, - ownerEmail, - automation.meta.createdBy, - orgId, - ), + notificationEmail, }, options, ); + if (lookupFailure) { + await finishAutomationRun( + historyId, + "error", + lookupFailure.message, + lookupFailure.errorCode, + { + notify: false, + requirePersisted: true, + expectedRunId: null, + }, + ); + console.error( + `[automations] Could not verify the alert recipient for "${automation.name}" (history "${historyId}", org "${orgId ?? "none"}"):`, + lookupFailure, + ); + throw lookupFailure; + } + return historyId; } export async function runBackgroundAutomation( @@ -623,6 +661,7 @@ export async function runBackgroundAutomation( deps.appId, ); } catch (err) { + if (err instanceof BackgroundAutomationRunError) throw err; console.error( `[automations] Could not open a history record for "${automation.name}"; running anyway:`, err, diff --git a/templates/content/actions/database-setup-mcp.db.test.ts b/templates/content/actions/database-setup-mcp.db.test.ts index 54b0388a5a7..32bd44d27bf 100644 --- a/templates/content/actions/database-setup-mcp.db.test.ts +++ b/templates/content/actions/database-setup-mcp.db.test.ts @@ -51,6 +51,7 @@ let outsiderClient: MCPClient; function callerIdentity(userEmail: string): MCPCallerIdentity { return { userEmail, + orgId: null, orgDomain: undefined, oauthScopes: ["mcp:read", "mcp:write"], }; diff --git a/templates/content/actions/document-description-mcp.db.test.ts b/templates/content/actions/document-description-mcp.db.test.ts index 6ddf2bdf61d..834d113d757 100644 --- a/templates/content/actions/document-description-mcp.db.test.ts +++ b/templates/content/actions/document-description-mcp.db.test.ts @@ -65,6 +65,7 @@ async function connect(userEmail: string, writes = true) { }, { userEmail, + orgId: null, orgDomain: undefined, oauthScopes: writes ? ["mcp:read", "mcp:write"] : ["mcp:read"], }, diff --git a/templates/content/actions/document-trash-mcp.db.test.ts b/templates/content/actions/document-trash-mcp.db.test.ts index 08e4111122f..4188f5d5ad1 100644 --- a/templates/content/actions/document-trash-mcp.db.test.ts +++ b/templates/content/actions/document-trash-mcp.db.test.ts @@ -50,6 +50,7 @@ let outsiderClient: MCPClient; function callerIdentity(userEmail: string): MCPCallerIdentity { return { userEmail, + orgId: null, orgDomain: undefined, oauthScopes: ["mcp:read", "mcp:write"], };