feat: adapt to webhooks v2#16
Conversation
| if err != nil { | ||
| res.WriteHeader(http.StatusBadRequest) | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
You should not reject the Webhook with a 400 when you fail to parse, instead respond with 2XX and maybe log an warning or something? this is not an issue for the sender (top.gg) in this case but likely a parsing issue and responding with a non 2XX makes top.gg webhook retry which is likely failing again
There was a problem hiding this comment.
Noted! Thank you so much for your feedback! Will work on them!
There was a problem hiding this comment.
| if err = json.Unmarshal(body, payload); err != nil { | ||
| res.WriteHeader(http.StatusBadRequest) | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
same as above, you should not respond with Bad Request on a parsing error on your side
There was a problem hiding this comment.
|
|
||
| defer req.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(req.Body) |
There was a problem hiding this comment.
Ideally io.LimitReader is used to prevent some kind of DoS by memory exhaustion as the content can pretty much be sent by anyone.
It may be worth adding it to the client as well e.g. due to the bot description, although not sure if it's unlimited in length or not anymore to be fair
There was a problem hiding this comment.
Also done it for the client in #15. See this commit. Nevermind, this one is reverted as the Top.gg API has their own constraints for things like long descriptions.
There was a problem hiding this comment.
Noted! Is this what you meant?
Looking good!
3 participants
The following pull request is a fragment of a larger pull request.
It adapts the SDK's webhooks functionality to the newer approach. And since this change is obviously breaking, the pull request is a major update.