From 260d7cbda4cb597553790ce55670f725fae71aed Mon Sep 17 00:00:00 2001 From: Mark McIntosh Date: Thu, 20 Aug 2026 10:12:51 -0400 Subject: [PATCH] fix(search): close SQLi, draft leak, and DoS in public /api/search The ai-search plugin mounts POST /api/search with no auth. Three fixes: - SQL injection: filters.dateRange.field was interpolated raw into SQL in column-identifier position (`c.${field} >= ?`). Allowlist it to created_at/updated_at, falling back to created_at otherwise. - Unbounded page size: clamp the keyword-search limit to a hard ceiling (100) and floor the offset at 0. - Unpublished/PII exposure: non-privileged callers are now locked to status=published (overriding any status filter in the body), and /api/search/analytics (which exposes other users' popular queries) returns 403 to anyone who isn't admin/editor/author. Privilege is read from the session the app middleware already populates. Deeper ACL/tenant scoping and rate-limiting for this surface are folded into the FTS5 search rewrite (#1058). --- .../plugins/ai-search-injection.test.ts | 79 +++++++++++++++++ .../plugins/ai-search-route-auth.test.ts | 84 +++++++++++++++++++ .../ai-search-plugin/routes/api.ts | 25 ++++++ .../ai-search-plugin/services/ai-search.ts | 26 ++++-- tests/e2e/109-search-security.spec.ts | 39 +++++++++ 5 files changed, 248 insertions(+), 5 deletions(-) create mode 100644 packages/core/src/__tests__/plugins/ai-search-injection.test.ts create mode 100644 packages/core/src/__tests__/plugins/ai-search-route-auth.test.ts create mode 100644 tests/e2e/109-search-security.spec.ts diff --git a/packages/core/src/__tests__/plugins/ai-search-injection.test.ts b/packages/core/src/__tests__/plugins/ai-search-injection.test.ts new file mode 100644 index 000000000..9717ae4e2 --- /dev/null +++ b/packages/core/src/__tests__/plugins/ai-search-injection.test.ts @@ -0,0 +1,79 @@ +import { describe, it, expect } from 'vitest' +import { AISearchService } from '../../plugins/core-plugins/ai-search-plugin/services/ai-search' +import type { SearchQuery } from '../../plugins/core-plugins/ai-search-plugin/types' + +// Guards the keyword-search SQL builder against injection via `dateRange.field` +// (a column identifier that cannot be a bound `?`) and against unbounded page +// sizes. We capture every prepared SQL string + bind params and assert on them. + +type Rec = { sql: string; params: unknown[] } + +function makeCapturingDb(records: Rec[]) { + return { + prepare(sql: string) { + const rec: Rec = { sql, params: [] } + records.push(rec) + const stmt: any = { + bind: (...p: unknown[]) => { rec.params = p; return stmt }, + first: async () => (/count\(\*\)/i.test(sql) ? { count: 0 } : null), + all: async () => ({ results: [] }), + run: async () => ({}), + } + return stmt + }, + } as any +} + +const resultsQueryOf = (records: Rec[]) => + records.find((r) => /ORDER BY c\.updated_at DESC/.test(r.sql)) + +describe('ai-search keyword SQL builder @api-keys', () => { + it('never interpolates an un-allowlisted dateRange.field into SQL', async () => { + const records: Rec[] = [] + const service = new AISearchService(makeCapturingDb(records)) + const payload = "id) OR (SELECT 1 FROM auth_user) -- " + const query: SearchQuery = { + query: 'x', + mode: 'keyword', + filters: { dateRange: { field: payload, start: new Date('2020-01-01') } }, + } as any + + await service.search(query) + + const results = resultsQueryOf(records) + expect(results).toBeDefined() + // The injection payload must not appear anywhere in the generated SQL... + for (const r of records) { + expect(r.sql).not.toContain(payload) + expect(r.sql).not.toContain('OR (SELECT') + } + // ...and the sink must fall back to the safe default column. + expect(results!.sql).toContain('c.created_at >=') + }) + + it('preserves an allowlisted dateRange.field (updated_at)', async () => { + const records: Rec[] = [] + const service = new AISearchService(makeCapturingDb(records)) + await service.search({ + query: 'x', + mode: 'keyword', + filters: { dateRange: { field: 'updated_at', end: new Date('2025-01-01') } }, + } as any) + + const results = resultsQueryOf(records) + expect(results!.sql).toContain('c.updated_at <=') + }) + + it('clamps an oversized limit to the ceiling', async () => { + const records: Rec[] = [] + const service = new AISearchService(makeCapturingDb(records)) + await service.search({ query: 'x', mode: 'keyword', limit: 999999, offset: -5 } as any) + + const results = resultsQueryOf(records) + // results query binds [...searchParams, limit, offset] — the last two. + const params = results!.params + const [limit, offset] = params.slice(-2) + expect(limit).toBe(100) + expect(offset).toBe(0) // negative offset floored to 0 + }) +}) diff --git a/packages/core/src/__tests__/plugins/ai-search-route-auth.test.ts b/packages/core/src/__tests__/plugins/ai-search-route-auth.test.ts new file mode 100644 index 000000000..a35cefbfd --- /dev/null +++ b/packages/core/src/__tests__/plugins/ai-search-route-auth.test.ts @@ -0,0 +1,84 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { Hono } from 'hono' + +// The public /api/search routes carry no auth middleware of their own; the +// app-level session middleware populates c.get('user'). These tests pin the +// per-route authorization: anonymous callers are locked to published results +// and cannot read analytics; privileged sessions retain full access. + +const searchSpy = vi.fn().mockResolvedValue({ results: [], total: 0 }) +const analyticsSpy = vi.fn().mockResolvedValue({ popular_queries: [] }) + +vi.mock('../../plugins/core-plugins/ai-search-plugin/services/ai-search', () => ({ + AISearchService: class { + search = (...args: any[]) => searchSpy(...args) + getSearchAnalytics = (...args: any[]) => analyticsSpy(...args) + getSearchSuggestions = vi.fn().mockResolvedValue([]) + }, +})) + +import apiRoutes from '../../plugins/core-plugins/ai-search-plugin/routes/api' + +// Mount behind a middleware that optionally sets a user, mirroring the app's +// session middleware. A test header selects the principal. +function makeApp() { + const app = new Hono() + app.use('*', async (c, next) => { + const role = c.req.header('x-test-role') + if (role) c.set('user' as never, { userId: 'u1', email: 'u@x.com', role } as never) + await next() + }) + app.route('/api/search', apiRoutes) + return app +} + +const env = { DB: {} } as any + +beforeEach(() => { + searchSpy.mockClear() + analyticsSpy.mockClear() +}) + +describe('ai-search public route authorization @api-keys', () => { + it('forces status=[published] for an anonymous search, overriding a draft filter', async () => { + const app = makeApp() + const res = await app.request('/api/search', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ query: 'x', filters: { status: ['draft'] } }), + }, env) + + expect(res.status).toBe(200) + expect(searchSpy).toHaveBeenCalledTimes(1) + expect(searchSpy.mock.calls[0][0].filters.status).toEqual(['published']) + }) + + it('preserves the requested status filter for a privileged (editor) search', async () => { + const app = makeApp() + const res = await app.request('/api/search', { + method: 'POST', + headers: { 'content-type': 'application/json', 'x-test-role': 'editor' }, + body: JSON.stringify({ query: 'x', filters: { status: ['draft'] } }), + }, env) + + expect(res.status).toBe(200) + expect(searchSpy.mock.calls[0][0].filters.status).toEqual(['draft']) + }) + + it('denies analytics to an anonymous caller (403)', async () => { + const app = makeApp() + const res = await app.request('/api/search/analytics', { method: 'GET' }, env) + expect(res.status).toBe(403) + expect(analyticsSpy).not.toHaveBeenCalled() + }) + + it('allows analytics for an admin session', async () => { + const app = makeApp() + const res = await app.request('/api/search/analytics', { + method: 'GET', + headers: { 'x-test-role': 'admin' }, + }, env) + expect(res.status).toBe(200) + expect(analyticsSpy).toHaveBeenCalledTimes(1) + }) +}) diff --git a/packages/core/src/plugins/core-plugins/ai-search-plugin/routes/api.ts b/packages/core/src/plugins/core-plugins/ai-search-plugin/routes/api.ts index 76477145c..536eb26a8 100644 --- a/packages/core/src/plugins/core-plugins/ai-search-plugin/routes/api.ts +++ b/packages/core/src/plugins/core-plugins/ai-search-plugin/routes/api.ts @@ -13,6 +13,18 @@ type Variables = { const apiRoutes = new Hono<{ Bindings: Bindings; Variables: Variables }>() +// Roles allowed to search across non-published content and to read search +// analytics. Everyone else (including anonymous callers) is limited to published +// results. The app-level session middleware populates `c.get('user')` on every +// route, so this reflects the real signed-in principal even though these routes +// carry no auth middleware of their own. +const PRIVILEGED_SEARCH_ROLES = ['admin', 'editor', 'author'] + +function isPrivilegedSearcher(c: { get: (k: 'user') => { role?: string } | undefined }): boolean { + const user = c.get('user') + return !!user && PRIVILEGED_SEARCH_ROLES.includes(user.role || '') +} + /** * POST /api/search * Execute search query @@ -44,6 +56,13 @@ apiRoutes.post('/', async (c) => { } } + // Non-privileged callers only ever see published content — override any + // status filter supplied in the body so drafts/archived rows cannot be + // requested from the public endpoint. + if (!isPrivilegedSearcher(c)) { + query.filters = { ...(query.filters || {}), status: ['published'] } + } + const results = await service.search(query) return c.json({ @@ -103,6 +122,12 @@ apiRoutes.get('/suggest', async (c) => { * Get search analytics */ apiRoutes.get('/analytics', async (c) => { + // Analytics exposes aggregate query volume and other users' popular search + // terms — restrict to privileged sessions. (This route is intended as + // /admin/api/search/analytics; the guard makes the current mount safe.) + if (!isPrivilegedSearcher(c)) { + return c.json({ success: false, error: 'Unauthorized' }, 403) + } try { const db = c.env.DB const ai = (c.env as any).AI diff --git a/packages/core/src/plugins/core-plugins/ai-search-plugin/services/ai-search.ts b/packages/core/src/plugins/core-plugins/ai-search-plugin/services/ai-search.ts index d250baa2e..0c11e2211 100644 --- a/packages/core/src/plugins/core-plugins/ai-search-plugin/services/ai-search.ts +++ b/packages/core/src/plugins/core-plugins/ai-search-plugin/services/ai-search.ts @@ -10,6 +10,16 @@ import type { import { CustomRAGService } from './custom-rag.service' import { getCollectionRegistry } from '../../../../services/collection-registry' +// Column names permitted in the keyword-search date-range filter. Because the +// column reaches SQL in identifier position (it cannot be a bound `?`), only +// values in this allowlist are ever interpolated; anything else falls back to +// `created_at`. This is the guard against SQL injection via `dateRange.field`. +const DATE_RANGE_FIELDS = new Set(['created_at', 'updated_at']) + +// Hard ceiling on the number of rows a single keyword search may return, so an +// unauthenticated caller cannot request an unbounded page. +const MAX_SEARCH_LIMIT = 100 + /** * AI Search Service * Handles search operations, settings management, and collection detection @@ -342,9 +352,13 @@ export class AISearchService { conditions.push("c.status != 'deleted'") } - // Date range filter + // Date range filter — the column name is allowlisted, never interpolated + // raw from request input. `dateRange.field` reaches SQL in identifier + // position (it cannot be bound as a `?`), so an un-allowlisted value would + // be a SQL injection sink. if (query.filters?.dateRange) { - const field = query.filters.dateRange.field || 'created_at' + const requestedField = query.filters.dateRange.field || 'created_at' + const field = DATE_RANGE_FIELDS.has(requestedField) ? requestedField : 'created_at' if (query.filters.dateRange.start) { conditions.push(`c.${field} >= ?`) params.push(query.filters.dateRange.start.getTime()) @@ -372,9 +386,11 @@ export class AISearchService { const countResult = await countStmt.bind(...params).first<{ count: number }>() const total = countResult?.count || 0 - // Get results - const limit = query.limit || settings.results_limit - const offset = query.offset || 0 + // Get results — clamp the page size so an anonymous caller cannot request + // an unbounded result set (memory pressure / bulk exfiltration). + const requestedLimit = Number(query.limit) || settings.results_limit + const limit = Math.min(Math.max(1, requestedLimit), MAX_SEARCH_LIMIT) + const offset = Math.max(0, Number(query.offset) || 0) const resultsStmt = this.db.prepare(` SELECT diff --git a/tests/e2e/109-search-security.spec.ts b/tests/e2e/109-search-security.spec.ts new file mode 100644 index 000000000..9c25dc859 --- /dev/null +++ b/tests/e2e/109-search-security.spec.ts @@ -0,0 +1,39 @@ +import { test, expect } from '@playwright/test' + +// Hardening for the public /api/search endpoint (ai-search-plugin): +// - the dateRange.field SQL-injection sink is allowlisted (no 500 / no injection) +// - the result limit is clamped +// - analytics is no longer readable by anonymous callers +// These run unauthenticated (no seed needed) against the deployed preview. + +test.describe('public search security @smoke @search', () => { + test('malicious dateRange.field does not error or inject', async ({ request }) => { + const res = await request.post('/api/search', { + data: { + query: 'test', + filters: { dateRange: { field: 'id) OR (SELECT 1 FROM auth_user) -- ', start: '2020-01-01' } }, + }, + }) + // The allowlist falls back to a safe column, so the query executes normally + // (200) rather than surfacing a SQL error (500). + expect(res.status()).toBe(200) + const body = await res.json() + expect(body.success).toBe(true) + }) + + test('an oversized limit is clamped to the ceiling', async ({ request }) => { + const res = await request.post('/api/search', { + data: { query: 'a', limit: 999999 }, + }) + expect(res.status()).toBe(200) + const body = await res.json() + const results = body?.data?.results ?? [] + expect(Array.isArray(results)).toBe(true) + expect(results.length).toBeLessThanOrEqual(100) + }) + + test('analytics is denied to anonymous callers', async ({ request }) => { + const res = await request.get('/api/search/analytics') + expect(res.status()).toBe(403) + }) +})