Repository navigation
Report non-credential 403s as upstream_forbidden with the upstream message - #2257
Closed
shardulbee wants to merge 1 commit into
Closed
shardulbee wants to merge 1 commit into
shardulbee wants to merge 1 commit into
Conversation
Author
|
Closing this. Why:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An upstream 403 does not always mean bad credentials. An API also sends 403 when the account's plan does not include a feature. Executor reported every such 403 as
connection_rejectedand told the caller to re-authenticate. The upstream's reason was only indetails, and MCP text results show onlycode: message, so the caller did not see it.This change makes a 403 a credential failure only when the response says so. Other 403s fail with
upstream_forbiddenand the upstream's message. This matches v2, where a bare 403 isrejected("does not prove invalid credentials") and the upstream's error goes with it.on(upstream 401 or 403) if 403 and scope is insufficient # unchanged return oauth_scope_insufficient + if 401, or 403 with a credential signal # WWW-Authenticate challenge, + # invalid_token / UNAUTHENTICATED code, + # or "invalid API key" / "token expired" text return connection_rejected "HTTP 401: <upstream message>. Re-authenticate…" + else + return upstream_forbidden "HTTP 403 (forbidden): <upstream message>. Credentials were not reported as invalid…"The check is
isCredentialRejectionin@executor-js/sdk/core, next todetectInsufficientScope. The OpenAPI and GraphQL plugins use it.Linked issue
Related to #2126 (Cloudflare challenge 403s shown as rejected credentials, closed for v1). That 403 has no credential signal, so with this change it is also
upstream_forbidden, notconnection_rejected.Verification
Before: an MCP
invokeof an operation whose upstream returns403 {"message":"feature not available on current billing plan"}:After:
e2e
scenarios/upstream-forbidden.test.ts(new) passes on selfhost and in CI. Through MCPinvokeon one connection: a write succeeds; the plan-gated 403 text has the upstream message andHTTP 403, and has no "re-authenticate"; a 401 still says "Re-authenticate" with the upstream's reason.e2e
scenarios/oauth-scope-insufficient.test.ts: its plainPERMISSION_DENIED403 now expectsupstream_forbiddenwith the upstream message. Passes on selfhost.upstream-failures.test.ts(OpenAPI): with the oldbacking.ts, 4 of the new tests fail. With this change, all 19 pass. The GraphQL plugin and core tests cover the same cases.CI skips Format, Lint, Typecheck and Test on fork PRs, so I ran them locally:
bun run format:checkbun run lintbun run typecheck: rantsgo --noEmitfor core/sdk, plugin-openapi, plugin-graphql and e2e only; the full turbo run needs more memory than the machine had.bun run test: ran the full vitest suites for core/sdk, plugin-openapi and plugin-graphql only: 967, 340 and 120 tests pass.e2e:
upstream-forbidden,oauth-scope-insufficient(selfhost). The failing cloud shard isfirst-party-oauth.test.ts, which fails the same way onmain.Merge danger
Door: two-way. No storage or contract shape changes; revert restores the old labels.
Blast radius: callers that branch on
connection_rejectedfor a 403. Those 403s now come back asupstream_forbiddenunless the response names a credential problem. Reactive token refresh is not affected: it only runs onconnection_rejectedwith status 401.Checklist