search

mattermost / mattermost-plugin-zoom

Zoom plugin for Mattermost :electric_plug:
Apache License 2.0
107 stars 68 forks source link

Webhook is not working when the secret contains a `+` character #296

Closed DHaussermann closed 9 months ago

DHaussermann commented 1 year ago

During testing it was noticed that Webhook validation functionality would intermittently not work during setup.

With some dev help investigating @mickmister noticed the issue to occurs when the Mattermost webhook secret contains a +

Follow-up testing seems to confirm this. No issues found with other special characters (- and _ can also be included) This would have also stopped the webhook deliveries until the admins tries gegenerating the secret.

This would not be a recent regression.

Steps:

Observed Zoom App wizard will not validate the webhook

hanzei commented 1 year ago

@DHaussermann Can you please check if this issue affects other plugins?

avas27JTG commented 10 months ago

This was a bug in Mattermost that was identified earlier and has a JIRA ticket https://mattermost.atlassian.net/browse/MM-51451.

This bug has been fixed in the MM now, but we can also add the fix in the plugin itself to avoid incompatibility with different MM versions where this bug still exists. Example plugin fix: https://github.com/mattermost/mattermost-plugin-azure-devops/pull/150#discussion_r1138553194

mickmister commented 9 months ago

@avas27JTG I think we can close this as the core issue is fixed, and it sort of occurs by chance anyway. Thoughts? @avas27JTG @hanzei

avas27JTG commented 9 months ago

@avas27JTG I think we can close this as the core issue is fixed, and it sort of occurs by chance anyway. Thoughts? @avas27JTG @hanzei

@mickmister I'm not sure about "core issue is fixed" can you please mention what you mean by that? Also, I was able to reproduce this issue when + character was present in the generated secret.

mickmister commented 9 months ago

I'm not sure about "core issue is fixed" can you please mention what you mean by that?

@avas27JTG I was referring to the fact that the webapp no longer produces generated values with + in them as of about 7 months ago https://github.com/mattermost/mattermost/pull/22869. I haven't tested this personally but are you still experiencing the + showing up, with the updated webapp version?

For existing generated values that have +s in them, I think it's okay if those are broken since we haven't had any customers report an issue with this AFAIK

avas27JTG commented 9 months ago

I'm not sure about "core issue is fixed" can you please mention what you mean by that?

@avas27JTG I was referring to the fact that the webapp no longer produces generated values with + in them as of about 7 months ago mattermost/mattermost#22869. I haven't tested this personally but are you still experiencing the + showing up, with the updated webapp version?

For existing generated values that have +s in them, I think it's okay if those are broken since we haven't had any customers report an issue with this AFAIK

thanks for clarifying @mickmister its fixed in master but do we need to also fix in the plugin also in case if someone is running on an MM version different from the master?

mickmister commented 9 months ago

@avas27JTG I don't think so. We would also want to look through all other plugins affected by this, and I don't think it's worth the trouble. @DHaussermann Thoughts on closing this, as imo this is solved via the fix in the webapp?

Note that the bug in the webapp was that we were only replacing one instance of the +, so the issue only occurs when the generated secret happened to have more than one +

avas27JTG commented 9 months ago

Okay then should we close this issue? @mickmister @DHaussermann