Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Verify GitHub webhook signature header in Node.js
/*
* Verify GitHub webhook signature header in Node.js
* Written by stigok and others (see gist link for contributor comments)
* https://gist.github.com/stigok/57d075c1cf2a609cb758898c0b202428
* Licensed CC0 1.0 Universal
*/
const crypto = require('crypto')
const express = require('express')
const bodyParser = require('body-parser')
const secret = CHANGE_ME;
// For these headers, a sigHashAlg of sha1 must be used instead of sha256
// GitHub: X-Hub-Signature
// Gogs: X-Gogs-Signature
const sigHeaderName = 'X-Hub-Signature-256'
const sigHashAlg = 'sha256'
const app = express()
// Saves a valid raw JSON body to req.rawBody
// Credits to https://stackoverflow.com/a/35651853/90674
app.use(bodyParser.json({
verify: (req, res, buf, encoding) => {
if (buf && buf.length) {
req.rawBody = buf.toString(encoding || 'utf8');
}
},
}))
function verifyPostData(req, res, next) {
if (!req.rawBody) {
return next('Request body empty')
}
const sig = Buffer.from(req.get(sigHeaderName) || '', 'utf8')
const hmac = crypto.createHmac(sigHashAlg, secret)
const digest = Buffer.from(sigHashAlg + '=' + hmac.update(req.rawBody).digest('hex'), 'utf8')
if (sig.length !== digest.length || !crypto.timingSafeEqual(digest, sig)) {
return next(`Request body digest (${digest}) did not match ${sigHeaderName} (${sig})`)
}
return next()
}
app.post('/', verifyPostData, function (req, res) {
res.status(200).send('Request body was signed')
})
app.use((err, req, res, next) => {
if (err) console.error(err)
res.status(403).send('Request body was not signed or verification failed')
})
app.listen(3000, () => console.log("Listening on port 3000"))
@ProductOfAmerica

This comment has been minimized.

Copy link

@ProductOfAmerica ProductOfAmerica commented Nov 1, 2018

Thanks man!

@TuanMinPay

This comment has been minimized.

Copy link

@TuanMinPay TuanMinPay commented Jul 3, 2019

thanks

@theBliz

This comment has been minimized.

Copy link

@theBliz theBliz commented Jul 16, 2019

for future reference, const sigHeaderName = 'X-Hub-Signature' should be const sigHeaderName = 'x-hub-signature'

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Jul 16, 2019

@theBliz: You may be right. It highlights that it should get a header case-insensitive. Replaced req.headers[key] with req.get(key). Thanks!

@theBliz

This comment has been minimized.

Copy link

@theBliz theBliz commented Jul 16, 2019

@stigok just checked and I can confirm that with req.get(sigHeaderName) it works smoothly :)

Unrelated question: I don't get why you do return next('error message'). How do you handle the logging for those? Can you explain to me the reasoning behind?

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Jul 16, 2019

@theBliz: It was supposed to be an example implementation where you'd handle errors yourself. I added a log statement now for brevity ;)

@theBliz

This comment has been minimized.

Copy link

@theBliz theBliz commented Jul 16, 2019

lol
I thought it was some cool strategy I didn't know about! Thanks for the clarification :D

@jpsear

This comment has been minimized.

Copy link

@jpsear jpsear commented Jan 6, 2020

@stigok Thank you for this — very useful!

@animir

This comment has been minimized.

Copy link

@animir animir commented Jan 24, 2020

@stigok Thanks for the example.

It is important to use timing safe signatures comparing against timing attacks:

  const bufferDigest = Buffer.from(digest, 'utf8');
  const bufferChecksum = Buffer.from(checksum, 'utf8');

  if (!crypto.timingSafeEqual(bufferDigest, bufferChecksum)) {
    return res.status(403).end()
  }
@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Jan 24, 2020

@animir cheers! I just published an update. It is also important to compare the lengths of the buffers before calling timingSafeEqual to avoid exception being thrown.

@Juriy

This comment has been minimized.

Copy link

@Juriy Juriy commented Feb 7, 2020

@stigok why are we using 'utf-8' encoding in one place to convert from string to buffer, but then we use "hex" in the other? I think it should be "hex" everywhere.

// digest is constructed with "hex"
const digest = Buffer.from('sha1=' + hmac.update(payload).digest('hex'), 'utf8')

// checksum is constructed with "utf8"
const checksum = Buffer.from(sig, 'utf8')

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Feb 7, 2020

@Juriy it's because we have to create Buffers to compare them with crypto.timingSafeEqual. The text that is prepended to the hex digest (sha1=) is not hex itself: Buffer.from('sha1=' + hmac.update(payload).digest('hex'), 'utf8'). If you think I'm misunderstanding you, please provide example code to what you are proposing.

@tpb1962

This comment has been minimized.

Copy link

@tpb1962 tpb1962 commented Jun 8, 2020

Hi. I'm new to node.js. I had to do a proof of concept to process a webhook with Azure Web App and Node,js. By using the code as is, besides the correct secret, it worked great. Thanks! I do get this warning and I'm not sure why.

(node:10332) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

I used Visual Studio 19 and pushed to Azure Web App. Any ideas why I receive most appreciated.

Thanks,

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Jun 8, 2020

@tpb1962 this is probably not coming from a verbatim copy of this script. If you read the source, we are not using Buffer() directly, but are in fact using Buffer.from() as the warning recommends. If you still see this warning, please provide your local Node.js version information.

@tpb1962

This comment has been minimized.

Copy link

@tpb1962 tpb1962 commented Jun 8, 2020

Hi,

