From d2570e63e2649a9f152d3f1a0b07854f3eecae6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Nikolaou?= Date: Mon, 30 Sep 2019 20:50:12 +0300 Subject: [PATCH] Remove config repository dependency from webhook middleware Similarly to #791, we can remove the `\Illuminate\Contracts\Config\Repository` in the `VerifyWebhookSignature`'s constructor. For consistency, the configuration is accessed with the `config()` helper like the rest of the code. Since we don't need to mock the configuration repository anymore, the test has been moved to the integration suite, because we need to bootstrap the framework. As a bonus, I have added tests for checking [replay attacks](https://stripe.com/docs/webhooks/signatures#replay-attacks) against the tolerance configuration setting. --- .../Middleware/VerifyWebhookSignature.php | 23 +-- .../VerifyWebhookSignatureTest.php | 134 ++++++++++++++++++ tests/Unit/VerifyWebhookSignatureTest.php | 103 -------------- 3 files changed, 136 insertions(+), 124 deletions(-) create mode 100644 tests/Integration/VerifyWebhookSignatureTest.php delete mode 100644 tests/Unit/VerifyWebhookSignatureTest.php diff --git a/src/Http/Middleware/VerifyWebhookSignature.php b/src/Http/Middleware/VerifyWebhookSignature.php index 0cf3662f..0d3d0992 100644 --- a/src/Http/Middleware/VerifyWebhookSignature.php +++ b/src/Http/Middleware/VerifyWebhookSignature.php @@ -3,31 +3,12 @@ namespace Laravel\Cashier\Http\Middleware; use Closure; -use Illuminate\Contracts\Config\Repository as Config; use Stripe\Exception\SignatureVerificationException; use Stripe\WebhookSignature; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; class VerifyWebhookSignature { - /** - * The configuration repository instance. - * - * @var \Illuminate\Contracts\Config\Repository - */ - protected $config; - - /** - * Create a new middleware instance. - * - * @param \Illuminate\Contracts\Config\Repository $config - * @return void - */ - public function __construct(Config $config) - { - $this->config = $config; - } - /** * Handle the incoming request. * @@ -43,8 +24,8 @@ public function handle($request, Closure $next) WebhookSignature::verifyHeader( $request->getContent(), $request->header('Stripe-Signature'), - $this->config->get('cashier.webhook.secret'), - $this->config->get('cashier.webhook.tolerance') + config('cashier.webhook.secret'), + config('cashier.webhook.tolerance') ); } catch (SignatureVerificationException $exception) { throw new AccessDeniedHttpException($exception->getMessage(), $exception); diff --git a/tests/Integration/VerifyWebhookSignatureTest.php b/tests/Integration/VerifyWebhookSignatureTest.php new file mode 100644 index 00000000..9f0a78a1 --- /dev/null +++ b/tests/Integration/VerifyWebhookSignatureTest.php @@ -0,0 +1,134 @@ + 'secret']); + config(['cashier.webhook.tolerance' => 300]); + + $this->request = new Request([], [], [], [], [], [], 'Signed Body'); + } + + public function test_response_is_received_when_secret_matches() + { + $this->withTimestamp(time()); + $this->withSignedSignature('secret'); + + $response = (new VerifyWebhookSignature()) + ->handle($this->request, function ($request) { + return new Response('OK'); + }); + + $this->assertEquals('OK', $response->content()); + } + + public function test_response_is_received_when_timestamp_is_within_tolerance_zone() + { + $this->withTimestamp(time() - 300); + $this->withSignedSignature('secret'); + + $response = (new VerifyWebhookSignature()) + ->handle($this->request, function ($request) { + return new Response('OK'); + }); + + $this->assertEquals('OK', $response->content()); + } + + public function test_app_aborts_when_timestamp_is_too_old() + { + $this->withTimestamp(time() - 301); + $this->withSignedSignature('secret'); + + $this->expectException(AccessDeniedHttpException::class); + $this->expectExceptionMessage('Timestamp outside the tolerance zone'); + + $response = (new VerifyWebhookSignature()) + ->handle($this->request, function ($request) { + }); + } + + public function test_app_aborts_when_timestamp_is_invalid() + { + $this->withTimestamp('invalid'); + $this->withSignedSignature('secret'); + + $this->expectException(AccessDeniedHttpException::class); + $this->expectExceptionMessage('Unable to extract timestamp and signatures from header'); + + $response = (new VerifyWebhookSignature()) + ->handle($this->request, function ($request) { + }); + } + + public function test_app_aborts_when_secret_does_not_match() + { + $this->withTimestamp(time()); + $this->withSignature('fail'); + + $this->expectException(AccessDeniedHttpException::class); + $this->expectExceptionMessage('No signatures found matching the expected signature for payload'); + + (new VerifyWebhookSignature()) + ->handle($this->request, function ($request) { + }); + } + + public function test_app_aborts_when_no_secret_was_provided() + { + $this->withTimestamp(time()); + $this->withSignedSignature(''); + + $this->expectException(AccessDeniedHttpException::class); + $this->expectExceptionMessage('No signatures found matching the expected signature for payload'); + + (new VerifyWebhookSignature()) + ->handle($this->request, function ($request) { + }); + } + + public function withTimestamp($timestamp) + { + $this->timestamp = $timestamp; + } + + public function withSignedSignature($secret) + { + return $this->withSignature( + $this->sign($this->request->getContent(), $secret) + ); + } + + public function withSignature($signature) + { + $this->request->headers->set('Stripe-Signature', 't='.$this->timestamp.',v1='.$signature); + + return $this; + } + + private function sign($payload, $secret) + { + return hash_hmac('sha256', $this->timestamp.'.'.$payload, $secret); + } +} diff --git a/tests/Unit/VerifyWebhookSignatureTest.php b/tests/Unit/VerifyWebhookSignatureTest.php deleted file mode 100644 index 364921ac..00000000 --- a/tests/Unit/VerifyWebhookSignatureTest.php +++ /dev/null @@ -1,103 +0,0 @@ -setSignedSignature('secret'); - - $response = (new VerifyWebhookSignature($mock->config)) - ->handle($mock->request, function ($request) { - return new Response('OK'); - }); - - $this->assertEquals('OK', $response->content()); - } - - public function test_app_aborts_when_secret_does_not_match() - { - $mock = VerifierMock::withWebhookSecret('secret') - ->setSignature('fail'); - - $this->expectException(AccessDeniedHttpException::class); - $this->expectExceptionMessage('No signatures found matching the expected signature for payload'); - - (new VerifyWebhookSignature($mock->config)) - ->handle($mock->request, function ($request) { - }); - } - - public function test_app_aborts_when_no_secret_was_provided() - { - $mock = VerifierMock::withWebhookSecret('secret') - ->setSignedSignature(''); - - $this->expectException(AccessDeniedHttpException::class); - $this->expectExceptionMessage('No signatures found matching the expected signature for payload'); - - (new VerifyWebhookSignature($mock->config)) - ->handle($mock->request, function ($request) { - }); - } -} - -class VerifierMock -{ - /** - * @var \Illuminate\Contracts\Config\Repository - */ - public $config; - - /** - * @var \Illuminate\Http\Request - */ - public $request; - - public function __construct($webhookSecret) - { - $this->config = m::mock(Config::class); - $this->config->shouldReceive('get')->with('cashier.webhook.secret')->andReturn($webhookSecret); - $this->config->shouldReceive('get')->with('cashier.webhook.tolerance')->andReturn(300); - $this->request = new Request([], [], [], [], [], [], 'Signed Body'); - } - - public static function withWebhookSecret($webhookSecret) - { - return new self($webhookSecret); - } - - public function setSignedSignature($secret) - { - $signature = $this->sign($this->request->getContent(), $secret); - - return $this->setSignature($signature); - } - - public function setSignature($signature) - { - $this->request->headers->set('Stripe-Signature', 't='.time().',v1='.$signature); - - return $this; - } - - private function sign($payload, $secret) - { - return hash_hmac('sha256', time().'.'.$payload, $secret); - } -}