diff --git a/apps/host-cloudflare/src/worker.ts b/apps/host-cloudflare/src/worker.ts index 74a1be074..06220bc84 100644 --- a/apps/host-cloudflare/src/worker.ts +++ b/apps/host-cloudflare/src/worker.ts @@ -1,3 +1,5 @@ +import { decodeOAuthCallbackState } from "@executor-js/sdk/shared"; + import { makeCloudflareApp } from "./app"; import type { CloudflareEnv } from "./config"; @@ -27,12 +29,24 @@ const resolveHandler = (env: CloudflareEnv) => { return handlerPromise; }; +const OAUTH_CALLBACK_PATH = "/api/oauth/callback"; + +const normalizeOAuthCallbackState = (request: Request): Request => { + if (request.method !== "GET" && request.method !== "HEAD") return request; + const url = new URL(request.url); + if (url.pathname !== OAUTH_CALLBACK_PATH) return request; + const callbackState = decodeOAuthCallbackState(url.searchParams.get("state")); + if (callbackState === null) return request; + url.searchParams.set("state", callbackState.state); + return new Request(url, request); +}; + export default { fetch: async (request: Request, env: CloudflareEnv, ctx: ExecutionContext): Promise => { const serve = await resolveHandler(env); if (new URL(request.url).pathname === "/mcp") { return serve.mcp(request, env, ctx); } - return serve.app(request); + return serve.app(normalizeOAuthCallbackState(request)); }, }; diff --git a/apps/host-cloudflare/wrangler.jsonc b/apps/host-cloudflare/wrangler.jsonc index 6b3a12b7f..d15a50aac 100644 --- a/apps/host-cloudflare/wrangler.jsonc +++ b/apps/host-cloudflare/wrangler.jsonc @@ -22,7 +22,7 @@ { "binding": "DB", "database_name": "executor", - "database_id": "ae748ca1-032c-4427-a1a0-fe39db77d1a9", + "database_id": "029d8b51-2a1b-43ec-961b-991a3fea0a0c", }, ], // Plugin blob seam backend: multi-MB values (resolved OpenAPI specs, @@ -55,13 +55,15 @@ // (the at-rest secret-encryption key) is a SECRET — set it with // `wrangler secret put EXECUTOR_SECRET_KEY`, never in vars. "vars": { - "ACCESS_TEAM_DOMAIN": "your-team.cloudflareaccess.com", - "ACCESS_AUD": "", + "ACCESS_TEAM_DOMAIN": "mateusz-7ac.cloudflareaccess.com", + "ACCESS_AUD": "44e8b9acc8e46bae7e4671d7bb6af41d56a042864cf14dea70e33b85b6f99922", "ACCESS_NAME_CLAIM": "name", "ACCESS_GROUPS_CLAIM": "groups", - "ADMIN_EMAILS": "", + "ADMIN_EMAILS": "mateusz@synthetic.ai", "SELF_HOSTED_ORG_ID": "default", - "SELF_HOSTED_ORG_NAME": "Default", + "SELF_HOSTED_ORG_NAME": "Synthetic", + "VITE_PUBLIC_SITE_URL": "https://executor-cloudflare.mateusz-7ac.workers.dev", + "SELF_HOSTED_ORG_SLUG": "synthetic", // VITE_PUBLIC_SITE_URL is intentionally unset: with no static URL the worker // derives the web base URL from each request's origin (RequestWebOrigin), so // secret/OAuth handoff links match whatever host the user actually reached. diff --git a/packages/core/sdk/src/core-schema.ts b/packages/core/sdk/src/core-schema.ts index 2469ac67b..7a0211890 100644 --- a/packages/core/sdk/src/core-schema.ts +++ b/packages/core/sdk/src/core-schema.ts @@ -33,7 +33,7 @@ export const dateColumn = (name: string) => column(name, "timestamp"); // The policy callback hands us a `ConditionBuilder` typed to the specific table's // columns; it isn't assignable to the generic `Record` builder // (column-name positions are contravariant), so accept it loosely and re-narrow. -const ownerVisibility = (builder: unknown, context: ExecutorOwnerPolicyContext) => +const ownerVisibility = (builder: unknown, context: ExecutorOwnerPolicyContext | undefined) => ownerVisibilityCondition(builder as AnyConditionBuilder, context) as Condition | boolean; /** A truly global table (the blob store). Isolation is carried in the row's diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 42dee8cd1..fc5ecfce4 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -106,7 +106,11 @@ import { type ToolPolicy, type UpdateToolPolicyInput, } from "./policies"; -import type { CredentialProvider, ProviderEntry } from "./provider"; +import type { + CredentialProvider, + CredentialProviderScope, + ProviderEntry, +} from "./provider"; import type { AnyPlugin, Elicit, @@ -1371,6 +1375,7 @@ export const createExecutor = (effect: Effect.Effect) => fuma.transaction(effect); @@ -1492,6 +1497,11 @@ export const createExecutor = `${row.owner}:${row.subject}:${row.integration}:${row.name}`; + const connectionCredentialScope = (row: ConnectionRow): CredentialProviderScope => ({ + owner: row.owner as Owner, + subject: String(row.subject), + }); + const loadOAuthClientRow = ( owner: Owner, slug: string, @@ -1526,7 +1536,10 @@ export const createExecutor = => // Share a single refresh per connection so concurrent resolves of the same // connection all await one refresh-token grant (the AS rotates the refresh @@ -1666,14 +1692,24 @@ export const createExecutor = refreshInFlight.delete(key))), + // Cache the grant with cleanup attached to the underlying execution, + // rather than to an awaiting caller fiber. This keeps the entry only for + // the grant's de-duplication window, regardless of waiter interruption. + let gated!: Effect.Effect< + string | null, + StorageFailure | CredentialResolutionError + >; + const grant = performTokenRefresh(row, provider).pipe( + Effect.ensuring( + Effect.sync(() => { + if (refreshInFlight.get(key) === gated) refreshInFlight.delete(key); + }), + ), ); + gated = yield* Effect.cached(grant); // Re-check after building (a peer fiber may have registered first while // we built ours) so everyone converges on the same shared grant. const winner = refreshInFlight.get(key) ?? gated; @@ -1687,8 +1723,19 @@ export const createExecutor = }`). const resolveConnectionValues = ( row: ConnectionRow, + options?: { readonly force?: boolean }, ): Effect.Effect, StorageFailure | CredentialResolutionError> => Effect.gen(function* () { + // The caller may hold a row loaded by an earlier operation. Re-read it + // before consulting expiry or provider values so rotations performed by + // another executor instance are observed. + const freshRow = yield* findConnectionRow({ + owner: row.owner as Owner, + integration: IntegrationSlug.make(row.integration), + name: ConnectionName.make(row.name), + }); + if (!freshRow) return {}; + row = freshRow; const provider = credentialProviders.get(row.provider); if (!provider) { return yield* new CredentialProviderNotRegisteredError({ @@ -1698,13 +1745,19 @@ export const createExecutor = = {}; for (const [variable, itemId] of Object.entries(connectionItemIds(row))) { - out[variable] = yield* provider.get(ProviderItemId.make(itemId)); + out[variable] = yield* provider.get( + ProviderItemId.make(itemId), + connectionCredentialScope(row), + ); } return out; }).pipe( @@ -1724,8 +1777,9 @@ export const createExecutor = => - resolveConnectionValues(row).pipe( + resolveConnectionValues(row, options).pipe( Effect.map((values) => values[PRIMARY_INPUT_VARIABLE] ?? null), ); @@ -1749,23 +1803,25 @@ export const createExecutor = => foldResolutionFailure( Effect.gen(function* () { const row = yield* findConnectionRow(ref); if (!row) return null; - return yield* resolveConnectionValue(row); + return yield* resolveConnectionValue(row, options); }), ); const resolveConnectionValuesByRef = ( ref: ConnectionRef, + options?: { readonly force?: boolean }, ): Effect.Effect, StorageFailure> => foldResolutionFailure( Effect.gen(function* () { const row = yield* findConnectionRow(ref); if (!row) return {}; - return yield* resolveConnectionValues(row); + return yield* resolveConnectionValues(row, options); }), ); @@ -2167,8 +2223,8 @@ export const createExecutor = resolveConnectionValueByRef(ref), - getValues: () => resolveConnectionValuesByRef(ref), + getValue: (options) => resolveConnectionValueByRef(ref, options), + getValues: (options) => resolveConnectionValuesByRef(ref, options), }) .pipe( Effect.mapError((cause) => @@ -2363,10 +2419,14 @@ export const createExecutor = ownedKeys(input.owner), + catch: (cause) => storageFailureFromUnknown("invalid owner", cause), + }); for (const i of pasted) { const itemId = `connection:${input.owner}:${input.integration}:${name}:${i.variable}`; if ("value" in i.origin && provider.set) { - yield* provider.set(ProviderItemId.make(itemId), i.origin.value); + yield* provider.set(ProviderItemId.make(itemId), i.origin.value, credentialScope); } itemIds[i.variable] = itemId; } @@ -3842,6 +3902,7 @@ export const createExecutor = connectionsRemove(ref), refresh: (ref) => connectionsRefresh(ref), markToolsStale: (ref) => connectionsMarkToolsStale(ref), - resolveValue: (ref) => resolveConnectionValueByRef(ref), + resolveValue: (ref, options) => resolveConnectionValueByRef(ref, options), }, providers: { list: () => providersList(), diff --git a/packages/core/sdk/src/index.ts b/packages/core/sdk/src/index.ts index 73a8e9f6c..5dce12693 100644 --- a/packages/core/sdk/src/index.ts +++ b/packages/core/sdk/src/index.ts @@ -99,7 +99,7 @@ export type { export type { Tool, ToolDef, ToolListFilter, ToolAnnotations } from "./tool"; // Credential providers. -export type { CredentialProvider, ProviderEntry } from "./provider"; +export type { CredentialProvider, CredentialProviderScope, ProviderEntry } from "./provider"; // Public projections / detection. export { ToolSchemaView, IntegrationDetectionResult } from "./types"; diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 5130dcb77..1aead66ae 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -114,6 +114,8 @@ export type OAuthScopePolicy = * connection row + produces tools), the owner binding, and the redirect base. */ export interface OAuthServiceDeps { readonly fuma: IFumaClient; + /** Policy-free handle used only for OAuth callback state lifecycle. */ + readonly sessionFuma: IFumaClient; readonly owner: OwnerBinding; readonly tenant: string; readonly subject: string | null; @@ -1173,9 +1175,14 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { input: OAuthCompleteInput, ): Effect.Effect => Effect.gen(function* () { - const sessionRow = yield* deps.fuma.use("oauth_session.findFirst", (db) => + // OAuth callbacks may arrive under a different authenticated identity than + // the browser that started the flow. State is an unguessable 256-bit value + // with a 15-minute TTL, so this single lookup deliberately bypasses owner + // visibility while retaining tenant/state matching. + const sessionRow = yield* deps.sessionFuma.use("oauth_session.findFirst", (db) => looseDb(db).findFirst("oauth_session", { - where: (b: any) => b("state", "=", String(input.state)), + where: (b: any) => + b.and(b("tenant", "=", deps.tenant), b("state", "=", String(input.state))), }), ); if (!sessionRow) { @@ -1327,12 +1334,17 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { }); } const itemId = accessItemId(target.owner, target.integration, target.name); - yield* provider.set(ProviderItemId.make(itemId), token.access_token); + const credentialScope = deps.ownedKeys(target.owner); + yield* provider.set(ProviderItemId.make(itemId), token.access_token, credentialScope); let refreshItemId: string | null = null; if (token.refresh_token) { refreshItemId = refreshItemIdFor(itemId); - yield* provider.set(ProviderItemId.make(refreshItemId), token.refresh_token); + yield* provider.set( + ProviderItemId.make(refreshItemId), + token.refresh_token, + credentialScope, + ); } const oauthScope = recordedOAuthScope(token, requestedScopes); @@ -1362,10 +1374,11 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { }); const deleteSession = (state: OAuthState): Effect.Effect => - deps.fuma + deps.sessionFuma .use("oauth_session.delete", (db) => looseDb(db).deleteMany("oauth_session", { - where: (b: any) => b("state", "=", String(state)), + where: (b: any) => + b.and(b("tenant", "=", deps.tenant), b("state", "=", String(state))), }), ) .pipe(Effect.asVoid); diff --git a/packages/core/sdk/src/oauth-session-policy-free-handle.test.ts b/packages/core/sdk/src/oauth-session-policy-free-handle.test.ts new file mode 100644 index 000000000..4f18f2022 --- /dev/null +++ b/packages/core/sdk/src/oauth-session-policy-free-handle.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; + +import { collectTables, createExecutor } from "./executor"; +import { createSqliteTestFumaDb } from "./sqlite-test-db"; +import { Tenant } from "./ids"; + +// Regression test for the "Cannot read properties of undefined (reading +// 'tenant')" StorageError that shipped when `oauth.complete`'s policy-free +// `sessionFuma` handle (packages/core/sdk/src/executor.ts: +// `makeFumaClient(rootDbUntyped)`) ran a real `findFirst`/`deleteMany` against +// an `oauth_session` row with NO query context bound. +// +// `makeTestConfig`/`makeTestWorkspaceHarness` (test-config.ts) pre-wrap +// `config.db` with `withQueryContext(testDb.db, { tenant, subject })` BEFORE +// handing it to `createExecutor`, so `rootDbUntyped` inside `executor.ts` +// already carries a bound context in every other test in this package — the +// crash never reproduces there. Production hosts (apps/local, apps/cloud) pass +// a genuinely context-free `FumaDb` (built by `createExecutorFumaDb` / +// `createSqliteFumaDb`, with no `withQueryContext` applied) as `config.db`, so +// `rootDbUntyped` has no bound context and `sessionFuma` is the raw handle. +// This test reproduces that exact production shape by constructing the +// executor directly off `createSqliteTestFumaDb(...).db` (unbound), instead of +// going through the test-config helpers. +describe("oauth session lookup on a policy-free (context-unbound) handle", () => { + it("does not throw when the root db has no bound owner-policy context", async () => { + const tables = collectTables(); + const testDb = await createSqliteTestFumaDb({ tables, namespace: "oauth_policy_free_test" }); + + const executor = await Effect.runPromise( + createExecutor({ + tenant: Tenant.make("test-tenant"), + onElicitation: "accept-all", + db: testDb.db, // NOT wrapped with withQueryContext — matches production wiring. + }), + ); + + try { + // Before the fix, `oauth.complete`'s `deps.sessionFuma.use("oauth_session.findFirst", ...)` + // dereferenced `context.tenant` on an undefined context and threw a + // StorageError ("Cannot read properties of undefined (reading 'tenant')") + // for ANY state, even one that doesn't exist. After the fix, a + // policy-free read with no matching row cleanly reports "not found" + // instead of crashing. + const result = await Effect.runPromiseExit( + executor.oauth.complete({ + state: "nonexistent-state" as never, + code: "unused", + callbackDomain: null, + } as never), + ); + + expect(result._tag).toBe("Failure"); + if (result._tag === "Failure") { + const failure = JSON.stringify(result.cause); + expect(failure).not.toContain("Cannot read properties of undefined"); + expect(failure).not.toContain("reading 'tenant'"); + } + + // `cancel` exercises the same policy-free handle's deleteMany path. The + // owned-table delete policy is visibility-only (unlike create/update), so + // an absent context is allowed and the service's explicit tenant+state + // predicate remains the complete isolation boundary. + await expect( + Effect.runPromise(executor.oauth.cancel("nonexistent-state" as never)), + ).resolves.toBeUndefined(); + } finally { + if (executor.close) await Effect.runPromise(executor.close()); + await testDb.close(); + } + }); +}); diff --git a/packages/core/sdk/src/owner-policy.ts b/packages/core/sdk/src/owner-policy.ts index 9751e4902..d01ede35b 100644 --- a/packages/core/sdk/src/owner-policy.ts +++ b/packages/core/sdk/src/owner-policy.ts @@ -51,11 +51,24 @@ const requireContext = ( }; /** The rows the bound `{ tenant, subject }` may see/mutate: org rows in the - * tenant, plus this subject's own user rows. */ + * tenant, plus this subject's own user rows. + * + * `context` is `undefined` for policy-free handles that deliberately bypass + * owner visibility while relying on their OWN explicit `tenant` filter in the + * caller's `where` clause (e.g. the OAuth session lookup/delete-by-`state`, + * which may run under a different identity than the one that started the + * flow). Returning `true` here means "add no extra restriction" (see + * `mergePolicyCondition` in the fumadb orm) — it does NOT mean "no rows", and + * it does NOT scope by tenant on its own; callers without a bound context + * MUST supply their own tenant filter or risk a cross-tenant read. Do not + * reach for this for anything that lacks that explicit tenant filter — writes + * still hard-require context via `requireContext` in `assertOwnerWritable`/ + * `assertOwnerPatch`. */ export const ownerVisibilityCondition = ( builder: AnyConditionBuilder, - context: ExecutorOwnerPolicyContext, + context: ExecutorOwnerPolicyContext | undefined, ): Condition | boolean => { + if (!context) return true; const orgClause = builder.and( builder("tenant", "=", context.tenant), builder("owner", "=", "org"), diff --git a/packages/core/sdk/src/plugin.ts b/packages/core/sdk/src/plugin.ts index 6d4dbc23b..88b4a04fc 100644 --- a/packages/core/sdk/src/plugin.ts +++ b/packages/core/sdk/src/plugin.ts @@ -239,7 +239,10 @@ export interface PluginCtx { readonly markToolsStale: (ref: ConnectionRef) => Effect.Effect; /** Resolve a connection's value through its provider (and OAuth refresh). * null if the provider can't produce one. */ - readonly resolveValue: (ref: ConnectionRef) => Effect.Effect; + readonly resolveValue: ( + ref: ConnectionRef, + options?: { readonly force?: boolean }, + ) => Effect.Effect; }; /** Registered credential backends — for discovery (browse a backend's items). */ @@ -314,11 +317,15 @@ export interface ResolveToolsInput { readonly template: AuthTemplateSlug | null; /** Lazily resolve the connection's credential value via its provider — only * the kinds that actually call out (mcp) pay for it. */ - readonly getValue: () => Effect.Effect; + readonly getValue: (options?: { + readonly force?: boolean; + }) => Effect.Effect; /** Lazily resolve every credential input (`variable → value`) — the * multi-input analog of `getValue`, for methods whose placements reference * more than one variable. Empty map when the connection isn't persisted. */ - readonly getValues: () => Effect.Effect, StorageFailure>; + readonly getValues: (options?: { + readonly force?: boolean; + }) => Effect.Effect, StorageFailure>; } export interface ResolveToolsResult { diff --git a/packages/core/sdk/src/promise.ts b/packages/core/sdk/src/promise.ts index d48d10607..4379bc89b 100644 --- a/packages/core/sdk/src/promise.ts +++ b/packages/core/sdk/src/promise.ts @@ -38,7 +38,7 @@ export type { // Credential providers are Effect-native (their `get`/`set` return `Effect`s), // but Promise consumers still author them to register an inline writable store // via `createExecutor({ providers })`. -export type { CredentialProvider, ProviderEntry } from "./provider"; +export type { CredentialProvider, CredentialProviderScope, ProviderEntry } from "./provider"; export type { CreateToolPolicyInput, RemoveToolPolicyInput, diff --git a/packages/core/sdk/src/provider.ts b/packages/core/sdk/src/provider.ts index 42a3defa4..10e7850a6 100644 --- a/packages/core/sdk/src/provider.ts +++ b/packages/core/sdk/src/provider.ts @@ -18,6 +18,14 @@ export interface ProviderEntry { readonly name: string; } +/** Partition that owns a connection credential. Providers without partitioned + * storage may ignore it. */ +export interface CredentialProviderScope { + readonly owner: "org" | "user"; + /** Empty for org credentials; the connection row's subject for user credentials. */ + readonly subject: string; +} + export interface CredentialProvider { readonly key: ProviderKey; /** If false, we never write here — `set`/`delete` are skipped and a referenced @@ -25,10 +33,23 @@ export interface CredentialProvider { readonly writable: boolean; /** Resolve a value by opaque id. The single hop a credential goes through * before its template is applied. The provider interprets the id. */ - readonly get: (id: ProviderItemId) => Effect.Effect; - readonly has?: (id: ProviderItemId) => Effect.Effect; - readonly set?: (id: ProviderItemId, value: string) => Effect.Effect; - readonly delete?: (id: ProviderItemId) => Effect.Effect; + readonly get: ( + id: ProviderItemId, + scope?: CredentialProviderScope, + ) => Effect.Effect; + readonly has?: ( + id: ProviderItemId, + scope?: CredentialProviderScope, + ) => Effect.Effect; + readonly set?: ( + id: ProviderItemId, + value: string, + scope?: CredentialProviderScope, + ) => Effect.Effect; + readonly delete?: ( + id: ProviderItemId, + scope?: CredentialProviderScope, + ) => Effect.Effect; /** Browse entries for discovery (pick a 1Password item). Optional — some * backends can't enumerate. */ readonly list?: () => Effect.Effect; diff --git a/packages/plugins/encrypted-secrets/src/index.test.ts b/packages/plugins/encrypted-secrets/src/index.test.ts index 42ef88139..820a325ea 100644 --- a/packages/plugins/encrypted-secrets/src/index.test.ts +++ b/packages/plugins/encrypted-secrets/src/index.test.ts @@ -9,10 +9,8 @@ import { decryptSecret, deriveKey, encryptSecret, encryptedSecretsPlugin } from // In-memory PluginStorageFacade fake (owner-partitioned), enough to exercise // the provider exactly as the executor's plugin-storage table would. // -// v2: the provider keys values by the opaque `ProviderItemId` (the storage -// `key`); writes carry an `owner` the host supplies. Reads via `get`/`list` -// are not owner-filtered — the connection row that references the id owns the -// partition. +// v2: the provider keys values by the opaque `ProviderItemId` and uses the +// explicit connection scope for owner-partitioned reads and writes. // --------------------------------------------------------------------------- const makeFakeStorage = () => { @@ -131,11 +129,24 @@ describe("provider", () => { expect(stored.startsWith("v1.")).toBe(true); }); - // removed: "a secret in one scope is invisible to another scope" — v2 drops - // the scope arg entirely. The provider keys solely by the opaque - // `ProviderItemId`; the referencing connection row owns the (tenant, owner, - // subject) partition, so cross-scope isolation is no longer the provider's - // concern to enforce or test. + test("a user-bound executor stores and resolves an org connection secret in the org partition", async () => { + const { provider, rows } = makeProvider("master", Owner.make("user")); + const orgConnection = { owner: "org" as const, subject: "" }; + + await Effect.runPromise(provider.set!(id("oauth:org:linear"), "token", orgConnection)); + + expect([...rows.values()][0]!.owner).toBe("org"); + expect( + await Effect.runPromise(provider.get(id("oauth:org:linear"), orgConnection)), + ).toBe("token"); + // A different user-bound executor uses the same explicit org scope; its + // caller subject is intentionally irrelevant to the provider lookup. + expect( + await Effect.runPromise( + provider.get(id("oauth:org:linear"), { owner: "org", subject: "" }), + ), + ).toBe("token"); + }); test("get returns null for a missing id", async () => { const { provider } = makeProvider("master"); diff --git a/packages/plugins/encrypted-secrets/src/index.ts b/packages/plugins/encrypted-secrets/src/index.ts index 8447fb7f0..590504f91 100644 --- a/packages/plugins/encrypted-secrets/src/index.ts +++ b/packages/plugins/encrypted-secrets/src/index.ts @@ -9,6 +9,7 @@ import { ProviderKey, StorageError, type CredentialProvider, + type CredentialProviderScope, type OwnerBinding, type PluginCtx, } from "@executor-js/sdk"; @@ -26,11 +27,10 @@ import { // server, replacing the OS-keychain/plaintext-file providers that assume a // single desktop user. // -// v2: the provider sees only an opaque `ProviderItemId` — there is NO scope -// arg. The connection row that references the id owns the (tenant, owner, -// subject) partition; the encrypted value is keyed solely by the opaque id. -// Plugin storage writes still carry an `owner` (the executor's binding), which -// is captured once from the ctx at provider construction. +// v2: the encrypted value is keyed by an opaque `ProviderItemId`, while +// connection operations explicitly supply the connection row's owner scope. +// This keeps org credentials tenant-shared even when a user-bound executor +// creates or refreshes them, and keeps user credentials under their subject. // --------------------------------------------------------------------------- type PluginStorage = PluginCtx["pluginStorage"]; @@ -87,40 +87,48 @@ const ownerOf = (binding: OwnerBinding): Owner => const makeEncryptedProvider = ( key: Buffer, storage: PluginStorage, - owner: Owner, -): CredentialProvider => ({ - key: ENCRYPTED_PROVIDER_KEY, - writable: true, - - get: (id: ProviderItemId) => - storage - .get({ collection: COLLECTION, key: id }) - .pipe( - Effect.flatMap((entry) => (entry ? decryptSecret(key, entry.data) : Effect.succeed(null))), - ), + defaultOwner: Owner, +): CredentialProvider => { + const ownerFor = (scope?: CredentialProviderScope): Owner => + Owner.make(scope?.owner ?? defaultOwner); + + return { + key: ENCRYPTED_PROVIDER_KEY, + writable: true, + + get: (id: ProviderItemId, scope?: CredentialProviderScope) => + storage + .getForOwner({ collection: COLLECTION, key: id, owner: ownerFor(scope) }) + .pipe( + Effect.flatMap((entry) => (entry ? decryptSecret(key, entry.data) : Effect.succeed(null))), + ), - has: (id: ProviderItemId) => - storage.get({ collection: COLLECTION, key: id }).pipe(Effect.map((entry) => entry !== null)), + has: (id: ProviderItemId, scope?: CredentialProviderScope) => + storage + .getForOwner({ collection: COLLECTION, key: id, owner: ownerFor(scope) }) + .pipe(Effect.map((entry) => entry !== null)), - set: (id: ProviderItemId, value: string) => - encryptSecret(key, value).pipe( - Effect.flatMap((payload) => - storage.put({ collection: COLLECTION, key: id, owner, data: payload }), + set: (id: ProviderItemId, value: string, scope?: CredentialProviderScope) => + encryptSecret(key, value).pipe( + Effect.flatMap((payload) => + storage.put({ collection: COLLECTION, key: id, owner: ownerFor(scope), data: payload }), + ), + Effect.asVoid, ), - Effect.asVoid, - ), - delete: (id: ProviderItemId) => storage.remove({ collection: COLLECTION, key: id, owner }), + delete: (id: ProviderItemId, scope?: CredentialProviderScope) => + storage.remove({ collection: COLLECTION, key: id, owner: ownerFor(scope) }), - list: () => - storage - .list({ collection: COLLECTION }) - .pipe( - Effect.map((entries) => - entries.map((entry) => ({ id: ProviderItemId.make(entry.key), name: entry.key })), + list: () => + storage + .list({ collection: COLLECTION }) + .pipe( + Effect.map((entries) => + entries.map((entry) => ({ id: ProviderItemId.make(entry.key), name: entry.key })), + ), ), - ), -}); + }; +}; export interface EncryptedSecretsPluginConfig { /** diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index 4459f2bc1..1ac90eba7 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -1247,12 +1247,16 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { const transport: string = parsed.transport === "stdio" ? "stdio" : (parsed.remoteTransport ?? "auto"); + const authMethod = + parsed.transport === "remote" + ? selectAuthMethod(parsed, String(credential.template)) + : undefined; + // An apikey method with unresolved inputs fails the invocation // explicitly instead of dialing unauthenticated. if (parsed.transport === "remote") { - const method = selectAuthMethod(parsed, String(credential.template)); - if (method?.kind === "apikey") { - const missing = requiredPlacementVariables(method.placements).filter( + if (authMethod?.kind === "apikey") { + const missing = requiredPlacementVariables(authMethod.placements).filter( (variable) => credential.values[variable] == null, ); if (missing.length > 0) { @@ -1266,14 +1270,6 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { } } - const connector: McpConnector = yield* buildConnectorInput( - parsed, - credential.values, - String(credential.template), - allowStdio, - options?.httpClientLayer ?? ctx.httpClientLayer, - ).pipe(Effect.map((ci) => createMcpConnector(ci))); - const connectionRef = { owner: credential.owner, integration: credential.integration, @@ -1287,6 +1283,59 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { // tools read re-lists instead of serving the drifted catalog. let toolListChanged = false; + const invokeWithValues = (values: Record) => + buildConnectorInput( + parsed, + values, + String(credential.template), + allowStdio, + options?.httpClientLayer ?? ctx.httpClientLayer, + ).pipe( + Effect.map((ci) => createMcpConnector(ci)), + Effect.flatMap((connector: McpConnector) => + invokeMcpTool({ + toolId: String(toolRow.name), + toolName: stamp.toolName, + args, + transport, + connector, + elicit, + onToolListChanged: () => { + toolListChanged = true; + }, + }), + ), + Effect.onExit(() => + toolListChanged + ? ctx.connections.markToolsStale(connectionRef).pipe(Effect.ignore) + : Effect.void, + ), + ); + + const raw = yield* invokeWithValues(credential.values).pipe( + Effect.catchTag("McpInvocationError", (error) => { + if ( + authMethod?.kind !== "oauth2" || + (error.status !== 401 && error.status !== 403) + ) { + return Effect.fail(error); + } + return ctx.connections.resolveValue(connectionRef, { force: true }).pipe( + Effect.flatMap((value) => + invokeWithValues({ ...credential.values, [TOKEN_VARIABLE]: value }), + ), + ); + }), + ); +/* discarded common-ancestor conflict variant + const connector: McpConnector = yield* buildConnectorInput( + parsed, + credential.values, + String(credential.template), + allowStdio, + options?.httpClientLayer ?? ctx.httpClientLayer, + ).pipe(Effect.map((ci) => createMcpConnector(ci))); + const raw = yield* invokeMcpTool({ toolId: String(toolRow.name), toolName: stamp.toolName, @@ -1294,16 +1343,64 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { transport, connector, elicit, - onToolListChanged: () => { - toolListChanged = true; - }, - }).pipe( - Effect.onExit(() => - toolListChanged - ? ctx.connections.markToolsStale(connectionRef).pipe(Effect.ignore) - : Effect.void, - ), + }); +*/ /* discarded pre-upstream conflict variant + const invokeWithValues = ( + values: Record, + ): Effect.Effect< + unknown, + McpConnectionError | McpInvocationError | McpOAuthReauthorizationRequired + > => + buildConnectorInput( + parsed, + values, + String(credential.template), + allowStdio, + options?.httpClientLayer ?? ctx.httpClientLayer, + ).pipe( + Effect.map((ci) => createMcpConnector(ci)), + Effect.flatMap((connector: McpConnector) => + invokeMcpTool({ + toolId: String(toolRow.name), + toolName: stamp.toolName, + args, + transport, + connector, + elicit, + }), + ), + ); + + // A locally unexpired OAuth token can still have been revoked or + // rotated. On an upstream auth response, force one refresh and retry + // with the newly resolved value. The retry is deliberately outside any + // recursive/looping path: a second auth failure reaches the existing + // error mapping below unchanged. + const raw = yield* invokeWithValues(credential.values).pipe( + Effect.catchTag("McpInvocationError", (error) => { + if ( + authMethod?.kind !== "oauth2" || + (error.status !== 401 && error.status !== 403) + ) { + return Effect.fail(error); + } + return ctx.connections + .resolveValue( + { + owner: credential.owner, + integration: credential.integration, + name: credential.connection, + }, + { force: true }, + ) + .pipe( + Effect.flatMap((value) => + invokeWithValues({ ...credential.values, [TOKEN_VARIABLE]: value }), + ), + ); + }), ); +*/ const envelope = Option.getOrUndefined(decodeMcpToolCallEnvelope(raw)); if (envelope?.isError === true) {