developmentseed / jekyll-hook

No Longer Maintained | A server that listens for GitHub webhook posts and renders a Jekyll site
BSD 3-Clause "New" or "Revised" License
508 stars 83 forks source link

Seems to only works on first webhook request #21

Closed Caffe1neAdd1ct closed 9 years ago

Caffe1neAdd1ct commented 9 years ago

Hi,

Testing out jekyll-hook, great idea and works perfectly the first time it receives a webhook. On all subsequent hook requests it fails with a 403 on github with the following message in terminal:

TypeError: HmacUpdate fail at Hmac.Hash.update (crypto.js:209:17) at app.use.express.bodyParser.verify (/home/redmine/jekyll-hook/jekyll-hook.js:27:33) at /home/redmine/jekyll-hook/node_modules/express/node_modules/connect/lib/middleware/json.js:52:5 at /home/redmine/jekyll-hook/node_modules/express/node_modules/connect/node_modules/body-parser/lib/read.js:79:9 at IncomingMessage.onEnd (/home/redmine/jekyll-hook/node_modules/express/node_modules/connect/node_modules/body-parser/node_modules/raw-body/index.js:136:7) at IncomingMessage.g (events.js:180:16) at IncomingMessage.emit (events.js:92:17) at _stream_readable.js:929:16 at process._tickCallback (node.js:419:13)

I've tried running both through forever and ./ and i get the same result. Let me know if there is any further info i can give which may help to diagnose this.

Caffe1neAdd1ct commented 9 years ago

Definitely failing on lines 28 -> 33 where the request sig is verified. Not sure i'll be able to solve this one, but i'll update with any progress / info.

Caffe1neAdd1ct commented 9 years ago

Moved the following line:

var hmac = crypto.createHmac('sha1', config.secret);

inside the

app.use(express.bodyParser({ verify: function(req,res,buffer){

block and used console.log to view config.secret, just wanted to make sure it was being fetched / set. Weirdly the second push hook request to jekyll-hook has worked, plus a few more consecutive requests. So the question is why doesn't this line work correctly when up with the other var's being set?

Caffe1neAdd1ct commented 9 years ago

Raised a pull request to show the change which fixed this issue for me.

scisco commented 9 years ago

The pull request was merged. Thanks for catching the bug.