fix(mcp): include OAuth state parameter in authorization URLs

[nearfamiliarcow]

Problem

MCP OAuth authorization fails with servers that require the state parameter (e.g. Attio):

{"error":"invalid_request","error_description":"Invalid value provided for: state"}

The authorize_mcp_server() function in src/tools/mcp/auth.rs builds the authorization URL without a state parameter. While OAuth 2.1 makes state optional when PKCE is used, the MCP specification does not forbid servers from requiring it, and some servers (confirmed: Attio at mcp.attio.com) enforce it as mandatory — rejecting any authorization request that omits it.

This affects both the pre-configured OAuth path and the Dynamic Client Registration (DCR) path, since both converge before the authorization URL is built and neither injects a state value.

Why extra_params doesn't solve this

The pre-configured OAuth path passes through oauth.extra_params from the MCP server config, so in theory a user could manually add "state": "<value>" to their config. However:

1. The DCR path always starts with HashMap::new() — there's no config to set extra params for dynamically registered clients
2. A hardcoded static state value would be insecure (defeats CSRF protection)
3. State must be cryptographically random and unique per request

Fix

Generate a 128-bit cryptographically random state parameter (via OsRng, base64url-encoded without padding) and inject it into extra_params after both code paths converge but before build_authorization_url() is called. This is a 6-line change plus mut on the binding.

Design decisions

Why not validate state on the callback?

The callback listener (wait_for_authorization_callback) passes None for state validation intentionally:

1. PKCE is the security boundary. The code_verifier/code_challenge exchange already binds the authorization code to the token request, preventing authorization code injection — the primary attack that state mitigates in non-PKCE flows.

2. Not all MCP servers echo state back. Strict validation would break servers that accept state but don't include it in the redirect. Since MCP servers are diverse (custom implementations, not just major OAuth providers), this is a real risk.

3. Other callers are unaffected. The shared wait_for_callback() in oauth_helpers.rs supports optional state validation. Other OAuth flows in the codebase (cli/tool.rs, extensions/manager.rs) already generate and validate state with Some(&state) — this change doesn't touch those paths.

State is sent for compatibility, PKCE provides the security guarantee.

Testing

• Regression test added: test_authorization_url_includes_state_parameter verifies state appears in the URL, uses base64url-safe encoding (no +, /, =), has sufficient entropy (≥128 bits), and is unique per generation
• Full test suite passes (3,046 tests, 0 failures)
• Clippy clean with --all --all-features
• Manually verified: Attio MCP OAuth flow succeeds with this fix applied

Test plan

• cargo test — all unit tests pass
• cargo clippy --all --benches --tests --examples --all-features — zero warnings
• Manual test: ironclaw mcp auth <server> against an MCP server that requires state (e.g. Attio)
• Manual test: ironclaw mcp auth <server> against an MCP server that does NOT require state — verify no regression

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an issue preventing MCP OAuth authorization with servers that strictly enforce the inclusion of the "state" parameter in authorization requests. By programmatically generating and embedding a secure, unique "state" value, the system now ensures broader compatibility with diverse OAuth providers without compromising security, as the primary protection against CSRF and authorization code injection is maintained through PKCE.

Highlights

• Problem Addressed: Fixed MCP OAuth authorization failures with servers (like Attio) that require the "state" parameter, which was previously omitted.
• Solution Implemented: Generated a cryptographically random 128-bit "state" parameter and injected it into the authorization URL for all OAuth flows.
• Security Rationale: Ensured compatibility by sending "state" while relying on PKCE for primary security, intentionally not validating "state" on callback due to PKCE's role and potential server variations.
• Testing: Added a new regression test to verify the "state" parameter's presence, correct base64url encoding, sufficient entropy, and uniqueness.

Changelog

• src/tools/mcp/auth.rs
  • Made the extra_params variable mutable to allow dynamic insertion of the state parameter.
  • Implemented the generation of a cryptographically random, base64url-encoded state parameter and added it to the extra_params map.
  • Updated comments to clarify the rationale for not validating the state parameter during the authorization callback, emphasizing PKCE's role in security.
  • Added a new unit test, test_authorization_url_includes_state_parameter, to validate the correct generation and inclusion of the state parameter in authorization URLs.

Activity

• The author nearfamiliarcow identified a critical compatibility issue with MCP OAuth authorization.
• A robust solution involving cryptographic state generation was implemented.
• Comprehensive testing, including a new regression test, full test suite pass, and Clippy checks, was performed.
• Manual verification confirmed the fix's effectiveness with a real-world server (Attio).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly addresses an issue where MCP OAuth authorization fails with servers requiring a state parameter by adding a cryptographically random state to the authorization URL. The change is well-reasoned, affects both pre-configured and dynamic client registration paths, and includes a thorough regression test. My review includes one suggestion to improve the robustness of the random number generation by handling potential errors instead of panicking, which is better practice within a function that already returns a Result.

[zmanian]

Reviewed the diff carefully. This is a clean, well-scoped fix. Approving.

Security assessment:

• State generation is correct: 128 bits from OsRng (CSPRNG), base64url-encoded without padding. Good entropy, URL-safe, no padding characters that could cause issues.
• The insertion point (after both pre-configured and DCR paths converge, before build_authorization_url) is the right place -- covers both flows with a single code path.
• The mut on the destructuring binding is the minimal change needed.

On not validating state on callback:

The rationale in the PR description is sound. PKCE (code_verifier/code_challenge) already binds the authorization code to the token exchange, which is the primary defense against authorization code injection -- the same attack state traditionally mitigates. The wait_for_callback infrastructure already supports Some(&state) validation, so if a future MCP server ecosystem standardizes on echoing state, it would be straightforward to enable. For now, sending state without validating it is a defensible pragmatic choice given the diversity of MCP server implementations.

Minor note: If a user had manually configured "state" in oauth.extra_params, this overwrites it -- but that's actually better behavior since a static configured value would be insecure.

Test coverage: The regression test validates presence in URL, encoding safety, entropy length, and uniqueness. Reasonable coverage for this change.