fix(security): do not follow redirects off the validated origin (#22)

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

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"

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 <key>` 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<string, mixed> $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);
+    }
 }