diff --git a/server/src/__tests__/project-goal-validation.test.ts b/server/src/__tests__/project-goal-validation.test.ts new file mode 100644 index 0000000000..117a792be2 --- /dev/null +++ b/server/src/__tests__/project-goal-validation.test.ts @@ -0,0 +1,162 @@ +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { eq } from "drizzle-orm"; +import { + companies, + createDb, + goals, + projectGoals, + projects as projectsTable, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { HttpError } from "../errors.js"; +import { projectService } from "../services/projects.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres project goal validation tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`, + ); +} + +// A project's goals must exist and belong to the same company. Before this +// validation, a nonexistent goal id died at the projects.goal_id foreign key +// as an opaque 500 (observed live 2026-09-03, retried four times by the +// caller), and a goal from another company linked silently — the foreign key +// proves existence, not ownership. +describeEmbeddedPostgres("project goal validation", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + let prefixCounter = 0; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-project-goal-validation-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(projectGoals); + await db.delete(projectsTable); + await db.delete(goals); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + async function seedCompany(name: string): Promise { + prefixCounter += 1; + const [company] = await db + .insert(companies) + .values({ name, issuePrefix: `GV${prefixCounter}` }) + .returning(); + return company.id; + } + + async function seedGoal(companyId: string, title: string): Promise { + const [goal] = await db + .insert(goals) + .values({ companyId, title, level: "task", status: "active" }) + .returning(); + return goal.id; + } + + function expectUnprocessable(error: unknown, unknownGoalId: string) { + expect(error).toBeInstanceOf(HttpError); + expect((error as HttpError).status).toBe(422); + expect((error as Error).message).toContain(unknownGoalId); + } + + it("creates and links a project to a goal of the same company", async () => { + const companyId = await seedCompany("Valid Co"); + const goalId = await seedGoal(companyId, "Ship it"); + const projects = projectService(db); + + const created = await projects.create(companyId, { name: "Rocket", goalIds: [goalId] }); + expect(created.goalIds).toEqual([goalId]); + }); + + it("rejects a create whose goal id does not exist, before any insert", async () => { + const companyId = await seedCompany("Missing Goal Co"); + const projects = projectService(db); + const ghost = "db7da378-99a6-43ea-964f-000000000000"; + + const failure = await projects + .create(companyId, { name: "Rocket", goalIds: [ghost] }) + .then(() => null, (error: unknown) => error); + expectUnprocessable(failure, ghost); + + const rows = await db.select({ id: projectsTable.id }).from(projectsTable); + expect(rows).toHaveLength(0); + }); + + it("rejects the legacy single goalId field the same way", async () => { + const companyId = await seedCompany("Legacy Field Co"); + const projects = projectService(db); + const ghost = "db7da378-99a6-43ea-964f-111111111111"; + + const failure = await projects + .create(companyId, { name: "Rocket", goalId: ghost }) + .then(() => null, (error: unknown) => error); + expectUnprocessable(failure, ghost); + }); + + it("rejects another company's goal on create — the FK only proves existence", async () => { + const companyId = await seedCompany("Home Co"); + const otherCompanyId = await seedCompany("Other Co"); + const foreignGoalId = await seedGoal(otherCompanyId, "Not yours"); + const projects = projectService(db); + + const failure = await projects + .create(companyId, { name: "Rocket", goalIds: [foreignGoalId] }) + .then(() => null, (error: unknown) => error); + expectUnprocessable(failure, foreignGoalId); + }); + + it("ignores the legacy goalId when an explicit empty goalIds list wins resolution", async () => { + // goalIds and goalId may arrive together; the resolved set (goalIds + // first) is canonical for persistence too. Before this rule, an empty + // list skipped validation while the raw legacy id was still written — + // unvalidated, and unchecked for ownership. + const companyId = await seedCompany("Conflicting Fields Co"); + const otherCompanyId = await seedCompany("Conflicting Other Co"); + const foreignGoalId = await seedGoal(otherCompanyId, "Should not link"); + const projects = projectService(db); + + const created = await projects.create(companyId, { + name: "Rocket", + goalIds: [], + goalId: foreignGoalId, + }); + expect(created.goalIds).toEqual([]); + + const [row] = await db + .select({ goalId: projectsTable.goalId }) + .from(projectsTable) + .where(eq(projectsTable.id, created.id)); + expect(row.goalId).toBeNull(); + }); + + it("rejects an update to an unknown or foreign goal and leaves links unchanged", async () => { + const companyId = await seedCompany("Update Co"); + const otherCompanyId = await seedCompany("Update Other Co"); + const goodGoalId = await seedGoal(companyId, "Good goal"); + const foreignGoalId = await seedGoal(otherCompanyId, "Foreign goal"); + const projects = projectService(db); + + const created = await projects.create(companyId, { name: "Rocket", goalIds: [goodGoalId] }); + + const failure = await projects + .update(created.id, { goalIds: [foreignGoalId] }) + .then(() => null, (error: unknown) => error); + expectUnprocessable(failure, foreignGoalId); + + const fetched = await projects.getById(created.id); + expect(fetched?.goalIds).toEqual([goodGoalId]); + }); +}); diff --git a/server/src/services/projects.ts b/server/src/services/projects.ts index 5ac1b50e4c..0f193a98a7 100644 --- a/server/src/services/projects.ts +++ b/server/src/services/projects.ts @@ -28,6 +28,7 @@ import { type PluginManagedProjectDeclaration, type PluginManagedProjectResolution, } from "@paperclipai/shared"; +import { unprocessable } from "../errors.js"; import { listCurrentRuntimeServicesForProjectWorkspaces } from "./workspace-runtime-read-model.js"; import { parseProjectExecutionWorkspacePolicy } from "./execution-workspace-policy.js"; import { mergeProjectWorkspaceRuntimeConfig, readProjectWorkspaceRuntimeConfig } from "./project-workspace-runtime-config.js"; @@ -401,6 +402,31 @@ async function attachListMetrics( } /** Sync the project_goals join table for a single project. */ +/** + * Every goal a project links to must exist and belong to the same company. + * Without this check a nonexistent id only dies at the projects.goal_id + * foreign key — an opaque 500 the caller retries (observed live + * 2026-09-03: four identical retries of one bad id) — and a goal from + * another company would link silently, because the foreign key proves + * existence, not ownership. + */ +async function assertGoalsBelongToCompany(db: Db, companyId: string, goalIds: string[]): Promise { + if (goalIds.length === 0) return; + const unique = [...new Set(goalIds)]; + const found = await db + .select({ id: goals.id }) + .from(goals) + .where(and(eq(goals.companyId, companyId), inArray(goals.id, unique))); + const foundIds = new Set(found.map((row) => row.id)); + const unknown = unique.filter((goalId) => !foundIds.has(goalId)); + if (unknown.length > 0) { + throw unprocessable( + `Unknown goal id(s) for this company: ${unknown.join(", ")}`, + { unknownGoalIds: unknown }, + ); + } +} + async function syncGoalLinks(db: Db, projectId: string, companyId: string, goalIds: string[]) { // Delete existing links await db.delete(projectGoals).where(eq(projectGoals.projectId, projectId)); @@ -547,6 +573,7 @@ export function projectService(db: Db) { ): Promise => { const { goalIds: inputGoalIds, ...projectData } = data; const ids = resolveGoalIds({ goalIds: inputGoalIds, goalId: projectData.goalId }); + if (ids && ids.length > 0) await assertGoalsBelongToCompany(db, companyId, ids); // Note: color is intentionally NOT auto-assigned. New projects default to // `color = null` (neutral gray) unless an explicit color is supplied. See PAP-68. @@ -558,7 +585,11 @@ export function projectService(db: Db) { projectData.name = resolveProjectNameForUniqueShortname(projectData.name, existingProjects); // Also write goalId to the legacy column (first goal or null) - const legacyGoalId = ids && ids.length > 0 ? ids[0] : projectData.goalId ?? null; + // The resolved set is canonical for persistence as well as validation: + // falling back to the raw legacy field here would write an id that + // skipped validation whenever `goalIds: []` and `goalId` arrive + // together (goalIds wins resolution, mirroring the update path). + const legacyGoalId = ids?.[0] ?? null; const row = await db .insert(projects) @@ -794,6 +825,9 @@ export function projectService(db: Db) { .where(eq(projects.id, id)) .then((rows) => rows[0] ?? null); if (!existingProject) return null; + if (ids && ids.length > 0) { + await assertGoalsBelongToCompany(db, existingProject.companyId, ids); + } if (projectData.name !== undefined) { const existingShortname = normalizeProjectUrlKey(existingProject.name);