From 5aff0a09a0a47b8ee1453007d4bc34dd92810d1f Mon Sep 17 00:00:00 2001 From: Shardul Baral <16765155+shardulbee@users.noreply.github.com> Date: Sat, 10 Oct 2026 17:37:04 +0000 Subject: [PATCH] Report non-credential 403s as upstream_forbidden with the upstream message --- .changeset/upstream-forbidden-not-rejected.md | 5 + .../oauth-scope-insufficient.test.ts | 17 +- e2e/scenarios/upstream-forbidden.test.ts | 206 ++++++++++++++++++ .../core/sdk/src/credential-rejection.test.ts | 84 +++++++ packages/core/sdk/src/credential-rejection.ts | 155 +++++++++++++ packages/core/sdk/src/index.ts | 1 + .../plugins/graphql/src/sdk/plugin.test.ts | 50 ++++- packages/plugins/graphql/src/sdk/plugin.ts | 27 ++- packages/plugins/openapi/src/sdk/backing.ts | 30 ++- .../openapi/src/sdk/upstream-failures.test.ts | 83 ++++++- 10 files changed, 647 insertions(+), 11 deletions(-) create mode 100644 .changeset/upstream-forbidden-not-rejected.md create mode 100644 e2e/scenarios/upstream-forbidden.test.ts create mode 100644 packages/core/sdk/src/credential-rejection.test.ts create mode 100644 packages/core/sdk/src/credential-rejection.ts diff --git a/.changeset/upstream-forbidden-not-rejected.md b/.changeset/upstream-forbidden-not-rejected.md new file mode 100644 index 0000000000..8f2f9ee31d --- /dev/null +++ b/.changeset/upstream-forbidden-not-rejected.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +An upstream HTTP 403 from an OpenAPI or GraphQL tool no longer claims the connection's credentials were rejected unless the response says so (an authentication challenge, a credential error code, or a message naming a bad credential). Other 403s, such as a feature gated by the account's plan, fail with `upstream_forbidden` and the upstream's own message. Rejected-credential failures now include the upstream's message too, so MCP callers see the reason in the text result. diff --git a/e2e/scenarios/oauth-scope-insufficient.test.ts b/e2e/scenarios/oauth-scope-insufficient.test.ts index dcfea135e4..403f4e15b1 100644 --- a/e2e/scenarios/oauth-scope-insufficient.test.ts +++ b/e2e/scenarios/oauth-scope-insufficient.test.ts @@ -12,8 +12,8 @@ // only `mail.read`. Calling the `files.read` operation dispatches upstream // (catalog projection by scope is #1384, tracked separately), which answers // with a Google-shaped scope-insufficient 403 — and the failure the sandbox -// sees carries the new code, while the ordinary revoked-token 403 stays -// `connection_rejected`. +// sees carries the new code, while an ordinary permission 403 is reported as +// `upstream_forbidden` with the upstream's reason. import { randomBytes } from "node:crypto"; import { createServer } from "node:http"; @@ -302,14 +302,19 @@ scenario( "the recovery tells the agent to reconnect with broader access", ).toContain("broader access"); - // An ordinary 403 with no scope signal keeps the existing - // classification — the new code is additive, not a re-label. + // An ordinary 403 with no scope or credential signal is a refusal + // of the operation, not of the credential: it reports the + // upstream's reason and does not send the agent to re-authenticate. const plainForbidden = yield* invoke(addressOf("adminAction")); expect(plainForbidden.ok, "the plain-403 call failed").toBe(false); expect( plainForbidden.error?.code, - "a 403 without a scope signal stays connection_rejected", - ).toBe("connection_rejected"); + "a 403 without a scope or credential signal is upstream_forbidden", + ).toBe("upstream_forbidden"); + expect( + plainForbidden.error?.message ?? "", + "the message carries the upstream's reason", + ).toContain("not allowed"); // And the in-scope operation works, proving the connection is fine. const inScope = yield* invoke(addressOf("readInbox")); diff --git a/e2e/scenarios/upstream-forbidden.test.ts b/e2e/scenarios/upstream-forbidden.test.ts new file mode 100644 index 0000000000..cf55bc0bbf --- /dev/null +++ b/e2e/scenarios/upstream-forbidden.test.ts @@ -0,0 +1,206 @@ +// Cross-target: an upstream 403 that is not about the credential (a feature +// gated by the account's plan, a resource the account may not touch) must not +// tell the caller to re-authenticate. MCP text results carry only +// `code: message`, so the upstream's own reason has to be in the message, or +// the caller is left re-checking a connection that works. A 401 is still a +// rejected credential, and its message carries the upstream reason too. +import { randomBytes } from "node:crypto"; +import { createServer } from "node:http"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; +import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/sdk/shared"; + +import { decodeToolSearch } from "./support/search-invoke"; + +import { scenario } from "../src/scenario"; +import { Api, Mcp, Target } from "../src/services"; + +const api = composePluginApi([openApiHttpPlugin()] as const); + +const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`; + +const PLAN_MESSAGE = "feature not available on current billing plan"; + +/** Upstream on 127.0.0.1: `POST /devices/{id}/attributes/{key}` is gated by + * the account's plan (403), `POST /tags` succeeds with the same credential, + * and `GET /session` answers 401 for a revoked token. */ +const serveUpstream = () => + Effect.acquireRelease( + Effect.callback<{ readonly url: string; readonly close: () => void }>((resume) => { + const server = createServer((request, response) => { + const path = request.url ?? ""; + request.resume(); + const send = (status: number, body: unknown) => { + response.writeHead(status, { "content-type": "application/json" }); + response.end(JSON.stringify(body)); + }; + if (request.method === "POST" && path.startsWith("/devices/")) { + return send(403, { message: PLAN_MESSAGE }); + } + if (request.method === "POST" && path.startsWith("/tags")) { + return send(200, { ok: true }); + } + if (request.method === "GET" && path.startsWith("/session")) { + return send(401, { message: "token revoked" }); + } + return send(404, { message: "not found" }); + }); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + resume( + Effect.succeed({ + url: `http://127.0.0.1:${port}`, + close: () => { + server.close(); + server.closeAllConnections(); + }, + }), + ); + }); + }), + (server) => Effect.sync(server.close), + ); + +const spec = (baseUrl: string): string => + JSON.stringify({ + openapi: "3.0.3", + info: { title: "Devices API", version: "1.0.0" }, + servers: [{ url: baseUrl }], + paths: { + "/devices/{id}/attributes/{key}": { + post: { + operationId: "setDeviceAttribute", + tags: ["devices"], + parameters: [ + { name: "id", in: "path", required: true, schema: { type: "string" } }, + { name: "key", in: "path", required: true, schema: { type: "string" } }, + ], + responses: { "200": { description: "ok" } }, + }, + }, + "/tags": { + post: { + operationId: "setTags", + tags: ["devices"], + responses: { "200": { description: "ok" } }, + }, + }, + "/session": { + get: { + operationId: "getSession", + tags: ["session"], + responses: { "200": { description: "ok" } }, + }, + }, + }, + }); + +scenario( + "Auth failures · a plan-gated 403 reports the upstream reason instead of asking to re-authenticate", + { timeout: 120_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const mcp = yield* Mcp; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const upstream = yield* serveUpstream(); + const slug = unique("forbid"); + + yield* Effect.ensuring( + Effect.gen(function* () { + yield* client.openapi.addSpec({ + payload: { + spec: { kind: "blob", value: spec(upstream.url) }, + slug, + baseUrl: upstream.url, + authenticationTemplate: [ + { + slug: "apiKey", + type: "apiKey", + headers: { authorization: ["Bearer ", { type: "variable", name: "token" }] }, + }, + ], + }, + }); + yield* client.connections.create({ + payload: { + owner: "org", + name: ConnectionName.make("main"), + integration: IntegrationSlug.make(slug), + template: AuthTemplateSlug.make("apiKey"), + value: "tok_valid", + }, + }); + + const passthrough = mcp.session(identity, { mode: "passthrough", artifacts: false }); + const found = decodeToolSearch( + (yield* passthrough.call("search", { + query: slug, + integration: slug, + owner: "org", + connection: "main", + })).raw, + ).structuredContent; + const toolId = (operation: string) => { + const tool = found.items.find((item) => item.id.endsWith(`.${operation}`)); + expect(tool, `search lists ${operation}`).toBeDefined(); + return tool!.id; + }; + + // The same connection writes fine: the credential is accepted. + const tagged = yield* passthrough.call("invoke", { + tool: toolId("setTags"), + arguments: {}, + }); + expect(tagged.ok, `a write on the same connection succeeds: ${tagged.text}`).toBe(true); + + // THE guarantee: the plan-gated 403 text names the upstream's reason + // and status, and does not blame the credential. + const gated = yield* passthrough.call("invoke", { + tool: toolId("setDeviceAttribute"), + arguments: { id: "dev_1", key: "custom:tier" }, + }); + expect(gated.ok, "the plan-gated call fails").toBe(false); + expect(gated.text, "the caller sees the upstream's reason").toContain(PLAN_MESSAGE); + expect(gated.text, "the caller sees the upstream status").toContain("HTTP 403"); + expect(gated.text, "the failure is a forbidden, not a rejected credential").toContain( + "upstream_forbidden", + ); + expect(gated.text, "no re-authenticate advice for a working connection").not.toMatch( + /re-?authenticate|rejected credentials/i, + ); + + // A 401 is still a rejected credential, now with the upstream's reason. + const revoked = yield* passthrough.call("invoke", { + tool: toolId("getSession"), + arguments: {}, + }); + expect(revoked.ok, "the revoked-token call fails").toBe(false); + expect(revoked.text).toContain("connection_rejected"); + expect(revoked.text, "the 401 carries the upstream's reason").toContain("token revoked"); + expect(revoked.text, "a 401 still asks for re-authentication").toContain( + "Re-authenticate", + ); + }), + Effect.gen(function* () { + yield* client.connections + .remove({ + params: { + owner: "org", + integration: IntegrationSlug.make(slug), + name: ConnectionName.make("main"), + }, + }) + .pipe(Effect.ignore); + yield* client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore); + }), + ); + }), + ), +); diff --git a/packages/core/sdk/src/credential-rejection.test.ts b/packages/core/sdk/src/credential-rejection.test.ts new file mode 100644 index 0000000000..4382573276 --- /dev/null +++ b/packages/core/sdk/src/credential-rejection.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it } from "@effect/vitest"; + +import { isCredentialRejection, upstreamErrorMessage } from "./credential-rejection"; + +describe("isCredentialRejection", () => { + it("treats every 401 as a credential rejection", () => { + expect(isCredentialRejection({ status: 401, body: "" })).toBe(true); + expect(isCredentialRejection({ status: 401, body: { message: "nope" } })).toBe(true); + }); + + it("does not treat a plan or permission 403 as a credential rejection", () => { + for (const body of [ + { message: "feature not available on current billing plan" }, + '{"message":"feature not available on current billing plan"}', + { error: { status: "PERMISSION_DENIED", message: "Caller lacks permission" } }, + { message: "Resource not accessible by integration" }, + "Forbidden", + "", + null, + ]) { + expect(isCredentialRejection({ status: 403, body }), JSON.stringify(body)).toBe(false); + } + }); + + it("treats a 403 carrying an authentication challenge as a credential rejection", () => { + expect( + isCredentialRejection({ + status: 403, + body: null, + headers: { "WWW-Authenticate": 'Bearer realm="api", error="invalid_token"' }, + }), + ).toBe(true); + }); + + it("treats a 403 naming a credential error code as a credential rejection", () => { + expect(isCredentialRejection({ status: 403, body: { error: "invalid_token" } })).toBe(true); + expect( + isCredentialRejection({ + status: 403, + body: { error: { status: "UNAUTHENTICATED", message: "Request had invalid auth" } }, + }), + ).toBe(true); + }); + + it("treats a 403 whose message names a bad credential as a credential rejection", () => { + for (const body of [ + { message: "Invalid API key" }, + { message: "Bad credentials" }, + { errors: [{ detail: "The access token has expired" }] }, + "API key is invalid", + { error: "Not authenticated" }, + ]) { + expect(isCredentialRejection({ status: 403, body }), JSON.stringify(body)).toBe(true); + } + }); + + it("does not classify non-credential prose that shares a word", () => { + expect(isCredentialRejection({ status: 403, body: { message: "invalid request body" } })).toBe( + false, + ); + expect(isCredentialRejection({ status: 403, body: { message: "missing field: name" } })).toBe( + false, + ); + }); +}); + +describe("upstreamErrorMessage", () => { + it("reads the message from JSON and plain-text bodies", () => { + expect(upstreamErrorMessage({ message: "feature not available on current billing plan" })).toBe( + "feature not available on current billing plan", + ); + expect(upstreamErrorMessage('{"error":{"message":"db timeout"}}')).toBe("db timeout"); + expect(upstreamErrorMessage("Forbidden")).toBe("Forbidden"); + expect(upstreamErrorMessage({ errors: [{ message: "Not authenticated" }] })).toBe( + "Not authenticated", + ); + }); + + it("returns undefined when the body carries no text", () => { + expect(upstreamErrorMessage("")).toBeUndefined(); + expect(upstreamErrorMessage(null)).toBeUndefined(); + expect(upstreamErrorMessage({ data: null })).toBeUndefined(); + }); +}); diff --git a/packages/core/sdk/src/credential-rejection.ts b/packages/core/sdk/src/credential-rejection.ts new file mode 100644 index 0000000000..4c45b22ff6 --- /dev/null +++ b/packages/core/sdk/src/credential-rejection.ts @@ -0,0 +1,155 @@ +// Telling a credential rejection apart from an ordinary "forbidden". An +// upstream 401 always means the credential was not accepted. A 403 is +// ambiguous: it is also how APIs refuse an operation the caller IS +// authenticated for (billing plan, resource permissions, org policy). Labelling +// those connection_rejected sends the agent and the user to re-authenticate a +// connection that works, so a 403 counts as a credential rejection only when +// the response itself says so: +// +// - a WWW-Authenticate challenge (RFC 7235: the server is asking for +// credentials). Scope-insufficient challenges are classified separately +// by detectInsufficientScope before this runs. +// - an OAuth/RPC error code naming the credential (`invalid_token`, +// `UNAUTHENTICATED`, ...) in the body. +// - an error message naming a bad, expired, or missing credential +// ("Invalid API key", "token expired", "Bad credentials"). +// +// A miss is benign: the failure is reported as an upstream 403 carrying the +// upstream's own message, so a credential problem is still legible. + +import { parseChallenges } from "./www-authenticate"; + +const MAX_DEPTH = 8; +const MESSAGE_CAP = 500; + +const isRecord = (value: unknown): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value); + +const parseJsonSafe = (text: string): unknown => { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: classifying an untrusted upstream error body; a parse failure just means "not a JSON body", never an error path + try { + // oxlint-disable-next-line executor/no-json-parse -- boundary: same untrusted-body classification; the parsed value is only structurally inspected, never decoded into domain types + return JSON.parse(text) as unknown; + } catch { + return undefined; + } +}; + +// Exact error-code values (OAuth 2.0 / RFC 6750, google.rpc, common REST +// envelopes) that name the credential rather than the operation. +const CREDENTIAL_ERROR_CODES = new Set([ + "invalid_token", + "invalid_client", + "invalid_grant", + "unauthorized_client", + "unauthenticated", + "not_authenticated", + "authentication_failed", + "authentication_required", + "invalid_api_key", + "invalid_credentials", + "bad_credentials", + "token_expired", + "expired_token", + "access_token_expired", + "credentials_missing", + "api_key_invalid", + "api_key_expired", +]); + +const CODE_FIELDS = ["error", "code", "status", "reason", "type", "error_code", "errorCode"]; +const MESSAGE_FIELDS = [ + "message", + "error_description", + "errorMessage", + "detail", + "title", + "description", + "error", +]; + +// Credential vocabulary in prose. Both halves are required for the first +// pattern so "invalid request" or "missing field" never classify. +const CREDENTIAL_MESSAGE_PATTERNS = [ + /\b(?:invalid|expired|revoked|missing|malformed|incorrect|unknown|disabled|bad|wrong)\b[\w\s-]{0,30}?\b(?:tokens?|credentials?|api[ _-]?keys?|access[ _-]?keys?|secret|signature|authorization header|bearer)\b/i, + /\b(?:tokens?|credentials?|api[ _-]?keys?|access[ _-]?keys?|signature)\b[\w\s-]{0,30}?\b(?:is |are |has |have )?(?:invalid|expired|revoked|missing|malformed|incorrect|not valid|been revoked)\b/i, + /\b(?:unauthenticated|not authenticated|authentication (?:failed|required|is required|credentials)|login required|requires authentication)\b/i, +]; + +const normalizedBody = (body: unknown): unknown => { + if (typeof body !== "string") return body; + const parsed = parseJsonSafe(body); + return parsed === undefined ? body : parsed; +}; + +const collect = ( + body: unknown, + depth: number, + out: { codes: string[]; messages: string[] }, +): void => { + if (depth > MAX_DEPTH) return; + if (Array.isArray(body)) { + for (const item of body) collect(item, depth + 1, out); + return; + } + if (!isRecord(body)) return; + for (const field of CODE_FIELDS) { + const value = body[field]; + if (typeof value === "string") out.codes.push(value); + } + for (const field of MESSAGE_FIELDS) { + const value = body[field]; + if (typeof value === "string" && value.trim().length > 0) out.messages.push(value.trim()); + } + for (const value of Object.values(body)) { + if (isRecord(value) || Array.isArray(value)) collect(value, depth + 1, out); + } +}; + +const bodySignals = (body: unknown): { codes: string[]; messages: string[] } => { + const out = { codes: [] as string[], messages: [] as string[] }; + const normalized = normalizedBody(body); + if (typeof normalized === "string") { + const text = normalized.trim(); + if (text.length > 0) out.messages.push(text.slice(0, MESSAGE_CAP)); + return out; + } + collect(normalized, 0, out); + return out; +}; + +const hasAuthChallenge = (headers: Record | undefined): boolean => + Object.entries(headers ?? {}).some(([name, value]) => { + if (name.toLowerCase() !== "www-authenticate") return false; + const challenges = parseChallenges(value); + return challenges !== null && challenges.length > 0; + }); + +/** The upstream's own error text, when its body carries one: the first + * message-like field of a JSON error envelope, or a plain-text body. Used to + * put the upstream's words in front of the caller instead of only in + * structured details. */ +export const upstreamErrorMessage = (body: unknown): string | undefined => { + const message = bodySignals(body).messages[0]; + if (message === undefined) return undefined; + return message.length > MESSAGE_CAP ? `${message.slice(0, MESSAGE_CAP)}…` : message; +}; + +/** Whether an upstream 401/403 means the credential itself was not accepted + * (so re-authenticating or updating the connection is the fix). A 401 always + * does; a 403 only when its challenge header, error code, or message says + * so. Run detectInsufficientScope first: a scope shortfall is its own case. */ +export const isCredentialRejection = (input: { + readonly status: number; + readonly body?: unknown; + readonly headers?: Record; +}): boolean => { + if (input.status === 401) return true; + if (input.status !== 403) return false; + if (hasAuthChallenge(input.headers)) return true; + const { codes, messages } = bodySignals(input.body); + if (codes.some((code) => CREDENTIAL_ERROR_CODES.has(code.toLowerCase()))) return true; + return messages.some((message) => + CREDENTIAL_MESSAGE_PATTERNS.some((pattern) => pattern.test(message)), + ); +}; diff --git a/packages/core/sdk/src/index.ts b/packages/core/sdk/src/index.ts index 627d3de1de..79c288b8ab 100644 --- a/packages/core/sdk/src/index.ts +++ b/packages/core/sdk/src/index.ts @@ -529,6 +529,7 @@ export { insufficientScopeFromEmbeddedJson, type InsufficientScopeDetection, } from "./insufficient-scope"; +export { isCredentialRejection, upstreamErrorMessage } from "./credential-rejection"; // Endpoint sanitization for span attributes — plugins stamping a user-supplied // endpoint must strip its credential-bearing parts first. diff --git a/packages/plugins/graphql/src/sdk/plugin.test.ts b/packages/plugins/graphql/src/sdk/plugin.test.ts index 45290fbfec..82a5e4fcbb 100644 --- a/packages/plugins/graphql/src/sdk/plugin.test.ts +++ b/packages/plugins/graphql/src/sdk/plugin.test.ts @@ -590,8 +590,56 @@ describe("graphqlPlugin real protocol server", () => { expect(result).toMatchObject({ ok: false, - error: { code: "connection_rejected", status: 401 }, + error: { + code: "connection_rejected", + status: 401, + message: expect.stringContaining("HTTP 401: Not authenticated. Re-authenticate"), + }, + }); + }), + ); + + it.effect("reports a plan-gated 403 with the upstream reason, not rejected credentials", () => + Effect.gen(function* () { + const server = yield* serveTestHttpApp((request) => + Effect.gen(function* () { + const webRequest = yield* HttpServerRequest.toWeb(request); + const body = yield* Effect.promise(() => webRequest.text()); + if (body.includes("__schema")) { + return HttpServerResponse.jsonUnsafe({ data: introspectionResult }); + } + return HttpServerResponse.jsonUnsafe( + { message: "feature not available on current billing plan" }, + { status: 403 }, + ); + }), + ); + const executor = yield* makeExecutor(); + + yield* executor.graphql.addIntegration({ + endpoint: server.url("/graphql"), + slug: "plan_graph", + }); + yield* createOrgConnection(executor, { + integration: "plan_graph", + name: "main", + template: "none", + }); + + const result = yield* executor.execute(toolAddr("plan_graph", "main", "query.hello"), { + name: "Ada", + }); + + expect(result).toMatchObject({ + ok: false, + error: { + code: "upstream_forbidden", + status: 403, + message: expect.stringContaining("feature not available on current billing plan"), + }, }); + const message = (result as { error: { message: string } }).error.message; + expect(message).not.toMatch(/re-?authenticate|rejected credentials/i); }), ); diff --git a/packages/plugins/graphql/src/sdk/plugin.ts b/packages/plugins/graphql/src/sdk/plugin.ts index 176e6d7721..02189ac30d 100644 --- a/packages/plugins/graphql/src/sdk/plugin.ts +++ b/packages/plugins/graphql/src/sdk/plugin.ts @@ -5,6 +5,8 @@ import { HttpClient } from "effect/unstable/http"; import { authToolFailure, detectInsufficientScope, + isCredentialRejection, + upstreamErrorMessage, AuthTemplateSlug, definePlugin, IntegrationAlreadyExistsError, @@ -1457,10 +1459,33 @@ export const graphqlPlugin = definePlugin((options?: GraphqlPluginOptions) => { }, }); } + // The upstream's own words go in the message: structured details + // are not shown to every caller (MCP text results carry only the + // message). + const upstreamText = upstreamErrorMessage(result.body); + const upstreamMessage = + upstreamText === undefined ? "" : `: ${upstreamText.replace(/[.\s]+$/, "")}`; + // A 403 is also how APIs refuse an operation the credential IS + // accepted for (billing plan, resource permissions). Only a 401, or + // a 403 whose response names a credential problem, is a rejection. + if ( + !isCredentialRejection({ + status: result.status, + body: result.body, + headers: result.headers, + }) + ) { + return ToolResult.fail({ + code: "upstream_forbidden", + status: result.status, + message: `GraphQL integration "${integration}" refused this request with HTTP ${result.status} (forbidden)${upstreamMessage}. The connection's credentials were not reported as invalid; check the account's plan, permissions, or the resource itself.`, + details: { status: result.status, body: result.body }, + }); + } return authToolFailure({ code: "connection_rejected", status: result.status, - message: `Upstream rejected credentials for GraphQL integration "${integration}" with HTTP ${result.status}. Re-authenticate or update the connection before retrying this tool.`, + message: `Upstream rejected credentials for GraphQL integration "${integration}" with HTTP ${result.status}${upstreamMessage}. Re-authenticate or update the connection before retrying this tool.`, integration: { id: integration, scope: credential.owner }, credential: { kind: "upstream", label: "Upstream authorization" }, upstream: { diff --git a/packages/plugins/openapi/src/sdk/backing.ts b/packages/plugins/openapi/src/sdk/backing.ts index 82cd6dcdba..995d787702 100644 --- a/packages/plugins/openapi/src/sdk/backing.ts +++ b/packages/plugins/openapi/src/sdk/backing.ts @@ -9,6 +9,7 @@ import { authToolFailure, classifyProbeResponse, detectInsufficientScope, + isCredentialRejection, sortHealthCheckCandidatesByIdentity, extractIdentity, extractResponseFields, @@ -810,10 +811,37 @@ export const invokeOpenApiBackedTool = (input: { details: result.error, }); } + // The upstream's own words, when it sent any (the extractor's + // status-only fallback adds nothing to a message that names the status). + const extracted = extractOpenApiUpstreamMessage(result.error, result.status); + const upstreamMessage = + extracted === `Upstream returned HTTP ${result.status}` + ? "" + : `: ${extracted.replace(/[.\s]+$/, "")}`; + // A 403 is also how APIs refuse an operation the credential IS + // accepted for (billing plan, resource permissions). Only a 401, or a + // 403 whose response names a credential problem, is a rejection; + // anything else is reported with the upstream's own message, since + // telling the caller to re-authenticate a working connection sends + // them after the wrong fix. + if ( + !isCredentialRejection({ + status: result.status, + body: result.error, + headers: result.headers, + }) + ) { + return ToolResult.fail({ + code: "upstream_forbidden", + status: result.status, + message: `Upstream "${integration}" refused this request with HTTP ${result.status} (forbidden)${upstreamMessage}. The connection's credentials were not reported as invalid; check the account's plan, permissions, or the resource itself.`, + details: result.error, + }); + } return openApiAuthToolFailure({ code: "connection_rejected", status: result.status, - message: `Upstream rejected credentials for "${integration}" with HTTP ${result.status}. Re-authenticate or update the connection "${input.credential.connection}" before retrying this tool.`, + message: `Upstream rejected credentials for "${integration}" with HTTP ${result.status}${upstreamMessage}. Re-authenticate or update the connection "${input.credential.connection}" before retrying this tool.`, owner: input.credential.owner, integration, connection: String(input.credential.connection), diff --git a/packages/plugins/openapi/src/sdk/upstream-failures.test.ts b/packages/plugins/openapi/src/sdk/upstream-failures.test.ts index 1f52b30c50..3899e37b89 100644 --- a/packages/plugins/openapi/src/sdk/upstream-failures.test.ts +++ b/packages/plugins/openapi/src/sdk/upstream-failures.test.ts @@ -241,7 +241,9 @@ describe("OpenAPI upstream failure modes", () => { error: { code: "connection_rejected", status: 401, - message: expect.stringContaining("Upstream rejected credentials"), + message: expect.stringContaining( + 'Upstream rejected credentials for "f" with HTTP 401: invalid token. Re-authenticate', + ), details: { category: "authentication", upstream: { @@ -384,7 +386,37 @@ describe("OpenAPI upstream failure modes", () => { }), ); - it.effect("ordinary 403 without a scope signal stays connection_rejected", () => + // A 403 is also how APIs refuse an operation the credential is accepted + // for. Without a credential signal it must not tell the caller to + // re-authenticate, and the upstream's own reason must be in the message: + // MCP text results carry only `code: message`, never `details`. + it.effect("plan-gated 403 reports the upstream reason, not rejected credentials", () => + Effect.gen(function* () { + const server = yield* startScriptedServer(() => ({ + status: 403, + headers: { "content-type": "application/json" }, + body: '{"message":"feature not available on current billing plan"}', + })); + const { executor, address } = yield* buildExecutorForOpenApiServer(server); + + const result = yield* executor.execute(address, {}); + + expect(result).toMatchObject({ + ok: false, + error: { + code: "upstream_forbidden", + status: 403, + message: expect.stringContaining("feature not available on current billing plan"), + details: { message: "feature not available on current billing plan" }, + }, + }); + const message = (result as { error: { message: string } }).error.message; + expect(message).toContain("HTTP 403"); + expect(message).not.toMatch(/re-?authenticate|rejected credentials/i); + }), + ); + + it.effect("permission 403 without a scope or credential signal is upstream_forbidden", () => Effect.gen(function* () { const server = yield* startScriptedServer(() => ({ status: 403, @@ -395,6 +427,53 @@ describe("OpenAPI upstream failure modes", () => { const result = yield* executor.execute(address, {}); + expect(result).toMatchObject({ + ok: false, + error: { + code: "upstream_forbidden", + status: 403, + message: expect.stringContaining("Caller lacks permission"), + }, + }); + }), + ); + + it.effect("403 naming a bad credential stays connection_rejected with the upstream reason", () => + Effect.gen(function* () { + const server = yield* startScriptedServer(() => ({ + status: 403, + headers: { "content-type": "application/json" }, + body: '{"message":"Invalid API key"}', + })); + const { executor, address } = yield* buildExecutorForOpenApiServer(server); + + const result = yield* executor.execute(address, {}); + + expect(result).toMatchObject({ + ok: false, + error: { + code: "connection_rejected", + status: 403, + message: expect.stringMatching(/HTTP 403: Invalid API key\. Re-authenticate/), + }, + }); + }), + ); + + it.effect("403 with an authentication challenge stays connection_rejected", () => + Effect.gen(function* () { + const server = yield* startScriptedServer(() => ({ + status: 403, + headers: { + "content-type": "application/json", + "www-authenticate": 'Bearer realm="api", error="invalid_token"', + }, + body: '{"message":"Forbidden"}', + })); + const { executor, address } = yield* buildExecutorForOpenApiServer(server); + + const result = yield* executor.execute(address, {}); + expect(result).toMatchObject({ ok: false, error: { code: "connection_rejected", status: 403 },