From 1eabcf2f55ec99712af5c23f45bc2ec7f5f5d0d6 Mon Sep 17 00:00:00 2001 From: Morris Jencen Chavez Date: Thu, 10 Sep 2026 18:29:03 +0000 Subject: [PATCH] fix: correct multipart boundary mismatch when forwarding file uploads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PassageService forwarded the client's original Content-Type header (including its multipart boundary) even when the multipart branch goes on to build an entirely new multipart body via attach(). Guzzle generates a fresh random boundary for that body and only sets a matching Content-Type header when none is already present on the request, so the forwarded header's boundary silently won instead and never matched the boundary actually used in the body. Every multipart file upload proxied through Passage was corrupted on the wire as a result, undetected because the existing Http::fake()-based test suite never serializes the outgoing body or validates it against the Content-Type header. The multipart branch now excludes the client's Content-Type header from what gets forwarded, letting Guzzle set the boundary-correct one itself. Also adds a real-HTTP-transport integration test lane (tests/Feature/PassageRealServerIntegrationTest.php), proxying through an actual PHP built-in web server instead of Http::fake(), covering body types, header forwarding, query forwarding, streaming, and the allowed-hosts guard — this class of bug is invisible to a fake-based HTTP layer. --- src/Services/PassageService.php | 17 +- .../PassageRealServerIntegrationTest.php | 256 ++++++++++++++++++ tests/Fixtures/real-server-router.php | 55 ++++ tests/Unit/PassageServiceTest.php | 27 ++ 4 files changed, 353 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/PassageRealServerIntegrationTest.php create mode 100644 tests/Fixtures/real-server-router.php diff --git a/src/Services/PassageService.php b/src/Services/PassageService.php index 6f98b07..09429be 100644 --- a/src/Services/PassageService.php +++ b/src/Services/PassageService.php @@ -22,6 +22,21 @@ public function callService(Request $request, PendingRequest $service, string $u $headers = array_merge($headers, ForwardedHeaderResolver::forwardedHeaders($request)); } + $contentType = $request->header('Content-Type', ''); + + if (count($request->allFiles()) > 0 || str_contains(strtolower($contentType), 'multipart/form-data')) { + // The multipart branch below builds a brand new multipart body via + // attach(), and Guzzle generates its own random boundary for it and + // sets a matching Content-Type header — but only when no Content-Type + // header is already present on the request (see Guzzle\Client's + // "_conditional" header handling). Forwarding the client's original + // Content-Type here (with its own, different boundary) would win over + // Guzzle's, so the boundary declared in the header would never match + // the one actually used in the body, corrupting every multipart part + // for the upstream to parse. + unset($headers['Content-Type']); + } + $service = $service->withHeaders($headers); if (in_array($method, ['get', 'head'])) { @@ -39,8 +54,6 @@ public function callService(Request $request, PendingRequest $service, string $u return $this->dispatch($service, $method, $uri); } - $contentType = $request->header('Content-Type', ''); - // PHP consumes php://input while populating $_POST/$_FILES for // multipart/form-data requests, so getContent() is always empty for // them — even when the request has no file fields, only text ones. diff --git a/tests/Feature/PassageRealServerIntegrationTest.php b/tests/Feature/PassageRealServerIntegrationTest.php new file mode 100644 index 0000000..ff31017 --- /dev/null +++ b/tests/Feature/PassageRealServerIntegrationTest.php @@ -0,0 +1,256 @@ +process !== null) { + return; + } + + $this->port = $this->findFreePort(); + + if ($this->port === 0) { + return; + } + + $this->logFile = tempnam(sys_get_temp_dir(), 'passage-real-server-'); + $router = __DIR__.'/../Fixtures/real-server-router.php'; + + $process = @proc_open( + [PHP_BINARY, '-S', "127.0.0.1:{$this->port}", $router], + [1 => ['file', $this->logFile, 'w'], 2 => ['file', $this->logFile, 'w']], + $pipes, + ); + + if ($process === false) { + return; + } + + $this->process = $process; + $this->ready = $this->waitUntilAcceptingConnections(); + } + + public function stop(): void + { + if ($this->process !== null) { + proc_terminate($this->process); + proc_close($this->process); + $this->process = null; + } + + if ($this->logFile !== null && file_exists($this->logFile)) { + unlink($this->logFile); + } + } + + public function isReady(): bool + { + return $this->ready; + } + + public function baseUri(): string + { + return "http://127.0.0.1:{$this->port}/"; + } + + private function findFreePort(): int + { + $socket = @stream_socket_server('tcp://127.0.0.1:0', $errno, $errstr); + + if ($socket === false) { + return 0; + } + + $name = stream_socket_get_name($socket, false); + fclose($socket); + + return (int) substr($name, strrpos($name, ':') + 1); + } + + private function waitUntilAcceptingConnections(): bool + { + $deadline = microtime(true) + 5; + + while (microtime(true) < $deadline) { + $connection = @fsockopen('127.0.0.1', $this->port, $errno, $errstr, 0.1); + + if ($connection !== false) { + fclose($connection); + + return true; + } + + usleep(50_000); + } + + return false; + } +} + +class RealServerEchoHandler extends PassageHandler +{ + public function getOptions(): array + { + return ['base_uri' => RealServerProcess::instance()->baseUri()]; + } +} + +class RealServerStreamingHandler extends PassageHandler +{ + public function getOptions(): array + { + return [ + 'base_uri' => RealServerProcess::instance()->baseUri(), + 'passage_streaming' => true, + ]; + } +} + +class RealServerAllowedHostHandler extends PassageHandler +{ + public function getOptions(): array + { + return ['base_uri' => RealServerProcess::instance()->baseUri()]; + } +} + +class RealServerDisallowedHostHandler extends PassageHandler +{ + public function getOptions(): array + { + return ['base_uri' => 'http://upstream.invalid.test:1/']; + } +} + +beforeAll(function () { + RealServerProcess::instance()->start(); +}); + +afterAll(function () { + RealServerProcess::instance()->stop(); +}); + +beforeEach(function () { + if (! RealServerProcess::instance()->isReady()) { + $this->markTestSkipped('Could not start the PHP built-in server used for real-server integration tests.'); + } +}); + +describe('real HTTP transport', function () { + it('forwards a JSON body, custom headers, and query parameters over a real socket', function () { + Passage::post('real/json/{path?}', RealServerEchoHandler::class); + + $response = $this->withHeaders(['X-Client-Header' => 'from-client']) + ->postJson('/real/json/echo?filter=active', ['name' => 'Ada']); + + $response->assertOk(); + $payload = $response->json(); + + expect($payload['method'])->toBe('POST') + ->and($payload['path'])->toBe('/echo') + ->and($payload['query'])->toBe(['filter' => 'active']) + ->and(json_decode($payload['body'], true))->toBe(['name' => 'Ada']) + ->and($payload['headers'])->toHaveKey('X-CLIENT-HEADER', 'from-client'); + }); + + it('forwards a urlencoded form body over a real socket', function () { + Passage::post('real/form/{path?}', RealServerEchoHandler::class); + + // PassageService forwards this branch's raw request body verbatim + // (see its comment on why it doesn't re-encode $request->post()), so + // the raw content must be set explicitly here — Laravel's post() + // helper only populates the parsed request/post array, not the raw + // body a real client's request would carry. + $response = $this->call( + 'POST', + '/real/form/echo', + content: http_build_query(['username' => 'ada', 'role' => 'admin']), + ); + + $response->assertOk(); + $payload = $response->json(); + + expect($payload['post'])->toBe(['username' => 'ada', 'role' => 'admin']); + }); + + it('forwards a multipart file upload byte-for-byte over a real socket', function () { + Passage::post('real/upload/{path?}', RealServerEchoHandler::class); + + $file = UploadedFile::fake()->createWithContent('avatar.txt', str_repeat('a', 1024)); + + $response = $this->post('/real/upload/echo', ['label' => 'profile', 'avatar' => $file]); + + $response->assertOk(); + $payload = $response->json(); + + expect($payload['post'])->toBe(['label' => 'profile']) + ->and($payload['files'])->toHaveKey('avatar') + ->and($payload['files']['avatar']['name'])->toBe('avatar.txt') + ->and($payload['files']['avatar']['size'])->toBe(1024); + }); + + it('streams a chunked upstream response back to the client over a real socket', function () { + Passage::get('real/stream/{path?}', RealServerStreamingHandler::class); + + $response = $this->get('/real/stream/stream'); + + $response->assertOk(); + expect($response->streamedContent()) + ->toBe("chunk-0\nchunk-1\nchunk-2\nchunk-3\nchunk-4\n"); + }); + + it('rejects an upstream host outside allowed_hosts before making a real network call', function () { + config()->set('passage.security.enforce_allowed_hosts', true); + config()->set('passage.security.allowed_hosts', ['127.0.0.1']); + + Passage::get('real/disallowed/{path?}', RealServerDisallowedHostHandler::class); + + $this->get('/real/disallowed/echo') + ->assertForbidden() + ->assertJson(['error' => 'Upstream host is not permitted.']); + }); + + it('allows an upstream host in allowed_hosts over a real socket', function () { + config()->set('passage.security.enforce_allowed_hosts', true); + config()->set('passage.security.allowed_hosts', ['127.0.0.1']); + + Passage::get('real/allowed/{path?}', RealServerAllowedHostHandler::class); + + $response = $this->get('/real/allowed/echo?ok=1'); + + $response->assertOk(); + expect($response->json('query'))->toBe(['ok' => '1']); + }); +}); diff --git a/tests/Fixtures/real-server-router.php b/tests/Fixtures/real-server-router.php new file mode 100644 index 0000000..db78ca6 --- /dev/null +++ b/tests/Fixtures/real-server-router.php @@ -0,0 +1,55 @@ + $value) { + if (str_starts_with($key, 'HTTP_')) { + $headers[str_replace('_', '-', substr($key, 5))] = $value; + } elseif (in_array($key, ['CONTENT_TYPE', 'CONTENT_LENGTH'], true) && $value !== '') { + $headers[str_replace('_', '-', $key)] = $value; + } +} + +$files = array_map( + fn (array $file) => ['name' => $file['name'], 'size' => $file['size']], + $_FILES +); + +header('Content-Type: application/json'); +echo json_encode([ + 'method' => $_SERVER['REQUEST_METHOD'], + 'path' => $uri, + 'query' => $_GET, + 'headers' => $headers, + 'body' => file_get_contents('php://input'), + 'post' => $_POST, + 'files' => $files, +]); diff --git a/tests/Unit/PassageServiceTest.php b/tests/Unit/PassageServiceTest.php index a28f665..fc8d942 100644 --- a/tests/Unit/PassageServiceTest.php +++ b/tests/Unit/PassageServiceTest.php @@ -335,6 +335,33 @@ public function all(?string $key = null): array expect($this->service->callService($request, $pending, 'upload'))->toBe($mockResponse); }); + it('excludes the client Content-Type header for a multipart request, so Guzzle can set its own boundary', function () { + // The attach()-built multipart body below gets a fresh, randomly + // generated boundary from Guzzle. Guzzle only sets a matching + // Content-Type header for it when none is already present on the + // request — forwarding the client's own Content-Type here (with its + // own, different boundary) would silently win instead, so the + // boundary declared in the header would never match the one the + // body actually uses, corrupting every multipart part for the + // upstream to parse. + $file = UploadedFile::fake()->create('avatar.png', 10, 'image/png'); + $request = Request::create('/test', 'POST', server: [ + 'CONTENT_TYPE' => 'multipart/form-data; boundary=----ClientBoundary', + ]); + $request->files->set('avatar', $file); + + $mockResponse = Mockery::mock(Response::class); + $pending = Mockery::mock(PendingRequest::class); + $pending->shouldReceive('withHeaders') + ->once() + ->withArgs(fn (array $headers) => ! array_key_exists('Content-Type', $headers)) + ->andReturn($pending); + $pending->shouldReceive('attach')->once()->andReturn($pending); + $pending->shouldReceive('post')->once()->with('upload', [])->andReturn($mockResponse); + + expect($this->service->callService($request, $pending, 'upload'))->toBe($mockResponse); + }); + it('forwards multiple files uploaded under the same field name without crashing', function () { $file1 = UploadedFile::fake()->create('one.txt', 5, 'text/plain'); $file2 = UploadedFile::fake()->create('two.txt', 5, 'text/plain');