Skip to content

test(notifications): add webhook egress control regression guard #877#1112

Open
NaitikVerma6776 wants to merge 2 commits into
utksh1:mainfrom
NaitikVerma6776:fix/webhook-egress-test
Open

test(notifications): add webhook egress control regression guard #877#1112
NaitikVerma6776 wants to merge 2 commits into
utksh1:mainfrom
NaitikVerma6776:fix/webhook-egress-test

Conversation

@NaitikVerma6776

Copy link
Copy Markdown
Contributor

Description

Reviewed backend/secuscan/notification_service.py and identified that send_webhook uses a raw httpx.AsyncClient without explicit egress-policy regression coverage. Existing notification service tests validate delivery behavior but do not verify that webhook sends respect network egress controls.

Added testing/backend/integration/test_webhook_egress_policy.py with focused integration coverage for:

  • Blocking webhook delivery to loopback addresses (127.0.0.1)
  • Blocking webhook delivery to private RFC-1918 ranges (10.0.0.1)
  • Treating HTTP 4xx responses as delivery failures with actionable error messages
  • Recording egress-policy failures as FAILED in notification_history
  • Recording webhook timeouts as FAILED in notification_history
  • Ensuring failed deliveries remain retryable once egress conditions recover

All tests fully mock httpx and make no real network requests.

Related Issues

Closes #877

Type of Change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

python -m pytest testing/backend/integration/test_webhook_egress_policy.py -v

Result:

  • 6 tests passed
  • Runtime: ~2.72s

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.

@NaitikVerma6776 NaitikVerma6776 changed the title test(notifications): add webhook egress control regression guard test(notifications): add webhook egress control regression guard #877 Jun 20, 2026
@utksh1 utksh1 added level:intermediate 35 pts difficulty label for moderate contributor PRs type:testing Testing work category bonus label area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests labels Jun 20, 2026

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes. The title says this guards webhook egress control, but the tests mostly mock httpx failures rather than proving the network policy blocks private/loopback destinations before delivery. Please assert the actual egress policy path and keep the notification_history assertions focused.

@NaitikVerma6776 NaitikVerma6776 force-pushed the fix/webhook-egress-test branch from eb12929 to 906b1dc Compare June 21, 2026 15:11
@NaitikVerma6776

Copy link
Copy Markdown
Contributor Author

heyy @utksh1 .Thanks for the feedback — you're right, the previous tests mocked httpx directly and never exercised the real policy path. I dug into send_webhook and found it already implements its own SSRF protection independent of the general network policy: it resolves the hostname via DNS, then validates every resolved IP against settings.notification_blocked_ip_ranges (which blocks 127.0.0.0/8, 169.254.0.0/16, and the cloud metadata IP by default) before httpx is ever touched.

Rewrote the tests to assert this real path directly, with no httpx mocking for the blocking assertions:

  • localhost resolves to 127.0.0.1 and is blocked via real DNS resolution
  • A literal loopback IP in the URL is blocked the same way
  • The cloud metadata address 169.254.169.254 is blocked
  • A malformed URL with no hostname fails cleanly pre-DNS
  • End-to-end: deliver_via_rule records a FAILED history row with the actual blocking reason for a loopback target, with no mocking of send_webhook itself
  • A blocked delivery does not get marked as already-delivered, so a corrected rule can retry
  • A legitimate target with a real (mocked-at-DNS-level) public IP still passes through to the HTTP layer, proving the check doesn't false-positive

All 7 tests pass locally in 1.26s.

@NaitikVerma6776

Copy link
Copy Markdown
Contributor Author

heyy @utksh1, Heads up — I'm noticing the same backend-unit (cancelled) and backend-tests (skipped) pattern across multiple open PRs, not just this one. Looks like a CI infrastructure issue (runner timeout or workflow config) rather than something caused by individual branches. Wanted to flag it in case it needs attention on the workflow side rather than per-PR fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests level:intermediate 35 pts difficulty label for moderate contributor PRs type:testing Testing work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TASK] Add notification webhook egress policy coverage to backend tests

2 participants