Eventsub webhook_secret verification fixes

[kamalmostafa]

Pull request summary

Fix #366 eventsub webhook_secret verification bug. Make eventsub callback more robust (missing signature/timestamp headers). Add additional logging; fix log message spelling errors.

Checklist

• [x] If code changes were made then they have been tested.
  • [ ] I have updated the documentation to reflect the changes.
  • [ ] I have updated the changelog with a quick recap of my changes.
• [x] This PR fixes an issue.
• [ ] This PR adds something new (e.g. new method or parameters).
• [ ] This PR is a breaking change (e.g. methods or parameters removed/renamed)
• [ ] This PR is not a code change (e.g. documentation, README, ...)

[chillymosh]

Please update the changelog

[kamalmostafa]

I'm confused what these changes are fixing, other than the early return? Can you elaborate, as these fields that are being try/except'ed are not optional. They are required to perform a validation of the message, and if they don't exist, the client should error out, as further processing is a waste of processing power

Thanks for your review @IAmTomahawkx :)

... if they don't exist, the client should error out ...

Right, but it should do that gracefully!

My argument is that if those fields don't exist (or really if anything about the received http request is unsatisfactory), that the TwitchIO client should handle that situation by gracefully returning an http error. But that's not what happens. What happens is that the TwitchIO client crashes with a traceback like the following, and fails to return any http error.

$ twitch -F "$CALLBACK_URL" event trigger follow    # <- note the lack of "-s webhook_secret"

05:19:28 ERROR    aiohttp.server       Error handling request
Traceback (most recent call last): 
  File ".../aiohttp/web_protocol.py", line 435, in _handle_request 
    resp = await request_handler(request) 
  File ".../aiohttp/web_app.py", line 504, in _handle 
    resp = await handler(request) 
  File ".../twitchio/ext/eventsub/server.py", line 238, in _callback 
    event = _message_types[typ](self, payload, request) 
  File ".../twitchio/ext/eventsub/models.py", line 104, in __init__
    self.headers = Headers(request)
  File ".../twitchio/ext/eventsub/models.py", line 78, in __init__
    self.signature: str = request.headers["Twitch-Eventsub-Message-Signature"]
KeyError: 'Twitch-Eventsub-Message-Signature'

My commit eventsub: handle message with missing signature/timestamp fixes that specific condition. Instead of crashing, the request gets processed (with an empty signature and timestamp strings) and then properly rejected later by the .valid() call -- and that allows for the proper http error to be returned.

However, after thinking about this further I'd argue that all of the accesses of "presumed" request.headers in init () are really just as bad. It is undesirable that ANY malformed incoming http request should be able to crash TwitchIO. A better solution (than my proposed commit) would be to ensure that init() can't ever crash due to a malformed request by (1) actually checking for the presence of all presumed and required header fields and (2) doing that outside of init() but before the call to .valid() -- where we can properly return the suitable http error.

If you'd like to drop eventsub: handle message with missing signature/timestamp from this PR (or would like me to), I'll submit a separate one to address it as described. Note though that it is important to apply the fix for #366 before fixing this crash, or else "no-webhook-secret" messages will actually slip past any validation!

[chillymosh]

I believe the majority of this PR is solely for a CLI isolated scenario. In my opinion the only worthwhile portion is the != 200: return to stop potential event invocations. Along with the spelling corrections.

Those headers are always present when a message is received from Twitch. You will also receive a failed response from Twitch if you try and sub with an empty string or less than 10 character string and the lib shows that error message when attempting this, so you are extremely unlikely to receive a response back with the challenge as an empty string, but the return is still good to implement.

[IAmTomahawkx]

The correct place to handle that is the server callback, a simple try except while constructing the object would suffice, as if anything goes wrong while constructing the entire thing is a throwaway. As for the missing fields from using twitchCLI, that's a twitchcli issue, not a lib issue. an error doesnt actually crash aiohttp, it just produces a noisy traceback and returns a 500 error. The traceback may not be ideal but the 500 is fine for our usecase.

[kamalmostafa]

In my opinion the only worthwhile portion is the != 200: return to stop potential event invocations.

Feel free to apply just the first commit, which implements only that #366 fix, and close this PR.

[IAmTomahawkx]

closed in 64f3fb7