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

Unable to verify headers using @sendgrid/eventwebhook lib #1322

Closed
abonnell opened this issue Dec 10, 2021 · 6 comments · Fixed by #1340
Closed

Unable to verify headers using @sendgrid/eventwebhook lib #1322

abonnell opened this issue Dec 10, 2021 · 6 comments · Fixed by #1340
Labels
status: waiting for feedback waiting for feedback from the submitter type: question question directed at the library

Comments

@abonnell
Copy link

abonnell commented Dec 10, 2021

Issue Summary

When attempting to use the @sendgrid/eventwebhook methods to verify Sendgrid signed event headers the method verifySignature() always returns false. Below code is written as an expressJS middleware function to run when the webhook sends a POST call to our endpoint.

Steps to Reproduce

  1. Set up a server to receive POST data from the Sendgrid webhooks and enable signed event headers
  2. Send a Webhook test event to your handler and note the false result

Expected response from method is true

Code Snippet

const { EventWebhook, EventWebhookHeader } = require('@sendgrid/eventwebhook');

const pubkey = 'string pubkey';

const verifySendgridWebhook = (req, res, next) => {
  console.log('Verifying webhook headers...')

  const webhook = new EventWebhook();
  const webhookHeaders = new EventWebhookHeader();

  const headers = req.headers;
  const headerSig = headers[EventWebhookHeader.SIGNATURE().toLowerCase()];
  const headerTimestamp = headers[EventWebhookHeader.TIMESTAMP().toLowerCase()];

  const buffer = Buffer.from(JSON.stringify(req.body));
  const ecpk = webhook.convertPublicKeyToECDSA(pubkey);
  const verified = webhook.verifySignature(ecpk, buffer.toString(), headerSig, headerTimestamp)

  if (verified) {
    console.log('Verified headers');
    next();
  } else {
    console.log('Invalid headers. 401 unauthorized.');
    res.status(401).send("Unauthorized");
  }
}

Technical details:

  • sendgrid-nodejs version: "@sendgrid/eventwebhook": "^7.4.5"
  • node version: v12.18.2 OR v16.13.1 (tried both)
@lisacopeland
Copy link

I am having this same problem - I am on the same version of sendgrid-nodejs and I used the sample code from https://www.twilio.com/docs/runtime/quickstart/validate-webhook-requests-from-sendgrid

@Angel2367716
Copy link

Has anyone found a fix for this? I'm also having the same issues.

@childish-sambino
Copy link
Contributor

Do the changes in the linked PR help to validate the payload?

@childish-sambino childish-sambino added status: waiting for feedback waiting for feedback from the submitter type: question question directed at the library labels Feb 18, 2022
@danmana
Copy link
Contributor

danmana commented Feb 23, 2022

From my experience, it is very important that the request body is not parsed as JSON or string, and is the raw Buffer!

This is the reason that the "hack" with JSON.stringify() and new line replacements from the linked pr seems to work, but it is not 100% correct!
The signature sent by the server is computed on the raw body. Any json parsing and stringify operations are not guaranteed to produce identical results!

The correct way to do it is to make sure that body-parser.json does not parse this webhook endpoint at all.

For example, this is the setup I have and is working 100% (I cleaned it up a bit to share it here).
Note: I use express-unless to make sure that body-parser.json does not process this endpoint, and I use body-parser.raw instead.

const unless = require("express-unless");
const bodyParser = require("body-parser");
const { EventWebhook, EventWebhookHeader } = require("@sendgrid/eventwebhook");

// Important: apply body-parser json for all endpoints EXCEPT the webhook!
const parse_json = bodyParser.json();
parse_json.unless = unless;
app.use(
  parse_json.unless({
    path: ["/api/webhooks/sendgrid"],
  })
);

app.post(
  "/api/webhooks/sendgrid",
  bodyParser.raw({ type: "application/json" }), // Important: parse body as raw Buffer
  (req, res) => {
    console.log("Is Buffer?", Buffer.isBuffer(req.body)); // must return true !

    const signature = req.get(EventWebhookHeader.SIGNATURE());
    const timestamp = req.get(EventWebhookHeader.TIMESTAMP());
    const eventWebhook = new EventWebhook();
    const key = eventWebhook.convertPublicKeyToECDSA(pubkey);

    if (eventWebhook.verifySignature(key, req.body, signature, timestamp)) {
      const events = JSON.parse(req.body);
      // TODO: handle events
      res.sendStatus(204);
    } else {
      res.status(401).json({ message: "Invalid signature" });
    }
  }
);

@eroo36
Copy link

eroo36 commented Feb 23, 2022

@danmana
Hi, just a question,
Is bodyParser.raw({ type: "application/json" }) the middleware supposed to override app.use(bodyParser.json())that was used before the endpoint? Seems like it does not override it from your solution.

And also do we have to use the body-parser package? It seems it is deprecated and is now included in express package.express.raw() could be used as well, I guess?
Could close the PR if you create a new one with your solution, but seems they are not really checking it.

@danmana
Copy link
Contributor

danmana commented Feb 23, 2022

@eroo36

Yes, we are in fact "overriding" the middleware for the webhook.

This is a global middleware that applies to all paths, except the webhook - see unless.
Most people use incorrectly app.use(bodyParser.json()) which applies it to all routes, and is impossible to override. you should use this instead:

app.use(
  parse_json.unless({
    path: ["/api/webhooks/sendgrid"],
  })
);

This is a middleware applied just to the webhook path (nothing to override as we excluded this path from the global middleware using unless).

app.post(
  "/api/webhooks/sendgrid",
  bodyParser.raw({ type: "application/json" }), // Important: parse body as raw Buffer

Yes you can replace bodyParser.json() with express.json() and bodyparser.raw() with express.raw().
In fact, that's what I'm using, I just figured most people were still using the old version (which is not wrong in any way)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for feedback waiting for feedback from the submitter type: question question directed at the library
Projects
None yet
6 participants