mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-24 18:19:05 +02:00
Guard routine UUID lookups without narrowing valid inputs (#15313)
## Thinking Path > - Paperclip manages work for AI agents and their companies. > - Routines expose details, triggers, and management actions through resource IDs. > - These resource IDs are UUIDs. The UI and CLI do not resolve short prefixes. > - A malformed ID reaches a PostgreSQL UUID comparison and returns a server error. > - A lookup guard can return the existing not-found response before that query. > - This PR preserves valid PostgreSQL UUID input forms and the existing access checks. ## Linked Issues or Issue Description Refs #11471. This is a credited continuation of the lookup-guard approach from @mv2woods. That older PR remains open and unchanged. Its review requested the required PR description sections. This continuation uses current master and adds UUID compatibility and authorization coverage. The earlier `isUuidLike` guard restricts UUID versions to 1–5 and trims input. The database currently accepts other UUID values and input forms. This guard preserves the existing [PostgreSQL UUID input contract](https://www.postgresql.org/docs/current/datatype-uuid.html), including uppercase, paired braces, omitted hyphens, and hyphens after groups of four digits. It rejects whitespace and short prefixes without rewriting the value sent to the database. ## What Changed - Guard the shared routine and private-trigger UUID lookups. Malformed resource IDs return null, so existing routes return their normal 404 response. - Keep company access, assignee permissions, body validation order, and database error propagation unchanged. - Add real PostgreSQL tests for stored UUIDv4, UUIDv7, nil, and max values in six input forms. Add no-query checks for malformed input and HTTP coverage across all 15 root-resource route handlers. - Document complete resource IDs and distinguish them from opaque public webhook IDs. ## Verification - `pnpm exec vitest run server/src/__tests__/routines-service.test.ts server/src/__tests__/routines-e2e.test.ts`: 91 passed, including the new PostgreSQL and HTTP regressions. - Independent review found no blockers and passed all 9 focused PostgreSQL and HTTP regression cases on this commit. - Full local `pnpm -r typecheck` and `pnpm build` passed on Node 24.21.0 with pnpm 9.15.4. - Full Linux CI passed on `0a2990eca2f904335821027a322e4db868151eb3`: 53 successful checks and 2 inapplicable Storybook skips. No retries were needed. This provides aggregate suite coverage; a duplicate full local aggregate was not run. - [Greptile reviewed this exact commit](https://github.com/paperclipai/paperclip/pull/15313#issuecomment-6010222312) at 5/5 with no findings or unresolved threads. The PR is ready for review and has no merge conflicts. - `git diff --check` and the added-diff secret and PII scan passed. ## Risks Malformed routine and private-trigger IDs now return 404 instead of a database error. Full UUIDs retain the existing company and assignee checks. This change does not add prefix lookup, alter public webhook IDs, or validate unrelated nested revision/thread IDs or query parameters. No migration is required. ## Model Used OpenAI GPT-6 through Codex, with reasoning, repository editing, code execution, and independent agent review. The exact deployment model ID and context window are not exposed in this session. ## 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 - [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 - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
bf14f803d5
commit
f858207161
4 files changed
+187
No files matched your search
@@ -21,6 +21,13 @@ GET /api/routines/{routineId}
|
||||
|
||||
Returns routine details including triggers.
|
||||
|
||||
Routine and trigger resource IDs must be complete UUIDs from the API response.
|
||||
Short prefixes and malformed IDs return the same `404` as a missing resource.
|
||||
UUID lookup accepts PostgreSQL's existing input forms, including uppercase,
|
||||
compact and braced UUIDs; it does not trim whitespace or resolve prefixes.
|
||||
Company access and routine-assignee permissions still apply. Public trigger
|
||||
IDs in webhook URLs are separate opaque identifiers, not resource UUIDs.
|
||||
|
||||
## Create Routine
|
||||
|
||||
```
|
||||
|
||||
@@ -238,6 +238,119 @@ describeEmbeddedPostgres("routine routes end-to-end", () => {
|
||||
return { companyId, agentId, projectId, userId };
|
||||
}
|
||||
|
||||
function routineResourceRequests(app: express.Express, routineId: string, triggerId = routineId) {
|
||||
const path = `/api/routines/${encodeURIComponent(routineId)}`;
|
||||
const triggerPath = `/api/routine-triggers/${encodeURIComponent(triggerId)}`;
|
||||
const nestedId = "12345678-1234-4234-8234-123456789abc";
|
||||
return [
|
||||
request(app).get(path),
|
||||
request(app).get(`${path}/runs`),
|
||||
request(app).get(`${path}/revisions`),
|
||||
request(app).get(`${path}/description/annotations`),
|
||||
request(app).get(`${path}/description/annotations/${nestedId}`),
|
||||
request(app).post(`${path}/description/annotations`).send({
|
||||
baseRevisionId: nestedId, baseRevisionNumber: 1, body: "Review this text",
|
||||
selector: {
|
||||
quote: { exact: "text" },
|
||||
position: { normalizedStart: 0, normalizedEnd: 4, markdownStart: 0, markdownEnd: 4 },
|
||||
},
|
||||
}),
|
||||
request(app).post(`${path}/description/annotations/${nestedId}/comments`).send({ body: "Review" }),
|
||||
request(app).patch(`${path}/description/annotations/${nestedId}`).send({ status: "resolved" }),
|
||||
request(app).patch(path).send({ title: "Updated routine" }),
|
||||
request(app).post(`${path}/revisions/${nestedId}/restore`).send({}),
|
||||
request(app).post(`${path}/triggers`).send({ kind: "api" }),
|
||||
request(app).post(`${path}/run`).send({}),
|
||||
request(app).patch(triggerPath).send({ enabled: false }),
|
||||
request(app).delete(triggerPath),
|
||||
request(app).post(`${triggerPath}/rotate-secret`).send({}),
|
||||
];
|
||||
}
|
||||
|
||||
it("returns not found without queries for malformed routine and trigger IDs on every resource route", async () => {
|
||||
const { companyId, agentId, userId } = await seedFixture();
|
||||
const actors = [
|
||||
{ type: "board", userId, source: "session", companyIds: [companyId] },
|
||||
{ type: "agent", agentId, companyId },
|
||||
{ type: "none" },
|
||||
];
|
||||
const fullId = "12345678-1234-4234-8234-123456789abc";
|
||||
for (const actor of actors) {
|
||||
const app = await createApp(actor);
|
||||
const spies = [
|
||||
vi.spyOn(db, "select"), vi.spyOn(db, "insert"),
|
||||
vi.spyOn(db, "update"), vi.spyOn(db, "delete"),
|
||||
];
|
||||
try {
|
||||
for (const id of [fullId.slice(0, 8), "not-a-uuid", ` ${fullId}`, `${fullId}\n`, `{${fullId}\n}`]) {
|
||||
for (const pending of routineResourceRequests(app, id)) {
|
||||
const response = await pending;
|
||||
expect(response.status, JSON.stringify(response.body)).toBe(404);
|
||||
expect(response.body).toEqual({ error: response.req.path.includes("/routine-triggers/")
|
||||
? "Routine trigger not found" : "Routine not found" });
|
||||
}
|
||||
}
|
||||
const invalidBody = await request(app).patch("/api/routines/not-a-uuid").send({ title: 42 });
|
||||
expect(invalidBody.status).toBe(400);
|
||||
for (const spy of spies) expect(spy).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
for (const spy of spies) spy.mockRestore();
|
||||
}
|
||||
}
|
||||
}, 20_000);
|
||||
|
||||
it("keeps missing and cross-company routine resources indistinguishable", async () => {
|
||||
const { companyId, agentId, userId } = await seedFixture();
|
||||
const routineId = randomUUID();
|
||||
const triggerId = randomUUID();
|
||||
await db.insert(routines).values({ id: routineId, companyId, assigneeAgentId: agentId, title: "Private routine" });
|
||||
await db.insert(routineTriggers).values({ id: triggerId, companyId, routineId, kind: "api" });
|
||||
for (const actor of [
|
||||
{ type: "board", userId, source: "session", isInstanceAdmin: true, companyIds: [randomUUID()] },
|
||||
{ type: "agent", agentId: randomUUID(), companyId: randomUUID() },
|
||||
{ type: "none" },
|
||||
]) {
|
||||
const app = await createApp(actor);
|
||||
for (const [id, tid] of [[routineId, triggerId], [routineId.toUpperCase(), `{${triggerId}}`], [randomUUID(), randomUUID()]]) {
|
||||
for (const pending of routineResourceRequests(app, id!, tid!)) {
|
||||
const response = await pending;
|
||||
expect(response.status, JSON.stringify(response.body)).toBe(404);
|
||||
expect(response.body).toEqual({ error: response.req.path.includes("/routine-triggers/")
|
||||
? "Routine trigger not found" : "Routine not found" });
|
||||
}
|
||||
}
|
||||
}
|
||||
expect(await db.select().from(routines)).toMatchObject([{ id: routineId, title: "Private routine" }]);
|
||||
expect(await db.select().from(routineTriggers)).toMatchObject([{ id: triggerId, enabled: true }]);
|
||||
expect(await db.select().from(activityLog)).toEqual([]);
|
||||
}, 20_000);
|
||||
|
||||
it("preserves UUID routine reads and assignee-only mutations", async () => {
|
||||
const { companyId, agentId, userId } = await seedFixture();
|
||||
const routineId = randomUUID();
|
||||
const triggerId = randomUUID();
|
||||
await db.insert(routines).values({ id: routineId, companyId, assigneeAgentId: agentId, title: "Private routine" });
|
||||
await db.insert(routineTriggers).values({ id: triggerId, companyId, routineId, kind: "api" });
|
||||
const owner = await createApp({ type: "agent", agentId, companyId });
|
||||
const otherAgent = await createApp({ type: "agent", agentId: randomUUID(), companyId });
|
||||
const board = await createApp({ type: "board", userId, source: "session", companyIds: [companyId] });
|
||||
for (const app of [owner, otherAgent, board]) {
|
||||
const response = await request(app).get(`/api/routines/${routineId.toUpperCase()}`);
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body.id).toBe(routineId);
|
||||
}
|
||||
for (const [index, pending] of routineResourceRequests(otherAgent, routineId, triggerId).entries()) {
|
||||
const response = await pending;
|
||||
expect(response.status, JSON.stringify(response.body)).toBe(index < 2 ? 200 : 403);
|
||||
}
|
||||
const updated = await request(owner).patch(`/api/routines/${routineId}`).send({ title: "Owner update" });
|
||||
expect(updated.status, JSON.stringify(updated.body)).toBe(200);
|
||||
expect(updated.body.title).toBe("Owner update");
|
||||
const changedTrigger = await request(board).patch(`/api/routine-triggers/${triggerId}`).send({ enabled: false });
|
||||
expect(changedTrigger.status, JSON.stringify(changedTrigger.body)).toBe(200);
|
||||
expect(changedTrigger.body.enabled).toBe(false);
|
||||
}, 20_000);
|
||||
|
||||
it.each(["bearer", "hmac_sha256", "github_hmac", "none"] as const)(
|
||||
"authenticates %s HTTP deliveries, persists payloads, and enforces trigger lifecycle",
|
||||
async (signingMode) => {
|
||||
|
||||
@@ -246,6 +246,63 @@ describeEmbeddedPostgres("routine service live-execution coalescing", () => {
|
||||
.then((rows) => rows[0]!);
|
||||
}
|
||||
|
||||
it("rejects malformed routine and trigger IDs before any query", async () => {
|
||||
const { routine, svc } = await seedFixture();
|
||||
const malformed = [
|
||||
"", routine.id.slice(0, 8), "not-a-uuid", `${routine.id} `, ` ${routine.id}`,
|
||||
`${routine.id}\n`, `{${routine.id}`, `${routine.id}}`, `{${routine.id}\n}`,
|
||||
`${routine.id}-`, routine.id.replace("-", "--"), `g${routine.id.slice(1)}`,
|
||||
`${routine.id}\0`, `é${routine.id.slice(1)}`,
|
||||
`${routine.id.slice(0, 3)}-${routine.id.slice(3)}`,
|
||||
];
|
||||
const select = vi.spyOn(db, "select");
|
||||
try {
|
||||
for (const id of malformed) {
|
||||
await expect(svc.get(id)).resolves.toBeNull();
|
||||
await expect(svc.getDetail(id)).resolves.toBeNull();
|
||||
await expect(svc.getTrigger(id)).resolves.toBeNull();
|
||||
}
|
||||
expect(select).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
select.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("preserves lookup failures for valid routine and trigger IDs", async () => {
|
||||
const { routine, svc } = await seedFixture();
|
||||
const failure = new Error("Database unavailable");
|
||||
const select = vi.spyOn(db, "select").mockImplementation(() => { throw failure; });
|
||||
try {
|
||||
await expect(svc.get(routine.id)).rejects.toBe(failure);
|
||||
await expect(svc.getDetail(routine.id)).rejects.toBe(failure);
|
||||
await expect(svc.getTrigger(routine.id)).rejects.toBe(failure);
|
||||
} finally {
|
||||
select.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it.each([
|
||||
"12345678-1234-4234-8234-123456789abc",
|
||||
"12345678-1234-7234-8234-123456789abc",
|
||||
"00000000-0000-0000-0000-000000000000",
|
||||
"ffffffff-ffff-ffff-ffff-ffffffffffff",
|
||||
])("preserves PostgreSQL UUID lookup forms for %s", async (id) => {
|
||||
const { companyId, svc } = await seedFixture();
|
||||
await db.insert(routines).values({ id, companyId, title: "UUID lookup fixture" });
|
||||
await db.insert(routineTriggers).values({ id, companyId, routineId: id, kind: "api" });
|
||||
const compact = id.replaceAll("-", "");
|
||||
const forms = [
|
||||
id, id.toUpperCase(), `{${id}}`, compact,
|
||||
compact.match(/.{4}/g)!.join("-"),
|
||||
`{${compact.slice(0, 8)}-${compact.slice(8, 16)}-${compact.slice(16)}}`,
|
||||
];
|
||||
for (const input of forms) {
|
||||
await expect(svc.get(input)).resolves.toMatchObject({ id, companyId });
|
||||
await expect(svc.getDetail(input)).resolves.toMatchObject({ id, companyId });
|
||||
await expect(svc.getTrigger(input)).resolves.toMatchObject({ id, companyId, routineId: id });
|
||||
}
|
||||
});
|
||||
|
||||
it("clears transient routine run failures when execution issues resume", async () => {
|
||||
const { companyId, issueSvc, routine, svc } = await seedFixture();
|
||||
const runId = randomUUID();
|
||||
|
||||
@@ -665,6 +665,14 @@ function mapRoutineDescriptionDocument(row: {
|
||||
};
|
||||
}
|
||||
|
||||
// Match PostgreSQL UUID input, including compact/braced forms and any version.
|
||||
// Do not trim or resolve prefixes: the exact input must be safe for a UUID query.
|
||||
function isRoutineUuidInput(id: string): boolean {
|
||||
if (/\s/.test(id)) return false;
|
||||
const value = id.startsWith("{") && id.endsWith("}") ? id.slice(1, -1) : id;
|
||||
return /^(?:[0-9a-f]{4}-?){7}[0-9a-f]{4}$/i.test(value);
|
||||
}
|
||||
|
||||
export function routineService(
|
||||
db: Db,
|
||||
deps: {
|
||||
@@ -682,6 +690,7 @@ export function routineService(
|
||||
});
|
||||
|
||||
async function getRoutineById(id: string) {
|
||||
if (!isRoutineUuidInput(id)) return null;
|
||||
return db
|
||||
.select()
|
||||
.from(routines)
|
||||
@@ -766,6 +775,7 @@ export function routineService(
|
||||
}
|
||||
|
||||
async function getTriggerById(id: string) {
|
||||
if (!isRoutineUuidInput(id)) return null;
|
||||
return db
|
||||
.select()
|
||||
.from(routineTriggers)
|
||||
|
||||
Reference in new issue
Block a user