Skip to content

Commit 1f2f5ec

Browse files
authored
Treat webhook 422 responses as proper failures (#6426)
HookRelay returns 422 when it rejects a webhook delivery request, for example when its SSRF protection blocks the target URL. Previously, the `discard_on` handler only called `increment! :failure_count`, which doesn't contribute to the disable threshold. This meant webhooks with blocked URLs would accumulate failures indefinitely without ever being disabled. Call `failure!` instead of `increment! :failure_count` in the `discard_on` handler. `failure!` tracks `failures_since_last_success`, which will automatically disable the webhook after exceeding the failure threshold. 422s were previously given special treatment under the assumption they might not represent a real delivery failure. But there's no scenario where a 422 from HookRelay should not count as a failure -- the request was understood and rejected.
1 parent a60f315 commit 1f2f5ec

2 files changed

Lines changed: 30 additions & 1 deletion

File tree

app/jobs/notify_web_hook_job.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ class NotifyWebHookJob < ApplicationJob
3333
# has to come after the retry on
3434
discard_on(Faraday::UnprocessableEntityError) do |j, e|
3535
logger.info({ webhook_id: j.webhook.id, url: j.webhook.url, response: JSON.parse(e.response_body) })
36-
j.webhook.increment! :failure_count
36+
j.webhook.failure!(completed_at: Time.current)
3737
end
3838

3939
def perform(*)

test/jobs/notify_web_hook_job_test.rb

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,5 +85,34 @@ class NotifyWebHookJobTest < ActiveJob::TestCase
8585
assert_performed_jobs 1, only: NotifyWebHookJob
8686
assert_enqueued_jobs 0, only: NotifyWebHookJob
8787
end
88+
89+
should "record a failure on a 422" do
90+
stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-#{@hook.id}")
91+
.to_return_json(status: 422, body: { error: "Invalid url" })
92+
93+
perform_enqueued_jobs do
94+
@job.enqueue
95+
end
96+
97+
@hook.reload
98+
99+
assert_equal 1, @hook.failures_since_last_success
100+
end
101+
102+
should "disable the webhook after repeated 422s exceed the failure threshold" do
103+
@hook.update!(
104+
failures_since_last_success: WebHook::FAILURE_DISABLE_THRESHOLD - 1,
105+
created_at: (WebHook::FAILURE_DISABLE_DURATION + 1.minute).ago
106+
)
107+
108+
stub_request(:post, "https://api.hookrelay.dev/hooks///webhook_id-#{@hook.id}")
109+
.to_return_json(status: 422, body: { error: "Invalid url" })
110+
111+
perform_enqueued_jobs do
112+
@job.enqueue
113+
end
114+
115+
refute_predicate @hook.reload, :enabled?
116+
end
88117
end
89118
end

0 commit comments

Comments
 (0)