fix: validate identifiers in workflow query builder to prevent SQL injection (#490)

buildListQuery interpolated the table name, filter column names, and the
ORDER BY clause directly into SQL. Current callers only pass hardcoded
values, so this is not exploitable today — but the generic signature means
any future caller wiring user input into a column or sort parameter would
create a direct SQL injection with no compiler warning.

Validate table/column names as bare SQL identifiers and require an explicit
direction on ORDER BY clauses, throwing on anything else. Filter values
remain parameterized as before. Export the helper and add unit tests
covering valid queries and rejected injection attempts.

Co-authored-by: Claude Opus 4 <noreply@anthropic.com>

Author: alari76

server/workflow-engine.test.ts

@@ -30,7 +30,7 @@ vi.mock('fs', async (importOriginal) => {
   }
 })
 
-import { WorkflowEngine, WorkflowSkipped, SessionGoneError, initWorkflowEngine, getWorkflowEngine, shutdownWorkflowEngine, cronMatchesDate } from './workflow-engine.js'
+import { WorkflowEngine, WorkflowSkipped, SessionGoneError, initWorkflowEngine, getWorkflowEngine, shutdownWorkflowEngine, cronMatchesDate, buildListQuery } from './workflow-engine.js'
 import type { StepHandler } from './workflow-engine.js'
 
 describe('WorkflowEngine', () => {
@@ -450,6 +450,42 @@ describe('WorkflowEngine', () => {
     })
   })
 
+  describe('buildListQuery', () => {
+    it('builds a parameterized query for valid inputs', () => {
+      const { sql, params } = buildListQuery('workflow_runs', {
+        filters: [{ column: 'kind', value: 'test' }, { column: 'status', value: 'failed' }],
+        orderBy: 'created_at DESC',
+        limit: 10,
+        offset: 5,
+      })
+      expect(sql).toBe('SELECT * FROM workflow_runs WHERE 1=1 AND kind = ? AND status = ? ORDER BY created_at DESC LIMIT ? OFFSET ?')
+      expect(params).toEqual(['test', 'failed', 10, 5])
+    })
+
+    it('rejects an injection attempt in the table name', () => {
+      expect(() => buildListQuery('workflow_runs; DROP TABLE workflow_runs', { filters: [] }))
+        .toThrow(/invalid table name/)
+    })
+
+    it('rejects an injection attempt in a filter column', () => {
+      expect(() => buildListQuery('workflow_runs', {
+        filters: [{ column: 'kind = 1 OR 1=1 --', value: 'x' }],
+      })).toThrow(/invalid filter column/)
+    })
+
+    it('rejects an injection attempt in the orderBy clause', () => {
+      expect(() => buildListQuery('workflow_runs', {
+        filters: [],
+        orderBy: 'created_at; DROP TABLE workflow_runs',
+      })).toThrow(/invalid orderBy clause/)
+    })
+
+    it('rejects an orderBy without an explicit direction', () => {
+      expect(() => buildListQuery('workflow_runs', { filters: [], orderBy: 'created_at' }))
+        .toThrow(/invalid orderBy clause/)
+    })
+  })
+
   describe('getRun', () => {
     it('returns null for non-existent run', () => {
       mockGet.mockReturnValueOnce(undefined)

server/workflow-engine.ts

@@ -282,17 +282,45 @@ interface ListQueryOpts {
   offset?: number
 }
 
-/** Build a parameterized SELECT query from typed filter objects. */
-function buildListQuery(table: string, opts: ListQueryOpts): { sql: string; params: unknown[] } {
+/**
+ * A bare SQL identifier: a table or column name with no quoting, spaces, or
+ * other metacharacters. Filter values are always parameterized, but table and
+ * column names cannot be — so they are validated against this pattern to ensure
+ * a future caller passing user-controlled input cannot inject SQL.
+ */
+const SQL_IDENTIFIER = /^[a-z_][a-z0-9_]*$/i
+
+/** An ORDER BY clause: a single identifier followed by an explicit direction. */
+const SQL_ORDER_BY = /^[a-z_][a-z0-9_]* (ASC|DESC)$/i
+
+/**
+ * Build a parameterized SELECT query from typed filter objects.
+ *
+ * Exported for testing. Filter values are parameterized; table/column/orderBy
+ * are validated as bare SQL identifiers since they cannot be parameterized.
+ */
+export function buildListQuery(table: string, opts: ListQueryOpts): { sql: string; params: unknown[] } {
+  if (!SQL_IDENTIFIER.test(table)) {
+    throw new Error(`buildListQuery: invalid table name '${table}'`)
+  }
+
   const params: unknown[] = []
   let sql = `SELECT * FROM ${table} WHERE 1=1`
 
   for (const f of opts.filters) {
+    if (!SQL_IDENTIFIER.test(f.column)) {
+      throw new Error(`buildListQuery: invalid filter column '${f.column}'`)
+    }
     sql += ` AND ${f.column} = ?`
     params.push(f.value)
   }
 
-  if (opts.orderBy) sql += ` ORDER BY ${opts.orderBy}`
+  if (opts.orderBy) {
+    if (!SQL_ORDER_BY.test(opts.orderBy)) {
+      throw new Error(`buildListQuery: invalid orderBy clause '${opts.orderBy}'`)
+    }
+    sql += ` ORDER BY ${opts.orderBy}`
+  }
   if (opts.limit) { sql += ` LIMIT ?`; params.push(opts.limit) }
   if (opts.offset) { sql += ` OFFSET ?`; params.push(opts.offset) }