fix(api): harden webhook and store metadata fetches#2196
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
📝 WalkthroughWalkthroughThis PR adds two security-hardening features: icon response size limiting and webhook protocol validation. Icon fetching now rejects oversized payloads via a streaming buffer helper, and local webhook URLs are restricted to HTTP/HTTPS protocols. Both features include unit tests validating their enforcement. ChangesIcon Size Limiting
Webhook Protocol Validation
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/webhook-delivery-security.unit.test.ts (1)
108-148: ⚡ Quick winSolid test coverage for the protocol validation hardening.
The three test cases effectively verify the key security constraint: when local webhook testing is enabled, only HTTP(S) protocols are accepted, rejecting schemes like
ftp://andfile://that could potentially be exploited.Consider adding edge case coverage for completeness
While not critical for this PR, the following test cases would strengthen coverage:
it('allows public HTTPS URLs when local webhooks are enabled', async () => { mockGetEnv.mockImplementation((_c: unknown, key: string) => key === 'CAPGO_ALLOW_LOCAL_WEBHOOK_URLS' ? 'true' : '', ) const { getWebhookUrlValidationError } = await import('../supabase/functions/_backend/utils/webhook.ts') expect( getWebhookUrlValidationError(createContext(), 'https://example.com/webhook'), ).toBeNull() }) it('rejects localhost URLs when local webhooks are disabled', async () => { mockGetEnv.mockReturnValue('') const { getWebhookUrlValidationError } = await import('../supabase/functions/_backend/utils/webhook.ts') expect( getWebhookUrlValidationError(createContext(), 'https://localhost/webhook'), ).toBe('Webhook URL must point to a public host') })These cases verify that the early-return logic doesn't inadvertently change behavior for public URLs and that localhost is properly rejected when the flag is disabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/webhook-delivery-security.unit.test.ts` around lines 108 - 148, The tests currently cover protocol validation but miss two edge cases; add two new it() tests that call getWebhookUrlValidationError (imported from ../supabase/functions/_backend/utils/webhook.ts) using the existing mockGetEnv and createContext helpers: 1) when CAPGO_ALLOW_LOCAL_WEBHOOK_URLS is 'true', assert getWebhookUrlValidationError(createContext(), 'https://example.com/webhook') returns null (public HTTPS allowed); 2) when CAPGO_ALLOW_LOCAL_WEBHOOK_URLS is '', assert getWebhookUrlValidationError(createContext(), 'https://localhost/webhook') returns 'Webhook URL must point to a public host' to verify localhost is rejected when the flag is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/webhook-delivery-security.unit.test.ts`:
- Around line 108-148: The tests currently cover protocol validation but miss
two edge cases; add two new it() tests that call getWebhookUrlValidationError
(imported from ../supabase/functions/_backend/utils/webhook.ts) using the
existing mockGetEnv and createContext helpers: 1) when
CAPGO_ALLOW_LOCAL_WEBHOOK_URLS is 'true', assert
getWebhookUrlValidationError(createContext(), 'https://example.com/webhook')
returns null (public HTTPS allowed); 2) when CAPGO_ALLOW_LOCAL_WEBHOOK_URLS is
'', assert getWebhookUrlValidationError(createContext(),
'https://localhost/webhook') returns 'Webhook URL must point to a public host'
to verify localhost is rejected when the flag is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a57eba3-fefc-46c9-b565-a81e8f9757a4
📒 Files selected for processing (4)
supabase/functions/_backend/public/app/store_metadata.tssupabase/functions/_backend/utils/webhook.tstests/store-metadata.unit.test.tstests/webhook-delivery-security.unit.test.ts
mingisrookie
left a comment
mingisrookie
left a comment
There was a problem hiding this comment.
One logging path in the webhook hardening still keeps the raw webhook URL: when getWebhookUrlValidationError() blocks delivery, deliverWebhook() calls cloudlogErr({ ..., url, error: urlValidationError, ... }).
That means a rejected URL such as https://example.test/webhook?token=secret or a malformed/local URL with embedded credentials/query state can still be retained verbatim in operational logs, even though the delivery is correctly blocked before fetch(). This is especially easy to miss because the new tests assert the validation result but do not inspect the blocked-delivery log payload.
I would log URL shape metadata here instead (scheme category, host present, path segment count, hasQuery) or at least strip search/credentials before logging. A regression can call deliverWebhook() with a URL containing a fake query token that is rejected and assert the serialized cloudlogErr calls do not include the token.
|
Addressed the logging concern in the latest commit. Blocked webhook deliveries no longer log the raw URL. The validation log now records URL shape metadata only ( Also added regression coverage for a blocked URL containing a fake query token and the two validation edge cases CodeRabbit suggested. Validation:
|
|
mingisrookie
left a comment
mingisrookie
left a comment
There was a problem hiding this comment.
Rechecked the current head (1c4231f7) and this addresses my logging concern: blocked deliveries now log urlInfo metadata instead of the raw url, and the regression asserts a query token is absent from serialized cloudlogErr calls. I also ran the focused tests locally:
bun x vitest run tests/webhook-delivery-security.unit.test.ts tests/store-metadata.unit.test.ts
Result: 2 files passed, 10 tests passed.
|
Closing as AI-generated spam. Part of a 50+ PR wave of duplicate |
3 participants
/claim #1667
Summary
CAPGO_ALLOW_LOCAL_WEBHOOK_URLSdoes not also allow unsupported schemes such asfile:orftp:.Security note
This is public-safe hardening. The webhook changes preserve the intended local HTTP testing path while blocking non-HTTP schemes and avoiding secret-bearing URL retention in blocked-delivery logs. The store metadata change preserves existing allowlisted app-store behavior while capping fetched icon payload size.
Test plan
./node_modules/.bin/vitest.exe run tests/webhook-delivery-security.unit.test.ts tests/store-metadata.unit.test.ts --reporter=verbosegit diff --checkScreenshots
Not applicable; backend/security hardening only.
Checklist