Skip to content

fix: return clean auth failures for invalid MCP bearer tokens#620

Closed
ArshyaAI wants to merge 1 commit into
garrytan:masterfrom
ArshyaAI:fix/http-mcp-auth-errors
Closed

fix: return clean auth failures for invalid MCP bearer tokens#620
ArshyaAI wants to merge 1 commit into
garrytan:masterfrom
ArshyaAI:fix/http-mcp-auth-errors

Conversation

@ArshyaAI

@ArshyaAI ArshyaAI commented May 4, 2026

Copy link
Copy Markdown

Summary

  • throw MCP SDK InvalidTokenError for expired and unknown OAuth bearer tokens
  • tighten the HTTP OAuth E2E expectation so invalid /mcp bearer tokens must return a clean 401 invalid_token response, not 500
  • add PGLite unit coverage that verifyAccessToken rejects invalid/expired tokens with InvalidTokenError

Closes #616.

Verification

  • bun run typecheck
  • bun test test/oauth.test.ts
  • bun test test/e2e/serve-http-oauth.test.ts (skips without DATABASE_URL)

Note: this avoids changing GBrain auth semantics in downstream AStack wrappers; the fix aligns the provider with the MCP SDK bearerAuth error contract.


View in Codesmith
Need help on this PR? Tag @codesmith with what you need.

  • Let Codesmith autofix CI failures and bot reviews

@ArshyaAI

ArshyaAI commented May 4, 2026

Copy link
Copy Markdown
Author

AStack/OpenClaw remote-MCP readiness follow-up evidence for this PR:

  • Branch head: b3e4f25d325ee14c6bde8dd915180c8224f3f0cd
  • Base checked: upstream master at 058fe695756ed16e43916d907af3845338430156 (0.26.7)
  • Local PR worktree verification already passed:
    • bun run typecheck -> pass
    • bun test test/oauth.test.ts -> 42 pass
    • bun test test/e2e/serve-http-oauth.test.ts skipped without DATABASE_URL in the safe local test lane

Live downstream canary evidence from AStack Remote MCP service after applying the same InvalidTokenError semantics in its wrapper:

  • missing token -> 401
  • bad token -> 401
  • expired token -> 401
  • revoked client token request -> 400
  • DCR disabled -> 404
  • admin route denial -> 404
  • CORS default-deny -> pass
  • log redaction -> pass
  • read-only write denial -> pass
  • remote MCP exposed tool count -> 38
  • read/write/search/version/delete/restore canary -> pass

Why this matters for downstream readiness: without this upstream fix, invalid/expired MCP bearer tokens can surface as 500 via the MCP SDK auth path. AStack currently has a temporary service-local safety patch; landing this PR removes the need to keep that auth behavior AStack-custom.

@ArshyaAI ArshyaAI force-pushed the fix/http-mcp-auth-errors branch from b3e4f25 to 444fb2d Compare May 5, 2026 00:34
@ArshyaAI

ArshyaAI commented May 5, 2026

Copy link
Copy Markdown
Author

Rebased onto upstream master 9c2dc4cd544cd8013e0eee7a6ffb8536d3c2f13a / 0.26.8 and force-with-lease pushed. New head: 444fb2d1b80b7b0f353ab76d72471a124e331ca1.

Targeted verification after rebase:

  • bun test test/oauth.test.ts test/e2e/serve-http-oauth.test.ts --timeout 30000 -> 42 pass, 26 skip because DATABASE_URL was not set, 0 fail
  • bun run typecheck -> pass

This remains the upstream counterpart for the AStack Remote MCP canary requirement where invalid/expired bearer tokens must return clean 401/403 rather than surfacing as a 500.

@ArshyaAI ArshyaAI force-pushed the fix/http-mcp-auth-errors branch from 444fb2d to c6a3f95 Compare May 5, 2026 05:38
@ArshyaAI

ArshyaAI commented May 5, 2026

Copy link
Copy Markdown
Author

