fix: block app-scoped API keys from org webhook actions#1812
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a centralized helper function 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}
Contributor
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/webhooks.test.ts`:
- Around line 482-502: Convert the two permission tests (the one using
createdWebhookId and appScopedKey and its counterpart) to use it.concurrent()
and avoid shared mutable state by creating isolated fixtures inside each test:
generate a fresh webhook (replace reliance on createdWebhookId by calling the
same seeding function used elsewhere) and a fresh app-scoped API key (instead of
using appScopedKey) at the start of the test, then call the /webhooks/test
endpoint and assert the 400/no_permission response; modify the tests that
reference createdWebhookId and appScopedKey to seed their own webhook and API
key locally before making the request so the tests are fully independent and
safe to run in parallel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3606255e-b1f7-4c43-967f-49e38416737f
📒 Files selected for processing (2)
supabase/functions/_backend/public/webhooks/index.tstests/webhooks.test.ts
Comment on lines
+482
to
+502
| it('rejects app-scoped API keys for org-scoped webhook tests', async () => { | ||
| if (!createdWebhookId || !appScopedKey) | ||
| throw new Error('Webhook test prerequisites were not created') | ||
|
|
||
| const response = await fetch(`${BASE_URL}/webhooks/test`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'x-api-key': appScopedKey, | ||
| }, | ||
| body: JSON.stringify({ | ||
| orgId: WEBHOOK_TEST_ORG_ID, | ||
| webhookId: createdWebhookId, | ||
| }), | ||
| }) | ||
|
|
||
| expect(response.status).toBe(400) | ||
| const data = await response.json() as { error: string, message: string } | ||
| expect(data.error).toBe('no_permission') | ||
| expect(data.message).toContain('App-scoped API keys') | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify shared mutable globals used by tests
rg -n "^\s*let\s+(createdWebhookId|lastDeliveryId|appScopedKey|appScopedKeyId)\b" tests/webhooks.test.ts
# Verify non-concurrent vs concurrent test declarations
rg -n "^\s*it\(" tests/webhooks.test.ts
rg -n "^\s*it\.concurrent\(" tests/webhooks.test.ts || trueRepository: Cap-go/capgo
Length of output: 2437
Use it.concurrent() and isolated fixtures for API key permission tests.
The tests at lines 482–502 and 581–601 use it() and rely on shared mutable state (createdWebhookId, appScopedKey). Per coding guidelines, convert these to it.concurrent() and create dedicated seed data for each test instead of depending on earlier test mutations. This enables parallel execution and prevents test order dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/webhooks.test.ts` around lines 482 - 502, Convert the two permission
tests (the one using createdWebhookId and appScopedKey and its counterpart) to
use it.concurrent() and avoid shared mutable state by creating isolated fixtures
inside each test: generate a fresh webhook (replace reliance on createdWebhookId
by calling the same seeding function used elsewhere) and a fresh app-scoped API
key (instead of using appScopedKey) at the start of the test, then call the
/webhooks/test endpoint and assert the 400/no_permission response; modify the
tests that reference createdWebhookId and appScopedKey to seed their own webhook
and API key locally before making the request so the tests are fully independent
and safe to run in parallel.
|
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 (AI generated)
/webhooks/testand/webhooks/deliveries/retryMotivation (AI generated)
App-scoped API keys were being authorized for organization-wide webhook test and retry actions because the webhook permission helper only checked org admin rights plus
limited_to_orgs, while ignoringlimited_to_apps. That breaks the intended delegation boundary for scoped keys.Business Impact (AI generated)
This closes a high-severity authorization gap that could let lower-scope API keys trigger org webhook traffic outside their declared app boundary. Fixing it reduces cross-app privilege leakage and protects downstream customer integrations.
Test Plan (AI generated)
bunx eslint supabase/functions/_backend/public/webhooks/index.ts tests/webhooks.test.tsbun run supabase:with-env -- bunx vitest run tests/webhooks.test.tsbun run test:backendGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests