[detector] feat: Rootly Webhook Detector #4492
Conversation
| // Use identifiers in the secret preferably, or the provider name. | ||
| func (s Scanner) Keywords() []string { | ||
| return []string{"webhooks.rootly.com"} | ||
| } |
There was a problem hiding this comment.
This is great and will greatly reduce noise but I think it is more restrictive than it should be. What I mean is: webhooks.rootly.com will only appear if the webhook URL itself is present in the file or config (e.g. https://webhooks.rootly.com/incoming/...).
That’s great for catching leaked webhook endpoints, but it won’t catch files that only contain the secret, which is often stored separately (in .env, YAML, or JSON).
Many developers store only the secret like this:
ROOTLY_WEBHOOK_SECRET=abcdef123456
or
rootly:
webhook_secret: "abcdef123456"
In those cases, the detector won’t trigger because webhooks.rootly.com isn’t present.
So, my recommendation would be to use a combination of provider and contextual keywords to improve coverage. For example, we can include rootly, rootly_webhook/rootlywebhook along with what we have already added. Let me know if this makes sense.
There was a problem hiding this comment.
Since the string rootly is already a keyword, all other keywords are redundant since they all contain rootly.
…ds for precise detection
| case http.StatusInternalServerError: | ||
| // 500: Auth is valid but there was a server error (e.g., parsing issue with our test payload) | ||
| // This is the expected response that indicates the token is valid without creating alerts. | ||
| return true, nil |
There was a problem hiding this comment.
This makes sense in principle, but I’m concerned about treating 500 as a hard signal that a token is valid. 500 is a generic server error and could indicate a service outage or other problem. If Rootly returns 500 for many requests during an outage, we could end up marking a lot of secrets as verified incorrectly.
Would it make sense to only treat 500 as valid when the response body contains a known, stable parsing-error signature? Basically, some logic to distinguish between “500 due to a genuine server issue/outage” and “500 due to our test payload being malformed.” Does that sound reasonable?
There was a problem hiding this comment.
+1 This is a valid concern.
There was a problem hiding this comment.
There is no exact message from the server side tho. I'll try to communicate with Rootly team to see if they can provide an endpoint for checking webhook validity.
4 participants
Description:
This PR Adding Rootly Webhook Detector
Checklist:
make test-community)?make lintthis requires golangci-lint)?