Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing handling of SuspiciousOperationException in RouteCollection #50466

Closed
jnoordsij opened this issue Mar 12, 2024 · 4 comments
Closed

Missing handling of SuspiciousOperationException in RouteCollection #50466

jnoordsij opened this issue Mar 12, 2024 · 4 comments

Comments

@jnoordsij
Copy link
Contributor

Laravel Version

10.45.0 (also 11.x-dev)

PHP Version

8.3.3

Database Driver & Version

N/A

Description

Recently a request was sent to my application with POST method, but with the body including a (malicious) _method: __construct parameter. By default HttpMethodParameterOverride is enabled by in the Laravel request pipeline, see:

/**
* Create a new Illuminate HTTP request from server variables.
*
* @return static
*/
public static function capture()
{
static::enableHttpMethodParameterOverride();
return static::createFromBase(SymfonyRequest::createFromGlobals());
}

Within the scope of handling the request, the method is then obtained within the scope of the match method on the RouteCollection class, which results in a SuspiciousOperationException being thrown here:
https://github.com/symfony/symfony/blob/37d91c760bd91c431f91999c243d3d2ee76853dc/src/Symfony/Component/HttpFoundation/Request.php#L1166-L1168

After this being unhandled in any manner, the result is the application rendering a 404 response. However, this in my eyes has two issues:

Steps To Reproduce

Send a POST request to any endpoint on the application, with an "invalid" value for _method in the body.

@driesvints
Copy link
Member

Hey @jnoordsij. I agree we should use 400 here. Would appreciate a PR!

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@Jimbolino
Copy link
Contributor

Would it make sense to disable the call to enableHttpMethodParameterOverride()
Or add a config() or env to enable/disable it?

@jnoordsij
Copy link
Contributor Author

Would it make sense to disable the call to enableHttpMethodParameterOverride() Or add a config() or env to enable/disable it?

This is definitely something that sounds like a reasonable follow-up in my opinion.

However given this issue has been solved with #50735 I'll close this now; feel free to create a PR for your suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants