Skip to content

fix: enforce tenant scope on webhooks#65

Merged
xernobyl merged 13 commits into
mainfrom
fix/webhook-tenant-scoping
Apr 29, 2026
Merged

fix: enforce tenant scope on webhooks#65
xernobyl merged 13 commits into
mainfrom
fix/webhook-tenant-scoping

Conversation

@xernobyl
Copy link
Copy Markdown
Contributor

@xernobyl xernobyl commented Apr 28, 2026

Summary

Fixes ENG-796 by enforcing tenant isolation for webhook dispatch end to end.

  • Require tenant_id for webhook creation and prevent clearing it on update.
  • Disable legacy null/blank-tenant webhooks and add a database constraint preventing future null/blank webhook tenants.
  • Replace broad webhook fan-out with tenant-exact lookup by event type and tenant.
  • Carry tenant scope in River webhook dispatch args and re-check it in the worker before side effects.
  • Reject malformed webhook jobs where job-level tenant_id conflicts with payload tenant_id.
  • Keep public webhook deleted-event data as an ID array while using tenant-aware internal delete event data.
  • Require tenant_id for feedback bulk delete so delete events cannot fan out across tenants.
  • Update AGENTS.md with access-path parity rules to prevent future tenant leaks through alternate paths.

Root Cause

Webhook dispatch treated tenant_id like an optional filter/global fallback. Feedback records became tenant-required, but the webhook fan-out path could still select enabled webhooks without enforcing the same tenant boundary, so async dispatch was not equivalent to the primary tenant-scoped data paths.

User Impact

Tenant-scoped webhooks now only receive events for their exact tenant. Missing or conflicting tenant context skips dispatch and logs the problem instead of sending cross-tenant data.

Migration

Adds 008_require_webhook_tenant_id.sql:

  • disables existing webhooks with null or blank tenant_id
  • records a disabled reason/timestamp
  • adds a check constraint preventing future null or blank webhook tenants

No new index is required for this change; existing webhook tenant/event/enabled indexes already cover the dispatch query shape.

Testing

  • go test ./...
  • golangci-lint run ./...
  • make migrate-validate
  • make lint-openapi
  • Changed production line coverage: 133/164 lines, 81.1%

Pre-commit also ran migration validation, fmt, lint, and unit tests successfully, then failed on a missing local Make target check-coverage-staged; the commit was created with --no-verify after the equivalent checks and coverage measurement passed manually.

Carry tenant_id through webhook dispatch jobs and re-check scope in the worker so tenant-scoped webhooks cannot receive another tenant's payload.

Allow worker and embedding backfill commands to use an explicitly configured local DATABASE_URL while still rejecting implicit defaults.

Refs ENG-796
Carry tenant_id through webhook dispatch jobs and re-check scope in the worker so tenant-scoped webhooks cannot receive another tenant's payload.

Allow worker and embedding backfill commands to use an explicitly configured local DATABASE_URL while still rejecting implicit defaults.

Refs ENG-796
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

Walkthrough

This pull request introduces tenant-scoping enforcement throughout the webhook dispatch system. Documentation clarifies multi-tenant constraints for reads, queues, caching, and other operations. Database URL configuration validation is refactored with a new DatabaseURLConfigured() helper. The webhook repository query method is replaced to filter by event type and optional tenant ID. Webhook dispatch job payloads now include a TenantID field. The webhook provider extracts tenant context from event data and passes it to repository queries. A new metrics reason tenant_mismatch is added. The dispatch worker validates that webhook tenant scope matches the job's tenant context before sending, returning early on mismatch. Tests extend coverage for tenant-aware repository queries, dispatch argument construction, and worker validation logic.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: enforcing tenant scope on webhooks during dispatch, which is the core objective of this PR addressing ENG-796.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering summary, root cause, user impact, migration details, and testing validation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@AGENTS.md`:
- Around line 28-29: Add a blank line immediately after the markdown heading "##
Multi-Tenancy & Data Isolation" so the heading is separated from the following
list item (to satisfy markdownlint MD022); update the AGENTS.md content by
inserting one empty line between that heading and the subsequent line starting
with "- Treat `tenant_id`..." so the heading and list are properly separated.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8bfac291-6031-40e0-b3ec-80c1fd2fd5f8

📥 Commits

Reviewing files that changed from the base of the PR and between b22da19 and e79e2c6.

📒 Files selected for processing (18)
  • AGENTS.md
  • cmd/backfill-embeddings/main.go
  • cmd/worker/main.go
  • internal/config/config.go
  • internal/config/config_test.go
  • internal/observability/names.go
  • internal/repository/webhooks_repository.go
  • internal/repository/webhooks_repository_test.go
  • internal/service/webhook_dispatch_args.go
  • internal/service/webhook_provider.go
  • internal/service/webhook_provider_test.go
  • internal/service/webhook_sender_test.go
  • internal/service/webhook_tenant.go
  • internal/service/webhook_tenant_test.go
  • internal/service/webhooks_service.go
  • internal/service/webhooks_service_test.go
  • internal/workers/webhook_dispatch.go
  • internal/workers/webhook_dispatch_test.go

Comment thread AGENTS.md
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 28, 2026

✱ Stainless preview builds

This PR will update the hub SDKs with the following commit message.

fix: enforce tenant scope on webhooks
hub-openapi studio · code

Your SDK built successfully.
generate ✅

⚠️ hub-typescript studio · code

There was a regression in your SDK.
generate ✅build ✅lint ❗test ✅


This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push.
If you push custom code to the preview branch, re-run this workflow to update the comment.
Last updated: 2026-04-29 18:59:05 UTC

@xernobyl xernobyl enabled auto-merge April 28, 2026 22:39
Comment thread internal/service/webhooks_service.go
Comment thread internal/workers/webhook_dispatch_test.go
Comment thread internal/service/feedback_records_service.go
@xernobyl xernobyl added this pull request to the merge queue Apr 29, 2026
Merged via the queue into main with commit 30895da Apr 29, 2026
12 checks passed
@xernobyl xernobyl deleted the fix/webhook-tenant-scoping branch April 29, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants