maintenance: validate MCP OAuth exchanges - #4279
Conversation
|
Author remediation update: The reviewed protocol gaps are now covered: PKCE S256 is transaction-bound and verified; public and confidential clients use distinct authentication rules; the repository-known credential is restricted to explicit development mode; authorization artifacts and access tokens have enforced expiry and bounded cleanup; unsupported refresh tokens are no longer issued; request bodies are bounded; redirect, scope, and response type checks are enforced; consent uses a one-time session-bound nonce; and public endpoints use a configured HTTPS origin with URI encoding. Focused Rust validation passed (59 tests, fmt, and clippy). The current GitHub head also has successful MCP dev/release, backend, E2E, license, and label checks. The author-side blocker is resolved; maintainer review is still required. |
6be2c68 to
ac02a5d
Compare
|
Follow-up remediation: dynamic client registration is now rate-limited to 16 successful registrations per minute, idle clients expire after one hour, expired entries are pruned before the 1,024-client capacity check, and only a successful token exchange renews a client. The old head accepted the 61st anonymous registration; the new contract returns HTTP 429 with Retry-After and proves expired-capacity recovery. Full Rust validation passed: 61 tests, rustfmt, and Clippy with warnings denied. |
39eb042 to
a6429a2
Compare
|
Follow-up for the reopened registration review is available on head The reviewed head failed three new outcome contracts:
The registration limiter is now per resolved source with a bounded 4,096-source table. It uses the socket peer by default. Deployments behind a proxy can explicitly configure exact trusted CIDRs; only those peers may provide Successful token issue and rotation now keep the registered client valid through the complete refresh-token lifetime. At client capacity, only the oldest never-activated registration is reclaimed; a client with a live refresh credential is protected. All 67 Rust tests, rustfmt, Clippy for all targets/features with warnings denied, and Git whitespace checks pass locally. The three old-head failures and trusted-proxy spoofing/bounded-state cases are included in that result. GitHub CI is running on the new head. |
|
Final current-head CI update for
The PR is based on |
What changed
none) and confidential (client_secret_post) dynamic clientsexpires_inmatch server-side validationHostRegistration lifetime and capacity
An unused registration expires after one hour. After a successful authorization-code or refresh-token exchange, the registered client remains valid through the complete 24-hour lifetime of the issued refresh token; rotation extends both together.
Registration admission is scoped by resolved source, so one caller cannot occupy every caller's window. The source table is capped at 4,096 entries. Direct deployments use the accepted TCP peer. Reverse-proxy deployments may set
MCP_OAUTH_TRUSTED_PROXY_CIDRSto exact proxy networks; only those peers may supplyX-Forwarded-For, the rightmost untrusted hop is selected, malformed or overlong chains fall back to the socket peer, and/0trust is rejected.The client store remains capped at 1,024 entries. Expired entries are pruned first. If anonymous clients fill the remaining capacity, the oldest client that has never received a refresh token is reclaimed. A client with a live refresh credential is not evicted. This keeps open MCP registration interoperable without allowing unapproved registrations to permanently reserve the store.
Operator and compatibility impact
Production requires:
There is no built-in production client credential. OAuth-capable clients dynamically register and use PKCE. Existing tokens from the previous in-memory implementation do not survive a process restart and are intentionally invalid after this protocol correction.
Regression proof
The reviewed head failed three new outcome contracts:
All three now pass. Additional contracts prove the rate-limit source table remains bounded, trusted proxy chains resolve the rightmost untrusted address, untrusted peers cannot spoof forwarding headers, and invalid or address-family-wide proxy CIDRs are rejected.
Validation
cargo +1.88.0 test --manifest-path mcp-servers/mcp-bash-server/Cargo.toml— 67 passedcargo +1.88.0 clippy --all-targets --all-features --manifest-path mcp-servers/mcp-bash-server/Cargo.toml -- -D warningscargo +1.88.0 fmt --manifest-path mcp-servers/mcp-bash-server/Cargo.toml -- --checkgit diff --checkAI assistance: used for draft implementation and test iteration.
Human validation: reproduced the global-window, client/refresh lifetime, and unactivated-capacity failures; then ran the complete Rust suite, Clippy, rustfmt, peer/proxy admission, protected eviction, PKCE, expiry, refresh rotation, replay, redirect encoding, oversized-body, and public-base-URL contracts.
Risk notes: OAuth state remains process-local; a multi-instance deployment must provide sticky routing or replace the store with shared bounded state. A reverse proxy must be listed narrowly and must overwrite or safely append
X-Forwarded-For.