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

Webhook signature validation fails for Meetings with non-ASCII chars in the topic #349

Closed
LucasMetal opened this issue Jul 9, 2024 · 4 comments
Assignees
Labels
Bug This change resolves a defect
Milestone

Comments

@LucasMetal
Copy link

LucasMetal commented Jul 9, 2024

Webhook calls sent by Zoom contain the topic of the meeting in their body, and when that topic contains non-ASCII chars the signature validation fails.

Example webhook call body:

{
    "event":"meeting.ended",
    "payload":
    {
        "account_id":"XXXXXXX",
        "object":
        {
            "duration":60,
            "start_time":"2024-07-09T17:51:44Z",
            "timezone":"UTC",
            "end_time":"2024-07-09T17:51:58Z",
            "topic":"My Test",
            "id":"55555555555",
            "type":2,
            "uuid":"xxxxxxxxxxxxxxxxxxxxxxxx",
            "host_id":"xxxxxxxxxxxxxxxxxxxxxx"
        }
    },
    "event_ts":1720547519023
}

Repro steps:

  • Create a sample app with code to process webhooks (including the signature validation, etc) as per the examples in doc
  • Configure the environment in Zoom, etc, to receive some webhooks for meeting started, meeting ended, etc.
  • Create a Meeting with non-ASCII chars in the topic, example: "Test 🚒🚒 ? - ’ - – 🚗 HOLA"
  • Take an action to fire a webhook (start the meeting, end it, etc)
  • You'll get a "Webhook signature validation failed." exception in the sample app.

Root of the problem

I have done some debugging and testing, and the problem is in WebhookParser.cs file, line 37.
var hashAsBytes = hmac.ComputeHash(Encoding.ASCII.GetBytes(message));

that line doesn't consider possible non-ASCII chars in the body, thus in the end generating a different signature than Zoom does. So the validation fails.

Possible fix

Changing the previosly mentioned line to:
var hashAsBytes = hmac.ComputeHash(Encoding.UTF8.GetBytes(message));
seems to fix the issue.

I wasn't able to create a test for this case (there are no tests for webhook signature validations), but I can send a PR with the change if needed (although maybe it's not worth it).

Temporal workaround

For anyone that needs a quick workaround, I was able to get away with this:

private string Sanitize(string topic)
{
    // https://stackoverflow.com/a/123340/5478655
    // Keep only printable ASCII chars
    return Regex.Replace(topic, @"[^\u0020-\u007E]+", string.Empty);
}

Thanks for all the effort put in this great library !!

@Jericho
Copy link
Owner

Jericho commented Jul 10, 2024

Thanks for reporting this issue and, most importantly, for investing the time to investigate and suggest a possible solution.

I have done some debugging and testing, and the problem is in WebhookParser.cs file, line 37. var hashAsBytes = hmac.ComputeHash(Encoding.ASCII.GetBytes(message)); that line doesn't consider possible non-ASCII chars in the body, thus in the end generating a different signature than Zoom does. So the validation fails.

At first glance, what you are saying makes sense but I seem to recall that I tried UTF8 when I worked on this feature and something didn't work as expected. But that was 2 years ago so I may not be remembering correctly. I'll be away all day tomorrow but I'll invest time to test your suggestion when I come back.

@Jericho Jericho self-assigned this Jul 11, 2024
@Jericho Jericho added the Bug This change resolves a defect label Jul 11, 2024
@Jericho Jericho added this to the 0.78.0 milestone Jul 11, 2024
@Jericho
Copy link
Owner

Jericho commented Jul 11, 2024

Good news: I am able to reproduce the problem by creating a meeting with the "Test 🚒🚒 ? - ’ - – 🚗 HOLA" topic as you suggested.

More good news: I can resolve this problem by switching to UTF8 encoding as you suggested

Even more good news: I have added unit tests for testing the VerifySignature method when the webhook payload contains non-ASCII characters as well as a more standard scenario when all characters are ASCII. You'll be able to add new unit tests if you ever discover new scenarios.

I'll start working on a new release and publish it over the weekend.

@LucasMetal
Copy link
Author

Awesome man!!! Thanks for the quick, friendly and informative responses!
And for the fix itself of course!

Take care ;)

@Jericho Jericho closed this as completed Jul 12, 2024
@Jericho
Copy link
Owner

Jericho commented Jul 12, 2024

🎉 This issue has been resolved in version 0.78.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This change resolves a defect
Projects
None yet
Development

No branches or pull requests

2 participants