8000 bug #52429 [HttpClient] Replace `escapeshellarg` to prevent overpassi… · symfony/symfony@e8ce12a · GitHub
[go: up one dir, main page]
Skip to content

Commit e8ce12a

Browse files
fabpotfabpot
committed
bug #52429 [HttpClient] Replace escapeshellarg to prevent overpassing ARG_MAX (alexandre-daubois)
This PR was merged into the 6.3 branch. Discussion ---------- [HttpClient] Replace `escapeshellarg` to prevent overpassing `ARG_MAX` | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #49693 | License | MIT I'm not 100% sure if it is a bugfix or a feature. I used Nicolas' suggestion in the issue to sanitize the input and used `--data-raw` to avoid any automatic formatting. Removing the use of `escapeshellarg()` also allows to remove `HttpClientDataCollectorTest::testItDoesNotGeneratesCurlCommandsForNotEncodableBody()`. Indeed, the body can now be encoded and will result on the following cURL command: ``` curl \\n --compressed \\n --request POST \\n --url 'http://localhost:8057/json' \\n --header 'Accept: */*' \\n --header 'Content-Length: 1' \\n --header 'Content-Type: application/x-www-form-urlencoded' \\n --header 'Accept-Encoding: gzip' \\n --header 'User-Agent: Symfony HttpClient (Native)' \\n --data-raw '\x00' ``` Commits ------- 3b0bb11 [HttpClient] Replace `escapeshellarg` to prevent overpassing `ARG_MAX`
2 parents 3e2f2e3 + 3b0bb11 commit e8ce12a

File tree

2 files changed

+21
-50
lines changed

2 files changed

+21
-50
lines changed

‎src/Symfony/Component/HttpClient/DataCollector/HttpClientDataCollector.php

Lines changed: 16 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Symfony\Component\HttpFoundation\Response;
1818
use Symfony\Component\HttpKernel\DataCollector\DataCollector;
1919
use Symfony\Component\HttpKernel\DataCollector\LateDataCollectorInterface;
20+
use Symfony\Component\Process\Process;
2021
use Symfony\Component\VarDumper\Caster\ImgStub;
2122

2223
/**
@@ -193,27 +194,14 @@ private function getCurlCommand(array $trace): ?string
193194
$dataArg = [];
194195

195196
if ($json = $trace['options']['json'] ?? null) {
196-
if (!$this->argMaxLengthIsSafe($payload = self::jsonEncode($json))) {
197-
return null;
198-
}
199-
$dataArg[] = '--data '.escapeshellarg($payload);
197+
$dataArg[] = '--data-raw '.$this->escapePayload(self::jsonEncode($json));
200198
} elseif ($body = $trace['options']['body'] ?? null) {
201199
if (\is_string($body)) {
202-
if (!$this->argMaxLengthIsSafe($body)) {
203-
return null;
204-
}
205-
try {
206-
$dataArg[] = '--data '.escapeshellarg($body);
207-
} catch (\ValueError) {
208-
return null;
209-
}
200+
$dataArg[] = '--data-raw '.$this->escapePayload($body);
210201
} elseif (\is_array($body)) {
211202
$body = explode('&', self::normalizeBody($body));
212203
foreach ($body as $value) {
213-
if (!$this->argMaxLengthIsSafe($payload = urldecode($value))) {
214-
return null;
215-
}
216-
$dataArg[] = '--data '.escapeshellarg($payload);
204+
$dataArg[] = '--data-raw '.$this->escapePayload(urldecode($value));
217205
}
218206
} else {
219207
return null;
@@ -255,13 +243,18 @@ private function getCurlCommand(array $trace): ?string
255243 10000
return implode(" \\\n ", $command);
256244
}
257245

258-
/**
259-
* Let's be defensive : we authorize only size of 8kio on Windows for escapeshellarg() argument to avoid a fatal error.
260-
*
261-
* @see https://github.com/php/php-src/blob/9458f5f2c8a8e3d6c65cc181747a5a75654b7c6e/ext/standard/exec.c#L397
262-
*/
263-
private function argMaxLengthIsSafe(string $payload): bool
246+
private function escapePayload(string $payload): string
264247
{
265-
return \strlen($payload) < ('\\' === \DIRECTORY_SEPARATOR ? 8100 : 256000);
248+
static $useProcess;
249+
250+
if ($useProcess ??= class_exists(Process::class)) {
251+
return (new Process([$payload]))->getCommandLine();
252+
}
253+
254+
if ('\\' === \DIRECTORY_SEPARATOR) {
255+
return '"'.str_replace('"', '""', $payload).'"';
256+
}
257+
258+
return "'".str_replace("'", "'\\''", $payload)."'";
266259
}
267260
}

‎src/Symfony/Component/HttpClient/Tests/DataCollector/HttpClientDataCollectorTest.php

Lines changed: 5 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ public static function provideCurlRequests(): iterable
248248
--header %1$sContent-Type: application/x-www-form-urlencoded%1$s \\
249249
--header %1$sAccept-Encoding: gzip%1$s \\
250250
--header %1$sUser-Agent: Symfony HttpClient (Native)%1$s \\
251-
--data %1$sfoobarbaz%1$s',
251+
--data-raw %1$sfoobarbaz%1$s',
252252
];
253253
yield 'POST with array body' => [
254254
[
@@ -286,7 +286,7 @@ public function __toString(): string
286286
--header %1$sContent-Length: 211%1$s \\
287287
--header %1$sAccept-Encoding: gzip%1$s \\
288288
--header %1$sUser-Agent: Symfony HttpClient (Native)%1$s \\
289-
--data %1$sfoo=fooval%1$s --data %1$sbar=barval%1$s --data %1$sbaz=bazval%1$s --data %1$sfoobar[baz]=bazval%1$s --data %1$sfoobar[qux]=quxval%1$s --data %1$sbazqux[0]=bazquxval1%1$s --data %1$sbazqux[1]=bazquxval2%1$s --data %1$sobject[fooprop]=foopropval%1$s --data %1$sobject[barprop]=barpropval%1$s --data %1$stostring=tostringval%1$s',
289+
--data-raw %1$sfoo=fooval%1$s --data-raw %1$sbar=barval%1$s --data-raw %1$sbaz=bazval%1$s --data-raw %1$sfoobar[baz]=bazval%1$s --data-raw %1$sfoobar[qux]=quxval%1$s --data-raw %1$sbazqux[0]=bazquxval1%1$s --data-raw %1$sbazqux[1]=bazquxval2%1$s --data-raw %1$sobject[fooprop]=foopropval%1$s --data-raw %1$sobject[barprop]=barpropval%1$s --data-raw %1$stostring=tostringval%1$s',
290290
];
291291

292292
// escapeshellarg on Windows replaces double quotes & percent signs with spaces
@@ -337,7 +337,7 @@ public function __toString(): string
337337
--header %1$sContent-Length: 120%1$s \\
338338
--header %1$sAccept-Encoding: gzip%1$s \\
339339
--header %1$sUser-Agent: Symfony HttpClient (Native)%1$s \\
340-
--data %1$s{"foo":{"bar":"baz","qux":[1.1,1.0],"fred":["\u003Cfoo\u003E","\u0027bar\u0027","\u0022baz\u0022","\u0026blong\u0026"]}}%1$s',
340+
--data-raw %1$s{"foo":{"bar":"baz","qux":[1.1,1.0],"fred":["\u003Cfoo\u003E","\u0027bar\u0027","\u0022baz\u0022","\u0026blong\u0026"]}}%1$s',
341341
];
342342
}
343343
}
@@ -397,29 +397,7 @@ public function testItDoesNotGeneratesCurlCommandsForUnsupportedBodyType()
397397
/**
398398
* @requires extension openssl
399399
*/
400-
public function testItDoesNotGeneratesCurlCommandsForNotEncodableBody()
401-
{
402-
$sut = new HttpClientDataCollector();
403-
$sut->registerClient('http_client', $this->httpClientThatHasTracedRequests([
404-
[
405-
'method' => 'POST',
406-
'url' => 'http://localhost:8057/json',
407-
'options' => [
408-
'body' => "\0",
409-
],
410-
],
411-
]));
412-
$sut->lateCollect();
413-
$collectedData = $sut->getClients();
414-
self::assertCount(1, $collectedData['http_client']['traces']);
415-
$curlCommand = $collectedData['http_client']['traces'][0]['curlCommand'];
416-
self::assertNull($curlCommand);
417-
}
418-
419-
/**
420-
* @requires extension openssl
421-
*/
422-
public function testItDoesNotGeneratesCurlCommandsForTooBigData()
400+
public function testItDoesGenerateCurlCommandsForBigData()
423401
{
424402
$sut = new HttpClientDataCollector();
425403
$sut->registerClient('http_client', $this->httpClientThatHasTracedRequests([
@@ -435,7 +413,7 @@ public function testItDoesNotGeneratesCurlCommandsForTooBigData()
435413
$collectedData = $sut->getClients();
436414
self::assertCount(1, $collectedData['http_client']['traces']);
437415
$curlCommand = $collectedData['http_client']['traces'][0]['curlCommand'];
438-
self::assertNull($curlCommand);
416+
self::assertNotNull($curlCommand);
439417
}
440418

441419
private function httpClientThatHasTracedRequests($tracedRequests): TraceableHttpClient

0 commit comments

Comments
 (0)
0