diff --git a/.changeset/oauth-local-reject-user-client.md b/.changeset/oauth-local-reject-user-client.md new file mode 100644 index 0000000000..f83e7a7ab4 --- /dev/null +++ b/.changeset/oauth-local-reject-user-client.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Reject user-owned OAuth clients when the subject is local, including before DCR. diff --git a/apps/cloud/src/mcp/session-build-semaphore.test.ts b/apps/cloud/src/mcp/session-build-semaphore.test.ts index 3d4ad76343..584b65ee0e 100644 --- a/apps/cloud/src/mcp/session-build-semaphore.test.ts +++ b/apps/cloud/src/mcp/session-build-semaphore.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, beforeEach } from "@effect/vitest"; +import { describe, expect, it, beforeEach, afterEach, vi } from "@effect/vitest"; import { acquireBuildSlot, @@ -13,6 +13,10 @@ describe("session-build-semaphore", () => { resetBuildSlotsForTest(); }); + afterEach(() => { + vi.useRealTimers(); + }); + it("grants up to the cap immediately, with no wait", async () => { const results = await Promise.all([ acquireBuildSlot().promise, @@ -214,6 +218,7 @@ describe("session-build-semaphore", () => { }); it("proceeds without a slot when the queue wait exceeds the timeout, and does not count it as active", async () => { + vi.useFakeTimers(); await Promise.all([ acquireBuildSlot().promise, acquireBuildSlot().promise, @@ -223,6 +228,10 @@ describe("session-build-semaphore", () => { expect(currentActiveBuildsForTest()).toBe(4); const timedOutHandle = acquireBuildSlot(10); + await vi.advanceTimersByTimeAsync(9); + expect(currentQueueLengthForTest()).toBe(1); + expect(currentActiveBuildsForTest()).toBe(4); + await vi.advanceTimersByTimeAsync(1); const result = await timedOutHandle.promise; expect(result).toEqual({ acquired: false, waitMs: expect.any(Number), timedOut: true }); diff --git a/e2e/local/oauth-client-ownership.test.ts b/e2e/local/oauth-client-ownership.test.ts new file mode 100644 index 0000000000..5792851968 --- /dev/null +++ b/e2e/local/oauth-client-ownership.test.ts @@ -0,0 +1,156 @@ +import { expect } from "@effect/vitest"; +import { connectEmulator } from "@executor-js/emulate"; +import { Effect } from "effect"; +import { HttpApiClient } from "effect/unstable/httpapi"; +import { FetchHttpClient, HttpClient, HttpClientRequest } from "effect/unstable/http"; +import { composePluginApi } from "@executor-js/api/server"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, +} from "@executor-js/sdk/shared"; + +import { createEmulatorInstance } from "../src/emulator-instance"; +import { scenario } from "../src/scenario"; +import { Browser, Cli, RunDir } from "../src/services"; +import { withLocalServer } from "./local-server"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +scenario( + "Local OAuth · reject user clients before registration while org clients connect", + { timeout: 180_000 }, + Effect.scoped( + Effect.gen(function* () { + const cli = yield* Cli; + const runDir = yield* RunDir; + const browser = yield* Browser; + const base = yield* createEmulatorInstance("mcp", "local-ownership"); + const emulator = yield* Effect.promise(() => + connectEmulator({ baseUrl: base, service: "mcp" }), + ); + yield* Effect.promise(() => emulator.seed({ users: [{ login: "local-oauth-user" }] })); + yield* withLocalServer(cli, runDir, (server) => + Effect.gen(function* () { + const client = yield* HttpApiClient.make(api, { + baseUrl: new URL("/api", server.origin).toString(), + transformClient: HttpClient.mapRequest((request) => + HttpClientRequest.setHeader(request, "authorization", `Bearer ${server.token}`), + ), + }).pipe(Effect.provide(FetchHttpClient.layer)); + // The CLI fixture binds port0; pass its actual callback through the + // public override instead of its pre-bind default port. + const redirectUri = new URL("/api/oauth/callback", server.origin).toString(); + const endpoints = { authorizationUrl: `${base}/authorize`, tokenUrl: `${base}/token` }; + const slug = IntegrationSlug.make("local-oauth-owner"); + const registration = { + slug: OAuthClientSlug.make("local-oauth-app"), + issuer: base, + registrationEndpoint: `${base}/register`, + ...endpoints, + resource: `${base}/mcp`, + scopes: ["repo", "read:user"], + tokenEndpointAuthMethodsSupported: ["none"], + redirectUri, + originIntegration: slug, + }; + const rejected = yield* client.oauth + .createClient({ + payload: { + owner: "user", + slug: OAuthClientSlug.make("forbidden-personal"), + grant: "authorization_code", + ...endpoints, + clientId: "unused-client", + clientSecret: "unused-secret", + }, + }) + .pipe(Effect.flip); + expect(rejected._tag).toBe("InternalError"); + const deniedDcr = yield* client.oauth + .registerDynamic({ payload: { ...registration, owner: "user" } }) + .pipe(Effect.flip); + expect(deniedDcr._tag).toBe("InternalError"); + const before = yield* Effect.promise(() => emulator.ledger.list()); + expect(before.some((entry) => entry.path === "/register")).toBe(false); + expect((yield* client.oauth.listClients()).some((entry) => entry.owner === "user")).toBe( + false, + ); + yield* client.mcp.addServer({ + payload: { + slug, + name: "Local OAuth", + endpoint: `${base}/mcp`, + transport: "remote", + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + yield* Effect.addFinalizer(() => + client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore), + ); + const registered = yield* client.oauth.registerDynamic({ + payload: { ...registration, owner: "org" }, + }); + yield* Effect.addFinalizer(() => + client.oauth + .removeClient({ params: { slug: registered.client }, payload: { owner: "org" } }) + .pipe(Effect.ignore), + ); + const started = yield* client.oauth.start({ + payload: { + owner: "org", + client: registered.client, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: slug, + template: AuthTemplateSlug.make("oauth2"), + redirectUri, + }, + }); + if (started.status !== "redirect") return yield* Effect.die("Expected OAuth redirect"); + yield* browser.session({ label: "local" }, async ({ page, step }) => { + await step("Sign in to the local console", async () => { + await page.goto(server.url); + await page + .getByTestId("integration-entry-executor") + .first() + .waitFor({ timeout: 30_000 }); + }); + await step("Authorize an org-owned OAuth client", async () => { + await page.goto(started.authorizationUrl); + await page.getByText("Authorize MCP client", { exact: true }).waitFor(); + const authorize = new URL(started.authorizationUrl); + const approved = await page.request.post(`${base}/authorize/approve`, { + form: { ...Object.fromEntries(authorize.searchParams), login: "local-oauth-user" }, + maxRedirects: 0, + }); + expect(approved.status()).toBe(302); + const callback = approved.headers().location; + if (!callback) throw new Error("Missing OAuth callback"); + await page.goto(callback); + await page.getByText("Connected", { exact: true }).waitFor({ timeout: 30_000 }); + }); + }); + const catalog = yield* client.tools.list({ query: { integration: slug } }); + expect(catalog.some((entry) => entry.name === "get_me" && entry.owner === "org")).toBe( + true, + ); + const ledger = yield* Effect.promise(() => emulator.ledger.list()); + expect( + ledger.some((entry) => entry.path === "/token" && entry.response.status === 200), + ).toBe(true); + expect( + ledger.some( + (entry) => + entry.path === "/mcp" && + entry.response.status === 200 && + entry.identity.user?.login === "local-oauth-user", + ), + ).toBe(true); + }), + ); + }), + ), +); diff --git a/packages/core/sdk/src/oauth-flow.test.ts b/packages/core/sdk/src/oauth-flow.test.ts index b9860574bb..1857a6e18e 100644 --- a/packages/core/sdk/src/oauth-flow.test.ts +++ b/packages/core/sdk/src/oauth-flow.test.ts @@ -263,6 +263,54 @@ describe("oauth.start / oauth.complete", () => { }), ), ); + it.effect("createClient rejects owner user when subject is local", () => + Effect.gen(function* () { + const executor = yield* createExecutor(makeTestConfig({ plugins, subject: "local" })); + let seen = false; + yield* executor.oauth + .createClient({ + owner: "user", + slug: OAuthClientSlug.make("personal"), + authorizationUrl: "https://example.com/authorize", + tokenUrl: "https://example.com/token", + grant: "authorization_code", + clientId: "id", + clientSecret: "secret", + }) + .pipe( + Effect.catchTag("StorageError", (err) => { + seen = true; + expect(err.message).toContain("User-owned OAuth clients are not supported"); + return Effect.void; + }), + ); + expect(seen).toBe(true); + }), + ); + + it.effect("local user DCR is rejected before contacting the provider", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer(); + const executor = yield* createExecutor(makeTestConfig({ plugins, subject: "local" })); + const error = yield* executor.oauth + .registerDynamicClient({ + owner: "user", + slug: CLIENT, + issuer: server.issuerUrl, + registrationEndpoint: server.registrationEndpoint, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + scopes: ["read"], + redirectUri: "http://localhost/callback", + }) + .pipe(Effect.flip); + expect(Predicate.isTagged("StorageError")(error)).toBe(true); + expect(yield* server.requests).toEqual([]); + expect(yield* executor.oauth.listClients()).toEqual([]); + }), + ), + ); it.effect("persists HTTP Basic client auth for code exchange and refresh", () => Effect.scoped( @@ -999,7 +1047,7 @@ describe("oauth.start / oauth.complete", () => { ); expect(Predicate.isTagged("OAuthStartError")(error)).toBe(true); const startError = error as OAuthStartError; - expect(startError.message).toContain("must use a Workspace app"); + expect(startError.message).toContain("must use an org-owned OAuth client"); }), ), ); diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index dd443144bd..0519bcc6fd 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -878,6 +878,13 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { input: CreateOAuthClientInput, ): Effect.Effect => Effect.gen(function* () { + if (input.owner === "user" && deps.subject === "local") { + return yield* new StorageError({ + message: + 'User-owned OAuth clients are not supported on single-workspace hosts. Use owner "org" instead.', + cause: undefined, + }); + } // The `first-party:` namespace is reserved for config-declared apps — a // stored row under it would be shadowed by (or worse, impersonate) the // host's own app. @@ -1393,6 +1400,13 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { OAuthRegisterDynamicError | OrgWriteDeniedError | StorageFailure > => Effect.gen(function* () { + if (input.owner === "user" && deps.subject === "local") { + return yield* new StorageError({ + message: + 'User-owned OAuth clients are not supported on single-workspace hosts. Use owner "org" instead.', + cause: undefined, + }); + } yield* deps.guardOrgWrite(input.owner); const issuer = canonicalDcrIssuer(input.issuer, input.registrationEndpoint); // Resolved before the reuse decision: a persisted client registered with @@ -1671,7 +1685,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { }); if (!firstPartyFlow && input.owner === "org" && input.clientOwner === "user") { return yield* new OAuthStartError({ - message: "A Workspace connection must use a Workspace app.", + message: "An org connection must use an org-owned OAuth client.", }); } // Load the app by its EXPLICIT owner (the caller knows it — no derivation).