fix(webhooks): enforce API key expiration policy#1876
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe webhook permission checks were enhanced to consult org-level API key policies: API keys are validated with Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebhookAPI as Webhook Endpoint
participant PermCheck as checkWebhookPermission
participant PolicyEngine as apikeyHasOrgRightWithPolicy
participant OrgService as Organization Policy
Client->>WebhookAPI: HTTP request with API key
WebhookAPI->>PermCheck: validate permission (apikey path)
PermCheck->>PermCheck: if apikey.limited_to_apps -> reject (no_permission)
PermCheck->>PolicyEngine: apikeyHasOrgRightWithPolicy(orgId, supabaseApikey(...))
PolicyEngine->>OrgService: read org policy / expiration requirement
alt Org requires expiring key and key is non-expiring
OrgService-->>PolicyEngine: policy enforces expiring keys
PolicyEngine-->>PermCheck: policy violation
PermCheck-->>WebhookAPI: 401 org_requires_expiring_key
WebhookAPI-->>Client: 401 Error
else Org access denied
OrgService-->>PolicyEngine: access denied
PolicyEngine-->>PermCheck: invalid org access
PermCheck-->>WebhookAPI: 401 invalid_org_id
WebhookAPI-->>Client: 401 Error
else Authorized
PolicyEngine-->>PermCheck: permission OK
PermCheck-->>WebhookAPI: allow operation (admin check continues)
WebhookAPI-->>Client: proceed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2baddc5bf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/public/webhooks/index.ts (1)
59-73:⚠️ Potential issue | 🟠 MajorPolicy check missing in
checkWebhookPermissionV2.The
checkWebhookPermissionV2function (used by/testand/deliveries/retryendpoints viamiddlewareV2) does not callapikeyHasOrgRightWithPolicy. This means when an API key authenticates through the V2 path, theorg_requires_expiring_keypolicy is not enforced.The
assertOrgWebhookScopehelper only checksapikeyHasOrgRight(basic org scope) but not the policy compliance thatapikeyHasOrgRightWithPolicyprovides.Consider adding the policy check to
checkWebhookPermissionV2for consistency:Proposed fix
export async function checkWebhookPermissionV2( c: Context<MiddlewareKeyVariables, any, any>, orgId: string, auth: AuthInfo, ): Promise<void> { // Check org admin access if (!(await hasOrgRight(c, orgId, auth.userId, 'admin'))) { throw simpleError('no_permission', 'You need admin access to manage webhooks', { org_id: orgId }) } // If using API key, also check the key has org access if (auth.authType === 'apikey' && auth.apikey) { + const orgCheck = await apikeyHasOrgRightWithPolicy(c, auth.apikey, orgId, supabaseApikey(c, c.get('capgkey') as string)) + if (!orgCheck.valid) { + if (orgCheck.error === 'org_requires_expiring_key') { + throw quickError(401, 'org_requires_expiring_key', 'This organization requires API keys with an expiration date. Please use a different key or update this key with an expiration date.') + } + throw simpleError('invalid_org_id', 'You can\'t access this organization', { org_id: orgId }) + } assertOrgWebhookScope(orgId, auth.apikey) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/webhooks/index.ts` around lines 59 - 73, checkWebhookPermissionV2 currently skips policy enforcement for API keys; when auth.authType === 'apikey' and auth.apikey is present, call apikeyHasOrgRightWithPolicy(orgId, auth.apikey, 'org_requires_expiring_key') (or the appropriate policy-checking wrapper) instead of (or in addition to) assertOrgWebhookScope so the org policy is enforced for V2 paths; update checkWebhookPermissionV2 to invoke apikeyHasOrgRightWithPolicy (or a helper that wraps it) and remove relying solely on apikeyHasOrgRight/assertOrgWebhookScope.
🧹 Nitpick comments (2)
tests/webhooks-apikey-policy.test.ts (2)
80-94: Consider adding a timeout toafterAllfor consistency.The
beforeAllhook has a 60-second timeout (line 78), butafterAlldoes not. Given that cleanup also involves multiple async operations, adding a matching timeout would prevent Vitest teardown issues.afterAll(async () => { // ... cleanup code -}) +}, 60000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/webhooks-apikey-policy.test.ts` around lines 80 - 94, The afterAll cleanup hook lacks the same 60s timeout used in beforeAll, which can cause teardown flakiness; update the afterAll declaration (the one calling getSupabaseClient(), deleting from 'webhooks', 'apikeys', 'org_users', 'orgs', and 'stripe_info' using createdWebhookId, legacyApiKeyId, policyOrgId, policyCustomerId) to include a 60_000ms timeout (e.g., pass 60000 as the second argument) so the async cleanup has the same allowed time as beforeAll.
96-155: Consider adding test coverage for/webhooks/testand/webhooks/deliveries/retryendpoints.These endpoints use
middlewareV2andcheckWebhookPermissionV2, which is a different auth path than the endpoints covered here. Adding regression tests for these would ensure policy enforcement is consistent across all webhook endpoints.it('rejects webhook test for legacy non-expiring org key', async () => { if (!legacyApiKeyValue || !createdWebhookId) throw new Error('Test prerequisites were not created') const response = await fetch(`${BASE_URL}/webhooks/test`, { method: 'POST', headers: { 'Content-Type': 'application/json', 'x-api-key': legacyApiKeyValue, }, body: JSON.stringify({ orgId: policyOrgId, webhookId: createdWebhookId, }), }) expect(response.status).toBe(401) const data = await response.json() as { error: string } expect(data.error).toBe('org_requires_expiring_key') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/webhooks-apikey-policy.test.ts` around lines 96 - 155, Add regression tests to assert the org API key expiration policy is enforced for the webhook endpoints that use the alternate auth path: /webhooks/test and /webhooks/deliveries/retry. Create two tests similar to the existing ones that use legacyApiKeyValue (and createdWebhookId for the test/retry cases) to call POST /webhooks/test and POST /webhooks/deliveries/retry (with orgId and webhookId in the JSON body and using the x-api-key header if that’s what middlewareV2 expects), then assert response.status === 401 and response.json().error === 'org_requires_expiring_key'; ensure you reference middlewareV2 and checkWebhookPermissionV2 behavior by using the same auth header and fixtures (policyOrgId, legacyApiKeyValue, createdWebhookId, BASE_URL) as the other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@supabase/functions/_backend/public/webhooks/index.ts`:
- Around line 59-73: checkWebhookPermissionV2 currently skips policy enforcement
for API keys; when auth.authType === 'apikey' and auth.apikey is present, call
apikeyHasOrgRightWithPolicy(orgId, auth.apikey, 'org_requires_expiring_key') (or
the appropriate policy-checking wrapper) instead of (or in addition to)
assertOrgWebhookScope so the org policy is enforced for V2 paths; update
checkWebhookPermissionV2 to invoke apikeyHasOrgRightWithPolicy (or a helper that
wraps it) and remove relying solely on apikeyHasOrgRight/assertOrgWebhookScope.
---
Nitpick comments:
In `@tests/webhooks-apikey-policy.test.ts`:
- Around line 80-94: The afterAll cleanup hook lacks the same 60s timeout used
in beforeAll, which can cause teardown flakiness; update the afterAll
declaration (the one calling getSupabaseClient(), deleting from 'webhooks',
'apikeys', 'org_users', 'orgs', and 'stripe_info' using createdWebhookId,
legacyApiKeyId, policyOrgId, policyCustomerId) to include a 60_000ms timeout
(e.g., pass 60000 as the second argument) so the async cleanup has the same
allowed time as beforeAll.
- Around line 96-155: Add regression tests to assert the org API key expiration
policy is enforced for the webhook endpoints that use the alternate auth path:
/webhooks/test and /webhooks/deliveries/retry. Create two tests similar to the
existing ones that use legacyApiKeyValue (and createdWebhookId for the
test/retry cases) to call POST /webhooks/test and POST
/webhooks/deliveries/retry (with orgId and webhookId in the JSON body and using
the x-api-key header if that’s what middlewareV2 expects), then assert
response.status === 401 and response.json().error ===
'org_requires_expiring_key'; ensure you reference middlewareV2 and
checkWebhookPermissionV2 behavior by using the same auth header and fixtures
(policyOrgId, legacyApiKeyValue, createdWebhookId, BASE_URL) as the other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0ac94d7-4de2-4126-9af3-01cf9f0be613
📒 Files selected for processing (3)
supabase/functions/_backend/public/webhooks/index.tstests/webhooks-apikey-policy.test.tstests/webhooks.test.ts
|
1 participant
Summary (AI generated)
require_apikey_expirationin the shared webhook API-key permission pathorg_requires_expiring_key401 response for webhook management endpointsMotivation (AI generated)
Webhook management was still using a weaker API-key permission check than the organization endpoints. That let legacy non-expiring keys continue to manage webhooks after an org explicitly enabled the expiring-key policy.
Business Impact (AI generated)
This closes a policy-enforcement gap on an admin surface and makes Capgo's org security setting behave consistently. It reduces the risk that customers keep privileged automation running on keys they explicitly intended to phase out.
Test Plan (AI generated)
bun lintbun run supabase:with-env -- bunx vitest run tests/webhooks-apikey-policy.test.ts tests/webhooks.test.ts tests/apikeys-expiration.test.tsbun run test:backendbun run test:allGenerated with AI
Summary by CodeRabbit
New Features
Tests