Code is copy and paste except for the secret line and the last line for the port ( app.listen(process.env.port); ) . Node version is 10.6.1 . I have attached some screenshots. I figured it may be because of something with running in Azure but not sure. It works fine just get that warning and as you said no "buffer" statements that should be flagged are being used.

`/*

const crypto = require('crypto')
const express = require('express')
const bodyParser = require('body-parser')

const secret = 'TimsSecret';

// GitHub: X-Hub-Signature
// Gogs: X-Gogs-Signature
const sigHeaderName = 'X-Hub-Signature'

const app = express()
app.use(bodyParser.json())

function verifyPostData(req, res, next) {
const payload = JSON.stringify(req.body)
if (!payload) {
return next('Request body empty')
}

const sig = req.get(sigHeaderName) || ''
const hmac = crypto.createHmac('sha1', secret)
const digest = Buffer.from('sha1=' + hmac.update(payload).digest('hex'), 'utf8')
const checksum = Buffer.from(sig, 'utf8')

if (checksum.length !== digest.length || !crypto.timingSafeEqual(digest, checksum)) {
    return next(`Request body digest (${digest}) did not match ${sigHeaderName} (${checksum})`)
}
return next()

}

app.post('/', verifyPostData, function (req, res) {
res.status(200).send('Request body was signed')
})

app.use((err, req, res, next) => {
if (err) console.error(err)
res.status(403).send('Request body was not signed or verification failed')
})

//app.listen(3000)
app.listen(process.env.port);
`

Thanks
nodehook1

@tpb1962

This comment has been minimized.

Copy link

@tpb1962 tpb1962 commented Jun 8, 2020

Sorry, it is 10.16.1. Package.json updated as well. Republished and still got the warning.

Thanks

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Jun 8, 2020

I am unable to reproduce this on Linux with Node.js v10.16.1 and express@4.17.1. Maybe this is some Windows-specific issue. Good luck :)

@tpb1962

This comment has been minimized.

Copy link

@tpb1962 tpb1962 commented Jun 8, 2020

@stigok - I'd say you are right. Thanks for checking!

@BrunoBlanes

This comment has been minimized.

Copy link

@BrunoBlanes BrunoBlanes commented Jul 15, 2020

Thanks.

@roastedmonk

This comment has been minimized.

Copy link

@roastedmonk roastedmonk commented Aug 6, 2020

How "Length Extension Attack" is mitigated in this code?

@nirsky

This comment has been minimized.

Copy link

@nirsky nirsky commented Aug 10, 2020

Thank you for this! Worked perfectly.

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Aug 10, 2020

@roaste

How "Length Extension Attack" is mitigated in this code?

This solution is using HMAC, which is not susceptible to length extension attacks.

Note that since HMAC doesn't use this construction, HMAC hashes are not prone to length extension attacks.[5]
Wikipedia

@pimverkerk

This comment has been minimized.

Copy link

@pimverkerk pimverkerk commented Aug 24, 2020

Thank for the help.
For me it only worked when using req.body instead of JSON.stringify(req.body) when calling hmac.update(payload)

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Aug 25, 2020

@pimverkerk that sounds strange. When using bodyParser.json() it should only be parsing requests with Content-Type: application/json, and req.body should be a JavaScript object. Are you using GitHub webhooks, and otherwise using a verbatim copy of this code?

@ludcila

This comment has been minimized.

Copy link

@ludcila ludcila commented Aug 26, 2020

Hi @stigok, thanks for sharing this!

I used this code for a different purpose, not for Github webhooks. In my case, I found that stringifying the parsed body does not necessarily return the exact original request body. Consider for example JSON.stringify(JSON.parse("{\"value\":1.0}")), which returns {"value":1}. I don't know if this happens with the particular data sent by the Github webhooks, but if that is the case, I found the solutions here to be helpful https://stackoverflow.com/a/35651853/1531992

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Aug 26, 2020

@ludcila oh, I see. So this can become a problem when verifying the signature. In JavaScript (1 === 1.0) === true, and 1.0 will be represented as 1 between de- and serialisation. Thanks for reporting. I'll see what I can do here.

@pistazie

This comment has been minimized.

Copy link

@pistazie pistazie commented Jan 25, 2021

works with sha256 too.

@andrefcdias

This comment has been minimized.

Copy link

@andrefcdias andrefcdias commented Feb 16, 2021

I would suggest changing it to sha256 and using the header X-Hub-Signature-256 as it's the recommendation from GitHub

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Feb 16, 2021

@andrefcdias could you please post a reference?

@andrefcdias

This comment has been minimized.

Copy link

@andrefcdias andrefcdias commented Feb 16, 2021

@stigok sure! Here's the docs

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Feb 23, 2021

I would suggest changing it to sha256 and using the header X-Hub-Signature-256 as it's the recommendation from GitHub

@andrefcdias thank you! I've updated the code with new header name and hash alg.

@stigok

This comment has been minimized.

Copy link
Owner Author

@stigok stigok commented Feb 23, 2021

Hi @stigok, thanks for sharing this!

I used this code for a different purpose, not for Github webhooks. In my case, I found that stringifying the parsed body does not necessarily return the exact original request body. Consider for example JSON.stringify(JSON.parse("{\"value\":1.0}")), which returns {"value":1}. I don't know if this happens with the particular data sent by the Github webhooks, but if that is the case, I found the solutions here to be helpful https://stackoverflow.com/a/35651853/1531992

Thanks again! Now imlemented!

@boombang

This comment has been minimized.

Copy link

@boombang boombang commented Feb 25, 2021

thank you :)

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