search

octokit / webhooks.js

GitHub webhook events toolset for Node.js
MIT License
307 stars 80 forks source link

[BUG]: webhooks.js v12.0.10 onward breaks compatibility with @fastify/middie middleware library #1009

Closed didley closed 2 months ago

didley commented 2 months ago

What happened?

Due to a conditional change within getPayload() from if(request.body) to if("body" in request) compatibility with the middleware library @fastify/meddie library is broken. Although this may be an upstream issue with how Middie parses the request, this is an easy fix that was an existing behaviour prior to v12.0.10[0].

If request.body is undefined, Middie parses this as a defined key with undefined value as apposed to not indulging the body property like node:http and express do.

I've only inspected middleware request body handling with node:http, express, @fastify/express and @fastify/middie but this may impact other libraries other than @fastify/middie.

[0] https://github.com/octokit/webhooks.js/compare/v12.0.9...v12.0.10

Versions

webhooks.js 13.2.6

Relevant log output

No response

Code of Conduct

UPDATE: This may be being fixed upstream within @fastify/middie, if attempting to reproduce this bug please use a Middie <=8.3.0. I think the @octokit/webhooks patch should still be completed as this could be quite difficult to diagnose if the user is on a <=8.3.0 of Middie or if they have constraints for upgrading it.

github-actions[bot] commented 2 months ago

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

Uzlopak commented 2 months ago

Hmm.

@gurgunday what is faster?

if (request.body), if ('body' in request) or if (typeof request.body !== 'undefined') Wasnt there some Special bytecode for typeof checks?

gurgunday commented 2 months ago

Not sure about in but first one has JumpIfToBooleanFlase and the third as you said has CheckTypeOf

There is also TestUndefined for exp !== undefined I think

Don't really know which is faster implementation wise, but I'm guessing if TestUndefined specifically exists, it should be as good as it gets

github-actions[bot] commented 2 months ago

:tada: This issue has been resolved in version 13.2.7 :tada:

The release is available on:

Your semantic-release bot :package::rocket: