feat: webhooks - #270
Conversation
Domain-event backbone (events:domain + sinks with inline/null fallback), event registry, Standard Webhooks signing with AES-GCM secret storage, matcher with owner cache, dispatcher with pending cap, Mongo claim-loop delivery executor with retry ladder and auto-disable, full API surface gated by the webhooks flag, worker consumer groups + embedded runtime, producers in UrlService and CustomDomainService.
…rface link.expired now fires from both discovery sites (max-clicks branch in the click handler, lazy time-expiry flip on resolve), gated once per link by the existing atomic updates. bot.detected and key.created are cut from the catalog: tracked bots already ride link.clicked as is_bot, and blocked bots produce no click by design. Adds per-delivery outcome logs, the webhooks Axiom dashboard, and .env.example entries. Comments rewritten to be self-contained.
meta_tags changes now carry only the four public fields, dropping the internal updated_ip/updated_at/image_meta bookkeeping, and datetimes in changes serialize as ISO 8601 like every other wire timestamp. Password redaction unchanged.
domain.verified fails the catalog's own admission test (verification is user-initiated, the owner is watching the dashboard) and domain.suspended belongs to first-party email when the reverify worker ships, not to subscriber-built webhook infrastructure. Catalog is six link events; adding a domain category back later is a registry entry.
… ops Events split by causer, not by field: actor edits ride link.updated and its changes map (status included), system-discovered facts get named events (link.expired today, link.blocked when the safety framework ships a producer). status_changed only ever fired for owner edits and double-fired alongside updated. Bulk ops now emit per item, keeping the promise that subscribers cannot tell bulk from a loop; the changes-map builder moved to webhooks.payloads so both producers share one shape.
Snapshots now carry geo_rules and meta_tags (document projection, no entitlement lookups), the changes map speaks the snapshot's public vocabulary (expire_after surfaces as expires_at), link.clicked gains long_url from the cached data, and the envelope carries the event id so consumers can correlate across endpoints and the delivery log.
Alongside the parsed browser/os/device fields: receiver-side debugging and bot forensics need the original string. Visitor-controlled input, so it is control-char-stripped and bounded to 512 chars before riding the wire, same posture as the UTM bounds.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds an opt-in webhook platform with endpoint CRUD, event subscriptions, encrypted signing secrets, durable delivery records, retries, signed HTTP delivery, worker and inline runtimes, API routes, domain-event fanout, indexes, metrics, and integration/unit coverage. ChangesWebhook platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
app.py (1)
198-214: 🩺 Stability & Availability | 🔵 TrivialConsider bounding the shutdown wait on the embedded executor task.
await webhook_executor_taskaftercancel()has no timeout. TodayDeliveryExecutor.run()only exits viaCancelledErrorand its await points (sleep, network I/O) are cancellation-friendly, so this should return promptly — but if that loop ever grows a non-cancellable blocking call, shutdown could stall past the deploy's termination grace period.🛡️ Optional hardening: bound the wait
if webhook_executor_task is not None: webhook_executor_task.cancel() - with suppress(asyncio.CancelledError): - await webhook_executor_task + with suppress(asyncio.CancelledError): + await asyncio.wait_for(webhook_executor_task, timeout=10)🤖 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 `@app.py` around lines 198 - 214, Bound the shutdown wait for webhook_executor_task in the application lifespan cleanup: after cancelling the task, await it with the existing asyncio timeout mechanism and preserve suppression of normal cancellation, while ensuring an overlong shutdown does not block deployment termination. Keep the existing embedded executor startup and cancellation flow unchanged.tests/integration/api_v1/test_webhooks.py (1)
55-140: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a cross-tenant ownership isolation test.
This suite thoroughly covers CRUD, signing, retries, and delivery logs for a single user, but no test asserts that a second user cannot GET/PATCH/DELETE/test-send/list-deliveries on another user's endpoint (the fakes already gate on
user_id, e.g.find_owned,delete_endpoint). Given secrets and delivery logs are exposed here, a short test exercising a second_make_user()against another user'screated['id']and asserting 404s would close this gap cheaply.Also applies to: 304-483
🤖 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/integration/api_v1/test_webhooks.py` around lines 55 - 140, Add an integration test using two users created via _make_user(), with the first user creating an endpoint and the second attempting GET, PATCH, DELETE, test-send, and list-deliveries operations against the first user’s endpoint ID. Assert each cross-tenant request returns 404, preserving the existing endpoint setup and authentication helpers.axiom/dashboards/spoo-webhooks.json (1)
27-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
is_testfilter applied inconsistently across charts.Only
stat-deliveredexcludes test pings (is_test != true);stat-delivery-success-rate,stat-p95-delivery,ts-dispatch-by-type,ts-delivery-outcomes,ts-attempt-failures-by-status,table-slowest-endpoints, andtable-failing-endpointsdo not. Test webhook sends (dashboard "send test" pings) will skew success-rate, latency percentile, and failing-endpoint metrics used for on-call triage.Example fix for one chart
{ "id": "stat-delivery-success-rate", "name": "Delivery success rate %", "type": "Statistic", "query": { - "apl": "['spoo-prod']\n| where event in (\"webhook_delivered\", \"webhook_delivery_attempt_failed\")\n| summarize total = count(), ok = countif(event == \"webhook_delivered\")\n| project rate = round(100.0 * ok / total, 2)" + "apl": "['spoo-prod']\n| where event in (\"webhook_delivered\", \"webhook_delivery_attempt_failed\") and is_test != true\n| summarize total = count(), ok = countif(event == \"webhook_delivered\")\n| project rate = round(100.0 * ok / total, 2)" } },🤖 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 `@axiom/dashboards/spoo-webhooks.json` around lines 27 - 106, Apply the same is_test exclusion used by stat-delivered to the queries for stat-delivery-success-rate, stat-p95-delivery, ts-dispatch-by-type, ts-delivery-outcomes, ts-attempt-failures-by-status, table-slowest-endpoints, and table-failing-endpoints. Add the filter so test pings are excluded while preserving each chart’s existing event conditions and aggregations.workers/click_worker.py (1)
162-208: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant
GeoIPServiceinstantiation when both webhooks and stats groups run.
webhook_geoipis constructed at line 187 forWebhookClickConsumer, and a separategeoipinstance is constructed at line 210 for thestatsgroup — both open the same.mmdbfiles independently.app.py's lifespan comment notes GeoIP is meant to be "a singleton so the .mmdb readers are opened once and reused". Consider building oneGeoIPServiceup front in_build_runtimeand sharing it between the webhooks and stats consumers.Proposed consolidation
runtime = _WorkerRuntime( mongo_client=mongo_client, cache_redis=cache_redis, counter_redis=None, ) + shared_geoip: GeoIPService | None = None + worker_domain_sink = None if run_webhooks: ... - webhook_geoip = GeoIPService(settings.geoip_country_db, settings.geoip_city_db) + shared_geoip = shared_geoip or GeoIPService( + settings.geoip_country_db, settings.geoip_city_db + ) runtime.consumers["webhooks"] = WebhookClickConsumer( - dispatcher, webhook_geoip, settings.system_default_domain + dispatcher, shared_geoip, settings.system_default_domain ) ... if "stats" in groups: - geoip = GeoIPService(settings.geoip_country_db, settings.geoip_city_db) + geoip = shared_geoip or GeoIPService( + settings.geoip_country_db, settings.geoip_city_db + )Also applies to: 209-222
🤖 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 `@workers/click_worker.py` around lines 162 - 208, Consolidate GeoIP initialization in _build_runtime by constructing a single GeoIPService before the webhooks and stats consumer setup. Reuse that shared instance for WebhookClickConsumer and the stats consumer, removing the separate webhook_geoip and geoip constructions while preserving their existing configuration arguments.tests/unit/repositories/test_indexes.py (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWebhook-endpoint indexes (
ix_matcher,user_id+status) aren't asserted.
webhook_endpoints_colis mocked but no assertion checks itscreate_indexcalls, unlike the events/deliveries collections.ix_matcherbacks the hot matcher-lookup path (SubscriptionMatcher), so it's worth pinning here too.Suggested additional assertions
webhook_deliveries_col.create_index.assert_any_await( [("created_at", 1)], expireAfterSeconds=2_592_000, name="ttl_created_at" ) + webhook_endpoints_col.create_index.assert_any_await( + [("user_id", 1), ("events", 1), ("status", 1)], name="ix_matcher" + )Also applies to: 46-48, 67-80
🤖 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/unit/repositories/test_indexes.py` around lines 30 - 32, Extend the index-creation assertions for webhook_endpoints_col to verify its create_index calls, matching the existing assertions for webhook_events_col and webhook_deliveries_col. Pin both the ix_matcher index and the compound user_id-plus-status index, including their configured fields and relevant options.services/webhooks/signing.py (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the secret-storage documentation.
Lines 6-7 state that secrets are stored hash-only, but
WebhookEndpointDoc.signing_secret_encstores ciphertext and the executor decrypts it to sign deliveries. Keep this contract description accurate.🤖 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 `@services/webhooks/signing.py` around lines 1 - 10, Update the module docstring’s secret-storage description to state that the signing secret is stored encrypted/ciphertext and decrypted by the delivery executor when signing webhooks. Keep the existing format, one-time display, and retry timestamp behavior unchanged.services/bulk_url_service.py (1)
621-655: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-item event emission on bulk paths adds request latency for large batches.
_emit_updated/_emit_deletedawait eachself._events.emit(...)one at a time. Since sinks never raise (errors are swallowed internally), these can safely run concurrently to bound per-request latency on large bulk operations.♻️ Suggested refactor
async def _emit_updated(self, docs: list[UrlV2Doc], set_ops: dict) -> None: """One link.updated per changed item — post-state snapshot plus the same changes map the single-item producer builds.""" - for doc in docs: - owner = link_owner_id(doc) - if owner is None: - continue - merged = doc.model_dump(by_alias=True) - merged.update(set_ops) - merged["_id"] = doc.id - post_doc = UrlV2Doc.from_mongo(merged) - await self._events.emit( - DomainEvent( - type="link.updated", - owner_id=owner, - data={ - "link": link_snapshot(post_doc), - "changes": event_changes(doc, set_ops), - }, - ) - ) + events = [] + for doc in docs: + owner = link_owner_id(doc) + if owner is None: + continue + merged = doc.model_dump(by_alias=True) + merged.update(set_ops) + merged["_id"] = doc.id + post_doc = UrlV2Doc.from_mongo(merged) + events.append( + DomainEvent( + type="link.updated", + owner_id=owner, + data={ + "link": link_snapshot(post_doc), + "changes": event_changes(doc, set_ops), + }, + ) + ) + await asyncio.gather(*(self._events.emit(e) for e in events))🤖 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 `@services/bulk_url_service.py` around lines 621 - 655, The _emit_updated and _emit_deleted methods currently await each event emission sequentially, increasing latency for large batches. Build the per-item DomainEvent emissions in each method and await them concurrently, using the existing self._events.emit calls while preserving owner filtering, event payloads, and empty-batch behavior.
🤖 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 `@config.py`:
- Around line 646-647: Update the webhook initialization path around
AppSettings.secret_key and WebhookSettings so startup fails when webhooks are
enabled and the resolved master secret is empty. Validate SECRET_KEY or its
configured fallback before creating WebhookSettings, while preserving normal
initialization when a non-empty secret is available.
In `@dependencies/wiring.py`:
- Around line 240-251: Update the Webhooks startup wiring around
DeliveryExecutor to detect an empty settings.secret_key when Webhooks are
enabled and emit a warning consistent with the existing session/JWT startup
checks. Reuse the established warning/logging mechanism and ensure the check
occurs during application startup before WebhookService or DeliveryExecutor uses
the key.
- Around line 218-281: In the webhook startup wiring around
queue_redis_for_webhooks and wh_settings.enabled, add a log.warning when queue
Redis is configured, webhooks are enabled, and settings.click_events.sink is
"inline", clearly indicating that click webhooks may be silent. Keep the
existing sink selection unchanged unless wiring the click-stream webhook path is
already supported by the surrounding code.
In `@infrastructure/safe_fetch.py`:
- Around line 258-264: Update the URL parsing and DNS resolution try block in
post_public() to also catch FetchTransientError from _resolve_public_ip() and
return it as a PostResult delivery failure, preserving the exception message and
existing result shape so transient failures are recorded and rescheduled.
In `@repositories/webhook_delivery_repository.py`:
- Around line 39-42: Replace the separate count_pending() admission flow with an
atomic per-endpoint pending reservation that increments only when the configured
capacity remains available, and make dispatch proceed only when that conditional
update succeeds. Release the reservation whenever a delivery transitions out of
pending, including successful, failed, and canceled paths, while keeping the
reservation consistent with existing delivery state.
- Around line 125-143: Update mark_failed to increment attempt_count in the same
atomic update that appends the DeliveryAttempt, keeping the stored count
consistent with the newly added terminal failure attempt.
In `@routes/api_v1/webhooks.py`:
- Line 122: Update the webhook route handlers in routes/api_v1/webhooks.py so
every non-catalog operation—not just creation—calls
flag_svc.require(WEBHOOKS_FLAG, user) before proceeding, including list,
modification, test, and manual retry endpoints. Preserve catalog operations’
existing behavior and ensure gated operations cannot initiate outbound
deliveries without entitlement.
- Around line 57-60: Update the ReadUser dependency to use
require_scopes_verified with WEBHOOKS_READ_SCOPES, matching ManageUser’s
verified-email enforcement while preserving the existing read scope requirements
for webhook list, detail, and delivery-log endpoints.
In `@schemas/dto/requests/webhook.py`:
- Around line 54-57: Update the PATCH request model’s description field to
enforce the same 256-character maximum as the create model, using the existing
Field validation pattern in the surrounding webhook request fields. Keep the
field optional and preserve its current default behavior.
In `@services/events/sinks.py`:
- Around line 71-87: Remove the unsupported ref_policy="ACKED" argument from the
Redis xadd call in emit, preserving the existing stream write and fallback
behavior. Do not attempt to retain ACKED trimming until the redis-py dependency
supports that keyword.
In `@services/webhooks/consumers.py`:
- Around line 42-51: The stream consumer methods WebhookClickConsumer.consume
and WebhookDomainConsumer.consume allow dispatcher failures to escape uncaught.
Wrap each await self._dispatcher.dispatch(event) call in try/except matching
WebhookFanoutClickSink.emit’s containment and logging behavior, so dispatch
errors are logged and handled deliberately without propagating from the
subscriber.
In `@services/webhooks/executor.py`:
- Around line 105-109: Update WebhookExecutor.attempt so deliveries for paused
or otherwise temporarily non-active endpoints remain pending and are retried
after the endpoint resumes, rather than being marked failed as
endpoint_inactive; preserve failure handling for endpoints that no longer exist
if required by the existing flow. Adjust the corresponding unit test in
test_executor.py to assert the delivery remains pending under a paused endpoint.
---
Nitpick comments:
In `@app.py`:
- Around line 198-214: Bound the shutdown wait for webhook_executor_task in the
application lifespan cleanup: after cancelling the task, await it with the
existing asyncio timeout mechanism and preserve suppression of normal
cancellation, while ensuring an overlong shutdown does not block deployment
termination. Keep the existing embedded executor startup and cancellation flow
unchanged.
In `@axiom/dashboards/spoo-webhooks.json`:
- Around line 27-106: Apply the same is_test exclusion used by stat-delivered to
the queries for stat-delivery-success-rate, stat-p95-delivery,
ts-dispatch-by-type, ts-delivery-outcomes, ts-attempt-failures-by-status,
table-slowest-endpoints, and table-failing-endpoints. Add the filter so test
pings are excluded while preserving each chart’s existing event conditions and
aggregations.
In `@services/bulk_url_service.py`:
- Around line 621-655: The _emit_updated and _emit_deleted methods currently
await each event emission sequentially, increasing latency for large batches.
Build the per-item DomainEvent emissions in each method and await them
concurrently, using the existing self._events.emit calls while preserving owner
filtering, event payloads, and empty-batch behavior.
In `@services/webhooks/signing.py`:
- Around line 1-10: Update the module docstring’s secret-storage description to
state that the signing secret is stored encrypted/ciphertext and decrypted by
the delivery executor when signing webhooks. Keep the existing format, one-time
display, and retry timestamp behavior unchanged.
In `@tests/integration/api_v1/test_webhooks.py`:
- Around line 55-140: Add an integration test using two users created via
_make_user(), with the first user creating an endpoint and the second attempting
GET, PATCH, DELETE, test-send, and list-deliveries operations against the first
user’s endpoint ID. Assert each cross-tenant request returns 404, preserving the
existing endpoint setup and authentication helpers.
In `@tests/unit/repositories/test_indexes.py`:
- Around line 30-32: Extend the index-creation assertions for
webhook_endpoints_col to verify its create_index calls, matching the existing
assertions for webhook_events_col and webhook_deliveries_col. Pin both the
ix_matcher index and the compound user_id-plus-status index, including their
configured fields and relevant options.
In `@workers/click_worker.py`:
- Around line 162-208: Consolidate GeoIP initialization in _build_runtime by
constructing a single GeoIPService before the webhooks and stats consumer setup.
Reuse that shared instance for WebhookClickConsumer and the stats consumer,
removing the separate webhook_geoip and geoip constructions while preserving
their existing configuration arguments.
🪄 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: 241ebd59-f517-429f-8489-7f62fa5b3571
📒 Files selected for processing (55)
.env.exampleapp.pyaxiom/dashboards/spoo-webhooks.jsonconfig.pydependencies/__init__.pydependencies/auth.pydependencies/services.pydependencies/wiring.pyinfrastructure/crypto.pyinfrastructure/safe_fetch.pymiddleware/rate_limiter.pyrepositories/indexes.pyrepositories/webhook_delivery_repository.pyrepositories/webhook_endpoint_repository.pyrepositories/webhook_event_repository.pyroutes/api_v1/__init__.pyroutes/api_v1/webhooks.pyschemas/dto/requests/api_key.pyschemas/dto/requests/webhook.pyschemas/dto/responses/webhook.pyschemas/enums/webhook.pyschemas/models/webhook.pyservices/bulk_url_service.pyservices/click/handlers.pyservices/events/__init__.pyservices/events/contract.pyservices/events/protocol.pyservices/events/sinks.pyservices/feature_flag_service.pyservices/url_service.pyservices/webhooks/__init__.pyservices/webhooks/consumers.pyservices/webhooks/dispatcher.pyservices/webhooks/executor.pyservices/webhooks/matcher.pyservices/webhooks/payloads.pyservices/webhooks/registry.pyservices/webhooks/renderers/__init__.pyservices/webhooks/renderers/protocol.pyservices/webhooks/renderers/raw.pyservices/webhooks/service.pyservices/webhooks/signing.pyshared/scopes.pytests/integration/api_v1/test_webhooks.pytests/smoke/test_click_sink_wiring.pytests/smoke/test_custom_domain_cf_wiring.pytests/unit/repositories/test_indexes.pytests/unit/services/test_bulk_url_service.pytests/unit/services/webhooks/__init__.pytests/unit/services/webhooks/test_executor.pytests/unit/services/webhooks/test_matcher_dispatcher.pytests/unit/services/webhooks/test_payloads.pytests/unit/services/webhooks/test_registry.pytests/unit/services/webhooks/test_signing.pyworkers/click_worker.py
Zingzy
left a comment
Zingzy
left a comment
There was a problem hiding this comment.
Verdict: mergeable, and merging is safe — but nothing here should be enabled for a real user until the tier-1 list below is closed. This is a genuinely strong piece of architecture: WEBHOOKS_ENABLED defaults false → NullDomainEventSink, producers stay unconditional, and I verified the production redirect path (stream rung) is byte-for-byte unchanged — webhook work lives in the worker, never in the GET response. So the risk gate is flag-enable time, not merge. I ran the full webhooks suite (65 pass) plus five adversarial deep-dives (SSRF, crypto, executor concurrency, hot-path cost, authz); the findings are what to finish before you seed the first webhooks flag, framed that way throughout.
Architecture read — the load-bearing decisions are right
- The
services/events/backbone as a separate package with webhooks as one consumer is the correct seam: alerts/audit slot in as sibling consumer groups without touching producers. Not speculative — it's already carrying two feeds (clicks + domain) in this PR. - Feature-off is genuinely free, and I checked rather than trusted: NullSink
emitis a no-op, no worker groups mount, the redirect path's click sink is only wrapped on the inline (no-queue-Redis) rung. In prod with queue Redis, GET /{code} is untouched. - Mongo-only executor, embedded OR worker, atomic lease-claim (
find_one_and_updateonix_claim) — elegant, needs no new infra, self-heals on restart, and N concurrent executors are safe. The claim/backoff/render-once/crash-window semantics are all correct (verified line by line). - Signing is spec-correct — the published Standard Webhooks reference vector is pinned and passes; secret storage is authenticated AES-GCM with a random per-encryption nonce; the secret is returned exactly once and never logged. Confidentiality and integrity hold.
- Registry-as-code driving validation + catalog + samples + test sends + wildcard expansion (can't drift), with disciplined event governance (causer-not-field;
status_changed/bot.detectedcut with reasons). Payload sanitization holds across every event type — nocreation_ip, noowner_id, no password hash; the earlierupdated_ipleak fix survives. - IDOR: clean. Every reachable by-id path enforces ownership in the Mongo query. Test send / delivery log / manual retry is Stripe-grade DX.
Tier 1 — close before enabling the flag (correctness/security that will bite the first real subscriber or the operator)
1. SECRET_KEY rotation turns every endpoint into an immortal poison delivery (live). _headers calls decrypt_secret unguarded (executor.py:223). Rotating SECRET_KEY re-keys _derive_aes_key, so GCM auth fails with InvalidTag on every stored secret. That throws before any attempt is recorded → the loop's generic except logs webhook_executor_tick_failed and sleeps → the 60s lease expires → the same row is re-claimed → throws again, forever. The endpoint never disables and never exhausts. SECRET_KEY rotation is a routine security op (it's how JWTs get invalidated in this very ecosystem), and it silently converts webhooks into an infinite retry storm with no operator signal beyond a log line. Wrap decrypt: on failure, mark the delivery terminally failed and disable the endpoint with a distinct reason, so rotation degrades loudly and recoverably.
2. dropped_since_last is dead code — the flagship event loses data silently. make_delivery_row hardcodes dropped_since_last: 0 (dispatcher.py:41); record_success drains the counter and returns it, but the caller discards the return (executor.py:138); nothing ever reads endpoint.dropped_count when building a row. So the pending-cap drop signal increments a counter that's zeroed on the next success and never reaches a consumer. For link.clicked feeding an analytics pipeline — the whole "turn spoo into infrastructure" use case — a subscriber who falls behind the 1000-cap loses clicks with zero indication, and the mechanism designed to tell them (D13) is unwired. Wire it (carry the endpoint's current dropped_count onto the next row and reset on that row's success) or cut it and log the drop loudly. The dropped_since_last=42 test builds a state the pipeline can never produce, so it green-lights the dead path.
3. Test-send and manual-retry mutate real endpoint health and enroll in the background ladder. is_test is used only for logging (executor.py:147); nothing guards the health counters or the reschedule. Consequences: a transient-failed test send calls record_attempt_and_reschedule, setting next_attempt_at → the row becomes claimable and silently becomes a 10-hour background delivery (contradicting the "only executor this row meets" contract); exhausted test sends call record_exhausted, so a user clicking "test" on a broken endpoint ~10 times auto-disables their own endpoint; a passing test calls record_success, resetting a real failure streak and masking a failing endpoint. Test/retry should be single-shot and must not touch consecutive_failures/total_* or the retry schedule.
Tier 2 — hardening before GA (abuse/reputation/trust, given spoo's phishing-flag history)
4. Endpoints auto-disable silently. The on_disabled hook is None in both wiring sites, so an endpoint dying after 10 failures notifies the user only via a log event. For infrastructure positioning, a user's integration going dark with no email is a trust-breaker — wire the notification before GA.
5. Abuse/reputation cluster — decide the kill-switch. POST /{id}/test runs the real pipeline synchronously and returns status_code + 256-byte response_body + latency for any user-supplied public HTTPS URL. That's an authenticated "fetch the first 256 bytes + status + timing of any HTTPS host, from spoo's egress IP" oracle/amplifier (~600 probes/hr/credential). Internal SSRF is blocked (the guard is tight — see below), so this is external-only, but it's still spoo's IP hitting third parties on demand. Compounding it: per-click fanout POSTs to user URLs make spoo a potential reflector, there's no per-user delivery-rate cap (only a 5-endpoint count cap), and flipping the webhooks flag off does not stop /test or existing deliveries — only CREATE is gated. So the flag is an onboarding gate, not an abuse lever. Decide the actual abuse response (per-endpoint disable is the only current one) and consider a global delivery-rate cap.
6. The delivery-time SSRF guard has zero direct tests. The guard itself is solid — I tried hard: https-only + resolve-then-pin-to-IP + no-redirects blocks loopback/RFC1918/link-local/CGNAT/metadata-IP/userinfo/DNS-rebind/redirect-to-Redis. But post_public has no direct test asserting it rejects http, rejects a private IP, or refuses redirects — test_executor and test_webhooks patch it out entirely. A future refactor silently regresses SSRF. Add the tests. One real gap in the guard: 64:ff9b::/96 (NAT64) returns is_global=True, so https://[64:ff9b::7f00:1]/ passes _is_public (safe_fetch.py:69) and on a NAT64/DNS64 host routes to an internal HTTPS service — reject the prefix explicitly.
7. Bulk ops add per-item awaited work even when the feature is off. Bulk delete/update emit one event per item in an awaited loop, each doing a UrlV2Doc.from_mongo re-parse (bulk_url_service.py:634) with no enabled guard, plus a sequential XADD per item in stream mode. A 1000-link bulk op adds ~1000 re-parses (feature off) or ~1000 serial XADDs (on) to that request. Batch the writes and guard the re-parse behind the sink being non-null.
Nits
- Signing docstring lies about the security model.
signing.py:6says the secret is "stored hash-only (SHA-256)" — it's AES-GCM encrypted (it has to be; the server signs with it).hash_signing_secretis dead in production (test-only). Fix the docstring and drop the function — a maintainer trusting it would assume secrets are irreversible at rest. - Rotation scaffolding is inert.
previous_secret_enc/previous_secret_expires_atand the dual-signing branch exist but no route ever writes them, so rotation grace can't happen. Fine as a Phase-2 seam — but note that when you do wire rotation, the grace-window comparison atexecutor.py:229compares a naive Mongo datetime against an awarenow()and willTypeError(the loop-poisoning kind); useas_aware_utc()(the repo already has it). Cheap to fix now while it's dead. - Pending-cap and quota are both racy (count-then-write): concurrent dispatch can breach the 1000 cap, concurrent creates can exceed 5 endpoints. The quota race mirrors the existing api-key pattern, so it's a consistent stance, not a regression — noting for completeness.
update_fields/list_by_endpointenforce ownership via a preceding fetch, not in their own filter — safe today, but a latent regression trap; folduser_idinto the query likedelete_endpointdoes.
Product sense
The catalog scope is right: five events (created/updated/deleted/clicked/expired), fat snapshots so consumers don't need a follow-up call, additive-only contract, raw-flavor first. link.clicked is correctly the flagship — it's what makes real-time analytics/Zapier pipelines possible — which is exactly why Tier-1 #2 (silent drop, dead drop-signal) matters most: the highest-value event is the one that loses data quietly under load. Deferring Discord/Slack flavors to Phase 2 is defensible, though "point a webhook at your Discord" is the single most-requested easy win for this indie audience and the scaffolding's already there. The debuggability triad (test send + full delivery log with rendered body + manual retry) is the right DX bet and genuinely best-in-class — it just needs Tier-1 #3 fixed so the test button can't disable the thing it's testing. Net: the product thesis is sound and the surface is well-chosen; the gap is reliability-under-load and silent-failure trust, which is precisely what an "infrastructure" positioning is judged on.
Merge-safety summary
Merge is safe: dark by default, hot path verified untouched, CI green across the matrix, feature-off cost negligible. Nothing above blocks the merge. Tier 1 blocks enabling the flag for a real user (data loss + operator-facing crash loop + self-disabling test button); Tier 2 blocks GA (silent disable, abuse levers, SSRF test coverage). Sequence the tail as: land the events backbone + management API now (dark) → close Tier 1 → seed your own flag and dogfood link.clicked at volume → wire disable-email + abuse cap → open the flag.
Secret-rotation failures now terminate the row and disable the endpoint with a distinct reason instead of livelocking the claim loop. Test sends and manual retries are single-shot: no ladder, no health mutation. dropped_since_last is wired end to end (dispatch carries the endpoint counter, success drains it). Paused endpoints defer deliveries instead of failing them. NAT64 literals rejected, transient DNS failures become recorded outcomes, and post_public gains direct SSRF tests. Startup refuses WEBHOOKS_ENABLED without SECRET_KEY, click fanout wraps whenever clicks track inline, reads require verified email, and test/retry carry the feature-flag gate.
|
Review batch addressed in ed562e6. Disposition per finding: Fixed
Not changed, with reasons
|
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)
services/webhooks/signing.py (1)
54-64: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce timestamp freshness in
verify().
verify()only validates the signature, so it accepts any valid signature for an arbitrarily oldwebhook-timestamp, opening replay and old-delivery failures. Move to Standard Webhooks’ timestamp-tolerance check here or require every documented consumer example/docs caller to reject timestamps outside a small window before processing the event.🤖 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 `@services/webhooks/signing.py` around lines 54 - 64, Update verify() to enforce Standard Webhooks timestamp freshness before accepting any signature, rejecting timestamps outside the configured small tolerance window while preserving constant-time matching and support for multiple rotation signatures. Ensure documented consumer examples and callers use this verification behavior rather than bypassing the freshness check.
🤖 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.
Outside diff comments:
In `@services/webhooks/signing.py`:
- Around line 54-64: Update verify() to enforce Standard Webhooks timestamp
freshness before accepting any signature, rejecting timestamps outside the
configured small tolerance window while preserving constant-time matching and
support for multiple rotation signatures. Ensure documented consumer examples
and callers use this verification behavior rather than bypassing the freshness
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cda45660-d79e-481e-83da-355eb867c8f8
📒 Files selected for processing (18)
app.pyconfig.pydependencies/wiring.pyinfrastructure/safe_fetch.pyrepositories/indexes.pyrepositories/webhook_delivery_repository.pyroutes/api_v1/webhooks.pyschemas/dto/requests/webhook.pyschemas/enums/webhook.pyservices/webhooks/dispatcher.pyservices/webhooks/executor.pyservices/webhooks/signing.pytests/integration/api_v1/test_webhooks.pytests/unit/infrastructure/test_safe_fetch_post.pytests/unit/services/webhooks/test_executor.pytests/unit/services/webhooks/test_matcher_dispatcher.pytests/unit/services/webhooks/test_signing.pytests/unit/test_config.py
💤 Files with no reviewable changes (1)
- tests/unit/services/webhooks/test_signing.py
🚧 Files skipped from review as they are similar to previous changes (13)
- services/webhooks/dispatcher.py
- schemas/enums/webhook.py
- config.py
- app.py
- schemas/dto/requests/webhook.py
- infrastructure/safe_fetch.py
- tests/unit/services/webhooks/test_matcher_dispatcher.py
- repositories/webhook_delivery_repository.py
- dependencies/wiring.py
- routes/api_v1/webhooks.py
- repositories/indexes.py
- services/webhooks/executor.py
- tests/integration/api_v1/test_webhooks.py
Zingzy
left a comment
Zingzy
left a comment
There was a problem hiding this comment.
Re-reviewed ed562e6 against the head I first reviewed (dcb21b3) — I read the delta and re-verified each finding at the choke point rather than trusting the changelog, and ran the affected suite (123 pass, incl. the new direct post_public SSRF tests and config-guard tests). All three Tier-1 items are genuinely closed, and several Tier-2/nit items came with them. My original findings stand as having been real; nothing I flagged turned out to be a false alarm, and every fix lands where the defect was.
Tier 1 — verified fixed:
- Decrypt crash-loop:
_headersis now wrapped (executor.py); a decrypt failure marks the deliverysecret_unreadableand disables the endpoint with the newSECRET_UNREADABLEreason (single-shot rows skip the disable). The infinite reclaim livelock is dead — aSECRET_KEYrotation now degrades loudly and recoverably. Confirmed the exception can no longer escape before an attempt is recorded. dropped_since_last:make_delivery_rownow snapshotsendpoint.dropped_countonto the row, the renderer surfaces it,record_successdrains it. The drop signal reaches the subscriber. The at-least-once repeat-until-landed behavior you noted in the comment is the right tradeoff (consumers dedup onwebhook-id, the number is informational).- Test/retry single-shot:
single_shot = is_test or status != PENDINGcleanly excludes them fromrecord_success/record_exhausted/the retry ladder and 410-disable. A failing test can no longer auto-disable a real endpoint and a passing one can no longer mask a failure streak. Bonus: PAUSED nowdefers (5-min recheck) instead of terminally failing the queued delivery — that also closes the pause-destroys-deliveries semantic.
Also closed: NAT64 64:ff9b::/96 rejected outright in _is_public (rejecting the whole translation prefix is cleaner than unwrap-and-recheck — agreed); post_public now catches FetchTransientError/InvalidURL → returns data not an exception, so a slow DNS name can't leave a row leased; config hard-fails WEBHOOKS_ENABLED without SECRET_KEY; the grace-window comparison goes through as_aware_utc so Phase-2 rotation can't poison; signing docstring corrected to AES-GCM and dead hash_signing_secret removed; mark_failed now $incs attempt_count; reads require verified email. And flag-gating test+retry is the right call — it means flipping the flag off now stops the on-demand outbound amplifier, which was the sharp half of the abuse concern; keeping passive management ungated is a coherent doctrine.
The two you skipped with reasons — I agree with both: the pending-cap count-then-insert is a protective backstop, not an SLA, and a one-batch overshoot doesn't justify a reservation-counter write on every dispatch. And letting a dispatch exception escape the stream consumer is correct — it leaves the message unacked for the claimer/DLQ at-least-once path; the asymmetry with the inline sink (which swallows, because there's no message to leave pending) is right, not an oversight.
Merge: clear. Nothing outstanding blocks it, and it's still dark by default.
Before GA (the flag-enable tail, all explicitly deferred and fine to carry): wire the on-disabled email (→ the notifications framework) so endpoints don't die silently; add a per-user delivery-rate cap (the endpoint-count cap doesn't bound the reflector surface once link.clicked subscribers exist); and decide whether /test should keep echoing the 256-byte response body (it's a content-peek of any public HTTPS URL from spoo's egress, now gated behind the flag + 10/min, so bounded — but worth a conscious call at GA). None of these gate the merge or the first dogfood.
Good, fast turnaround — the fixes are precise and the reasoning on the two declines is sound.
build_link_clicked called the async get_country_code without awaiting it, leaking a coroutine into the payload for any non-geo click whose country had to be resolved from the mmdb. Made the builder async and awaited the call; both callers (the stream consumer and the inline fanout sink) already run in async context. A load test through the real worker surfaced it — unit tests dispatched pre-built events and missed the geoip branch.
Endpoints can now point straight at a Discord or Slack incoming webhook. A shared copy layer picks wording and density per event (compact for clicks, field grids for lifecycle, a colored diff block for link.updated on Discord); unknown event types degrade to a JSON code block so future events never terminal-fail flavored endpoints.
Rate limiting is receiver flow control, not endpoint failure: honor Retry-After (capped at 15m, 60s fallback) via the existing defer path, leaving the retry ladder and the auto-disable streak untouched. Test sends still report a terminal outcome synchronously.
Clicks carry an inline field grid with guaranteed substance: Device always, From reading direct when there is no referrer, plus Location, UTM, Bot and Clicks when present. Lifecycle cards state never and unlimited as real answers, and deleted links report lifetime clicks and age. Long values like destinations stay full width.
Secrets are stored encrypted, never hashed, since the executor reads them back to sign. Owners can now fetch the full secret from the endpoint instead of copying it exactly once at creation. Unreadable secrets (rotated master key) report cleanly instead of raising.
An endpoint with two deliveries mid-retry showed delivered 0 of 0, contradicting its own delivery log. total_deliveries now increments when the row is enqueued, so the ratio reads 0 of 2 while pending and 2 of 2 once landed. Test sends stay out of the counters.
Every click dimension gets its own field in a two-column grid closed by spacer fields, country codes carry flag emoji, and the running click count always renders. Created cards state the full link configuration including password, bot blocking, meta tags and geo rules. Messages post under the spoo.me name and avatar, the wire timestamp now carries its UTC offset so Discord localizes it, and the middle-dot separator is gone from embeds.
The discord flavor now renders a components-only message: an accent container with a heading, divider, grouped substance lines and a small-text footer whose timestamp Discord localizes per viewer. Renderers can declare query params the executor appends at delivery, which carries the with_components switch. Mentions are hard-disabled since text displays can ping and payload values are user-controlled.
…veal to sessions Payload values reaching components-v2 text displays are now backslash-escaped, so a crafted Referer header cannot plant a masked link or any live markup in the subscriber's channel; masked-link targets also encode parentheses. The secret reveal endpoint requires an interactive session, matching how key creation refuses delegated credentials.
feat(webhooks): discord and slack delivery flavors
github-project-automation
Bot
moved this from 📋 Planning Stage
to ✔️ Done
in spoo.me Development Roadmap
What
Real-time event deliveries to subscriber URLs, following the Standard Webhooks specification. When something happens to your links, spoo.me tells your other tools: your own server, a Zapier hook, anything that accepts an HTTPS POST.
Fully opt-in and dark by default:
WEBHOOKS_ENABLED=falsewires a null sink and mounts nothing, and access is additionally gated per user by thewebhooksfeature flag (default deny, 403 when not granted).Events
Five event types, one rule: events split by who caused them. Actor edits ride
link.updatedand itschangesmap (status included); system-discovered facts get named events.link.createdlink.updatedchangesmaps each field to old/newlink.deletedlink.clickedis_botin payload)link.expiredPayloads carry the full public link snapshot plus event context.
link.clickedis flat and dimensional: country/city, browser/os/device, bounded raw user agent, referrer, UTM,long_url, running click total. Bulk operations emit per item, indistinguishable from a loop. Nothing password-shaped, IP-shaped, or internal ever rides the wire: password fields become presence booleans, meta tags reduce to their four public fields, and payload validation is pinned to the registry models by test.GET /api/v1/webhooks/event-typesis a public catalog with exact sample payloads; the same fixtures power test sends, so docs and behavior cannot drift.Delivery
webhook-id/webhook-timestamp/webhook-signatureheaders, HMAC-SHA256,whsec_secrets shown once and stored AES-GCM encrypted (the server signs at delivery time, so hash-only storage is not an option). Verified against the published reference test vector.webhook-idis stable across retries and bodies are frozen after first render, so consumer dedup works on either.dropped_since_lastas a reconcile signal.Architecture
Two Redis Streams feed a dispatcher: the existing click stream gains a
webhooksconsumer group (reader plus claimer pair, DLQ guard, same machinery as the stats and hotness groups), and a new low-volumeevents:domainstream carries link lifecycle facts fromUrlServiceand the bulk operations. The dispatcher matches subscriptions (owner-level Redis cache in front, so accounts without webhooks cost nothing on the click path) and records delivery intent in Mongo. A claim-loop executor owns delivery: atomicfindOneAndUpdateclaims with leases make N concurrent executors safe, and the loop needs only Mongo, so it runs in the click worker in production or embedded in the app process for self-hosts without a queue Redis. Deployments without any Redis still work: dispatch happens inline at emit time.Events are stored once per fact; delivery rows are thin references. Rendering happens at first attempt, keeping dispatch insert-only on the consumer ack path.
Security
safe_fetchhardening.webhooks:manageandwebhooks:readAPI key scopes; five endpoints per user (config), 20KB payload cap, tight rate limits on create and test sends.Testing
Full suite green (2815). New coverage: signing vectors, registry/wildcard expansion, matcher scoping, dispatcher fan-out and cap, executor retry/disable paths, route integration with frozen wire shapes, bulk emission parity. Also verified end to end against live Mongo and Redis with a real signed HTTPS delivery to a public echo endpoint.
Rollout
Merges dark. Enabling requires
WEBHOOKS_ENABLED=trueplus seeding thewebhooksfeature flag. Env reference is in.env.example; an Axiom dashboard for dispatch/delivery health ships inaxiom/dashboards/. Follow-ups tracked separately: endpoint auto-disable email notification, Discord/Slack payload flavors, dashboard UI.Summary by CodeRabbit
Closes #271.