Skip to content

Conversation

@msabramo
Copy link

@msabramo msabramo commented May 31, 2025

Add state param to OAuth /authorize request; increases security and allows OAuth to work with Okta

Motivation and Context

Some OAuth servers, such as Okta require the state parameter to be present to help prevent CSRF attacks.
More info:

In particular, https://support.okta.com/help/s/article/the-authentication-request-has-an-invalid-state-parameter?language=en_US says:

Okta requires the OAuth 2.0 state parameter on all requests to the /authorize endpoint to prevent cross-site request forgery (CSRF). The OAuth 2.0 specification requires that clients protect their redirect URIs against CSRF by sending a value in the authorized request that binds the request to the user-agent's authenticated state. Using the state parameter is also a countermeasure to several other known attacks, as outlined in OAuth 2.0 Threat Model and Security Considerations.

How Has This Been Tested?

  • npm run dev
  • Connect to "whoami" SSE server at http://localhost:3001/sse
  • using a corporate Okta instance as the authorization server.
  • Was able to:
    • successfully login
    • execute a whoami tool
    • verify that the tool knew info about me from OIDC.
Screenshot 2025-05-31 at 12 51 51 AM

Breaking Changes

Hopefully none

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
    Screenshot 2025-05-31 at 12 47 23 AM
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Cc: @xiaoyijun, @phuctm97, @jspahrsummers, @jerome3o-anthropic

@msabramo msabramo force-pushed the send-state-parameter-for-oauth-authorize-requests branch from 0a09986 to 091f68e Compare May 31, 2025 08:56
@xiaoyijun
Copy link
Collaborator

Hi @msabramo, thanks for your PR! I am currently considering syncing the mcp-auth fork of inspector with the official repo. Would you be open to submitting this PR to the official inspector repo as well? Of course, I will first merge this PR into the current project.

I plan to do this after modelcontextprotocol#345 is merged. If there are further updates to the official inspector repo, would you be able to help keep things in sync there?

Previously, I was syncing PRs from my personal fork to the official repo, so submitting PRs to the official repo directly from this project (mcp-auth) is a bit inconvenient.

Thanks again for your contribution!

@msabramo
Copy link
Author

msabramo commented Jun 4, 2025

Hi @msabramo, thanks for your PR! I am currently considering syncing the mcp-auth fork of inspector with the official repo. Would you be open to submitting this PR to the official inspector repo as well?

You're welcome! I'm happy to contribute to the very nice work you've done here to make auth much easier in MCP and I'd be happy to submit this to the official repo tool so that even more people can benefit from it!

I already have a few PRs in the official inspector repo:
https://github.com/modelcontextprotocol/inspector/pulls/msabramo

@xiaoyijun
Copy link
Collaborator

Hi @msabramo, some tests failed—could you take a look?

@msabramo
Copy link
Author

msabramo commented Jun 4, 2025

Hi @msabramo, some tests failed—could you take a look?

Well, it looks like these same errors might be happening on main, as a result of a push you did 3 weeks ago?

https://github.com/mcp-auth/inspector/actions/runs/15085688162

Since I'm looking at it now, let me see if I can fix them and submit another PR for that.

from apparently forgetting to add some new defaultProps

```
> @modelcontextprotocol/[email protected] build
> tsc -b && vite build

src/components/__tests__/Sidebar.test.tsx:44:10 - error TS2739: Type '{ connectionStatus: "disconnected"; transportType: "stdio"; setTransportType: Mock<UnknownFunction>; command: string; setCommand: Mock<UnknownFunction>; ... 16 more ...; setConfig: Mock<...>; }' is missing the following properties from type 'SidebarProps': oauthClientId, setOauthClientId, oauthParams, setOauthParams

44         <Sidebar {...defaultProps} {...props} />
            ~~~~~~~

src/components/__tests__/Sidebar.test.tsx:185:12 - error TS2739: Type '{ bearerToken: string; transportType: "sse"; connectionStatus: "disconnected"; setTransportType: Mock<UnknownFunction>; command: string; setCommand: Mock<UnknownFunction>; ... 15 more ...; setConfig: Mock<...>; }' is missing the following properties from type 'SidebarProps': oauthClientId, setOauthClientId, oauthParams, setOauthParams

185           <Sidebar
               ~~~~~~~

src/components/__tests__/Sidebar.test.tsx:215:12 - error TS2739: Type '{ bearerToken: string; transportType: "sse"; connectionStatus: "disconnected"; setTransportType: Mock<UnknownFunction>; command: string; setCommand: Mock<UnknownFunction>; ... 15 more ...; setConfig: Mock<...>; }' is missing the following properties from type 'SidebarProps': oauthClientId, setOauthClientId, oauthParams, setOauthParams

215           <Sidebar
               ~~~~~~~

src/components/__tests__/Sidebar.test.tsx:379:12 - error TS2739: Type '{ env: Record<string, string>; setEnv: Mock<UnknownFunction>; connectionStatus: "disconnected"; transportType: "stdio"; setTransportType: Mock<UnknownFunction>; ... 16 more ...; setConfig: Mock<...>; }' is missing the following properties from type 'SidebarProps': oauthClientId, setOauthClientId, oauthParams, setOauthParams

379           <Sidebar {...defaultProps} env={updatedEnv} setEnv={setEnv} />
               ~~~~~~~

src/components/__tests__/Sidebar.test.tsx:414:12 - error TS2739: Type '{ env: { NEW_KEY: string; }; connectionStatus: "disconnected"; transportType: "stdio"; setTransportType: Mock<UnknownFunction>; command: string; setCommand: Mock<UnknownFunction>; ... 15 more ...; setConfig: Mock<...>; }' is missing the following properties from type 'SidebarProps': oauthClientId, setOauthClientId, oauthParams, setOauthParams

414           <Sidebar {...defaultProps} env={{ NEW_KEY: "test_value" }} />
               ~~~~~~~

src/components/__tests__/Sidebar.test.tsx:599:12 - error TS2739: Type '{ config: InspectorConfig; setConfig: Mock<UnknownFunction>; connectionStatus: "disconnected"; transportType: "stdio"; setTransportType: Mock<...>; ... 16 more ...; loggingSupported: boolean; }' is missing the following properties from type 'SidebarProps': oauthClientId, setOauthClientId, oauthParams, setOauthParams

599           <Sidebar
               ~~~~~~~

Found 6 errors.
```
@msabramo
Copy link
Author

msabramo commented Jun 4, 2025

Hi @msabramo, some tests failed—could you take a look?

Well, it looks like these same errors might be happening on main, as a result of a push you did 3 weeks ago?

https://github.com/mcp-auth/inspector/actions/runs/15085688162

Since I'm looking at it now, let me see if I can fix them and submit another PR for that.

Hi @xiaoyijun, check out #4

@msabramo msabramo requested a review from xiaoyijun June 4, 2025 16:43
@xiaoyijun xiaoyijun merged commit 6523a66 into mcp-auth:main Jun 5, 2025
2 checks passed
xiaoyijun pushed a commit that referenced this pull request Jul 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants