From e48523579b4758baaa751f3988816664e4302675 Mon Sep 17 00:00:00 2001 From: Karl Waldman Date: Sun, 13 Sep 2026 12:59:02 -0400 Subject: [PATCH] fix(security): do not follow redirects off the validated origin (#22) `CurlTransport` set `CURLOPT_FOLLOWLOCATION => true`. The #17 origin guard validates the URL the SDK *sends*; it says nothing about where the server points us next. Driving a real cross-origin 302 through the real transport: $client = new Client($key, 'http://127.0.0.1:18081', 5.0, 0); $client->latest('BRENT_CRUDE_USD'); // -> code=PWNED price=1, returned as authoritative price data Both halves are fixed. Data: redirects are not followed at all. A 3xx raises `TransportException` naming the `Location`, so a redirect surfaces as a redirect instead of as "invalid JSON" three frames later. The resolved `CURLINFO_EFFECTIVE_URL` is still compared against the requested origin as a tripwire, so re-enabling following or a future libcurl default change fails loudly rather than silently. Credential: not following means the key never leaves the origin the client already validated. Following and re-checking afterwards cannot achieve that - the request carrying `Authorization` is already on the wire before there is anything to inspect. libcurl strips that header across an origin change only since 7.58.0, and `composer.json` required `ext-curl: "*"` with no floor, so on an older host the leak was real. `lib-curl: ">=7.58.0"` is now declared; `ext-curl`'s own version tracks PHP's and cannot express a libcurl floor. The production API does not redirect - verified against api.oilpriceapi.com/v1/prices/latest and /v1/demo/prices, num_redirects=0 - so no supported call pattern changes. An intentional proxy is still reachable by pointing $baseUrl at the final URL. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- CHANGELOG.md | 10 + composer.json | 3 +- src/Http/CurlTransport.php | 68 ++++++- tests/RedirectOriginTest.php | 291 ++++++++++++++++++++++++++++++ tests/fixtures/foreign-origin.php | 24 +++ tests/fixtures/redirector.php | 22 +++ 6 files changed, 415 insertions(+), 3 deletions(-) create mode 100644 tests/RedirectOriginTest.php create mode 100644 tests/fixtures/foreign-origin.php create mode 100644 tests/fixtures/redirector.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 56147ab..0ee8c95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ ### Security +- Do not follow HTTP redirects. `CurlTransport` set + `CURLOPT_FOLLOWLOCATION => true`, so a 302 from the configured API host + delivered a body from a different origin that the SDK decoded and returned + as an authoritative price. The #17 origin guard validates the URL the SDK + sends and cannot see where the server points next. Redirects now raise + `TransportException` naming the `Location`, the resolved effective URL is + checked against the requested origin as a tripwire, and `composer.json` + declares the `lib-curl >=7.58.0` floor the SDK relied on implicitly for + stripping `Authorization` across an origin change. The production API does + not redirect, so no supported call pattern changes. - Reject raw API paths that would move the request off the configured base origin. The base URL and the caller-supplied path were concatenated, so a path such as `@evil.tld/v1/prices` turned the API host into URL userinfo and diff --git a/composer.json b/composer.json index 0ab57e1..c9a4961 100644 --- a/composer.json +++ b/composer.json @@ -46,7 +46,8 @@ "require": { "php": ">=8.1", "ext-curl": "*", - "ext-json": "*" + "ext-json": "*", + "lib-curl": ">=7.58.0" }, "require-dev": { "phpunit/phpunit": "^10.5 || ^11.0 || ^12.0 || ^13.0" diff --git a/src/Http/CurlTransport.php b/src/Http/CurlTransport.php index 47195d0..9008f7e 100644 --- a/src/Http/CurlTransport.php +++ b/src/Http/CurlTransport.php @@ -11,6 +11,13 @@ * * Zero third-party dependencies so the SDK runs anywhere PHP does, * including shared hosting and WordPress environments. + * + * Redirects are not followed. The client validates the origin of the URL it + * sends; a `Location` header is the server choosing a different one after the + * fact, which is outside that guard. Following one let a 302 from the API host + * return a body from somewhere else as an authoritative price, and on libcurl + * older than 7.58.0 carried `Authorization: Token ` along with it. The + * production API does not redirect, so nothing legitimate is lost. */ final class CurlTransport implements HttpTransport { @@ -29,8 +36,10 @@ public function request(string $method, string $url, array $headers, float $time CURLOPT_CUSTOMREQUEST => $method, CURLOPT_HTTPHEADER => $headerLines, CURLOPT_RETURNTRANSFER => true, - CURLOPT_FOLLOWLOCATION => true, - CURLOPT_MAXREDIRS => 3, + // Do not follow redirects: see the class docblock. The credential + // must not leave the origin the client already validated. + CURLOPT_FOLLOWLOCATION => false, + CURLOPT_MAXREDIRS => 0, CURLOPT_TIMEOUT_MS => (int) round($timeout * 1000), CURLOPT_CONNECTTIMEOUT_MS => (int) round(min($timeout, 10.0) * 1000), CURLOPT_HEADERFUNCTION => static function ($ch, string $line) use (&$responseHeaders): int { @@ -55,9 +64,64 @@ public function request(string $method, string $url, array $headers, float $time } $statusCode = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE); + $effectiveUrl = (string) curl_getinfo($ch, CURLINFO_EFFECTIVE_URL); + + // Tripwire, not the control. With redirect following off this can only + // differ if someone re-enables it or a future libcurl changes its + // default, and in either case the body must not be returned. + if ($effectiveUrl !== '' && !self::sameOrigin($url, $effectiveUrl)) { + throw new TransportException(sprintf( + 'Refusing the response from %s: the request to %s was redirected to a different ' + . 'origin, and data from another origin is not this API\'s data.', + $effectiveUrl, + $url, + )); + } + + if ($statusCode >= 300 && $statusCode < 400) { + throw new TransportException(sprintf( + 'HTTP request to %s was answered with a %d redirect to %s. This SDK does not ' + . 'follow redirects, because a redirect moves the request - and the API key - ' + . 'to an origin the client never validated. Point $baseUrl at the final URL if ' + . 'the redirect is intentional.', + $url, + $statusCode, + $responseHeaders['location'] ?? '(no Location header)', + )); + } // Note: no curl_close() - it has been a no-op since PHP 8.0 and is // deprecated as of PHP 8.5; the handle is freed when it goes out of scope. return new HttpResponse($statusCode, $responseHeaders, (string) $body); } + + /** + * Scheme, host and effective port must all match; userinfo on either side + * is a mismatch, because the URL the client built carries none. + */ + private static function sameOrigin(string $a, string $b): bool + { + $left = parse_url($a); + $right = parse_url($b); + + if (!is_array($left) || !is_array($right)) { + return false; + } + + return self::originOf($left) === self::originOf($right); + } + + /** + * @param array $parts + */ + private static function originOf(array $parts): string + { + $scheme = strtolower((string) ($parts['scheme'] ?? '')); + $host = strtolower((string) ($parts['host'] ?? '')); + $defaultPorts = ['http' => 80, 'https' => 443]; + $port = $parts['port'] ?? ($defaultPorts[$scheme] ?? null); + $userInfo = isset($parts['user']) || isset($parts['pass']) ? 'userinfo@' : ''; + + return $scheme . '://' . $userInfo . $host . ':' . ($port === null ? '' : (string) $port); + } } diff --git a/tests/RedirectOriginTest.php b/tests/RedirectOriginTest.php new file mode 100644 index 0000000..4de1662 --- /dev/null +++ b/tests/RedirectOriginTest.php @@ -0,0 +1,291 @@ +` to the new host. + * + * Everything here drives the real `CurlTransport` against real loopback + * servers. A mock transport cannot see this defect - it lives inside cURL. + */ +final class RedirectOriginTest extends TestCase +{ + private const KEY = 'fixture_key_NOT_REAL_0123456789'; + + /** @var list */ + private array $servers = []; + + protected function tearDown(): void + { + foreach ($this->servers as $server) { + proc_terminate($server); + proc_close($server); + } + $this->servers = []; + } + + /** + * @return array + */ + public static function redirectStatuses(): array + { + return [ + '301 moved permanently' => [301], + '302 found' => [302], + '303 see other' => [303], + '307 temporary redirect' => [307], + '308 permanent redirect' => [308], + ]; + } + + /** + * The defect exactly as reported: a cross-origin 3xx from the configured + * API host must not produce price data, and the foreign host must never + * be contacted at all. + */ + #[DataProvider('redirectStatuses')] + public function testCrossOriginRedirectNeitherReturnsDataNorReachesTheForeignHost(int $status): void + { + $log = $this->makeLog(); + $foreignPort = $this->startForeignOrigin($log); + $apiPort = $this->startRedirector('http://127.0.0.1:' . $foreignPort, $status); + + $client = new Client(self::KEY, 'http://127.0.0.1:' . $apiPort, 5.0, 0); + + try { + $price = $client->latest('BRENT_CRUDE_USD'); + $this->fail(sprintf( + 'A %d to a foreign origin returned price data: code=%s price=%s', + $status, + is_array($price) ? 'list' : $price->code, + is_array($price) ? 'list' : (string) $price->price, + )); + } catch (TransportException | ApiException $e) { + $this->assertStringNotContainsString('PWNED', $e->getMessage()); + } + + $this->assertSame( + '', + trim((string) file_get_contents($log)), + 'The SDK followed the redirect and contacted the foreign origin: ' + . (string) file_get_contents($log), + ); + } + + /** + * The credential half, stated on its own. Even where modern libcurl would + * strip `Authorization` across an origin change, the SDK must not depend + * on that: `composer.json` carries no libcurl floor a host must meet. + */ + public function testTheApiKeyNeverReachesTheRedirectTarget(): void + { + $log = $this->makeLog(); + $foreignPort = $this->startForeignOrigin($log); + $apiPort = $this->startRedirector('http://127.0.0.1:' . $foreignPort, 302); + + $client = new Client(self::KEY, 'http://127.0.0.1:' . $apiPort, 5.0, 0); + + try { + $client->latest('BRENT_CRUDE_USD'); + } catch (TransportException | ApiException) { + // expected + } + + $captured = (string) file_get_contents($log); + $this->assertStringNotContainsString(self::KEY, $captured, 'The API key reached the redirect target: ' . $captured); + $this->assertSame('', trim($captured), 'The redirect target was contacted at all: ' . $captured); + } + + /** + * A redirect body is not data. Even when the redirect points at the + * configured origin, the 3xx envelope must not be decoded and returned as + * a price. + */ + public function testSameOriginRedirectBodyIsNotReturnedAsAPrice(): void + { + $apiPort = $this->freePort(); + $this->startRedirectorOnPort($apiPort, 'http://127.0.0.1:' . $apiPort, 302); + + $client = new Client(self::KEY, 'http://127.0.0.1:' . $apiPort, 5.0, 0); + + try { + $price = $client->latest('BRENT_CRUDE_USD'); + $this->fail(sprintf( + 'A same-origin 302 body was returned as a price: code=%s', + is_array($price) ? 'list' : $price->code, + )); + } catch (TransportException | ApiException $e) { + $this->assertStringNotContainsString('REDIRECT_BODY', $e->getMessage()); + } + } + + /** + * The error a caller sees has to say what happened. A redirect surfacing as + * "invalid JSON" sends an integrator hunting the wrong problem. + */ + public function testRedirectRaisesAnErrorThatNamesTheRedirect(): void + { + $log = $this->makeLog(); + $foreignPort = $this->startForeignOrigin($log); + $apiPort = $this->startRedirector('http://127.0.0.1:' . $foreignPort, 302); + + $client = new Client(self::KEY, 'http://127.0.0.1:' . $apiPort, 5.0, 0); + + try { + $client->latest('BRENT_CRUDE_USD'); + $this->fail('Expected the redirect to raise.'); + } catch (TransportException | ApiException $e) { + $this->assertMatchesRegularExpression( + '/redirect/i', + $e->getMessage(), + 'The error must name the redirect: ' . $e->getMessage(), + ); + } + } + + /** + * A non-redirect response over the same real transport still works, so the + * fix is not "reject everything". + */ + public function testOrdinaryResponseOverTheRealTransportStillWorks(): void + { + $port = $this->freePort(); + $this->spawn($port, __DIR__ . '/fixtures/router.php', []); + + $client = new Client('valid-smoke-key', 'http://127.0.0.1:' . $port, 5.0, 0); + $price = $client->latest('BRENT_CRUDE_USD'); + + $this->assertSame('BRENT_CRUDE_USD', $price->code); + $this->assertSame(71.80, $price->price); + } + + /** + * Regression fence on the setting itself: the runtime guard below is a + * tripwire, not the control. The control is that the transport does not + * follow redirects at all. + */ + public function testTransportDoesNotEnableFollowLocation(): void + { + $source = (string) file_get_contents(dirname(__DIR__) . '/src/Http/CurlTransport.php'); + + $this->assertStringNotContainsString( + 'CURLOPT_FOLLOWLOCATION => true', + $source, + 'Re-enabling redirect following re-opens the #22 origin bypass.', + ); + $this->assertStringContainsString('CURLOPT_FOLLOWLOCATION => false', $source); + } + + /** + * `composer.json` must state the libcurl floor it relies on rather than + * accepting any version of a library whose credential-stripping behaviour + * changed in 7.58.0. + */ + public function testComposerDeclaresALibcurlFloor(): void + { + $composer = json_decode( + (string) file_get_contents(dirname(__DIR__) . '/composer.json'), + true, + flags: JSON_THROW_ON_ERROR, + ); + + $this->assertArrayHasKey('lib-curl', $composer['require']); + $this->assertMatchesRegularExpression('/^>=7\.58/', (string) $composer['require']['lib-curl']); + } + + private function makeLog(): string + { + $log = tempnam(sys_get_temp_dir(), 'opa-redirect-'); + self::assertIsString($log); + file_put_contents($log, ''); + + return $log; + } + + private function startForeignOrigin(string $log): int + { + $port = $this->freePort(); + $this->spawn($port, __DIR__ . '/fixtures/foreign-origin.php', ['FOREIGN_LOG' => $log]); + + return $port; + } + + private function startRedirector(string $target, int $status): int + { + $port = $this->freePort(); + $this->startRedirectorOnPort($port, $target, $status); + + return $port; + } + + private function startRedirectorOnPort(int $port, string $target, int $status): void + { + $this->spawn($port, __DIR__ . '/fixtures/redirector.php', [ + 'REDIRECT_TO' => $target, + 'REDIRECT_STATUS' => (string) $status, + ]); + } + + /** + * @param array $env + */ + private function spawn(int $port, string $script, array $env): void + { + $descriptors = [['pipe', 'r'], ['file', '/dev/null', 'a'], ['file', '/dev/null', 'a']]; + $server = proc_open( + sprintf('exec php -S 127.0.0.1:%d %s', $port, escapeshellarg($script)), + $descriptors, + $pipes, + dirname(__DIR__), + $env + getenv(), + ); + + if (!is_resource($server)) { + $this->markTestSkipped('Could not start a loopback PHP server.'); + } + + $this->servers[] = $server; + $this->waitForPort($port); + } + + private function freePort(): int + { + $sock = stream_socket_server('tcp://127.0.0.1:0', $errno, $errstr); + self::assertIsResource($sock, 'Could not allocate a loopback port: ' . $errstr); + $name = stream_socket_get_name($sock, false); + fclose($sock); + self::assertIsString($name); + + return (int) substr($name, (int) strrpos($name, ':') + 1); + } + + private function waitForPort(int $port): void + { + for ($i = 0; $i < 100; $i++) { + $conn = @stream_socket_client('tcp://127.0.0.1:' . $port, $errno, $errstr, 0.1); + if (is_resource($conn)) { + fclose($conn); + + return; + } + usleep(50_000); + } + + self::fail('Loopback server on port ' . $port . ' never came up.'); + } +} diff --git a/tests/fixtures/foreign-origin.php b/tests/fixtures/foreign-origin.php new file mode 100644 index 0000000..b14c6f7 --- /dev/null +++ b/tests/fixtures/foreign-origin.php @@ -0,0 +1,24 @@ + $_SERVER['REQUEST_URI'] ?? null, + 'host' => $_SERVER['HTTP_HOST'] ?? null, + 'authorization' => $_SERVER['HTTP_AUTHORIZATION'] ?? null, + ], JSON_THROW_ON_ERROR) . "\n", FILE_APPEND); +} + +header('Content-Type: application/json'); +echo json_encode([ + 'status' => 'success', + 'data' => ['code' => 'PWNED', 'price' => 1, 'currency' => 'USD'], +], JSON_THROW_ON_ERROR); diff --git a/tests/fixtures/redirector.php b/tests/fixtures/redirector.php new file mode 100644 index 0000000..ee212b9 --- /dev/null +++ b/tests/fixtures/redirector.php @@ -0,0 +1,22 @@ + 'success', + 'data' => ['code' => 'REDIRECT_BODY', 'price' => 2, 'currency' => 'USD'], +], JSON_THROW_ON_ERROR);