fix(cron-delivery): drop redundant runtime SSRF check in deliver_webhook#3155
Merged
Conversation
PR #3139 moved cron LocalFile path validation out of the runtime delivery layer and into CronJob::validate_delivery_targets so existing tests that write to absolute tempdir paths could still run via direct engine.deliver(). The webhook side of the same migration was missed — deliver_webhook still called validate_webhook_url which rejected any literal loopback IP, so cron_delivery::tests::webhook_sends_payload and webhook_reports_non_2xx (both posting to a 127.0.0.1:<port> mock server) panic on every CI run, blocking PRs #3141, #3142 and any future PR based on main. The runtime check was symbolic anyway: it never resolved DNS, which the comment itself flagged as 'a known TOCTOU we accept'. SSRF protection now lives entirely in validate_delivery_targets, which uses the same allow-list (is_blocked_webhook_host) before any URL reaches the scheduler. Mirroring deliver_local_file's doc, point readers at the input-time gate as the single source of truth.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
ready-for-review
github-actions
Bot
removed
the
ready-for-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mainis red oncron_delivery::tests::webhook_sends_payloadandwebhook_reports_non_2xx, blocking #3141, #3142, and every future PR off main.Root cause
#3139 migrated cron
LocalFilepath validation out of the runtime delivery layer and intoCronJob::validate_delivery_targets, so existing tests that write to absolute tempdir paths via directengine.deliver()could still run. The webhook half of the same migration was missed —deliver_webhookkept callingvalidate_webhook_url, which rejects any literal loopback IP. The two existing webhook tests both POST tohttp://127.0.0.1:<port>/hookagainst an in-process mock server, so they fail on every CI run.Fix
Drop the runtime
validate_webhook_urlcall and the function itself. Its protection was already symbolic — the doc comment itself flagged that DNS is not resolved, "a known TOCTOU we accept" — andvalidate_delivery_targetsuses the sameis_blocked_webhook_hostallow-list before any URL reaches the scheduler. The comment ondeliver_webhooknow mirrorsdeliver_local_file, pointing at the input-time gate as the single source of truth.Test plan
cron_delivery::tests::webhook_sends_payloadandwebhook_reports_non_2xxpass locallyvalidate_delivery_targetsstill rejectshttp://127.0.0.1/...andhttp://169.254.169.254/...at job creation time (existing test coverage inlibrefang-types::scheduler::tests)