Security hardening (0307, Option A): real signature verification, SQL identifier allowlist, footgun warnings

.changeset/security-sync-integrity-hardening.md

@@ -0,0 +1,18 @@
+---
+'@xnetjs/sync': minor
+---
+
+Harden the sync-integrity primitives so look-alike helpers no longer give false
+assurance (exploration 0307):
+
+- `verifyIntegrity` now performs **real Ed25519 signature verification** against
+  the key recovered from each change's author DID (previously it only checked
+  that the signature field was non-empty). It accepts an optional `resolveKey`
+  override; the default is self-certifying `did:key` resolution.
+- `attemptRepair`'s `recompute-hash` action — which overwrites a change's stored
+  hash and can launder tampered payloads — is now gated behind an explicit
+  `{ trustHashRecompute: true }` opt-in and refused by default.
+- `AuthorizedYjsSyncProvider.handleRemoteUpdate` now enforces the Yjs update
+  size cap before applying, and `validateChain` / the handler registry /
+  `quickIntegrityCheck` document that they are structural-only and do not
+  authenticate authorship.

docs/specs/protocol/02-data-model.md

@@ -101,7 +101,7 @@ signed **Change** records ([`change.ts`](../../../packages/sync/src/change.ts)):
 
 ```ts
 interface Change<T> {
-  protocolVersion?: number      // 3 in xnet/1.0 (CURRENT_PROTOCOL_VERSION)
+  protocolVersion?: number      // 4 in xnet/1.0 (CURRENT_PROTOCOL_VERSION)
   id: string                    // unique change id
   type: string                  // "node-change"
   payload: T                    // NodePayload (below)
@@ -141,7 +141,7 @@ signatures.** The algorithm (reference: `computeChangeHash` / `signChange` in
 **Step 1 — select fields to hash.** Take the unsigned change (all fields *except*
 `hash` and `signature`). If `protocolVersion` is `0` or absent (legacy), remove
 the `protocolVersion` field before hashing. For `xnet/1.0` (`protocolVersion =
-3`), keep it.
+4`, the current `CURRENT_PROTOCOL_VERSION`), keep it.
 
 **Step 2 — canonical JSON.** Serialize with:
 - Object keys sorted **lexicographically, recursively** at every nesting level

packages/hub/src/config.ts

