feat(webhooks): enforce standard webhook API access#2244
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces legacy Capgo webhook signing with Standard Webhooks v1 (base64), moves frontend webhook CRUD to Edge Functions, migrates handlers to AuthInfo + supabaseAdmin via middlewareV2, tightens DB RLS/privileges, and adds retry scheduling and automatic disablement. ChangesStandard Webhooks Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
riderx
force-pushed
the
codex/standard-webhooks-compliance
branch
2 times, most recently
from
efb8b83 to
3f58b47
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
supabase/functions/_backend/utils/webhook.ts (1)
371-386:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the delivery timeout active until the response body is fully read.
Line 383 clears the abort timer as soon as headers arrive. A receiver can then stall
response.text()indefinitely, and an earlyfetch()failure skipsclearTimeout()entirely. This leaves an external call effectively unbounded on the worker path.💡 Suggested fix
try { const controller = new AbortController() const timeoutId = setTimeout(() => controller.abort(), WEBHOOK_DELIVERY_TIMEOUT_MS) - - const response = await fetch(url, { - method: 'POST', - headers, - body: payloadString, - redirect: 'manual', - signal: controller.signal, - }) - - clearTimeout(timeoutId) - const duration = Date.now() - startTime - const responseBody = await response.text() - const retryAfter = response.headers.get('retry-after') + try { + const response = await fetch(url, { + method: 'POST', + headers, + body: payloadString, + redirect: 'manual', + signal: controller.signal, + }) + + const duration = Date.now() - startTime + const responseBody = await response.text() + const retryAfter = response.headers.get('retry-after') - cloudlog({ - requestId: c.get('requestId'), - message: 'Webhook delivery attempt', - deliveryId, - url, - status: response.status, - success: response.ok, - duration, - }) + cloudlog({ + requestId: c.get('requestId'), + message: 'Webhook delivery attempt', + deliveryId, + url, + status: response.status, + success: response.ok, + duration, + }) - return { - success: response.ok, - status: response.status, - body: responseBody.slice(0, 10000), // Limit stored body size - duration, - retryAfter, + return { + success: response.ok, + status: response.status, + body: responseBody.slice(0, 10000), // Limit stored body size + duration, + retryAfter, + } + } + finally { + clearTimeout(timeoutId) } }🤖 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 `@supabase/functions/_backend/utils/webhook.ts` around lines 371 - 386, The abort timer is cleared immediately after fetch resolves headers, allowing a stalled response.text() to hang; also if fetch throws the timeout isn't cleared. Keep the delivery timeout active until the full body is read by moving clearTimeout(timeoutId) to after await response.text() (so the timer and AbortController.signal cover the entire read), and ensure timeoutId is cleared in a finally block so it's always cleaned up if fetch or response.text() throws; reference the AbortController, timeoutId, WEBHOOK_DELIVERY_TIMEOUT_MS, the fetch(...) call, and response.text() in the changes.supabase/functions/_backend/public/webhooks/deliveries.ts (1)
148-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing error handling for delivery update.
The update to reset delivery state doesn't check for errors. If this fails, the delivery would be queued but with incorrect state data.
Suggested fix
// Reset delivery status and queue for retry - await supabase + const { error: updateError } = await supabase .from('webhook_deliveries') .update({ status: 'pending', attempt_count: 0, response_status: null, response_body: null, completed_at: null, duration_ms: null, next_retry_at: null, }) .eq('id', body.deliveryId) + + if (updateError) { + throw simpleError('update_failed', 'Failed to reset delivery for retry', { error: updateError }) + }🤖 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 `@supabase/functions/_backend/public/webhooks/deliveries.ts` around lines 148 - 159, The update call to reset delivery state using supabase.from('webhook_deliveries').update(...).eq('id', body.deliveryId) lacks error handling; capture the result (e.g., const { data, error } = await supabase.from(...).update(...).eq(...)), check if error is truthy, and handle it (log the error with context and the deliveryId using your logger or console.error and return or throw an appropriate error/HTTP response so the caller knows the reset failed) to prevent leaving the delivery in an inconsistent state.supabase/functions/_backend/public/webhooks/put.ts (1)
80-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid returning full webhook rows from the update endpoint
This response currently returns the entire
webhooksrow (select()), which can leak sensitive columns (notably the webhook signing secret). Return an explicit safe column list instead.Suggested fix
+const webhookPublicSelect = 'id, org_id, name, url, enabled, events, created_at, updated_at, created_by' + const { data, error } = await supabase .from('webhooks') .update(updateData) .eq('id', body.webhookId) - .select() + .select(webhookPublicSelect) .single()🤖 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 `@supabase/functions/_backend/public/webhooks/put.ts` around lines 80 - 94, The update endpoint currently calls supabase.from('webhooks').update(updateData).eq('id', body.webhookId).select().single(), which returns the full row (including sensitive fields like signing_secret); change the .select() call to explicitly list only safe columns (e.g., .select('id,name,url,owner_id,created_at,updated_at') or whatever non-secret columns your webhooks table exposes) so the response does not leak secrets, keep the rest of the flow (error handling via simpleError and returning c.json with webhook: data) unchanged.
🧹 Nitpick comments (1)
tests/webhook-queue-processing.test.ts (1)
185-185: 💤 Low valueUse
it.concurrent()for parallel test execution.Per coding guidelines, tests should use
it.concurrent()instead ofit()to enable parallel execution within the same file for faster CI/CD.Suggested fix
- it('dispatches and delivers webhook queue messages end to end', { timeout: 30000 }, async () => { + it.concurrent('dispatches and delivers webhook queue messages end to end', { timeout: 30000 }, async () => {As per coding guidelines: "use it.concurrent() instead of it() to run tests in parallel within the same file for faster CI/CD"
🤖 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-queue-processing.test.ts` at line 185, The test declaration using it(...) should be changed to it.concurrent(...) to enable parallel execution; locate the test with the title "dispatches and delivers webhook queue messages end to end" (in tests/webhook-queue-processing.test.ts) and replace the it(...) call with it.concurrent(...), preserving the existing timeout option ({ timeout: 30000 }) and the async test function signature so behavior and timing remain unchanged.
🤖 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.
Inline comments:
In `@supabase/functions/_backend/public/webhooks/get.ts`:
- Line 21: The webhook signing secret is being exposed because webhookSchema
includes secret and code uses select('*'); fix by removing/excluding the secret
from outgoing responses: create a separate response schema (e.g.,
WebhookResponse or stripSecretFromWebhook) that omits the secret field and use
that for serialization, and replace any select('*') calls with explicit column
lists that do not include "secret" (or explicitly omit "secret") in all handlers
that return webhooks (references: webhookSchema, select('*') usages and the
GET/list and single endpoint handlers currently returning the schema). Ensure
you update the list and single-response paths (and any other places noted) to
return the non-secret schema.
In `@supabase/functions/_backend/public/webhooks/put.ts`:
- Around line 29-33: The handler currently uses the admin client
supabaseAdmin(c) which bypasses RLS; replace it with the user-authenticated
Supabase client obtained from the request context (the same client used for
user-facing endpoints) so RLS and user session enforcement apply. Locate the
supabase variable created via supabaseAdmin(c) in put.ts and swap it for the
authenticated client factory (the one that reads the request/session) and ensure
you still query .from('webhooks') and handle existingWebhook and fetchError as
before; also ensure the request's auth token/session is forwarded to the client
so permission checks run.
In `@supabase/functions/_backend/utils/webhook.ts`:
- Around line 300-319: The function getWebhookRetryDelaySeconds allows negative
jitter to drop the final delay below enforced minimums (Retry-After and the
5-minute throttle); fix by computing the enforced minimum first (use
WEBHOOK_RETRY_DELAYS_SECONDS[retryIndex], increase to 5*60 when status in
WEBHOOK_RETRY_THROTTLE_STATUSES, and take parseRetryAfterSeconds(retryAfter) if
present) then add jitter to that base delay and finally clamp the result so it
cannot go below that enforced minimum and cannot exceed
WEBHOOK_MAX_RETRY_AFTER_SECONDS; update the jitter logic around the symbols
getWebhookRetryDelaySeconds, WEBHOOK_RETRY_DELAYS_SECONDS,
WEBHOOK_RETRY_THROTTLE_STATUSES, parseRetryAfterSeconds, and
WEBHOOK_MAX_RETRY_AFTER_SECONDS accordingly.
---
Outside diff comments:
In `@supabase/functions/_backend/public/webhooks/deliveries.ts`:
- Around line 148-159: The update call to reset delivery state using
supabase.from('webhook_deliveries').update(...).eq('id', body.deliveryId) lacks
error handling; capture the result (e.g., const { data, error } = await
supabase.from(...).update(...).eq(...)), check if error is truthy, and handle it
(log the error with context and the deliveryId using your logger or
console.error and return or throw an appropriate error/HTTP response so the
caller knows the reset failed) to prevent leaving the delivery in an
inconsistent state.
In `@supabase/functions/_backend/public/webhooks/put.ts`:
- Around line 80-94: The update endpoint currently calls
supabase.from('webhooks').update(updateData).eq('id',
body.webhookId).select().single(), which returns the full row (including
sensitive fields like signing_secret); change the .select() call to explicitly
list only safe columns (e.g.,
.select('id,name,url,owner_id,created_at,updated_at') or whatever non-secret
columns your webhooks table exposes) so the response does not leak secrets, keep
the rest of the flow (error handling via simpleError and returning c.json with
webhook: data) unchanged.
In `@supabase/functions/_backend/utils/webhook.ts`:
- Around line 371-386: The abort timer is cleared immediately after fetch
resolves headers, allowing a stalled response.text() to hang; also if fetch
throws the timeout isn't cleared. Keep the delivery timeout active until the
full body is read by moving clearTimeout(timeoutId) to after await
response.text() (so the timer and AbortController.signal cover the entire read),
and ensure timeoutId is cleared in a finally block so it's always cleaned up if
fetch or response.text() throws; reference the AbortController, timeoutId,
WEBHOOK_DELIVERY_TIMEOUT_MS, the fetch(...) call, and response.text() in the
changes.
---
Nitpick comments:
In `@tests/webhook-queue-processing.test.ts`:
- Line 185: The test declaration using it(...) should be changed to
it.concurrent(...) to enable parallel execution; locate the test with the title
"dispatches and delivers webhook queue messages end to end" (in
tests/webhook-queue-processing.test.ts) and replace the it(...) call with
it.concurrent(...), preserving the existing timeout option ({ timeout: 30000 })
and the async test function signature so behavior and timing remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6718a486-e9da-489c-bd2c-0727a7536d6d
📒 Files selected for processing (19)
messages/en.jsonsrc/pages/settings/organization/Webhooks.vuesrc/stores/webhooks.tssupabase/functions/_backend/public/webhooks/delete.tssupabase/functions/_backend/public/webhooks/deliveries.tssupabase/functions/_backend/public/webhooks/get.tssupabase/functions/_backend/public/webhooks/index.tssupabase/functions/_backend/public/webhooks/post.tssupabase/functions/_backend/public/webhooks/put.tssupabase/functions/_backend/public/webhooks/test.tssupabase/functions/_backend/triggers/webhook_delivery.tssupabase/functions/_backend/utils/webhook.tssupabase/migrations/20260512080234_standard_webhook_secrets.sqltests/hashed-apikey-rls.test.tstests/webhook-delivery-redirect.unit.test.tstests/webhook-delivery-security.unit.test.tstests/webhook-queue-processing.test.tstests/webhook-signature.test.tstests/webhooks.test.ts
riderx
force-pushed
the
codex/standard-webhooks-compliance
branch
from
3f58b47 to
237cc74
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/stores/webhooks.ts (2)
352-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset delivery state on missing org and failed delivery fetches.
deliveriesanddeliveryPaginationstay populated when this request bails out, so switching webhooks/orgs and then hitting an API error can display the previous webhook’s payloads and response bodies under the new selection.💡 Suggested fix
if (!orgId) { + deliveries.value = [] + deliveryPagination.value = null console.error('No organization selected') return } @@ if (error) { + deliveries.value = [] + deliveryPagination.value = null console.error('Failed to fetch deliveries:', error) return } @@ catch (err) { + deliveries.value = [] + deliveryPagination.value = null console.error('Error fetching deliveries:', err) }🤖 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 `@src/stores/webhooks.ts` around lines 352 - 380, The deliveries list and pagination are not cleared when the fetch bails (no orgId) or fails, causing old data to show; update the logic in the block that calls supabase.functions.invoke (using buildWebhookFunctionPath) so that when orgId is missing, when the response has error, or in the catch handler you explicitly set deliveries.value = [] and deliveryPagination.value = null before returning, and retain isLoadingDeliveries.value reset in the finally block.
69-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear cached webhooks when org context is missing or the fetch fails.
These early returns leave the previous org’s webhook list in Pinia state, so a failed org switch can keep showing stale webhook URLs/events from the last org in the current view.
💡 Suggested fix
if (!orgId) { + webhooks.value = [] console.error('No organization selected') return } @@ if (error) { + webhooks.value = [] console.error('Failed to fetch webhooks:', error) return } @@ catch (err) { + webhooks.value = [] console.error('Error fetching webhooks:', err) }🤖 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 `@src/stores/webhooks.ts` around lines 69 - 89, The early returns in the webhook fetch leave stale webhooks in Pinia; update the fetch logic around orgId, the supabase.functions.invoke error branch, and the catch block to clear cached webhooks and reset loading — specifically set webhooks.value = [] (and ensure isLoading.value = false) before returning when orgId is missing, when the invoke returns an error, and inside the catch block; locate these changes around the orgId check, the try/await call to supabase.functions.invoke(buildWebhookFunctionPath('webhooks', { orgId })), and the error handling surrounding isLoading.value and webhooks.value.src/pages/settings/organization/Webhooks.vue (1)
205-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the verification example aligned with legacy secret decoding.
The backend still signs non-
whsec_secrets as raw UTF-8 bytes, and it also falls back to raw text for malformed historicalwhsec_values. This sample always base64-decodes, so it will reject some existing secrets that the server still accepts.💡 Suggested fix
+function decodeWebhookSecret(secret) { + if (!secret.startsWith('whsec_')) + return Buffer.from(secret, 'utf8') + + const decoded = Buffer.from(secret.slice('whsec_'.length), 'base64') + return decoded.length >= 24 && decoded.length <= 64 + ? decoded + : Buffer.from(secret, 'utf8') +} + function verifyWebhookSignature(rawBody, headers, secret) { @@ - const secretBytes = Buffer.from(secret.replace(/^whsec_/, ''), 'base64') + const secretBytes = decodeWebhookSecret(secret) const signaturePayload = `${messageId}.${timestamp}.${rawBody}` const hmac = crypto.createHmac('sha256', secretBytes)🤖 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 `@src/pages/settings/organization/Webhooks.vue` around lines 205 - 221, The verifyWebhookSignature function currently always base64-decodes the secret (after stripping whsec_), which mismatches the backend that accepts raw UTF-8 secrets and falls back to raw text when historical whsec_ values are malformed; update verifyWebhookSignature to: if secret startsWith 'whsec_' attempt to base64-decode secret.slice(7) inside a try/catch and on decode error fall back to Buffer.from(originalSuffix, 'utf8'); if secret does not start with 'whsec_' use Buffer.from(secret, 'utf8'); then proceed to build signaturePayload and compute expectedSignature as before.
🤖 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.
Inline comments:
In `@supabase/functions/_backend/utils/webhook.ts`:
- Around line 379-408: The current code calls response.text() which loads the
entire response into memory; replace that with a streaming read that caps bytes
read (e.g., 10000) to avoid large allocations: add a helper like
readWebhookResponsePreview(response, maxBytes) that uses
response.body?.getReader(), a TextDecoder, reads chunks until maxBytes reached
or stream ends, decodes only the needed subarray, calls reader.cancel() and
final decoder.decode(), and returns the preview string; then use that helper
instead of response.text() when setting responseBody in the webhook delivery
logic (the try block that currently assigns responseBody and returns the
object).
---
Outside diff comments:
In `@src/pages/settings/organization/Webhooks.vue`:
- Around line 205-221: The verifyWebhookSignature function currently always
base64-decodes the secret (after stripping whsec_), which mismatches the backend
that accepts raw UTF-8 secrets and falls back to raw text when historical whsec_
values are malformed; update verifyWebhookSignature to: if secret startsWith
'whsec_' attempt to base64-decode secret.slice(7) inside a try/catch and on
decode error fall back to Buffer.from(originalSuffix, 'utf8'); if secret does
not start with 'whsec_' use Buffer.from(secret, 'utf8'); then proceed to build
signaturePayload and compute expectedSignature as before.
In `@src/stores/webhooks.ts`:
- Around line 352-380: The deliveries list and pagination are not cleared when
the fetch bails (no orgId) or fails, causing old data to show; update the logic
in the block that calls supabase.functions.invoke (using
buildWebhookFunctionPath) so that when orgId is missing, when the response has
error, or in the catch handler you explicitly set deliveries.value = [] and
deliveryPagination.value = null before returning, and retain
isLoadingDeliveries.value reset in the finally block.
- Around line 69-89: The early returns in the webhook fetch leave stale webhooks
in Pinia; update the fetch logic around orgId, the supabase.functions.invoke
error branch, and the catch block to clear cached webhooks and reset loading —
specifically set webhooks.value = [] (and ensure isLoading.value = false) before
returning when orgId is missing, when the invoke returns an error, and inside
the catch block; locate these changes around the orgId check, the try/await call
to supabase.functions.invoke(buildWebhookFunctionPath('webhooks', { orgId })),
and the error handling surrounding isLoading.value and webhooks.value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e5e0e25-39b7-45f2-af10-c143150382bd
📒 Files selected for processing (22)
messages/en.jsonsrc/components/WebhookDeliveryLog.vuesrc/components/WebhookForm.vuesrc/pages/settings/organization/Webhooks.vuesrc/stores/webhooks.tssupabase/functions/_backend/public/webhooks/delete.tssupabase/functions/_backend/public/webhooks/deliveries.tssupabase/functions/_backend/public/webhooks/get.tssupabase/functions/_backend/public/webhooks/index.tssupabase/functions/_backend/public/webhooks/post.tssupabase/functions/_backend/public/webhooks/put.tssupabase/functions/_backend/public/webhooks/response.tssupabase/functions/_backend/public/webhooks/test.tssupabase/functions/_backend/triggers/webhook_delivery.tssupabase/functions/_backend/utils/webhook.tssupabase/migrations/20260512080234_standard_webhook_secrets.sqltests/hashed-apikey-rls.test.tstests/webhook-delivery-redirect.unit.test.tstests/webhook-delivery-security.unit.test.tstests/webhook-queue-processing.test.tstests/webhook-signature.test.tstests/webhooks.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/webhook-delivery-redirect.unit.test.ts
- supabase/functions/_backend/public/webhooks/response.ts
- tests/webhook-delivery-security.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- supabase/functions/_backend/public/webhooks/delete.ts
- supabase/functions/_backend/public/webhooks/index.ts
- supabase/functions/_backend/public/webhooks/post.ts
- supabase/functions/_backend/public/webhooks/deliveries.ts
- tests/webhooks.test.ts
- tests/webhook-queue-processing.test.ts
- messages/en.json
- supabase/functions/_backend/triggers/webhook_delivery.ts
- supabase/functions/_backend/public/webhooks/test.ts
- tests/hashed-apikey-rls.test.ts
- tests/webhook-signature.test.ts
- supabase/migrations/20260512080234_standard_webhook_secrets.sql
|
Review note: this still logs raw webhook URLs in delivery paths, which can expose customer endpoint secrets. In I would replace the raw |
|
Second review note: The code does I would use a bounded stream reader here, similar to the helper introduced in other hardening PRs, and stop reading once the response preview limit is exceeded. A unit test with a chunked response larger than the preview cap would cover the regression. |
riderx
force-pushed
the
codex/standard-webhooks-compliance
branch
from
237cc74 to
af9a70f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
supabase/functions/_backend/utils/webhook.ts (1)
219-241: 💤 Low valueVerify edge case handling in
decodeSerializedWebhookSecret.The function handles both new
whsec_prefixed base64 secrets and legacy raw-text secrets. However,codePointAt(i)can returnundefinedfor indices beyond the string length, and the fallback?? 0handles this. The length check (24-64 bytes) ensures compatibility with Standard Webhooks symmetric keys.Consider adding a brief comment explaining the byte length validation for future maintainers.
🤖 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 `@supabase/functions/_backend/utils/webhook.ts` around lines 219 - 241, Summary: Add a clarifying comment about the byte-length check in decodeSerializedWebhookSecret. Update the decodeSerializedWebhookSecret function to include a short inline comment immediately above the bytes.length validation explaining that Standard Webhooks symmetric keys are expected to be 24–64 random bytes (and that existing Capgo hex strings decode to 24 bytes), so the length check allows base64-decoded whsec_ secrets to be treated as binary keys while falling back to legacy raw-text signing otherwise; keep existing handling of codePointAt(i) with the ?? 0 fallback unchanged.
🤖 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.
Inline comments:
In `@tests/webhooks.test.ts`:
- Line 5: Replace usages of the BASE_URL constant with the getEndpointUrl(path)
test helper in this test file so webhook endpoint calls route to the correct
backend in worker-mode; locate any references to BASE_URL (imported from
test-utils.ts and used to construct webhook URLs in tests/webhooks.test.ts) and
change them to call getEndpointUrl('/your/webhook/path') or compose the path via
getEndpointUrl, ensuring imports include getEndpointUrl and removing BASE_URL
where no longer needed.
- Around line 195-240: The tests rely on file-scoped mutable state
(createdWebhookId, lastDeliveryId) and explicit ordering; remove that dependency
by giving each test its own setup/teardown: replace uses of
createdWebhookId/lastDeliveryId with per-test values returned from a factory
helper (e.g., createTestWebhook(), createTestDelivery()) invoked in a beforeEach
or inline inside each it, and update tests that call
getAuthenticatedAnonClient()/fetchWithRetry() to operate on those per-test IDs;
alternatively, if ordering truly cannot be avoided, add a top-of-file comment
and test-runner annotation to mark the file as sequential (i.e., disable
concurrent execution) and document why.
---
Nitpick comments:
In `@supabase/functions/_backend/utils/webhook.ts`:
- Around line 219-241: Summary: Add a clarifying comment about the byte-length
check in decodeSerializedWebhookSecret. Update the decodeSerializedWebhookSecret
function to include a short inline comment immediately above the bytes.length
validation explaining that Standard Webhooks symmetric keys are expected to be
24–64 random bytes (and that existing Capgo hex strings decode to 24 bytes), so
the length check allows base64-decoded whsec_ secrets to be treated as binary
keys while falling back to legacy raw-text signing otherwise; keep existing
handling of codePointAt(i) with the ?? 0 fallback unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb8a2102-7826-4d68-8380-5879f57305cc
📒 Files selected for processing (22)
messages/en.jsonsrc/components/WebhookDeliveryLog.vuesrc/components/WebhookForm.vuesrc/pages/settings/organization/Webhooks.vuesrc/stores/webhooks.tssupabase/functions/_backend/public/webhooks/delete.tssupabase/functions/_backend/public/webhooks/deliveries.tssupabase/functions/_backend/public/webhooks/get.tssupabase/functions/_backend/public/webhooks/index.tssupabase/functions/_backend/public/webhooks/post.tssupabase/functions/_backend/public/webhooks/put.tssupabase/functions/_backend/public/webhooks/response.tssupabase/functions/_backend/public/webhooks/test.tssupabase/functions/_backend/triggers/webhook_delivery.tssupabase/functions/_backend/utils/webhook.tssupabase/migrations/20260512080234_standard_webhook_secrets.sqltests/hashed-apikey-rls.test.tstests/webhook-delivery-redirect.unit.test.tstests/webhook-delivery-security.unit.test.tstests/webhook-queue-processing.test.tstests/webhook-signature.test.tstests/webhooks.test.ts
✅ Files skipped from review due to trivial changes (2)
- tests/webhook-delivery-redirect.unit.test.ts
- src/components/WebhookDeliveryLog.vue
🚧 Files skipped from review as they are similar to previous changes (13)
- supabase/functions/_backend/public/webhooks/response.ts
- supabase/functions/_backend/triggers/webhook_delivery.ts
- supabase/functions/_backend/public/webhooks/test.ts
- supabase/functions/_backend/public/webhooks/index.ts
- src/components/WebhookForm.vue
- tests/hashed-apikey-rls.test.ts
- supabase/functions/_backend/public/webhooks/put.ts
- tests/webhook-queue-processing.test.ts
- supabase/functions/_backend/public/webhooks/post.ts
- supabase/migrations/20260512080234_standard_webhook_secrets.sql
- supabase/functions/_backend/public/webhooks/get.ts
- supabase/functions/_backend/public/webhooks/deliveries.ts
- tests/webhook-signature.test.ts
riderx
force-pushed
the
codex/standard-webhooks-compliance
branch
from
af9a70f to
899cb44
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@supabase/functions/_backend/public/webhooks/test.ts`:
- Around line 82-85: The update to attempt_count on the webhook_deliveries row
is not checked; modify the call to await
supabase.from('webhook_deliveries').update(...) .eq('id', delivery.id) to
capture the result and error, verify that result.error or status indicates
success, and if it failed log the error (using the existing logger) and
return/throw an HTTP error or non-2xx response instead of continuing. Ensure you
reference the same update call and delivery.id, and keep behavior consistent
with other DB error handling in this file (e.g., propagate failure back to the
caller and avoid returning success when the write did not persist).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5b73094-7bee-43ed-b355-c9d2ed909780
📒 Files selected for processing (22)
messages/en.jsonsrc/components/WebhookDeliveryLog.vuesrc/components/WebhookForm.vuesrc/pages/settings/organization/Webhooks.vuesrc/stores/webhooks.tssupabase/functions/_backend/public/webhooks/delete.tssupabase/functions/_backend/public/webhooks/deliveries.tssupabase/functions/_backend/public/webhooks/get.tssupabase/functions/_backend/public/webhooks/index.tssupabase/functions/_backend/public/webhooks/post.tssupabase/functions/_backend/public/webhooks/put.tssupabase/functions/_backend/public/webhooks/response.tssupabase/functions/_backend/public/webhooks/test.tssupabase/functions/_backend/triggers/webhook_delivery.tssupabase/functions/_backend/utils/webhook.tssupabase/migrations/20260512080234_standard_webhook_secrets.sqltests/hashed-apikey-rls.test.tstests/webhook-delivery-redirect.unit.test.tstests/webhook-delivery-security.unit.test.tstests/webhook-queue-processing.test.tstests/webhook-signature.test.tstests/webhooks.test.ts
✅ Files skipped from review due to trivial changes (2)
- messages/en.json
- supabase/functions/_backend/public/webhooks/index.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- tests/webhook-delivery-redirect.unit.test.ts
- supabase/functions/_backend/public/webhooks/delete.ts
- supabase/functions/_backend/triggers/webhook_delivery.ts
- src/components/WebhookDeliveryLog.vue
- tests/hashed-apikey-rls.test.ts
- src/components/WebhookForm.vue
- tests/webhook-queue-processing.test.ts
- tests/webhook-delivery-security.unit.test.ts
- supabase/functions/_backend/public/webhooks/response.ts
- supabase/functions/_backend/public/webhooks/deliveries.ts
- supabase/functions/_backend/public/webhooks/post.ts
- supabase/migrations/20260512080234_standard_webhook_secrets.sql
- supabase/functions/_backend/public/webhooks/get.ts
- src/pages/settings/organization/Webhooks.vue
- tests/webhook-signature.test.ts
- supabase/functions/_backend/utils/webhook.ts
- supabase/functions/_backend/public/webhooks/put.ts
- src/stores/webhooks.ts
- tests/webhooks.test.ts
riderx
force-pushed
the
codex/standard-webhooks-compliance
branch
from
899cb44 to
59e20a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/utils/webhook.ts (1)
426-433:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop logging raw webhook URLs.
These log entries still include the full
url, which can carry shared secrets in query params or credentials. Please log sanitized metadata instead, otherwise delivery logs become a secret-leak surface.💡 Suggested shape
+function getWebhookUrlLogMeta(url: string) { + try { + const parsed = new URL(url) + return { + protocol: parsed.protocol, + hostname: parsed.hostname, + pathSegmentCount: parsed.pathname.split('/').filter(Boolean).length, + hasQuery: parsed.search.length > 0, + hasCredentials: Boolean(parsed.username || parsed.password), + } + } + catch { + return { invalidUrl: true } + } +} + if (urlValidationError) { const duration = Date.now() - startTime cloudlogErr({ requestId: c.get('requestId'), message: 'Webhook delivery blocked by URL validation', deliveryId, - url, + webhookTarget: getWebhookUrlLogMeta(url), error: urlValidationError, duration, }) @@ cloudlog({ requestId: c.get('requestId'), message: 'Webhook delivery attempt', deliveryId, - url, + webhookTarget: getWebhookUrlLogMeta(url), status: response.status, success: response.ok, duration, }) @@ cloudlogErr({ requestId: c.get('requestId'), message: 'Webhook delivery failed', deliveryId, - url, + webhookTarget: getWebhookUrlLogMeta(url), error: errorMessage, duration, })Also worth adding a unit test that verifies logs never contain a secret-bearing URL like
?token=secret.Also applies to: 483-491, 509-516
🤖 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 `@supabase/functions/_backend/utils/webhook.ts` around lines 426 - 433, The cloudlogErr calls in webhook.ts are logging raw webhook URLs (e.g., the call that passes url to cloudlogErr with message "Webhook delivery blocked by URL validation"), which can leak secrets; update those logging sites (the cloudlogErr invocations around the deliveryId/url/error/duration blocks and the similar blocks at the other noted ranges) to log a sanitizedUrl or metadata instead (e.g., host, pathname, and a redacted query string or a boolean indicating presence of query params) rather than the full url, and add a unit test that constructs a webhook URL containing a secret query param (like ?token=secret) and asserts that the captured logs do not contain the secret or the full URL.
🤖 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.
Inline comments:
In `@supabase/migrations/20260512080234_standard_webhook_secrets.sql`:
- Around line 37-38: Add a backfill UPDATE after the ALTER TABLE to bring
in-flight deliveries to the new policy: run an UPDATE on webhook_deliveries (the
same migration file) targeting rows where state = 'pending' and max_attempts is
NULL or less than 10, setting max_attempts = 10 (or use greatest(max_attempts,
10) if you prefer to preserve higher values); reference the table
webhook_deliveries and column max_attempts and ensure this UPDATE runs in the
same migration after ALTER COLUMN so existing pending deliveries adopt the new
default.
---
Outside diff comments:
In `@supabase/functions/_backend/utils/webhook.ts`:
- Around line 426-433: The cloudlogErr calls in webhook.ts are logging raw
webhook URLs (e.g., the call that passes url to cloudlogErr with message
"Webhook delivery blocked by URL validation"), which can leak secrets; update
those logging sites (the cloudlogErr invocations around the
deliveryId/url/error/duration blocks and the similar blocks at the other noted
ranges) to log a sanitizedUrl or metadata instead (e.g., host, pathname, and a
redacted query string or a boolean indicating presence of query params) rather
than the full url, and add a unit test that constructs a webhook URL containing
a secret query param (like ?token=secret) and asserts that the captured logs do
not contain the secret or the full URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1deceb20-0a88-40c6-b703-db342ff42642
📒 Files selected for processing (26)
messages/en.jsonsrc/auto-imports.d.tssrc/components/WebhookDeliveryLog.vuesrc/components/WebhookForm.vuesrc/pages/settings/organization/Webhooks.vuesrc/stores/webhooks.tssrc/types/supabase.types.tssupabase/functions/_backend/public/webhooks/delete.tssupabase/functions/_backend/public/webhooks/deliveries.tssupabase/functions/_backend/public/webhooks/get.tssupabase/functions/_backend/public/webhooks/index.tssupabase/functions/_backend/public/webhooks/post.tssupabase/functions/_backend/public/webhooks/put.tssupabase/functions/_backend/public/webhooks/response.tssupabase/functions/_backend/public/webhooks/test.tssupabase/functions/_backend/triggers/webhook_delivery.tssupabase/functions/_backend/triggers/webhook_dispatcher.tssupabase/functions/_backend/utils/supabase.types.tssupabase/functions/_backend/utils/webhook.tssupabase/migrations/20260512080234_standard_webhook_secrets.sqltests/hashed-apikey-rls.test.tstests/webhook-delivery-redirect.unit.test.tstests/webhook-delivery-security.unit.test.tstests/webhook-queue-processing.test.tstests/webhook-signature.test.tstests/webhooks.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/components/WebhookDeliveryLog.vue
- src/types/supabase.types.ts
- supabase/functions/_backend/utils/supabase.types.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/webhook-delivery-redirect.unit.test.ts
- supabase/functions/_backend/public/webhooks/response.ts
- supabase/functions/_backend/public/webhooks/deliveries.ts
- supabase/functions/_backend/triggers/webhook_delivery.ts
- supabase/functions/_backend/public/webhooks/index.ts
- supabase/functions/_backend/public/webhooks/get.ts
- tests/webhook-queue-processing.test.ts
- tests/webhook-signature.test.ts
- supabase/functions/_backend/public/webhooks/post.ts
- tests/webhook-delivery-security.unit.test.ts
- src/stores/webhooks.ts
|
I think this should require |
|
One more independent pagination issue:
|
|
The new |
|
|
There is still one raw webhook URL log before the new sanitization layer runs. This should probably use |
|
There is one more raw webhook URL egress path outside the delivery logs. After max retries, Webhook receiver URLs can carry bearer tokens or signed query params, so this still exports the full secret-bearing URL to the notification provider (and to logs on Bento failure), even though the delivery/logging paths now use sanitized |
KCDaemon
left a comment
KCDaemon
left a comment
There was a problem hiding this comment.
Rechecked the latest head (3f185ac). The URL redaction work in deliverWebhook() is good, but the delivery trigger still leaks the full customer webhook URL in two remaining paths.
In supabase/functions/_backend/triggers/webhook_delivery.ts, the initial cloudlog() includes url: deliveryData.url before validation/delivery. Later, after max retries, the sendNotifOrg() payload includes webhook_url: webhook.url. Both can contain customer secrets in query strings, path tokens, or embedded credentials. This PR already added getWebhookLogUrlMetadata() for safer logging elsewhere, so these remaining full-URL paths are inconsistent with the new privacy boundary.
I would keep this blocked until the trigger uses redacted/metadata-only URL information for logs and notification variables, or otherwise guarantees those sinks are safe for full customer URLs. The current GitHub status also has a failing Run tests job. git diff --check origin/main...origin/pr-2244 passes locally.
5 participants
Summary (AI generated)
type,webhook-id,webhook-timestamp, andwebhook-signatureheaders while keeping legacyX-Capgo-*headers for compatibility.webhooks/webhook_deliveriestable access away from anon/authenticated Supabase SDK clients.Retry-Afterhandling, throttling for load-related responses, 410 auto-disable, and disable-after-max-attempts.Motivation (AI generated)
Webhook secrets and delivery payloads should not be readable or mutable directly from the console Supabase SDK. Centralizing CRUD through API handlers removes duplicated access logic and gives us one authorization path while moving the delivery protocol closer to the Standard Webhooks spec: https://github.com/standard-webhooks/standard-webhooks/blob/main/spec/standard-webhooks.md
Business Impact (AI generated)
This reduces the risk of webhook secret exposure, keeps webhook behavior consistent between API users and the console, and improves interoperability for customers using Standard Webhooks-compatible verification tooling without breaking existing Capgo webhook consumers.
Test Plan (AI generated)
bun run lint:sql supabase/migrations/20260512080234_standard_webhook_secrets.sqlbun lintbun run lint:backendbun typecheckbunx vitest run tests/webhook-delivery-security.unit.test.ts tests/webhook-delivery-redirect.unit.test.ts tests/webhook-signature.test.ts tests/webhooks.test.ts tests/webhooks-apikey-policy.test.ts tests/webhook-queue-processing.test.tsbunx vitest run tests/hashed-apikey-rls.test.tsbun test:backendGenerated with AI
Summary by CodeRabbit
New Features
Security Improvements
UI
Tests