fix(webhooks): stop following delivery redirects#1813
Conversation
📝 WalkthroughWalkthroughThe fetch used by Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/webhook-delivery-security.unit.test.ts`:
- Line 35: Replace the non-concurrent test declarations with concurrent ones:
change the two occurrences of it('sends webhook requests with manual redirect
handling', ...) and the other it(...) at the second location to
it.concurrent(...) in tests/webhook-delivery-security.unit.test.ts so both tests
run in parallel per the repo policy; locate the test blocks by their test
descriptions and update the it() calls to it.concurrent().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07ac9fb3-383f-4705-a7c2-71d5add5a69f
📒 Files selected for processing (2)
supabase/functions/_backend/utils/webhook.tstests/webhook-delivery-security.unit.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/webhook-delivery-security.unit.test.ts (1)
37-40: Potential test flakiness with concurrent global stubbing.Both
it.concurrent()tests callvi.stubGlobal('fetch', ...)which modifies a shared global. Since concurrent tests in the same file share execution context, one test's stub could overwrite the other's mid-execution. Similarly,afterEachrunningvi.unstubAllGlobals()for a completed test could affect a concurrent test still in progress.In practice, these tests are fast enough that races are unlikely, but for robustness you could consider either:
- Using a per-test module isolation with
vi.resetModules()before each dynamic import- Or wrapping these specific tests in a nested
describewithout concurrent executionAlso applies to: 71-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/webhook-delivery-security.unit.test.ts` around lines 37 - 40, The tests use vi.stubGlobal('fetch') inside it.concurrent blocks which can race across concurrent tests and vi.unstubAllGlobals() in afterEach; to fix, avoid shared global stubbing by calling vi.resetModules() before each dynamic import and then stubbing fetch within each test (ensure the dynamic import of ../supabase/functions/_backend/utils/webhook.ts happens after resetModules so deliverWebhook binds to the per-test stub), or alternatively remove it.concurrent and wrap the tests in a non-concurrent describe so vi.stubGlobal/vi.unstubAllGlobals around each test won't interfere with others; reference the existing it.concurrent usage, vi.stubGlobal/vi.unstubAllGlobals, vi.resetModules(), and the dynamic import of deliverWebhook.
🤖 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-security.unit.test.ts`:
- Around line 37-40: The tests use vi.stubGlobal('fetch') inside it.concurrent
blocks which can race across concurrent tests and vi.unstubAllGlobals() in
afterEach; to fix, avoid shared global stubbing by calling vi.resetModules()
before each dynamic import and then stubbing fetch within each test (ensure the
dynamic import of ../supabase/functions/_backend/utils/webhook.ts happens after
resetModules so deliverWebhook binds to the per-test stub), or alternatively
remove it.concurrent and wrap the tests in a non-concurrent describe so
vi.stubGlobal/vi.unstubAllGlobals around each test won't interfere with others;
reference the existing it.concurrent usage, vi.stubGlobal/vi.unstubAllGlobals,
vi.resetModules(), and the dynamic import of deliverWebhook.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff5f66fa-b845-4252-ad3d-c7caa8c5a2cc
📒 Files selected for processing (1)
tests/webhook-delivery-security.unit.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/webhook-delivery-security.unit.test.ts (1)
35-35:⚠️ Potential issue | 🟡 MinorUse concurrent test declarations in this
.test.tsfileLine 35 and Line 70 still use
it(...); this file should useit.concurrent(...)per repo test policy.Suggested patch
- it('sends webhook requests with manual redirect handling', async () => { + it.concurrent('sends webhook requests with manual redirect handling', async () => { @@ - it('does not treat redirect responses as successful deliveries', async () => { + it.concurrent('does not treat redirect responses as successful deliveries', async () => {As per coding guidelines,
tests/**/*.test.{ts,tsx}: ALL TEST FILES RUN IN PARALLEL; useit.concurrent()instead ofit()to maximize parallelism; create dedicated seed data when tests modify shared resources.Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/webhook-delivery-security.unit.test.ts` at line 35, Replace non-concurrent test declarations in this test file by changing plain it(...) calls to it.concurrent(...) for all tests (including the one titled "sends webhook requests with manual redirect handling" and the other test flagged around line 70) so the file follows the repo policy of running test files in parallel; update any imports or helpers if needed to preserve behavior but do not change test logic or titles—only replace the it function calls with it.concurrent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/webhook-delivery-security.unit.test.ts`:
- Line 35: Replace non-concurrent test declarations in this test file by
changing plain it(...) calls to it.concurrent(...) for all tests (including the
one titled "sends webhook requests with manual redirect handling" and the other
test flagged around line 70) so the file follows the repo policy of running test
files in parallel; update any imports or helpers if needed to preserve behavior
but do not change test logic or titles—only replace the it function calls with
it.concurrent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8679afa9-3aa1-417e-8a19-dd24ce7e80c3
📒 Files selected for processing (1)
tests/webhook-delivery-security.unit.test.ts
|
1 participant
Summary (AI generated)
deliverWebhook()uses manual redirect handlingMotivation (AI generated)
The current webhook delivery path validates only the initial URL, then lets
fetch()follow redirects automatically. That leaves a redirect-based SSRF path open even when the original URL passes validation.Business Impact (AI generated)
This closes a high-severity SSRF vector in webhook delivery, reducing risk to internal services and metadata endpoints without changing the public webhook configuration flow for customers.
Test Plan (AI generated)
bun run supabase:with-env -- bunx vitest run tests/webhook-delivery-security.unit.test.tsbun run supabase:with-env -- bunx vitest run tests/webhooks.test.ts tests/webhook-signature.test.tsbunx eslint supabase/functions/_backend/utils/webhook.ts tests/webhook-delivery-security.unit.test.tsGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests