feat(mcp): add OAuth 2.1 authorization with per-tool scope extraction#2636
Merged
feat(mcp): add OAuth 2.1 authorization with per-tool scope extraction#2636
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds OAuth 2.0 and JWKS-based authentication to MCP servers, introducing scope-driven access control, GraphQL operation scope extraction, and authentication middleware with RFC 9728 compliance. It replaces the third-party MCP implementation with the official go-sdk and provides comprehensive test utilities and E2E tests for OAuth-protected MCP operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Vulnerabilitiesrouter-tests/go.mod
router/go.mod
Only included vulnerabilities with severity high or higher. OpenSSF ScorecardScorecard details
Scanned Files
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Router image scan passed✅ No security vulnerabilities found in image: |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
asoorm
force-pushed
the
ahmet/mcp-per-tool-scope-extraction
branch
from
242eb2b to
22a55c6
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
asoorm
force-pushed
the
ahmet/mcp-per-tool-scope-extraction
branch
from
22a55c6 to
4133ddc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
router/pkg/mcpserver/server.go (2)
941-977:⚠️ Potential issue | 🟠 MajorTreat non-2xx upstream responses as tool errors.
This function never checks
resp.StatusCode, so a 401/500 HTML or plain-text response from the router falls through to the success path and is returned as normal tool content. Tool callers then can't distinguish upstream auth/transport failures from successful GraphQL results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/server.go` around lines 941 - 977, Check resp.StatusCode after reading the body and before parsing GraphQL: if the status is not 2xx, return an error CallToolResult (IsError: true) that includes the HTTP status code and the response body text so callers can distinguish upstream auth/transport failures; update the logic around resp, body, and the existing GraphQLResponse handling to short-circuit non-2xx responses and use mcp.CallToolResult (with &mcp.TextContent{Text: ...}) to surface the status and body.
431-478:⚠️ Potential issue | 🟠 MajorPreserve request headers even when OAuth is disabled.
executeGraphQLQueryforwards headers fromheadersFromContext(ctx), but this path only injects them whenauthMiddleware.HTTPMiddlewareruns. On non-OAuth MCP deployments, downstream GraphQL calls lose the originalAuthorization/cookie/session headers.Proposed fix
- // MCP endpoint with HTTP-level authentication - // Per MCP spec: "authorization MUST be included in every HTTP request from client to server" - mcpHandler := http.Handler(streamableHTTPHandler) + // Preserve original request headers for downstream GraphQL forwarding. + mcpHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := requestHeadersFromRequest(r.Context(), r) + streamableHTTPHandler.ServeHTTP(w, r.WithContext(ctx)) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/server.go` around lines 431 - 478, The closure passed to mcp.NewStreamableHTTPHandler currently returns s.server without propagating request headers, so executeGraphQLQuery (which reads headers via headersFromContext(ctx)) loses Authorization/cookie information when authMiddleware.HTTPMiddleware isn’t used; modify that closure (the func(req *http.Request) *mcp.Server passed to mcp.NewStreamableHTTPHandler) to inject the incoming request headers into the request context (using the project's header-to-context helper that pairs with headersFromContext or by adding the headers under the same context key used by headersFromContext) before returning s.server, so downstream executeGraphQLQuery can read the original headers even when OAuth/authMiddleware is disabled.
🟡 Minor comments (6)
router/go.mod-93-99 (1)
93-99:⚠️ Potential issue | 🟡 MinorNote:
golang.org/x/oauth2 v0.34.0is a valid release but pre-v1.The dependency
golang.org/x/oauth2 v0.34.0is a valid, properly tagged release (Dec 1, 2025). However, it is a pre-v1 module (v0.x), which means it does not offer stability guarantees under Go versioning standards. Verify that using a pre-v1 dependency aligns with your project's dependency policies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/go.mod` around lines 93 - 99, The go.mod currently requires the pre-v1 module golang.org/x/oauth2 v0.34.0; confirm this complies with our dependency policy and either (a) upgrade to a stable v1+ release if policy disallows pre-v1 (replace golang.org/x/oauth2 v0.34.0 with the approved v1+ tag), or (b) explicitly document/justify acceptance of this pre-v1 dependency (add a short comment in router/go.mod and/or the PR description referencing golang.org/x/oauth2 v0.34.0) so reviewers know this was intentional; locate the entry for golang.org/x/oauth2 in router/go.mod to apply the change.demo/go.mod-97-97 (1)
97-97:⚠️ Potential issue | 🟡 MinorAddress version inconsistency: demo uses mcp-go v0.43.2 while router/router-tests use v0.36.0.
The demo module indirectly depends on mcp-go v0.43.2 (pulled in via router-tests → testenv), but the router and router-tests modules explicitly use v0.36.0. While v0.43.2 is stable (December 2025 release with bugfixes only, no known regressions), this version discrepancy warrants clarification:
- Is the v0.43.2 bump in demo intentional, or should it align with router/router-tests at v0.36.0?
- Since router migrated to
modelcontextprotocol/go-sdkv1.4.0, should demo complete the same migration instead of relying on the mcp-go indirect dependency?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/go.mod` at line 97, The demo/go.mod currently has an indirect dependency github.com/mark3labs/mcp-go v0.43.2 which conflicts with router/router-tests pinned to v0.36.0; decide and enforce a single source of truth: either (A) align demo to router by replacing the indirect dependency with github.com/mark3labs/mcp-go v0.36.0 in demo/go.mod, or (B) migrate demo to the new upstream package modelcontextprotocol/go-sdk v1.4.0 (mirror the router migration) by adding the modelcontextprotocol/go-sdk v1.4.0 requirement and updating any imports that reference github.com/mark3labs/mcp-go to the new SDK; update go.mod accordingly and run go mod tidy to ensure no indirect version skew remains.router/pkg/config/testdata/config_full.json-205-218 (1)
205-218:⚠️ Potential issue | 🟡 MinorPlease cover the scope-challenge mode in this full-config fixture.
The new OAuth block exercises scopes and JWKS, but not the challenge-mode setting introduced by this PR. That leaves the exhaustive config fixture unable to catch load/default/round-trip regressions for the strict/smart/optimistic behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/testdata/config_full.json` around lines 205 - 218, Add the new scope-challenge setting into the OAuth block of the full-config fixture by adding the "ScopeChallengeMode" property (set to a representative value like "strict" — other valid values are "smart" or "optimistic") alongside "ScopeChallengeIncludeTokenScopes"; this ensures the config_full.json fixture exercises the challenge-mode code paths when loading/round-tripping the "OAuth" object.router/pkg/mcpserver/scope_challenge.md-13-15 (1)
13-15:⚠️ Potential issue | 🟡 MinorAdd fence languages so markdownlint stops flagging this doc.
These unlabeled blocks are the MD040 warnings from static analysis.
textis enough for all four examples here.📝 Minimal fix
-``` +```textApply the same opening-fence change to the remaining unlabeled examples in this file.
Also applies to: 42-44, 55-57, 67-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/scope_challenge.md` around lines 13 - 15, The unlabeled code fences in scope_challenge.md (e.g., the example showing `[["a", "b"], ["c", "d"]] → (a AND b) OR (c AND d)` and the other examples at the ranges you noted) should be changed to use a labeled fence, e.g., replace the opening ``` with ```text for each unlabeled block so markdownlint MD040 is satisfied; apply this same change to the other unlabeled examples referenced (lines ~42-44, 55-57, 67-74) so every code fence is explicitly `text`.router-tests/mcp_oauth_e2e_test.go-108-113 (1)
108-113:⚠️ Potential issue | 🟡 MinorMake the auth-error assertions strict.
Both tests pass on any non-nil error right now. If connection setup breaks for an unrelated reason, the
AuthErrorchecks are skipped and the test still goes green.Proposed fix
- authErr, ok := err.(*AuthError) - if ok { - assert.Equal(t, http.StatusUnauthorized, authErr.StatusCode, "should return HTTP 401") - assert.NotEmpty(t, authErr.ResourceMetadataURL, "should include resource_metadata for OAuth discovery") - t.Logf("Invalid token rejected with HTTP 401: %v", authErr) - } + var authErr *AuthError + require.ErrorAs(t, err, &authErr) + assert.Equal(t, http.StatusUnauthorized, authErr.StatusCode, "should return HTTP 401") + assert.NotEmpty(t, authErr.ResourceMetadataURL, "should include resource_metadata for OAuth discovery") + t.Logf("Invalid token rejected with HTTP 401: %v", authErr)Also applies to: 146-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/mcp_oauth_e2e_test.go` around lines 108 - 113, The test is currently skipping AuthError assertions when err is nil or not an AuthError; change the checks to be strict by first asserting an error was returned (e.g., require.Error(t, err)) and then asserting the error is an AuthError (e.g., require.True(t, ok) or require.IsType(t, &AuthError{}, err)) before inspecting authErr.StatusCode and authErr.ResourceMetadataURL in the block for authErr; update both occurrences that use authErr/ok (lines around the existing AuthError handling) so the test fails if err is nil or of the wrong type rather than silently passing.router-tests/mcp_oauth_e2e_test.go-175-180 (1)
175-180:⚠️ Potential issue | 🟡 Minor
ToolsCallalready forces write access onget_schemain this fixture.The middleware checks
ToolsCallfor everytools/callbefore it looks at built-in tool scopes. WithToolsCall: []string{"mcp:tools:write"}, the later subtests expectingget_schemato work with onlymcp:tools:readcannot match the server behavior unless the sharedToolsCallgate is relaxed andGetSchema/ExecuteGraphQLare configured separately.Also applies to: 220-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/mcp_oauth_e2e_test.go` around lines 175 - 180, The fixture sets ToolsCall: []string{"mcp:tools:write"} in MCPOAuthScopesConfiguration which preemptively enforces write access for every tools/call request and prevents subtests that expect get_schema to succeed with only read scope; change the fixture to remove or relax the shared ToolsCall gate and instead configure scopes explicitly for GetSchema and ExecuteGraphQL (e.g., leave ToolsCall empty or minimal and add per-endpoint scopes for GetSchema and ExecuteGraphQL), and ensure ScopeChallengeMode remains "required_and_existing" while tests that assert behavior with mcp:tools:read update the explicit GetSchema/ExecuteGraphQL scope entries accordingly so the middleware will evaluate the built-in tool scopes as intended.
🧹 Nitpick comments (5)
router/go.mod (1)
141-141: Add// indirectcomment for consistency.The
github.com/modelcontextprotocol/go-sdk v1.4.0dependency is placed in the secondrequireblock, which typically contains indirect dependencies, but it lacks the// indirectcomment. This creates inconsistency with Go module conventions.If this is a direct dependency, it should be moved to the first
requireblock (lines 5-57). If it's indirect, add the// indirectcomment.📝 Suggested fix to add indirect comment
- github.com/modelcontextprotocol/go-sdk v1.4.0 + github.com/modelcontextprotocol/go-sdk v1.4.0 // indirect🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/go.mod` at line 141, The require entry "github.com/modelcontextprotocol/go-sdk v1.4.0" is in the secondary require block but lacks the "// indirect" marker; either move that module line into the primary require block if it's a direct dependency or append the comment "// indirect" to the existing line if it is truly indirect so the go.mod follows convention and remains consistent with other entries.router-tests/cmd/oauth-server/main.go (1)
415-419: Consider handlingrand.Readerror.While unlikely to fail in practice, ignoring the error from
rand.Readcould theoretically produce weak randomness:_, _ = rand.Read(b)For a test server this is acceptable, but for completeness:
Optional: Handle the error
func randomHex(n int) string { b := make([]byte, n) - _, _ = rand.Read(b) + if _, err := rand.Read(b); err != nil { + // Fallback to less secure but functional random for test server + for i := range b { + b[i] = byte(i ^ 0xAB) + } + } return hex.EncodeToString(b) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/cmd/oauth-server/main.go` around lines 415 - 419, The function randomHex currently ignores the error from rand.Read; update randomHex to check the returned error from rand.Read(b) and handle it (e.g., panic/fmt.Errorf or log.Fatalf) instead of discarding it so a failure to fill b cannot produce weak output; locate the randomHex function and replace the anonymous discard of rand.Read's error with an error check and an explicit failure handling strategy (panic/log/return) appropriate for the test server.router-tests/testutil/oauth_server_test.go (1)
36-125: Please pin the unhappy-path OAuth invariants too.The suite exercises the happy paths, but it never asserts that disallowed grant types, mismatched/missing
redirect_uri, or missing/wrong PKCE parameters are rejected. Adding those cases will stop the embedded server from staying more permissive than the real flow you're trying to emulate.Also applies to: 162-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/testutil/oauth_server_test.go` around lines 36 - 125, Test suite TestOAuthTestServer_ClientCredentials currently covers only happy paths; add negative tests in the same file that assert the test server (created via NewOAuthTestServer with OAuthClient entries) rejects invalid requests: (1) send a request with an unsupported grant_type and assert HTTP 400/401, (2) include a client that requires a redirect_uri and send requests with missing or mismatched redirect_uri and assert rejection, and (3) for authorization_code flows requiring PKCE, send requests with missing or incorrect code_challenge/code_verifier and assert rejection; place these cases alongside the existing t.Run blocks so they exercise the same TokenEndpoint() and confirm behavior matches the real OAuth invariants.router-tests/testutil/jwt_helper.go (1)
28-90: Consider extracting the shared JWKS/token-server scaffolding.This constructor, readiness probe, JWKS handler, token minting, and shutdown path now mirror the same logic in
router-tests/testutil/oauth_server.go. A small shared helper would keep claim defaults,kidhandling, and startup behavior from drifting.Also applies to: 92-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/testutil/jwt_helper.go` around lines 28 - 90, The NewJWKSTestServer implementation duplicates scaffolding found in oauth_server.go (constructor, readiness probe, JWKS handler, token minting, shutdown); refactor by extracting a shared test-server helper (e.g., TestAuthServer or jwks_helper) that exposes reusable functions/types used by both NewJWKSTestServer and the oauth server: move startup/listening logic (currently in NewJWKSTestServer), readiness probe logic (waitForReady), JWKS handler (handleJWKS), token minting, and shutdown into that helper and update NewJWKSTestServer to compose/instantiate this shared helper (preserving keyID/kid handling, claim defaults, issuer/audience fields and methods like waitForReady, handleJWKS, and server.Close/Shutdown) so both files reuse the same implementation and avoid drift.router/pkg/config/config.schema.json (1)
2498-2645: Makejwks_configurationthe single source of truth.
mcp.oauth.jwksduplicates the new$defs.jwks_configuration, and they already drifted:allowed_useexists only in the inline version. Either switch both JWKS sites to$ref: "#/$defs/jwks_configuration"after filling in the missing fields, or drop the unused def before it diverges further.Also applies to: 3929-4059
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config.schema.json` around lines 2498 - 2645, The jwks schema is duplicated: mcp.oauth.jwks is an inline definition while $defs.jwks_configuration is the intended canonical type and currently missing the allowed_use property; make jwks_configuration the single source of truth by updating $defs.jwks_configuration to include all fields present in the inline jwks (notably allowed_use, refresh_unknown_kid, refresh_interval, algorithms, secret, symmetric_algorithm, header_key_id, audiences, url) and then replace the inline mcp.oauth.jwks definition with a $ref to "#/$defs/jwks_configuration" (or, if you prefer to keep the inline version, remove the unused $defs.jwks_configuration to avoid divergence). Ensure the oneOf rules (the two variants: URL-based and secret-based) are preserved inside $defs.jwks_configuration so validation behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client-tests/mcp-ts/src/mcp-client-credentials.test.ts`:
- Around line 88-109: The test suite "MCP client_credentials with scopes" only
exercises the happy path; modify the test using ClientCredentialsProvider and
StreamableHTTPClientTransport so it actually verifies per-tool scope
enforcement: create two provider instances (one with limited scopes and one with
broader scopes), connect the Client (name 'mcp-ts-scoped-test') and call a
specific tool or endpoint whose required scope is known (use the tool name you
expect to enforce scope), assert that the limited-scoped provider receives an
authorization/denial (expect an error or 403) when invoking the tool, then
reconnect with the broader-scoped provider and assert success (tools call
returns expected result), and ensure transport.close() is still called in
finally blocks for both cases.
- Around line 66-76: The test currently can return early without calling any
tool when neither queryTool nor a 'get_schema' tool exists; change the fallback
so the test fails or exercises a real call: assert that tools.length > 0 (or
throw a descriptive error) instead of just logging and returning, and then
invoke client.callTool({ name: tools[0].name, arguments: {} }) and assert the
result is defined; update the block that references queryTool, the 'get_schema'
lookup, and the fallback logging to perform the assertion/call so the "should
call a read-only tool" test cannot silently pass as a noop.
In `@router-tests/mcp_auth_e2e_test.go`:
- Around line 194-223: The tests in mcp_auth_e2e_test.go create MCP clients
(NewMCPAuthClient) and call methods like Connect and CallTool but never enable
real auth: only config.MCPConfiguration.Enabled = true is set; you must
configure and enable auth in each relevant subtest by populating
config.MCPOAuthConfiguration (including required scopes and a test JWKS/JWT
issuer) via testenv.Config, supply matching tokens (or rotation scenarios) and
expected scope requirements, and update assertions to assert success/failure
according to auth (e.g., for the "Scope upgrade on persistent session" test
assert the exact expected outcome rather than accepting both). Ensure tests that
exercise token injection, insufficient-scope challenge, and same-session
rotation explicitly set JWKS, required scopes, and use
previewToken/NewMCPAuthClient tokens that match those settings so failures
reflect real auth behavior.
- Around line 29-47: The authRoundTripper struct has concurrent access races on
token and lastResponse; modify authRoundTripper to include a sync.RWMutex (or
sync.Mutex) and use it to protect all reads/writes to token and lastResponse:
acquire a read lock when reading token in RoundTrip to set the Authorization
header, and upgrade to a write lock (or use a write lock) when assigning
a.lastResponse after the base.RoundTrip call; likewise guard SetToken and
checkAuthError to acquire the appropriate lock when reading or setting
token/lastResponse. Ensure you reference and update the authRoundTripper type
and its methods RoundTrip, SetToken, and checkAuthError to use the mutex
consistently to eliminate data races.
In `@router-tests/mcp_auth_harness_example.go`:
- Around line 89-113: The example is still reading required_scopes from the
JSON-RPC body (jsonRPCResp) instead of handling an HTTP 403 + WWW-Authenticate
challenge on the transport; update the logic that inspects resp (check
resp.StatusCode == 403) to parse the WWW-Authenticate header for the
insufficient_scope challenge and required_scopes, print the HTTP challenge in
the expected output block (alongside or instead of the JSON-RPC block), and
return the scopes extracted from the HTTP header; apply the same fix to the
other example blocks noted (the ranges around lines 158-167 and 206-241) so they
all check resp.StatusCode and WWW-Authenticate before relying on the JSON-RPC
error payload.
In `@router-tests/testutil/oauth_server.go`:
- Around line 200-225: In handleClientCredentials (and the other token handlers
referenced), verify the client's GrantTypes before issuing a token: after
loading the client (client := s.clients[clientID]) check that the requested
grant (e.g., "client_credentials" in handleClientCredentials and
"authorization_code" in the auth-code handler) is present in client.GrantTypes
(or return an "unauthorized_client" via s.tokenError with an appropriate message
and http.StatusUnauthorized) and only call s.issueTokenResponse when the grant
is allowed; do the same for the other handlers mentioned (the auth-code branch
and the refresh/token branches) so clients cannot mint tokens for grant types
they were not registered for.
- Around line 30-36: The auth flow advertises PKCE (S256) but the authCode type
and exchange path never bind or verify PKCE/redirects; update the authCode
struct to include codeChallenge (and codeChallengeMethod if needed) and store
the original redirectURI when issuing codes, then modify the token/exchange
handler to validate the provided code_verifier against the stored codeChallenge
(applying S256 when method is S256) and to compare the exchanged redirect_uri to
the stored redirectURI before issuing tokens; alternatively, if you prefer not
to implement PKCE/redirect binding now, remove S256 from the discovery metadata
so the server does not advertise PKCE support (ensure changes reference
authCode, the authorization code issuance path, and the token exchange handler).
- Around line 301-308: The redirect URL is built by simple string concatenation
(variables redirectURI, code, state, and location) which fails when redirectURI
already has query params and doesn't escape values; update the callback
construction to parse redirectURI into a url.URL, use its Query()/url.Values to
set or add the "code" and preserve "state" (URL-encode values), then rebuild the
full URL via url.URL.String() and pass that to http.Redirect(w, r, ...) so
existing query params are preserved and parameters are properly escaped.
In `@router/pkg/config/config.go`:
- Around line 1067-1094: The MCPOAuthScopesConfiguration fields lack env tags so
env.Parse(&cfg.Config) ignores MCP_OAUTH_SCOPES_* variables; update the struct
(MCPOAuthScopesConfiguration) to add env tags for each field (Initialize,
ToolsList, ToolsCall, ExecuteGraphQL, GetOperationInfo, GetSchema) such that
their tags produce MCP_OAUTH_SCOPES_<FIELD> env names (e.g.
env:"SCOPES_INITIALIZE", env:"SCOPES_TOOLS_LIST", env:"SCOPES_TOOLS_CALL",
env:"SCOPES_EXECUTE_GRAPHQL", env:"SCOPES_GET_OPERATION_INFO",
env:"SCOPES_GET_SCHEMA") and include any envDefault values if desired so
env-only deployments populate these scope slices.
In `@router/pkg/config/config.schema.json`:
- Around line 2583-2598: The JSON schema defaults for refresh_unknown_kid (the
"max_wait" and "interval" properties) do not match the values seeded in code
(config.RefreshUnknownKID.MaxWait = 2m and config.RefreshUnknownKID.Interval =
30s); update the schema's "default" fields for max_wait and interval in the
refresh_unknown_kid sections (both occurrences) to "2m" and "30s" respectively
so the schema/defaulted/generated docs align with the runtime defaults defined
by config.RefreshUnknownKID.
- Around line 2441-2668: When mcp.oauth.enabled is true the schema must require
at least one JWKS entry; update the existing if/then block so the "then" not
only requires server.base_url but also requires oauth.jwks (and ensure jwks has
"minItems": 1 to prevent an empty array). Concretely, in the same if/then you
added, add under "then" -> "properties" -> "oauth": { "required": ["jwks"] } and
add/ensure at the top-level oauth.jwks definition includes "minItems": 1 so a
non-empty JWKS list is enforced (this references the "oauth" object, the "jwks"
array and the existing if/then block).
In `@router/pkg/mcpserver/auth_middleware.go`:
- Around line 23-25: The middleware currently treats body-read/size-limit
failures as auth failures; update the handler in auth_middleware.go that reads
the request body (the code using maxBodyBytes) to return a payload-specific
error instead of a 401/WWW-Authenticate response: detect read errors and
size-limit hits from the limited reader/ReadAll and respond with
http.StatusRequestEntityTooLarge (413) or http.StatusBadRequest as appropriate,
ensure you do not set a WWW-Authenticate header on those responses, and keep
auth 401 responses only for genuine auth failures.
In `@router/pkg/mcpserver/operation_manager.go`:
- Around line 91-98: The ComputeToolScopes method currently only sets
om.operations[i].RequiredScopes when fieldReqs is non-empty, leaving stale
scopes if annotations are removed; update OperationsManager.ComputeToolScopes to
reset om.operations[i].RequiredScopes (e.g., set to nil or empty slice) before
or in the else branch so that when extractor.ExtractScopesForOperation returns
zero fieldReqs the operation's RequiredScopes is cleared; modify the loop over
om.operations in ComputeToolScopes to explicitly clear RequiredScopes for each
operation when no scopes are computed.
In `@router/pkg/mcpserver/scope_extractor.go`:
- Around line 72-88: ComputeCombinedScopes currently materializes the full
Cartesian product via crossProduct which can explode; modify
ComputeCombinedScopes (and crossProduct) to enforce a hard cap (e.g.,
MaxScopeCombinations) and stop cross-product expansion once the cap would be
exceeded, returning a truncated result or an explicit error/indicator the cap
was hit so callers can fall back to lazy/iterative auth checks; alternatively
implement a lazy iterator/yielder API for combinations and change callers to
consume combinations until satisfied. Update the symbols ComputeCombinedScopes
and crossProduct to check combination counts during expansion and surface a
clear signal when the cap is reached so downstream code (e.g., where
ComputeCombinedScopes is called) can perform incremental or best-challenge
evaluation instead of relying on a full product.
In `@router/pkg/mcpserver/server.go`:
- Around line 779-787: The handler is currently logging raw identity claims (sub
and email) in GraphQLSchemaServer.handleOperation which exposes PII; change the
log to avoid raw identifiers by either omitting sub/email or computing and
logging a non-reversible correlation key (e.g., hash of getClaimString(claims,
"sub") or getClaimString(claims, "email")) and use that instead in the zap
fields; update the same pattern in the other occurrence (around the operation
handler at lines 981-987) and keep the log context (operation name via
handler.operation.Name) but remove direct PII exposure from logger.Debug.
- Around line 1055-1117: The metadata currently sets
ProtectedResourceMetadata.Resource to resourceURL (origin only); change it to
advertise the MCP endpoint path by ensuring the resource includes "/mcp" (e.g.
use a cleaned base URL from s.serverBaseURL or the derived resourceURL and
append "/mcp" without duplicating slashes). Update the code that computes
resourceURL (used to build ResourceDocumentation and Resource) or set
metadata.Resource explicitly to the normalized resource + "/mcp", trimming any
trailing slash (use strings.TrimRight) so ProtectedResourceMetadata.Resource
consistently points at the path-scoped /mcp protected resource.
- Around line 217-225: When OAuth is enabled the code currently advertises a
resource_metadata URL even when AuthorizationServerURL is empty, causing a dead
discovery link; update the constructor checks (in the block referencing
options.OAuthConfig and options.OAuthConfig.JWKS) to also require
options.OAuthConfig.AuthorizationServerURL to be non-empty and fail closed (call
cancel() and return an error) if it is empty; alternatively, ensure Serve() and
the resource_metadata builder use the same guard by either mounting discovery
only when AuthorizationServerURL is present or by preventing advertising the
resource_metadata URL when AuthorizationServerURL is empty—apply this validation
consistently for the constructor paths around the
AuthorizationServerURL/resource_metadata logic (also mirror the same fix in the
later similar block referenced in the review).
---
Outside diff comments:
In `@router/pkg/mcpserver/server.go`:
- Around line 941-977: Check resp.StatusCode after reading the body and before
parsing GraphQL: if the status is not 2xx, return an error CallToolResult
(IsError: true) that includes the HTTP status code and the response body text so
callers can distinguish upstream auth/transport failures; update the logic
around resp, body, and the existing GraphQLResponse handling to short-circuit
non-2xx responses and use mcp.CallToolResult (with &mcp.TextContent{Text: ...})
to surface the status and body.
- Around line 431-478: The closure passed to mcp.NewStreamableHTTPHandler
currently returns s.server without propagating request headers, so
executeGraphQLQuery (which reads headers via headersFromContext(ctx)) loses
Authorization/cookie information when authMiddleware.HTTPMiddleware isn’t used;
modify that closure (the func(req *http.Request) *mcp.Server passed to
mcp.NewStreamableHTTPHandler) to inject the incoming request headers into the
request context (using the project's header-to-context helper that pairs with
headersFromContext or by adding the headers under the same context key used by
headersFromContext) before returning s.server, so downstream executeGraphQLQuery
can read the original headers even when OAuth/authMiddleware is disabled.
---
Minor comments:
In `@demo/go.mod`:
- Line 97: The demo/go.mod currently has an indirect dependency
github.com/mark3labs/mcp-go v0.43.2 which conflicts with router/router-tests
pinned to v0.36.0; decide and enforce a single source of truth: either (A) align
demo to router by replacing the indirect dependency with
github.com/mark3labs/mcp-go v0.36.0 in demo/go.mod, or (B) migrate demo to the
new upstream package modelcontextprotocol/go-sdk v1.4.0 (mirror the router
migration) by adding the modelcontextprotocol/go-sdk v1.4.0 requirement and
updating any imports that reference github.com/mark3labs/mcp-go to the new SDK;
update go.mod accordingly and run go mod tidy to ensure no indirect version skew
remains.
In `@router-tests/mcp_oauth_e2e_test.go`:
- Around line 108-113: The test is currently skipping AuthError assertions when
err is nil or not an AuthError; change the checks to be strict by first
asserting an error was returned (e.g., require.Error(t, err)) and then asserting
the error is an AuthError (e.g., require.True(t, ok) or require.IsType(t,
&AuthError{}, err)) before inspecting authErr.StatusCode and
authErr.ResourceMetadataURL in the block for authErr; update both occurrences
that use authErr/ok (lines around the existing AuthError handling) so the test
fails if err is nil or of the wrong type rather than silently passing.
- Around line 175-180: The fixture sets ToolsCall: []string{"mcp:tools:write"}
in MCPOAuthScopesConfiguration which preemptively enforces write access for
every tools/call request and prevents subtests that expect get_schema to succeed
with only read scope; change the fixture to remove or relax the shared ToolsCall
gate and instead configure scopes explicitly for GetSchema and ExecuteGraphQL
(e.g., leave ToolsCall empty or minimal and add per-endpoint scopes for
GetSchema and ExecuteGraphQL), and ensure ScopeChallengeMode remains
"required_and_existing" while tests that assert behavior with mcp:tools:read
update the explicit GetSchema/ExecuteGraphQL scope entries accordingly so the
middleware will evaluate the built-in tool scopes as intended.
In `@router/go.mod`:
- Around line 93-99: The go.mod currently requires the pre-v1 module
golang.org/x/oauth2 v0.34.0; confirm this complies with our dependency policy
and either (a) upgrade to a stable v1+ release if policy disallows pre-v1
(replace golang.org/x/oauth2 v0.34.0 with the approved v1+ tag), or (b)
explicitly document/justify acceptance of this pre-v1 dependency (add a short
comment in router/go.mod and/or the PR description referencing
golang.org/x/oauth2 v0.34.0) so reviewers know this was intentional; locate the
entry for golang.org/x/oauth2 in router/go.mod to apply the change.
In `@router/pkg/config/testdata/config_full.json`:
- Around line 205-218: Add the new scope-challenge setting into the OAuth block
of the full-config fixture by adding the "ScopeChallengeMode" property (set to a
representative value like "strict" — other valid values are "smart" or
"optimistic") alongside "ScopeChallengeIncludeTokenScopes"; this ensures the
config_full.json fixture exercises the challenge-mode code paths when
loading/round-tripping the "OAuth" object.
In `@router/pkg/mcpserver/scope_challenge.md`:
- Around line 13-15: The unlabeled code fences in scope_challenge.md (e.g., the
example showing `[["a", "b"], ["c", "d"]] → (a AND b) OR (c AND d)` and the
other examples at the ranges you noted) should be changed to use a labeled
fence, e.g., replace the opening ``` with ```text for each unlabeled block so
markdownlint MD040 is satisfied; apply this same change to the other unlabeled
examples referenced (lines ~42-44, 55-57, 67-74) so every code fence is
explicitly `text`.
---
Nitpick comments:
In `@router-tests/cmd/oauth-server/main.go`:
- Around line 415-419: The function randomHex currently ignores the error from
rand.Read; update randomHex to check the returned error from rand.Read(b) and
handle it (e.g., panic/fmt.Errorf or log.Fatalf) instead of discarding it so a
failure to fill b cannot produce weak output; locate the randomHex function and
replace the anonymous discard of rand.Read's error with an error check and an
explicit failure handling strategy (panic/log/return) appropriate for the test
server.
In `@router-tests/testutil/jwt_helper.go`:
- Around line 28-90: The NewJWKSTestServer implementation duplicates scaffolding
found in oauth_server.go (constructor, readiness probe, JWKS handler, token
minting, shutdown); refactor by extracting a shared test-server helper (e.g.,
TestAuthServer or jwks_helper) that exposes reusable functions/types used by
both NewJWKSTestServer and the oauth server: move startup/listening logic
(currently in NewJWKSTestServer), readiness probe logic (waitForReady), JWKS
handler (handleJWKS), token minting, and shutdown into that helper and update
NewJWKSTestServer to compose/instantiate this shared helper (preserving
keyID/kid handling, claim defaults, issuer/audience fields and methods like
waitForReady, handleJWKS, and server.Close/Shutdown) so both files reuse the
same implementation and avoid drift.
In `@router-tests/testutil/oauth_server_test.go`:
- Around line 36-125: Test suite TestOAuthTestServer_ClientCredentials currently
covers only happy paths; add negative tests in the same file that assert the
test server (created via NewOAuthTestServer with OAuthClient entries) rejects
invalid requests: (1) send a request with an unsupported grant_type and assert
HTTP 400/401, (2) include a client that requires a redirect_uri and send
requests with missing or mismatched redirect_uri and assert rejection, and (3)
for authorization_code flows requiring PKCE, send requests with missing or
incorrect code_challenge/code_verifier and assert rejection; place these cases
alongside the existing t.Run blocks so they exercise the same TokenEndpoint()
and confirm behavior matches the real OAuth invariants.
In `@router/go.mod`:
- Line 141: The require entry "github.com/modelcontextprotocol/go-sdk v1.4.0" is
in the secondary require block but lacks the "// indirect" marker; either move
that module line into the primary require block if it's a direct dependency or
append the comment "// indirect" to the existing line if it is truly indirect so
the go.mod follows convention and remains consistent with other entries.
In `@router/pkg/config/config.schema.json`:
- Around line 2498-2645: The jwks schema is duplicated: mcp.oauth.jwks is an
inline definition while $defs.jwks_configuration is the intended canonical type
and currently missing the allowed_use property; make jwks_configuration the
single source of truth by updating $defs.jwks_configuration to include all
fields present in the inline jwks (notably allowed_use, refresh_unknown_kid,
refresh_interval, algorithms, secret, symmetric_algorithm, header_key_id,
audiences, url) and then replace the inline mcp.oauth.jwks definition with a
$ref to "#/$defs/jwks_configuration" (or, if you prefer to keep the inline
version, remove the unused $defs.jwks_configuration to avoid divergence). Ensure
the oneOf rules (the two variants: URL-based and secret-based) are preserved
inside $defs.jwks_configuration so validation behavior remains identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03dcc95f-a4ff-463b-aa61-3a8ead8853ab
⛔ Files ignored due to path filters (4)
client-tests/mcp-ts/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamldemo/go.sumis excluded by!**/*.sumrouter-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
client-tests/Makefileclient-tests/mcp-ts/package.jsonclient-tests/mcp-ts/src/mcp-client-credentials.test.tsclient-tests/mcp-ts/tsconfig.jsonclient-tests/mcp-ts/vitest.config.tsdemo/go.modrouter-tests/cmd/oauth-server/main.gorouter-tests/go.modrouter-tests/mcp_auth_e2e_test.gorouter-tests/mcp_auth_harness_example.gorouter-tests/mcp_oauth_e2e_test.gorouter-tests/mcp_test.gorouter-tests/oauth-serverrouter-tests/testenv/testenv.gorouter-tests/testutil/auth_helpers.gorouter-tests/testutil/jwt_helper.gorouter-tests/testutil/oauth_server.gorouter-tests/testutil/oauth_server_test.gorouter/core/graph_server.gorouter/core/router.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/mcpserver/auth_middleware.gorouter/pkg/mcpserver/auth_middleware_test.gorouter/pkg/mcpserver/errors.gorouter/pkg/mcpserver/execute_graphql_scope_test.gorouter/pkg/mcpserver/operation_manager.gorouter/pkg/mcpserver/schema_compiler.gorouter/pkg/mcpserver/scope_challenge.gorouter/pkg/mcpserver/scope_challenge.mdrouter/pkg/mcpserver/scope_challenge_test.gorouter/pkg/mcpserver/scope_extractor.gorouter/pkg/mcpserver/scope_extractor_test.gorouter/pkg/mcpserver/server.gorouter/pkg/schemaloader/loader.gorouter/pkg/schemaloader/loader_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/pkg/schemaloader/loader.go (1)
35-43:⚠️ Potential issue | 🟠 MajorGuard against a nil logger.
Loggeris a nullable pointer, butLoadOperationsFromDirectorycallsl.Logger.Error(...)on every error path without checking for nil. If any caller passesniltoNewOperationLoaderor instantiatesOperationLoaderdirectly, the first file parsing/validation failure will panic instead of logging.Suggested fix
func NewOperationLoader(logger *zap.Logger, schemaDoc *ast.Document) *OperationLoader { + if logger == nil { + logger = zap.NewNop() + } return &OperationLoader{ SchemaDocument: schemaDoc, Logger: logger, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/schemaloader/loader.go` around lines 35 - 43, OperationLoader currently stores a nullable Logger and LoadOperationsFromDirectory calls l.Logger.Error(...) without guarding for nil; update NewOperationLoader to ensure Logger is non-nil by defaulting to a no-op logger when logger == nil, or add nil-checks before every l.Logger.Error use in LoadOperationsFromDirectory; specifically modify NewOperationLoader to replace a nil logger with zap.NewNop() (or equivalent) so OperationLoader.Logger is always safe to call, and confirm LoadOperationsFromDirectory uses the Logger field (l.Logger.Error) without additional nil checks.
♻️ Duplicate comments (12)
router-tests/cmd/oauth-server/main.go (1)
128-132:⚠️ Potential issue | 🟠 MajorBind auth codes to registered redirect URIs.
/authorizeaccepts anyredirect_uriand stores codes without that URI, so/tokencannot verify the callback on exchange. That leaves an open redirect here and makes the authorization-code flow behave differently from a real OAuth server. Store the allowed redirect URIs on the client, bind the selected URI toauthCode, and require an exact match during exchange.Also applies to: 252-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/cmd/oauth-server/main.go` around lines 128 - 132, The auth code is not tied to a redirect URI—update the authCode type to include the selected redirect URI, ensure the client representation (client struct) contains its allowed redirect URIs, validate the incoming redirect_uri in the /authorize handler against the client's allowed list and store the chosen redirect_uri on the authCode, then in the /token handler require an exact match between the presented redirect_uri and the redirect_uri stored on the authCode (and that it is an allowed URI for the client) before issuing tokens.router-tests/mcp_auth_e2e_test.go (2)
193-335:⚠️ Potential issue | 🟠 MajorThese auth E2Es still do not exercise real auth.
Every subtest only enables
MCP.Enabled; none configureMCP.OAuth, JWKS, or required scopes, and the “Scope upgrade” case explicitly accepts both success and failure. As written, this file passes even if JWT validation, insufficient-scope challenges, and same-session token rotation are broken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/mcp_auth_e2e_test.go` around lines 193 - 335, The tests in TestMCPAuthorizationWithOfficialSDK are not exercising real auth because the test env only sets MCP.Enabled and never configures MCP.OAuth, JWKS, or required scopes; update the test cases to supply a real/fixture OAuth configuration (populate MCP.OAuth/JWKS and the server’s required scopes) so JWT validation and scope checks run, then make deterministic assertions (do not accept both success and failure) around CallTool responses; specifically adjust the testenv.Config passed into each t.Run, and ensure the Scope upgrade subtest uses NewMCPAuthClient, SetToken and CallTool to trigger an actual insufficient-scope error before calling SetToken and asserting the write succeeds with the upgraded token.
29-47:⚠️ Potential issue | 🟠 MajorGuard
authRoundTripperstate with a mutex.
http.Clientcan callRoundTripconcurrently, andSetToken/checkAuthErrortouch the same fields. As written,tokenandlastResponserace and can misattribute failures after a token change.🔧 Suggested fix
import ( "context" "fmt" "net/http" "strings" + "sync" "testing" @@ type authRoundTripper struct { base http.RoundTripper + mu sync.RWMutex token string lastResponse *http.Response } @@ - if a.token != "" { - req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", a.token)) - } + a.mu.RLock() + token := a.token + a.mu.RUnlock() + if token != "" { + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + } @@ - a.lastResponse = resp + a.mu.Lock() + a.lastResponse = resp + a.mu.Unlock() return resp, err } @@ func (c *MCPAuthClient) SetToken(token string) { - c.roundTripper.token = token + c.roundTripper.mu.Lock() + c.roundTripper.token = token + c.roundTripper.mu.Unlock() } @@ - if c.roundTripper.lastResponse == nil { + c.roundTripper.mu.RLock() + resp := c.roundTripper.lastResponse + c.roundTripper.mu.RUnlock() + if resp == nil { return nil } - - resp := c.roundTripper.lastResponseAlso applies to: 120-122, 146-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/mcp_auth_e2e_test.go` around lines 29 - 47, The authRoundTripper fields token and lastResponse are accessed concurrently; add a mutex (e.g., sync.Mutex or sync.RWMutex) to authRoundTripper and guard all reads/writes to token and lastResponse in RoundTrip, SetToken, and checkAuthError: acquire the lock to read token into a local variable, release it before calling a.base.RoundTrip, then after receiving resp reacquire the lock to set lastResponse; likewise protect SetToken and checkAuthError so they lock when mutating or reading those fields. Apply the same mutex-guard pattern to the other authRoundTripper usages mentioned (the other ranges) to eliminate data races.router/pkg/mcpserver/server.go (3)
779-787:⚠️ Potential issue | 🟠 MajorAvoid logging raw identity claims on every tool call.
suband especiallyAlso applies to: 981-987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/server.go` around lines 779 - 787, The current handleOperation logging emits raw PII ("sub" and "email") via getClaimString(GetClaimsFromContext), so update the logging in GraphQLSchemaServer.handleOperation (and similarly where repeated around handler.operation.Name) to avoid PII: instead derive and log a non-reversible correlation key (e.g., hash the "sub" claim) or omit both claims entirely, and log only the correlation key plus handler.operation.Name and any non-sensitive context; use GetClaimsFromContext and getClaimString to read claims but do the hashing/omission before calling s.logger.Debug.
217-276:⚠️ Potential issue | 🟠 MajorRequire
AuthorizationServerURLwhen OAuth is enabled.The constructor always builds a
resource_metadataURL and passes it to the auth middleware, butServeonly mounts/.well-known/oauth-protected-resource/mcpwhenAuthorizationServerURLis non-empty. With the current checks, clients can receive a dead discovery URL inWWW-Authenticate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/server.go` around lines 217 - 276, When OAuth is enabled, require options.OAuthConfig.AuthorizationServerURL to be set to avoid passing a dead discovery URL to NewMCPAuthMiddleware; add a validation like the existing ServerBaseURL check that cancels context and returns an error if AuthorizationServerURL is empty, and then use AuthorizationServerURL (or ensure consistency) when building resourceMetadataURL before calling NewMCPAuthMiddleware (references: options.OAuthConfig.AuthorizationServerURL, options.ServerBaseURL, resourceMetadataURL, NewMCPAuthMiddleware).
1055-1117:⚠️ Potential issue | 🟠 MajorAdvertise
/mcpas the protected resource, not just the origin.This metadata is path-scoped to the MCP endpoint, but
ProtectedResourceMetadata.Resourceis currently the bare base URL. Clients that use the advertised resource indicator/audience will request tokens for the wrong protected resource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/server.go` around lines 1055 - 1117, The metadata currently sets ProtectedResourceMetadata.Resource to the base resourceURL, but the MCP metadata is path-scoped to /mcp; update the assignment so Resource advertises the full MCP path (e.g., append "/mcp" to resourceURL while avoiding double slashes) before constructing metadata. Locate the resourceURL variable and the metadata := ProtectedResourceMetadata{... Resource: ... } block and replace Resource: resourceURL with Resource: fmt.Sprintf("%s/mcp", strings.TrimRight(resourceURL, "/")) (or equivalent) so Resource and ResourceDocumentation consistently point to the MCP endpoint.router/pkg/mcpserver/scope_extractor.go (1)
74-85:⚠️ Potential issue | 🟠 MajorCap or avoid the full scope cross-product on auth paths.
ComputeCombinedScopesstill materializes every OR-combination eagerly. For arbitraryexecute_graphqloperations this grows exponentially with the number of scoped fields, so one large request can burn a lot of CPU/memory before authorization even finishes. Add a hard cap or switch this path to lazy/short-circuit evaluation.Also applies to: 129-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/scope_extractor.go` around lines 74 - 85, ComputeCombinedScopes eagerly builds the full cross-product of OR-groups via crossProduct which can explode; add a hard cap or lazy short-circuit to avoid materializing too many combinations. Introduce a MAX_SCOPE_COMBINATIONS constant and change crossProduct and ComputeCombinedScopes to accept a maxLimit (or return an error/indicator) so that crossProduct can count combinations and early-return once the limit is exceeded; alternatively implement an iterator/generator API that yields combinations and lets callers stop early. Update callers of ComputeCombinedScopes/crossProduct (and FieldScopeRequirement handling) to check for the limit-exceeded signal and handle it (e.g., fall back to coarse-grained auth or return an explicit error). Ensure function signatures mention the new limit/indicator so behavior is explicit.router/pkg/config/config.go (1)
1062-1095:⚠️ Potential issue | 🟠 Major
MCP_OAUTH_SCOPES_*env config is still a no-op.
MCPOAuthConfigurationopts intoenvPrefix:"MCP_OAUTH_", butScopeshas no nestedenvPrefixand the leaf fields only have YAML tags. Withenv.Parse(&cfg.Config), env-only deployments silently load no MCP OAuth tool scopes. AddenvPrefix:"SCOPES_"onScopesandenv:"..."tags on each field soMCP_OAUTH_SCOPES_*works as intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config.go` around lines 1062 - 1095, MCPOAuthConfiguration's Scopes field isn't receiving env vars because MCPOAuthScopesConfiguration lacks an envPrefix and its slice fields have no env tags; update the Scopes field on type MCPOAuthConfiguration to include struct tag envPrefix:"SCOPES_" and add env:"<NAME>" tags to each field in MCPOAuthScopesConfiguration (e.g. Initialize -> env:"INITIALIZE", ToolsList -> env:"TOOLS_LIST", ToolsCall -> env:"TOOLS_CALL", ExecuteGraphQL -> env:"EXECUTE_GRAPHQL", GetOperationInfo -> env:"GET_OPERATION_INFO", GetSchema -> env:"GET_SCHEMA") so MCP_OAUTH_SCOPES_* environment variables are parsed by env.Parse(&cfg.Config).router-tests/testutil/oauth_server.go (2)
200-225:⚠️ Potential issue | 🟠 MajorReject grant types the client was never registered for.
Both token branches authenticate the client, but neither checks
client.GrantTypesbefore issuing a token. Anauthorization_code-only client can still mintclient_credentialstokens, and vice versa, so this test server accepts flows a real authorization server should reject.Also applies to: 227-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/testutil/oauth_server.go` around lines 200 - 225, The handler handleClientCredentials currently authenticates the client but never verifies that the client is allowed to use the "client_credentials" grant; update handleClientCredentials to check the registered client.GrantTypes (from the clients map) and return a token error (invalid_grant or unauthorized) if "client_credentials" is not present before calling issueTokenResponse; likewise update the authorization code path (e.g., handleAuthCode / the branch in 227-274) to require "authorization_code" in client.GrantTypes before issuing tokens. Use the existing client lookup (s.clients[clientID]) and tokenError response pattern to reject disallowed grant types.
23-36:⚠️ Potential issue | 🟠 MajorThe auth-code flow is looser than the metadata it advertises.
Metadata claims
S256PKCE support, butauthCodenever stores a challenge and the exchange path never reads acode_verifier. Registration also dropsredirect_uris,/authorizeaccepts anyredirect_uri, and/tokennever compares the exchangedredirect_uriwith the one stored on the code. That makes this helper materially more permissive than the OAuth 2.1 server your auth-code E2Es are supposed to model. Tighten the code/redirect binding here, or stop advertisingS256.Also applies to: 159-171, 227-308, 318-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/testutil/oauth_server.go` around lines 23 - 36, The test OAuth helper advertises PKCE S256 and redirect URI binding but doesn't implement them: extend OAuthClient to include RedirectURIs (e.g., RedirectURIs []string), update authCode to store redirectURI, codeChallenge, and codeChallengeMethod, make the /authorize handler validate the provided redirect_uri against the client's RedirectURIs and store the chosen redirectURI plus any code_challenge/code_challenge_method on the created authCode, and update the /token exchange handler to verify the incoming redirect_uri matches the stored authCode.redirectURI and to validate the code_verifier when codeChallengeMethod == "S256" (compute S256 of verifier and compare to stored codeChallenge); reference OAuthClient, authCode, the authorize handler, and the token exchange handler when making these changes.router/pkg/config/config.schema.json (1)
2650-2668:⚠️ Potential issue | 🟠 MajorRequire a non-empty
oauth.jwkslist when OAuth is enabled.The current
if/thenonly forcesserver.base_url.mcp.oauth.enabled: truestill validates with no JWKS providers configured, so users can enable token enforcement without any verifier and only fail later at runtime.🔒 Suggested schema constraint
"then": { "properties": { + "oauth": { + "required": ["jwks"], + "properties": { + "jwks": { + "minItems": 1 + } + } + }, "server": { "required": ["base_url"] } }, "required": ["server"] }Based on learnings, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config.schema.json` around lines 2650 - 2668, The schema should require a non-empty oauth.jwks array when oauth.enabled is true: update the existing if/then block so the "then" also requires "oauth": { "properties": { "jwks": { "type":"array", "minItems":1, "items": { "type":"object", "required":["secret","algorithm","kid"], "properties": { "secret":{"type":"string","minLength":1}, "algorithm":{"type":"string","minLength":1}, "kid":{"type":"string","minLength":1} } } } } } in addition to the existing server.base_url requirement so mcp.oauth.enabled:true fails schema validation unless at least one JWKS provider with non-empty secret, algorithm and kid is provided.router/pkg/mcpserver/auth_middleware.go (1)
197-206:⚠️ Potential issue | 🟠 MajorReturn payload errors here, not auth challenges.
Lines 200-205 still convert body-read and body-size failures into
401plusWWW-Authenticate. A malformed or oversized JSON-RPC request is not an auth failure, and challenging here sends clients into the wrong recovery flow.♻️ Suggested fix
if r.Body != nil { limitedReader := io.LimitReader(r.Body, maxBodyBytes+1) body, err = io.ReadAll(limitedReader) if err != nil { - m.sendUnauthorizedResponse(w, fmt.Errorf("failed to read request body")) + http.Error(w, "failed to read request body", http.StatusBadRequest) return } if int64(len(body)) > maxBodyBytes { - m.sendUnauthorizedResponse(w, fmt.Errorf("request body too large")) + http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) return } // Restore body for downstream handlers r.Body = io.NopCloser(bytes.NewBuffer(body)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/auth_middleware.go` around lines 197 - 206, The code currently treats body read/size failures as auth failures by calling m.sendUnauthorizedResponse; instead return a payload/bad-request error (HTTP 400) without an auth challenge. Replace the two calls to m.sendUnauthorizedResponse in the r.Body handling block (after io.ReadAll and after the size check) with a call that returns a 400-level payload error (e.g. m.sendBadRequestResponse or m.sendPayloadErrorResponse) that includes the underlying error details (wrap the err from io.ReadAll in the log/error message and include a clear "request body too large" message for the size check) so clients get a proper JSON-RPC/malformed-payload response rather than a WWW-Authenticate challenge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/mcp_oauth_e2e_test.go`:
- Around line 108-113: The test currently only performs status/resource-metadata
assertions when err is an *AuthError* and silently skips them otherwise; add an
explicit assertion that the error is the expected type before those checks—for
the block using authErr, ok := err.(*AuthError) assert the type with
require.True(t, ok) (or require.IsType(t, &AuthError{}, err)) and then perform
assert.Equal(t, http.StatusUnauthorized, authErr.StatusCode) and
assert.NotEmpty(t, authErr.ResourceMetadataURL); apply the same change to the
other occurrences referenced (the blocks around lines 146-151 and 194-198) so
transport/setup failures cannot incorrectly pass the test.
- Around line 23-24: The test currently only uses the initial readOnlyToken for
get_schema and then swaps to a new token, so it exercises rotation but not scope
upgrade — add a pre-upgrade call to execute_graphql using the client configured
with readOnlyToken (the token returned from oauthServer.CreateTokenWithScopes
and applied via SetToken) that invokes the same operation used later and assert
it returns a 403 with an "insufficient_scope" error before you call
SetToken(newToken); apply the same change to the other similar blocks (the other
test sections that use readOnlyToken and SetToken) so each has an
execute_graphql pre-upgrade assertion expecting 403 insufficient_scope.
In `@router-tests/testutil/jwt_helper.go`:
- Around line 33-35: The helper currently calls freeport.GetOne(t) and publishes
issuer and jwksURL before actually binding, allowing races; change the flow to
reserve the port by creating a net.Listener (e.g., net.Listen on "tcp" with
":"+portStr) immediately after obtaining the free port (or let the OS pick with
":0" and read listener.Addr()) and only then construct and publish issuer and
jwksURL from the bound listener's address; apply the same change to the other
occurrences flagged (around the blocks using freeport.GetOne/t, jwksURL, issuer
at the ranges you noted) so the listener is held open until the server is
started.
In `@router/pkg/config/config.schema.json`:
- Around line 2493-2497: The schema currently exposes only
scope_challenge_include_token_scopes while the runtime uses
config.MCPOAuthConfiguration.ScopeChallengeMode; add a new top-level property
"scope_challenge_mode" (string or enum matching the MCPOAuthConfiguration enum
values used at runtime) to the mcp.oauth section of
router/pkg/config/config.schema.json so config files and docs can set the new
mode, and retain "scope_challenge_include_token_scopes" as a deprecated boolean
alias that maps to the equivalent mode value for backwards compatibility; ensure
the new property description documents the enum values and that the boolean
field is marked deprecated in its description.
In `@router/pkg/mcpserver/auth_middleware.go`:
- Around line 62-85: The constructor NewMCPAuthMiddleware should only require a
non-nil tokenDecoder when auth is enabled; change the nil-check so it returns an
error only if enabled is true and tokenDecoder == nil, and only call
authentication.NewHttpHeaderAuthenticator (and propagate its error) when enabled
is true; when enabled is false set authenticator to nil and proceed to return
the MCPAuthMiddleware with enabled, resourceMetadataURL, scopes and
scopeChallengeIncludeTokenScopes as before.
In `@router/pkg/mcpserver/scope_challenge.md`:
- Around line 13-15: The fenced code blocks in scope_challenge.md (e.g., the
example showing [["a", "b"], ["c", "d"]] → (a AND b) OR (c AND d) and the other
examples at lines 42-44, 55-57, 67-74) are missing language tags; update each
triple-backtick fence to include a language identifier such as text (e.g.,
change ``` to ```text) so markdownlint stops flagging them and the docs check
becomes quiet.
In `@router/pkg/mcpserver/server.go`:
- Around line 441-449: The new mcp.NewStreamableHTTPHandler call uses a
getServer closure that returns s.server without running the request header
extraction, so requestHeadersFromRequest is never invoked and headersFromContext
cannot find the caller headers; to fix, ensure the requestHeadersFromRequest
hook runs for the incoming /mcp requests before returning the server — e.g., in
the getServer function that currently returns s.server, call the same
requestHeadersFromRequest(req.Context() or req) logic (the routine that
populates headers into the request context) and return s.server with the
enriched context so headersFromContext can read the forwarded headers; update
the closure passed to mcp.NewStreamableHTTPHandler accordingly while keeping
s.server as the returned server instance.
---
Outside diff comments:
In `@router/pkg/schemaloader/loader.go`:
- Around line 35-43: OperationLoader currently stores a nullable Logger and
LoadOperationsFromDirectory calls l.Logger.Error(...) without guarding for nil;
update NewOperationLoader to ensure Logger is non-nil by defaulting to a no-op
logger when logger == nil, or add nil-checks before every l.Logger.Error use in
LoadOperationsFromDirectory; specifically modify NewOperationLoader to replace a
nil logger with zap.NewNop() (or equivalent) so OperationLoader.Logger is always
safe to call, and confirm LoadOperationsFromDirectory uses the Logger field
(l.Logger.Error) without additional nil checks.
---
Duplicate comments:
In `@router-tests/cmd/oauth-server/main.go`:
- Around line 128-132: The auth code is not tied to a redirect URI—update the
authCode type to include the selected redirect URI, ensure the client
representation (client struct) contains its allowed redirect URIs, validate the
incoming redirect_uri in the /authorize handler against the client's allowed
list and store the chosen redirect_uri on the authCode, then in the /token
handler require an exact match between the presented redirect_uri and the
redirect_uri stored on the authCode (and that it is an allowed URI for the
client) before issuing tokens.
In `@router-tests/mcp_auth_e2e_test.go`:
- Around line 193-335: The tests in TestMCPAuthorizationWithOfficialSDK are not
exercising real auth because the test env only sets MCP.Enabled and never
configures MCP.OAuth, JWKS, or required scopes; update the test cases to supply
a real/fixture OAuth configuration (populate MCP.OAuth/JWKS and the server’s
required scopes) so JWT validation and scope checks run, then make deterministic
assertions (do not accept both success and failure) around CallTool responses;
specifically adjust the testenv.Config passed into each t.Run, and ensure the
Scope upgrade subtest uses NewMCPAuthClient, SetToken and CallTool to trigger an
actual insufficient-scope error before calling SetToken and asserting the write
succeeds with the upgraded token.
- Around line 29-47: The authRoundTripper fields token and lastResponse are
accessed concurrently; add a mutex (e.g., sync.Mutex or sync.RWMutex) to
authRoundTripper and guard all reads/writes to token and lastResponse in
RoundTrip, SetToken, and checkAuthError: acquire the lock to read token into a
local variable, release it before calling a.base.RoundTrip, then after receiving
resp reacquire the lock to set lastResponse; likewise protect SetToken and
checkAuthError so they lock when mutating or reading those fields. Apply the
same mutex-guard pattern to the other authRoundTripper usages mentioned (the
other ranges) to eliminate data races.
In `@router-tests/testutil/oauth_server.go`:
- Around line 200-225: The handler handleClientCredentials currently
authenticates the client but never verifies that the client is allowed to use
the "client_credentials" grant; update handleClientCredentials to check the
registered client.GrantTypes (from the clients map) and return a token error
(invalid_grant or unauthorized) if "client_credentials" is not present before
calling issueTokenResponse; likewise update the authorization code path (e.g.,
handleAuthCode / the branch in 227-274) to require "authorization_code" in
client.GrantTypes before issuing tokens. Use the existing client lookup
(s.clients[clientID]) and tokenError response pattern to reject disallowed grant
types.
- Around line 23-36: The test OAuth helper advertises PKCE S256 and redirect URI
binding but doesn't implement them: extend OAuthClient to include RedirectURIs
(e.g., RedirectURIs []string), update authCode to store redirectURI,
codeChallenge, and codeChallengeMethod, make the /authorize handler validate the
provided redirect_uri against the client's RedirectURIs and store the chosen
redirectURI plus any code_challenge/code_challenge_method on the created
authCode, and update the /token exchange handler to verify the incoming
redirect_uri matches the stored authCode.redirectURI and to validate the
code_verifier when codeChallengeMethod == "S256" (compute S256 of verifier and
compare to stored codeChallenge); reference OAuthClient, authCode, the authorize
handler, and the token exchange handler when making these changes.
In `@router/pkg/config/config.go`:
- Around line 1062-1095: MCPOAuthConfiguration's Scopes field isn't receiving
env vars because MCPOAuthScopesConfiguration lacks an envPrefix and its slice
fields have no env tags; update the Scopes field on type MCPOAuthConfiguration
to include struct tag envPrefix:"SCOPES_" and add env:"<NAME>" tags to each
field in MCPOAuthScopesConfiguration (e.g. Initialize -> env:"INITIALIZE",
ToolsList -> env:"TOOLS_LIST", ToolsCall -> env:"TOOLS_CALL", ExecuteGraphQL ->
env:"EXECUTE_GRAPHQL", GetOperationInfo -> env:"GET_OPERATION_INFO", GetSchema
-> env:"GET_SCHEMA") so MCP_OAUTH_SCOPES_* environment variables are parsed by
env.Parse(&cfg.Config).
In `@router/pkg/config/config.schema.json`:
- Around line 2650-2668: The schema should require a non-empty oauth.jwks array
when oauth.enabled is true: update the existing if/then block so the "then" also
requires "oauth": { "properties": { "jwks": { "type":"array", "minItems":1,
"items": { "type":"object", "required":["secret","algorithm","kid"],
"properties": { "secret":{"type":"string","minLength":1},
"algorithm":{"type":"string","minLength":1},
"kid":{"type":"string","minLength":1} } } } } } in addition to the existing
server.base_url requirement so mcp.oauth.enabled:true fails schema validation
unless at least one JWKS provider with non-empty secret, algorithm and kid is
provided.
In `@router/pkg/mcpserver/auth_middleware.go`:
- Around line 197-206: The code currently treats body read/size failures as auth
failures by calling m.sendUnauthorizedResponse; instead return a
payload/bad-request error (HTTP 400) without an auth challenge. Replace the two
calls to m.sendUnauthorizedResponse in the r.Body handling block (after
io.ReadAll and after the size check) with a call that returns a 400-level
payload error (e.g. m.sendBadRequestResponse or m.sendPayloadErrorResponse) that
includes the underlying error details (wrap the err from io.ReadAll in the
log/error message and include a clear "request body too large" message for the
size check) so clients get a proper JSON-RPC/malformed-payload response rather
than a WWW-Authenticate challenge.
In `@router/pkg/mcpserver/scope_extractor.go`:
- Around line 74-85: ComputeCombinedScopes eagerly builds the full cross-product
of OR-groups via crossProduct which can explode; add a hard cap or lazy
short-circuit to avoid materializing too many combinations. Introduce a
MAX_SCOPE_COMBINATIONS constant and change crossProduct and
ComputeCombinedScopes to accept a maxLimit (or return an error/indicator) so
that crossProduct can count combinations and early-return once the limit is
exceeded; alternatively implement an iterator/generator API that yields
combinations and lets callers stop early. Update callers of
ComputeCombinedScopes/crossProduct (and FieldScopeRequirement handling) to check
for the limit-exceeded signal and handle it (e.g., fall back to coarse-grained
auth or return an explicit error). Ensure function signatures mention the new
limit/indicator so behavior is explicit.
In `@router/pkg/mcpserver/server.go`:
- Around line 779-787: The current handleOperation logging emits raw PII ("sub"
and "email") via getClaimString(GetClaimsFromContext), so update the logging in
GraphQLSchemaServer.handleOperation (and similarly where repeated around
handler.operation.Name) to avoid PII: instead derive and log a non-reversible
correlation key (e.g., hash the "sub" claim) or omit both claims entirely, and
log only the correlation key plus handler.operation.Name and any non-sensitive
context; use GetClaimsFromContext and getClaimString to read claims but do the
hashing/omission before calling s.logger.Debug.
- Around line 217-276: When OAuth is enabled, require
options.OAuthConfig.AuthorizationServerURL to be set to avoid passing a dead
discovery URL to NewMCPAuthMiddleware; add a validation like the existing
ServerBaseURL check that cancels context and returns an error if
AuthorizationServerURL is empty, and then use AuthorizationServerURL (or ensure
consistency) when building resourceMetadataURL before calling
NewMCPAuthMiddleware (references: options.OAuthConfig.AuthorizationServerURL,
options.ServerBaseURL, resourceMetadataURL, NewMCPAuthMiddleware).
- Around line 1055-1117: The metadata currently sets
ProtectedResourceMetadata.Resource to the base resourceURL, but the MCP metadata
is path-scoped to /mcp; update the assignment so Resource advertises the full
MCP path (e.g., append "/mcp" to resourceURL while avoiding double slashes)
before constructing metadata. Locate the resourceURL variable and the metadata
:= ProtectedResourceMetadata{... Resource: ... } block and replace Resource:
resourceURL with Resource: fmt.Sprintf("%s/mcp", strings.TrimRight(resourceURL,
"/")) (or equivalent) so Resource and ResourceDocumentation consistently point
to the MCP endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a08866d-6d88-4655-8384-194c81bd1c64
⛔ Files ignored due to path filters (3)
demo/go.sumis excluded by!**/*.sumrouter-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
demo/go.modrouter-tests/cmd/oauth-server/main.gorouter-tests/go.modrouter-tests/mcp_auth_e2e_test.gorouter-tests/mcp_auth_harness_example.gorouter-tests/mcp_oauth_e2e_test.gorouter-tests/mcp_test.gorouter-tests/testenv/testenv.gorouter-tests/testutil/auth_helpers.gorouter-tests/testutil/jwt_helper.gorouter-tests/testutil/oauth_server.gorouter-tests/testutil/oauth_server_test.gorouter/core/graph_server.gorouter/core/router.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/mcpserver/auth_middleware.gorouter/pkg/mcpserver/auth_middleware_test.gorouter/pkg/mcpserver/errors.gorouter/pkg/mcpserver/execute_graphql_scope_test.gorouter/pkg/mcpserver/operation_manager.gorouter/pkg/mcpserver/schema_compiler.gorouter/pkg/mcpserver/scope_challenge.gorouter/pkg/mcpserver/scope_challenge.mdrouter/pkg/mcpserver/scope_challenge_test.gorouter/pkg/mcpserver/scope_extractor.gorouter/pkg/mcpserver/scope_extractor_test.gorouter/pkg/mcpserver/server.gorouter/pkg/schemaloader/loader.gorouter/pkg/schemaloader/loader_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- router/core/graph_server.go
- router-tests/testutil/auth_helpers.go
- router/pkg/mcpserver/execute_graphql_scope_test.go
- router-tests/testenv/testenv.go
- router/pkg/config/testdata/config_full.json
- demo/go.mod
- router/pkg/mcpserver/schema_compiler.go
- router/core/router.go
- router/pkg/config/testdata/config_defaults.json
- router-tests/mcp_auth_harness_example.go
- router/pkg/mcpserver/operation_manager.go
- router/pkg/schemaloader/loader_test.go
- router/pkg/mcpserver/scope_challenge.go
asoorm
force-pushed
the
ahmet/mcp-per-tool-scope-extraction
branch
from
4133ddc to
7c0d352
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
router/pkg/mcpserver/scope_extractor.go (1)
72-89:⚠️ Potential issue | 🟠 MajorAvoid materializing the full scope product on the auth path.
The
MAX_OR_SCOPESnote only caps each field; it does not cap the combined16^Nsearch space across an operation. A singleexecute_graphqlrequest with many scoped fields can still drive exponential CPU/memory work before scope evaluation finishes. Please cap combinations or switch this path to lazy satisfaction/challenge evaluation.Also applies to: 131-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/scope_extractor.go` around lines 72 - 89, ComputeCombinedScopes currently materializes the full Cartesian product of per-field OrScopes (via crossProduct), which can explode to 16^N and cause OOM/CPU spikes on the auth path; change this to avoid full materialization by capping combinations or performing lazy evaluation: modify ComputeCombinedScopes (and crossProduct) to either return an iterator/stream or accept a callback that evaluates/checks each combination on-the-fly and aborts when a global cap (configurable maxCombinedScopes) is reached, or implement early pruning so you never build more than maxCombinedScopes combined scopes; ensure FieldScopeRequirement.OrScopes is consumed lazily and that callers of ComputeCombinedScopes are updated to handle the new iterator/callback behavior and the configurable cap.router/pkg/mcpserver/auth_middleware.go (2)
168-177:⚠️ Potential issue | 🟠 MajorReturn payload errors here, not auth challenges.
failed to read request bodyandrequest body too largeare malformed/payload-limit failures, not authentication failures. Returning401plusWWW-Authenticatehere will send clients down a re-auth flow instead of surfacing400/413.♻️ Suggested change
body, err = io.ReadAll(limitedReader) if err != nil { - m.sendUnauthorizedResponse(w, fmt.Errorf("failed to read request body")) + http.Error(w, "failed to read request body", http.StatusBadRequest) return } if int64(len(body)) > maxBodyBytes { - m.sendUnauthorizedResponse(w, fmt.Errorf("request body too large")) + http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/auth_middleware.go` around lines 168 - 177, The code in the auth middleware currently treats payload read errors as authentication failures by calling m.sendUnauthorizedResponse; instead, change the handling in the block that reads the POST body (where r.Method, maxBodyBytes and body are used) to return proper payload errors: on io.ReadAll error return a 400 Bad Request (or use an existing m.sendBadRequest/m.sendError function) and when len(body) > maxBodyBytes return a 413 Payload Too Large, ensuring you do not set WWW-Authenticate or a 401 status for these cases; update calls to use the appropriate response helper (or write the correct status/message) rather than m.sendUnauthorizedResponse.
62-85:⚠️ Potential issue | 🟠 MajorOnly require a decoder when MCP auth is enabled.
This still rejects
nilunconditionally and always builds an authenticator, soenabled=falsesetups fail unless callers inject a dummy decoder.♻️ Suggested change
func NewMCPAuthMiddleware(tokenDecoder authentication.TokenDecoder, enabled bool, resourceMetadataURL string, scopes MCPScopeConfig, scopeChallengeIncludeTokenScopes bool) (*MCPAuthMiddleware, error) { - if tokenDecoder == nil { - return nil, fmt.Errorf("token decoder must be provided") - } - - authenticator, err := authentication.NewHttpHeaderAuthenticator(authentication.HttpHeaderAuthenticatorOptions{ - Name: "mcp-auth", - TokenDecoder: tokenDecoder, - }) - if err != nil { - return nil, fmt.Errorf("failed to create authenticator: %w", err) - } + var authenticator authentication.Authenticator + if enabled { + if tokenDecoder == nil { + return nil, fmt.Errorf("token decoder must be provided when MCP auth is enabled") + } + + var err error + authenticator, err = authentication.NewHttpHeaderAuthenticator(authentication.HttpHeaderAuthenticatorOptions{ + Name: "mcp-auth", + TokenDecoder: tokenDecoder, + }) + if err != nil { + return nil, fmt.Errorf("failed to create authenticator: %w", err) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/auth_middleware.go` around lines 62 - 85, NewMCPAuthMiddleware currently rejects a nil tokenDecoder and always builds an authenticator even when enabled is false; change the logic so the function only requires tokenDecoder and constructs authentication.NewHttpHeaderAuthenticator when enabled is true — i.e., if enabled && tokenDecoder == nil return an error, and only call authentication.NewHttpHeaderAuthenticator (and assign to MCPAuthMiddleware.authenticator) when enabled is true; when disabled return a middleware with authenticator left nil and other fields set as before (refer to NewMCPAuthMiddleware, tokenDecoder, enabled, and authenticator).router-tests/mcp_oauth_e2e_test.go (3)
51-65:⚠️ Potential issue | 🟠 MajorAssert the pre-upgrade
execute_graphqldenial beforeSetToken().Right now this only proves mid-session token rotation. Add a
readOnlyTokencall toexecute_graphqland require403 insufficient_scopebefore switching tonewToken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/mcp_oauth_e2e_test.go` around lines 51 - 65, The test is missing an assertion that the pre-upgrade (read-only) token is denied access to "execute_graphql" before you call client.SetToken(newToken); add a CallTool invocation using the existing readOnlyToken (or whatever variable holds the initial read-only token) to call "execute_graphql" and assert it fails with a 403/insufficient_scope error (use require.Error and assert the error message/status contains "403" or "insufficient_scope"), then continue with oauthServer.CreateTokenWithScopes(...), client.SetToken(newToken), and the existing successful CallTool assertion; reference the client.CallTool, client.SetToken, and oauthServer.CreateTokenWithScopes symbols when making the change.
108-113:⚠️ Potential issue | 🟡 MinorMake
AuthErrorpart of the assertion.These branches silently skip the status/resource-metadata checks when the error is not
*AuthError, so a transport/setup failure can still pass.Also applies to: 146-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/mcp_oauth_e2e_test.go` around lines 108 - 113, The test currently only checks status and ResourceMetadataURL when the error type assertion to *AuthError succeeds, allowing non-AuthError failures to slip through; change the flow to explicitly assert the error is an *AuthError before inspecting fields by replacing the conditional type-check (authErr, ok := err.(*AuthError); if ok { ... }) with a hard test assertion (e.g., require.IsType(t, &AuthError{}, err) or require.True(t, ok, "expected *AuthError, got %T", err)) and then type-assert to access authErr and assert its StatusCode and ResourceMetadataURL; make the same replacement for the other occurrence that checks AuthError (lines referencing the same pattern).
194-198:⚠️ Potential issue | 🟡 MinorLock this subtest to the exact insufficient-scope result.
This request uses a valid JWT and only misses
mcp:connect, so it should be*AuthErrorwith403 insufficient_scope. Allowing any error type and either401or403makes the failure contract too loose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/mcp_oauth_e2e_test.go` around lines 194 - 198, The test currently allows either a 401 or 403 and non-AuthError types; tighten it to require the specific insufficient-scope result by asserting the error is of type *AuthError (authErr), that authErr.StatusCode == http.StatusForbidden, and that the returned error message or body includes the string "insufficient_scope" (or equivalent field) so the subtest only passes for the exact insufficient-scope 403 outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/mcp_oauth_e2e_test.go`:
- Around line 175-178: The test config sets ToolsCall to only "mcp:tools:write",
which preempts the built-in read-vs-write tool scope checks in
auth_middleware.go and prevents the get_schema subtests from exercising
read-only behavior; update the test Scopes (MCPOAuthScopesConfiguration) so
ToolsCall includes the read scope (e.g., add "mcp:tools:read") or change the
specific subtest scopes to use a read-capable ToolsCall for the get_schema cases
so the method gate in auth_middleware.go sees the intended read scope rather
than being masked by write-only.
---
Duplicate comments:
In `@router-tests/mcp_oauth_e2e_test.go`:
- Around line 51-65: The test is missing an assertion that the pre-upgrade
(read-only) token is denied access to "execute_graphql" before you call
client.SetToken(newToken); add a CallTool invocation using the existing
readOnlyToken (or whatever variable holds the initial read-only token) to call
"execute_graphql" and assert it fails with a 403/insufficient_scope error (use
require.Error and assert the error message/status contains "403" or
"insufficient_scope"), then continue with
oauthServer.CreateTokenWithScopes(...), client.SetToken(newToken), and the
existing successful CallTool assertion; reference the client.CallTool,
client.SetToken, and oauthServer.CreateTokenWithScopes symbols when making the
change.
- Around line 108-113: The test currently only checks status and
ResourceMetadataURL when the error type assertion to *AuthError succeeds,
allowing non-AuthError failures to slip through; change the flow to explicitly
assert the error is an *AuthError before inspecting fields by replacing the
conditional type-check (authErr, ok := err.(*AuthError); if ok { ... }) with a
hard test assertion (e.g., require.IsType(t, &AuthError{}, err) or
require.True(t, ok, "expected *AuthError, got %T", err)) and then type-assert to
access authErr and assert its StatusCode and ResourceMetadataURL; make the same
replacement for the other occurrence that checks AuthError (lines referencing
the same pattern).
- Around line 194-198: The test currently allows either a 401 or 403 and
non-AuthError types; tighten it to require the specific insufficient-scope
result by asserting the error is of type *AuthError (authErr), that
authErr.StatusCode == http.StatusForbidden, and that the returned error message
or body includes the string "insufficient_scope" (or equivalent field) so the
subtest only passes for the exact insufficient-scope 403 outcome.
In `@router/pkg/mcpserver/auth_middleware.go`:
- Around line 168-177: The code in the auth middleware currently treats payload
read errors as authentication failures by calling m.sendUnauthorizedResponse;
instead, change the handling in the block that reads the POST body (where
r.Method, maxBodyBytes and body are used) to return proper payload errors: on
io.ReadAll error return a 400 Bad Request (or use an existing
m.sendBadRequest/m.sendError function) and when len(body) > maxBodyBytes return
a 413 Payload Too Large, ensuring you do not set WWW-Authenticate or a 401
status for these cases; update calls to use the appropriate response helper (or
write the correct status/message) rather than m.sendUnauthorizedResponse.
- Around line 62-85: NewMCPAuthMiddleware currently rejects a nil tokenDecoder
and always builds an authenticator even when enabled is false; change the logic
so the function only requires tokenDecoder and constructs
authentication.NewHttpHeaderAuthenticator when enabled is true — i.e., if
enabled && tokenDecoder == nil return an error, and only call
authentication.NewHttpHeaderAuthenticator (and assign to
MCPAuthMiddleware.authenticator) when enabled is true; when disabled return a
middleware with authenticator left nil and other fields set as before (refer to
NewMCPAuthMiddleware, tokenDecoder, enabled, and authenticator).
In `@router/pkg/mcpserver/scope_extractor.go`:
- Around line 72-89: ComputeCombinedScopes currently materializes the full
Cartesian product of per-field OrScopes (via crossProduct), which can explode to
16^N and cause OOM/CPU spikes on the auth path; change this to avoid full
materialization by capping combinations or performing lazy evaluation: modify
ComputeCombinedScopes (and crossProduct) to either return an iterator/stream or
accept a callback that evaluates/checks each combination on-the-fly and aborts
when a global cap (configurable maxCombinedScopes) is reached, or implement
early pruning so you never build more than maxCombinedScopes combined scopes;
ensure FieldScopeRequirement.OrScopes is consumed lazily and that callers of
ComputeCombinedScopes are updated to handle the new iterator/callback behavior
and the configurable cap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ab644e4-91e8-4078-8fac-88424f8c9781
📒 Files selected for processing (4)
router-tests/mcp_oauth_e2e_test.gorouter/pkg/mcpserver/auth_middleware.gorouter/pkg/mcpserver/scope_extractor.gorouter/pkg/mcpserver/server_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
router-tests/testutil/oauth_server.go (3)
324-331:⚠️ Potential issue | 🟠 MajorEncode the redirect callback instead of concatenating strings.
Line 326 drops existing query parameters on
redirect_uri, and Line 328 appends rawstate. That will still corrupt otherwise valid callbacks in browser/E2E flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/testutil/oauth_server.go` around lines 324 - 331, The redirect construction currently concatenates strings (using redirectURI, code, and state) which drops existing query params and risks invalid URLs; fix by parsing redirectURI with url.Parse, get its query via u.Query(), set "code"=code and if state!="" set "state"=state, then encode the query back with u.RawQuery = q.Encode() and use u.String() as the location passed to http.Redirect; update the code that computes location (where redirectURI, code, state are used) to use this URL-safe approach.
31-37:⚠️ Potential issue | 🟠 MajorThe auth-code flow still advertises PKCE without enforcing it.
authCodenow storesredirectURI, but the exchange path never checksredirect_uriand still never validates acode_verifieragainst a stored challenge even though discovery continues to advertiseS256. The embedded AS is still more permissive than the flow the E2E tests are supposed to validate.Also applies to: 160-176, 228-275, 315-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/testutil/oauth_server.go` around lines 31 - 37, The auth-code flow advertises PKCE but does not enforce it; update the authCode struct to store the PKCE code_challenge and challenge_method (in addition to redirectURI already added), then in the token/exchange handler validate that the presented redirect_uri matches authCode.redirectURI and, if a code_challenge is present or the discovery advertises S256, require a code_verifier and verify it against the stored code_challenge (compute S256 when challenge_method == "S256" and compare). Ensure these checks occur in the exchange path that redeems authCode instances so the server rejects mismatches or missing verifiers.
201-225:⚠️ Potential issue | 🟠 MajorHonor registered
grant_typesbefore issuing tokens.
GrantTypesis persisted on the client, but neither token handler checks it. A client registered for only one flow can still use the other one, so these tests are not exercising the same client-registration constraints a real AS would.Also applies to: 228-275, 341-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/testutil/oauth_server.go` around lines 201 - 225, The handlers currently ignore the client's registered GrantTypes so a client allowed only for one flow can use others; update OAuthTestServer.handleClientCredentials and the password-grant handler (e.g., OAuthTestServer.handlePasswordGrant) to check the client's GrantTypes slice contains "client_credentials" or "password" respectively before issuing a token; if the grant type is not permitted, call s.tokenError(w, "unauthorized_client", "grant type not allowed for client", http.StatusUnauthorized) instead of issuing a token (use the existing s.mu.RLock lookup of client and then s.issueTokenResponse when permitted).router-tests/cmd/oauth-server/main.go (1)
329-332:⚠️ Potential issue | 🟠 MajorBuild the callback URL with
url.URL, not string concatenation.Line 329 breaks as soon as
redirect_urialready has query parameters, and Line 331 appends rawstatewithout escaping. Valid OAuth clients will get malformed callbacks.🔧 Minimal fix
- location := fmt.Sprintf("%s?code=%s", redirectURI, code) - if state != "" { - location += "&state=" + state - } + u, err := url.Parse(redirectURI) + if err != nil { + http.Error(w, "invalid redirect_uri", http.StatusBadRequest) + return + } + q := u.Query() + q.Set("code", code) + if state != "" { + q.Set("state", state) + } + u.RawQuery = q.Encode() + location := u.String()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/cmd/oauth-server/main.go` around lines 329 - 332, The callback URL is built by naive string concatenation (using redirectURI, code, state -> location) which breaks if redirectURI already contains query params and does not escape state; change to parse redirectURI with net/url (url.Parse or url.URL), add/merge query parameters using url.Values (set "code" and, if non-empty, "state"), then reconstruct the final location via url.String() so values are properly encoded and existing queries preserved.
🧹 Nitpick comments (1)
router-tests/cmd/oauth-server/main.go (1)
140-180: Extract the shared OAuth server core into one package.This constructor introduces a second hand-maintained implementation of the same OAuth server already living in
router-tests/testutil/oauth_server.go. The two have already drifted on request parsing, auth-code state, and client modeling, so local repros and E2E behavior will keep diverging unless the core logic is shared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/cmd/oauth-server/main.go` around lines 140 - 180, The newOAuthServer function duplicates OAuth server logic already implemented in router-tests/testutil/oauth_server.go; extract the shared core (request parsing, auth-code state, client modeling, JWKS handling) into a single reusable package and have newOAuthServer and any other test servers reuse it. Concretely, move the oauthHandler type and its methods (handleJWKS, handleASMetadata, handleToken, handleRegister, handleAuthorize), the serverWithHandler wrapper, JWKS setup (jwks.NewRSACrypto, jwkset.NewMemoryStorage, MarshalJWK, KeyWrite) and the withCORS helper into a common package (e.g., testutil/oauthserver), then update newOAuthServer to construct and wire that shared oauthserver API (passing port, clientID, clientSecret, defaultScopes) instead of reimplementing the handler logic so both places reference the same types and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/cmd/oauth-server/main.go`:
- Around line 122-127: The client struct and token issuance logic must persist
and enforce allowed grant types: add a grant_types []string (or map) field to
the client type and populate it in handleRegister when parsing incoming
registration payloads; then in the token issuance paths (the functions/branches
that handle grant_type requests in the token endpoint) check that the requested
grant_type is present in the stored client.grant_types before minting a token
and return an error if not allowed. Update any helper functions that load or
validate clients to include grant_types so that authorization_code-only clients
cannot receive client_credentials tokens.
- Around line 129-133: The metadata advertises PKCE (S256) but the auth-code
flow does not bind or validate PKCE or redirect URIs; update the authCode type
and exchange logic so codes retain the original code_challenge and redirect_uri,
and make handleCodeExchange validate the incoming code_verifier (against stored
code_challenge using S256) and the redirect_uri before issuing tokens; also
ensure the server metadata (where S256 is advertised) is only published if the
code creation path stores a code_challenge and exchange enforces verification,
and add clear error returns in handleCodeExchange for missing/invalid
code_verifier or mismatched redirect_uri to block unbound exchanges.
In `@router-tests/mcp_auth_e2e_test.go`:
- Around line 91-97: The Connect method currently wraps any connect error and
discards WWW-Authenticate details; update MCPAuthClient.Connect to call
checkAuthError(ctx, err) (the same helper used in CallTool) when
c.client.Connect returns an error, and if checkAuthError returns a non-nil error
return that instead of the generic fmt.Errorf; otherwise fall back to the
existing wrapped error; use the existing symbols c.client.Connect,
checkAuthError, authRoundTripper and CallTool as references to locate where to
add this conditional handling.
---
Duplicate comments:
In `@router-tests/cmd/oauth-server/main.go`:
- Around line 329-332: The callback URL is built by naive string concatenation
(using redirectURI, code, state -> location) which breaks if redirectURI already
contains query params and does not escape state; change to parse redirectURI
with net/url (url.Parse or url.URL), add/merge query parameters using url.Values
(set "code" and, if non-empty, "state"), then reconstruct the final location via
url.String() so values are properly encoded and existing queries preserved.
In `@router-tests/testutil/oauth_server.go`:
- Around line 324-331: The redirect construction currently concatenates strings
(using redirectURI, code, and state) which drops existing query params and risks
invalid URLs; fix by parsing redirectURI with url.Parse, get its query via
u.Query(), set "code"=code and if state!="" set "state"=state, then encode the
query back with u.RawQuery = q.Encode() and use u.String() as the location
passed to http.Redirect; update the code that computes location (where
redirectURI, code, state are used) to use this URL-safe approach.
- Around line 31-37: The auth-code flow advertises PKCE but does not enforce it;
update the authCode struct to store the PKCE code_challenge and challenge_method
(in addition to redirectURI already added), then in the token/exchange handler
validate that the presented redirect_uri matches authCode.redirectURI and, if a
code_challenge is present or the discovery advertises S256, require a
code_verifier and verify it against the stored code_challenge (compute S256 when
challenge_method == "S256" and compare). Ensure these checks occur in the
exchange path that redeems authCode instances so the server rejects mismatches
or missing verifiers.
- Around line 201-225: The handlers currently ignore the client's registered
GrantTypes so a client allowed only for one flow can use others; update
OAuthTestServer.handleClientCredentials and the password-grant handler (e.g.,
OAuthTestServer.handlePasswordGrant) to check the client's GrantTypes slice
contains "client_credentials" or "password" respectively before issuing a token;
if the grant type is not permitted, call s.tokenError(w, "unauthorized_client",
"grant type not allowed for client", http.StatusUnauthorized) instead of issuing
a token (use the existing s.mu.RLock lookup of client and then
s.issueTokenResponse when permitted).
---
Nitpick comments:
In `@router-tests/cmd/oauth-server/main.go`:
- Around line 140-180: The newOAuthServer function duplicates OAuth server logic
already implemented in router-tests/testutil/oauth_server.go; extract the shared
core (request parsing, auth-code state, client modeling, JWKS handling) into a
single reusable package and have newOAuthServer and any other test servers reuse
it. Concretely, move the oauthHandler type and its methods (handleJWKS,
handleASMetadata, handleToken, handleRegister, handleAuthorize), the
serverWithHandler wrapper, JWKS setup (jwks.NewRSACrypto,
jwkset.NewMemoryStorage, MarshalJWK, KeyWrite) and the withCORS helper into a
common package (e.g., testutil/oauthserver), then update newOAuthServer to
construct and wire that shared oauthserver API (passing port, clientID,
clientSecret, defaultScopes) instead of reimplementing the handler logic so both
places reference the same types and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61fcb640-8439-465b-84e2-1bc20ca48c67
📒 Files selected for processing (3)
router-tests/cmd/oauth-server/main.gorouter-tests/mcp_auth_e2e_test.gorouter-tests/testutil/oauth_server.go
asoorm
force-pushed
the
ahmet/mcp-per-tool-scope-extraction
branch
2 times, most recently
from
dd42f2a to
abbc389
Compare
asoorm
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2636 +/- ##
==========================================
+ Coverage 64.45% 65.65% +1.20%
==========================================
Files 311 254 -57
Lines 44295 26426 -17869
Branches 4764 0 -4764
==========================================
- Hits 28551 17351 -11200
+ Misses 15721 7681 -8040
- Partials 23 1394 +1371
🚀 New features to boost your workflow:
|
asoorm
force-pushed
the
ahmet/mcp-per-tool-scope-extraction
branch
3 times, most recently
from
423246c to
f7cd469
Compare
… and configurable scope challenge modes
- Fix server_test.go Reload() calls to match new signature (second arg) - Sanitize error_description quotes in WWW-Authenticate headers (RFC 6750) - Skip body parsing for non-POST requests in auth middleware - Remove unused authenticateRequest method and contains wrapper - Fix ScopeChallengeMode → ScopeChallengeIncludeTokenScopes in e2e test
- Validate redirect_uri against registered URIs in both OAuth test servers - Remove scaffold tests (TestMCPAuthorizationWithOfficialSDK) that had no real auth - Remove unused mcp_auth_harness_example.go - Keep MCPAuthClient helpers used by real OAuth e2e tests
…ings - Remove unused JWKSTestServer and redundant MCPScopeConfig struct - Remove enabled field from middleware (caller gates creation) - Extract token scopes once per request with set-based O(1) lookups - Consolidate scope challenge response methods into shared helper - Increase auth middleware body limit from 1MB to 10MB - Remove S256 PKCE claim from test OAuth server (not implemented) - Add env tags for OAuth scope configuration fields - Require JWKS in JSON schema when OAuth enabled - Fix refresh_unknown_kid schema defaults to match Go (max_wait 2m, interval 30s) - Always reset RequiredScopes in ComputeToolScopes to clear stale state - Fix RFC 9728 resource URL to include /mcp path - Make resource_documentation configurable at MCP level
Moves the MCP debug proxy from client-tests/mcp-ts/mcp-debug-proxy.mjs to router-tests/cmd/mcp-debug-proxy, alongside the test OAuth server and JWKS helpers. Stdlib-only, uses httputil.ReverseProxy.
Renames client-tests/mcp-ts/mcp.test.config.yaml to router/mcp.oauth.config.yaml so it lives with the other example configs. Generalized the comments and marked the fields a reviewer needs to edit for their own setup.
Noroth
requested changes
Apr 17, 2026
Co-authored-by: Ludwig Bedacht <ludwig.bedacht@gmail.com>
Co-authored-by: Ludwig Bedacht <ludwig.bedacht@gmail.com>
Co-authored-by: Ludwig Bedacht <ludwig.bedacht@gmail.com>
Co-authored-by: Ludwig Bedacht <ludwig.bedacht@gmail.com>
endigma
approved these changes
Apr 20, 2026
Member
endigma
left a comment
endigma
left a comment
There was a problem hiding this comment.
approved, my additional comments are part of ludwig's review
Noroth
approved these changes
Apr 20, 2026
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
4 participants
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
@requiresScopesdirectives in the federated graph schemaexecute_graphql,get_schema,get_operation_info)Summary by CodeRabbit
New Features
Configuration
Improvements