stripe / stripe-node

Node.js library for the Stripe API.
https://stripe.com
MIT License
3.8k stars 740 forks source link

Webhook signature verification and bodyParser.json issue #341

Closed manu-st closed 7 years ago

manu-st commented 7 years ago

If you use in your express app bodyParser.json() for all routes, and then have a dedicated route for stripe's webhook, then the second call to bodyParser.json({verify: ...}) as done in the example has no effect.

I have the following code:

app.use(bodyParser.json());
...
app.use('/stripeUrl', stripeRoute);
app.use('/user', userRoute);
....

The StripeRoute is defined exactly as the example.

Unfortunately it doesn't work. Only if I comment out the first call to bodyParser.json does it work. Which mean that I cannot have a call to bodyParser just for my route, I need to add the verify function my top call to bodyParser.json and to avoid having to pay for the conversion all the time check for the Url to match my stripe route.

Maybe the doc should reflect that, and if it is supposed to work then some guidance as it doesn't for me.

Thanks!

brandur-stripe commented 7 years ago

@jlomas-stripe Would you mind providing some guidance/advice on this one? Thanks!

ronzeidman commented 7 years ago

This is the way I'm handling it:

// main file
app
    .set(...)
    .use(...<helmet, cors, compression, logging...>)
    .use('/stripe-webhooks', stripeWebhookRoutes) //the webhooks must come before the default json body parser
    .use(bodyParser.json())

// webhook handling file

export const stripeWebhookRoutes = Router()
    .use(bodyParser.raw({type: '*/*'}))
    .post('', eventParser(process.env.STRIPE_WEBHOOK_SECRET), mainStripeWebhook)
    .post('/connect', eventParser(process.env.STRIPE_WEBHOOK_CONNECT_SECRET), connectStripeWebhook);

function eventParser(secret) {
    return (req, res, next) => {
        try {
            req.body = stripe.webhooks.constructEvent(
                req.body.toString(),
                req.headers['stripe-signature'],
                secret);
        } catch (error) {
            return res.sendStatus(httpStatus.BAD_REQUEST);
        }
        return next();
    }
}
manu-st commented 7 years ago

@ronzeidman Thanks for sharing your solution, mine is:

app.use(bodyParser.json({
    // Because Stripe needs the raw body, we compute it but only when hitting the Stripe callback URL.
    verify: function(req,res,buf) {
        var url = req.originalUrl;
        if (url.startsWith('/stripe-webhooks')) {
            req.rawBody = buf.toString()
        }
    }}));

At the end it does what we need but I think it should be provided in the doc as currently you might think that the example would be easily plug-able as is but it is not.

jlomas-stripe commented 7 years ago

This is a really good point, but I think it's going to be pretty specific to how your application is implemented.

I guess there are a number of different ways this could be implemented; the idea of the documentation example was just to provide a way to get it working quickly.

manu-st commented 7 years ago

It might be good to highlight this in the example that proper care has to be done for the bodyParser. Especially in the official docs at https://stripe.com/docs/webhooks where there is no mention on how to set this up, and I think the following comment:

// We use a router here so it can use its own `bodyParser` instance to add
// the raw POST data to the `request`

is actually confusing making you believe that it will work all the time because it is in its own route.

lincolnthree commented 7 years ago

Yep. I ran into this too. Still not sure how I want to handle it.

floatingLomas commented 7 years ago

Those docs were based on my example so ... this is my bad.

I'll take a look and see if there's another/better/easier/more-consistent way to get at the raw body in Express without using body-parser.

jlomas-stripe commented 7 years ago

Ok, so you can accomplish the same with just simple middleware:

function addRawBody(req, res, next) {
  req.setEncoding('utf8');

  var data = '';

  req.on('data', function(chunk) {
    data += chunk;
  });

  req.on('end', function() {
    req.rawBody = data;

    next();
  });
}
joaoreynolds commented 7 years ago

I'm pulling my hair out and could really use some insight. I've tried all methods mentioned above (using bodyParser.raw, using the addRawBody middleware).

Note: this is an event for a connected account, I need to handle the account.application.deauthorized event. Not sure if the fact that it's a connected account has anything to do with it.

I'm using the latest stripe package v4.24.0

Here's my first middleware/route handler (no other middleware above this):

app.post('/stripe-webhooks', bodyParser.raw({type: '*/*'}), postStripeWebhooks)
//...further down I do the standard, json parser (but even if I remove that it makes not difference)
app.use(bodyParser.json())

And postStripeWebhooks:

const body = req.body.toString()
try {
  const event = stripe.webhooks.constructEvent(body, req.headers['stripe-signature'], config.stripeWebhookSecret)
  console.log(event)
}
catch(e) {
  console.log('Error', e.message)
}

Every way I go about it I get the same result: No signatures found matching the expected signature for payload I've also tried everything from the example at https://github.com/stripe/stripe-node/blob/master/examples/webhook-signing/express.js Please help!

jlomas-stripe commented 7 years ago

@joaoreynolds If you use the 'simple middleware' from the example, and use request.rawBody (instead of req.body.toString()), does that work?

joaoreynolds commented 7 years ago

*epic facepalm

I just thought, "know what? I better check my webhook secret key even though I'm sure I have that set".

I had the wrong effing webhook secret key stored in my production environment variables!

(That moment when you realize you're a terrible developer and nobody should hire you, haha)

Thanks @jlomas-stripe for being willing to help.

In an effort to be somewhat productive to this thread, after cleaning my code, I found that @manu-st 's version was the least-invasive to my app, allowing me to use bodyParser.json() on the rest of my app.

jlomas-stripe commented 7 years ago

You're very welcome - any time! :)

Ontokrat commented 6 years ago

My implementation in Meteor - thanks to @manu-st. I use Picker as router Server Side (allow filter and sub-routes). Body-parser in JSON for every webhook / exception for Stripe: verification of webhook's signature (raw) and then handling the same webhook in JSON.

import bodyParser from 'body-parser';
import { Picker } from 'meteor/meteorhacks:picker';

// Middleware declaration
Picker.middleware(bodyParser.json({
  limit: '10MB',
  verify: function(req,res,buf) {
    var url = req.originalUrl;
    if (url.startsWith('/stripe-webhooks')) {
      req.rawBody = buf.toString()
    }
  }
}));

// filtered main route
var postRoutes = Picker.filter(function(req, res, buf) {
  var url = req.originalUrl;
  if (url.startsWith('/stripe-webhooks')) {
    // verify webhook's signature if he comes from stripe
    // 'req.rawBody' available here
    // Got only 'empty constructEvent' from official library ( let event = stripe.webhooks.constructEvent(req.body, sig, endpointSecret); )
    // adapt here https://github.com/stripe/stripe-node/blob/master/lib/Webhooks.js
    // see details: https://stripe.com/docs/webhooks#verify-manually
  }
    // handle only POST requests
    return req.method == "POST"
});

// from route, handling webhook and sending request
postRoutes.route('url', (request, response) => {
  // handle result here
});
lomegg commented 6 years ago

The issue is still present and driving me mad.

floatingLomas commented 6 years ago

@lomegg Can you provide more details on your issue and the specifics of your implementation, ie versions, where/how you're running it?

lomegg commented 6 years ago

@floatingLomas I am running it with express when getting a webhook after the simple cart charge (those are not required but I use them as confirmation in my eCommerce, so I need them to be signature verified). My issue is that I am getting object instead of json even after I use bodyParser as middleware like this
var rawParser = bodyParser.raw({type: "*/*"}); app.all('/webhook', rawParser, keystone.middleware.api, routes.api.webhook.process);

Note that verify option in bodyParser doesn't get called if I assign function to it. No matter how I try, I can't get webhook request body in any form other than javascript object. And if I convert that object to json, I'm getting validation error because it's not pure request body.

jlomas-stripe commented 6 years ago

@lomegg What does keystone.middleware.api do? Does that overwrite the body by chance?

lomegg commented 6 years ago

@jlomas-stripe It provides some api methods, I was testing with it switched off to the same effect. But I will double check on that again, thanks

lomegg commented 6 years ago

Still getting that SyntaxError: Unexpected token o in JSON at position 1 even after I chained bodyParser

floatingLomas commented 6 years ago

Best bet is to share the code in routes.api.webhook.process here, or probably better, with Support: https://support.stripe.com/email

nabilfreeman commented 6 years ago

Thanks @manu-st for such an elegant solution - using req.bodyRaw instead of req.body worked perfectly.

It meant that we didn't have to refactor our Express routing. Great job!

dburles commented 6 years ago

@lomegg I had the same issue and I found that another part of the application was adding in a bodyParser.json() middleware.

pincheira commented 6 years ago

@manu-st 's solution is what I got it working with. The thing is that most people's express setup is doing:

app.use(bodyParser.json());

which sets the json parser for all body objects by default. So I just now pass a setup object to it that includes the block that @manu-st showed.

app.use(bodyParser.json(setupForStripeWebhooks));

That object being:


const setupForStripeWebhooks = {
  // Because Stripe needs the raw body, we compute it but only when hitting the Stripe callback URL.
  verify: function (req, res, buf) {
    var url = req.originalUrl;
    if (url.startsWith('/webhooks/stripe')) {
      req.rawBody = buf.toString();
    }
  }
};
karlr-stripe commented 6 years ago

(posting this for visibility for people who find this through search engines) If you use express-generator to create your project, it will likely add the app.use(express.json()); line to your app.js which also sets the JSON parser on all request bodies by default. So that is another thing to watch out for!

wdmtech commented 5 years ago

For those seeking solutions for Feathers.js (like I was) check out feathers-stripe-webhooks which will do all this for you 👍

DanielGibbsNZ commented 5 years ago

For those seeking solutions for Feathers.js (like I was) check out feathers-stripe-webhooks which will do all this for you 👍

Unfortunately this does not seem to be the case. I get the same problem even when using feathers-stripe-webhooks.

stevus commented 5 years ago

Why do these issues keep getting closed? Feels like there's a big issue with the signature validation code around needing the raw body. I am also having this problem but hard to find a solution when every issue gets diagnosed as user error.

jlomas-stripe commented 5 years ago

@stevus This isn't a stripe-node issue, it's a problem with your web framework, or the way you've configured it or are using it - there isn't really much we can do from our end here unfortunately.

In order to calculate the signature, stripe-node needs the exact, complete raw text body as received by your application; if your framework doesn't handle making that available because it's parsed/altered it in some way, then this won't work for you - unless you can find a workaround, and that workaround is going to be very specific to your application, because it will depend on how your application is configured/what it does/how it does what it does.

SamMatthewsIsACommonName commented 5 years ago

One thing that I find weird is the inconsistency. About 50% of the time it works, the other 50% I get the no signatures found matching the expected signature issue. Any suggestions on why it might work half the time and not the other? For example certain events seem to always get through, others never

Edit: I have the latest release too (6.15.0)

It seems some event types are supported, and some aren't basically?

stevus commented 5 years ago

@jlomas-stripe it might indeed be a framework issue, but the way the Stripe node package requires a raw payload doesn't fit too well with some well established patterns for body-parsing etc. It just feels like this implementation only works with hobby or quick-hack builds where the POST handler is right at the Express app root where only a body parser is used.

If you have a solution that works well with an Express Router where body parsers can be switched out ad-hoc, I think that would solve a lot of these issues.

ob-stripe commented 5 years ago

@stevus I don't think that's specific to Stripe -- all implementations of webhook signatures I'm aware of rely on a MAC or digital signature that uses the raw payload. I understand this is a common pain point for Node users, but as @jlomas-stripe said above I'm not sure there's much more we can do to help with this.

@SamMatthewsIsACommonName There's no reason why signature verification would fail or succeed depending on the event's type -- the signature is computed and verified using the raw payload and is not even aware that the payload is a JSON string. If you can give more information about how your webhook handler is set up, we might be able to help you debug this issue. I also recommend you reach out to Stripe's support at https://support.stripe.com/email if you haven't already.

SamMatthewsIsACommonName commented 5 years ago

@ob-stripe reason or not this is what happens, I'm purely going off of what is happening:

screen shot 2018-11-23 at 11 11 10 am screen shot 2018-11-23 at 11 11 27 am

The handler is set up as per the docs but uses req.rawBody as per firebase cloud functions spec. I don't understand why if it was set up wrong at all it would work 50% of time and not work other 50%?

Edit: added setup:

screen shot 2018-11-23 at 11 15 29 am
ob-stripe commented 5 years ago

Sorry, I wasn't trying to imply that you were wrong and the issue you're describing wasn't happening, merely that it shouldn't be happening.

I think Firebase's rawBody returns a Buffer instance. Can you try converting it to a string? E.g. with req.rawBody.toString('utf8')

SamMatthewsIsACommonName commented 5 years ago

Great thank you this seems to be working based off of the test I've done so far. I'll keep monitoring. Hopefully that's the case! Weird that it was still able to work before doing that for some... I'll reply if it happens again. Thanks

ob-stripe commented 5 years ago

Cool, thanks for the quick reply. We can fix this particular case in the library itself, I'll work on a fix.

SamMatthewsIsACommonName commented 5 years ago

Ok great thanks, I thought the battle was won finding the right way to get the rawBody haha. Thanks!

ob-stripe commented 5 years ago

@SamMatthewsIsACommonName We just released stripe-node 6.15.1 which should let you pass Buffer instances to Webhook.constructEvent() without having to manually convert them to strings yourself. Thanks for reporting this, hopefully this makes the library a bit easier to use for you and other Firebase users!

SamMatthewsIsACommonName commented 5 years ago

Great thanks!

Edit: I'm still seeing the error. Please let me monitor with the new version for a while. Thanks

SamMatthewsIsACommonName commented 5 years ago

Ok after monitoring a while, the error is still happening (with latest release 6.15.1). I included a log check of:

   console.log('rawBody is buffer: ', Buffer.isBuffer(req.rawBody));

And this is what you're seeing in the screen shots. So it's correctly identifying it as a buffer for both the ones that work (most of them) and the ones that don't(some of them), however the toString doesn't seem to be solving the issue for all

screen shot 2018-11-25 at 11 34 05 pm

the log after the error is the log of the signature (req.headers['stripe-signature'])

ob-stripe commented 5 years ago

@SamMatthewsIsACommonName Argh :/ Can you try adding some code to manually extract the event IDs in case the signature verification fails (something like JSON.parse(req.rawBody).id should work), and reach out to support at https://support.stripe.com/email with some of those event IDs as well as their respective Stripe-Signature headers? Hopefully we can figure out what these events have in common and causes the signature verification to fail.

SamMatthewsIsACommonName commented 5 years ago

@ob-stripe will do. I'll start writing each fail to the database with the headers and ids. It does seem to be related to events weirdly because they come in bursts of all failing at once or all passing... So it's like there is some time where stripe sends a lot of events of some type every day. Anyway I'll log them all to database as you suggest. Thanks

lomegg commented 5 years ago

@ob-stripe will do. I'll start writing each fail to the database with the headers and ids. It does seem to be related to events weirdly because they come in bursts of all failing at once or all passing... So it's like there is some time where stripe sends a lot of events of some type every day. Anyway I'll log them all to database as you suggest. Thanks

Sounds like some async process mutating the body and sometimes it's being slower than stripe check

SamMatthewsIsACommonName commented 5 years ago

Oh wow, ok I've found the issue. The ones that are failing are coming from test data... I'm so sorry! It was still enabled and hitting the endpoint with events from test accounts etc... So sorry for the time wasting!

ob-stripe commented 5 years ago

@SamMatthewsIsACommonName No worries! Glad you figured it out and thanks for letting us know :)

mebibou commented 5 years ago

What about allowing json in constructEvent? wouldn't it just solve everyone's problems?

ob-stripe commented 5 years ago

@mebibou No, the signature verification needs the raw body as a text string. Reserializing already deserialized JSON back into a text string doesn't work as the resulting string might be different (key order is not guaranteed, indentation might be different, etc.).

kurry421 commented 5 years ago

for anyone still struggling... this is what worked for me.

...
var app = express();
//This is specific raw body for /webhooks/stripe route 
app.use('/webhooks/stripe', bodyParser.raw({type: "*/*"}))
//This is normal body parsing for everything else
app.use(bodyParser.json());

...
//later on..

const stripe = require('stripe')('sk_..');
const endpointSecret = 'whsec...';

app.post('/webhooks/stripe', (request, response) => {
  const sig = request.headers['stripe-signature'];
  let event;
  try {
    event = stripe.webhooks.constructEvent(request.body, sig, endpointSecret);
  } catch (err) {
    return response.status(400).send(`Webhook Error: ${err.message}`);
  }

  // Handle the checkout.session.completed event
  if (event.type === 'checkout.session.completed') {
    const session = event.data.object;

    //Complete function here ...
  }

  // Return a response to acknowledge receipt of the event
  response.json({received: true});
});
danlupascu commented 5 years ago

