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`, {});