Validate OAuth authorization state - #1726
Conversation
Generate a unique state for each authorization code flow and require callbacks to return the matching redirect state before token exchange. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the OAuth authorization-code flow in ModelContextProtocol.Core by generating a per-transaction state value, sending it in the authorization request, and requiring an exact match in the authorization response before exchanging the code for tokens. It also extends the callback/result plumbing (including samples and tests) to propagate state alongside code and iss, while keeping the obsolete code-only callback path (without state validation) during its compatibility window.
Changes:
- Add per-authorization
stategeneration, request inclusion, and exact-match validation prior to token exchange. - Extend
AuthorizationResultplus default/manual and test callback handlers to capture/returnstate. - Expand test coverage for state presence/mismatch and ensure additional auth parameters cannot override reserved parameters (including
state).
Show a summary per file
| File | Description |
|---|---|
| tests/ModelContextProtocol.TestOAuthServer/Program.cs | Adds a counter for authorization-code token exchanges to assert code-exchange blocking behavior in tests. |
| tests/ModelContextProtocol.ConformanceClient/Program.cs | Updates redirect parsing to extract and return state in AuthorizationResult. |
| tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs | Updates test callback handler to return state from redirects. |
| tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs | Adds state validation tests, uniqueness coverage, and expands reserved-parameter override coverage to include state. |
| src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs | Generates state, includes it in auth URL, validates response state, and updates default/manual redirect handler to parse full redirect URL (including state). |
| src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs | Updates docs to require returning state (and optionally iss) from the callback handler; clarifies reserved params include state. |
| src/ModelContextProtocol.Core/Authentication/AuthorizationResult.cs | Adds State property and updates docs/remarks to describe its security role and requirement. |
| samples/ProtectedMcpClient/Program.cs | Updates sample callback implementation to read/return state. |
Review details
Comments suppressed due to low confidence (1)
src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs:722
- Same as above: this throw hard-codes
AuthorizationCallbackHandlerin the message, which can be misleading when the obsolete redirect delegate is configured (it is wrapped and still flows through here). Consider a handler-agnostic message.
if (string.IsNullOrEmpty(authResult.Code))
{
ThrowFailedToHandleUnauthorizedResponse($"The {nameof(ClientOAuthOptions.AuthorizationCallbackHandler)} returned a null or empty authorization code.");
}
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Low
tarekgh
left a comment
There was a problem hiding this comment.
A few notes from reviewing the state-validation change. The core security logic (256-bit per-transaction state, exact match before code exchange, reserved-parameter protection) looks correct and is well covered by the new tests. The items below are about app-compat and diagnostics.
Clarify the default callback contract, use callback-neutral diagnostics, and document the state-validation limitation of the obsolete callback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
statevalue for each OAuth authorization-code transactionstatein authorization requests and require an exact callback match before code exchangeAuthorizationResultand callback implementations to return redirect stateThis is a follow-up to tarekgh's review comment on #1605, which identified the missing authorization-response binding.
Compatibility
AuthorizationCallbackHandlerandAuthorizationResultshipped inModelContextProtocol.Core2.0.0-rc.1 via #1605. This is a behavioral breaking change from rc.1: custom handlers must now copy the redirect'sstatequery parameter intoAuthorizationResult.State; missing or mismatched state is rejected before token exchange.