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

HTTP Client unable to post raw body with more than a thousand ampersands #42349

Closed
Krisell opened this issue May 11, 2022 · 4 comments · Fixed by #42364
Closed

HTTP Client unable to post raw body with more than a thousand ampersands #42349

Krisell opened this issue May 11, 2022 · 4 comments · Fixed by #42364

Comments

@Krisell
Copy link
Contributor

Krisell commented May 11, 2022

  • Laravel Version: 9.12.1
  • PHP Version: 8.1.5

Description:

In a new Laravel install, running the following command fails with the error message below:

Artisan::command('send', function () {
    Http::withBody(
        str_repeat('A text with a thousand ampersands &. ', 1000), 'text/html'
    )->post('https://some-url.com');
});
  ErrorException

  parse_str(): Input variables exceeded 1000. To increase the limit change max_input_vars in php.ini.

  at vendor/laravel/framework/src/Illuminate/Http/Client/PendingRequest.php:911
    907▕             $laravelData = (string) $urlString->after('?');
    908▕         }
    909▕
    910▕         if (is_string($laravelData)) {
  ➜ 911▕             parse_str($laravelData, $parsedData);
    912▕
    913▕             $laravelData = is_array($parsedData) ? $parsedData : [];
    914▕         }
    915▕

I understand that this could be solved by increasing max_input_vars, but I'm confused why the raw POST body is being parsed as query/form-data parameters at all. I'm not familiar enough with the HTTP Client to understand all details, but the problem might be related to testing-specific code (relevant when faking the client).

By making the following change in PendingRequest.php, the request is successful and the correct data is sent.

protected function parseRequestData($method, $url, array $options)
{
    $laravelData = $options[$this->bodyFormat] ?? $options['query'] ?? [];

>>   // This "solves" the problem, by ensuring the parse_str on line 911 is skipped.
>>    if ($this->bodyFormat === 'body') {
>>        return [];
>>   }

    ...

    if (is_string($laravelData)) {
        parse_str($laravelData, $parsedData);

The Client tests in the framework seem to pass with this change.

Steps To Reproduce:

The example mentioned above is sufficient, and note that a thousand ampersands could commonly appear in HTML code being transmitted (as in our use case). Changing the content type to text/plain doesn't help.

Http::withBody(
    str_repeat('A text with a thousand ampersands &. ', 1000), 'text/html'
)->post('https://some-url.com');
@Krisell Krisell changed the title HTTP Client unable to post raw body with more than a thousand ampersands. HTTP Client unable to post raw body with more than a thousand ampersands May 11, 2022
@driesvints
Copy link
Member

I have to be honest that I don't know myself. But if you can work around this with max_input_vars then I suggest to do that. Otherwise I think it's okay to try a PR with the change you proposed. I also tried this and no tests failed for me but I'm unsure if we have enough coverage for the faking of HTTP requests.

@Krisell
Copy link
Contributor Author

Krisell commented May 12, 2022

I'll try to make a PR with these changes and we can discuss the validity of the solution there.

I don't think increasing max_input_vars is a viable solution, in cases where user-provided input is sent in a POST request, there could be any number of & symbols and having an upper limit at all is quite strange.

@driesvints
Copy link
Member

@Krisell agreed, but if it's a workaround for you right now then I suggest to take it. I'm not entirely sure what will break if we implement the change you propose.

@Krisell
Copy link
Contributor Author

Krisell commented May 12, 2022

I did a bad job searching for previous reports of this issue. Here's the same issue reported for context: #36976

I'll read through the historic discussions and see if I can understand what is going on.

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 a pull request may close this issue.

2 participants