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

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

Closed
1 task done
didley opened this issue Apr 27, 2024 · 4 comments · Fixed by #1010
Closed
1 task done
Labels
released Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented, or is being fixed

Comments

@didley
Copy link
Contributor

didley commented Apr 27, 2024

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] v12.0.9...v12.0.10

Versions

webhooks.js 13.2.6

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's 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.

@didley didley added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented, or is being fixed labels Apr 27, 2024
Copy link
Contributor

👋 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! 🚀

@kfcampbell kfcampbell moved this from 🆕 Triage to 🔥 Backlog in 🧰 Octokit Active Apr 29, 2024
@kfcampbell kfcampbell added Status: Up for grabs Issues that are ready to be worked on by anyone and removed Status: Triage This is being looked at and prioritized labels Apr 29, 2024
@Uzlopak
Copy link
Contributor

Uzlopak commented May 2, 2024

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
Copy link

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-project-automation github-project-automation bot moved this from 🔥 Backlog to ✅ Done in 🧰 Octokit Active May 6, 2024
Copy link
Contributor

github-actions bot commented May 6, 2024

🎉 This issue has been resolved in version 13.2.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented, or is being fixed
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants