From 9dea3263703dba203687eae180bbf00a19e386a2 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 18 Sep 2026 11:41:16 -0700 Subject: [PATCH] fix(oauth): request all advertised scopes, keep sync verdicts visible Scope discovery capped the request at 100 scopes. A resource that advertises more (PostHog lists 150) got a token missing the scopes its MCP server needs, so every new connection synced zero tools. Bound the request by scope-string length (8 KiB) instead. A credential-only health check then reported healthy over the sync-stamped rejection, hiding the failure. Sync-supplied verdicts now carry the tool_sync_failed reason and are served until a sync succeeds. Co-Authored-By: Claude Fable 5.1 --- .changeset/oauth-discovered-scope-budget.md | 5 ++ packages/core/sdk/src/executor.ts | 27 +++++++- packages/core/sdk/src/health-check.ts | 1 + packages/core/sdk/src/oauth-flow.test.ts | 67 +++++++++++++++++++ .../core/sdk/src/oauth-scope-union.test.ts | 45 +++++++++++-- packages/core/sdk/src/oauth-service.ts | 23 ++++++- 6 files changed, 156 insertions(+), 12 deletions(-) create mode 100644 .changeset/oauth-discovered-scope-budget.md diff --git a/.changeset/oauth-discovered-scope-budget.md b/.changeset/oauth-discovered-scope-budget.md new file mode 100644 index 000000000..c71d10c41 --- /dev/null +++ b/.changeset/oauth-discovered-scope-budget.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Request every scope a resource advertises during OAuth scope discovery, bounded by an 8 KiB scope-string budget instead of a 100-scope count. Resources with many fine-grained scopes previously received a token missing the ones it needed. Health checks without a probe no longer replace a tool-sync failure verdict with "healthy". diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index cd89ab907..551032d8d 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3605,7 +3605,15 @@ export const createExecutor = => findConnectionRow(ref).pipe( Effect.flatMap((fresh) => - fresh === null || oauthReauthRequiredFromProviderState(fresh.provider_state) !== null + fresh === null || + oauthReauthRequiredFromProviderState(fresh.provider_state) !== null || + // A credential verdict cannot refute a failed tool sync; only a + // successful sync clears that record (see `isToolSyncHealth`). + isToolSyncHealth(Option.getOrNull(decodeLastHealth(fresh.last_health))) ? Effect.void : persistHealthResult(ref, fresh, result), ), @@ -5241,6 +5253,17 @@ export const createExecutor = + result.status === "healthy" && previous !== null && isToolSyncHealth(previous) + ? previous + : result, + ), Effect.tap((result) => persistProbeHealthResult(ref, result)), Effect.map((result) => ({ source: "credential_only" as const, diff --git a/packages/core/sdk/src/health-check.ts b/packages/core/sdk/src/health-check.ts index a4d6f1cb2..9e39047d5 100644 --- a/packages/core/sdk/src/health-check.ts +++ b/packages/core/sdk/src/health-check.ts @@ -145,6 +145,7 @@ export type HealthCheckResult = typeof HealthCheckResult.Type; export const toolSyncHealthDetailPrefix = "Tool sync failing"; export const isToolSyncHealth = (result: HealthCheckResult | null | undefined): boolean => + result?.reason === "tool_sync_failed" || result?.detail?.startsWith(toolSyncHealthDetailPrefix) === true; // --------------------------------------------------------------------------- diff --git a/packages/core/sdk/src/oauth-flow.test.ts b/packages/core/sdk/src/oauth-flow.test.ts index 7496ce1ef..b98d50071 100644 --- a/packages/core/sdk/src/oauth-flow.test.ts +++ b/packages/core/sdk/src/oauth-flow.test.ts @@ -2033,6 +2033,73 @@ describe("oauth token refresh in resolveConnectionValue", () => { ), ); + it.effect( + "checkHealth without a probe serves a sync-stamped verdict instead of burying it under healthy", + () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); + const { executor, config } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + + yield* executor.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + resource: server.mcpResourceUrl, + }); + + const started = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + yield* executor.oauth.complete({ state: started.state, code: callback.code }); + + // Tool sync found the upstream rejecting the freshly minted token + // (e.g. an MCP discovery handshake answering 401) and stamped it. + // The token itself still resolves, so a credential-only check would + // otherwise report healthy and hide a connection that has no tools. + const stamped = { + status: "expired", + checkedAt: Date.now(), + detail: "MCP OAuth reauthorization required", + reason: "tool_sync_failed", + }; + yield* Effect.promise(() => + config.db.updateMany("connection", { + where: (b) => b("name", "=", "main"), + set: { last_health: stamped }, + }), + ); + + const result = yield* executor.connections.checkHealth({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }); + expect(result).toMatchObject(stamped); + + const row = yield* Effect.promise(() => + config.db.findFirst("connection", { where: (b) => b("name", "=", "main") }), + ); + expect(row?.last_health).toMatchObject(stamped); + }), + ), + ); + it.effect("records missing authorization-code scopes without blocking the connection", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/core/sdk/src/oauth-scope-union.test.ts b/packages/core/sdk/src/oauth-scope-union.test.ts index 47b92d16e..97dbd3dce 100644 --- a/packages/core/sdk/src/oauth-scope-union.test.ts +++ b/packages/core/sdk/src/oauth-scope-union.test.ts @@ -713,13 +713,42 @@ describe("oauth.start integration-driven scopes", () => { ), ); - it.effect("(j) caps server-advertised resource scopes so the authorize URL stays bounded", () => + it.effect("(j) requests every advertised scope of a large but realistic resource list", () => Effect.scoped( Effect.gen(function* () { - // A hostile/buggy server advertises far more scopes than any real - // template. Discovery caps the request at 100 so the authorize URL - // cannot be blown up. - const manyScopes = Array.from({ length: 200 }, (_, i) => `scope:${i}`); + // A fine-grained resource can legitimately advertise well over a + // hundred scopes (PostHog lists 150). Dropping any of them mints a + // token the resource rejects, so the whole list must be requested. + const manyScopes = Array.from( + { length: 150 }, + (_, i) => `resource_${i}:${i % 2 === 0 ? "read" : "write"}`, + ); + const server = yield* serveMetadataServer({ prm: { scopesSupported: manyScopes } }); + const executor = yield* setupMcpScopeClient(server); + + const started = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + + expect(scopesFromAuthorizeUrl(started.authorizationUrl)).toEqual(manyScopes); + }), + ), + ); + + it.effect("(j2) caps server-advertised resource scopes so the authorize URL stays bounded", () => + Effect.scoped( + Effect.gen(function* () { + // A hostile/buggy server advertises an absurd list. Discovery keeps + // the longest leading prefix whose joined `scope` value fits the + // 8 KiB budget so the authorize URL cannot be blown up. + const manyScopes = Array.from({ length: 2000 }, (_, i) => `scope:${i}`); const server = yield* serveMetadataServer({ prm: { scopesSupported: manyScopes } }); const executor = yield* setupMcpScopeClient(server); @@ -735,8 +764,10 @@ describe("oauth.start integration-driven scopes", () => { if (started.status !== "redirect") return; const requested = scopesFromAuthorizeUrl(started.authorizationUrl); - expect(requested.length).toBe(100); - expect(requested).toEqual(manyScopes.slice(0, 100)); + expect(requested.length).toBeLessThan(manyScopes.length); + expect(requested).toEqual(manyScopes.slice(0, requested.length)); + expect(requested.join(" ").length).toBeLessThanOrEqual(8192); + expect([...requested, manyScopes[requested.length]].join(" ").length).toBeGreaterThan(8192); }), ), ); diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index a98eeda03..7d8d7d46e 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -782,9 +782,26 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // Caps on server-controlled discovery input — a hostile or buggy server must // not be able to hang `oauth.start` or overflow the authorize URL. const MAX_DISCOVERY_AUTH_SERVERS = 3; // AS-failover lists are tiny in practice - const MAX_DISCOVERED_SCOPES = 100; // far beyond any realistic authorization template - const capScopes = (scopes: readonly string[]): readonly string[] => - dedupeScopes(scopes).slice(0, MAX_DISCOVERED_SCOPES); + // The cap is on the encoded `scope` parameter's length, not the scope + // count: the URL is what overflows, and a real resource can legitimately + // advertise well over a hundred fine-grained scopes (PostHog lists 150). + // Dropping any advertised scope silently mints a token the resource then + // rejects, so the budget is generous — 8 KiB leaves room for the rest of the + // authorize URL under the common 8-16 KiB request-line limits — and only an + // absurd list is truncated. + const MAX_DISCOVERED_SCOPE_CHARS = 8192; + const capScopes = (scopes: readonly string[]): readonly string[] => { + const unique = dedupeScopes(scopes); + let length = 0; + let count = 0; + for (const scope of unique) { + const next = length + scope.length + (count > 0 ? 1 : 0); + if (next > MAX_DISCOVERED_SCOPE_CHARS) break; + length = next; + count += 1; + } + return unique.slice(0, count); + }; // Bound a whole discovery sequence (PRM + up to MAX_DISCOVERY_AUTH_SERVERS AS // fetches, each with its own request timeout). 30s is larger than a single