fix(mcp): implement router-level logger injection for MCP auth by duwenxin99 · Pull Request #3067 · googleapis/mcp-toolbox

duwenxin99 · 2026-04-15T21:21:13Z

Any unexpected errors during token validation now explicitly deny access rather than bypassing security checks.
Moved the injection of the logger into the request context to the router level. This ensures that the logger is available to all subsequent middlewares and handlers.

gemini-code-assist

Code Review

This pull request enhances the generic authentication service by allowing custom introspection endpoints, methods, and parameter names, specifically to support Google's token validation. It also introduces a fallback for the audience claim and ensures the MCP middleware fails closed on unexpected errors. Feedback indicates a security regression due to the removal of the active claim check in the introspection response, and several instances where Go naming conventions for acronyms were not followed.

I am having trouble creating individual review comments. Click here to see my feedback.

internal/auth/generic/generic.go (346-356)

The check for the active claim in the introspection response has been removed. According to RFC 7662, the active field is REQUIRED and indicates whether the token is currently valid. Removing this check is a security regression as it allows tokens that the provider explicitly identifies as inactive (e.g., revoked or expired) to be accepted if they still contain other claims.

To support providers like Google that do not return this field, you should use a pointer to a boolean (*bool) to detect if the field was present in the JSON, and only reject the token if it is explicitly false.

	var introspectResp struct {
		Active   *bool           "json:\"active\"" // Use pointer to detect presence
		Scope    string          "json:\"scope\"" 
		Aud      json.RawMessage "json:\"aud\"" 
		Audience json.RawMessage "json:\"audience\"" 
		Exp      int64           "json:\"exp\"" 
	}

	if err := json.Unmarshal(body, &introspectResp); err != nil {
		return &MCPAuthError{Code: http.StatusInternalServerError, Message: fmt.Sprintf("failed to parse introspection response: %v", err), ScopesRequired: a.ScopesRequired}
	}

	// Verify active status if the field is present (RFC 7662 requirement)
	if introspectResp.Active != nil && !*introspectResp.Active {
		return &MCPAuthError{Code: http.StatusUnauthorized, Message: "token is not active", ScopesRequired: a.ScopesRequired}
	}

internal/auth/generic/generic.go (62)

In Go, acronyms like URL should be all-caps (e.g., introspectionURL). This maintains consistency with jwksURL on the same line and follows the project's existing style.

	jwksURL, introspectionURL, err := discoverOIDCConfig(httpClient, cfg.AuthorizationServer)

References

Go style guide recommends that acronyms should be all-caps to maintain consistency. ^(link)

internal/auth/generic/generic.go (164)

Following Go naming conventions, this field should be named introspectionURL.

	introspectionURL string

References

Go style guide recommends that acronyms should be all-caps to maintain consistency. ^(link)

averikitsch

@Yuan325 can you be final approver.

hits313 · 2026-04-30T08:35:56Z

hi @duwenxin99 and team

Thanks for landing this the fail-closed default is correct,
and both error paths from the original report are blocked.

One small suggestion before merge: the new TestMCPAuthMiddleware
covers the malformed-introspection-JSON case but not the
unreachable-introspection-endpoint case (which produced
MCPAuthError{Code: 500} in the original PoC). A short variant
with mockOIDC.Close() or a closed port would close the gap.

Happy to open a follow-up PR if helpful.

Hitarth

github-actions · 2026-05-04T03:59:41Z

🧨 Preview deployments removed.

Cloudflare Pages environments for pr-3067 have been deleted.

🤖 I have created a release *beep* *boop* --- ## [1.2.0](v1.1.0...v1.2.0) (2026-05-07) ### Features * Add support for HTTPS/TLS listener ([#3126](#3126)) ([8bc385d](8bc385d)) * **source/bigquery:** Add maximumBytesBilled source config ([#2724](#2724)) ([42f2d07](42f2d07)) * **source/cloud-storage:** Add bucket and object management tools ([#3129](#3129)) ([8de9bcf](8de9bcf)) * **source/cloud-storage:** Add Cloud Storage source with list_objects and read_object tools ([#3081](#3081)) ([da27b37](da27b37)) * **source/cloud-storage:** Add write/copy/move/delete object tools ([#3139](#3139)) ([b225fc4](b225fc4)) * **tools/knowledge-catalog:** Search Data Quality Scans ([#2444](#2444)) ([1c63551](1c63551)) ### Bug Fixes * Allow converting string literal block with list ([#3050](#3050)) ([36ab2a9](36ab2a9)), closes [#3023](#3023) * **mcp:** Implement router-level logger injection for MCP auth ([#3067](#3067)) ([ccc7cf5](ccc7cf5)) * Prevent test.db from being created during unit tests ([#3042](#3042)) ([d10d2ca](d10d2ca)) * Remove hardcoded * allowed origin for sse ([#3054](#3054)) ([c4c7bd9](c4c7bd9)) * **sources/postgres:** Apply URL encoding to query string params ([#3020](#3020)) ([6b860f4](6b860f4)) * **tool/looker-conversational-analytics:** OAuth token in GDA payload fix ([#3058](#3058)) ([6632d96](6632d96)) * **tools/bigquery-execute-sql:** Avoid surfacing invalid queries as MCP 500s ([#3056](#3056)) ([7ed92c8](7ed92c8)) * **tools/looker:** Fix OAuth for Converational Analytics ([#3044](#3044)) ([f9e3e55](f9e3e55)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com>

…eapis#3067) - Any unexpected errors during token validation now explicitly deny access rather than bypassing security checks. - Moved the injection of the logger into the request context to the router level. This ensures that the logger is available to all subsequent middlewares and handlers. Fix: googleapis#3076

🤖 I have created a release *beep* *boop* --- ## [1.2.0](googleapis/mcp-toolbox@v1.1.0...v1.2.0) (2026-05-07) ### Features * Add support for HTTPS/TLS listener ([googleapis#3126](googleapis#3126)) ([8bc385d](googleapis@8bc385d)) * **source/bigquery:** Add maximumBytesBilled source config ([googleapis#2724](googleapis#2724)) ([42f2d07](googleapis@42f2d07)) * **source/cloud-storage:** Add bucket and object management tools ([googleapis#3129](googleapis#3129)) ([8de9bcf](googleapis@8de9bcf)) * **source/cloud-storage:** Add Cloud Storage source with list_objects and read_object tools ([googleapis#3081](googleapis#3081)) ([da27b37](googleapis@da27b37)) * **source/cloud-storage:** Add write/copy/move/delete object tools ([googleapis#3139](googleapis#3139)) ([b225fc4](googleapis@b225fc4)) * **tools/knowledge-catalog:** Search Data Quality Scans ([googleapis#2444](googleapis#2444)) ([1c63551](googleapis@1c63551)) ### Bug Fixes * Allow converting string literal block with list ([googleapis#3050](googleapis#3050)) ([36ab2a9](googleapis@36ab2a9)), closes [googleapis#3023](googleapis#3023) * **mcp:** Implement router-level logger injection for MCP auth ([googleapis#3067](googleapis#3067)) ([ccc7cf5](googleapis@ccc7cf5)) * Prevent test.db from being created during unit tests ([googleapis#3042](googleapis#3042)) ([d10d2ca](googleapis@d10d2ca)) * Remove hardcoded * allowed origin for sse ([googleapis#3054](googleapis#3054)) ([c4c7bd9](googleapis@c4c7bd9)) * **sources/postgres:** Apply URL encoding to query string params ([googleapis#3020](googleapis#3020)) ([6b860f4](googleapis@6b860f4)) * **tool/looker-conversational-analytics:** OAuth token in GDA payload fix ([googleapis#3058](googleapis#3058)) ([6632d96](googleapis@6632d96)) * **tools/bigquery-execute-sql:** Avoid surfacing invalid queries as MCP 500s ([googleapis#3056](googleapis#3056)) ([7ed92c8](googleapis@7ed92c8)) * **tools/looker:** Fix OAuth for Converational Analytics ([googleapis#3044](googleapis#3044)) ([f9e3e55](googleapis@f9e3e55)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com>

duwenxin99 requested a review from a team as a code owner April 15, 2026 21:21

blunderbuss-gcf Bot assigned Yuan325 Apr 15, 2026

duwenxin99 closed this Apr 15, 2026

duwenxin99 force-pushed the fix-auth-logger branch from 57de20f to 42f2d07 Compare April 15, 2026 21:22

add mcp logger

d20b159

duwenxin99 reopened this Apr 15, 2026

gemini-code-assist Bot reviewed Apr 15, 2026

View reviewed changes

fix(auth/generic): return MCPAuthError on read failure

03deb66

duwenxin99 changed the title ~~fix(mcp): implement router-level logger injection for MCP auth~~ fix!(mcp): implement router-level logger injection for MCP auth Apr 16, 2026

duwenxin99 mentioned this pull request Apr 16, 2026

fix(auth)!: prevent fail-open bypass on generic auth errors #3078

Closed

5 tasks

averikitsch reviewed Apr 17, 2026

View reviewed changes

duwenxin99 changed the title ~~fix!(mcp): implement router-level logger injection for MCP auth~~ fix(mcp): implement router-level logger injection for MCP auth Apr 28, 2026

Yuan325 reviewed Apr 28, 2026

View reviewed changes

Comment thread internal/server/mcp.go

Comment thread internal/auth/generic/generic.go

averikitsch reviewed Apr 28, 2026

View reviewed changes

Comment thread internal/auth/generic/generic.go Outdated

averikitsch reviewed Apr 28, 2026

View reviewed changes

Comment thread internal/server/mcp.go

duwenxin99 added 3 commits April 29, 2026 10:29

fix comments

187b1b0

update test

29307d6

revert

2472ecd

Deeven-Seru reviewed Apr 30, 2026

View reviewed changes

Comment thread internal/server/server.go Outdated

Comment thread internal/server/server_test.go

remove http handler logger injection

37b78b4

Yuan325 approved these changes Apr 30, 2026

View reviewed changes

duwenxin99 and others added 4 commits April 30, 2026 12:05

add unreachable introspection test

f3550e5

return non-plain text json error

240ae36

Merge branch 'main' into fix-auth-logger

5a922c8

fix lint

58220b6

averikitsch reviewed Apr 30, 2026

View reviewed changes

Comment thread internal/server/server.go

averikitsch reviewed Apr 30, 2026

View reviewed changes

Comment thread internal/server/server.go Outdated

averikitsch approved these changes Apr 30, 2026

View reviewed changes

use custom jsonrpc auth error codes

90f0bd2

duwenxin99 and others added 2 commits May 3, 2026 23:07

Merge branch 'main' into fix-auth-logger

92ade63

update comments

6689d37

duwenxin99 enabled auto-merge (squash) May 4, 2026 03:47

duwenxin99 merged commit ccc7cf5 into main May 4, 2026
18 checks passed

duwenxin99 deleted the fix-auth-logger branch May 4, 2026 03:59

release-please Bot mentioned this pull request May 1, 2026

chore(main): release 1.2.0 #3059

Merged

BrewTestBot mentioned this pull request May 7, 2026

mcp-toolbox 1.2.0 Homebrew/homebrew-core#281497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mcp): implement router-level logger injection for MCP auth#3067

fix(mcp): implement router-level logger injection for MCP auth#3067
duwenxin99 merged 13 commits into
mainfrom
fix-auth-logger

duwenxin99 commented Apr 15, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

averikitsch left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hits313 commented Apr 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

duwenxin99 commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

internal/auth/generic/generic.go (346-356)

internal/auth/generic/generic.go (62)

internal/auth/generic/generic.go (164)

Uh oh!

averikitsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hits313 commented Apr 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

duwenxin99 commented Apr 15, 2026 •

edited

Loading