Rebased onto upstream master ee9ceb327a39b0c705ee945c6cfe821de11d34ed / 0.27.0 and force-with-lease pushed. New head: c6a3f9548d88b99eb72a2ed0787207ae98606cbe.\n\nTargeted verification after rebase:\n- Resolved the only rebase conflict by preserving upstream isUndefinedColumnError import and adding MCP SDK InvalidTokenError.\n- bun test test/oauth.test.ts test/e2e/serve-http-oauth.test.ts --timeout 30000 -> 56 pass, 28 skip because DATABASE_URL was not set, 0 fail\n- bun run typecheck -> pass\n\nNote: upstream 0.26.9/0.27.0 added broader OAuth/MCP hardening and wraps handleRequest, but verifyAccessToken on master still throws generic Error('Token expired'/'Invalid token'). This PR remains the small SDK-aligned change that maps invalid/expired bearer tokens to clean OAuth 401 invalid_token behavior for the downstream Remote MCP canary.

@ArshyaAI

ArshyaAI commented May 5, 2026

Copy link
Copy Markdown
Author

Live downstream evidence from AStack/OpenClaw dogfood cut on 2026-05-05:

  • Consumed in custom branch ArshyaAI/gbrain@de11d4c858b1a880931c008fbee3ceef92a8329a together with PRs fix: align resolver routing fixtures #619 and fix: embed stale chunks by page identity #626.
  • Remote MCP/OAuth fixture canary passed on Railway Remote MCP service after deploy b57e27c0-351e-4db1-9c0e-914922c1769d.
  • Canary covered missing token, bad token, expired token, revoked client, DCR disabled, admin denial, CORS default-deny, log redaction, read-only write denial, read/write/search/version/delete/restore, and fixture client revocation.
  • Direct GBrain, local Codex, local Claude Code, and OpenClaw-mediated canaries also passed against the same custom cut.

This PR remains one of the upstream-clean blockers for AStack FULL PASS; current dogfood status is intentionally custom/non-upstream-clean until merged and consumed.

@ArshyaAI

ArshyaAI commented May 6, 2026

Copy link
Copy Markdown
Author

2026-05-06 maintainer merge packet / downstream readiness refresh:

  • PR state checked today: OPEN, MERGEABLE, no checks/reviews blocking from GitHub metadata.
  • AStack/OpenClaw live dogfood cut still consumes this PR via custom branch ArshyaAI/gbrain@de11d4c858b1a880931c008fbee3ceef92a8329a.
  • Fresh Remote MCP/OAuth fixture canary passes on Railway Remote MCP: missing token 401, bad token 401, expired token 401, revoked client denial 400, DCR disabled 404, admin denial 404, CORS default-deny, log redaction, read-only write denial, tools list, read/write/search/version/delete/restore, fixture clients revoked.
  • Fresh gates also pass for runtime doctor, direct GBrain, local Codex, local Claude Code, runtime risk logs, shell quota guard, and scheduler burn-in.
  • Remaining FULL PASS blockers are now upstream-clean only: merge/consume fix: align resolver routing fixtures #619, fix: return clean auth failures for invalid MCP bearer tokens #620, fix: embed stale chunks by page identity #626, then move runtime/Docker pins from custom cut back to upstream master.

Why this PR matters: it removes the AStack-local Remote MCP auth semantics patch so invalid/expired bearer tokens are clean OAuth failures upstream, not custom runtime behavior.

@garrytan

Copy link
Copy Markdown
Owner

Thank you for this work @ArshyaAI — closing as already in master at src/core/oauth-provider.ts:542, 600. Both expired and unknown bearer tokens now throw InvalidTokenError from the MCP SDK, returning a clean 401 invalid_token response instead of a 500.

Verified during the v0.41.3.0 fix wave scope challenge (#1403). The fix is in production; closing for queue cleanup.

@garrytan garrytan closed this May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: HTTP MCP invalid bearer token returns 500 instead of 401/403

2 participants