adding failing test for request.body

[dylancarlone]

This test shows that the middleware will hang when request.body is created by consuming the request data stream.

closes #40

[dylancarlone]

I was actually able to get this to work just fine by wrapping the middleware and firing events after the middleware has subscribed:

function (request, response) {
  webhooks.middleware(request, response);

  if (request.body) {
    request.emit('data', Buffer.from(JSON.stringify(request.body)));
    request.emit('end');
  }
}

This would be an easy fix, whether or not it's implemented inside the middleware itself.

[gr2m]

sounds good to me 👍 sorry for the long response times, I’m mostly out until April 22nd

[dylancarlone]

I added the fix and the tests are now passing. If you don't think this is the way to do it or would prefer to hold off, I'm happy just refiring the events in my code and leaving the package be.

[BerkeleyTrue]

Well, just opened an issue for this. (#40)

ping @dylancarlone

[dylancarlone]

@BerkeleyTrue I had success duplicating the logic I added to this PR in my own app:

webhooks.js/middleware/middleware.js

Line 70 in 41c0930

if (request.body) {

My problem initially came from FireBase parsing the request body before any of my own code ran.

[nbransby]

@dylancarlone do you mean firebase functions? Im getting a timeout in firebase functions on calling verifyAndReceive, is this related?

[gr2m]

I’ve rebased the PR, so this is ready to merge. I can’t think of any disadvantages of merging this. Does anyone have any objections or concerns?

[nbransby]

Not from me.. I'm waiting for it!

[gr2m]

I wonder, is there a need to emit the events, why not just call verifyAndReceive directly?

  if (request.body) {
    verifyAndReceive(state, {
      id: id,
      name: eventName,
      payload: request.body,
      signature
    })

      .then(() => {
        response.end('ok\n')
      })

      .catch(error => {
        response.statusCode = error.status || 500
        response.end(error.toString())
      })
  }

[gr2m]

See my change at b380f87 which omits re-emitting the "data" event on the request object. Test is still passing, so unless anyone objects I’ll go ahead with that :)

[gr2m]

@dylancarlone @nbransby I’ve updated the implementation. Could you check if it works with Firebase functions now?

[nbransby]

Works for me

[nbransby]

Sorry but it would appear its still hanging on master for me my firebase function is defined as:

export const webhook = https.onRequest((req, resp) => webhooks.verifyAndReceive({
    id: req.headers['x-github-delivery'],
    name: req.headers['x-github-event'],
    payload: req.body,
    signature: req.headers['x-hub-signature']
}));

results in

Function execution took 60002 ms, finished with status: 'timeout'

[gr2m]

This seems unrelated to this pull request. You use the . verifyAndReceive method directly, while this PR was about the http middleware.

I don’t know firebase functions, but from these docs: https://firebase.google.com/docs/functions/http-events it looks like you still have to send a response, in your code example you don’t do anything with the resp parameter.

Maybe try this instead:

export const webhook = https.onRequest(webhooks.middleware);

[octokitbot]

🎉 This PR is included in version 5.0.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