for anyone still struggling... this is what worked for me.

...
var app = express();
//This is specific raw body for /webhooks/stripe route 
app.use('/webhooks/stripe', bodyParser.raw({type: "*/*"}))
//This is normal body parsing for everything else
app.use(bodyParser.json());

...
//later on..

const stripe = require('stripe')('sk_..');
const endpointSecret = 'whsec...';

app.post('/webhooks/stripe', (request, response) => {
  const sig = request.headers['stripe-signature'];
  let event;
  try {
    event = stripe.webhooks.constructEvent(request.body, sig, endpointSecret);
  } catch (err) {
    return response.status(400).send(`Webhook Error: ${err.message}`);
  }

  // Handle the checkout.session.completed event
  if (event.type === 'checkout.session.completed') {
    const session = event.data.object;

    //Complete function here ...
  }

  // Return a response to acknowledge receipt of the event
  response.json({received: true});
});

this worked great for me! Thank you!!

filipbarak commented 4 years ago

for anyone still struggling... this is what worked for me.

...
var app = express();
//This is specific raw body for /webhooks/stripe route 
app.use('/webhooks/stripe', bodyParser.raw({type: "*/*"}))
//This is normal body parsing for everything else
app.use(bodyParser.json());

...
//later on..

const stripe = require('stripe')('sk_..');
const endpointSecret = 'whsec...';

app.post('/webhooks/stripe', (request, response) => {
  const sig = request.headers['stripe-signature'];
  let event;
  try {
    event = stripe.webhooks.constructEvent(request.body, sig, endpointSecret);
  } catch (err) {
    return response.status(400).send(`Webhook Error: ${err.message}`);
  }

  // Handle the checkout.session.completed event
  if (event.type === 'checkout.session.completed') {
    const session = event.data.object;

    //Complete function here ...
  }

  // Return a response to acknowledge receipt of the event
  response.json({received: true});
});

weirdly worked for me even though it's very similar to all that I've tried before!

Thanks!

pincheira commented 4 years ago

@joshbedo I guess the bodyParser needs to be set before.

I do it like this:


  const setupForStripeWebhooks = {
    // Because Stripe needs the raw body, we compute it but only when hitting the Stripe callback URL.
    verify: function (req, res, buf) {
      var url = req.originalUrl;
      if (url.startsWith('/webhooks/stripe')) {
        req.rawBody = buf.toString();
      }
    }
  };
  app.use(bodyParser.json(setupForStripeWebhooks));

I hope this snippet can help.