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

[9.x] Fix TrimStrings middleware with non-UTF8 characters #42065

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

MrMicky-FR
Copy link
Contributor

@MrMicky-FR MrMicky-FR commented Apr 20, 2022

Currently, the TrimStrings middleware sets to null all requests inputs with a unicode character not in UTF-8, which can be really frustrating to debug

It can be reproduced by using a POST request with a body like foo=abcd%E9: the foo input will just be null if the TrimString middleware is used.

This issue was introduced in #40600, because preg_replace with u flag will return null for a non-UTF8 input (with Malformed UTF-8 characters, possibly incorrectly encoded as preg_last_error_msg())

As a fix, when preg_replace return a null value, it's simply pass the value to the PHP trim function, which was the behavior used by Laravel for years. This doesn’t change the current behavior for valid UTF-8 inputs.

@MrMicky-FR
Copy link
Contributor Author

MrMicky-FR commented Apr 20, 2022

Not sure why the tests are failing, as the failing test (Mockery\Exception\NoMatchingExpectationException in ThrottlesExceptionsWithRedisTest) seems unrelated to the changes in this PR ? And the tests was passing for the commit on my fork: https://github.com/MrMicky-FR/laravel-framework/commit/5076a1da69bc465797ff6ebe7957aca369df0f9f
EDIT: Tests are now all passing after restarting them with a dummy commit

This will trigger the tests again
@taylorotwell taylorotwell merged commit 4b233ad into laravel:9.x Apr 21, 2022
@MrMicky-FR MrMicky-FR deleted the fix_trim_strings_unicode branch April 22, 2022 07:31
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.

None yet

2 participants