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

[10.x] Enhance malformed request handling #50470

Closed
wants to merge 4 commits into from

Conversation

jnoordsij
Copy link
Contributor

@jnoordsij jnoordsij commented Mar 12, 2024

Solves #50466.

This PR aims to have better handling of malformed requests; in particular those which throw exceptions when certain methods on the request are called, like getMethod with content as described in issue mentioned above.

To Do:

Implementation notes:

  • I've broadened matching from just SuspiciousOperationException to RequestExceptionInterface, given that Symfony recommends this per documentation in this interface. This is a separate that can be removed if wanted, however it seems more accurate and should generally not make any difference for valid requests. No additional tests have been added for other possible exceptions as part of this broadening; this can be part of a follow-up issue/MR.
  • Due to CompiledRouteCollection using a copy of the Request instance in the scope of the match method (due to requestWithoutTrailingSlash call), the getMethod call is never called on the 'original' request within the route handling and thus causes an exception when isMethod('HEAD') is performed in the scope of preparing the error response. This is once again caught by the exception handler and then correctly rendered on the second attempt. Albeit technically a little ugly, this is probably not worth addressing further as the outcome is correct and these types of requests should be extremly rare anyways.
  • The existing Bad hostname provided message seems to be based on this line, but given that the exception also happens on incorrect method values (see test), a generic message seems to be more appropriate.

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.

@jnoordsij jnoordsij force-pushed the malformed-request-handling branch from a69d5e1 to 3d66a96 Compare March 14, 2024 08:39
@crynobone crynobone linked an issue Mar 14, 2024 that may be closed by this pull request
@driesvints
Copy link
Member

Hey @jnoordsij if you would like feedback on this PR please mark it as ready for review.

@jnoordsij jnoordsij marked this pull request as ready for review March 16, 2024 16:03
@taylorotwell
Copy link
Member

Hey @jnoordsij - this PR makes sense to me, but can you send it to 11.x? Thanks!

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.

Missing handling of SuspiciousOperationException in RouteCollection
3 participants