@@ -142,6 +142,24 @@ export const resolveConfig = (cliOptions: Partial<HubConfig>): HubConfig => {
   const demo = cliOptions.demo ?? process.env.HUB_MODE === 'demo'
   const demoOverrides = getDemoOverrides(demo) ?? undefined
 
+  // Loudly flag security-relevant footguns at startup (exploration 0307). These
+  // stay non-default; a warning makes an intentional relaxation visible in logs
+  // rather than silent.
+  if (!auth) {
+    console.warn(
+      '[hub] SECURITY: auth is DISABLED (HUB_AUTH=false) — every connection is ' +
+        'treated as an anonymous client with wildcard capabilities and room ' +
+        'authorization is skipped. Do not run this on an open network.'
+    )
+  }
+  if (allowUnsignedReplication) {
+    console.warn(
+      '[hub] SECURITY: HUB_ALLOW_UNSIGNED_REPLICATION is enabled — unsigned Yjs ' +
+        'updates are accepted and applied to hub-held document state without ' +
+        'authorship verification. Enable only for trusted, closed deployments.'
+    )
+  }
+
   return {
     ...DEFAULT_CONFIG,
     ...cliOptions,

packages/hub/src/storage/sqlite.ts

@@ -48,6 +48,22 @@ const assertSafePath = (base: string, key: string): string => {
   return resolved
 }
 
+// Column identifiers in the dynamic database-row query are client-supplied and
+// get interpolated into SQL (json_extract paths + SELECT aliases), where they
+// cannot be parameterized. Allowlist them to a conservative property-key charset
+// so a hostile identifier can't break out of the string literal / add clauses
+// (exploration 0307). Values elsewhere in the query stay parameterized.
+const SAFE_JSON_COLUMN = /^[A-Za-z0-9_.-]{1,128}$/
+const assertSafeColumnId = (columnId: string): string => {
+  if (typeof columnId !== 'string' || !SAFE_JSON_COLUMN.test(columnId)) {
+    throw new Error(`Unsafe column identifier: ${JSON.stringify(columnId)}`)
+  }
+  return columnId
+}
+// `json_extract(data, '$.<col>')` for a validated column identifier.
+const jsonExtractColumn = (columnId: string): string =>
+  `json_extract(data, '$.${assertSafeColumnId(columnId)}')`
+
 const SCHEMA_SQL = `
   CREATE TABLE IF NOT EXISTS doc_state (
     doc_id TEXT PRIMARY KEY,
@@ -2367,7 +2383,7 @@ export const createSQLiteStorage = (
     if (select && select.length > 0) {
       const cols = ['id', 'database_id', 'sort_key', 'created_at', 'created_by', 'updated_at']
       for (const col of select) {
-        cols.push(`json_extract(data, '$.${col}') as "${col}"`)
+        cols.push(`${jsonExtractColumn(col)} as "${assertSafeColumnId(col)}"`)
       }
       selectClause = cols.join(', ')
     }
@@ -2405,9 +2421,9 @@ export const createSQLiteStorage = (
     if (sorts && sorts.length > 0) {
       const orderBy = sorts
         .map((s) => {
-          const col =
-            s.columnId === 'sortKey' ? 'sort_key' : `json_extract(data, '$.${s.columnId}')`
-          return `${col} ${s.direction.toUpperCase()}`
+          const col = s.columnId === 'sortKey' ? 'sort_key' : jsonExtractColumn(s.columnId)
+          const direction = s.direction.toUpperCase() === 'DESC' ? 'DESC' : 'ASC'
+          return `${col} ${direction}`
         })
         .join(', ')
       sql += ` ORDER BY ${orderBy}, id ASC`
@@ -2502,7 +2518,7 @@ export const createSQLiteStorage = (
     params: unknown[]
   } {
     const { columnId, operator, value } = condition
-    const col = `json_extract(data, '$.${columnId}')`
+    const col = jsonExtractColumn(columnId)
 
     switch (operator) {
       case 'equals':

packages/hub/test/config.test.ts

@@ -1,4 +1,4 @@
-import { afterEach, describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it, vi } from 'vitest'
 import { resolveConfig } from '../src/config'
 
 const resetEnv = () => {
@@ -10,6 +10,8 @@ const resetEnv = () => {
   delete process.env.HUB_AWARENESS_MAX_UPDATE_SIZE
   delete process.env.FLY_REGION
   delete process.env.FLY_MACHINE_ID
+  delete process.env.HUB_AUTH
+  delete process.env.HUB_ALLOW_UNSIGNED_REPLICATION
 }
 
 describe('resolveConfig', () => {
@@ -46,4 +48,29 @@ describe('resolveConfig', () => {
     const config = resolveConfig({ awarenessMaxUpdateSize: 1024 })
     expect(config.awarenessMaxUpdateSize).toBe(2048)
   })
+
+  it('does not warn for the safe defaults', () => {
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    resolveConfig({})
+    expect(warn).not.toHaveBeenCalled()
+    warn.mockRestore()
+  })
+
+  it('loudly warns when auth is disabled', () => {
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    process.env.HUB_AUTH = 'false'
+    resolveConfig({})
+    expect(warn).toHaveBeenCalledWith(expect.stringMatching(/SECURITY.*auth is DISABLED/))
+    warn.mockRestore()
+  })
+
+  it('loudly warns when unsigned replication is enabled', () => {
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    process.env.HUB_ALLOW_UNSIGNED_REPLICATION = 'true'
+    resolveConfig({})
+    expect(warn).toHaveBeenCalledWith(
+      expect.stringMatching(/SECURITY.*HUB_ALLOW_UNSIGNED_REPLICATION/)
+    )
+    warn.mockRestore()
+  })
 })

packages/hub/test/database-rows.test.ts

@@ -260,6 +260,53 @@ describe('Database Row Storage', () => {
 
         expect(result.rows.some((r) => r.id === 'row-1')).toBe(true)
       })
+
+      // Column identifiers are interpolated into SQL and cannot be
+      // parameterized; they must be allowlisted so a hostile identifier cannot
+      // inject clauses (exploration 0307).
+      it('should reject a SELECT column with SQL metacharacters', async () => {
+        await expect(
+          storage.queryDatabaseRows({
+            databaseId: 'db-1',
+            select: [`title')) UNION SELECT sql FROM sqlite_master --`]
+          })
+        ).rejects.toThrow(/Unsafe column identifier/)
+      })
+
+      it('should reject a filter column with SQL metacharacters', async () => {
+        await expect(
+          storage.queryDatabaseRows({
+            databaseId: 'db-1',
+            filters: {
+              operator: 'and',
+              conditions: [
+                { columnId: `status') = 'active' OR '1'='1`, operator: 'equals', value: 'x' }
+              ]
+            }
+          })
+        ).rejects.toThrow(/Unsafe column identifier/)
+      })
+
+      it('should reject a sort column with SQL metacharacters', async () => {
+        await expect(
+          storage.queryDatabaseRows({
+            databaseId: 'db-1',
+            sorts: [{ columnId: `title') DESC; DROP TABLE database_rows --`, direction: 'asc' }]
+          })
+        ).rejects.toThrow(/Unsafe column identifier/)
+      })
+
+      it('should still accept ordinary column identifiers', async () => {
+        const result = await storage.queryDatabaseRows({
+          databaseId: 'db-1',
+          sorts: [{ columnId: 'title', direction: 'asc' }],
+          filters: {
+            operator: 'and',
+            conditions: [{ columnId: 'status', operator: 'equals', value: 'active' }]
+          }
+        })
+        expect(result.rows.length).toBeGreaterThan(0)
+      })
     })
   })
 

