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)
📝 WalkthroughWalkthroughWebhook permission checks now perform policy-aware org access validation via 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 / auth
WebhookAPI->>PermCheck: validate permission (apikey path or jwt)
alt API key path
PermCheck->>PermCheck: reject if apikey.limited_to_apps present (no_permission)
PermCheck->>PolicyEngine: apikeyHasOrgRightWithPolicy(orgId, supabaseApikey(...))
PolicyEngine->>OrgService: retrieve org policy (expiring-key requirement)
alt org requires expiring key and key 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
WebhookAPI-->>Client: proceed
end
else JWT/admin path
PermCheck->>PermCheck: perform existing admin/org checks
PermCheck-->>WebhookAPI: allow or deny based on admin check
WebhookAPI-->>Client: proceed or 401
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c235268288
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/webhooks-apikey-policy.test.ts (2)
3-3: Route these requests throughgetEndpointUrl()instead ofBASE_URL.Hard-coding
BASE_URLhere bypasses the test helper that switches targets underUSE_CLOUDFLARE_WORKERS, so this file is easier to desync from the actual backend worker selection. UsegetEndpointUrl(path)for the/apikeyand/webhooks*calls.As per coding guidelines, "Use `getEndpointUrl(path)` test helper to route to correct worker based on endpoint; use `USE_CLOUDFLARE_WORKERS=true` env var to switch backend target between Supabase and CF Workers".♻️ Suggested cleanup
-import { BASE_URL, getSupabaseClient, headers, TEST_EMAIL, USER_ID } from './test-utils.ts' +import { getEndpointUrl, getSupabaseClient, headers, TEST_EMAIL, USER_ID } from './test-utils.ts' + +const APIKEY_URL = getEndpointUrl('/apikey') +const WEBHOOKS_URL = getEndpointUrl('/webhooks') +const WEBHOOKS_TEST_URL = getEndpointUrl('/webhooks/test') +const WEBHOOKS_RETRY_URL = getEndpointUrl('/webhooks/deliveries/retry') - const keyResponse = await fetch(`${BASE_URL}/apikey`, { + const keyResponse = await fetch(APIKEY_URL, { method: 'POST', headers, @@ - const response = await fetch(`${BASE_URL}/webhooks?orgId=${policyOrgId}`, { + const response = await fetch(`${WEBHOOKS_URL}?orgId=${policyOrgId}`, { headers: { 'Content-Type': 'application/json', 'Authorization': legacyApiKeyValue,Also applies to: 47-104, 132-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/webhooks-apikey-policy.test.ts` at line 3, Tests are using BASE_URL directly which bypasses the test helper that selects the backend; update all requests in tests/webhooks-apikey-policy.test.ts (including calls to /apikey and any /webhooks* requests mentioned around lines 47-104 and 132-245) to use getEndpointUrl(path) instead of concatenating BASE_URL (e.g., replace BASE_URL + '/apikey' and BASE_URL + '/webhooks…' with getEndpointUrl('/apikey') and getEndpointUrl('/webhooks...')); keep other helpers like headers and getSupabaseClient unchanged and ensure getEndpointUrl is imported/used consistently so the USE_CLOUDFLARE_WORKERS toggle routes requests correctly.
93-104: Add one success case for a compliant expiring key.This suite only proves the reject path. Because the second
/apikeyfixture keeps onlyid, it never verifies that a valid expiring org key can still use the webhook endpoints after the policy flip. If the new guard accidentally rejected every API key, this file would still pass. Persist the returned key and assert one happy-path request with it.🤖 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 93 - 104, The test currently only stores expiringSubkeyId from the POST /apikey response so it never verifies a valid expiring org key can access webhook endpoints; update the POST assertion to capture and persist the actual API key/token returned (e.g., expand subkeyData to include the key string and store it in a new variable like expiringSubkeyKey), then perform one happy-path request to the webhook endpoint using that key (set Authorization or appropriate header) and assert a successful response (e.g., 200). Ensure references to subkeyResponse, subkeyData, expiringSubkeyId are updated to also handle and verify the returned key value.
🤖 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/webhooks-apikey-policy.test.ts`:
- Line 3: Tests are using BASE_URL directly which bypasses the test helper that
selects the backend; update all requests in tests/webhooks-apikey-policy.test.ts
(including calls to /apikey and any /webhooks* requests mentioned around lines
47-104 and 132-245) to use getEndpointUrl(path) instead of concatenating
BASE_URL (e.g., replace BASE_URL + '/apikey' and BASE_URL + '/webhooks…' with
getEndpointUrl('/apikey') and getEndpointUrl('/webhooks...')); keep other
helpers like headers and getSupabaseClient unchanged and ensure getEndpointUrl
is imported/used consistently so the USE_CLOUDFLARE_WORKERS toggle routes
requests correctly.
- Around line 93-104: The test currently only stores expiringSubkeyId from the
POST /apikey response so it never verifies a valid expiring org key can access
webhook endpoints; update the POST assertion to capture and persist the actual
API key/token returned (e.g., expand subkeyData to include the key string and
store it in a new variable like expiringSubkeyKey), then perform one happy-path
request to the webhook endpoint using that key (set Authorization or appropriate
header) and assert a successful response (e.g., 200). Ensure references to
subkeyResponse, subkeyData, expiringSubkeyId are updated to also handle and
verify the returned key value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f060d4b-9aeb-4e78-b93e-dd152a72ce11
📒 Files selected for processing (3)
supabase/functions/_backend/public/webhooks/index.tstests/organization-api.test.tstests/webhooks-apikey-policy.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/organization-api.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