fix(ingestion): build /mcp's bearer and the metrics wrap inside buildMux (#154) #183

Merged
mathias merged 2 commits from w3/154-mcp-auth-wiring into main 2026-10-05 17:48:44 +00:00
Owner

Closes #154. Stacked on #181: the branch also contains 474473c. Review only a1e0361; merge after #181.

  • buildMux now takes the MCP handler and wraps /mcp with the same BearerMiddleware and credential as REST. "/mcp" and "GET /metrics" are off the public allowlist, so passing either as a public route is refused.
  • buildMux registers GET /metrics and returns the metrics-wrapped handler, and main passes it to ListenAndServe unchanged.
  • Tests: /mcp with no token or a wrong token gets 401, a valid token reaches the handler; a nil MCP handler or registry is refused; requests are observed through buildMux's own /metrics.
  • Flip tests: registering /mcp unwrapped turns the test red, and dropping the metrics wrap turns 4 tests red.
  • Comments now claim only what is tested. The ListenAndServe line and the symlink tripwire wiring (from the #154 comment) are still untested and tracked in #182.

CI=1 task check exits 0.

🤖 Generated with Claude Code


Created via git-mcp on behalf of @mathias

Closes #154. **Stacked on #181**: the branch also contains 474473c. Review only a1e0361; merge after #181. - `buildMux` now takes the MCP handler and wraps `/mcp` with the same `BearerMiddleware` and credential as REST. `"/mcp"` and `"GET /metrics"` are off the public allowlist, so passing either as a public route is refused. - `buildMux` registers `GET /metrics` and returns the metrics-wrapped handler, and main passes it to `ListenAndServe` unchanged. - Tests: `/mcp` with no token or a wrong token gets 401, a valid token reaches the handler; a nil MCP handler or registry is refused; requests are observed through buildMux's own `/metrics`. - Flip tests: registering `/mcp` unwrapped turns the test red, and dropping the metrics wrap turns 4 tests red. - Comments now claim only what is tested. The `ListenAndServe` line and the symlink tripwire wiring (from the #154 comment) are still untested and tracked in #182. `CI=1 task check` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _Created via git-mcp on behalf of @mathias_
Author
Owner

pr-gatekeeper verdict: BLOCK (PR #183, commit a1e0361 only, closes #154)

Mode: PR mode. Scope: commit a1e0361, diffed against 474473c. #181 is reviewed separately and is still unmerged.

Blocking findings

The code change is correct. The block is on the comments. Part 2 of #154 is closed by narrowing the comments (the "or the comments are narrowed" clause), and the narrowed comments are still wrong, so that done-when clause is not met.

  1. ingestion/cmd/server/main.go:588-591 says ListenAndServe "must serve its result unchanged: that one line is the untested part". routes.go:44 says the same thing ("What stays untested is main's own line"), and so does #182. That is not the only untested line. All of main's buildMux(restAuth{...}, ..., public) call is untested. Checked by running it: I changed main.go's validator: jwtValidator to validator: nil, and go vet and go test ./cmd/server/ both exit 0. That change drops JWT acceptance on both /mcp and REST, which breaks the claude.ai OAuth connector. Narrow the comment, or say what is actually untested: the restAuth fields and public map that main passes in, plus the ListenAndServe line. Update #182 to match.
  2. ingestion/cmd/server/routes_test.go:25-27: the testMux comment says it builds the router "exactly as main does ... and real bearer config". It passes no JWT validator and no resourceMetadataURL. Because of that, these mutations of /mcp's middleware inside buildMux leave the whole suite green:
    • validator set to nil
    • resourceMetadataURL set to ""
    • a different realm
  3. ingestion/cmd/server/routes_test.go:88-91: the TestBuildMuxDoesNotWrapPublicRoutes comment still lists "bearer on /mcp" and "/metrics" as public routes it covers. Neither is in publicRoutes after this commit, so the test no longer iterates them. The comment is stale.

Recommended (not in #154's done-when, but it is the PR's own claim)

  1. The PR says /mcp gets "the same BearerMiddleware and credential as REST". The code supports that: one shared bearer closure. The tests do not prove it. Only the static token is asserted on /mcp. Two cheap assertions would make the claim tested:

    • a JWT-shaped token on /mcp while the issuer is unresolved gets 503, as the REST test already checks for /pending
    • a 401 on /mcp carries resource_metadata in WWW-Authenticate

    Both failures I found go the fail-closed way (a broken connector, not an open door), so this is a recommendation, not a block.

What I verified by running it

  • Old vs new router, side by side. I rebuilt 474473c's wiring in a scratch test (/mcp wrapped the old way, GET /metrics as a public route, the same REST mount, reg.Middleware outside) and compared it with the new buildMux. The cases were 5 methods, 21 paths, 7 Authorization shapes, a resolved and an unresolved JWT issuer, and resourceMetadataURL set and unset: 2940 cases. Status, WWW-Authenticate, Location and body were identical in every case. The only exception was the numeric values in /metrics bodies, and its label set was identical too. With the issuer unresolved, a JWT on /mcp gets 503, and a 401 on /mcp carries resource_metadata.
  • Routes. No route gained or lost auth. /metrics is still open.
  • /metrics observing itself. It was already observed by its own middleware before this commit: it sat on the mux that main wrapped. The old comment claiming it was excluded was false. This commit doesn't change that behaviour, and it adds one bounded series.
  • WithCapture. With a real mcp.Server wrapped by buildMux, calling WithCapture afterwards made capture appear in tools/list through the router: false before the call, true after. In main, buildMux now runs after WithCapture in any case.

Flip tests (routes.go mutations, go test ./cmd/server/ exit code)

Mutation Result Tests that went red
/mcp registered without the bearer red MCPWithoutAValidToken
/mcp registered as "/mcp/" red MCPWithoutAValidToken
/mcp bearer built with an empty token red MCPWithoutAValidToken
metrics wrap dropped red 4 tests
GET /metrics not registered red 1 test
/metrics put behind the bearer red 1 test
nil check removed red 1 test
"/mcp" put back on the allowlist red panic on duplicate pattern
REST mounted without the bearer red 6 tests
/mcp validator set to nil survived none
/mcp resourceMetadataURL set to "" survived none
/mcp realm changed survived none
main.go: validator set to nil survived none

CI

Run 9400, "CI / Lint / Test / Vet (pull_request)", passed on a1e0361. CI was green, and none of the findings above are things it checks. Locally, go vet exits 0, golangci-lint run ./cmd/server/... exits 0, and the tests exit 0 with 25 passing.

Not checked

  • #181's commit 474473c.
  • Packages outside ingestion/cmd/server.
  • Live deployment behaviour.
  • ListenAndServe and the symlink tripwire wiring (by design, tracked in #182).
## pr-gatekeeper verdict: BLOCK (PR #183, commit a1e0361 only, closes #154) Mode: PR mode. Scope: commit a1e0361, diffed against 474473c. #181 is reviewed separately and is still unmerged. ### Blocking findings The code change is correct. The block is on the comments. Part 2 of #154 is closed by narrowing the comments (the "or the comments are narrowed" clause), and the narrowed comments are still wrong, so that done-when clause is not met. 1. **`ingestion/cmd/server/main.go:588-591`** says ListenAndServe "must serve its result unchanged: that one line is the untested part". **`routes.go:44`** says the same thing ("What stays untested is main's own line"), and so does #182. That is not the only untested line. All of main's `buildMux(restAuth{...}, ..., public)` call is untested. Checked by running it: I changed main.go's `validator: jwtValidator` to `validator: nil`, and `go vet` and `go test ./cmd/server/` both exit 0. That change drops JWT acceptance on both /mcp and REST, which breaks the claude.ai OAuth connector. Narrow the comment, or say what is actually untested: the restAuth fields and public map that main passes in, plus the ListenAndServe line. Update #182 to match. 2. **`ingestion/cmd/server/routes_test.go:25-27`**: the testMux comment says it builds the router "exactly as main does ... and real bearer config". It passes no JWT validator and no resourceMetadataURL. Because of that, these mutations of /mcp's middleware inside buildMux leave the whole suite green: - validator set to nil - resourceMetadataURL set to "" - a different realm 3. **`ingestion/cmd/server/routes_test.go:88-91`**: the TestBuildMuxDoesNotWrapPublicRoutes comment still lists "bearer on /mcp" and "/metrics" as public routes it covers. Neither is in publicRoutes after this commit, so the test no longer iterates them. The comment is stale. ### Recommended (not in #154's done-when, but it is the PR's own claim) 4. The PR says /mcp gets "the same BearerMiddleware and credential as REST". The code supports that: one shared `bearer` closure. The tests do not prove it. Only the static token is asserted on /mcp. Two cheap assertions would make the claim tested: - a JWT-shaped token on /mcp while the issuer is unresolved gets 503, as the REST test already checks for /pending - a 401 on /mcp carries `resource_metadata` in WWW-Authenticate Both failures I found go the fail-closed way (a broken connector, not an open door), so this is a recommendation, not a block. ### What I verified by running it - **Old vs new router, side by side.** I rebuilt 474473c's wiring in a scratch test (/mcp wrapped the old way, GET /metrics as a public route, the same REST mount, reg.Middleware outside) and compared it with the new buildMux. The cases were 5 methods, 21 paths, 7 Authorization shapes, a resolved and an unresolved JWT issuer, and resourceMetadataURL set and unset: 2940 cases. Status, WWW-Authenticate, Location and body were identical in every case. The only exception was the numeric values in /metrics bodies, and its label set was identical too. With the issuer unresolved, a JWT on /mcp gets 503, and a 401 on /mcp carries resource_metadata. - **Routes.** No route gained or lost auth. /metrics is still open. - **/metrics observing itself.** It was already observed by its own middleware before this commit: it sat on the mux that main wrapped. The old comment claiming it was excluded was false. This commit doesn't change that behaviour, and it adds one bounded series. - **WithCapture.** With a real mcp.Server wrapped by buildMux, calling WithCapture afterwards made `capture` appear in tools/list through the router: false before the call, true after. In main, buildMux now runs after WithCapture in any case. ### Flip tests (routes.go mutations, `go test ./cmd/server/` exit code) | Mutation | Result | Tests that went red | |---|---|---| | /mcp registered without the bearer | red | MCPWithoutAValidToken | | /mcp registered as "/mcp/" | red | MCPWithoutAValidToken | | /mcp bearer built with an empty token | red | MCPWithoutAValidToken | | metrics wrap dropped | red | 4 tests | | GET /metrics not registered | red | 1 test | | /metrics put behind the bearer | red | 1 test | | nil check removed | red | 1 test | | "/mcp" put back on the allowlist | red | panic on duplicate pattern | | REST mounted without the bearer | red | 6 tests | | /mcp validator set to nil | **survived** | none | | /mcp resourceMetadataURL set to "" | **survived** | none | | /mcp realm changed | **survived** | none | | main.go: validator set to nil | **survived** | none | ### CI Run 9400, "CI / Lint / Test / Vet (pull_request)", passed on a1e0361. CI was green, and none of the findings above are things it checks. Locally, `go vet` exits 0, `golangci-lint run ./cmd/server/...` exits 0, and the tests exit 0 with 25 passing. ### Not checked - #181's commit 474473c. - Packages outside ingestion/cmd/server. - Live deployment behaviour. - ListenAndServe and the symlink tripwire wiring (by design, tracked in #182).
mathias force-pushed w3/154-mcp-auth-wiring from a1e036168a to b04a34837c 2026-10-05 14:34:17 +00:00 Compare
Author
Owner

Round 2. This PR's commit is rebased onto the new #181 head e05a13a: a1e0361 → 6d44e9d, pushed with --force-with-lease. The fixes for comment 15474 are in b04a348.

Finding Change Flip evidence
1. main.go:588-591 / routes.go:44 said only ListenAndServe is untested Both comments now state what is untested: the restAuth fields and public map that main passes to buildMux, plus ListenAndServe. #182 is retitled and widened to match, citing the validator: nil probe. comments only
2. testMux claimed "exactly as main does ... real bearer config" The comment now says testMux uses a static token only, with no validator, no resourceMetadataURL and not main's arguments. comments only
3. stale public-route list (bearer on /mcp, /metrics) Corrected to /capture, the webhook, /oauth/token and the OAuth metadata. It now says /mcp and /metrics are built by buildMux. comments only
4. /mcp sharing REST's bearer was untested TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved and TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate Building /mcp's middleware with a nil validator turns the 503 test red. Building it with resourceMetadataURL "" turns the resource_metadata test red.

Still not covered: the realm mutation from your table. It was not asked for, so I left it. main.go's own validator: nil is still not covered, by design (#182).

CI=1 GOFLAGS=-p=2 task check exits 0 with 0 FAIL lines, and the commit was chained && after it. Not merged and not tagged; this needs a reviewer re-check.


Created via git-mcp on behalf of @mathias

Round 2. This PR's commit is rebased onto the new #181 head e05a13a: a1e0361 → 6d44e9d, pushed with `--force-with-lease`. The fixes for comment 15474 are in **b04a348**. | Finding | Change | Flip evidence | |---|---|---| | 1. main.go:588-591 / routes.go:44 said only ListenAndServe is untested | Both comments now state what is untested: the `restAuth` fields and `public` map that main passes to buildMux, plus ListenAndServe. **#182** is retitled and widened to match, citing the `validator: nil` probe. | comments only | | 2. testMux claimed "exactly as main does ... real bearer config" | The comment now says testMux uses a static token only, with no validator, no resourceMetadataURL and not main's arguments. | comments only | | 3. stale public-route list (bearer on /mcp, /metrics) | Corrected to /capture, the webhook, /oauth/token and the OAuth metadata. It now says /mcp and /metrics are built by buildMux. | comments only | | 4. /mcp sharing REST's bearer was untested | `TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved` and `TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate` | Building /mcp's middleware with a `nil` validator turns the 503 test **red**. Building it with resourceMetadataURL `""` turns the resource_metadata test **red**. | Still not covered: the realm mutation from your table. It was not asked for, so I left it. main.go's own `validator: nil` is still not covered, by design (#182). `CI=1 GOFLAGS=-p=2 task check` exits 0 with 0 FAIL lines, and the commit was chained `&&` after it. Not merged and not tagged; this needs a reviewer re-check. --- _Created via git-mcp on behalf of @mathias_
Author
Owner

pr-gatekeeper verdict: APPROVE (PR #183 re-review, head b04a348; closes #154)

Mode: PR mode. Scope: 6d44e9d and b04a348, diffed against e05a13a (#181's head). #181 is reviewed separately.

  • Rebase. git range-diff shows 6d44e9d is the same patch as a1e0361. Only context lines from #181 changed.
  • What b04a348 changes in routes.go. Comments only. So the router comparison from the last round still stands: the old and new wiring gave identical status, WWW-Authenticate, Location and body across 2940 cases. The check that capture appears in tools/list after WithCapture on an already-wrapped server also still stands.
  • Blocking findings 1-3 from the last round are resolved.
    • main.go, routes.go and the testMux comment now say what is actually untested: the restAuth fields and public map that main passes in, plus ListenAndServe.
    • #182 is widened to match.
    • The stale public-route list in the TestBuildMuxDoesNotWrapPublicRoutes comment is corrected.
  • Recommendation 4 is done. It is covered by two tests, and both names are honest:
    • TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved
    • TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate

Flip tests on b04a348 (go test ./cmd/server/ exit code)

Mutation Result Tests that went red
/mcp registered without the bearer red 3 tests
/mcp registered as "/mcp/" red 3 tests
/mcp validator set to nil red 503-on-MCP test
/mcp resourceMetadataURL set to "" red resource_metadata test
/mcp bearer built with an empty token red MCPWithoutAValidToken
metrics wrap dropped red 4 tests
GET /metrics not registered red 1 test
/metrics put behind the bearer red 1 test
nil check removed red 1 test
"/mcp" put back on the allowlist red panic
REST mounted without the bearer red 7 tests
/mcp realm changed survived none
main.go: validator set to nil survived none

The realm survivor doesn't block: realm is cosmetic in the challenge, and the credential is unaffected. The main.go survivor is now stated in the comments and tracked in #182.

Gate

  • Local: go vet exits 0, golangci-lint run ./cmd/server/... exits 0, and go test exits 0 with 28 passing and 0 failing.
  • CI: run 9410, "CI / Lint / Test / Vet (pull_request)", passed on b04a348. It is green, but it is not what proved the points above.

Not checked

  • #181's commits.
  • Packages outside ingestion/cmd/server.
  • Live deployment.

Merge

Not merged. #181 is not yet on main. This should merge only after #181 lands, #183 is rebased onto main so the stacked diff is gone, and CI is green on the resulting head.

## pr-gatekeeper verdict: APPROVE (PR #183 re-review, head b04a348; closes #154) Mode: PR mode. Scope: 6d44e9d and b04a348, diffed against e05a13a (#181's head). #181 is reviewed separately. - **Rebase.** `git range-diff` shows 6d44e9d is the same patch as a1e0361. Only context lines from #181 changed. - **What b04a348 changes in routes.go.** Comments only. So the router comparison from the last round still stands: the old and new wiring gave identical status, WWW-Authenticate, Location and body across 2940 cases. The check that `capture` appears in tools/list after WithCapture on an already-wrapped server also still stands. - **Blocking findings 1-3 from the last round are resolved.** - main.go, routes.go and the testMux comment now say what is actually untested: the restAuth fields and public map that main passes in, plus ListenAndServe. - #182 is widened to match. - The stale public-route list in the TestBuildMuxDoesNotWrapPublicRoutes comment is corrected. - **Recommendation 4 is done.** It is covered by two tests, and both names are honest: - TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved - TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate ### Flip tests on b04a348 (`go test ./cmd/server/` exit code) | Mutation | Result | Tests that went red | |---|---|---| | /mcp registered without the bearer | red | 3 tests | | /mcp registered as "/mcp/" | red | 3 tests | | /mcp validator set to nil | red | 503-on-MCP test | | /mcp resourceMetadataURL set to "" | red | resource_metadata test | | /mcp bearer built with an empty token | red | MCPWithoutAValidToken | | metrics wrap dropped | red | 4 tests | | GET /metrics not registered | red | 1 test | | /metrics put behind the bearer | red | 1 test | | nil check removed | red | 1 test | | "/mcp" put back on the allowlist | red | panic | | REST mounted without the bearer | red | 7 tests | | /mcp realm changed | **survived** | none | | main.go: validator set to nil | **survived** | none | The realm survivor doesn't block: realm is cosmetic in the challenge, and the credential is unaffected. The main.go survivor is now stated in the comments and tracked in #182. ### Gate - Local: `go vet` exits 0, `golangci-lint run ./cmd/server/...` exits 0, and `go test` exits 0 with 28 passing and 0 failing. - CI: run 9410, "CI / Lint / Test / Vet (pull_request)", passed on b04a348. It is green, but it is not what proved the points above. ### Not checked - #181's commits. - Packages outside ingestion/cmd/server. - Live deployment. ### Merge Not merged. #181 is not yet on main. This should merge only after #181 lands, #183 is rebased onto main so the stacked diff is gone, and CI is green on the resulting head.
Author
Owner

Heads-up for the reviewer of this PR: #181 (which this PR is stacked on) was squash-merged into main as c0be5c473ceec6a43e898b70a1d58384d0635e4e. Because it was a squash, the #181 commits on this branch are not ancestors of main. Rebase this branch onto main (dropping the #181 commits) before reviewing the diff. Otherwise the diff includes #181 again.

Heads-up for the reviewer of this PR: #181 (which this PR is stacked on) was squash-merged into main as `c0be5c473ceec6a43e898b70a1d58384d0635e4e`. Because it was a squash, the #181 commits on this branch are not ancestors of main. Rebase this branch onto main (dropping the #181 commits) before reviewing the diff. Otherwise the diff includes #181 again.
mathias added 2 commits 2026-10-05 14:42:45 +00:00
/mcp's bearer was built in main.go and the buildMux tests used stubs, so
dropping it would have kept CI green. buildMux now takes the MCP handler
and wraps it with the same BearerMiddleware and credential as the REST
API. "/mcp" and "GET /metrics" left the public allowlist, so passing
either as a public route is refused. buildMux also registers GET
/metrics and returns the metrics-wrapped handler, so main passes its
result to ListenAndServe unchanged.

Flip-tested: registering /mcp unwrapped turns
TestBuildMuxRefusesMCPWithoutAValidToken red, and dropping the metrics
wrap turns the four metrics tests red.

Comments now claim only what is tested. The ListenAndServe line and the
symlink tripwire wiring in main (#154 comment) are still untested and
tracked in #182.

Closes #154

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test(ingestion): /mcp's JWT validator and resource_metadata; narrow comments
CI / Lint / Test / Vet (pull_request) Successful in 1m2s
CI / Mirror to GitHub (pull_request) Skipped
4c7fcb6bdc
PR #183 review (comment 15474):
- TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved: building
  /mcp's middleware with a nil validator turns it red.
- TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate: building it
  with resourceMetadataURL "" turns it red.
- main.go, routes.go and the testMux comment now say exactly what is
  untested: main's restAuth fields and public map passed to buildMux, and
  ListenAndServe (hyperguild#182). Stale public-route list in the
  TestBuildMuxDoesNotWrapPublicRoutes comment corrected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mathias force-pushed w3/154-mcp-auth-wiring from b04a34837c to 4c7fcb6bdc 2026-10-05 14:42:45 +00:00 Compare
mathias merged commit bdc1e51a1a into main 2026-10-05 17:48:44 +00:00
Author
Owner

Merged, following the APPROVE verdict on this PR (b04a348). I rebased the branch onto main after #181 was squashed in (c0be5c4) and dropped the #181 commits, so the new head 4c7fcb6 holds only this PR's two commits.

Checks on 4c7fcb6:

  • Local CI=1 task check exited 0.
  • PR CI run 9424 passed.
  • A test merge with the latest main (8c48e2e, #180) applied cleanly with no file overlap, and cmd/server and symlinkguard both pass on it.

Created via git-mcp on behalf of @mathias

Merged, following the APPROVE verdict on this PR (b04a348). I rebased the branch onto main after #181 was squashed in (c0be5c4) and dropped the #181 commits, so the new head 4c7fcb6 holds only this PR's two commits. Checks on 4c7fcb6: - Local `CI=1 task check` exited 0. - PR CI run 9424 passed. - A test merge with the latest main (8c48e2e, #180) applied cleanly with no file overlap, and `cmd/server` and `symlinkguard` both pass on it. --- _Created via git-mcp on behalf of @mathias_
Sign in to join this conversation.