fix(forms): escape stored HTML, gate submit on is_public, enforce RBAC on legacy forms routes

packages/core/src/routes/admin-forms.test.ts

@@ -0,0 +1,108 @@
+import { describe, it, expect, vi } from 'vitest'
+import { Hono } from 'hono'
+import { adminFormsRoutes } from './admin-forms'
+
+// Prior to this fix, adminFormsRoutes was gated by requireAuth() only — any
+// authenticated user of any role could manage forms and read submission data
+// (the 'forms:manage' nav permission was never actually enforced; requirePermission
+// is a no-op stub). This covers the requireRole(['admin','editor']) gate added
+// alongside the escaping/is_public fixes.
+
+function createMockDb() {
+  return {
+    prepare: vi.fn().mockImplementation(() => ({
+      bind: vi.fn().mockReturnThis(),
+      first: vi.fn().mockResolvedValue(null),
+      all: vi.fn().mockResolvedValue({ results: [] }),
+      run: vi.fn().mockResolvedValue({ success: true })
+    }))
+  }
+}
+
+function createSubmissionsMockDb(submissionDataJson: string) {
+  return {
+    prepare: vi.fn().mockImplementation((sql: string) => {
+      if (sql.includes('FROM forms WHERE')) {
+        return {
+          bind: vi.fn().mockReturnThis(),
+          first: vi.fn().mockResolvedValue({ id: 'form-1', display_name: 'Test Form' })
+        }
+      }
+      if (sql.includes('FROM form_submissions WHERE')) {
+        return {
+          bind: vi.fn().mockReturnThis(),
+          all: vi.fn().mockResolvedValue({
+            results: [{ id: 'sub-1', submitted_at: 0, submission_data: submissionDataJson }]
+          })
+        }
+      }
+      return {
+        bind: vi.fn().mockReturnThis(),
+        first: vi.fn().mockResolvedValue(null),
+        all: vi.fn().mockResolvedValue({ results: [] }),
+        run: vi.fn().mockResolvedValue({ success: true })
+      }
+    })
+  }
+}
+
+function createTestApp(db: any, user: { userId: string; email: string; role: string } | undefined) {
+  const app = new Hono()
+  app.use('/admin/forms/*', async (c, next) => {
+    c.env = { DB: db } as any
+    if (user) c.set('user', { ...user, exp: 0, iat: 0 })
+    await next()
+  })
+  app.route('/admin/forms', adminFormsRoutes)
+  return app
+}
+
+describe('admin-forms.ts — RBAC gate', () => {
+  it('rejects an authenticated non-admin/editor user with 403', async () => {
+    const app = createTestApp(createMockDb(), { userId: 'u1', email: 'subscriber@example.com', role: 'subscriber' })
+    const res = await app.request('/admin/forms', { headers: { Accept: 'application/json' } })
+    expect(res.status).toBe(403)
+  })
+
+  it('rejects an unauthenticated request with 401', async () => {
+    const app = createTestApp(createMockDb(), undefined)
+    const res = await app.request('/admin/forms', { headers: { Accept: 'application/json' } })
+    expect(res.status).toBe(401)
+  })
+
+  it('allows an admin past the gate', async () => {
+    const app = createTestApp(createMockDb(), { userId: 'u1', email: 'admin@example.com', role: 'admin' })
+    const res = await app.request('/admin/forms', { headers: { Accept: 'application/json' } })
+    expect(res.status).toBe(200)
+  })
+
+  it('allows an editor past the gate', async () => {
+    const app = createTestApp(createMockDb(), { userId: 'u1', email: 'editor@example.com', role: 'editor' })
+    const res = await app.request('/admin/forms', { headers: { Accept: 'application/json' } })
+    expect(res.status).toBe(200)
+  })
+})
+
+describe('admin-forms.ts — GET /:id/submissions XSS regression', () => {
+  // sanitizeDeep() (public-forms.ts) escapes submission VALUES at write time, but
+  // never touched object KEYS — an anonymous public submitter fully controls both.
+  // A malicious key survived storage unescaped and was dumped raw into this page's
+  // <pre> via JSON.stringify. Any admin/editor opening submissions for that form
+  // then executed it in their authenticated session. Regression for both the
+  // render-time escapeHtml() and the sanitizeDeep() key-escaping fix.
+  it('does not render an unescaped HTML tag from a submission data key', async () => {
+    const maliciousKey = '<img src=x onerror=alert(document.cookie)>'
+    const submissionDataJson = JSON.stringify({ [maliciousKey]: 'x' })
+    const app = createTestApp(
+      createSubmissionsMockDb(submissionDataJson),
+      { userId: 'u1', email: 'admin@example.com', role: 'admin' }
+    )
+
+    const res = await app.request('/admin/forms/form-1/submissions')
+    const html = await res.text()
+
+    expect(res.status).toBe(200)
+    expect(html).not.toContain(maliciousKey)
+    expect(html).toContain('&lt;img src=x onerror=alert(document.cookie)&gt;')
+  })
+})

packages/core/src/routes/admin-forms.ts

@@ -1,9 +1,10 @@
 import { Hono } from 'hono'
-import { requireAuth } from '../middleware'
+import { requireAuth, requireRole } from '../middleware'
 import { renderFormsListPage } from '../templates/pages/admin-forms-list.template'
 import { renderFormBuilderPage, type FormBuilderPageData } from '../templates/pages/admin-forms-builder.template'
 import { renderFormCreatePage } from '../templates/pages/admin-forms-create.template'
 import { TurnstileService } from '../plugins/core-plugins/turnstile-plugin/services/turnstile'
+import { escapeHtml } from '../utils/sanitize'
 
 // Type definitions for forms
 interface Form {
@@ -84,8 +85,13 @@ const isTableMissing = (err: any) =>
 
 export const adminFormsRoutes = new Hono<{ Bindings: Bindings; Variables: Variables }>()
 
-// Apply authentication middleware
+// Apply authentication + RBAC middleware. Every route in this file manages
+// form definitions or reads submission data (which may carry PII); the nav
+// item is already gated on the 'forms:manage' permission (manifest-registry.ts)
+// but that gate is UI-only (requirePermission is a no-op stub) — this is the
+// actual enforcement.
 adminFormsRoutes.use('*', requireAuth())
+adminFormsRoutes.use('*', requireRole(['admin', 'editor']))
 
 // Forms management - List all forms
 adminFormsRoutes.get('/', async (c) => {
@@ -438,7 +444,7 @@ adminFormsRoutes.get('/:id/submissions', async (c) => {
       <!DOCTYPE html>
       <html>
       <head>
-        <title>Submissions - ${form.display_name}</title>
+        <title>Submissions - ${escapeHtml(form.display_name as string)}</title>
         <style>
           body { font-family: system-ui; padding: 20px; }
           h1 { margin-bottom: 20px; }
@@ -451,7 +457,7 @@ adminFormsRoutes.get('/:id/submissions', async (c) => {
       </head>
       <body>
         <a href="/admin/forms" class="back-link">← Back to Forms</a>
-        <h1>Submissions: ${form.display_name}</h1>
+        <h1>Submissions: ${escapeHtml(form.display_name as string)}</h1>
         <p>Total submissions: ${submissions.results.length}</p>
         ${submissions.results.length > 0 ? `
           <table>
@@ -467,7 +473,7 @@ adminFormsRoutes.get('/:id/submissions', async (c) => {
                 <tr>
                   <td>${sub.id.substring(0, 8)}</td>
                   <td>${new Date(sub.submitted_at).toLocaleString()}</td>
-                  <td><pre>${JSON.stringify(JSON.parse(sub.submission_data), null, 2)}</pre></td>
+                  <td><pre>${escapeHtml(JSON.stringify(JSON.parse(sub.submission_data), null, 2))}</pre></td>
                 </tr>
               `).join('')}
             </tbody>

packages/core/src/routes/public-forms.integration.test.ts

@@ -0,0 +1,220 @@
+import { describe, it, expect, beforeEach, afterEach } from 'vitest'
+import Database from 'better-sqlite3'
+import { readFileSync } from 'node:fs'
+import { fileURLToPath } from 'node:url'
+import { dirname, join } from 'node:path'
+import { Hono } from 'hono'
+import { vi } from 'vitest'
+
+// Real-SQLite coverage for public-forms.ts. The mock-DB suite (public-forms.test.ts)
+// can't catch SQL-level regressions — e.g. a WHERE clause silently dropping a
+// condition — because its mock matches on `sql.includes('FROM forms WHERE')` and
+// ignores the rest of the query. This harness runs the real `forms`/`form_submissions`
+// schema from migration 0004 against better-sqlite3 so the actual SQL executes.
+
+vi.mock('../plugins/core-plugins/turnstile-plugin/services/turnstile', () => ({
+  TurnstileService: class MockTurnstileService {
+    getSettings = vi.fn().mockResolvedValue(null)
+    isEnabled = vi.fn().mockResolvedValue(false)
+    verifyToken = vi.fn().mockResolvedValue({ success: true })
+  }
+}))
+
+import { publicFormsRoutes } from './public-forms'
+
+const MIGRATIONS_DIR = join(dirname(fileURLToPath(import.meta.url)), '../../migrations')
+
+function normalize(v: unknown): number | string | bigint | Buffer | null {
+  if (v === undefined || v === null) return null
+  if (typeof v === 'boolean') return v ? 1 : 0
+  return v as number | string | bigint | Buffer
+}
+
+class TestStatement {
+  constructor(private sqlite: Database.Database, private sql: string, private binds: unknown[] = []) {}
+  bind(...args: unknown[]): TestStatement {
+    return new TestStatement(this.sqlite, this.sql, args.map(normalize))
+  }
+  async run() {
+    const info = this.sqlite.prepare(this.sql).run(...(this.binds as never[]))
+    return { success: true, meta: { changes: info.changes, last_row_id: info.lastInsertRowid } }
+  }
+  async first<T = unknown>(): Promise<T | null> {
+    const row = this.sqlite.prepare(this.sql).get(...(this.binds as never[]))
+    return (row ?? null) as T | null
+  }
+  async all<T = unknown>() {
+    const rows = this.sqlite.prepare(this.sql).all(...(this.binds as never[]))
+    return { results: rows as T[], success: true, meta: {} }
+  }
+}
+
+function createFormsTestD1() {
+  const sqlite = new Database(':memory:')
+  sqlite.pragma('foreign_keys = OFF') // mirrors D1 — see __tests__/utils/d1-sqlite.ts
+  const sql = readFileSync(join(MIGRATIONS_DIR, '0004_forms.sql'), 'utf-8')
+  sqlite.exec(sql)
+  return {
+    sqlite,
+    db: {
+      prepare(sql: string) {
+        return new TestStatement(sqlite, sql)
+      }
+    } as unknown as D1Database
+  }
+}
+
+function insertForm(sqlite: Database.Database, overrides: Partial<Record<string, unknown>> = {}) {
+  const now = Date.now()
+  const form = {
+    id: 'form-1',
+    name: 'test_form',
+    display_name: 'Test Form',
+    description: null,
+    category: 'general',
+    formio_schema: JSON.stringify({ components: [] }),
+    settings: JSON.stringify({}),
+    is_active: 1,
+    is_public: 1,
+    turnstile_enabled: 0,
+    turnstile_settings: null,
+    submission_count: 0,
+    created_at: now,
+    updated_at: now,
+    ...overrides
+  }
+  sqlite.prepare(`
+    INSERT INTO forms (id, name, display_name, description, category, formio_schema, settings, is_active, is_public, submission_count, created_at, updated_at)
+    VALUES (@id, @name, @display_name, @description, @category, @formio_schema, @settings, @is_active, @is_public, @submission_count, @created_at, @updated_at)
+  `).run(form)
+  return form
+}
+
+function createTestApp(db: D1Database) {
+  const app = new Hono()
+  app.use('/api/forms/*', async (c, next) => {
+    c.env = { DB: db } as any
+    await next()
+  })
+  app.route('/api/forms', publicFormsRoutes)
+  return app
+}
+
+describe('public-forms.ts — real-SQLite regression coverage', () => {
+  let harness: ReturnType<typeof createFormsTestD1>
+
+  beforeEach(() => {
+    harness = createFormsTestD1()
+  })
+
+  afterEach(() => {
+    harness.sqlite.close()
+  })
+
+  describe('POST /:identifier/submit — is_public gate', () => {
+    it('rejects submission to a private (is_public=0) form with 404', async () => {
+      insertForm(harness.sqlite, { is_public: 0 })
+      const app = createTestApp(harness.db)
+
+      const res = await app.request('/api/forms/test_form/submit', {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify({ data: { name: 'attacker' } })
+      })
+
+      expect(res.status).toBe(404)
+    })
+
+    it('accepts submission to a public (is_public=1) form', async () => {
+      insertForm(harness.sqlite, { is_public: 1 })
+      const app = createTestApp(harness.db)
+
+      const res = await app.request('/api/forms/test_form/submit', {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify({ data: { name: 'legit' } })
+      })
+
+      expect(res.status).toBe(200)
+      const row = harness.sqlite.prepare('SELECT * FROM form_submissions WHERE form_id = ?').get('form-1') as any
+      expect(row).toBeTruthy()
+      expect(JSON.parse(row.submission_data).name).toBe('legit')
+    })
+
+    it('rejects an inactive (is_active=0) form even if public', async () => {
+      insertForm(harness.sqlite, { is_active: 0, is_public: 1 })
+      const app = createTestApp(harness.db)
+
+      const res = await app.request('/api/forms/test_form/submit', {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify({ data: {} })
+      })
+
+      expect(res.status).toBe(404)
+    })
+  })
+
+  describe('GET /:identifier/turnstile-config — is_public gate', () => {
+    // KNOWN PRE-EXISTING BUG, separate from this patch: migration 0004_forms.sql
+    // never added `turnstile_enabled`/`turnstile_settings` columns to `forms`,
+    // but this route's SELECT names them explicitly — so on the real schema it
+    // 500s unconditionally, for public and private forms alike, before the
+    // is_public condition is ever evaluated. The is_public fix below is still
+    // correct (defense in depth, matches the other GET routes) and will start
+    // actually gating once those columns exist; until then this test documents
+    // the current, real, degraded-but-fails-closed behavior rather than
+    // asserting something that isn't true today. See conversation/handoff notes
+    // for the missing-migration finding — not fixed here, flagged separately.
+    it('500s today regardless of is_public, due to the missing turnstile_* columns', async () => {
+      insertForm(harness.sqlite, { is_public: 1 })
+      const app = createTestApp(harness.db)
+
+      const res = await app.request('/api/forms/test_form/turnstile-config')
+
+      expect(res.status).toBe(500)
+    })
+  })
+
+  describe('GET /:name — HTML escaping of admin-authored fields', () => {
+    it('escapes display_name and description against stored XSS', async () => {
+      insertForm(harness.sqlite, {
+        display_name: '</title><script>alert(1)</script>',
+        description: '<img src=x onerror=alert(2)>'
+      })
+      const app = createTestApp(harness.db)
+
+      const res = await app.request('/api/forms/test_form')
+      const html = await res.text()
+
+      expect(res.status).toBe(200)
+      expect(html).not.toContain('<script>alert(1)</script>')
+      expect(html).not.toContain('<img src=x onerror=alert(2)>')
+      expect(html).toContain('&lt;script&gt;alert(1)&lt;/script&gt;')
+      expect(html).toContain('&lt;img src=x onerror=alert(2)&gt;')
+    })
+
+    it('neutralizes a </script> breakout inside the embedded formio_schema JSON', async () => {
+      insertForm(harness.sqlite, {
+        formio_schema: JSON.stringify({
+          components: [{ type: 'textfield', label: '</script><script>alert(3)</script>' }]
+        })
+      })
+      const app = createTestApp(harness.db)
+
+      const res = await app.request('/api/forms/test_form')
+      const html = await res.text()
+
+      expect(res.status).toBe(200)
+      // The literal breakout sequence must never appear — the leading `<` (all a
+      // parser needs to recognize a closing tag) must be escaped to \u003c.
+      expect(html).not.toContain('</script><script>alert(3)</script>')
+      expect(html).toContain('\\u003c/script>\\u003cscript>alert(3)\\u003c/script>')
+      // And the schema must still be valid JSON once parsed back out of the script body.
+      const match = html.match(/const formioSchema = (.*);\s*\n\s*const settings/)
+      expect(match).toBeTruthy()
+      const parsed = JSON.parse(match![1])
+      expect(parsed.components[0].label).toBe('</script><script>alert(3)</script>')
+    })
+  })
+})