Secure Electric shape proxy: default-deny tables + validate client where

.changeset/secure-electric-shape-proxy.md

@@ -0,0 +1,5 @@
+---
+'@electric-ax/agents-server': patch
+---
+
+agents-server: harden the Electric shape proxy (`/_electric/electric/v1/shape`) against access-control bypasses. Requests for tables outside the explicitly scoped allowlist are now rejected with `403 TABLE_NOT_ALLOWED` instead of being forwarded with the privileged Electric secret and no row/column filter. Client-supplied `where` clauses that are not self-contained (unbalanced parentheses, top-level paren underflow, unterminated string/identifier literals, or SQL comment markers) are rejected with `400 INVALID_WHERE` so they cannot break out of the enforced per-tenant/per-principal scoping.

packages/agents-server/src/routing/hooks.ts

@@ -1,6 +1,7 @@
 import { SpanKind, SpanStatusCode } from '@opentelemetry/api'
 import { apiError } from '../electric-agents-http.js'
 import { ElectricAgentsError } from '../entity-manager.js'
+import { ElectricProxyError } from '../utils/server-utils.js'
 import { ELECTRIC_PRINCIPAL_HEADER } from '../principal.js'
 import { ATTR, extractTraceContext, tracer } from '../tracing.js'
 import { serverLog } from '../utils/log.js'
@@ -112,6 +113,12 @@ export function errorMapper(err: unknown, req: IRequest): Response {
   if (err instanceof ElectricAgentsError) {
     return apiError(err.status, err.code, err.message, err.details)
   }
+  if (err instanceof ElectricProxyError) {
+    serverLog.warn(
+      `[agent-server] Electric proxy rejected request (${err.code}): ${req.url}`
+    )
+    return apiError(err.status, err.code, err.message)
+  }
   serverLog.error(`[agent-server] Unhandled error:`, err)
   return apiError(500, `INTERNAL_SERVER_ERROR`, `Internal server error`)
 }

packages/agents-server/src/utils/server-utils.ts

@@ -90,6 +90,25 @@ export async function fileExists(filePath: string): Promise<boolean> {
   }
 }
 
