Skip to content

[Client] Add an auth parameter to HttpTransport with BearerToken - #663

Open
chr-hertel wants to merge 5 commits into
modelcontextprotocol:mainfrom
chr-hertel:client-auth-bearer-token
Open

chr-hertel wants to merge 5 commits into
modelcontextprotocol:mainfrom
chr-hertel:client-auth-bearer-token

Conversation

@chr-hertel

Copy link
Copy Markdown
Member

Part of #316 - first slice of the client authorization in chr-hertel#53, which is too big to review in one go.

Pass auth: to HttpTransport and every request gets authenticated, a refused one gets retried if the authenticator can do something about it:

$transport = new HttpTransport('https://mcp.example.com/mcp', auth: new BearerToken($accessToken));

BearerToken is the only implementation for now, OAuth builds on AuthenticatorInterface in the next slices. No discovery or sign-in here, so the conformance baseline doesn't change yet.

[BC Break] A 401 or 403 now fails with an AuthorizationException that names the challenge, also without auth: - it was a RequestException before. connect() wraps it in a ConnectionException, without a retry.

@chr-hertel chr-hertel added Client Issues & PRs related to the Client component auth Issues and PRs related to Authentication / OAuth labels Nov 4, 2026
@chr-hertel
chr-hertel requested a balanced review from Copilot November 4, 2026 22:01
@chr-hertel chr-hertel added this to the 1.0 milestone Nov 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Authentication defects can falsely report handshake success after a rejected notification, send empty or truncated retry payloads, block retries on unclosed response bodies, and misclassify SSE resumption authorization failures.

4 open issues
What changed in this PR

Adds client HTTP authentication as the first slice of #316, supporting existing bearer tokens and an extension point for future OAuth flows without adding discovery or sign-in.

Changes:

  • Adds HttpTransport::auth, BearerToken, and a PSR-18 decorator with bounded authentication retries.
  • Introduces AuthorizationException for refused requests and documents the compatibility change.
  • Adds tests for credential injection, challenge handling, and request-body replay.
File Description
tests/​Unit/​Client/​Transport/​HttpTransportTest.php Tests credential injection and authorization failures.
tests/​Unit/​Client/​Auth/​AuthenticatingHttpClientTest.php Tests authentication, retry limits, and body replay.
src/​Exception/​AuthorizationException.php Adds a dedicated authorization exception.
src/​Client/​Transport/​HttpTransport.php Integrates authentication and maps refused POST requests.
src/​Client/​Auth/​BearerToken.php Supplies fixed bearer-token credentials.
src/​Client/​Auth/​AuthenticatorInterface.php Defines credential and challenge-handling hooks.
src/​Client/​Auth/​AuthenticatingHttpClient.php Adds the authenticating HTTP client decorator.
src/​Client.php Updates exception annotations.
docs/​client/​transports.md Documents authentication configuration and extension points.
docs/​client/​errors.md Documents authorization errors and connection wrapping.
CHANGELOG.md Records authentication support and the exception compatibility break.

Review effort: Balanced

Comment thread src/Client/Transport/HttpTransport.php Resolved
Comment thread src/Client/Transport/HttpTransport.php Resolved
The HTTP client gets decorated, so the transport keeps speaking plain PSR-18:
AuthenticatingHttpClient authenticates every request and retries the ones the
authenticator can act on. BearerToken sends a token you already have.

A 401 or 403 now fails with an AuthorizationException that names the challenge,
which connect() wraps in a ConnectionException without a retry.
…ayed

A 401 or 403 on a notification or a stream resumption now fails with an
AuthorizationException. A retry closes the rejected response first, and a
non-seekable body is not retried.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The retry test’s string cast implicitly rewinds the stream, so it passes even without the decorator’s explicit rewind; using getContents() would verify that the body is restored before retrying.

1 open issue
4 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Rewinding the request body to byte zero on retry can change the submitted payload when the stream initially starts at a nonzero position; retries must restore the original position.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Retries rewind seekable request bodies to byte zero

src/​Client/​Auth/​AuthenticatingHttpClient.php:72

With a seekable body initially positioned past byte zero, an HTTP client that reads using getContents() sends from that position on the first attempt, but this rewind makes the retry send the entire stream. This can change the submitted payload (for example, payload becomes prefix:payload); capture the initial stream position before the first send and seek back to it for retries.

1 resolved since last review

Review effort: Balanced

@chr-hertel chr-hertel modified the milestones: 1.0.0, 0.10.0 Nov 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because an unpublished finding identifies an unclosed response body when authentication challenge handling throws.

No open issues 🎉

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues block the bearer-token authentication changes.

No open issues 🎉

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth Issues and PRs related to Authentication / OAuth Client Issues & PRs related to the Client component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants