Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verifying webhooks signatures #437

Closed
Francois-Francois opened this issue Aug 28, 2017 · 6 comments
Closed

Verifying webhooks signatures #437

Francois-Francois opened this issue Aug 28, 2017 · 6 comments

Comments

@Francois-Francois
Copy link

Stripe offer Stripe-Signature in headers when sending webhook notification.

Instead of checking if event exists on the Stripe API, which:

  • Hit the Stripe API rate limit on your account
  • Use no necessary bandwidth
  • Can fail or timeout, thinks like that
  • Stripe API sometimes respond with failure, even if the event exists (I had this issue 4 times, so it happens).

it's possible to use this method https://stripe.com/docs/webhooks#signatures

Few months ago, I switched from the StripeEvent::retrieve($id) approach on 2 larges projects (not with Cashier, my own implementation) to the Signature approach, that I feel more comfortable, and more robust.

It's easy to implement with a recent version of the Stripe library, and it can be done this way :

  • Keep the old way available
  • Check if new way is enabled, then use it. If not, use the old way.

This require 2 new env variables.

What do you think ?

@mcordingley
Copy link
Contributor

Without the signature verification, there really is no way to prevent malicious users from spoofing updates from Stripe. Depending on the application and the message spoofed, this could be bad.

Implementing this should be as simple as:

if (config('services.stripe.hook.secret')) {
    // throws Stripe\Error\SignatureVerification exception on verification failure
    Stripe\WebhookSignature::verifyHeader(
        $request->getContent(),
        $request->header('HTTP_STRIPE_SIGNATURE'),
        config('services.stripe.hook.secret'),
        config('services.stripe.hook.tolerance') // default in config file to 300 to match upstream
    );
}

@martinbean
Copy link

This feels like something that would make good middleware:

class WebhookController extends Controller
{
    public function __construct()
    {
        $this->middleware('stripe.webhook');
    }
}

@lorisleiva
Copy link

Thanks @mcordingley,

The $request->header() method did not work for me, I had to use $request->server() instead.
Here is my working set up in case it helps someone.

// App\Http\Middleware\VerifyStripeWebhookSignature

public function handle($request, Closure $next)
{
    if (! config('services.stripe.webhook.secret')) {
        return $next($request);
    }

    try {
        WebhookSignature::verifyHeader(
            $request->getContent(),
            $request->server('HTTP_STRIPE_SIGNATURE'),
            config('services.stripe.webhook.secret'),
            config('services.stripe.webhook.tolerance')
        );
    } catch (SignatureVerification $e) {
        return abort(404);
    }

    return $next($request);
}
// config/services.php

'stripe' => [
    'model' => App\User::class,
    'key' => env('STRIPE_KEY'),
    'secret' => env('STRIPE_SECRET'),
    'webhook' => [
        'secret' => env('STRIPE_WEBHOOK_SECRET'),
        'tolerance' => env('STRIPE_WEBHOOK_TOLERANCE', 300),
    ],
],
// App\Http\Kernel

protected $routeMiddleware = [
    // ...
    'from.stripe' => \App\Http\Middleware\VerifyStripeWebhookSignature::class,
];

@driesvints
Copy link
Member

This looks like a valid and good addition 👍

Welcoming PRs for this!

@driesvints driesvints changed the title [Proposal] Verifying webhooks signatures Verifying webhooks signatures Oct 9, 2018
@mcordingley
Copy link
Contributor

I'll put one together later this week, once I have the time.

@driesvints
Copy link
Member

Thanks @mcordingley for adding this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants