feat(security): plugin tables get company_id FK for tenant isolation (#5865)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - The plugin subsystem persists state into four tables (`plugin_entities`, `plugin_job_runs`, `plugin_logs`, `plugin_webhook_deliveries`) and those rows currently have no notion of an owning tenant — so company-deletion doesn't cascade plugin state, and operators have no way to query "what does this plugin own for company X?" > - The fix is one thin slice of tenant-isolation hygiene that doesn't change any external API: add a nullable `company_id` FK with `ON DELETE CASCADE` to the four plugin tables, index it, and scope the `plugin_entities` external-id uniqueness per-tenant > - The benefit is plugin-row tenant attribution + cascade cleanup, with zero impact on single-tenant local-first deploys (`NULL` continues to mean instance-scope) > **Rebase note (scope narrowed):** This PR originally also hardened the schedulers (`heartbeat.tickTimers` / `resumeQueuedRuns` / `enqueueWakeup` and `routines.tickScheduledTriggers`) to skip archived companies. That half has since landed on `master` via #7478 (`93206f73`, "Stop archived companies from waking agents") with a stricter implementation (`status != 'active'` plus a skipped-request audit row). On rebase those changes were dropped as redundant — `heartbeat.ts` and `routines.ts` are now identical to `master`, and the scheduler-specific tests were removed. **This PR is now DB-only.** ## Linked Issues or Issue Description No existing issue covers this directly — problem described in-PR: - Four plugin tables (`plugin_entities`, `plugin_job_runs`, `plugin_logs`, `plugin_webhook_deliveries`) persist rows with no notion of an owning tenant. - Company deletion therefore does not cascade plugin state, and operators have no way to query "what does this plugin own for company X?" - One thin slice of tenant-isolation hygiene fixes this without changing any external API: a nullable `company_id` FK with `ON DELETE CASCADE`, an index, and per-tenant scoping of the `plugin_entities` external-id uniqueness. - Part of the multi-tenant hardening initiative alongside #3967 (cross-tenant 404 oracle) and #5864 (per-company JWT keys). ## What Changed **Schema (`packages/db/src/schema/plugin_*.ts`):** - Nullable `companyId` FK with `onDelete: "cascade"` added to `plugin_entities`, `plugin_job_runs`, `plugin_logs`, `plugin_webhook_deliveries`. - A btree index on each new `company_id` column (`<table>_company_idx`). - `plugin_entities_external_idx` rescoped from `(plugin_id, entity_type, external_id)` to `(company_id, plugin_id, entity_type, external_id)` and switched to `UNIQUE … NULLS NOT DISTINCT` so instance-scope rows (`company_id IS NULL`) keep their dedup guarantee while tenants get their own namespace. **Migration:** - `0095_plugin_company_id_tenant_isolation.sql` — 14 statements: 4 column adds + 4 FK constraints (`ON DELETE CASCADE`) + 4 indexes + drop/recreate of the external-id unique constraint. - Journal entry + regenerated `0095_snapshot.json`. **Tests:** - `server/src/__tests__/plugin-tenant-isolation.test.ts` — `NULL` preserves backward compat; `CASCADE` on company delete across all four tables; per-tenant external-id namespacing; NULL-NULL collision rejected (`NULLS NOT DISTINCT`). ## Verification - `pnpm --filter @paperclipai/db run check:migrations` — pass. - `pnpm --filter @paperclipai/db typecheck` (`tsc`) — pass. - `vitest run plugin-tenant-isolation` — **4/4 pass** (embedded Postgres applies `0095` and exercises cascade + NULLS NOT DISTINCT). ## Notes - **Clean snapshot, no drift.** The earlier revision of this PR shipped a ~17.6k-line meta snapshot that was almost entirely pre-existing drift. On rebase the migration was renumbered (the old `0090_brainy_darkhawk` collided with `master`'s `0090_resource_memberships … 0094`) and regenerated from the current `master` baseline via `drizzle-kit generate`. The result is a 14-statement migration containing **only** the plugin-table changes — no unrelated drift. - **Backward-compatible.** `NULL company_id` continues to mean instance-scope (cron jobs, public webhooks). No new env vars, no API surface change. Single-tenant local-first deploys unaffected. ## Risks - **Migration is additive and nullable** — `0095` adds nullable `company_id` columns, FK constraints, and indexes; existing rows stay valid (`NULL` keeps meaning instance-scope) and no backfill is required. - **`ON DELETE CASCADE` is a behavioral change**: deleting a company now removes its plugin rows across all four tables. Intended (it is the point of the PR), but operators relying on plugin rows surviving company deletion would be affected. Covered by the cascade tests. - **Uniqueness semantics change on `plugin_entities`**: the external-id constraint is rescoped per-tenant and switched to `UNIQUE … NULLS NOT DISTINCT`, so two instance-scope rows (`company_id IS NULL`) with the same external id are now rejected instead of coexisting. Covered by the NULL-NULL collision test. - **No API surface change, no new env vars.** Single-tenant local-first deploys unaffected. (Section added retroactively to match the PR template; distilled from the What Changed / Notes sections above.) ## Model Used Same authoring setup as #5864 (same series, same day): Claude Opus 4.7 (1M context), extended thinking mode. (Section added retroactively.) ## Checklist - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] Tests run locally and pass (plugin-tenant-isolation 4/4) - [x] `check:migrations` + db typecheck pass - [x] No UI changes - [x] Migration carries only the intended changes (no snapshot drift) - [x] Scheduler half dropped as superseded by #7478 Part of the multi-tenant hardening initiative — see also #3967 (cross-tenant 404 oracle) and #5864 (per-company JWT keys). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Devin Foley <devin@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Jannes Stubbemann
committed by
GitHub
GitHub
parent
a5b3cc98b0
commit
05bcd3ce84
@@ -404,6 +404,14 @@ function sanitiseMeta(meta: Record<string, unknown> | null | undefined): Record<
|
||||
interface BufferedLogEntry {
|
||||
db: Db;
|
||||
pluginId: string;
|
||||
/**
|
||||
* Owning tenant for `plugin_logs.company_id` — populated when the caller
|
||||
* attributes the log/metric to a specific company so the row participates
|
||||
* in the `ON DELETE CASCADE` from `companies`. `null` means instance-scope
|
||||
* (cron jobs / public webhooks without a tenant); those rows survive
|
||||
* company deletes but are still attributable.
|
||||
*/
|
||||
companyId: string | null;
|
||||
level: string;
|
||||
message: string;
|
||||
meta: Record<string, unknown> | null;
|
||||
@@ -436,6 +444,7 @@ export async function flushPluginLogBuffer(): Promise<void> {
|
||||
for (const [dbInstance, group] of byDb) {
|
||||
const values = group.map((e) => ({
|
||||
pluginId: e.pluginId,
|
||||
companyId: e.companyId,
|
||||
level: e.level,
|
||||
message: e.message,
|
||||
meta: e.meta,
|
||||
@@ -1262,6 +1271,7 @@ export function buildHostServices(
|
||||
_logBuffer.push({
|
||||
db,
|
||||
pluginId,
|
||||
companyId: params.companyId ?? null,
|
||||
level: "metric",
|
||||
message: safeName,
|
||||
meta: sanitiseMeta({ value: params.value, tags: params.tags ?? null }),
|
||||
@@ -1310,6 +1320,7 @@ export function buildHostServices(
|
||||
_logBuffer.push({
|
||||
db,
|
||||
pluginId,
|
||||
companyId: params.companyId ?? null,
|
||||
level: level ?? "info",
|
||||
message: safeMessage,
|
||||
meta: safeMeta,
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { asc, eq, ne, sql, and } from "drizzle-orm";
|
||||
import { asc, eq, isNull, ne, sql, and } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import {
|
||||
plugins,
|
||||
@@ -473,27 +473,41 @@ export function pluginRegistryService(db: Db) {
|
||||
/**
|
||||
* Look up a plugin-owned entity mapping by its external identifier.
|
||||
*
|
||||
* Scope matches `plugin_entities_external_idx` (NULLS NOT DISTINCT):
|
||||
* pass the owning `companyId` (or `null` for instance-scope) to retrieve
|
||||
* the row that belongs to that tenant. Two companies can share the same
|
||||
* `(pluginId, entityType, externalId)` tuple — omitting `companyId` would
|
||||
* return the first matched row regardless of tenant, which is unsafe.
|
||||
*
|
||||
* @param pluginId - The UUID of the plugin.
|
||||
* @param entityType - The type of entity (e.g., 'project', 'issue').
|
||||
* @param externalId - The identifier in the external system.
|
||||
* @param companyId - Tenant scope; `null` for instance-scope entities.
|
||||
* @returns The matching `PluginEntityRecord` or null.
|
||||
*/
|
||||
getEntityByExternalId: (
|
||||
pluginId: string,
|
||||
entityType: string,
|
||||
externalId: string,
|
||||
) =>
|
||||
db
|
||||
companyId: string | null,
|
||||
) => {
|
||||
const companyIdPredicate =
|
||||
companyId == null
|
||||
? isNull(pluginEntities.companyId)
|
||||
: eq(pluginEntities.companyId, companyId);
|
||||
return db
|
||||
.select()
|
||||
.from(pluginEntities)
|
||||
.where(
|
||||
and(
|
||||
companyIdPredicate,
|
||||
eq(pluginEntities.pluginId, pluginId),
|
||||
eq(pluginEntities.entityType, entityType),
|
||||
eq(pluginEntities.externalId, externalId),
|
||||
),
|
||||
)
|
||||
.then((rows) => rows[0] ?? null),
|
||||
.then((rows) => rows[0] ?? null);
|
||||
},
|
||||
|
||||
/**
|
||||
* Create or update a persistent mapping between a Paperclip object and an
|
||||
@@ -509,11 +523,22 @@ export function pluginRegistryService(db: Db) {
|
||||
) => {
|
||||
// Drizzle doesn't support pg-specific onConflictDoUpdate easily in the insert() call
|
||||
// with complex where clauses, so we do it manually.
|
||||
// Match the per-tenant uniqueness of `plugin_entities_external_idx`
|
||||
// (companyId, pluginId, entityType, externalId) with NULLS NOT DISTINCT
|
||||
// semantics: two companies (and instance-scope NULLs across each other)
|
||||
// may share the same (pluginId, entityType, externalId) tuple, so the
|
||||
// lookup MUST scope by companyId — `isNull` for instance-scope, `eq`
|
||||
// otherwise — to avoid returning and overwriting another tenant's row.
|
||||
const companyIdPredicate =
|
||||
input.companyId == null
|
||||
? isNull(pluginEntities.companyId)
|
||||
: eq(pluginEntities.companyId, input.companyId);
|
||||
const existing = await db
|
||||
.select()
|
||||
.from(pluginEntities)
|
||||
.where(
|
||||
and(
|
||||
companyIdPredicate,
|
||||
eq(pluginEntities.pluginId, pluginId),
|
||||
eq(pluginEntities.entityType, input.entityType),
|
||||
eq(pluginEntities.externalId, input.externalId ?? ""),
|
||||
@@ -633,21 +658,29 @@ export function pluginRegistryService(db: Db) {
|
||||
/**
|
||||
* Record the start of a specific job execution.
|
||||
*
|
||||
* Pass the owning `companyId` so `plugin_job_runs.company_id` is populated
|
||||
* and the row participates in the `ON DELETE CASCADE` from `companies`.
|
||||
* `null` is the explicit instance-scope marker (cron jobs without a tenant);
|
||||
* those rows survive company deletes but are still attributable.
|
||||
*
|
||||
* @param pluginId - The UUID of the plugin.
|
||||
* @param jobId - The UUID of the parent job record.
|
||||
* @param trigger - What triggered this run (e.g., 'schedule', 'manual').
|
||||
* @param companyId - Tenant scope; `null` for instance-scope runs.
|
||||
* @returns The newly created `PluginJobRunRecord` in 'pending' status.
|
||||
*/
|
||||
createJobRun: async (
|
||||
pluginId: string,
|
||||
jobId: string,
|
||||
trigger: PluginJobRunTrigger,
|
||||
companyId: string | null,
|
||||
) => {
|
||||
return db
|
||||
.insert(pluginJobRuns)
|
||||
.values({
|
||||
pluginId,
|
||||
jobId,
|
||||
companyId,
|
||||
trigger,
|
||||
status: "pending",
|
||||
})
|
||||
@@ -686,14 +719,22 @@ export function pluginRegistryService(db: Db) {
|
||||
/**
|
||||
* Create a record for an incoming webhook delivery.
|
||||
*
|
||||
* Pass the owning `companyId` so `plugin_webhook_deliveries.company_id` is
|
||||
* populated and the row participates in the `ON DELETE CASCADE` from
|
||||
* `companies`. `null` is the explicit instance-scope marker (public
|
||||
* webhooks without a tenant); those rows survive company deletes but are
|
||||
* still attributable.
|
||||
*
|
||||
* @param pluginId - The UUID of the receiving plugin.
|
||||
* @param webhookKey - The endpoint key defined in the manifest.
|
||||
* @param companyId - Tenant scope; `null` for instance-scope deliveries.
|
||||
* @param input - The payload, headers, and optional external ID.
|
||||
* @returns The newly created `PluginWebhookDeliveryRecord` in 'pending' status.
|
||||
*/
|
||||
createWebhookDelivery: async (
|
||||
pluginId: string,
|
||||
webhookKey: string,
|
||||
companyId: string | null,
|
||||
input: {
|
||||
externalId?: string;
|
||||
payload: Record<string, unknown>;
|
||||
@@ -705,6 +746,7 @@ export function pluginRegistryService(db: Db) {
|
||||
.values({
|
||||
pluginId,
|
||||
webhookKey,
|
||||
companyId,
|
||||
externalId: input.externalId,
|
||||
payload: input.payload,
|
||||
headers: input.headers ?? {},
|
||||
|
||||
Reference in New Issue
Block a user