From 9b3eda3fc0a9e45ced62328109ba77e5509ed927 Mon Sep 17 00:00:00 2001 From: tomas-amaro Date: Thu, 3 Sep 2026 14:19:10 +0100 Subject: [PATCH 1/2] fix: stop notification exceptions disclosing the request body SECURITY-10974 / HackerOne 3688453. The reported HMAC disclosure was already fixed in d42cae6e, which removed both HMACs from AuthorizationException and switched the comparison to hash_equals(). Two of the report's recommendations were never applied, and the alternate channel it describes is still open - it just no longer carries the signature. NotificationException::customMessage() still concatenated the raw request body. The body is attacker-controlled and exception messages reach HTTP responses and log aggregators, so it was a reflection and log-injection channel. The message now carries the body length; getBody()/getHeaders() expose the full values for deliberate server-side logging. Notification::test_authorization() now rejects a missing HMAC header explicitly. Previously the array access raised an undefined-key warning - disclosing a filesystem path under display_errors - before hash_equals() coerced null to '' and failed closed anyway. This matches the .NET reference, which guards the missing header with !string.IsNullOrEmpty before comparing. sample/callback.php now catches both exception types and answers with a bare 401 or 400. This is the change that makes the reported attack unexploitable regardless of what the exception contains, and was recommendation 3 of the report. Verified by reproducing the report's proof of concept: the forged request now gets an empty 401, there is nothing to extract and replay, and a correctly signed notification still returns 200. Co-Authored-By: Claude Opus 5 (1M context) --- sample/callback.php | 28 +++++- .../Exception/NotificationException.php | 28 +++++- .../Model/Notification.php | 10 +- .../Model/NotificationTest.php | 94 +++++++++++++++++++ 4 files changed, 156 insertions(+), 4 deletions(-) diff --git a/sample/callback.php b/sample/callback.php index c96cf4a..4080fb5 100644 --- a/sample/callback.php +++ b/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); diff --git a/src/Riskified/DecisionNotification/Exception/NotificationException.php b/src/Riskified/DecisionNotification/Exception/NotificationException.php index f74b5c4..59f514b 100644 --- a/src/Riskified/DecisionNotification/Exception/NotificationException.php +++ b/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; } } diff --git a/src/Riskified/DecisionNotification/Model/Notification.php b/src/Riskified/DecisionNotification/Model/Notification.php index 53fe5c1..36d85d3 100644 --- a/src/Riskified/DecisionNotification/Model/Notification.php +++ b/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); diff --git a/tests/DecisionNotification/Model/NotificationTest.php b/tests/DecisionNotification/Model/NotificationTest.php index c2a5162..11dc19d 100644 --- a/tests/DecisionNotification/Model/NotificationTest.php +++ b/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 From 5d49cc04c808d66b3fb5376edb3372c706340438 Mon Sep 17 00:00:00 2001 From: tomas-amaro Date: Thu, 3 Sep 2026 14:19:10 +0100 Subject: [PATCH 2/2] fix: stop the autoloader disclosing the SDK install path on PHP 8 Found while reproducing the SECURITY-10974 proof of concept: every response carried a notice naming the SDK's absolute install path, because PHP 8 ignores spl_autoload_register()'s $do_throw argument and warns when it is passed false. Passing true silences it and is the behaviour you want on PHP 7 too - a failed autoloader registration should be loud. Unrelated to the reported vulnerability, kept as its own commit for that reason. Co-Authored-By: Claude Opus 5 (1M context) --- src/Riskified/autoloader.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Riskified/autoloader.php b/src/Riskified/autoloader.php index 3d3b8fe..2bc142e 100644 --- a/src/Riskified/autoloader.php +++ b/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);