synAdminandClaude Opus 5
8956b9f122
Item 5. GET /api/items/:id was the last route reading its id with a bare Number(), so an unreadable id reached Postgres and came back to the caller as a 500 for an item that cannot exist. It answers 404 now, like every other id-taking route since #207. It could not be fixed on its own, which is why it stayed. errorHandling.integration.test.ts used this route's looseness as its way of making a handler reject: tightening the parse would have left that test green while removing the thing it tests. So the test now fails a database call directly, with a spy on pool.query against a route that makes one. That is the failure the error middleware actually exists for, and it does not depend on any route declining to validate — the previous comment's own conclusion, that moving the trigger to cart.ts would only move the wart. Two things fixed along the way that the issue asked about but that switching to readId would not have delivered on its own. readId was not as strict as its name suggests. Number reads 5.0, 1e2, 0x10 and +5 as 5, 100, 16 and 5 — every one a positive integer, so every check readId made passed and the route fetched a real row for a URL nobody wrote. /items/5.0 answered with item 5. This never raised an error and so never announced itself; the issue noticed it only because #308 converted the comparison to a real integer. An id is a string of digits, so it is matched against digits before being parsed. readId is also now bounded at the top of a 32-bit serial. Above that Postgres raises 22003 rather than returning nothing, which is the same wrong answer to the caller as the 22P02 the function was written to prevent — a 500 for an id that identifies nothing. The leak assertion was passing for the wrong reason. It checks the response does not contain "syntax" or "items", and the error it was checking against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against as well, so a future error format cannot slip through by wording itself differently. Verified: tsc clean, lint 0 errors with no new warnings, 485 unit tests passing across 33 suites — seven of them new, covering the inputs above. The integration suite cannot run on this machine, so whether the rewritten error test passes is for CI to say. Item 4, coverage, is not closed by this and cannot be closed yet: the SonarQube scan step has been skipped on every recent run because it is gated on the earlier steps succeeding, and those steps were failing. The dashboard is therefore stale. Reported on the issue rather than guessed at. Refs #307 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
47 lines
1.6 KiB
TypeScript
47 lines
1.6 KiB
TypeScript
import { readId } from '../../src/utils';
|
|
|
|
describe('readId', () => {
|
|
it('reads a positive integer', () => {
|
|
expect(readId('7')).toBe(7);
|
|
expect(readId('999999')).toBe(999999);
|
|
});
|
|
|
|
// Each of these used to be sent to Postgres as text, raising 22P02 for an
|
|
// integer column and surfacing to the caller as a 500 (#207).
|
|
it.each(['abc', '', ' ', '1.5', '-1', '0', 'NaN', '1e5abc'])(
|
|
'refuses %p',
|
|
(value) => {
|
|
expect(readId(value)).toBeNull();
|
|
}
|
|
);
|
|
|
|
// These are the dangerous ones, because they were never a 500 and so never
|
|
// announced themselves (#307). Number reads each as a positive integer, so
|
|
// every check this function used to make passed and the route fetched a real
|
|
// row for a URL nobody wrote: /items/5.0 answered with item 5.
|
|
it.each(['5.0', '1e2', '0x10', '+5', '1_0'])(
|
|
'refuses %p, which parses to a positive integer but is not an id',
|
|
(value) => {
|
|
expect(readId(value)).toBeNull();
|
|
}
|
|
);
|
|
|
|
// The column is a 32-bit serial. Above that Postgres raises 22003 rather than
|
|
// returning nothing, which is the same wrong answer to the caller as the
|
|
// 22P02 this function was written to prevent.
|
|
it('refuses an id past the top of a 32-bit serial', () => {
|
|
expect(readId('2147483647')).toBe(2147483647);
|
|
expect(readId('2147483648')).toBeNull();
|
|
expect(readId('99999999999999999999')).toBeNull();
|
|
});
|
|
|
|
// Surrounding whitespace is a URL artefact rather than a different id.
|
|
it('reads an id with surrounding whitespace', () => {
|
|
expect(readId(' 7 ')).toBe(7);
|
|
});
|
|
|
|
it('refuses a missing param', () => {
|
|
expect(readId(undefined)).toBeNull();
|
|
});
|
|
});
|