MCP OAuth Stage 7: end-to-end integration fixture (#97) - #145
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces integration-test fixtures and a comprehensive end-to-end test suite for the MCP OAuth Stage 7 flow, utilizing a stub upstream IdP server. The review feedback identifies a critical bug where reading the response body as text inside an assertion consumes the stream and causes subsequent JSON parsing to fail. Additionally, improvements are suggested to guard the test teardown against uninitialized variables and to terminate active keep-alive connections during server shutdown to prevent potential test hangs.
heskew
force-pushed
the
feat/mcp-stage7-e2e
branch
from
June 30, 2026 23:25
47e3c6f to
e25ab6c
Compare
heskew
marked this pull request as ready for review
June 30, 2026 23:30
heskew
added a commit
that referenced
this pull request
Jun 30, 2026
gemini-code-assist on #145, non-blocking robustness: - stub IdP server.close() could wait on Node fetch/undici keep-alive connections → also call server.closeAllConnections() so teardown can't hang. - closeIdp was undefined if startStubIdp rejected in before(); after() now calls `await closeIdp?.()` so it can't throw a TypeError that masks the setup error. The third thread (DCR body double-read) was already fixed in 6039b55. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
heskew
added a commit
that referenced
this pull request
Jul 1, 2026
gemini review on #145 (non-blocking): a plain `.includes()` is more direct than `doesNotMatch(str, new RegExp(token))` for the two upstream-token non-passthrough assertions (and avoids interpreting the token as a regex pattern). Drop the now-unused doesNotMatch import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
heskew
added a commit
that referenced
this pull request
Jul 1, 2026
…er throw) claude review on #145: if teardownHarper threw, the subsequent closeIdp?.() was skipped, leaking the stub IdP server. Move it into a finally so the stub always closes regardless. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a full round-trip integration test for the MCP OAuth flow (issue #97). A Node-owned stub upstream IdP server handles authorize/token/userinfo; its ephemeral port is passed to Harper via the `env` option so the plugin's provider config (using ${STUB_IDP_*_URL} substitution) reaches the correct loopback address. The test drives every step with a hand-rolled HTTP client (fetch with redirect: 'manual') and asserts: 1. GET /mcp (no auth) → 401 + WWW-Authenticate: Bearer resource_metadata 2. PRM fetch → RFC 9728 shape (resource, authorization_servers) 3. AS metadata fetch → RFC 8414 shape (S256, resource_parameter_supported: true) 4. DCR POST → 201 + client_id issued 5. PKCE authorize → 302 → stub IdP /authorize 6. Stub IdP callback → plugin /oauth/stub/callback → final 302 with MCP code 7. Token exchange → 200 with access_token + refresh_token 8. Authenticated /mcp → 200 with echoed sub/aud/client_id claims 9. Refresh rotation → first refresh succeeds; replayed token → invalid_grant Negative cases: plain code_challenge_method rejected, missing resource rejected, garbage bearer rejected, query-string token ignored (401), upstream IdP token absent from all client-visible responses. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The combined steps-4-9 test failed on its first real CI run with "Body is
unusable: Body has already been read": the DCR status assertion interpolated
`${await dcrRes.text()}` into its failure message, and assertion-message
arguments are evaluated eagerly (even on pass), consuming the Response body
before the `dcrRes.json()` on the next line. Read the body once via text() and
JSON.parse it. (DCR itself returns 201 correctly — this was a test-only bug.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cross-model review (HEG step 10) flagged that the negative cases asserted only that *some* error param was present, not the spec code — a false-confidence gap in the conformance gate. Assert the exact codes: `invalid_request` for plain PKCE (authorize.ts:238) and `invalid_target` for missing `resource` (authorize.ts:251, RFC 8707). The two differ — verified against source (the review had assumed both were invalid_request). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gemini-code-assist on #145, non-blocking robustness: - stub IdP server.close() could wait on Node fetch/undici keep-alive connections → also call server.closeAllConnections() so teardown can't hang. - closeIdp was undefined if startStubIdp rejected in before(); after() now calls `await closeIdp?.()` so it can't throw a TypeError that masks the setup error. The third thread (DCR body double-read) was already fixed in 6039b55. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gemini review on #145 (non-blocking): a plain `.includes()` is more direct than `doesNotMatch(str, new RegExp(token))` for the two upstream-token non-passthrough assertions (and avoids interpreting the token as a regex pattern). Drop the now-unused doesNotMatch import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…er throw) claude review on #145: if teardownHarper threw, the subsequent closeIdp?.() was skipped, leaking the stub IdP server. Move it into a finally so the stub always closes regardless. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
heskew
force-pushed
the
feat/mcp-stage7-e2e
branch
from
July 1, 2026 03:38
9f52117 to
b90586f
Compare
Member
Author
|
|
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
1 participant
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.
Closes #97 — Stage 7 of the MCP OAuth epic (#86). The v1 conformance gate.
A hand-rolled HTTP client drives the full MCP OAuth round-trip against a booted Harper fixture with a test-owned stub upstream IdP. Test-only — no
src/changes. Based onmain(includes #134withMCPAuthand #141 audit/hook).What's added
integrationTests/fixtures/mcp-oauth/—config.yaml(plugin +mcp.enabled, pinned issuer/resource, onegenericprovider whose upstream URLs are injected via${STUB_IDP_*_URL}env vars),mcp-app.js(awithMCPAuth-guarded/mcproute echoing the verifiedsub/client_id/aud/scope),package.json.integrationTests/mcp-oauth-e2e.test.ts— a test-owned stub IdP (Nodehttpserver on an ephemeral port) + the full flow: unauthenticated401+ PRM (RFC 9728) + AS metadata (RFC 8414) → DCR → PKCE-S256 authorize → stub IdP → callback → token exchange → authenticated/mcp(200, echoed claims) → refresh rotation (replay →invalid_grant). Negatives:plainPKCE →invalid_request, missingresource→invalid_target(RFC 8707), garbage bearer → 401, query-string token ignored, and an explicit assertion that the upstream IdP token never appears in any client-visible response.Status
Green on CI — the full round-trip + all negatives pass on Node 22 and 24.
Review (HEG step 10)
Cross-model review (Codex + Harper-domain adjudication; Gemini leg skipped — test-only + oauth exfiltration policy): no blockers, no significant concerns. The domain pass code-traced the assertions against the real plugin (sub→email mapping, callback path, refresh-replay revoke, header-only token, upstream-token non-passthrough) and confirmed they're accurate. One suggestion applied — negative cases now assert the specific error codes rather than just "an error param present" (caught that missing-
resourcereturnsinvalid_target, notinvalid_request). One deferred: steps 4–9 share onetest()(a deliberate, documented state-sharing tradeoff for a sequential round-trip; acceptable).🤖 Generated with Claude Code