Skip to content

fix: apply MCP OAuth lifetime for gateway slugs#3711

Merged
joeyorlando merged 2 commits intomainfrom
fix/mcp-oauth-slug-lifetime
Apr 11, 2026
Merged

fix: apply MCP OAuth lifetime for gateway slugs#3711
joeyorlando merged 2 commits intomainfrom
fix/mcp-oauth-slug-lifetime

Conversation

@joeyorlando
Copy link
Copy Markdown
Contributor

@joeyorlando joeyorlando commented Apr 10, 2026

Summary

  • Resolve MCP OAuth resource URLs by UUID or gateway slug when applying org token lifetime settings
  • Covers Cursor-style /v1/mcp/default-mcp-gateway resource URLs so expires_in and persisted oauth_access_token.expires_at use the configured lifetime
  • Add regression test for a 300-second org lifetime with a gateway slug resource

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 10, 2026

Claude finished @joeyorlando's task in 3m 53s —— View job


PR Review: fix: apply MCP OAuth lifetime for gateway slugs

  • Read CLAUDE.md files for repo guidelines
  • Read changed files (auth.ts, auth.test.ts)
  • Verify AgentModel.resolveIdFromIdOrSlug exists and behavior
  • Check extractProfileIdFromMcpResource usage consistency
  • Analyze the PR diff
  • Provide review feedback

Summary

This PR fixes a real bug: when a client (e.g. Cursor) exchanges an authorization code for an MCP access token and passes a resource URL using the gateway slug (/v1/mcp/default-mcp-gateway) instead of the UUID, the org-configured mcpOauthAccessTokenLifetimeSeconds was silently ignored because the previous regex required a 36-char UUID: /^\/v1\/mcp\/([0-9a-f-]{36})$/i. The result was short-lived tokens that didn't honor the organization's lifetime setting, and persisted oauth_access_token.expires_at rows with the wrong value. Good catch.

The fix looks solid. A few observations and one small suggestion below.


What the change does

platform/backend/src/routes/auth.ts:733-755 rewrites getProfileIdFromResource to:

  1. Parse the resource as a URL.
  2. Accept both the token endpoint origin and buildOAuthIssuer() origin (nice — this matters when the backend and the configured ARCHESTRA_FRONTEND_URL differ, e.g. localhost:9000 vs frontend URL).
  3. Match ^/v1/mcp/([^/]+)$ to capture either a UUID or a slug.
  4. Resolve that value via AgentModel.resolveIdFromIdOrSlug (which was already used by oauth-server.ts:162 and mcp-gateway.utils.ts:556, so this is consistent with the rest of the codebase).

The function becomes async, and the caller in getMcpOauthAccessTokenLifetimeSeconds (auth.ts:677-681) is updated accordingly. The now-unused extractProfileIdFromMcpResource import is removed from this file.

The regression test at auth.test.ts:222-292 covers the exact Cursor scenario described in the PR body (300s org lifetime, resource using ${agent.slug}) and asserts both the response body (expires_in, expires_at) and the persisted oauth_access_token.expires_at — nicely thorough.


Observations / suggestions

1. decodeURIComponent on the path segment — minor inconsistency (nit)

auth.ts:749-751:

const match = resourceUrl.pathname.match(/^\/v1\/mcp\/([^/]+)$/);
const idOrSlug = match?.[1] ? decodeURIComponent(match[1]) : null;
return idOrSlug ? AgentModel.resolveIdFromIdOrSlug(idOrSlug) : null;

The two other places that resolve idOrSlug from a path do not decode:

  • platform/backend/src/routes/oauth-server.ts:156-163 (extractProfileIdFromResourcePath)
  • platform/backend/src/routes/mcp-gateway.utils.ts:542-558 (extractProfileIdAndTokenFromRequest)

In practice slugs generated by AgentModel.generateUniqueSlug (agent.ts:1531) go through urlSlugify and will be URL-safe, so the decode is a no-op 99% of the time. But if a caller encodes a reserved character (e.g. passes agent%2Dslug), this code would find it and the other two wouldn't. It's arguably the more correct behavior, but the inconsistency is worth either aligning or ignoring as out-of-scope — up to you.

2. extractProfileIdFromMcpResource is now only used by the JWT-bearer flow

After this change, extractProfileIdFromMcpResource in platform/backend/src/services/identity-providers/enterprise-managed/authorization.ts:189 is only referenced from exchangeIdentityAssertionForAccessToken at line 96 (the urn:ietf:params:oauth:grant-type:jwt-bearer flow).

Worth confirming: do enterprise-managed ID-JAG assertions ever set resource to the slug URL form? If yes, that flow would still reject with "Assertion JWT resource must reference a valid MCP Gateway URL." even though the same gateway's slug URL works for authorization_code. If no (i.e. assertion issuers always use the canonical UUID form), the current divergence is fine and intentional. Since the PR title/body scopes this to the MCP OAuth lifetime path, leaving JWT-bearer alone is reasonable — but worth a quick sanity check.

3. buildOAuthIssuer() is called on every token exchange

auth.ts:743:

const issuerOrigin = new URL(buildOAuthIssuer()).origin;

buildOAuthIssuer() just reads config.frontendBaseUrl and appends a slash, and new URL(...).origin is cheap, so this is totally fine. Not a concern — just flagging that if you ever wanted to, the origin is effectively static and could be hoisted to a module-level constant. Not worth changing.

4. Test coverage — good

The new test correctly:

  • Sets mcpOauthAccessTokenLifetimeSeconds: 300 (different from the existing 31_536_000 and 604_800 tests, so you can be sure the right lifetime is being applied rather than coincidentally matching)
  • Uses agent.slug in the resource URL
  • Asserts expires_in, expires_at, and the persisted DB row via OAuthAccessTokenModel.getByTokenHash
  • Provides a deterministic expires_at from the upstream response so getIssuedAtSeconds is exercised via the expires_at - expires_in branch at auth.ts:724-728

One small thing: the test passes agentType: "mcp_gateway" and name: "Default MCP Gateway" to makeAgent, but the behavior under test doesn't depend on agentType or name — any agent with a slug would suffice. Not a bug; it matches the "default gateway" framing of the bug report and makes the test read like a real scenario. Fine as-is.


Correctness checks

  • Origin allowlist at auth.ts:742-747: correctly preserves the existing "same origin as token endpoint" check and adds the issuer origin. A malicious resource pointing at a different host will still be rejected.
  • The regex ^/v1/mcp/([^/]+)$ requires exact path match (no trailing slash, no sub-path), matching the previous UUID-only regex's strictness. Good.
  • If resolveIdFromIdOrSlug returns null (unknown slug/UUID), the function falls back to getProfileIdFromReferenceId(params.referenceId) via the ?? chain at auth.ts:677-681. Correct behavior.
  • async conversion propagates correctly; the single caller already awaits.

Verdict

LGTM. The fix is targeted, consistent with existing slug-resolution patterns in the codebase, and covered by a good regression test. The only thing I'd consider before merging is a quick answer on observation #2 (whether the JWT-bearer flow should also accept slug-form resource URLs). If that flow is guaranteed to always receive canonical UUID URLs from its assertion issuers, ship it.
· Branch

@joeyorlando joeyorlando merged commit 362aaec into main Apr 11, 2026
47 of 48 checks passed
@joeyorlando joeyorlando deleted the fix/mcp-oauth-slug-lifetime branch April 11, 2026 01:53
joeyorlando added a commit that referenced this pull request Apr 11, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.2.10](platform-v1.2.9...platform-v1.2.10)
(2026-04-11)


### Features

* make posthog analytics configurable
([#3707](#3707))
([7ae9101](7ae9101))


### Bug Fixes

* `/llm/costs` table scrolling
([#3722](#3722))
([6a42ba8](6a42ba8))
* apply MCP OAuth lifetime for gateway slugs
([#3711](#3711))
([362aaec](362aaec))
* Bedrock tool name encoding
([#3706](#3706))
([0e2c2d1](0e2c2d1))
* costs timeframes and surface limit reset settings
([#3709](#3709))
([6e4154b](6e4154b))
* jira oauth discovery overrides
([#3721](#3721))
([2c4cf8f](2c4cf8f))
* OIDC discovery trusted origins for IdP registration
([#3714](#3714))
([adb5f5e](adb5f5e))
* preserve shared chat agents on fork
([#3715](#3715))
([252edfc](252edfc))
* reranker model dropdown labels
([#3704](#3704))
([ebd1c8a](ebd1c8a))
* session logs loading state
([#3712](#3712))
([ffba126](ffba126))


### Miscellaneous Chores

* **ci:** add ID-JAG MCP e2e test
([#3702](#3702))
([1a5078a](1a5078a))
* **deps:** bump next from 16.1.7 to 16.2.3 in /platform/frontend
([#3708](#3708))
([d47967c](d47967c))
* use neutral token prefixes with legacy support
([#3719](#3719))
([db5929c](db5929c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: archestra-ci[bot] <222894074+archestra-ci[bot]@users.noreply.github.com>
Co-authored-by: Joey Orlando <joey@archestra.ai>
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.

1 participant