[MCP] Resolve the token audience from the endpoint's owner - #2029
Merged
wwidergoldpimcore merged 8 commits intoSep 8, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Issuer-based discovery remains inconsistent, and query-bearing resources can select the wrong audience.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves MCP OAuth audience resolution to the protected-resource registry rather than a hardcoded Studio path.
Changes:
- Adds issuer-aware, segment-boundary resource resolution.
- Injects resolved audiences into the MCP authenticator.
- Adds unit coverage and dependency-injection wiring.
Assessment:
- Correct service boundary and no identified public API break.
- Incomplete across discovery call sites; query-bearing resources can also resolve ambiguously.
- Unit tests do not cover these end-to-end cases.
File summaries
| File | Description |
|---|---|
src/OAuth/Resolver/RequestResourceResolver.php |
Resolves the most specific resource. |
src/OAuth/Resolver/RequestResourceResolverInterface.php |
Defines the resolver contract. |
src/Security/Authenticator/Mcp/OAuthAccessTokenAuthenticator.php |
Uses the resolved audience. |
src/DependencyInjection/PimcoreStudioBackendExtension.php |
Injects the configured issuer. |
config/oauth.yaml |
Registers resolver services. |
tests/Unit/OAuth/Resolver/RequestResourceResolverTest.php |
Tests resource matching. |
tests/Unit/Security/Authenticator/Mcp/OAuthAccessTokenAuthenticatorTest.php |
Tests authenticator integration. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The authenticator derived the resource from a path hardcoded here, so only this bundle's own MCP servers could be reached with an audience-bound token; an endpoint registered by another bundle never matched. It now resolves the most specific registered resource covering the request, and declines when none does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ej6YARj6HakERFuERFuDF9
The authenticator now holds a token to the resource registered for the endpoint being called, so an endpoint whose owner registers none does not accept OAuth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ej6YARj6HakERFuERFuDF9
Deriving the audience from the issuer while the 401 challenge and the metadata lookup still used the request host sent clients to a document that resolved to nothing. Ranking on the whole identifier also let a long query outrank the resource that actually matched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ej6YARj6HakERFuERFuDF9
wwidergoldpimcore
force-pushed
the
feature/mcp-1309-resource-derivation
branch
from
September 8, 2026 12:11
66703b5 to
3113600
Compare
wwidergoldpimcore
approved these changes
Sep 8, 2026
The issuer is the authorization server's identity: stamped on tokens as `iss`, advertised in metadata, and the base for protected-resource URIs (`buildMcpServerResources` builds `<issuer>/pimcore-mcp/studio/<slug>`). Deriving it per request from the Host header is non-deterministic, and at container-build time yields no issuer at all — `buildMcpServerResources` returns an empty list, so no managed server registers as a resource and the authenticator then refuses OAuth for all of them. Reject `oauth.enabled: true` with a null issuer at config-compile time instead of failing silently at runtime, and document the issuer as the required public base URL (with reverse-proxy and env-var guidance). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t path McpAuthenticationEntryPoint built the RFC 9728 metadata URL from the raw request path, but the authenticator validates the token against the resource the RequestResourceResolver selects by longest prefix. When a broader resource covers the endpoint the two differ, and the challenge then advertised a metadata document that ProtectedResourceMetadataController's exact lookup answers with 404 — so the client could not discover where to authenticate. Resolve the request in the entry point too and build the metadata URL from the matched resource's path, falling back to the request path when nothing is registered. Regression test covers a broader-only registration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two follow-ups from the review: - The enabled-issuer check only rejected null, so `issuer: ''` (and non-string scalars) slipped through, yielding relative resource URIs and an empty `iss`. Require a non-empty absolute origin (scheme + host); reject blank/malformed. - The 401 challenge conflated "no resource matched" with "matched an origin-only root resource": parse_url() returns a null path for the latter, so it fell back to the request path and reintroduced the mismatch. Branch on the null match instead, and use the matched resource's path (empty for a root resource). Adds config rejection cases (empty, non-absolute) and a root-resource challenge regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous check lived on the parent oauth node and called parse_url() directly, which was wrong in both directions: - it rejected the documented `issuer: '%env(...)%'` form, because Symfony keeps env placeholders literal while the parent array node is validated; and - it accepted malformed issuers (path, query, fragment, userinfo, non-HTTP(S)), where a fragment or query swallows the appended `/pimcore-mcp/studio/...` path. Move the URL-shape check onto the scalar `issuer` node (Symfony skips it for unresolved placeholders, with an explicit `%…%` guard so it is also skipped in a bare Processor test) and require a bare http(s) origin — no userinfo, path, query or fragment. The parent node keeps only the enabled-without-issuer rule. Tests cover the env-backed form plus the malformed cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Issuer validation can be bypassed by malformed compound environment-placeholder values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
The issuer shape check was out of step with this configuration: nothing else here validates an operator-supplied URL, and client `redirect_uris` — the most security-sensitive URL in the OAuth config — is only checked for presence, with real enforcement at runtime. It also needed placeholder gymnastics to keep the documented `%env(...)%` form working. Drop the shape check and keep the part that matters: `issuer` is still required when OAuth is enabled, because omitting it registers no protected resource at all and every OAuth request then 401s with nothing pointing at the cause. The shape guidance moves to the documentation, including the one consequence that would otherwise be silent: a query or fragment collapses every MCP server onto the same token audience, defeating per-server isolation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
wwidergoldpimcore
merged commit Sep 8, 2026
f1a51e3
into
feature/mcp-1309-server-config-management
20 checks passed
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Second half of the split out of #2022, which is closed in favour of this and #2028. This one carries the MCP
side: how an endpoint behind the
pimcore_mcpfirewall gets the audience its token is checked against.The problem
OAuthAccessTokenAuthenticatorderived the resource URI from a path hardcoded in this bundle(
<host>/pimcore-mcp). That is wrong in three ways:/pimcore-mcpitself, and no protected resource isregistered for it, so with the audience enforced ([OAuth] Public resource-server contracts, an extensible scope catalogue, and resource-bound tokens #2028) an audience-bound token could never match.
proxy the two differ and a correctly issued token is refused at the endpoint it was issued for.
bundles too, and
/pimcore-mcp/agent/{group}from the Agent bundle could never match a hardcoded Studio path.The earlier attempt in #2022 fixed the first two by hardcoding
/pimcore-mcp/studio/<slug>instead, whichstill leaves any other bundle's endpoint unreachable.
What this does
Resolves the audience from whoever owns the endpoint. A new
RequestResourceResolverfinds the mostspecific registered protected resource covering the request, and the authenticator validates against that.
This bundle needs no per-consumer path knowledge: a bundle that registers its endpoint as a protected resource
works, which is the same contract Data Hub Simple REST already follows.
Matching is origin equality plus a path prefix that only matches on a segment boundary, the same rule a
standards-based client applies when deciding whether a resource covers an endpoint, so the server's check
agrees with what the client was told is allowed. Longest match wins, so a resource registered for one server
takes precedence over a broader one. The URI is built from the configured issuer when set.
When no registered resource covers the request the authenticator declines rather than guessing. Declining
leaves the rest of the chain intact, so PAT and session authentication are unaffected on endpoints that have
not opted in.
Compatibility
Nothing changes for a credential that is not a JWT-shaped bearer:
supports()still declines thepmcp_prefix, opaque PATs and session-bridge requests, so they never reach this code. An endpoint whose owner
registers no protected resource keeps working exactly as before on its other credentials, and simply does not
accept OAuth until it registers one. Registration is the switch.
Verified
Unit suite green (898 tests), PHPStan clean.
RequestResourceResolverTestcovers the Studio server case, anendpoint owned by another bundle, an unregistered endpoint, segment-boundary matching, longest-match
precedence, and issuer-versus-host derivation.
Follow-up
The Agent bundle registers no protected resource, so OAuth on
/pimcore-mcp/agent/*stays unavailable untilit does. That is a separate change in that repository, and this PR does not regress it: OAuth was never usable
there.
Once #2028 merges, the remaining MCP residue from #2022 lands on top of this: swapping the scope provider's
literals for
McpScopes::READ/WRITE, and the09_MCP_Server_Management.mddocumentation.🤖 Generated with Claude Code