fix(server): validate project goal ids exist and belong to the company (#12779)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Projects can link to goals, through the `goalIds` list or the legacy
`goalId` field. The project service writes those links on create and
update.
> - The service never checked the goal ids. A nonexistent id died at the
`projects.goal_id` foreign key as an opaque 500, and the caller got no
actionable feedback — observed live on 2026-09-03, where one caller
retried the same bad id four times.
> - The foreign key also only proves a goal exists, not who owns it. A
goal id from another company linked silently on a multi-company
instance.
> - This pull request asserts every resolved goal id exists under the
caller's company before any write, and rejects with a 422 that names the
unknown ids.
> - The benefit is a clear, actionable client error instead of a 500,
and no cross-company goal links.
## Linked Issues or Issue Description
**What happened?**
`POST /companies/:companyId/projects` with a `goalIds` entry that does
not exist fails with an internal error: `insert or update on table
"projects" violates foreign key constraint
"projects_goal_id_goals_id_fk"`. The caller sees a 500 and retries. A
goal id that exists but belongs to a different company is accepted and
linked.
**Expected behavior**
The request fails fast with a 422 that names the unknown goal id(s).
Goals from other companies are rejected the same way. Valid links behave
exactly as before.
**Steps to reproduce**
1. Create a company and no goals.
2. `POST /companies/:companyId/projects` with `{ "name": "Rocket",
"goalIds": ["<any-uuid>"] }`.
3. Before this change: 500 from the foreign key. After: 422 naming the
id.
**Deployment mode**
Any; observed on an authenticated public deployment.
## What Changed
- `assertGoalsBelongToCompany` in the project service: one query for the
resolved ids scoped to the company; unknown ids produce `unprocessable`
(422) with the ids in the message and details
- called on create (before the project row insert, so no partial writes)
and on update (scoped to the existing project's company); both `goalIds`
and the legacy `goalId` field flow through the same resolution
- new embedded-Postgres test file: valid link, nonexistent id on create
with no partial insert, legacy field, another company's goal on create,
and a foreign-goal update that leaves existing links unchanged
## Verification
- `pnpm vitest run src/__tests__/project-goal-validation.test.ts` — 5
passed
- adjacent suites (`project-icon-persistence`,
`project-shortname-resolution`, `issue-goal-fallback`,
`project-goal-telemetry-routes`, `heartbeat-referenced-projects`,
`projects-list-archived-routes`) — 35 passed
## Risks
- Low risk. One extra indexed select per create/update that carries goal
ids. Requests that previously 500ed now 422; requests that silently
linked a foreign goal now fail — both are corrections, not regressions.
- Existing rows with foreign links (written before this check) are
untouched; only new writes validate.
## Model Used
Claude Fable 5 (claude-fable-5) via Claude Code, extended thinking with
tool use.
## Checklist
- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above (none found for goal-id validation)
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes (doc
comments; no user-facing docs affected)
- [x] I have considered and documented any risks above
This commit is contained in:
parent
2177b85eb5
commit
db4eeb1688
|
|
@ -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<typeof createDb>;
|
||||
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | 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<string> {
|
||||
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<string> {
|
||||
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]);
|
||||
});
|
||||
});
|
||||
|
|
@ -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<void> {
|
||||
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<ProjectWithGoals> => {
|
||||
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);
|
||||
|
|
|
|||
Loading…
Reference in New Issue