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

Added type check to webhook signature #10

Merged
merged 9 commits into from
May 28, 2021
Merged

Added type check to webhook signature #10

merged 9 commits into from
May 28, 2021

Conversation

tbobkermux
Copy link
Contributor

I've implemented a conditional check to see if the signature header is a string or if it is an array and removed the error to upgrade axios package to the latest version.

Copy link
Collaborator

@erikpena erikpena left a comment

Choose a reason for hiding this comment

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

Would of helped if I click the "submit review" button 🙄

controllers/mux.ts Outdated Show resolved Hide resolved
controllers/mux.ts Outdated Show resolved Hide resolved
controllers/mux.ts Outdated Show resolved Hide resolved
@dylanjha
Copy link

This is unrelated to the PR, but I noticed we're doing JSON.stringify(body) here to get the raw webhook request body. Is there a way we can access the raw body directly?

Webhooks.verifyHeader(JSON.stringify(body), sig, config.webhook_signing_secret);

This is potentially problematic because of the following scenario

  1. Mux creates the signature based off the raw stringified JSON body
  2. The webserver that receives the request parses the string and turns it into a JSON object
  3. Application code does JSON.stringify on the JSON object to convert it back into a string

The problem is that the string in (1) and the string in (3) should be the same, but they're not guaranteed to be, and if they are not exactly the same, then the signature verification will fail.

Getting the raw request body eliminates the risk of this.

This is a common problem, and we document how to handle this in the node SDK: https://github.com/muxinc/mux-node-sdk#verifying-webhook-signatures and the elixir SDK: https://github.com/muxinc/mux-elixir#usage-in-phoenix

@erikpena
Copy link
Collaborator

erikpena commented May 1, 2021

This is unrelated to the PR, but I noticed we're doing JSON.stringify(body) here to get the raw webhook request body. Is there a way we can access the raw body directly?

Webhooks.verifyHeader(JSON.stringify(body), sig, config.webhook_signing_secret);

This is potentially problematic because of the following scenario

  1. Mux creates the signature based off the raw stringified JSON body
  2. The webserver that receives the request parses the string and turns it into a JSON object
  3. Application code does JSON.stringify on the JSON object to convert it back into a string

The problem is that the string in (1) and the string in (3) should be the same, but they're not guaranteed to be, and if they are not exactly the same, then the signature verification will fail.

Getting the raw request body eliminates the risk of this.

This is a common problem, and we document how to handle this in the node SDK: https://github.com/muxinc/mux-node-sdk#verifying-webhook-signatures and the elixir SDK: https://github.com/muxinc/mux-elixir#usage-in-phoenix

Great question. What I can say is that Strapi's request apis are based on Koa.js (https://koajs.com/). It might be possible to capture the raw body and grab the string up front. That would have to be looked at. But I agree, it's possible that stringifying a json object might change the order of the properties.

- Upgraded strapi peer dependency
- Removed unneeded strapi-helper-plugin (it's global)
- Upgraded strapi-utils dependency
- Implemented proper strapi lang interpolation for strings
- Introduced permissions scoping (currently, all admins can use)
@erikpena
Copy link
Collaborator

Quick update, so the webhooks bit looks like it is addressed. However, and with more concern, it seems that something has changed with how Strapi handles plugins and exposing settings in the admin dashboard. What's happening is that "General" links in the Strapi dashboard will show initially allowing a user to go in and configure Strapi (including the Mux Video Uploader). However, after saving, the whole "General" section on the left menu of Strapi disappears. This includes Marketplace, Plugins and all of the settings (not just Mux Video Uploader).

We've reached out to Strapi to see if we can get some understanding as to what is happening here. Will keep this thread apprised!

- Establish plugin permissions for settings and main using bootstrap
- Updated models and services to use new model schemes
- plugins are formatted to remove the `strapi-plugin-` for npm installs
- Commented out webhook signature check (see README.md)
- Updated README.md with notes on webhook caveat
- Update README.md with more accurate permission explanation
@erikpena
Copy link
Collaborator

With Strapi's help, we were able to resolve the permission issues and it's working 🎉 .

In summary, here is what we've done:

@tbobkermux could you run through some testing before we merge this in?

- Added steps for publishing release
@erikpena erikpena merged commit 04b5819 into master May 28, 2021
@erikpena erikpena deleted the tb/sig-type-issue branch May 28, 2021 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants