Merge pull request #791 from sebdesign/feature/invalid-webhook-signat…

…ures [10.x] Refactor handling of invalid webhook signatures
laravel · Sep 27, 2019 · 060fff8 · 060fff8
2 parents 3dc4636 + 6ce9d8a
commit 060fff8
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 36 deletions.
diff --git a/src/Http/Middleware/VerifyWebhookSignature.php b/src/Http/Middleware/VerifyWebhookSignature.php
@@ -4,19 +4,12 @@
 
 use Closure;
 use Illuminate\Contracts\Config\Repository as Config;
-use Illuminate\Contracts\Foundation\Application;
 use Stripe\Exception\SignatureVerificationException;
 use Stripe\WebhookSignature;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 
 class VerifyWebhookSignature
 {
-    /**
-     * The application instance.
-     *
-     * @var \Illuminate\Contracts\Foundation\Application
-     */
-    protected $app;
-
     /**
      * The configuration repository instance.
      *
@@ -27,13 +20,11 @@ class VerifyWebhookSignature
     /**
      * Create a new middleware instance.
      *
-     * @param  \Illuminate\Contracts\Foundation\Application  $app
      * @param  \Illuminate\Contracts\Config\Repository  $config
      * @return void
      */
-    public function __construct(Application $app, Config $config)
+    public function __construct(Config $config)
     {
-        $this->app = $app;
         $this->config = $config;
     }
 
@@ -43,6 +34,8 @@ public function __construct(Application $app, Config $config)
      * @param  \Illuminate\Http\Request  $request
      * @param  \Closure  $next
      * @return \Illuminate\Http\Response
+     *
+     * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
      */
     public function handle($request, Closure $next)
     {
@@ -54,7 +47,7 @@ public function handle($request, Closure $next)
                 $this->config->get('cashier.webhook.tolerance')
             );
         } catch (SignatureVerificationException $exception) {
-            $this->app->abort(403);
+            throw new AccessDeniedHttpException($exception->getMessage(), $exception);
         }
 
         return $next($request);

diff --git a/tests/Unit/VerifyWebhookSignatureTest.php b/tests/Unit/VerifyWebhookSignatureTest.php
@@ -3,13 +3,12 @@
 namespace Laravel\Cashier\Tests\Unit;
 
 use Illuminate\Contracts\Config\Repository as Config;
-use Illuminate\Contracts\Foundation\Application;
 use Illuminate\Http\Request;
 use Illuminate\Http\Response;
 use Laravel\Cashier\Http\Middleware\VerifyWebhookSignature;
 use Mockery as m;
 use PHPUnit\Framework\TestCase;
-use Symfony\Component\HttpKernel\Exception\HttpException;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 
 class VerifyWebhookSignatureTest extends TestCase
 {
@@ -23,7 +22,7 @@ public function test_response_is_received_when_secret_matches()
         $mock = VerifierMock::withWebhookSecret('secret')
             ->setSignedSignature('secret');
 
-        $response = (new VerifyWebhookSignature($mock->app, $mock->config))
+        $response = (new VerifyWebhookSignature($mock->config))
             ->handle($mock->request, function ($request) {
                 return new Response('OK');
             });
@@ -34,37 +33,32 @@ public function test_response_is_received_when_secret_matches()
     public function test_app_aborts_when_secret_does_not_match()
     {
         $mock = VerifierMock::withWebhookSecret('secret')
-            ->setSignature('fail')
-            ->expectAbort();
+            ->setSignature('fail');
 
-        $this->expectException(HttpException::class);
+        $this->expectException(AccessDeniedHttpException::class);
+        $this->expectExceptionMessage('No signatures found matching the expected signature for payload');
 
-        (new VerifyWebhookSignature($mock->app, $mock->config))
+        (new VerifyWebhookSignature($mock->config))
             ->handle($mock->request, function ($request) {
             });
     }
 
     public function test_app_aborts_when_no_secret_was_provided()
     {
         $mock = VerifierMock::withWebhookSecret('secret')
-            ->setSignedSignature('')
-            ->expectAbort();
+            ->setSignedSignature('');
 
-        $this->expectException(HttpException::class);
+        $this->expectException(AccessDeniedHttpException::class);
+        $this->expectExceptionMessage('No signatures found matching the expected signature for payload');
 
-        (new VerifyWebhookSignature($mock->app, $mock->config))
+        (new VerifyWebhookSignature($mock->config))
             ->handle($mock->request, function ($request) {
             });
     }
 }
 
 class VerifierMock
 {
-    /**
-     * @var \Illuminate\Contracts\Foundation\Application
-     */
-    public $app;
-
     /**
      * @var \Illuminate\Contracts\Config\Repository
      */
@@ -77,7 +71,6 @@ class VerifierMock
 
     public function __construct($webhookSecret)
     {
-        $this->app = m::mock(Application::class);
         $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);
@@ -89,13 +82,6 @@ public static function withWebhookSecret($webhookSecret)
         return new self($webhookSecret);
     }
 
-    public function expectAbort()
-    {
-        $this->app->shouldReceive('abort')->andThrow(HttpException::class, 403);
-
-        return $this;
-    }
-
     public function setSignedSignature($secret)
     {
         $signature = $this->sign($this->request->getContent(), $secret);