fix: stop webhook redirect SSRF

[riderx]

Summary (AI generated)

• stop webhook deliveries from automatically following HTTP redirects
• add a unit regression test that asserts deliverWebhook() uses redirect: 'manual'
• preserve existing webhook URL validation while closing the redirect-based SSRF gap

Motivation (AI generated)

Webhook URLs were validated only for the initial target. The delivery fetch still followed server-provided redirects automatically, which allowed a validated public HTTPS endpoint to bounce requests toward internal resources.

Business Impact (AI generated)

This closes a high-severity webhook SSRF path that could expose internal services or metadata endpoints and leak response data through webhook delivery records. Reducing that risk protects tenant trust and avoids incident response overhead.

Test Plan (AI generated)

• env TMPDIR=/tmp BUN_INSTALL_CACHE_DIR=/tmp/bun-cache XDG_CACHE_HOME=/tmp bunx eslint supabase/functions/_backend/utils/webhook.ts tests/webhook-delivery-redirect.unit.test.ts
• env TMPDIR=/tmp BUN_INSTALL_CACHE_DIR=/tmp/bun-cache XDG_CACHE_HOME=/tmp bunx vitest run tests/webhook-delivery-redirect.unit.test.ts
• env TMPDIR=/tmp BUN_INSTALL_CACHE_DIR=/tmp/bun-cache XDG_CACHE_HOME=/tmp bun run supabase:start
• env TMPDIR=/tmp BUN_INSTALL_CACHE_DIR=/tmp/bun-cache XDG_CACHE_HOME=/tmp bun run test:all

Generated with AI

Summary by CodeRabbit

• Bug Fixes

  • Improved webhook delivery handling to manually process HTTP redirects instead of automatically following them, preventing unwanted request routing to unexpected endpoints.

• Tests

  • Added test coverage validating manual redirect handling behavior in webhook delivery operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

Modified webhook delivery to handle HTTP redirects manually by adding redirect: 'manual' to the fetch configuration, ensuring redirects are not automatically followed. Added a corresponding unit test to verify the manual redirect behavior returns success: false with the 302 status.

Changes

Cohort / File(s)	Summary
Webhook Manual Redirect Handling supabase/functions/_backend/utils/webhook.ts	Added redirect: 'manual' parameter to fetch options in deliverWebhook, preventing automatic redirect following.
Webhook Redirect Test Coverage tests/webhook-delivery-redirect.unit.test.ts	New unit test file verifying that webhook delivery correctly handles manual redirects, asserting success: false and 302 status response.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• fix(webhooks): validate webhook URLs on delivery #1571: Also modifies deliverWebhook in the webhook utility with request handling changes (URL validation and early-return logic).

Poem

🐰 A redirect came bouncing down the line,
"No auto-follow!" the webhook did opine,
With manual: true, it holds its ground,
Tests verify redirects won't silently be found! 🔄

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: stop webhook redirect SSRF' clearly and concisely describes the primary change: preventing SSRF attacks via webhook redirects.
Description check	✅ Passed	The PR description covers the main summary, motivation, business impact, and a detailed test plan. However, it omits the optional screenshots section and has incomplete checklist items.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/webhook-manual-redirect-ghsa-72qg

📝 Coding Plan

• Generate coding plan for human review comments

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/webhook-delivery-redirect.unit.test.ts (1)

52-80: Consider isolating the fetch spy for concurrent test safety.

The test correctly validates the redirect: 'manual' fix, but spying on globalThis.fetch inside a concurrent test could cause interference with other concurrent tests that use fetch. Since this file currently has only one test, it's not immediately problematic, but it's fragile for future additions.

Consider wrapping the spy in the test body with explicit cleanup, or use vi.stubGlobal with per-test isolation if more fetch-dependent tests are added later.

💡 Optional: Explicit cleanup within the test

   it.concurrent('uses manual redirect mode for outbound webhook delivery', async () => {
     const fetchMock = vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response('redirect blocked', {
       status: 302,
       headers: {
         location: 'http://169.254.169.254/latest/meta-data',
       },
     }))
 
+    try {
       const result = await deliverWebhook(
         context as any,
         'delivery-123',
         'https://example.com/webhook',
         payload,
         'whsec_test_secret',
       )
 
       expect(fetchMock).toHaveBeenCalledWith(
         'https://example.com/webhook',
         expect.objectContaining({
           method: 'POST',
           redirect: 'manual',
         }),
       )
       expect(result).toMatchObject({
         success: false,
         status: 302,
         body: 'redirect blocked',
       })
+    }
+    finally {
+      fetchMock.mockRestore()
+    }
   })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/webhook-delivery-redirect.unit.test.ts` around lines 52 - 80, The test
currently spies on globalThis.fetch which can leak into other concurrent tests;
replace the vi.spyOn(globalThis, 'fetch') usage with a per-test stub (e.g.,
vi.stubGlobal('fetch', mockedFetch)) or ensure explicit cleanup by calling
fetchMock.mockRestore() (or vi.restoreAllMocks()) in an afterEach/ finally block
so the stub is removed; update the test that calls deliverWebhook to use the
stubbed fetch and add per-test teardown to prevent interference with other
tests.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/webhook-delivery-redirect.unit.test.ts`:
- Around line 52-80: The test currently spies on globalThis.fetch which can leak
into other concurrent tests; replace the vi.spyOn(globalThis, 'fetch') usage
with a per-test stub (e.g., vi.stubGlobal('fetch', mockedFetch)) or ensure
explicit cleanup by calling fetchMock.mockRestore() (or vi.restoreAllMocks()) in
an afterEach/ finally block so the stub is removed; update the test that calls
deliverWebhook to use the stubbed fetch and add per-test teardown to prevent
interference with other tests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f1acc7dc-18ca-4726-ab77-c0fda9303495

📥 Commits

Reviewing files that changed from the base of the PR and between e1cd9a0 and d4652ba.

📒 Files selected for processing (2)

• supabase/functions/_backend/utils/webhook.ts
• tests/webhook-delivery-redirect.unit.test.ts

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud