Skip to content

Conversation

@jsandfordhughescoop
Copy link
Contributor

@jsandfordhughescoop jsandfordhughescoop commented Oct 16, 2025

This PR enhances the OAuth client registration endpoint by adding validation and configurable redirect domain restrictions.

Changes

  • Validate redirect URIs against configured allowed domains.
  • Added config/mcp.php with:
    • allow_all_redirect_domains — toggle to allow all redirect domains.
    • allowed_redirect_domains — list of whitelisted domains.

Benefits

  • Improved security: prevents registration with unauthorized redirect URIs.
    This is effectively the only meaningful security enforcement available within the MCP specification, which otherwise does not provide mechanisms for authenticating or authorizing clients during registration.

Notes

I have not validated client name as this could be a breaking change to users who have overwritten the default of Passport functionality.

@github-actions
Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@jsandfordhughescoop jsandfordhughescoop force-pushed the feature/adds-security-to-oauth-registration branch from 42cf6d4 to 9ba0c1f Compare October 16, 2025 12:38
@alankpax
Copy link

Maybe another way would be to be able to set or override middleware and middleware groups on these routes by configuration ?

In my project I'm using spatie/csp package to have more control on what CSP I want to use on specific route.

@jsandfordhughescoop jsandfordhughescoop force-pushed the feature/adds-security-to-oauth-registration branch from f8b9aaf to 7cb733e Compare October 16, 2025 13:31
@jsandfordhughescoop
Copy link
Contributor Author

Maybe another way would be to be able to set or override middleware and middleware groups on these routes by configuration ?

In my project I'm using spatie/csp package to have more control on what CSP I want to use on specific route.

That’s a good idea. However, I think adding redirect domain whitelists as a configuration option makes it simpler for developers to implement, while also signalling that this is a recommended best practice.

@jsandfordhughescoop jsandfordhughescoop marked this pull request as ready for review October 16, 2025 13:35
@taylorotwell
Copy link
Member

Some formatting things that would need to be fixed here.

@taylorotwell taylorotwell marked this pull request as draft October 16, 2025 13:46
@jsandfordhughescoop
Copy link
Contributor Author

Some formatting things that would need to be fixed here.

@taylorotwell - Is there anything specific?

The composer test command has been run and Rector reformatted some code.

@jsandfordhughescoop jsandfordhughescoop force-pushed the feature/adds-security-to-oauth-registration branch from 9551868 to a4bbb73 Compare October 16, 2025 15:31
@jsandfordhughescoop jsandfordhughescoop marked this pull request as ready for review October 16, 2025 15:31
@jsandfordhughescoop
Copy link
Contributor Author

Some formatting things that would need to be fixed here.

@taylorotwell - Is there anything specific?

The composer test command has been run and Rector reformatted some code.

Have had a stab at some reformatting 🤞

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Oct 23, 2025

Please consider RFC 7591: OAuth 2.0 Dynamic Client Registration Protocol:

@jsandfordhughescoop
Copy link
Contributor Author

RFC 7591: OAuth 2.0 Dynamic Client Registration Protocol

A few points here are out of the scope of this PR IMO. I am just trying to provide some protection to the register endpoint.

@taylorotwell taylorotwell merged commit 13f80d6 into laravel:main Oct 24, 2025
18 checks passed
@barryvdh
Copy link

barryvdh commented Nov 7, 2025

I think the request parameter client_name got accidentally changed to name
Before:
https://github.com/laravel/mcp/pull/87/files#diff-92678f35c558701ce8a360588eb08f99026231c8b32cd0d118315b84ea75efb0L113
After:
https://github.com/laravel/mcp/pull/87/files#diff-728478d0055c4873d77a0804b2b3c8663a55a6204c39ea99063a57ab91573f85R40

The tests pass because not typehints are on the fake registry, but in practice they fail.

My colleague has added a fix in #104

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.

6 participants