Feat: Webhook fixes / improvements#2131
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds Slack as a supported webhook integration and introduces several webhook improvements including the ability to update webhook expressions and enhanced CEL context.
- Adds Slack webhook support with challenge handling and HMAC verification
- Introduces PATCH endpoint for updating webhook event key expressions
- Adds headers to the CEL environment for more flexible event key expressions
Reviewed Changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/schema/v1-core.sql | Adds SLACK and DISCORD to webhook source name enum |
| pkg/repository/v1/webhooks.go | Adds UpdateWebhook method for updating expressions |
| internal/cel/cel.go | Adds headers variable to CEL environment for webhooks |
| api/v1/server/handlers/v1/webhooks/receive.go | Implements Slack challenge handling and validation |
| api/v1/server/handlers/v1/webhooks/update.go | New PATCH handler for updating webhooks |
| frontend/app/src/pages/main/v1/webhooks/index.tsx | Adds Slack webhook configuration UI |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| } | ||
|
|
||
| // qq: should this be utc? |
There was a problem hiding this comment.
The comment 'qq: should this be utc?' should be resolved or removed. The timestamp comparison is already using UTC (time.Unix(timestamp, 0).UTC()), so the comment appears to be outdated.
| // qq: should this be utc? |
|
|
||
| if err != nil { | ||
| return false, &ValidationError{ | ||
| Code: Http500, |
There was a problem hiding this comment.
Should this be a 500 response?
There was a problem hiding this comment.
I think so? same for the one below too (failing to compute the signature, although maybe that one is more debatable and could be e.g. 403)
There was a problem hiding this comment.
ah gotcha, yeah this one looks like a correct 500, the one below does not (we'd fail if the signatures don't match right? definitely feels like a 403 instead)
There was a problem hiding this comment.
ah yeah sorry, we will fail and return 403 if the signatures don't match, but that error is just checking if we fail to compute the signature, not if it doesn't match the provided one
There was a problem hiding this comment.
got it, seems reasonable then
|
|
||
| if err != nil { | ||
| return false, &ValidationError{ | ||
| Code: Http500, |
Description
Type of change