packages/sync/src/chain.ts

@@ -66,6 +66,13 @@ export interface Fork<T = unknown> {
  * - All hashes are computed correctly (not tampered)
  * - Parent references exist in the chain (or are null for roots)
  *
+ * SECURITY: this is a **structural** check only — it does NOT verify Ed25519
+ * signatures, and a missing parent or a fork is reported (or tolerated) rather
+ * than treated as a hard failure. It is not an admission gate for untrusted
+ * input: authenticate authorship with `verifyChange` (and apply LWW/idempotency)
+ * at the ingest boundary — see `NodeStore.applyRemoteChange` and the hub's
+ * `node-relay.ts`, which run the real crypto checks (exploration 0307).
+ *
  * @param changes - The changes to validate
  * @returns Validation result
  */

packages/sync/src/change.ts

@@ -15,7 +15,13 @@ import { hashHex, sign, verify } from '@xnetjs/crypto'
  * Current protocol version for Change<T>.
  *
  * Version history:
- * - 3: Multi-level cryptography with hybrid signatures (Ed25519 + ML-DSA)
+ * - 4: Grinding-resistant LWW tiebreak key (exploration 0300). NOTE: the change
+ *   signature is still **Ed25519-only** — `signChange`/`verifyChange` use the
+ *   classical `@xnetjs/crypto` `sign`/`verify`. The hybrid/ML-DSA apparatus
+ *   (`hybrid-signing.ts`) is NOT wired into `Change<T>` yet; wiring it (or the
+ *   PQ envelope) is tracked in exploration 0307.
+ * - 3: Reserved for multi-level cryptography (hybrid Ed25519 + ML-DSA) — defined
+ *   in the crypto layer but not carried by the change-signing path.
  * - 2: V2 compact format with abbreviated field names
  * - 1: Initial versioned protocol (adds protocolVersion field)
  * - 0/undefined: Legacy unversioned changes (backward compat)

packages/sync/src/handlers/index.ts

@@ -254,6 +254,12 @@ export const changeHandlerRegistry = new ChangeHandlerRegistry()
 
 /**
  * Create a simple handler that accepts all versions.
+ *
+ * SECURITY: the handler pipeline does NOT verify change hashes or signatures,
+ * and the default `validate` accepts everything. Handlers process
+ * already-authenticated changes — callers MUST verify authorship (`verifyChange`
+ * + `verifyChangeHash`) before routing untrusted input through the registry
+ * (exploration 0307).
  */
 export function createHandler<T>(
   type: string,

packages/sync/src/integrity.test.ts

@@ -4,7 +4,7 @@
 
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import type { Change } from './change'
-import { generateKeyPair } from '@xnetjs/crypto'
+import { generateIdentity } from '@xnetjs/identity'
 import { describe, it, expect } from 'vitest'
 import { signChange, createUnsignedChange } from './change'
 import {
@@ -29,17 +29,19 @@ async function createTestChange(
     wallTime: number
   }> = {}
 ): Promise<Change<{ data: string }>> {
-  const keyPair = await generateKeyPair()
+  // Use a real did:key so the author DID binds to the signing key — the
+  // integrity checker now verifies signatures cryptographically (0307).
+  const { identity, privateKey } = generateIdentity()
   const unsigned = createUnsignedChange({
     id: overrides.id ?? `change-${Math.random().toString(36).slice(2)}`,
     type: 'test-change',
     payload: { data: 'test' },
     parentHash: (overrides.parentHash ?? null) as any,
-    authorDID: 'did:key:test' as any,
+    authorDID: identity.did,
     lamport: overrides.lamportTime ?? 1,
     wallTime: overrides.wallTime ?? Date.now()
   })
-  return signChange(unsigned, keyPair.privateKey)
+  return signChange(unsigned, privateKey)
 }
 
 async function createChain(length: number): Promise<Change<{ data: string }>[]> {
@@ -99,6 +101,18 @@ describe('verifyIntegrity', () => {
     expect(report.issues[0].type).toBe('signature-invalid')
   })
 
+  it('should detect a present-but-garbage signature (real Ed25519 verification)', async () => {
+    const change = await createTestChange({ id: 'forged-sig' })
+    // Non-empty but forged signature — previously passed a presence-only check.
+    ;(change as any).signature = new Uint8Array(64).fill(7)
+
+    const report = await verifyIntegrity([change], { skipHashes: true })
+
+    expect(report.issues).toHaveLength(1)
+    expect(report.issues[0].type).toBe('signature-invalid')
+    expect(report.valid).toBe(0)
+  })
+
   it('should detect duplicate IDs', async () => {
     const change1 = await createTestChange({ id: 'dup' })
     const change2 = await createTestChange({ id: 'dup' })
@@ -313,7 +327,8 @@ describe('attemptRepair', () => {
       }
     ]
 
-    const result = await attemptRepair([change], issues)
+    // recompute-hash is gated: it must be explicitly trusted (0307).
+    const result = await attemptRepair([change], issues, { trustHashRecompute: true })
 
     expect(result.repairCount).toBe(1)
     expect(result.remainingIssues).toHaveLength(0)
@@ -322,6 +337,32 @@ describe('attemptRepair', () => {
     expect(result.repaired[0].hash).toMatch(/^cid:blake3:/)
   })
 
+  it('should refuse recompute-hash on untrusted data by default', async () => {
+    const change = await createTestChange({ id: 'untrusted-hash' })
+    ;(change as any).hash = 'cid:blake3:bad'
+
+    const issues = [
+      {
+        changeId: 'untrusted-hash',
+        type: 'hash-mismatch' as const,
+        details: 'Hash mismatch',
+        severity: 'error' as const,
+        repairAction: {
+          type: 'recompute-hash' as const,
+          description: 'Recompute hash',
+          automatic: true
+        }
+      }
+    ]
+
+    // No trustHashRecompute flag → must NOT launder the tampered payload.
+    const result = await attemptRepair([change], issues)
+
+    expect(result.repairCount).toBe(0)
+    expect(result.remainingIssues).toHaveLength(1)
+    expect(result.repaired[0].hash).toBe('cid:blake3:bad')
+  })
+
   it('should not repair non-automatic issues', async () => {
     const change = await createTestChange({ id: 'no-repair' })