Repository navigation
Add package-centered inbound webhook endpoints - #910
Conversation
Introduce user-owned POST /@:username/webhooks/:endpointId/:urlSecret ingress that dispatches to a bound saved-package export, with hashed URL secrets, optional encrypted HMAC verification, ack/sync response modes, delivery debug logs, MCP webhooks domain capabilities, and docs.
Declare webhooks in package.json#kody.webhooks (export + optional secretName verification). Minting activates ingress at /@:username/webhooks/:packageKodyId/:webhookName/:urlSecret. Replace standalone endpoint CRUD with list/mint/rotate/enable/disable/delivery capabilities; keep HMAC, rate limits, delivery logs, and isolation.
📝 WalkthroughWalkthroughAdds package-declared inbound webhooks with manifest validation, D1 endpoint and delivery storage, MCP management capabilities, secure public ingress, package export dispatch, lifecycle tests, migration updates, and user and architecture documentation. ChangesInbound webhooks
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
🔎 Preview deployed: https://kody-pr-910.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/worker/src/webhooks/http.ts (1)
73-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMessage hardcodes the payload limit.
tooLargeResponsestates "1 MB limit" while the actual cap iswebhookMaxPayloadBytes; if that constant changes, this string silently drifts.🤖 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 `@packages/worker/src/webhooks/http.ts` around lines 73 - 84, Update tooLargeResponse to derive the error message’s payload limit from webhookMaxPayloadBytes instead of hardcoding “1 MB,” formatting the value appropriately for users while preserving the existing response structure and 413 status.packages/worker/src/webhooks/repo.ts (1)
293-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClamp reuses a hardcoded
50instead of the shared retention constant.
webhookDeliveriesRetainedPerEndpoint(types.ts) already defines the retention cap; hardcoding50again here means the two values can silently drift if the constant changes.♻️ Proposed fix
-import { - type WebhookDeliveryOutcome, - type WebhookDeliveryRecord, - type WebhookEndpointRecord, - webhookDeliveriesRetainedPerEndpoint, - webhookDeliveryErrorMaxLength, -} from './types.ts' +import { + type WebhookDeliveryOutcome, + type WebhookDeliveryRecord, + type WebhookEndpointRecord, + webhookDeliveriesRetainedPerEndpoint, + webhookDeliveryErrorMaxLength, +} from './types.ts' @@ - const limit = Math.min(Math.max(input.limit ?? 50, 1), 50) + const limit = Math.min( + Math.max(input.limit ?? webhookDeliveriesRetainedPerEndpoint, 1), + webhookDeliveriesRetainedPerEndpoint, + )🤖 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 `@packages/worker/src/webhooks/repo.ts` around lines 293 - 312, Update listWebhookDeliveriesForEndpoint to use the shared webhookDeliveriesRetainedPerEndpoint retention constant from types.ts for both limit clamping and the default limit, removing the hardcoded 50 values while preserving the existing minimum of 1 and query behavior.packages/worker/src/webhooks/service.ts (1)
143-175: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential manifest loads for each saved package.
Each package's manifest is awaited one at a time inside the
forloop; for users with several saved packages this serializes what could be independent I/O.♻️ Proposed fix
- const listed: Array<ListedWebhook> = [] - for (const savedPackage of filteredPackages) { - const loaded = await loadPackageManifestBySourceId({ - env: input.env, - baseUrl: input.baseUrl, - userId: input.userId, - sourceId: savedPackage.sourceId, - }).catch((error) => { - console.warn('Failed to load package manifest for webhooks', { - packageId: savedPackage.id, - sourceId: savedPackage.sourceId, - error, - }) - return null - }) - if (!loaded) continue - for (const webhook of listPackageWebhooks(loaded.manifest)) { + const listed: Array<ListedWebhook> = [] + const loadedManifests = await Promise.all( + filteredPackages.map((savedPackage) => + loadPackageManifestBySourceId({ + env: input.env, + baseUrl: input.baseUrl, + userId: input.userId, + sourceId: savedPackage.sourceId, + }) + .then((loaded) => ({ savedPackage, loaded })) + .catch((error) => { + console.warn('Failed to load package manifest for webhooks', { + packageId: savedPackage.id, + sourceId: savedPackage.sourceId, + error, + }) + return { savedPackage, loaded: null } + }), + ), + ) + for (const { savedPackage, loaded } of loadedManifests) { + if (!loaded) continue + for (const webhook of listPackageWebhooks(loaded.manifest)) {🤖 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 `@packages/worker/src/webhooks/service.ts` around lines 143 - 175, Update the manifest-loading flow around loadPackageManifestBySourceId so independent saved package loads run concurrently rather than sequentially inside the for loop. Preserve the existing per-package warning context, null-on-failure behavior, webhook listing, and minted metadata mapping while processing the concurrently loaded results.
🤖 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 `@packages/worker/src/app/router.ts`:
- Line 302: Add anonymous POST coverage for the webhook ingress route through
the app router, using a valid webhook URL without authentication. Assert that
path/secret validation occurs before account lease or persistence operations and
that the request cannot write data for another user.
In `@packages/worker/src/package-registry/types.ts`:
- Around line 71-77: Update packageWebhookVerificationSchema so
verification.header is validated as an HTTP field name using only valid token
characters, rejecting whitespace, newline, and other invalid values rather than
relying on min(1). Add a schema rejection test covering an invalid header value.
In `@packages/worker/src/webhooks/http.ts`:
- Around line 305-397: Move the checkRateLimit call for the resolved endpoint to
immediately after the endpoint existence/enabled guard, before secret validation
and any recordDelivery calls. Return rateLimitedResponse on disallowed requests
without persisting a delivery row, then remove the later rate-limit block while
preserving all existing rejection handling.
In `@packages/worker/src/webhooks/service.ts`:
- Around line 214-241: Replace the check-then-act flow around
getWebhookEndpointByKey, insertWebhookEndpoint, and updateWebhookEndpointSecret
with a single repository-level upsertWebhookEndpoint operation keyed by userId,
packageId, and webhookName. Ensure the upsert atomically inserts or rotates
urlSecretHash and rotatedAt on conflict, then preserve the existing
missing-record error handling.
---
Nitpick comments:
In `@packages/worker/src/webhooks/http.ts`:
- Around line 73-84: Update tooLargeResponse to derive the error message’s
payload limit from webhookMaxPayloadBytes instead of hardcoding “1 MB,”
formatting the value appropriately for users while preserving the existing
response structure and 413 status.
In `@packages/worker/src/webhooks/repo.ts`:
- Around line 293-312: Update listWebhookDeliveriesForEndpoint to use the shared
webhookDeliveriesRetainedPerEndpoint retention constant from types.ts for both
limit clamping and the default limit, removing the hardcoded 50 values while
preserving the existing minimum of 1 and query behavior.
In `@packages/worker/src/webhooks/service.ts`:
- Around line 143-175: Update the manifest-loading flow around
loadPackageManifestBySourceId so independent saved package loads run
concurrently rather than sequentially inside the for loop. Preserve the existing
per-package warning context, null-on-failure behavior, webhook listing, and
minted metadata mapping while processing the concurrently loaded results.
🪄 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 Plus
Run ID: 488d4f71-fd7f-4b27-bab6-df12d36b2c61
📒 Files selected for processing (46)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/index.mddocs/contributing/architecture/primitives.yamldocs/contributing/architecture/request-lifecycle.mddocs/contributing/architecture/webhooks.mddocs/use/index.mddocs/use/packages.mddocs/use/privacy.mddocs/use/webhooks.mddocs/use/what-can-kody-do.mdpackages/worker/migrations/0090-webhook-endpoints.sqlpackages/worker/src/app/account-data-targets.tspackages/worker/src/app/handlers/webhook-ingress.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/index.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/webhooks/domain.tspackages/worker/src/mcp/capabilities/webhooks/shared.tspackages/worker/src/mcp/capabilities/webhooks/webhook-capabilities.node.test.tspackages/worker/src/mcp/capabilities/webhooks/webhook-delivery-list.tspackages/worker/src/mcp/capabilities/webhooks/webhook-disable.tspackages/worker/src/mcp/capabilities/webhooks/webhook-enable.tspackages/worker/src/mcp/capabilities/webhooks/webhook-list.tspackages/worker/src/mcp/capabilities/webhooks/webhook-url-mint.tspackages/worker/src/mcp/capabilities/webhooks/webhook-url-rotate.tspackages/worker/src/mcp/tools/search-entity-registry.node.test.tspackages/worker/src/mcp/tools/search-package-rows.tspackages/worker/src/mcp/tools/search.node.test.tspackages/worker/src/package-registry/manifest.node.test.tspackages/worker/src/package-registry/manifest.tspackages/worker/src/package-registry/types.tspackages/worker/src/webhooks/crypto.node.test.tspackages/worker/src/webhooks/crypto.tspackages/worker/src/webhooks/headers.tspackages/worker/src/webhooks/http.tspackages/worker/src/webhooks/http.workers.test.tspackages/worker/src/webhooks/public-url.tspackages/worker/src/webhooks/repo.tspackages/worker/src/webhooks/service.node.test.tspackages/worker/src/webhooks/service.tspackages/worker/src/webhooks/types.tspackages/worker/wrangler.jsonctools/check-migrations.node.test.tstools/migration-ledger.json
| profileAvatar: createProfileAvatarHandler(env), | ||
| profileOgImage: createProfileOgImageHandler(env), | ||
| profileFollowApiPost: createProfileFollowApiPostHandler(env), | ||
| webhookIngress: createWebhookIngressHandler(env), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "## repo files matching router/handler names"
git ls-files | rg 'packages/worker/src/app/router\.ts|webhook|writeLease|middleware' | head -200
echo
echo "## packages/worker/src/app/router.ts size"
wc -l packages/worker/src/app/router.ts
echo
echo "## relevant router.ts slices"
sed -n '140,185p' packages/worker/src/app/router.ts
sed -n '280,315p' packages/worker/src/app/router.ts
echo
echo "## createAccountWriteLeaseMiddleware references"
rg -n "createAccountWriteLeaseMiddleware|createAppRouter|Middleware|middleware|execute" packages/worker/src -SRepository: kentcdodds/kody
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "## account-write-lease-middleware.ts"
sed -n '1,180p' packages/worker/src/app/account-write-lease-middleware.ts
echo
echo "## account-write-lease-middleware.node.test.ts"
sed -n '1,140p' packages/worker/src/app/account-write-lease-middleware.node.test.ts
echo
echo "## webhook-ingress handler"
sed -n '1,160p' packages/worker/src/app/handlers/webhook-ingress.ts
echo
echo "## router route maps and app handler"
sed -n '24,45p' packages/worker/src/app/routes.ts
sed -n '1,80p' packages/worker/src/app/handler.ts
echo "## package references and lock versions for `@remix-run/router` and Remix"
rg -n "`@remix-run/router`|react-router|remix" package.json . --glob 'package.json' --glob 'pnpm-lock.yaml' --glob 'package-lock.json' --glob 'yarn.lock' || true
echo
echo "## static behavioral probe of middleware array semantics from source"
python3 - <<'PY'
from pathlib import Path
p = Path('packages/worker/src/app/router.ts')
text = p.read_text()
print('router middleware list present:', 'middleware: [createAccountWriteLeaseMiddleware(env)]' in text)
print('webhookIngress action middleware omitted:', 'webhookIngress: createWebhookIngressHandler(env),' in text)
print('webhookIngress handler middleware field exists in repo files:')
for path in Path('packages/worker/src/app/handlers').glob('*.ts'):
if path.name == 'webhook-ingress.ts':
t = path.read_text()
for i, line in enumerate(t.splitlines(), 1):
if 'middleware' in line.lower():
print(f'{path}:{i}:{line.strip()}')
PYRepository: kentcdodds/kody
Length of output: 26592
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "## local remixed fetch-router availability"
if [ -d packages/worker/node_modules/@remix-run/fetch-router ]; then
echo found packages/worker/node_modules/@remix-run/fetch-router
fd . packages/worker/node_modules/@remix-run/fetch-router -e ts 2>/dev/null | head -50
rg -n "action.*middleware|middleware:.*action|routeMiddleware|executeRoute|executeHandler" packages/worker/node_modules/@remix-run/fetch-router -S --max-count 200
else
echo "no packages/worker/node_modules/@remix-run/fetch-router"
fi
echo
echo "## routes action middleware tests/usages"
rg -n "middleware:\s*\[\]|middleware:\s*\(|middleware.*action|createAppRouter\(" packages/worker/src/app packages/worker/src -S --glob '*.ts' | head -200
echo
echo "## webhook ingress tests"
rg -n "webhookIngress|WebhookIngress|handleWebhookIngressRequest|createWebhookIngressHandler" packages/worker/src packages/worker/test -SRepository: kentcdodds/kody
Length of output: 14958
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "## index.ts webhook ingress early intercept"
sed -n '430,490p' packages/worker/src/index.ts
echo
echo "## webhooks/http.ts ingress validation"
sed -n '30,85p' packages/worker/src/webhooks/http.ts
sed -n '260,345p' packages/worker/src/webhooks/http.ts
echo
echo "## webhooks/http.workers.test.ts anonymous validation coverage"
sed -n '160,250p' packages/worker/src/webhooks/http.workers.test.ts
echo
echo "## route/action definitions for webhookIngress"
sed -n '90,110p' packages/worker/src/app/routes.ts
echo
echo "## router-specificity test"
sed -n '1,120p' packages/worker/src/app/router-specificity.node.test.ts
echo
echo "## worker package remix version"
sed -n '1,45p' packages/worker/package.jsonRepository: kentcdodds/kody
Length of output: 11635
🌐 Web query:
@remix-run/fetch-router route action middleware middleware option empty array route router middleware 0.20.1
💡 Result:
In @remix-run/fetch-router, you do not need to provide an empty array (middleware: []) for route actions. As of version 0.18.0 and later, the middleware option is optional when defining route actions using the object form { handler, middleware? } [1]. You can simply omit the middleware property entirely when it is not needed [1][2][3]. The library is designed such that action definitions (in router methods like router.get, router.post, or within controllers) accept an object where middleware is explicitly marked as optional [1][2]. This improvement was specifically made to clean up route definitions by removing the requirement to pass an empty array [4]. Regarding version 0.20.1, it is a patch release that includes updated dependencies (specifically route-pattern@0.23.0) but does not introduce changes to the middleware API model established in 0.20.0 and earlier versions [5][6]. For optimal type inference in newer versions (0.20.0+), it is recommended to use plain inline arrays for middleware options rather than custom-typed wrappers unless strictly necessary [5][2].
Citations:
- 1: https://github.com/remix-run/remix/releases/tag/fetch-router@0.18.0
- 2: https://www.npmjs.com/package/@remix-run/fetch-router
- 3: https://github.com/remix-run/remix/tree/main/packages/fetch-router
- 4: remix-run/remix@9a2977e
- 5: https://github.com/remix-run/remix/releases/tag/fetch-router%400.20.0
- 6: https://github.com/remix-run/remix/releases/tag/fetch-router@0.20.1
🌐 Web query:
site:github.com/remix-run/remix fetch-router middleware merge route middleware router middleware
💡 Result:
In fetch-router, middleware is categorized into two main types based on where it is applied, and it is governed by specific rules for execution and context sharing [1][2]. Router (Global) Middleware Router middleware is added at the router level during initialization using createRouter({ middleware }) [1][2]. This middleware runs on every request, regardless of whether a route matches, and executes before the router performs any route matching [1][2]. It is ideal for tasks such as logging, profiling, or modifying the request context to influence matching (e.g., method overriding or form data parsing) [1][2]. Route (Inline) Middleware Route middleware is added to individual routes, controllers, or actions via router.map, router.get, etc., using the { handler, middleware? } object structure [1][3][2]. It runs only when the specific route is matched, executing downstream from any router middleware [1][2]. This type of middleware is used for per-route concerns like authentication, authorization, and data validation [1][2]. Key Middleware Mechanics 1. Execution Chain: Middleware must explicitly continue the request chain by calling next or by returning a Response [4]. Returning undefined without calling next will result in an error [4]. 2. Context Threading: fetch-router supports sharing request-scoped context across middleware and handlers [1]. Middleware can provide values to the context, which are then available to subsequent middleware and the final route handler [4]. Types are automatically inferred from middleware arrays, allowing handlers to access context provided by the chain [4]. 3. Composition: Middleware arrays are passed directly to router options, controllers, or actions [1][5]. For complex scenarios where a middleware chain must be reusable or stored as a specific type, createMiddleware can be used to preserve the chain's tuple type, though plain inline arrays are preferred for most use cases [4]. 4. Nesting: Controller middleware only applies to the actions within that specific controller and is not inherited by registered nested route maps; protection logic should be explicitly applied to each controller that requires it [6]. Top results: [1][2][4]
Citations:
- 1: https://github.com/remix-run/remix/blob/main/packages/fetch-router/README.md
- 2: https://github.com/remix-run/remix/releases/tag/fetch-router@0.8.0
- 3: https://github.com/remix-run/remix/releases/tag/fetch-router@0.18.0
- 4: https://github.com/remix-run/remix/releases/tag/fetch-router%400.20.0
- 5: https://github.com/remix-run/remix/blob/main/packages/fetch-router/src/lib/router.ts
- 6: https://github.com/remix-run/remix/releases/tag/fetch-router@0.19.0
Add an anonymous POST coverage for webhook ingress.
The webhook ingress path is intercepted before the app router, and router middleware still runs on every fetch, so the remaining concern only applies if the app router fall-through is reachable. Add a test that POSTs to a valid webhook URL without auth and confirms the handler validates the path/secret before any account lease/persistence and does not write across users.
🤖 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 `@packages/worker/src/app/router.ts` at line 302, Add anonymous POST coverage
for the webhook ingress route through the app router, using a valid webhook URL
without authentication. Assert that path/secret validation occurs before account
lease or persistence operations and that the request cannot write data for
another user.
Upsert minted URL secrets to avoid concurrent-mint constraint failures, rate-limit before delivery-log writes (and skip logging 429s), validate verification headers as HTTP field names, and reuse the retention constant for delivery list limits.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 23748d3. Configure here.
| ) | ||
| if (!rateLimit.allowed) { | ||
| return rateLimitedResponse(rateLimit.retryAfterSeconds ?? 60) | ||
| } |
There was a problem hiding this comment.
Rate limit reveals minted webhooks
Medium Severity
Per-endpoint rate limiting runs only after a minted, enabled row is found. Guessing webhook names with invalid URL secrets on names without a mint never hits the limiter, while names with an active mint eventually return 429. That difference can reveal which webhook names have minted, enabled ingress for a known user and package.
Reviewed by Cursor Bugbot for commit 23748d3. Configure here.
| DO UPDATE SET | ||
| url_secret_hash = excluded.url_secret_hash, | ||
| rotated_at = excluded.rotated_at`, | ||
| ) |
There was a problem hiding this comment.
Remint leaves webhook disabled
Medium Severity
webhook_url_mint passes enabled: true into the upsert, but the ON CONFLICT branch only updates url_secret_hash and rotated_at. Reminting after webhook_disable rotates the URL secret while the row stays disabled, so the new credential still gets 404 at ingress until webhook_enable is called separately.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 23748d3. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/webhooks/repo.ts (1)
92-115: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftConcurrent mint responses can contain immediately invalid URLs.
ON CONFLICT ... DO UPDATEmakes the last writer replace the only stored hash. If two mints overlap,mintWebhookUrlForUserreturns both generated plaintext secrets, but only the last one remains valid; the other caller receives a successful yet unusable URL. Define a concurrency contract that coalesces/idempotently reuses concurrent mint requests, or support multiple active secret hashes during a rotation window.🤖 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 `@packages/worker/src/webhooks/repo.ts` around lines 92 - 115, Update mintWebhookUrlForUser and the webhook upsert flow around getWebhookEndpointByKey so overlapping mint requests cannot invalidate one caller’s returned URL; implement an atomic coalescing/idempotent reuse contract for concurrent mints, or retain multiple active secret hashes throughout the rotation window. Ensure every successful concurrent response remains valid against the stored webhook state.
🤖 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 `@packages/worker/src/webhooks/http.ts`:
- Around line 316-324: Move the endpoint-scoped checkRateLimit call and its
rateLimitedResponse branch to execute only after webhookUrlSecretMatches
succeeds, so invalid-secret requests cannot consume the endpoint delivery
budget. If invalid-secret deliveries remain logged, apply a separate
source-based limiter to that pre-auth path instead of reusing the endpoint
bucket.
- Around line 317-320: Update the rate-limit key passed by the webhook handler’s
checkRateLimit call to include userId alongside endpoint.id, preserving the
existing webhook namespace and configuration while ensuring the shared
rate-limit read/write path is tenant-scoped.
---
Outside diff comments:
In `@packages/worker/src/webhooks/repo.ts`:
- Around line 92-115: Update mintWebhookUrlForUser and the webhook upsert flow
around getWebhookEndpointByKey so overlapping mint requests cannot invalidate
one caller’s returned URL; implement an atomic coalescing/idempotent reuse
contract for concurrent mints, or retain multiple active secret hashes
throughout the rotation window. Ensure every successful concurrent response
remains valid against the stored webhook state.
🪄 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 Plus
Run ID: 9196cb9c-5c5d-4efe-8223-7414d320adb3
📒 Files selected for processing (5)
packages/worker/src/package-registry/manifest.node.test.tspackages/worker/src/package-registry/types.tspackages/worker/src/webhooks/http.tspackages/worker/src/webhooks/repo.tspackages/worker/src/webhooks/service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/worker/src/package-registry/manifest.node.test.ts
- packages/worker/src/package-registry/types.ts
- packages/worker/src/webhooks/service.ts
| // Bound every subsequent delivery-log write (including pre-auth rejects). | ||
| const rateLimit = await checkRateLimit( | ||
| env.APP_DB, | ||
| `webhook:endpoint:${endpoint.id}`, | ||
| webhookRateLimitConfig, | ||
| ) | ||
| if (!rateLimit.allowed) { | ||
| return rateLimitedResponse(rateLimit.retryAfterSeconds ?? 60) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not consume an endpoint’s delivery budget before URL-secret authentication.
Anyone who knows the public username/package/webhook route can send arbitrary secrets and exhaust this 60-request endpoint bucket, causing valid webhook calls to receive 429. Apply the endpoint budget after webhookUrlSecretMatches; if invalid-secret deliveries must remain logged, protect those with a separate source-based limiter.
🤖 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 `@packages/worker/src/webhooks/http.ts` around lines 316 - 324, Move the
endpoint-scoped checkRateLimit call and its rateLimitedResponse branch to
execute only after webhookUrlSecretMatches succeeds, so invalid-secret requests
cannot consume the endpoint delivery budget. If invalid-secret deliveries remain
logged, apply a separate source-based limiter to that pre-auth path instead of
reusing the endpoint bucket.
| const rateLimit = await checkRateLimit( | ||
| env.APP_DB, | ||
| `webhook:endpoint:${endpoint.id}`, | ||
| webhookRateLimitConfig, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Namespace the rate-limit key with userId.
This shared _rate_limits write is keyed only by endpoint.id; include the owning user explicitly rather than relying on endpoint-ID uniqueness for tenant isolation.
Proposed fix
- `webhook:endpoint:${endpoint.id}`,
+ `webhook:user:${endpoint.userId}:endpoint:${endpoint.id}`,As per coding guidelines, “every read/write path must be scoped by userId.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rateLimit = await checkRateLimit( | |
| env.APP_DB, | |
| `webhook:endpoint:${endpoint.id}`, | |
| webhookRateLimitConfig, | |
| const rateLimit = await checkRateLimit( | |
| env.APP_DB, | |
| `webhook:user:${endpoint.userId}:endpoint:${endpoint.id}`, | |
| webhookRateLimitConfig, |
🤖 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 `@packages/worker/src/webhooks/http.ts` around lines 317 - 320, Update the
rate-limit key passed by the webhook handler’s checkRateLimit call to include
userId alongside endpoint.id, preserving the existing webhook namespace and
configuration while ensuring the shared rate-limit read/write path is
tenant-scoped.
Source: Coding guidelines
3 participants
Summary
Inbound webhooks are declared on packages (
package.json#kody.webhooks) and activated per user by minting a URL secret — same shape askody.subscriptions, not standalone endpoint CRUD.name,export,responseMode(ack|sync), optional HMAC verification viasecretName(user secret store; never inline)POST /@:username/webhooks/:packageKodyId/:webhookName/:urlSecret404webhooksdomain): list, mint, rotate, enable, disable, delivery listinvokePackageExportwith{ webhook, request }; ack = 202 +waitUntil, sync = export JSON / 502System recap — adds Inbound webhooks (high risk)
Mode: recap · Base:
main@958206d6· Head:451939bfClassification: adds — new
webhooksprimitive (package-declared ingress withminted credential URLs).
Primitives touched
webhookssaved-packageskody.webhooksmanifest parse/validated1-app-dbwebhook_endpoints/webhook_deliveriescapability-registrywebhooksdomainapp-uisecretssecretNameat verificationmcp-serverSystem map
Inbound POSTs resolve a minted package webhook, verify optional HMAC via the
secrets store, then dispatch through package-invocation to the declared export;
MCP mint/list capabilities manage the credential URL.
Legend: green = composes (wiring only) · amber = extended by this PR · red =
new primitive · gray = context (unchanged, included only when an edge crosses
it).
Change flow
Before / after
kody.webhooks[]+ mintedPOST /@:username/webhooks/:packageKodyId/:webhookName/:urlSecretwebhook_list,webhook_url_mint,webhook_url_rotate,webhook_enable,webhook_disable,webhook_delivery_listInvariants
userId;ingress re-checks username ownership before dispatch.
secret-store references only).
Test plan
npm run validate(green locally)Notes
webhook_url_mint.Summary by CodeRabbit
New Features
Privacy & Security