SECURITY-10974: stop notification exceptions disclosing the request body

sample/callback.php

@@ -20,6 +20,8 @@
 include __DIR__.'/../src/Riskified/autoloader.php';
 use Riskified\Common\Riskified;
 use Riskified\Common\Signature;
+use Riskified\DecisionNotification\Exception\AuthorizationException;
+use Riskified\DecisionNotification\Exception\NotificationException;
 use Riskified\DecisionNotification\Model;
 
 # Replace with the 'shop domain' of your account in Riskified
@@ -53,10 +55,32 @@ function reduce_keys($carry, $item) {
 $body = @file_get_contents('php://input');
 $headers = array_intersect_key($canonical_headers, array_flip($valid_headers));
 
-$notification = new Model\Notification($signature, $headers, $body);
+// Never let a notification exception escape to the response.
+//
+// The exception message describes why authorization failed, and PHP will print an uncaught
+// exception - message, stack trace and file paths - straight into the response body when
+// display_errors is on. That turns your webhook endpoint into an oracle an unauthenticated
+// caller can query. Catch it here, answer with a bare status code, and keep the detail on the
+// server side where it belongs.
+$output = fopen('php://stdout', 'w');
+
+try {
+    $notification = new Model\Notification($signature, $headers, $body);
+} catch (AuthorizationException $e) {
+    http_response_code(401);
+    fputs($output, 'Rejected unauthorized notification: ' . $e->getMessage() . "\n");
+    fclose($output);
+    return true;
+} catch (NotificationException $e) {
+    // Covers BadPostJsonException and BadHeaderException - a signed but unusable payload.
+    http_response_code(400);
+    fputs($output, 'Rejected malformed notification: ' . $e->getMessage() . "\n");
+    fclose($output);
+    return true;
+}
+
 $msg = "Order #$notification->id changed to status '$notification->status' with message '$notification->description'\n";
 
-$output = fopen('php://stdout', 'w');
 fputs($output, $msg);
 fclose($output);
 

src/Riskified/DecisionNotification/Exception/NotificationException.php

@@ -46,6 +46,32 @@ protected function headersString() {
 
     protected function customMessage() {
         return 'Headers: ' . $this->headersString() .
-        ', Body: ' . $this->body;
+        ', Body length: ' . strlen((string) $this->body);
+    }
+
+    /**
+     * The raw request body that triggered this exception.
+     *
+     * Deliberately absent from getMessage(). The body is attacker-controlled, and exception
+     * messages routinely reach HTTP responses and log aggregators; echoing it back is an
+     * information-disclosure and log-injection channel. Read it here once you have decided
+     * where the body is safe to send.
+     *
+     * @return string The unmodified request body
+     */
+    public function getBody() {
+        return $this->body;
+    }
+
+    /**
+     * The request headers that triggered this exception, exactly as passed in.
+     *
+     * Unmasked, unlike headersString(), which is what getMessage() uses. Treat the result as
+     * sensitive and do not return it to the caller of your webhook endpoint.
+     *
+     * @return array The unmodified request headers
+     */
+    public function getHeaders() {
+        return $this->headers;
     }
 }

src/Riskified/DecisionNotification/Model/Notification.php

@@ -95,7 +95,15 @@ public function __construct($signature, $headers, $body) {
      */
     protected function test_authorization() {
         $signature = $this->signature;
-        $remote_hmac = $this->headers[$signature::HMAC_HEADER_NAME];
+        $hmac_header = $signature::HMAC_HEADER_NAME;
+        if (!isset($this->headers[$hmac_header]) || !is_string($this->headers[$hmac_header])) {
+            // An unsigned request is unauthorized, not a programming error. Without this guard
+            // the array access raises an undefined-key warning - which leaks a filesystem path
+            // when display_errors is on - before hash_equals() coerces null to '' and fails
+            // closed anyway. Same guard as the .NET reference (Utils/HttpUtils.cs).
+            throw new Exception\AuthorizationException($this->headers, $this->body);
+        }
+        $remote_hmac = $this->headers[$hmac_header];
         $local_hmac = $signature->calc_hmac($this->body);
         if (!hash_equals($remote_hmac, $local_hmac)) {
             throw new Exception\AuthorizationException($this->headers, $this->body);

src/Riskified/autoloader.php

@@ -27,5 +27,10 @@ function riskifiedAutoload($class) {
     return true;
 }
 
-// Register Riskified autoloader into the SPL autoloading stack (in order to support multiple autoloaders)
-spl_autoload_register('riskifiedAutoload', false, true);
+// Register Riskified autoloader into the SPL autoloading stack (in order to support multiple autoloaders).
+//
+// $do_throw must be true: PHP 8 ignores the argument and always throws, and passing false there
+// emits a notice on every request. Under display_errors that notice prints the SDK's absolute
+// install path into the response body. true is also the PHP 7 behaviour you want - a failed
+// registration should be loud - so this stays valid on the 7.0 floor phpcs enforces.
+spl_autoload_register('riskifiedAutoload', true, true);

tests/DecisionNotification/Model/NotificationTest.php

@@ -220,6 +220,100 @@ public function testParsesUsingSdkHttpDataSignature(): void {
             Riskified::$auth_token = $prevToken;
         }
     }
+
+    /**
+     * SECURITY-10974. The exception message reaches HTTP responses and logs, so the
+     * attacker-controlled request body must not be in it.
+     */
+    public function testAuthorizationExceptionMessageOmitsRequestBody(): void {
+        $marker = 'forged-by-attacker-marker';
+        $body = '{"order":{"id":"31337","status":"approved","description":"' . $marker . '"}}';
+        $sig = $this->signature();
+
+        try {
+            new Notification($sig, [$sig::HMAC_HEADER_NAME => 'wrong-hmac'], $body);
+            $this->fail('Expected AuthorizationException was not thrown');
+        } catch (AuthorizationException $e) {
+            $this->assertStringNotContainsString(
+                $marker,
+                $e->getMessage(),
+                'Request body must not be reflected in the exception message'
+            );
+            $this->assertStringNotContainsString(
+                $body,
+                $e->getMessage(),
+                'Request body must not be reflected in the exception message'
+            );
+        }
+    }
+
+    /**
+     * SECURITY-10974. The computed HMAC is the value the original report exfiltrated; it must
+     * never appear in the message, masked or otherwise.
+     */
+    public function testAuthorizationExceptionMessageOmitsComputedHmac(): void {
+        $body = '{"order":{"id":"1","status":"s","old_status":"o","description":null}}';
+        $sig = $this->signature();
+        $computed = $sig->calc_hmac($body);
+
+        try {
+            new Notification($sig, [$sig::HMAC_HEADER_NAME => 'wrong-hmac'], $body);
+            $this->fail('Expected AuthorizationException was not thrown');
+        } catch (AuthorizationException $e) {
+            $this->assertStringNotContainsString(
+                $computed,
+                $e->getMessage(),
+                'Server-computed HMAC must never appear in the exception message'
+            );
+        }
+    }
+
+    /**
+     * The body stays reachable for deliberate server-side logging - it is only the message
+     * that must stay clean.
+     */
+    public function testAuthorizationExceptionStillExposesBodyViaAccessor(): void {
+        $body = '{"order":{"id":"1","status":"s","old_status":"o","description":null}}';
+        $sig = $this->signature();
+        $headers = [$sig::HMAC_HEADER_NAME => 'wrong-hmac'];
+
+        try {
+            new Notification($sig, $headers, $body);
+            $this->fail('Expected AuthorizationException was not thrown');
+        } catch (AuthorizationException $e) {
+            $this->assertSame($body, $e->getBody());
+            $this->assertSame($headers, $e->getHeaders());
+        }
+    }
+
+    public function testMissingHmacHeaderThrowsAuthorizationException(): void {
+        $this->expectException(AuthorizationException::class);
+
+        $body = '{"order":{"id":"1","status":"s","old_status":"o","description":null}}';
+
+        new Notification($this->signature(), [], $body);
+    }
+
+    /**
+     * A request with no signature must fail closed without tripping an undefined-key warning -
+     * that warning discloses a filesystem path when display_errors is on.
+     */
+    public function testMissingHmacHeaderRaisesNoPhpWarning(): void {
+        $body = '{"order":{"id":"1","status":"s","old_status":"o","description":null}}';
+
+        set_error_handler(static function (int $severity, string $message): bool {
+            throw new \RuntimeException('Unexpected PHP diagnostic: ' . $message);
+        });
+
+        try {
+            new Notification($this->signature(), [], $body);
+            $this->fail('Expected AuthorizationException was not thrown');
+        } catch (AuthorizationException $e) {
+            $this->addToAssertionCount(1);
+        } finally {
+            restore_error_handler();
+        }
+    }
 }
 
 // phpcs:ignore PSR1.Classes.ClassDeclaration.MultipleClasses -- test-only signature stub kept alongside its test