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

fix: handle preparsed body payloads in verifyAndReceive accordingly #962

Closed
wants to merge 2 commits into from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jan 31, 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! 🚀

Comment on lines +18 to +20
typeof event.payload === "string"
? event.payload
: JSON.stringify(event.payload),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could cause problems if GitHub ever changes payload serialization, so the signature would not match...

We should check if it's possible to disable this parsing in Vercel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#963 should be a better implementation based on that guide

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no preference. But you should add a test to your PR.

@wolfy1339
Copy link
Member

wolfy1339 commented Feb 1, 2024

@Uzlopak from the discussion in #963, I think we might need to add the typeof condition here: https://github.com/probot/probot/blob/1eb03896e9772312c84f0be3da7bf3c0910797b6/src/context.ts#L72

I'm sorry if I'm wrong, but looking at the source code, my mind tells me that it is the place from where the request body is passed to this SDK.

That is the Context class, it doesn't call the webhooks at all. It only augments it.

The webhooks aren't really called anywhere, we just pass through the data from the integrated web server and do the handliin this library:
https://github.com/probot/probot/blob/1eb03896e9772312c84f0be3da7bf3c0910797b6/src/create-node-middleware.ts#L14

@wolfy1339
Copy link
Member

Closing as per discussion in #963

@wolfy1339 wolfy1339 closed this Feb 2, 2024
@Uzlopak Uzlopak deleted the fix-vercel branch February 2, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: Preparsed payloads are not handled correctly, here Vercel
4 participants