+/**
+ * Raised when an Electric shape proxy request must be rejected for security
+ * reasons (an un-scoped table, or a client `where` clause that could escape the
+ * enforced per-tenant/per-principal scoping). The global `errorMapper` hook
+ * maps this to an HTTP error response. Defined here (rather than reusing
+ * `ElectricAgentsError`) to keep this module free of the heavy entity-manager
+ * import graph.
+ */
+export class ElectricProxyError extends Error {
+  constructor(
+    readonly code: `INVALID_WHERE` | `TABLE_NOT_ALLOWED`,
+    message: string,
+    readonly status: number
+  ) {
+    super(message)
+    this.name = `ElectricProxyError`
+  }
+}
+
 export function buildElectricProxyTarget(options: {
   incomingUrl: URL
   electricUrl: string
@@ -117,6 +136,15 @@ export function buildElectricProxyTarget(options: {
     target.searchParams.set(`secret`, options.electricSecret)
   }
 
+  // The enforced scoping `where` is AND-combined with the client's own `where`.
+  // A client clause that is not self-contained (e.g. `1=1) OR (1=1`) could
+  // break out of its parenthesised group and neutralise the scoping under SQL
+  // operator precedence, so reject anything that isn't balanced.
+  const clientWhere = options.incomingUrl.searchParams.get(`where`)
+  if (clientWhere !== null && !isSelfContainedWhereClause(clientWhere)) {
+    throw new ElectricProxyError(`INVALID_WHERE`, `Invalid where clause`, 400)
+  }
+
   const table = options.incomingUrl.searchParams.get(`table`)
   if (table === `entities`) {
     target.searchParams.set(
@@ -224,11 +252,66 @@ export function buildElectricProxyTarget(options: {
         permissionBypass: options.permissionBypass,
       })
     )
+  } else {
+    // Default-deny: every shape request gets the privileged Electric secret
+    // (when configured) injected above, so only tables with explicit column +
+    // row scoping may be proxied. Any other table (or a missing `table` param)
+    // is rejected.
+    throw new ElectricProxyError(
+      `TABLE_NOT_ALLOWED`,
+      `Table is not available through the Electric proxy`,
+      403
+    )
   }
 
   return target
 }
 
+/**
+ * Returns true when a client-supplied Electric `where` clause is self-contained:
+ * its parentheses are balanced, never close below the top level, all string
+ * (`'`) and identifier (`"`) literals are terminated, and it contains no SQL
+ * comment markers. Such a clause cannot break out of the `(...)` group it is
+ * wrapped in when AND-combined with the enforced scoping predicate, nor comment
+ * out the trailing paren the proxy appends. Characters inside string/identifier
+ * literals are ignored. Comment markers are rejected unconditionally (even where
+ * harmless) as a conservative defensive measure; dollar-quoted and `E''` strings
+ * are not modeled and only ever cause fail-safe over-rejection, never a bypass.
+ */
+function isSelfContainedWhereClause(where: string): boolean {
+  let depth = 0
+  let quote: `'` | `"` | null = null
+  for (let i = 0; i < where.length; i++) {
+    const ch = where[i]
+    if (quote !== null) {
+      if (ch === quote) {
+        if (where[i + 1] === quote) {
+          i++ // doubled quote is an escaped literal quote
+        } else {
+          quote = null
+        }
+      }
+      continue
+    }
+    if (ch === `'` || ch === `"`) {
+      quote = ch
+    } else if (ch === `(`) {
+      depth++
+    } else if (ch === `)`) {
+      depth--
+      if (depth < 0) {
+        return false
+      }
+    } else if (
+      (ch === `-` && where[i + 1] === `-`) ||
+      (ch === `/` && where[i + 1] === `*`)
+    ) {
+      return false // SQL comment marker
+    }
+  }
+  return depth === 0 && quote === null
+}
+
 export function buildReadableEntitiesWhere(options: {
   tenantId: string
   principalUrl: string

packages/agents-server/test/routing-hooks.test.ts

@@ -1,6 +1,8 @@
-import { describe, expect, it } from 'vitest'
+import { describe, expect, it, vi } from 'vitest'
 import { AutoRouter } from 'itty-router'
 import { ElectricAgentsError } from '../src/entity-manager'
+import { ElectricProxyError } from '../src/utils/server-utils'
+import { serverLog } from '../src/utils/log'
 import {
   applyCors,
   errorMapper,
@@ -57,6 +59,44 @@ describe(`routing/hooks`, () => {
     })
   })
 
+  it(`errorMapper converts ElectricProxyError to its API error status`, async () => {
+    const response = errorMapper(
+      new ElectricProxyError(
+        `TABLE_NOT_ALLOWED`,
+        `Table is not available through the Electric proxy`,
+        403
+      ),
+      new Request(`http://x`) as IRequest
+    )
+    expect(response.status).toBe(403)
+    expect((await response.json()).error.code).toBe(`TABLE_NOT_ALLOWED`)
+  })
+
+  it(`errorMapper maps an INVALID_WHERE ElectricProxyError to a 400`, async () => {
+    const response = errorMapper(
+      new ElectricProxyError(`INVALID_WHERE`, `Invalid where clause`, 400),
+      new Request(`http://x`) as IRequest
+    )
+    expect(response.status).toBe(400)
+    expect((await response.json()).error.code).toBe(`INVALID_WHERE`)
+  })
+
+  it(`errorMapper logs a warning for ElectricProxyError rejections`, () => {
+    const warnSpy = vi.spyOn(serverLog, `warn`).mockImplementation(() => {})
+    try {
+      errorMapper(
+        new ElectricProxyError(`TABLE_NOT_ALLOWED`, `nope`, 403),
+        new Request(
+          `http://x/_electric/electric/v1/shape?table=secrets`
+        ) as IRequest
+      )
+      expect(warnSpy).toHaveBeenCalledOnce()
+      expect(warnSpy.mock.calls[0]?.join(` `)).toContain(`TABLE_NOT_ALLOWED`)
+    } finally {
+      warnSpy.mockRestore()
+    }
+  })
+
   it(`errorMapper turns unknown errors into a 500`, async () => {
     const response = errorMapper(
       new Error(`oops`),

packages/agents-server/test/server-utils.test.ts

@@ -1,5 +1,8 @@
 import { describe, expect, it } from 'vitest'
-import { buildElectricProxyTarget } from '../src/utils/server-utils'
+import {
+  ElectricProxyError,
+  buildElectricProxyTarget,
+} from '../src/utils/server-utils'
 
 function shapeTarget(query: string): URL {
   return buildElectricProxyTarget({
@@ -151,4 +154,144 @@ describe(`server utils`, () => {
     )
     expect(where).not.toMatch(/\bEXISTS\b/i)
   })
+
+  it(`rejects shape requests for tables outside the scoped allowlist`, () => {
+    // The proxy injects the privileged Electric secret into every /v1/shape
+    // request. Any table that is not explicitly scoped must be denied rather
+    // than forwarded with the secret attached and no row/column filter.
+    for (const table of [
+      `entity_permission_grants`,
+      `subscription_webhooks`,
+      `scheduled_tasks`,
+      `entity_bridges`,
+      `users; DROP TABLE users`,
+    ]) {
+      expect(() => shapeTarget(`table=${encodeURIComponent(table)}`)).toThrow(
+        ElectricProxyError
+      )
+    }
+  })
+
+  it(`rejects shape requests with no table param`, () => {
+    expect(() => shapeTarget(``)).toThrow(ElectricProxyError)
+  })
+
+  it(`rejects client where clauses that break out of the enforced scope`, () => {
+    // tenant_id = '...' AND (1=1) OR (1=1) collapses to OR TRUE under SQL
+    // precedence, defeating per-tenant/per-principal scoping.
+    expect(() =>
+      shapeTarget(`table=users&where=${encodeURIComponent(`1=1) OR (1=1`)}`)
+    ).toThrow(ElectricProxyError)
+  })
+
+  it(`rejects client where clauses that escape via a trailing comment`, () => {
+    expect(() =>
+      shapeTarget(`table=users&where=${encodeURIComponent(`1=1) OR 1=1 --`)}`)
+    ).toThrow(ElectricProxyError)
+  })
+
+  it(`rejects client where clauses with an unterminated string literal`, () => {
+    expect(() =>
+      shapeTarget(`table=users&where=${encodeURIComponent(`email = 'x`)}`)
+    ).toThrow(ElectricProxyError)
+  })
+
+  it(`rejects client where clauses containing SQL comment markers`, () => {
+    // A trailing `--` would comment out the closing paren the proxy appends.
+    for (const clause of [`kind = 'local' -- x`, `kind = 'local' /* x */`]) {
+      expect(() =>
+        shapeTarget(`table=users&where=${encodeURIComponent(clause)}`)
+      ).toThrow(ElectricProxyError)
+    }
+  })
+
+  it(`allows balanced client where clauses with OR inside parentheses`, () => {
+    const target = shapeTarget(
+      `table=users&where=${encodeURIComponent(`email = 'a' OR email = 'b'`)}`
+    )
+
+    expect(target.searchParams.get(`where`)).toBe(
+      `tenant_id = 'tenant-test' AND (email = 'a' OR email = 'b')`
+    )
+  })
+
+  it(`allows parentheses that appear inside string literals`, () => {
+    const target = shapeTarget(
+      `table=users&where=${encodeURIComponent(`display_name = 'a)b'`)}`
+    )
+
+    expect(target.searchParams.get(`where`)).toBe(
+      `tenant_id = 'tenant-test' AND (display_name = 'a)b')`
+    )
+  })
+
+  it(`rejects client where clauses that close the wrapping group early`, () => {
+    // The cleanest break-out: a top-level `)` that closes the proxy's wrapping
+    // `(` (paren depth dips below zero), distinct from the balanced `1=1) OR (1=1`.
+    for (const clause of [`a=1) OR (b=2`, `1=1)`]) {
+      expect(() =>
+        shapeTarget(`table=users&where=${encodeURIComponent(clause)}`)
+      ).toThrow(ElectricProxyError)
+    }
+  })
+
+  it(`wraps a top-level OR client clause so it stays scoped`, () => {
+    // Security depends on the enforced predicate AND-ing a *parenthesised*
+    // client clause; without the wrapping, `1=1 OR 2=2` would collapse scope.
+    const target = shapeTarget(
+      `table=users&where=${encodeURIComponent(`1=1 OR 2=2`)}`
+    )
+
+    expect(target.searchParams.get(`where`)).toBe(
+      `tenant_id = 'tenant-test' AND (1=1 OR 2=2)`
+    )
+  })
+
+  it(`allows double-quoted identifiers, ignoring parens inside them`, () => {
+    const target = shapeTarget(
+      `table=users&where=${encodeURIComponent(`"weird)col" = 1`)}`
+    )
+
+    expect(target.searchParams.get(`where`)).toBe(
+      `tenant_id = 'tenant-test' AND ("weird)col" = 1)`
+    )
+  })
+
+  it(`rejects an unterminated double-quoted identifier`, () => {
+    expect(() =>
+      shapeTarget(`table=users&where=${encodeURIComponent(`"col = 1`)}`)
+    ).toThrow(ElectricProxyError)
+  })
+
+  it(`allows doubled single quotes as escaped literal quotes`, () => {
+    const target = shapeTarget(
+      `table=users&where=${encodeURIComponent(`display_name = 'O''Brien'`)}`
+    )
+
+    expect(target.searchParams.get(`where`)).toBe(
+      `tenant_id = 'tenant-test' AND (display_name = 'O''Brien')`
+    )
+  })
+
+  it(`keeps parens inside escaped-quote string literals from breaking out`, () => {
+    // The `)` and `(` here live inside the string literal (the doubled quotes
+    // are escapes), so the clause is self-contained and must be allowed.
+    const target = shapeTarget(
+      `table=users&where=${encodeURIComponent(`name = 'a'') OR (''x'`)}`
+    )
+
+    expect(target.searchParams.get(`where`)).toBe(
+      `tenant_id = 'tenant-test' AND (name = 'a'') OR (''x')`
+    )
+  })
+
+  it(`allows comment markers that appear inside string literals`, () => {
+    const target = shapeTarget(
+      `table=users&where=${encodeURIComponent(`display_name = '-- legit'`)}`
+    )
+
+    expect(target.searchParams.get(`where`)).toBe(
+      `tenant_id = 'tenant-test' AND (display_name = '-- legit')`
+    )
+  })
 })