fix: team-scope workflow trigger uniqueness#165
Conversation
Trigger registration surfaced a latent multi-tenant bug: the uniqueness indexes on workflow_triggers were global, not team-scoped. - Migration 022 rebuilds the cron uniqueness index as (team_id, workflow_name, schedule). Workflow names are team-scoped, so two teams may legitimately share a name + schedule; the global index made the second team's apply fail with a unique-constraint violation. - Webhook paths stay a global routing key (an inbound POST /hooks/<path> carries no team identity, so the path must be globally unique). LookupWebhookTrigger no longer filters by the request's team; it resolves by path and returns the owning team from the matched row, and the webhook handler runs the workflow under that team. This also fixes non-default-team webhooks, which previously never routed because the lookup was scoped to the default team. - Email triggers have no uniqueness index and are unaffected. Tests: two teams registering the same workflow name + cron schedule; a non-default-team webhook resolved by a team-agnostic path lookup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnKgCRfMvFcZAkoVbr8k79
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR scopes cron trigger uniqueness to ChangesTeam-scoped trigger uniqueness and webhook routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhookHandler
participant LookupWebhookTrigger
participant Database
participant Auth
participant ExecuteWorkflow
Client->>WebhookHandler: POST /hooks/path
WebhookHandler->>LookupWebhookTrigger: lookup by path only
LookupWebhookTrigger->>Database: SELECT ... WHERE path = ?
Database-->>LookupWebhookTrigger: trigger row (team_id, workflow_name)
LookupWebhookTrigger-->>WebhookHandler: TriggerRecord
WebhookHandler->>Auth: WithUser(trigger.TeamID)
Auth-->>WebhookHandler: execCtx
WebhookHandler->>ExecuteWorkflow: executeWorkflow(execCtx, workflow)
Related Issues: Suggested labels: bug, multi-tenancy, database Suggested reviewers: dvflw Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2289607969
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| FROM workflow_triggers WHERE type = 'webhook' AND path = $1 AND enabled = true AND team_id = $2`, path, teamID, | ||
| ).Scan(&t.ID, &t.WorkflowName, &t.WorkflowVersion, &t.Type, &t.Schedule, &t.Path, &t.Secret, &t.Enabled) | ||
| `SELECT id, workflow_name, workflow_version, type, COALESCE(schedule, ''), COALESCE(path, ''), COALESCE(secret, ''), enabled, team_id | ||
| FROM workflow_triggers WHERE type = 'webhook' AND path = $1 AND enabled = true`, path, |
There was a problem hiding this comment.
In the normal mantle serve path srv.AuthStore is always set and Start wraps apiMux (including /hooks/) in AuthMiddleware, so requests that reach this function already have a caller team in ctx. This path-only lookup ignores that team and the handler then executes under the matched row's team_id, which lets an API key from team A POST to any known webhook path owned by team B and run B's workflow/credentials; keep the team predicate for authenticated requests or explicitly bypass auth only for public webhook mode with separate authorization.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/engine/internal/db/migrations/022_team_scoped_trigger_uniqueness.sql (2)
19-23: 🗄️ Data Integrity & Integration | 🔵 TrivialDown-migration can fail if cross-team duplicates already exist.
If two teams register the same
(workflow_name, schedule)cron pair after the Up migration runs (which is the whole point of this change), reverting to the global(workflow_name, schedule)unique index will fail with a duplicate-key violation. Worth documenting this as an accepted rollback limitation (or adding a pre-check) so an operator isn't surprised mid-rollback.🤖 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/engine/internal/db/migrations/022_team_scoped_trigger_uniqueness.sql` around lines 19 - 23, The Down migration for the workflow_triggers unique index can fail on rollback if multiple teams have already created the same cron pair. Update the migration comment or rollback path around the idx_workflow_triggers_cron DROP/CREATE UNIQUE INDEX block to explicitly document this as a rollback limitation, or add a pre-check before recreating the global unique index so operators get a clear failure mode instead of a surprise duplicate-key error.
14-17: 🚀 Performance & Scalability | 🔵 TrivialConsider index-creation locking for larger tables.
CREATE UNIQUE INDEX(withoutCONCURRENTLY) takes an exclusive lock onworkflow_triggersfor the duration of the build. Likely fine given current table size, but if this table grows or the migration runs against a live production system with meaningful trigger volume, considerCONCURRENTLYwith-- +goose NO TRANSACTION.🤖 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/engine/internal/db/migrations/022_team_scoped_trigger_uniqueness.sql` around lines 14 - 17, The unique index build in the migration is taking an exclusive lock on workflow_triggers, so update the index creation to use CREATE UNIQUE INDEX CONCURRENTLY if this migration may run on a live or larger table. Because CONCURRENTLY cannot run inside a transaction, adjust the goose migration markers accordingly with NO TRANSACTION, and keep the existing idx_workflow_triggers_cron definition and workflow_triggers target so the uniqueness behavior stays the same.
🤖 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.
Nitpick comments:
In
`@packages/engine/internal/db/migrations/022_team_scoped_trigger_uniqueness.sql`:
- Around line 19-23: The Down migration for the workflow_triggers unique index
can fail on rollback if multiple teams have already created the same cron pair.
Update the migration comment or rollback path around the
idx_workflow_triggers_cron DROP/CREATE UNIQUE INDEX block to explicitly document
this as a rollback limitation, or add a pre-check before recreating the global
unique index so operators get a clear failure mode instead of a surprise
duplicate-key error.
- Around line 14-17: The unique index build in the migration is taking an
exclusive lock on workflow_triggers, so update the index creation to use CREATE
UNIQUE INDEX CONCURRENTLY if this migration may run on a live or larger table.
Because CONCURRENTLY cannot run inside a transaction, adjust the goose migration
markers accordingly with NO TRANSACTION, and keep the existing
idx_workflow_triggers_cron definition and workflow_triggers target so the
uniqueness behavior stays the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb8007f4-af5d-419a-9a9f-94c9f17645c2
📒 Files selected for processing (6)
.changeset/team-scoped-trigger-uniqueness.mdpackages/engine/internal/db/migrations/022_team_scoped_trigger_uniqueness.sqlpackages/engine/internal/server/trigger.gopackages/engine/internal/server/trigger_test.gopackages/engine/internal/server/webhook.gopackages/engine/internal/workflow/triggers_test.go
Corrects the webhook approach from the previous commit. The /hooks/ endpoint is behind AuthMiddleware, so every webhook request carries the caller's authenticated team — it is not an anonymous public endpoint. Looking webhooks up by path alone (and running under the matched row's team) let an authenticated caller from team A trigger team B's webhook under B's credentials, a cross-tenant escalation (flagged in review). - LookupWebhookTrigger stays scoped to the caller's team; the handler runs the workflow under the request context (the caller's team), as before. - To still let two teams register the same webhook path, migration 022 now team-scopes the webhook path index too: (team_id, path). The caller's team disambiguates at lookup, so there is no ambiguity or leak. - Documented the down-migration limitation: recreating the global unique indexes fails if cross-team duplicates exist (operator must resolve them before downgrading). - Test rewritten: two teams register the same path; each caller resolves only its own trigger; a third team resolves nothing (no cross-team leak). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnKgCRfMvFcZAkoVbr8k79
|
Good catch — fixed in Security (Codex P1) + webhook multi-tenancy (CodeRabbit linked-issues warning) — both are the same root issue and are now resolved together. I'd wrongly assumed
Migration nitpicks:
Also updated the PR description's design note to match (team-scoped path namespace, not global). Generated by Claude Code |
michaelmcnees
deleted the
claude/office-assistant-mantle-feasibility-n8f4bk
branch
2 participants
Summary
Now that trigger registration is live (#162), a latent multi-tenant bug in
workflow_triggersis reachable: its uniqueness indexes were global, not team-scoped, so two teams couldn't register a workflow with the same name + cron schedule (the second team'smantle applyfailed with a unique-constraint violation). Non-default-team webhooks also never routed, because the webhook lookup was scoped to the request's team (always the default team for unauthenticated inbound requests).Changes
022rebuilds the cron uniqueness index as(team_id, workflow_name, schedule). Workflow names are team-scoped (workflow_definitionsisUNIQUE(team_id, name, version)), so two teams may legitimately share a name + schedule.POST /hooks/<path>carries no team identity, so the path must be unique across all teams.LookupWebhookTriggerno longer filters by the request's team; it resolves by path and returns the owningteam_idfrom the matched row, and the webhook handler runs the workflow under that team. This also fixes non-default-team webhooks, which previously 404'd.Design note
Two valid models existed for webhook multi-tenancy: team-prefixed paths, or a global path namespace with the team derived from the registration. I chose the latter — it matches how public webhook URLs work (the path is the routing key), needs no path-scheme change, and keeps a single global uniqueness guarantee. If you'd prefer team-prefixed paths, happy to switch.
Testing
make test— testcontainer tests (added below) run in CI; couldn't run locally (no Docker in the dev sandbox)make lint—go build ./...,go vet, andgolangci-lint runpassTestSave_CronTriggersUniquePerTeam— two teams register the same workflow name + cron schedule (previously failed on the second apply)TestLookupWebhookTrigger_GlobalPathReturnsOwningTeam— a non-default-team webhook resolves via a team-agnostic path lookup and reports the owning teamRelated Issues
Closes #164
Generated by Claude Code
Summary by CodeRabbit