From 376ef3d5975f009ce95a45f26cd61be0ac46b15a Mon Sep 17 00:00:00 2001 From: Kartios Date: Mon, 17 Aug 2026 18:41:28 -0700 Subject: [PATCH] Answer 404 for an id that cannot name a row, rather than 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured against a running server: five of the eleven Motion `:id` routes answered `500 {"error":"Internal error"}` for an id like `nope`, and the other six only answered 400 because their body schema happened to be checked first — a valid body would have reached the same cast. Nothing was wrong with the not-found handling. That branch was never reached: every id column is a `uuid`, so Postgres refuses the parameter with `22P02` several layers below it, and the error is not a MutationError so it leaves as a 500. 404 rather than 400, because a 400 for a malformed id and a 404 for a well-formed one tells anyone probing which of their guesses are the right shape — and this feature already routes "somebody else's private draft" through the same 404 so that no answer distinguishes the reasons a row is not yours to see. Also adds the AGENTS.md §5 393px check for the five Motion routes, which scripts/screenshots.mjs does not photograph. All five measure zero horizontal overflow at 393 and 1440, light and dark. The same 500 is reachable on /api/accounts/:id and /api/contracts/:id, which predates this branch and is left alone here. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/src/routes/motion.ts | 19 ++++- apps/api/src/services/motion.ts | 18 +++++ apps/api/test/motion.test.ts | 63 ++++++++++++++++- scripts/motion-overflow-check.mjs | 114 ++++++++++++++++++++++++++++++ 4 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 scripts/motion-overflow-check.mjs diff --git a/apps/api/src/routes/motion.ts b/apps/api/src/routes/motion.ts index 792325c..a00250d 100644 --- a/apps/api/src/routes/motion.ts +++ b/apps/api/src/routes/motion.ts @@ -265,9 +265,26 @@ function canPublish(principal: Principal): boolean { ); } +/** + * The id in the path, or a 404 — including when it is not a uuid at all. + * + * The shape check is the load-bearing half. Every id column here is `uuid`, so + * an id like `nope` reaches Postgres as a parameter it cannot cast and comes + * back as `22P02 invalid input syntax for type uuid`, which is not a + * `MutationError` and so leaves as a 500 with `{"error":"Internal error"}`. + * Measured against a running server before this was written: five of the eleven + * `:id` routes answered 500, and the other six only answered 400 because their + * body schema happened to be checked first — a valid body would have reached + * the same cast. + * + * 404 rather than 400, deliberately, and for the same reason the template read + * answers 404 for somebody else's private draft: an id that cannot name a row + * is an id for a row that does not exist, and two different codes for "no such + * template" would tell an enumerating caller which ids are well-formed. + */ function requiredId(params: Readonly>, resource: string): string { const id = params.id; - if (!id) throw MutationError.notFound(resource); + if (!id || !uuid.safeParse(id).success) throw MutationError.notFound(resource); return id; } diff --git a/apps/api/src/services/motion.ts b/apps/api/src/services/motion.ts index e426d49..8f60d30 100644 --- a/apps/api/src/services/motion.ts +++ b/apps/api/src/services/motion.ts @@ -77,6 +77,22 @@ import { MutationError } from '../lib/mutation'; export type MotionTransaction = Parameters[0]>[0]; +const UUID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +/** + * Whether a string can name a row at all. + * + * Every id here is a `uuid` column, so a lookup by `nope` never reaches the + * "no such row" branch: Postgres refuses the cast with `22P02` and the request + * leaves as a 500. The reads below therefore answer null for a malformed id, + * which the routes already turn into the 404 an unknown id gets — the same + * answer a private template gives, which is the point of routing both through + * one branch. `requiredId` in `routes/motion.ts` does the same for the writes. + */ +function isUuid(id: string): boolean { + return UUID.test(id); +} + /** * The subset of a `Principal` a motion read is allowed to see. A service that * took the whole principal would be one refactor away from consulting teams or @@ -519,6 +535,7 @@ export class MotionService { * produces, so a private title cannot be confirmed by probing for one. */ async template(viewer: MotionViewer, id: string): Promise { + if (!isUuid(id)) return null; const [row] = await this.db .select() .from(motionTemplates) @@ -544,6 +561,7 @@ export class MotionService { } async engagement(viewer: MotionViewer, id: string): Promise { + if (!isUuid(id)) return null; const [summary] = await this.engagementSummaries(eq(engagements.id, id), 1); if (!summary) return null; diff --git a/apps/api/test/motion.test.ts b/apps/api/test/motion.test.ts index d47fb20..4eb3c5c 100644 --- a/apps/api/test/motion.test.ts +++ b/apps/api/test/motion.test.ts @@ -39,7 +39,7 @@ import { motionTemplateUpdateDefinition, motionTemplateVersionDefinition, } from '../src/routes/motion'; -import { motionSlug, visibleTemplates } from '../src/services/motion'; +import { MotionService, motionSlug, visibleTemplates } from '../src/services/motion'; import { onTeam, principal } from './helpers/principal'; const OWNER = '00000000-0000-4000-8000-000000000001'; @@ -723,3 +723,64 @@ describe('qualification scores', () => { ); }); }); + +describe('an id that cannot name a row is a row that does not exist', () => { + /* + * Measured against a running server before this test existed: every `:id` + * route answered `500 {"error":"Internal error"}` for an id like `nope`. + * Nothing was wrong with the code that handled a missing row — that branch + * was simply never reached, because each id column is a `uuid` and Postgres + * refuses the cast with `22P02` several layers below it. The reads therefore + * check the shape before they ask, and the writes do it in `requiredId`. + * + * 404 rather than 400 is the load-bearing half. A 400 for a malformed id and + * a 404 for a well-formed one tells anyone probing which of their guesses + * are the right shape, and this feature already routes "somebody else's + * private draft" through the same 404 precisely so that no answer here + * distinguishes between the reasons a row is not yours to see. + */ + const viewer = { userId: OWNER, isPlatformAdmin: false }; + + it('answers null for a malformed template id without going to the database at all', async () => { + const { db, log } = database([]); + + assert.equal(await new MotionService(db).template(viewer, 'nope'), null); + assert.deepEqual(log.events, [], 'a malformed id must not reach Postgres to be refused'); + }); + + it('answers null for a malformed engagement id, likewise', async () => { + const { db, log } = database([]); + + assert.equal(await new MotionService(db).engagement(viewer, 'nope'), null); + assert.deepEqual(log.events, []); + }); + + it('still reads a well-formed id — the guard is a shape check, not a rejection of unknown ids', async () => { + const { db, log } = database([[]]); + + assert.equal(await new MotionService(db).template(viewer, ENGAGEMENT_ID), null); + assert.deepEqual(log.events, ['select'], 'a well-formed id is answered by the database'); + }); + + it('refuses a malformed id on a write as not_found, the same answer an unknown one gets', async () => { + const { db, log } = database([]); + + // A lead, so the write gets past the capability check and reaches the id: + // authorization deliberately precedes it, and a member would be refused + // here for the other reason entirely. + await assert.rejects( + executeMutation( + db, + lead, + async () => ({}), + motionTemplatePublishDefinition(), + { id: 'nope' }, + ), + (error: unknown) => error instanceof MutationError && error.code === 'not_found', + ); + // The transaction opens first — `executeMutation` owns that — but nothing + // is ever asked of it, which is the property that matters: no statement + // carrying `nope` was sent to Postgres to be refused there. + assert.deepEqual(log.events, ['transaction'], 'refused on shape, without a query'); + }); +}); diff --git a/scripts/motion-overflow-check.mjs b/scripts/motion-overflow-check.mjs new file mode 100644 index 0000000..a3ccdc5 --- /dev/null +++ b/scripts/motion-overflow-check.mjs @@ -0,0 +1,114 @@ +/* + * The 393px check from AGENTS.md §5, run over the Motion routes. + * + * node scripts/motion-overflow-check.mjs # PIG_WEB_URL=http://127.0.0.1:8975 + * + * `scripts/screenshots.mjs` already asserts zero horizontal overflow, but only + * on the ten pages it photographs, and Motion added five it does not know + * about. The rule it enforces has been fixed at individual call sites three + * separate times, so a new page group that nobody measures is exactly how it + * comes back. + * + * Checked per route, in light and dark, at 393 and 1440: + * - scrollWidth === clientWidth on the document (the sideways-scroll failure) + * - no element wider than the viewport (which one, if so — the message has to + * name the offender or nobody can act on it) + * - nothing thrown to the console, and no failed request + */ +import { chromium } from 'playwright'; + +const BASE = process.env.PIG_WEB_URL ?? 'http://127.0.0.1:8975'; + +const ROUTES = [ + { slug: 'motion', path: '/motion' }, + { slug: 'library', path: '/motion/library' }, + { slug: 'engagements', path: '/motion/engagements' }, +]; + +const VIEWPORTS = [ + { name: 'mobile', width: 393, height: 852 }, + { name: 'desktop', width: 1440, height: 900 }, +]; + +const browser = await chromium.launch({ channel: 'chrome' }); +let failures = 0; + +/** The detail pages need a real id, so they are discovered rather than typed. */ +const api = await fetch(`${BASE}/api/motion/templates`).then((r) => r.json()); +const template = api.templates?.[0]; +if (template) ROUTES.push({ slug: 'template', path: `/motion/library/${template.id}` }); +const engagements = await fetch(`${BASE}/api/motion/engagements`).then((r) => r.json()); +const engagement = engagements.engagements?.[0]; +if (engagement) ROUTES.push({ slug: 'engagement', path: `/motion/engagements/${engagement.id}` }); + +for (const theme of ['light', 'dark']) { + for (const vp of VIEWPORTS) { + const ctx = await browser.newContext({ + viewport: { width: vp.width, height: vp.height }, + colorScheme: theme, + isMobile: vp.name === 'mobile', + hasTouch: vp.name === 'mobile', + }); + // The sidebar and the dock are per-device states in localStorage; left to + // whatever a human last set, the measurement is not reproducible. + await ctx.addInitScript(` + localStorage.setItem('pig.sidebar', 'expanded'); + localStorage.setItem('pig.piggy.dock', 'closed'); + `); + + for (const route of ROUTES) { + const page = await ctx.newPage(); + const problems = []; + page.on('console', (m) => { + if (m.type() !== 'error') return; + if (m.text().includes('/api/piggy/') || m.text().includes('Failed to load resource')) return; + problems.push(`console: ${m.text()}`); + }); + page.on('pageerror', (e) => problems.push(`threw: ${e.message}`)); + page.on('response', (r) => { + // Piggy is a separate container and is normally down on a dev host, so + // its 503 is a fact about the environment rather than about the page. + // Every other status belongs to the route under test. + if (r.status() >= 400 && !r.url().includes('/api/piggy/')) { + problems.push(`${r.status()} ${r.url().replace(BASE, '')}`); + } + }); + + await page.goto(`${BASE}${route.path}`, { waitUntil: 'networkidle' }); + // The pages are lazy chunks behind a Suspense boundary; a measurement + // taken before the fallback resolves measures the spinner. + await page.waitForTimeout(1200); + + const measured = await page.evaluate(() => { + const doc = document.documentElement; + const overflow = doc.scrollWidth - doc.clientWidth; + const wide = [...document.querySelectorAll('*')] + .filter((el) => el.getBoundingClientRect().width > doc.clientWidth + 1) + .slice(0, 3) + .map((el) => `${el.tagName.toLowerCase()}.${(el.className?.baseVal ?? el.className ?? '') + .toString() + .split(' ') + .slice(0, 4) + .join('.')}`); + return { overflow, wide, text: document.body.innerText.slice(0, 120) }; + }); + + const label = `${route.slug} ${vp.name} ${theme}`; + if (measured.overflow !== 0) { + failures++; + console.log(`FAIL ${label}: overflows by ${measured.overflow}px — ${measured.wide.join(', ')}`); + } else if (problems.length) { + failures++; + console.log(`FAIL ${label}: ${[...new Set(problems)].slice(0, 4).join(' | ')}`); + } else { + console.log(`ok ${label}`); + } + await page.close(); + } + await ctx.close(); + } +} + +await browser.close(); +console.log(failures === 0 ? '\nevery Motion route: no overflow, no error' : `\n${failures} failed`); +process.exit(failures === 0 ? 0 : 1);