feat: add webhook verify utility function to node #457

mjcuva · 2022-05-16T23:03:15Z

Added a utility function to make verifying that a webhook request is legitimately from ReadMe easier!

Currently we recommend users have the following code in their endpoint:

  // Verify signature
  const [fullMatch, time, readmeSignature] = /t\=(.*),v0\=(.*)/.exec(req.headers["readme-signature"]);
  
  const unsigned = `${time}.${JSON.stringify(body)}`;
  const hmac = crypto.createHmac('sha256', 'PROJECT_SECRET');
  const verifySignature = hmac.update(unsigned).digest('hex');
  if (verifySignature !== readmeSignature) {
    return {};
  }

This moves this logic to a readme.verify function so we don't have to explain the security of how this weeks (though we should keep the steps for doing this manually in the docs like stripe does).

With this new function the code will look like this instead:

  // Verifies the request is legitimate and came from ReadMe
  const signature = req.headers['readme-signature'];
  const secret = '12321313'
  try {
    readme.verify(req.body, signature, secret);
  } catch (e) {
   // INVALID
    return res.sendStatus(401);
  }

Even though the number of lines of code is similar, it's a lot less complex. I also added code in readme.verify that checks the timestamp is recent to mitigate against replay attacks, which we should probably also be doing in the manual version.

We should also do this in the rest of the metrics sdks as well!

domharrington

I agree with this in principal for sure! Few comments inline. Can you add some docs for this? Wanna take a stab at adding for other languages, or do you think it's okay to merge in for JS initially?

domharrington · 2022-05-17T09:45:56Z

packages/node/src/lib/webhook-verify.ts

+  email: String;
+}
+
+export function verify(body: WebhookBody, signature: string, secret: string): WebhookBody {


How would you feel about calling this verifyWebhook() instead? verify on it's own is a little generic.

packages/node/src/lib/webhook-verify.ts

This is in core and doesnt need to come from npm

This test was throwing an error, but not the Expired Signature error that we were expecting. It was throwing because we were passing in `new Date()` into the test instead of `Date.now()`. I've fixed all of the tests here, and ensured that we're casting to int when constructing the new Date() in verify()

__tests__/integration-webhooks.test.js

+  });
+}
+
+const randomApiKey = 'rdme_abcdefghijklmnopqrstuvwxyz';


packages/node/__tests__/lib/webhook-verify.test.ts

packages/node/examples/express/webhook.js

+app.post('/webhook', express.json({ type: 'application/json' }), (req, res) => {
+  // Verify the request is legitimate and came from ReadMe
+  const signature = req.headers['readme-signature'];
+  // Your ReadMe secret
+  const secret = process.env.README_API_KEY;
+  try {
+    readme.verify(req.body, signature, secret);
+  } catch (e) {
+    // Handle invalid requests
+    return res.sendStatus(401);
+  }
+  // Fetch the user from the db
+  db.find({ email: req.body.email }).then(user => {
+    return res.json({
+      // OAS Security variables
+      petstore_auth: 'default-key',
+      basic_auth: { user: 'user', pass: 'pass' },
+    });
+  });
+});


Inspired by stripe-node

domharrington

I'm pretty happy about this apart from two things:

Stripe uses v1= as their live scheme to hold the data - do we wanna replicate this? https://stripe.com/docs/webhooks/signatures#verify-manually
Naming of the function - should it be verify() or webhookVerify() or verifyWebhook()? I could go either way.

packages/node/src/lib/webhook-verify.ts

mjcuva · 2022-07-18T22:55:05Z

@domharrington Maybe we should just go with verifyWebhook? I kinda like the explicitness I think?

Other than my one small comment I'm happy with this!