From 412775416fbfdf69e26022fe64ac3b9787ae9e36 Mon Sep 17 00:00:00 2001 From: Artificium Date: Mon, 27 Jul 2026 23:26:58 +0000 Subject: [PATCH] fix(app): validate agentId once for every route (#162) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twenty routes called getAgentId() inside their main try block, so the TypeError it throws for a malformed agent id fell through to the generic 500 branch — where fail() masks any 5xx message as "Internal server error". A caller sending /memory/bad%20agent/entities got a 500 with no indication the id was the problem, while 34 sibling routes returned a correct 400. The filed issue named 2 routes; it was 20. Fixes it in one place instead of twenty: an app.param('agentId', ...) validator covering all 55 agent-scoped routes, including the /pool routes carrying both :poolId and :agentId. Every route name-checks the same param, so coverage is complete with no per-route edits, and a route added later inherits correctness rather than depending on its author copying the idiom. Express resolves param callbacks at match time, so registration position does not limit coverage. getAgentId() stays as defense in depth. It should now be unreachable, but removing it would make handler correctness depend on middleware registration surviving future edits. Behavior change worth knowing: on the 34 already-correct routes, agentId is now validated before per-route body/query validation. The status is 400 either way — only which 400 message wins changes. Pinned by a test so it reads as a decision rather than a surprise. Also adds the two implemented /query parameters missing from the OpenAPI spec: `epistemic` (v3.9, the filed half of the issue) and `max_tokens` (found while checking — same array, no extra review surface). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011xCqQo49d3CEbn6oEvb3Ru --- src/app.ts | 25 ++++++++++++++- src/openapi.ts | 2 ++ tests/http-api.test.ts | 72 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 1 deletion(-) diff --git a/src/app.ts b/src/app.ts index 76699ce..f0b8aea 100644 --- a/src/app.ts +++ b/src/app.ts @@ -1657,11 +1657,34 @@ export function createApp(deps: AppDependencies): express.Express { // ─── Helpers ────────────────────────────────────────────────────────── const AGENT_ID_PATTERN = /^[\w.@:=-]{1,256}$/; + const AGENT_ID_MESSAGE = 'agentId must be 1-256 characters of [a-zA-Z0-9_.@:=-]'; + + // Reject malformed agent ids once, for every route that declares :agentId + // (all 55 agent-scoped routes, including the /pool routes that carry both + // :poolId and :agentId). Previously each handler was responsible for + // catching getAgentId's TypeError, and 20 of them called it inside their + // main try — so the TypeError fell through to the generic 500 branch and + // fail() masked the message as "Internal server error". Validating in one + // place makes correctness structural: a route added tomorrow inherits it + // instead of depending on its author copying the idiom. + // + // Express resolves param callbacks when a route matches, not when they are + // registered, so this covers routes defined above it. + app.param('agentId', (req: Request, res: Response, next: NextFunction, value: unknown) => { + if (typeof value !== 'string' || !AGENT_ID_PATTERN.test(value)) { + fail(res, 400, AGENT_ID_MESSAGE); + return; + } + next(); + }); + // Retained as defense in depth. With the param validator in front of every + // route this should be unreachable, but deleting it would make handler + // correctness depend on middleware registration surviving future edits. function getAgentId(req: Request): string { const id = pstr(req.params['agentId']); if (!AGENT_ID_PATTERN.test(id)) { - throw new TypeError('agentId must be 1-256 characters of [a-zA-Z0-9_.@:=-]'); + throw new TypeError(AGENT_ID_MESSAGE); } return id; } diff --git a/src/openapi.ts b/src/openapi.ts index df1617e..797eb03 100644 --- a/src/openapi.ts +++ b/src/openapi.ts @@ -105,6 +105,8 @@ export function buildOpenApiSpec(port: number): Record { { name: 'before', in: 'query', schema: { type: 'string', format: 'date-time' }, description: 'Only return memories before this timestamp' }, { name: 'decay', in: 'query', schema: { type: 'number', minimum: 0 }, description: 'Temporal decay rate per hour (0 = no decay)' }, { name: 'namespace', in: 'query', schema: { type: 'string', pattern: '^[a-z0-9][a-z0-9_-]*$' }, description: 'Memory namespace (default: "default")' }, + { name: 'max_tokens', in: 'query', schema: { type: 'integer', minimum: 1 }, description: 'Return only the highest-ranked results fitting within this token budget (estimate: content.length / 4)' }, + { name: 'epistemic', in: 'query', schema: { type: 'string', enum: ['only_established', 'include_provisional', 'include_contested', 'all'] }, description: 'Restrict results by calibrated uncertainty level (v3.9)' }, { name: 'explain', in: 'query', schema: { type: 'string', enum: ['true', 'false'] }, description: 'When "true", attach per-result explanation factors (rank_score, epistemic_status, search_mode, temporal_decay)' }, ], responses: { diff --git a/tests/http-api.test.ts b/tests/http-api.test.ts index d0b5f32..23c6af1 100644 --- a/tests/http-api.test.ts +++ b/tests/http-api.test.ts @@ -143,6 +143,78 @@ describe('Health and system endpoints', () => { // ─── Input Validation ─────────────────────────────────────────────────────── +describe('agentId validation (#162)', () => { + // A malformed agent id must produce 400 with the validation message on every + // agent-scoped route. Before the app.param validator, routes that called + // getAgentId() inside their main try returned 500 "Internal server error" + // instead — 20 routes were affected while 34 handled it correctly. + const BAD = 'bad%20agent'; + + it('returns 400 on a route that previously mishandled it (GET /entities)', async () => { + const res = await get(`/memory/${BAD}/entities`); + + assert.equal(res.status, 400); + const body = await res.json() as { ok: boolean; error: string }; + assert.equal(body.ok, false); + assert.ok(body.error.includes('agentId'), `error must name agentId: ${body.error}`); + }); + + it('returns 400 on GET /graph', async () => { + const res = await get(`/memory/${BAD}/graph?entity=thing`); + + assert.equal(res.status, 400); + const body = await res.json() as { ok: boolean; error: string }; + assert.ok(body.error.includes('agentId'), `error must name agentId: ${body.error}`); + }); + + it('returns 400 on a route that already handled it (GET /query)', async () => { + const res = await get(`/memory/${BAD}/query?q=test`); + + assert.equal(res.status, 400); + const body = await res.json() as { ok: boolean; error: string }; + assert.ok(body.error.includes('agentId'), `error must name agentId: ${body.error}`); + }); + + it('returns 400 on POST routes too', async () => { + const res = await post(`/memory/${BAD}/add`, { content: 'hello' }); + + assert.equal(res.status, 400); + const body = await res.json() as { ok: boolean; error: string }; + assert.ok(body.error.includes('agentId'), `error must name agentId: ${body.error}`); + }); + + it('over-long agent ids are rejected', async () => { + const res = await get(`/memory/${'a'.repeat(257)}/query?q=test`); + + assert.equal(res.status, 400); + const body = await res.json() as { ok: boolean; error: string }; + assert.ok(body.error.includes('agentId'), `error must name agentId: ${body.error}`); + }); + + it('agentId is validated BEFORE per-route body/query validation', async () => { + // Precedence change introduced with the validator: on the 34 routes that + // were already correct, a request invalid in both dimensions now reports + // the agentId problem rather than the body problem. Same status either + // way; pinned so the ordering is a documented choice, not a surprise. + const res = await get(`/memory/${BAD}/query`); // no ?q= either + + assert.equal(res.status, 400); + const body = await res.json() as { ok: boolean; error: string }; + assert.ok( + body.error.includes('agentId'), + `agentId must win over the missing-q error: ${body.error}`, + ); + }); + + it('a well-formed agent id still reaches the handler', async () => { + const res = await get(`/memory/${TEST_AGENT}/query`); // valid id, missing q + + assert.equal(res.status, 400); + const body = await res.json() as { ok: boolean; error: string }; + assert.ok(body.error.includes('"q"'), `must be the handler's own error: ${body.error}`); + }); +}); + describe('Input validation', () => { it('POST /memory/:agentId/add rejects missing content', async () => { const res = await post(`/memory/${TEST_AGENT}/add`, {});