diff --git a/.changeset/secure-electric-shape-proxy.md b/.changeset/secure-electric-shape-proxy.md new file mode 100644 index 0000000000..8567d8a236 --- /dev/null +++ b/.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. diff --git a/packages/agents-server/src/routing/hooks.ts b/packages/agents-server/src/routing/hooks.ts index 0aea49744d..0a5fc39209 100644 --- a/packages/agents-server/src/routing/hooks.ts +++ b/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`) } diff --git a/packages/agents-server/src/utils/server-utils.ts b/packages/agents-server/src/utils/server-utils.ts index 658a753904..5d35a5f5d0 100644 --- a/packages/agents-server/src/utils/server-utils.ts +++ b/packages/agents-server/src/utils/server-utils.ts @@ -90,6 +90,25 @@ export async function fileExists(filePath: string): Promise { } } +/** + * 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 diff --git a/packages/agents-server/test/routing-hooks.test.ts b/packages/agents-server/test/routing-hooks.test.ts index 84cb3761e5..30e653b910 100644 --- a/packages/agents-server/test/routing-hooks.test.ts +++ b/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`), diff --git a/packages/agents-server/test/server-utils.test.ts b/packages/agents-server/test/server-utils.test.ts index 78ca3c50e0..8e95c0f3a5 100644 --- a/packages/agents-server/test/server-utils.test.ts +++ b/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')` + ) + }) })