Answer 404 for an id that cannot name a row, rather than 500
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user