Report non-credential 403s as upstream_forbidden with the upstream message

.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.

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"));

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);
+        }),
+      );
+    }),
+  ),
+);

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();
+  });
+});