diff --git a/doc/DATABASE.md b/doc/DATABASE.md index 8af04cbd37b..cb9810ec53a 100644 --- a/doc/DATABASE.md +++ b/doc/DATABASE.md @@ -53,6 +53,12 @@ reads while holding its company provenance lock, so that plan can delay other agent launches. Missing, malformed, noncanonical, and foreign-company source IDs still deny access. +The subsequent source-task enumeration also keeps `issues.id` as a UUID. Its +OR predicate combines the workspace binding index with the task primary key. +Casting that ID to text forces another scan of company history even after the +workspace privacy query has passed. The workspace authorization regression +suite checks both plans and verifies the attached and retained source results. + To compare this query on a disposable fixture with 13,429 tasks: ```sh diff --git a/server/src/__tests__/workspace-authorization-query.test.ts b/server/src/__tests__/workspace-authorization-query.test.ts index ce66a9c7a19..31c702e908b 100644 --- a/server/src/__tests__/workspace-authorization-query.test.ts +++ b/server/src/__tests__/workspace-authorization-query.test.ts @@ -1,9 +1,11 @@ import { randomUUID } from "node:crypto"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import { and, eq, sql } from "drizzle-orm"; +import { drizzle } from "drizzle-orm/postgres-js"; import { agents, authUsers, companies, companyMemberships, createDb, executionWorkspaces, issueAccessGrants, issues, EMBEDDED_POSTGRES_TEST_TIMEOUT_MS } from "@paperclipai/db"; import { executionWorkspaceReadSqlCondition, type AuthorizationActor } from "../services/authorization.js"; +import { assertTaskWorkspaceAccess } from "../services/task-workspace-source-access.js"; import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; const support = await getEmbeddedPostgresTestSupport(); @@ -66,6 +68,40 @@ const support = await getEmbeddedPostgresTestSupport(); expect(privacyTraversals(result["QUERY PLAN"][0].Plan)).toBeLessThan(100); }); + it("enumerates attached and retained source tasks without scanning unrelated history", async () => { + await retain(sourceId); + const [attached] = await db.insert(issues).values({ companyId, title: "Attached source", executionWorkspaceId: workspaceId }) + .returning({ id: issues.id }); + try { + const statements: Array<{ sql: string; params: unknown[] }> = []; + const recorded = drizzle(db.$client, { + schema: db._.fullSchema, + logger: { logQuery: (query, params) => statements.push({ sql: query, params }) }, + }) as unknown as typeof db; + await assertTaskWorkspaceAccess(recorded, { type: "board", source: "local_implicit", userId: "local-board" }, companyId, workspaceId); + const query = statements.find(statement => statement.sql.startsWith('select "id" from "issues"') + && statement.sql.includes('"execution_workspace_id"')); + expect(query).toBeDefined(); + const rows = await db.$client.unsafe(query!.sql, query!.params as never[]); + expect(rows.map(row => row.id).sort()).toEqual([attached.id, sourceId].sort()); + for (const invalidId of [sourceId.toUpperCase(), "not-a-uuid"]) { + const params = query!.params.map(value => value === sourceId ? invalidId : value); + const noncanonical = await db.$client.unsafe(query!.sql, params as never[]); + expect(noncanonical.map(row => row.id)).toEqual([attached.id]); + } + const [result] = await db.$client.unsafe(`EXPLAIN (ANALYZE, FORMAT JSON) ${query!.sql}`, query!.params as never[]); + type Plan = { "Relation Name"?: string; "Actual Rows": number; "Actual Loops": number; "Rows Removed by Filter"?: number; Plans?: Plan[] }; + function issueRowsExamined(plan: Plan): number { + return (plan["Relation Name"] === "issues" + ? (plan["Actual Rows"] + (plan["Rows Removed by Filter"] ?? 0)) * plan["Actual Loops"] : 0) + + (plan.Plans ?? []).reduce((sum, child) => sum + issueRowsExamined(child), 0); + } + expect(issueRowsExamined(result["QUERY PLAN"][0].Plan)).toBeLessThan(100); + } finally { + await db.delete(issues).where(eq(issues.id, attached.id)); + } + }); + it.each(["missing", "malformed", "uppercase", "foreign"])("denies a %s retained source without throwing a UUID parse error", async kind => { const id = kind === "malformed" ? "not-a-uuid" : kind === "uppercase" ? sourceId.toUpperCase() : randomUUID(); if (kind === "foreign") { diff --git a/server/src/services/task-workspace-source-access.ts b/server/src/services/task-workspace-source-access.ts index cf226305110..bc0cf0a4d14 100644 --- a/server/src/services/task-workspace-source-access.ts +++ b/server/src/services/task-workspace-source-access.ts @@ -3,6 +3,7 @@ import { and, eq, inArray, or, sql } from "drizzle-orm"; import { forbidden } from "../errors.js"; import { accessService } from "./access.js"; import { executionWorkspaceReadSqlCondition, type AuthorizationActor } from "./authorization.js"; +import { canonicalExecutionUuid } from "./execution-identity.js"; type DbTransaction = Parameters[0]>[0]; type WorkspaceAccessContext = { write?: boolean; issueId?: string | null; parentIssueId?: string | null; assigneeAgentId?: string | null; assigneeUserId?: string | null }; @@ -46,7 +47,7 @@ export async function assertTaskWorkspaceAccess( const sources = await db.select({ id: issues.id }).from(issues).where(and( eq(issues.companyId, companyId), or(eq(issues.executionWorkspaceId, workspaceId), retainedIssueIds.length > 0 - ? inArray(sql`${issues.id}::text`, retainedIssueIds) : undefined), + ? inArray(sql`${issues.id}`, retainedIssueIds.map(id => canonicalExecutionUuid(sql`${id}::text`))) : undefined), )); const sourceIds = new Set(sources.map(source => source.id)); if (retainedIssueIds.some(id => !sourceIds.has(id))) throw forbidden("Task workspace source is no longer accessible");