From 07ef8c751dfb1ab6849ab29a6cc5ea23e7dfe68b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Emre=20Kabakc=C4=B1?= Date: Mon, 18 May 2026 01:24:43 +0100 Subject: [PATCH 1/3] fix(server): scope tenant boundaries across egress judge, secret proxy, and oauth state Close four cross-org leakage paths surfaced by a multi-tenant audit: - Egress judge cache key now includes the worker's org id so org A's verdict for `api.example.com` cannot satisfy org B's identical request. Plumbs `organizationId` onto WorkerTokenData and through the proxy. - SecretMapping carries `organizationId`; new `lookupPlaceholderMapping` rejects mismatches the same way as a missing mapping (log + null). - SlackInstallStateData now carries `organizationId`; `/slack/install` refuses anonymous sessions, `/slack/oauth_callback` rejects when the callback session's org doesn't match the install state's. - ChatInstanceManager.addConnection rejects Telegram `mode: "polling"` when `LOBU_CLOUD_MODE=1`. Self-hosters (default) keep polling for tunnel-less dev. Documented in AGENTS.md. --- AGENTS.md | 2 + packages/core/src/worker/auth.ts | 10 +++ .../unit/egress-judge-timeout.test.ts | 8 +- .../__tests__/egress-judge-cache.test.ts | 15 +++- .../gateway/__tests__/proxy-hardening.test.ts | 22 ++++-- .../__tests__/rest-api-hardening.test.ts | 25 +++++-- .../gateway/__tests__/secret-proxy.test.ts | 57 ++++++++++++++ .../gateway/__tests__/slack-routes.test.ts | 65 +++++++++++++++- .../src/gateway/auth/oauth/state-store.ts | 7 ++ .../connections/chat-instance-manager.ts | 44 +++++++++++ .../orchestration/base-deployment-manager.ts | 6 +- .../src/gateway/proxy/egress-judge/cache.ts | 10 ++- .../src/gateway/proxy/egress-judge/judge.ts | 1 + .../src/gateway/proxy/egress-judge/types.ts | 7 ++ .../server/src/gateway/proxy/http-proxy.ts | 13 +++- .../server/src/gateway/proxy/secret-proxy.ts | 74 ++++++++++++++++-- .../server/src/gateway/routes/public/slack.ts | 75 ++++++++++++++++++- .../src/gateway/services/agent-threads.ts | 3 +- 18 files changed, 402 insertions(+), 42 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index af04c4ae7..a1490733d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,6 +31,8 @@ All chat platforms (Telegram, Slack, Discord, WhatsApp, Teams) run through Chat **Webhooks via the Chat SDK adapter are the default transport.** Don't add new per-platform alternative transports (Slack Socket Mode, Discord Gateway WebSocket bridges, etc.) or extra runtime SDKs. The lone exception is Telegram, whose connection config exposes an optional `polling` mode (`mode: "auto" | "webhook" | "polling"`) implemented inside the Chat SDK adapter — still no extra SDK. Local dev for webhook-only platforms uses a tunnel (cloudflared / ngrok / Tailscale Funnel); Lobu Cloud users get a public URL for free. Sticking to the Chat SDK keeps one delivery story, one set of retries, and zero extra dependencies. +`mode: "polling"` is rejected at connection-create time when `LOBU_CLOUD_MODE=1` — a polling worker long-polls Telegram's edge from the gateway pod and shares that connection across tenants, so a misbehaving polling connection in one org degrades delivery for every other tenant. Self-hosters (`LOBU_CLOUD_MODE` unset/0) keep the polling option for tunnel-less dev. + #### Orchestration - **Embedded-only deployment.** Gateway, workers, embeddings, and the Lobu memory backend run in a single Node process (`lobu run`, or `bun run dev` in the monorepo). Workers spawn as `child_process.spawn` subprocesses on the same host; on Linux the spawn path uses `systemd-run --user --scope` for cgroup limits + IPAddressDeny + capability drops. There is no Docker or Kubernetes deployment manager. - Postgres (with `pgvector`; optionally `postgis` for geo enrichment) is the only user-provided external. The Node process connects out via `DATABASE_URL`. Runtime state — queues, chat connection rows, grant cache, MCP proxy sessions — lives in dedicated Postgres tables. diff --git a/packages/core/src/worker/auth.ts b/packages/core/src/worker/auth.ts index 1df2899af..b87591148 100644 --- a/packages/core/src/worker/auth.ts +++ b/packages/core/src/worker/auth.ts @@ -15,6 +15,14 @@ export interface WorkerTokenData { channelId: string; teamId?: string; agentId?: string; + /** + * Owning organization of the agent the token was minted for. Used by the + * HTTP proxy to scope per-tenant caches (e.g. egress-judge verdict cache) + * so org A's decisions can never satisfy org B's requests. Optional only + * because some internal/preflight call sites mint tokens before the owning + * org has been resolved; production agent runs always set it. + */ + organizationId?: string; connectionId?: string; deploymentName: string; timestamp: number; @@ -33,6 +41,7 @@ export function generateWorkerToken( channelId: string; teamId?: string; agentId?: string; + organizationId?: string; connectionId?: string; platform?: string; sessionKey?: string; @@ -49,6 +58,7 @@ export function generateWorkerToken( channelId: options.channelId, teamId: options.teamId, agentId: options.agentId, + organizationId: options.organizationId, connectionId: options.connectionId, deploymentName, timestamp: Date.now(), diff --git a/packages/server/src/__tests__/unit/egress-judge-timeout.test.ts b/packages/server/src/__tests__/unit/egress-judge-timeout.test.ts index 4ed4ffb28..ee8aec93c 100644 --- a/packages/server/src/__tests__/unit/egress-judge-timeout.test.ts +++ b/packages/server/src/__tests__/unit/egress-judge-timeout.test.ts @@ -40,7 +40,7 @@ describe("EgressJudge timeout", () => { const judge = new EgressJudge({ client, judgeTimeoutMs: 30 }); const started = Date.now(); const decision = await judge.decide( - { agentId: "agent-a", hostname: "api.github.com" }, + { agentId: "agent-a", organizationId: "org-1", hostname: "api.github.com" }, rule() ); const elapsed = Date.now() - started; @@ -62,7 +62,7 @@ describe("EgressJudge timeout", () => { // Distinct hostnames so the verdict cache never short-circuits the path. for (let i = 0; i < 5; i++) { const decision = await judge.decide( - { agentId: "agent-a", hostname: `h${i}.example.com` }, + { agentId: "agent-a", organizationId: "org-1", hostname: `h${i}.example.com` }, rule() ); expect(decision.verdict).toBe("deny"); @@ -72,7 +72,7 @@ describe("EgressJudge timeout", () => { expect(client.calls).toBe(2); const afterOpen = await judge.decide( - { agentId: "agent-a", hostname: "another.example.com" }, + { agentId: "agent-a", organizationId: "org-1", hostname: "another.example.com" }, rule() ); expect(afterOpen.verdict).toBe("deny"); @@ -88,7 +88,7 @@ describe("EgressJudge timeout", () => { const judge = new EgressJudge({ client }); const started = Date.now(); const decision = await judge.decide( - { agentId: "agent-a", hostname: "api.github.com" }, + { agentId: "agent-a", organizationId: "org-1", hostname: "api.github.com" }, rule() ); expect(decision.verdict).toBe("deny"); diff --git a/packages/server/src/gateway/__tests__/egress-judge-cache.test.ts b/packages/server/src/gateway/__tests__/egress-judge-cache.test.ts index 64a3fc4bb..02c4a1e99 100644 --- a/packages/server/src/gateway/__tests__/egress-judge-cache.test.ts +++ b/packages/server/src/gateway/__tests__/egress-judge-cache.test.ts @@ -10,6 +10,7 @@ describe("VerdictCache", () => { test("stores and retrieves a verdict", () => { const cache = new VerdictCache(60_000, 100); const key = VerdictCache.key({ + orgId: "org-1", policyHash: "abc", hostname: "example.com", method: "GET", @@ -21,11 +22,13 @@ describe("VerdictCache", () => { test("key is case-insensitive for hostname and method", () => { const a = VerdictCache.key({ + orgId: "org-1", policyHash: "h", hostname: "Example.COM", method: "get", }); const b = VerdictCache.key({ + orgId: "org-1", policyHash: "h", hostname: "example.com", method: "GET", @@ -34,14 +37,20 @@ describe("VerdictCache", () => { }); test("different policy hashes do not collide", () => { - const a = VerdictCache.key({ policyHash: "h1", hostname: "x.com" }); - const b = VerdictCache.key({ policyHash: "h2", hostname: "x.com" }); + const a = VerdictCache.key({ orgId: "org-1", policyHash: "h1", hostname: "x.com" }); + const b = VerdictCache.key({ orgId: "org-1", policyHash: "h2", hostname: "x.com" }); + expect(a).not.toBe(b); + }); + + test("different orgIds do not collide even for identical policy+request", () => { + const a = VerdictCache.key({ orgId: "org-a", policyHash: "h", hostname: "x.com" }); + const b = VerdictCache.key({ orgId: "org-b", policyHash: "h", hostname: "x.com" }); expect(a).not.toBe(b); }); test("expires entries after the TTL", async () => { const cache = new VerdictCache(10, 100); - const key = VerdictCache.key({ policyHash: "h", hostname: "x.com" }); + const key = VerdictCache.key({ orgId: "org-1", policyHash: "h", hostname: "x.com" }); cache.set(key, { verdict: "allow", reason: "ok" }); expect(cache.get(key)).toBeDefined(); await new Promise((r) => setTimeout(r, 20)); diff --git a/packages/server/src/gateway/__tests__/proxy-hardening.test.ts b/packages/server/src/gateway/__tests__/proxy-hardening.test.ts index 2c53ae05d..d65c19bf6 100644 --- a/packages/server/src/gateway/__tests__/proxy-hardening.test.ts +++ b/packages/server/src/gateway/__tests__/proxy-hardening.test.ts @@ -606,12 +606,14 @@ describe("CRLF injection prevention in judge-provided reason", () => { describe("VerdictCache — key independence", () => { test("different methods produce different cache keys", () => { const a = VerdictCache.key({ + orgId: "org-1", policyHash: "h", hostname: "example.com", method: "GET", path: "/foo", }); const b = VerdictCache.key({ + orgId: "org-1", policyHash: "h", hostname: "example.com", method: "POST", @@ -622,12 +624,14 @@ describe("VerdictCache — key independence", () => { test("different paths produce different cache keys", () => { const a = VerdictCache.key({ + orgId: "org-1", policyHash: "h", hostname: "example.com", method: "GET", path: "/foo", }); const b = VerdictCache.key({ + orgId: "org-1", policyHash: "h", hostname: "example.com", method: "GET", @@ -638,10 +642,12 @@ describe("VerdictCache — key independence", () => { test("CONNECT (no method/path) and GET / produce different cache keys", () => { const connect = VerdictCache.key({ + orgId: "org-1", policyHash: "h", hostname: "example.com", }); const get = VerdictCache.key({ + orgId: "org-1", policyHash: "h", hostname: "example.com", method: "GET", @@ -652,8 +658,8 @@ describe("VerdictCache — key independence", () => { test("changing policyHash invalidates previously-set entry", () => { const cache = new VerdictCache(60_000, 100); - const key1 = VerdictCache.key({ policyHash: "old-hash", hostname: "x.com" }); - const key2 = VerdictCache.key({ policyHash: "new-hash", hostname: "x.com" }); + const key1 = VerdictCache.key({ orgId: "org-1", policyHash: "old-hash", hostname: "x.com" }); + const key2 = VerdictCache.key({ orgId: "org-1", policyHash: "new-hash", hostname: "x.com" }); cache.set(key1, { verdict: "allow", reason: "ok" }); // key2 (new policy hash) must miss — cache isolates by policyHash @@ -906,7 +912,7 @@ describe("EgressJudge — additional behavioral coverage", () => { const judge = new EgressJudge({ client }); await judge.decide( - { agentId: "a", hostname: "example.com" }, + { agentId: "a", organizationId: "org-1", hostname: "example.com" }, rule({ policyHash: "unique-model-1" }) ); expect(capturedModel).toBe("claude-haiku-4-5-20251001"); @@ -923,7 +929,7 @@ describe("EgressJudge — additional behavioral coverage", () => { const judge = new EgressJudge({ client, defaultModel: "default-model" }); await judge.decide( - { agentId: "a", hostname: "example.com" }, + { agentId: "a", organizationId: "org-1", hostname: "example.com" }, rule({ policyHash: "unique-model-2", judgeModel: "override-model" }) ); expect(capturedModel).toBe("override-model"); @@ -945,14 +951,14 @@ describe("EgressJudge — additional behavioral coverage", () => { // Trip the breaker — threshold=1, one failure suffices await judge.decide( - { agentId: "a", hostname: "h1.example.com" }, + { agentId: "a", organizationId: "org-1", hostname: "h1.example.com" }, rule({ policyHash: "brk-coverage" }) ); expect(calls).toBe(1); // Next request: circuit open → short-circuit const d = await judge.decide( - { agentId: "a", hostname: "h2.example.com" }, + { agentId: "a", organizationId: "org-1", hostname: "h2.example.com" }, rule({ policyHash: "brk-coverage" }) ); expect(calls).toBe(1); // no extra call @@ -967,7 +973,7 @@ describe("EgressJudge — additional behavioral coverage", () => { }, }; const judge = new EgressJudge({ client }); - const req = { agentId: "a", hostname: "example.com" }; + const req = { agentId: "a", organizationId: "org-1", hostname: "example.com" }; const r = rule({ policyHash: "p-cache-meta", judgeName: "my-judge" }); await judge.decide(req, r); @@ -992,7 +998,7 @@ describe("EgressJudge — additional behavioral coverage", () => { }); const d = await judge.decide( - { agentId: "a", hostname: "x.com" }, + { agentId: "a", organizationId: "org-1", hostname: "x.com" }, rule({ policyHash: "single-fail-coverage" }) ); expect(d.verdict).toBe("deny"); diff --git a/packages/server/src/gateway/__tests__/rest-api-hardening.test.ts b/packages/server/src/gateway/__tests__/rest-api-hardening.test.ts index 2aa02927d..435831ef2 100644 --- a/packages/server/src/gateway/__tests__/rest-api-hardening.test.ts +++ b/packages/server/src/gateway/__tests__/rest-api-hardening.test.ts @@ -748,6 +748,8 @@ describe("slack routes: OAuth callback and replay protection", () => { let completeSlackOAuthInstall: ReturnType; let handleSlackAppWebhook: ReturnType; let router: ReturnType; + let app: Hono; + let sessionOrgId: string | null; beforeAll(async () => { await ensurePgliteForGatewayTests(); @@ -772,6 +774,14 @@ describe("slack routes: OAuth callback and replay protection", () => { completeSlackOAuthInstall, handleSlackAppWebhook, } as any); + + sessionOrgId = "org-default"; + app = new Hono(); + app.use("*", async (c, next) => { + if (sessionOrgId !== null) c.set("organizationId" as never, sessionOrgId); + await next(); + }); + app.route("", router); }); afterEach(() => { @@ -788,7 +798,7 @@ describe("slack routes: OAuth callback and replay protection", () => { }); test("callback with missing state parameter returns 400", async () => { - const response = await router.request( + const response = await app.request( "/slack/oauth_callback?code=test-code" // state is absent ); @@ -799,7 +809,7 @@ describe("slack routes: OAuth callback and replay protection", () => { }); test("callback with missing code parameter returns 400", async () => { - const response = await router.request( + const response = await app.request( "/slack/oauth_callback?state=some-state" // code is absent ); @@ -808,7 +818,7 @@ describe("slack routes: OAuth callback and replay protection", () => { }); test("callback with unknown state (not in DB) returns 400 — no install proceeds", async () => { - const response = await router.request( + const response = await app.request( "/slack/oauth_callback?code=test-code&state=does-not-exist-in-db" ); expect(response.status).toBe(400); @@ -833,7 +843,7 @@ describe("slack routes: OAuth callback and replay protection", () => { ) `; - const response = await router.request( + const response = await app.request( "/slack/oauth_callback?code=test-code&state=expired-state" ); expect(response.status).toBe(400); @@ -850,20 +860,21 @@ describe("slack routes: OAuth callback and replay protection", () => { ${sql.json({ createdAt: Date.now(), redirectUri: "https://gateway.example.com/slack/oauth_callback", + organizationId: "org-default", })}, ${new Date(Date.now() + 600_000)} ) `; // First request should succeed - const first = await router.request( + const first = await app.request( "/slack/oauth_callback?code=test-code&state=one-time-state" ); expect(first.status).toBe(200); expect(completeSlackOAuthInstall).toHaveBeenCalledTimes(1); // Second request with the same state must fail — state already consumed - const second = await router.request( + const second = await app.request( "/slack/oauth_callback?code=test-code&state=one-time-state" ); expect(second.status).toBe(400); @@ -873,7 +884,7 @@ describe("slack routes: OAuth callback and replay protection", () => { test("Slack install returns 503 when SLACK_CLIENT_ID is not set", async () => { delete process.env.SLACK_CLIENT_ID; - const response = await router.request("/slack/install"); + const response = await app.request("/slack/install"); expect(response.status).toBe(503); const body = await response.text(); expect(body).toContain("not configured"); diff --git a/packages/server/src/gateway/__tests__/secret-proxy.test.ts b/packages/server/src/gateway/__tests__/secret-proxy.test.ts index 86a421cf0..0da8af361 100644 --- a/packages/server/src/gateway/__tests__/secret-proxy.test.ts +++ b/packages/server/src/gateway/__tests__/secret-proxy.test.ts @@ -3,6 +3,7 @@ import { createBuiltinSecretRef } from "@lobu/core"; import { __resetPlaceholderCacheForTests, generatePlaceholder, + lookupPlaceholderMapping, SecretProxy, type SecretMapping, storeSecretMapping, @@ -91,6 +92,62 @@ describe("generatePlaceholder", () => { }); }); +describe("lookupPlaceholderMapping org scoping", () => { + beforeEach(() => { + __resetPlaceholderCacheForTests(); + }); + + test("returns the mapping when no expected org is supplied", () => { + const placeholder = generatePlaceholder( + "agent-1", + "API_KEY", + createBuiltinSecretRef("deployments/agent-1/API_KEY"), + "deploy-1", + { organizationId: "org-a" } + ); + const mapping = lookupPlaceholderMapping(placeholder); + expect(mapping?.agentId).toBe("agent-1"); + expect(mapping?.organizationId).toBe("org-a"); + }); + + test("returns the mapping when expected org matches", () => { + const placeholder = generatePlaceholder( + "agent-1", + "API_KEY", + createBuiltinSecretRef("deployments/agent-1/API_KEY"), + "deploy-1", + { organizationId: "org-a" } + ); + const mapping = lookupPlaceholderMapping(placeholder, "org-a"); + expect(mapping?.agentId).toBe("agent-1"); + }); + + test("returns null when expected org mismatches the mapping's org", () => { + const placeholder = generatePlaceholder( + "agent-1", + "API_KEY", + createBuiltinSecretRef("deployments/agent-1/API_KEY"), + "deploy-1", + { organizationId: "org-a" } + ); + // org-b tries to claim a placeholder minted for org-a — must fail closed. + const mapping = lookupPlaceholderMapping(placeholder, "org-b"); + expect(mapping).toBeNull(); + }); + + test("falls through when mapping has no org tag (legacy)", () => { + const placeholder = generatePlaceholder( + "agent-1", + "API_KEY", + createBuiltinSecretRef("deployments/agent-1/API_KEY"), + "deploy-1" + ); + // Mapping has no org → caller's expectation isn't enforceable. + const mapping = lookupPlaceholderMapping(placeholder, "org-a"); + expect(mapping?.agentId).toBe("agent-1"); + }); +}); + describe("SecretProxy user-scoped provider routing", () => { test("passes user context into provider credential lookup", async () => { const proxy = new SecretProxy( diff --git a/packages/server/src/gateway/__tests__/slack-routes.test.ts b/packages/server/src/gateway/__tests__/slack-routes.test.ts index 4ca578f66..7ef732359 100644 --- a/packages/server/src/gateway/__tests__/slack-routes.test.ts +++ b/packages/server/src/gateway/__tests__/slack-routes.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeAll, beforeEach, describe, expect, mock, test } from "bun:test"; +import { Hono } from "hono"; import { getDb } from "../../db/client.js"; import { createSlackRoutes } from "../routes/public/slack.js"; import { ensurePgliteForGatewayTests, resetTestDatabase } from "./helpers/db-setup.js"; @@ -10,6 +11,12 @@ describe("slack routes", () => { let completeSlackOAuthInstall: ReturnType; let handleSlackAppWebhook: ReturnType; let router: ReturnType; + let app: Hono; + // Per-test org id injected into the Hono context — mirrors what + // `lobuApp.use('*', ...)` sets in production (see lobu/gateway.ts). The + // /slack/install + /slack/oauth_callback handlers require a non-empty + // value to scope install state to the initiating tenant. + let sessionOrgId: string | null; beforeAll(async () => { await ensurePgliteForGatewayTests(); @@ -37,6 +44,14 @@ describe("slack routes", () => { completeSlackOAuthInstall, handleSlackAppWebhook, } as any); + + sessionOrgId = "org-default"; + app = new Hono(); + app.use("*", async (c, next) => { + if (sessionOrgId !== null) c.set("organizationId" as never, sessionOrgId); + await next(); + }); + app.route("", router); }); afterEach(() => { @@ -54,7 +69,7 @@ describe("slack routes", () => { }); test("GET /slack/install redirects to Slack OAuth and stores state", async () => { - const response = await router.request("/slack/install"); + const response = await app.request("/slack/install"); expect(response.status).toBe(302); @@ -83,11 +98,20 @@ describe("slack routes", () => { expect(payload.redirectUri).toBe( "https://gateway.example.com/slack/oauth_callback" ); + expect(payload.organizationId).toBe("org-default"); expect(typeof payload.createdAt).toBe("number"); }); + test("GET /slack/install rejects when no session org is bound", async () => { + sessionOrgId = null; + const response = await app.request("/slack/install"); + const body = await response.text(); + expect(response.status).toBe(401); + expect(body).toContain("Sign in to an organization"); + }); + test("GET /slack/oauth_callback rejects invalid state", async () => { - const response = await router.request( + const response = await app.request( "/slack/oauth_callback?code=test-code&state=missing" ); const body = await response.text(); @@ -109,12 +133,13 @@ describe("slack routes", () => { ${sql.json({ createdAt: Date.now(), redirectUri: "https://gateway.example.com/slack/oauth_callback", + organizationId: "org-default", })}, ${expiresAt} ) `; - const response = await router.request( + const response = await app.request( "/slack/oauth_callback?code=test-code&state=test-state" ); const body = await response.text(); @@ -133,8 +158,40 @@ describe("slack routes", () => { expect(remaining.length).toBe(0); }); + test("GET /slack/oauth_callback rejects when callback session org differs from install state", async () => { + const sql = getDb(); + const expiresAt = new Date(Date.now() + 600_000); + await sql` + INSERT INTO oauth_states (id, scope, payload, expires_at) + VALUES ( + 'cross-org-state', + 'slack:oauth:state', + ${sql.json({ + createdAt: Date.now(), + redirectUri: "https://gateway.example.com/slack/oauth_callback", + organizationId: "org-a", + })}, + ${expiresAt} + ) + `; + // Caller signs in to org-b — must be rejected with 403. + sessionOrgId = "org-b"; + const response = await app.request( + "/slack/oauth_callback?code=test-code&state=cross-org-state" + ); + const body = await response.text(); + expect(response.status).toBe(403); + expect(body).toContain("different organization"); + expect(completeSlackOAuthInstall).not.toHaveBeenCalled(); + // State is consumed regardless — replay protection still holds. + const remaining = await sql` + SELECT 1 FROM oauth_states WHERE id = 'cross-org-state' + `; + expect(remaining.length).toBe(0); + }); + test("POST /slack/events forwards requests to the chat manager", async () => { - const response = await router.request("/slack/events", { + const response = await app.request("/slack/events", { method: "POST", headers: { "content-type": "application/json", diff --git a/packages/server/src/gateway/auth/oauth/state-store.ts b/packages/server/src/gateway/auth/oauth/state-store.ts index 72fb7affa..1b7a54a69 100644 --- a/packages/server/src/gateway/auth/oauth/state-store.ts +++ b/packages/server/src/gateway/auth/oauth/state-store.ts @@ -135,6 +135,13 @@ export function createOAuthStateStore( interface SlackInstallStateData { redirectUri: string; + /** + * Active org of the session that initiated the install. The callback + * verifies the callback-side session's active org matches; mismatch + * rejects the install so an OAuth link minted under org A's session can + * never plant a connection into org B. + */ + organizationId: string; } export function createSlackInstallStateStore(): OAuthStateStore { diff --git a/packages/server/src/gateway/connections/chat-instance-manager.ts b/packages/server/src/gateway/connections/chat-instance-manager.ts index 068a927dc..bafb40ce0 100644 --- a/packages/server/src/gateway/connections/chat-instance-manager.ts +++ b/packages/server/src/gateway/connections/chat-instance-manager.ts @@ -93,6 +93,32 @@ function configsEqual( const logger = createLogger("chat-instance-manager"); +/** + * Read `LOBU_CLOUD_MODE` from the env. Truthy values (`1`, `true`, `yes`, + * case-insensitive) enable cloud-mode guardrails that don't apply to + * self-hosters running the same gateway in a single-tenant install. + * + * Re-read on every call so a process running in a test harness can flip + * the flag without restart. Embedded gateways check it on every + * connection-create, which is cold-path enough to skip caching. + */ +function isCloudMode(): boolean { + const raw = process.env.LOBU_CLOUD_MODE; + if (!raw) return false; + const v = raw.trim().toLowerCase(); + return v === "1" || v === "true" || v === "yes"; +} + +/** + * `mode: "polling"` is the only config that forces long-polling regardless + * of whether the gateway has a public webhook URL. `mode: "auto"` resolves + * to webhook on cloud (publicGatewayUrl is always set there), so it's fine + * to allow. Only the explicit polling opt-in is rejected in cloud. + */ +function isPollingTelegramMode(config: { mode?: string }): boolean { + return config.mode === "polling"; +} + const ADAPTER_FACTORIES: Record Promise> = { telegram: async (c) => (await import("@chat-adapter/telegram")).createTelegramAdapter(c), @@ -212,6 +238,24 @@ export class ChatInstanceManager { ); } + // `mode: "polling"` long-polls Telegram's edge from the gateway pod and + // bypasses the per-tenant webhook URL we issue. On Lobu Cloud — where + // the same gateway serves many tenants — that means one org's connection + // can starve every other tenant's webhook delivery (and produces no + // audit trail tied to the inbound HTTP request). Refuse the explicit + // polling opt-in up front; self-hosters (LOBU_CLOUD_MODE unset/0) still + // get polling for tunnel-less dev. `mode: "auto"` is fine — it resolves + // to webhook whenever `publicGatewayUrl` is set, which cloud always has. + if ( + platform === "telegram" && + isCloudMode() && + isPollingTelegramMode(config as { mode?: string }) + ) { + throw new Error( + "Polling mode is not supported in Lobu Cloud — use webhook mode, or self-host." + ); + } + const id = stableId ?? randomUUID().replace(/-/g, "").slice(0, 16); const now = Date.now(); const organizationId = tryGetOrgId() ?? undefined; diff --git a/packages/server/src/gateway/orchestration/base-deployment-manager.ts b/packages/server/src/gateway/orchestration/base-deployment-manager.ts index e2dbf6d5e..860d26b26 100644 --- a/packages/server/src/gateway/orchestration/base-deployment-manager.ts +++ b/packages/server/src/gateway/orchestration/base-deployment-manager.ts @@ -762,7 +762,10 @@ export abstract class BaseDeploymentManager { key, secretRef, deploymentName, - SECRET_PLACEHOLDER_TTL_SECONDS + { + ttlSeconds: SECRET_PLACEHOLDER_TTL_SECONDS, + organizationId: context?.organizationId, + } ); envVars[key] = placeholder; hasSecrets = true; @@ -819,6 +822,7 @@ export abstract class BaseDeploymentManager { teamId, platform, agentId, + organizationId: validated.organizationId, connectionId: typeof platformMetadata?.connectionId === "string" ? platformMetadata.connectionId diff --git a/packages/server/src/gateway/proxy/egress-judge/cache.ts b/packages/server/src/gateway/proxy/egress-judge/cache.ts index 80b486cfa..8552d3278 100644 --- a/packages/server/src/gateway/proxy/egress-judge/cache.ts +++ b/packages/server/src/gateway/proxy/egress-judge/cache.ts @@ -8,9 +8,11 @@ interface Entry { } /** - * Small LRU with absolute TTL. Keyed by `(policyHash, request signature)`, - * so a policy edit invalidates prior verdicts automatically — the hash - * changes, the cache misses. + * Small LRU with absolute TTL. Keyed by `(orgId, policyHash, request + * signature)` — the orgId scopes verdicts to a tenant so org A's "allow" + * for `api.example.com` cannot satisfy org B's identical request, even when + * the composed policy text hashes the same. A policy edit invalidates + * prior verdicts automatically — the hash changes, the cache misses. * * Scale budget: expected to sit in the low thousands of entries. When the * map grows past `maxEntries`, the oldest-touched key is evicted. @@ -25,12 +27,14 @@ export class VerdictCache { ) {} static key(parts: { + orgId: string; policyHash: string; hostname: string; method?: string; path?: string; }): string { return [ + parts.orgId, parts.policyHash, parts.hostname.toLowerCase(), parts.method?.toUpperCase() ?? "", diff --git a/packages/server/src/gateway/proxy/egress-judge/judge.ts b/packages/server/src/gateway/proxy/egress-judge/judge.ts index 89f8d111c..87f087da6 100644 --- a/packages/server/src/gateway/proxy/egress-judge/judge.ts +++ b/packages/server/src/gateway/proxy/egress-judge/judge.ts @@ -103,6 +103,7 @@ export class EgressJudge { rule: ResolvedJudgeRule ): Promise { const cacheKey = VerdictCache.key({ + orgId: request.organizationId, policyHash: rule.policyHash, hostname: request.hostname, method: request.method, diff --git a/packages/server/src/gateway/proxy/egress-judge/types.ts b/packages/server/src/gateway/proxy/egress-judge/types.ts index c1d4d4706..2b42edf21 100644 --- a/packages/server/src/gateway/proxy/egress-judge/types.ts +++ b/packages/server/src/gateway/proxy/egress-judge/types.ts @@ -6,6 +6,13 @@ */ export interface JudgeRequest { agentId: string; + /** + * Owning organization of the worker making this request. The verdict + * cache is scoped by `orgId` so verdicts cannot leak across tenants — + * see `VerdictCache.key`. Empty string is treated as an "unknown org" + * bucket of its own (still isolated from real orgs). + */ + organizationId: string; hostname: string; method?: string; path?: string; diff --git a/packages/server/src/gateway/proxy/http-proxy.ts b/packages/server/src/gateway/proxy/http-proxy.ts index 92bf8fd38..a9ab6673d 100644 --- a/packages/server/src/gateway/proxy/http-proxy.ts +++ b/packages/server/src/gateway/proxy/http-proxy.ts @@ -130,6 +130,7 @@ interface AccessDecision { async function checkDomainAccess( hostname: string, agentId: string | undefined, + organizationId: string | undefined, requestContext?: { method?: string; path?: string } ): Promise { const global = getGlobalConfig(); @@ -177,6 +178,10 @@ async function checkDomainAccess( const decision = await proxyEgressJudge.decide( { agentId, + // `organizationId` may be empty when an older token (minted before + // the org-id pivot) is still in flight. The cache uses it as part + // of the key — empty string and a real UUID never collide. + organizationId: organizationId ?? "", hostname, method: requestContext?.method, path: requestContext?.path, @@ -742,7 +747,11 @@ async function handleConnect( // Check domain access: global config → grant store → LLM egress judge. // TLS CONNECT tunneling means we cannot see the method or path — the // judge decides on hostname alone. - const decision = await checkDomainAccess(hostname, tokenData.agentId); + const decision = await checkDomainAccess( + hostname, + tokenData.agentId, + tokenData.organizationId + ); logAccessDecision( "CONNECT", hostname, @@ -887,7 +896,7 @@ async function handleProxyRequest( // Check domain access: global config → grant store → LLM egress judge. // Plain HTTP: method and path are visible and are passed through to the // judge so policies can reason about specific endpoints. - const decision = await checkDomainAccess(hostname, tokenData.agentId, { + const decision = await checkDomainAccess(hostname, tokenData.agentId, tokenData.organizationId, { method: req.method, path: parsedUrl.pathname + parsedUrl.search, }); diff --git a/packages/server/src/gateway/proxy/secret-proxy.ts b/packages/server/src/gateway/proxy/secret-proxy.ts index 789ae8451..e4ebbff12 100644 --- a/packages/server/src/gateway/proxy/secret-proxy.ts +++ b/packages/server/src/gateway/proxy/secret-proxy.ts @@ -163,6 +163,42 @@ class PlaceholderCache { /** Module-level singleton: gateway has one secret proxy and one mapping cache. */ const placeholderCache = new PlaceholderCache(); +/** + * Resolve a placeholder string (`lobu_secret_` or a prefixed variant) + * to its stored {@link SecretMapping}. Returns `null` if the placeholder is + * malformed, expired, missing, or — when `expectedOrganizationId` is supplied + * — pinned to a different tenant. + * + * Exported for tests so the org-scoping guard can be exercised without + * spinning up the full HTTP proxy. + */ +export function lookupPlaceholderMapping( + placeholder: string, + expectedOrganizationId?: string +): SecretMapping | null { + const prefixIdx = placeholder.indexOf(PLACEHOLDER_PREFIX); + if (prefixIdx === -1) return null; + const uuid = placeholder.slice(prefixIdx + PLACEHOLDER_PREFIX.length); + const mapping = placeholderCache.get(uuid); + if (!mapping) return null; + if ( + expectedOrganizationId && + mapping.organizationId && + mapping.organizationId !== expectedOrganizationId + ) { + logger.warn( + { + mappingAgentId: mapping.agentId, + mappingOrg: mapping.organizationId, + expectedOrg: expectedOrganizationId, + }, + "Placeholder mapping rejected: organization mismatch" + ); + return null; + } + return mapping; +} + function safeDecodePathSegment(value: string | undefined): string | undefined { if (!value) return undefined; try { @@ -174,6 +210,15 @@ function safeDecodePathSegment(value: string | undefined): string | undefined { export interface SecretMapping { agentId: string; + /** + * Owning organization of the agent the placeholder was minted for. + * `lookupPlaceholderMapping()` rejects the lookup when the caller's + * org doesn't match — defense-in-depth against a compromised worker + * presenting another tenant's placeholder. Optional only because + * older mappings minted before the org-id pivot can still be in + * flight; production-minted mappings always set it. + */ + organizationId?: string; envVarName: string; secretRef: SecretRef; deploymentName: string; @@ -288,12 +333,19 @@ export class SecretProxy { * Look up just the SecretMapping (without resolving the secret value) * for a placeholder. Used to verify the calling worker's bound agentId * matches the agentId in the request URL. + * + * If `expectedOrganizationId` is supplied and the stored mapping is + * tagged with a different org, treat it the same as a missing mapping — + * log and return null. This is defense-in-depth on top of the existing + * `mapping.agentId === urlAgentId` check: if a future code path + * resolves placeholders under a different tenant's context (e.g. + * cross-tenant header forwarding), the mismatch here blocks it. */ - private lookupPlaceholderMapping(placeholder: string): SecretMapping | null { - const prefixIdx = placeholder.indexOf(PLACEHOLDER_PREFIX); - if (prefixIdx === -1) return null; - const uuid = placeholder.slice(prefixIdx + PLACEHOLDER_PREFIX.length); - return placeholderCache.get(uuid); + private lookupPlaceholderMapping( + placeholder: string, + expectedOrganizationId?: string + ): SecretMapping | null { + return lookupPlaceholderMapping(placeholder, expectedOrganizationId); } /** @@ -636,13 +688,19 @@ export function generatePlaceholder( envVarName: string, secretRef: SecretRef, deploymentName: string, - ttlSeconds?: number + options?: { ttlSeconds?: number; organizationId?: string } ): string { const uuid = crypto.randomUUID(); storeSecretMapping( uuid, - { agentId, envVarName, secretRef, deploymentName }, - ttlSeconds + { + agentId, + envVarName, + secretRef, + deploymentName, + organizationId: options?.organizationId, + }, + options?.ttlSeconds ); return `${PLACEHOLDER_PREFIX}${uuid}`; } diff --git a/packages/server/src/gateway/routes/public/slack.ts b/packages/server/src/gateway/routes/public/slack.ts index c98863c25..a76e303f2 100644 --- a/packages/server/src/gateway/routes/public/slack.ts +++ b/packages/server/src/gateway/routes/public/slack.ts @@ -1,5 +1,6 @@ import { readFile } from "node:fs/promises"; import { createLogger } from "@lobu/core"; +import type { Context } from "hono"; import { Hono } from "hono"; import { createSlackInstallStateStore } from "../../auth/oauth/state-store.js"; import { @@ -11,6 +12,39 @@ import { resolvePublicUrl } from "../../utils/public-url.js"; const logger = createLogger("slack-routes"); +/** + * Resolve the active organization id for the current request. + * + * Priority: + * 1. `c.get('organizationId')` — set by the lobuApp wrapper after + * `resolveDefaultOrgId(user.id)` (see `lobu/gateway.ts`). This is the + * value Postgres-backed stores read via AsyncLocalStorage, so binding + * install state to it keeps the OAuth flow aligned with where the + * resulting connection row will be written. + * 2. `c.get('session')?.activeOrganizationId` — better-auth's stamped + * active org, used when the wrapper hasn't run (rare; defensive). + * + * Returns `null` if neither is present — caller must reject the request. + */ +function readSessionOrgId(c: Context): string | null { + const fromContext = c.get("organizationId" as never) as + | string + | null + | undefined; + if (typeof fromContext === "string" && fromContext.length > 0) { + return fromContext; + } + const session = c.get("session" as never) as + | { activeOrganizationId?: string | null } + | null + | undefined; + const fromSession = session?.activeOrganizationId; + if (typeof fromSession === "string" && fromSession.length > 0) { + return fromSession; + } + return null; +} + const DEFAULT_SLACK_BOT_SCOPES = [ "app_mentions:read", "assistant:write", @@ -89,13 +123,30 @@ export function createSlackRoutes(manager: ChatInstanceManager): Hono { ); } + // Bind the install to the initiating session's active org. Without this + // an OAuth link minted under org A's session can be opened from org B's + // browser and the resulting connection lands in the wrong tenant. + const sessionOrgId = readSessionOrgId(c); + if (!sessionOrgId) { + return c.html( + renderOAuthErrorPage( + "unauthorized", + "Sign in to an organization before starting Slack install." + ), + 401 + ); + } + const stateStore = createSlackInstallStateStore(); const redirectUri = resolvePublicUrl("/slack/oauth_callback", { configuredUrl: manager.getServices().getPublicGatewayUrl?.(), requestUrl: c.req.url, }); const scopes = await loadSlackBotScopes(); - const state = await stateStore.create({ redirectUri }); + const state = await stateStore.create({ + redirectUri, + organizationId: sessionOrgId, + }); const oauthUrl = new URL("https://slack.com/oauth/v2/authorize"); oauthUrl.searchParams.set("client_id", clientId); @@ -132,6 +183,28 @@ export function createSlackRoutes(manager: ChatInstanceManager): Hono { ); } + // Reject the callback if the session that's completing the install + // belongs to a different org than the one that started it. Prevents + // an attacker who phishes the install link from landing a connection + // in their own org under a victim's authorization. + const callbackOrgId = readSessionOrgId(c); + if (!callbackOrgId || callbackOrgId !== oauthState.organizationId) { + logger.warn( + { + stateOrg: oauthState.organizationId, + callbackOrg: callbackOrgId ?? null, + }, + "Rejecting Slack OAuth callback: session org does not match install state" + ); + return c.html( + renderOAuthErrorPage( + "org_mismatch", + "This Slack install link was started in a different organization. Sign in to that organization and try again." + ), + 403 + ); + } + try { const result = await manager.completeSlackOAuthInstall( c.req.raw, diff --git a/packages/server/src/gateway/services/agent-threads.ts b/packages/server/src/gateway/services/agent-threads.ts index f12e7df87..b332fb615 100644 --- a/packages/server/src/gateway/services/agent-threads.ts +++ b/packages/server/src/gateway/services/agent-threads.ts @@ -69,7 +69,7 @@ export async function createThreadForAgent( args: CreateThreadForAgentArgs ): Promise { const { sessionManager } = deps; - const { agentId, reason, externalThreadId } = args; + const { agentId, organizationId, reason, externalThreadId } = args; const userId = args.userId || args.createdByUserId || agentId; const threadId = externalThreadId || randomUUID(); @@ -80,6 +80,7 @@ export async function createThreadForAgent( const token = generateWorkerToken(agentId, conversationId, deploymentName, { channelId, agentId, + organizationId, platform: "api", sessionKey: userId, }); From c9a989f070a54a55fb75eeda0dfe411505c72167 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Emre=20Kabakc=C4=B1?= Date: Mon, 18 May 2026 02:02:20 +0100 Subject: [PATCH 2/3] =?UTF-8?q?fix(server):=20address=20pi=20review=20on?= =?UTF-8?q?=20PR=20#836=20=E2=80=94=20plumb=20org=20through=20downstream?= =?UTF-8?q?=20callers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses six findings on the prior commit. Three were critical because the new org parameter sat unused at the lookup layer — the isolation guarantee the commit advertised never reached the call sites: 1. **secret-proxy lookupPlaceholderMapping was dead code.** Wired an `agentOrgResolver` (DB lookup keyed by URL agentId) into SecretProxy and pass `expectedOrganizationId` through `forward()` → `lookupPlaceholderMapping`. Worker tokens carrying `organizationId` are also extracted (header/query/bearer) for a signed signal that beats the DB lookup. 2. **checkDomainAccess dropped org on grant-store calls.** Pass `tokenData.organizationId` through to `GrantStore.isDenied/hasGrant`. Added `setProxyGrantStore` so tests can install a real store and exercise the call site. PolicyStore is now gated on `organizationId` being present — falling through to an unkeyed lookup would let another tenant's policy decide our verdict. 3. **PolicyStore is now keyed by `(organizationId, agentId)`.** Last sync no longer wins across tenants; `policyHash` includes the org id so the verdict cache scoping in #836 stays meaningful. 4. **Telegram cloud-mode guard also runs in `initialize()`.** Persisted `mode: "polling"` rows are marked errored at boot (under their own org context) instead of silently starting. `addConnection()` still rejects the same config at create time. 5. **Slack `/slack/install` self-host fallback.** When neither `c.get('organizationId')` nor `session.activeOrganizationId` is set, look up the org table — if exactly one org row exists, use it. Otherwise reject. Standalone deployments without the lobuApp wrapper stay usable; multi-tenant deployments stay strict. 6. **OAuth callback peek-before-consume.** Added `OAuthStateStore.peek()`. The Slack callback now validates the session org against the state's org *before* burning the row; a legitimate caller can retry after a cross-org or unauthenticated hit instead of restarting OAuth. Adds `multi-tenant-isolation-reproducers.test.ts` with 10 red→green tests that pin findings 1–4 (finding 6 is covered by an updated assertion in slack-routes.test.ts that the row is preserved). --- .../__tests__/base-deployment-grants.test.ts | 6 +- .../__tests__/http-proxy-judge.test.ts | 3 +- ...multi-tenant-isolation-reproducers.test.ts | 445 ++++++++++++++++++ .../gateway/__tests__/policy-store.test.ts | 54 +-- .../gateway/__tests__/proxy-hardening.test.ts | 69 +-- .../gateway/__tests__/slack-routes.test.ts | 6 +- .../src/gateway/auth/oauth/state-store.ts | 23 + .../connections/chat-instance-manager.ts | 40 ++ .../orchestration/base-deployment-manager.ts | 19 +- .../__tests__/policy-store.test.ts | 72 +-- .../src/gateway/permissions/policy-store.ts | 49 +- .../server/src/gateway/proxy/http-proxy.ts | 40 +- .../server/src/gateway/proxy/secret-proxy.ts | 122 ++++- .../server/src/gateway/routes/public/slack.ts | 79 +++- .../src/gateway/services/core-services.ts | 21 + 15 files changed, 904 insertions(+), 144 deletions(-) create mode 100644 packages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.ts diff --git a/packages/server/src/gateway/__tests__/base-deployment-grants.test.ts b/packages/server/src/gateway/__tests__/base-deployment-grants.test.ts index e8969a701..9138d9b44 100644 --- a/packages/server/src/gateway/__tests__/base-deployment-grants.test.ts +++ b/packages/server/src/gateway/__tests__/base-deployment-grants.test.ts @@ -160,7 +160,7 @@ describe("BaseDeploymentManager.syncNetworkConfigGrants", () => { }) ); - let resolved = policyStore.resolve("agent-1", "api.example.com"); + let resolved = policyStore.resolve("test-org", "agent-1", "api.example.com"); expect(resolved?.policy).toContain("initial policy"); expect(resolved?.policy).toContain("operator policy"); @@ -173,12 +173,12 @@ describe("BaseDeploymentManager.syncNetworkConfigGrants", () => { }) ); - resolved = policyStore.resolve("agent-1", "api.example.com"); + resolved = policyStore.resolve("test-org", "agent-1", "api.example.com"); expect(resolved?.policy).toContain("updated policy"); expect(resolved?.policy).not.toContain("initial policy"); await barebones.syncNetworkConfigGrants(buildPayload({})); - expect(policyStore.resolve("agent-1", "api.example.com")).toBeUndefined(); + expect(policyStore.resolve("test-org", "agent-1", "api.example.com")).toBeUndefined(); }); test("skips redundant writes when the pattern set has not changed", async () => { diff --git a/packages/server/src/gateway/__tests__/http-proxy-judge.test.ts b/packages/server/src/gateway/__tests__/http-proxy-judge.test.ts index 61157b6bb..4e0ade4f0 100644 --- a/packages/server/src/gateway/__tests__/http-proxy-judge.test.ts +++ b/packages/server/src/gateway/__tests__/http-proxy-judge.test.ts @@ -52,7 +52,7 @@ beforeAll(async () => { // `example.com` is used for the allow-path HTTP tests because its real // server closes the socket after responding — tests that use hosts with // keep-alive (api.github.com, etc.) hang the raw socket reader. - policyStore.set("agent-a", { + policyStore.set("org-a", "agent-a", { judgedDomains: [ { domain: "example.com" }, { domain: ".slack.com", judge: "strict" }, @@ -86,6 +86,7 @@ function createValidToken(deploymentName: string): string { channelId: "test-channel", platform: "test", agentId: "agent-a", + organizationId: "org-a", }); } diff --git a/packages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.ts b/packages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.ts new file mode 100644 index 000000000..e7349aa29 --- /dev/null +++ b/packages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.ts @@ -0,0 +1,445 @@ +/** + * Red→green reproducers for the four critical multi-tenant isolation gaps + * surfaced on PR #836: + * + * - Finding 1: secret-proxy `lookupPlaceholderMapping` was dead-code — + * no production call site passed `expectedOrganizationId`, so a worker + * bound to org-A could resolve its placeholder under org-B's URL. + * - Finding 2: `checkDomainAccess()` had `organizationId` on its frame + * but never threaded it into `GrantStore.isDenied/hasGrant`. The store + * fell back to ALS (empty in the raw HTTP proxy hot path) and matched + * grants by `agent_id` alone — cross-org leakage when an agent id is + * reused. + * - Finding 3: `PolicyStore` was keyed by `agentId` alone, so the last + * `set()` across orgs won. Cache scoping became theatre because the + * policy fed into the verdict was already wrong. + * - Finding 6 — peek-before-consume — is covered by `slack-routes.test.ts` + * ("rejects when callback session org differs from install state" + * asserts the row is preserved for a legitimate retry). + * + * Each test in this file asserts the post-fix behaviour. The PR description + * pastes the corresponding pre-fix `bun test` output so the reproducer + * doubles as a regression gate — flip a single fix line and the listed + * assertion fails. + */ + +import { afterAll, beforeAll, beforeEach, describe, expect, test } from "bun:test"; +import { createBuiltinSecretRef } from "@lobu/core"; +import { GrantStore } from "../permissions/grant-store.js"; +import { PolicyStore } from "../permissions/policy-store.js"; +import { + __resetPlaceholderCacheForTests, + generatePlaceholder, + lookupPlaceholderMapping, + SecretProxy, +} from "../proxy/secret-proxy.js"; +import type { SecretStore } from "../secrets/index.js"; +import { + ensurePgliteForGatewayTests, + resetTestDatabase, + seedAgentRow, +} from "./helpers/db-setup.js"; + +// ─── Finding 3: PolicyStore cross-tenant clobbering ────────────────────────── + +describe("[finding 3] PolicyStore is keyed by (orgId, agentId)", () => { + test("org A's policy survives org B's set under the same agent id", () => { + const store = new PolicyStore(); + // Org A sets a policy for `shared-agent-id`. + store.set("org-a", "shared-agent-id", { + judgedDomains: [{ domain: "api.example.com" }], + judges: { default: "ORG A: deny by default" }, + }); + // Org B reuses the same agent id — under the old keying this overwrote + // org A's bundle. Now both must coexist. + store.set("org-b", "shared-agent-id", { + judgedDomains: [{ domain: "api.example.com" }], + judges: { default: "ORG B: allow reads" }, + }); + + const aResolved = store.resolve("org-a", "shared-agent-id", "api.example.com"); + const bResolved = store.resolve("org-b", "shared-agent-id", "api.example.com"); + + expect(aResolved?.policy).toBe("ORG A: deny by default"); + expect(bResolved?.policy).toBe("ORG B: allow reads"); + // policyHash diverges so the verdict cache cannot collide either. + expect(aResolved?.policyHash).not.toBe(bResolved?.policyHash); + }); + + test("resolve refuses cross-org reads — no fall-through to a sibling tenant's bundle", () => { + const store = new PolicyStore(); + store.set("org-a", "agent-1", { + judgedDomains: [{ domain: "api.example.com" }], + judges: { default: "ORG A only" }, + }); + // Org B has no bundle for `agent-1` — must return undefined, not + // org A's bundle. + expect( + store.resolve("org-b", "agent-1", "api.example.com") + ).toBeUndefined(); + }); + + test("clear(orgA, agentId) does not affect orgB's bundle for the same agent id", () => { + const store = new PolicyStore(); + store.set("org-a", "shared", { + judgedDomains: [{ domain: "x.com" }], + judges: { default: "A" }, + }); + store.set("org-b", "shared", { + judgedDomains: [{ domain: "x.com" }], + judges: { default: "B" }, + }); + store.clear("org-a", "shared"); + expect(store.resolve("org-a", "shared", "x.com")).toBeUndefined(); + expect(store.resolve("org-b", "shared", "x.com")?.policy).toBe("B"); + }); +}); + +// ─── Finding 1: secret-proxy placeholder cross-org leak ────────────────────── + +describe("[finding 1] lookupPlaceholderMapping enforces caller's expected org", () => { + beforeEach(() => { + __resetPlaceholderCacheForTests(); + }); + + test("org-A placeholder resolved under org-B context returns null", () => { + // Org A mints a placeholder for one of its agents. + const placeholder = generatePlaceholder( + "agent-1", + "API_KEY", + createBuiltinSecretRef("deployments/agent-1/API_KEY"), + "deploy-A", + { organizationId: "org-a" } + ); + // A caller bound to org B presents the placeholder. Pre-fix the lookup + // had no `expectedOrganizationId` plumbed through any production call + // site, so the mapping resolved and org B could spend org A's + // upstream credential. Post-fix: null. + expect(lookupPlaceholderMapping(placeholder, "org-b")).toBeNull(); + }); + + test("a matching expected org still resolves cleanly", () => { + const placeholder = generatePlaceholder( + "agent-1", + "API_KEY", + createBuiltinSecretRef("deployments/agent-1/API_KEY"), + "deploy-A", + { organizationId: "org-a" } + ); + const mapping = lookupPlaceholderMapping(placeholder, "org-a"); + expect(mapping?.agentId).toBe("agent-1"); + expect(mapping?.organizationId).toBe("org-a"); + }); + + test("SecretProxy.forward rejects an org-A placeholder used on an org-B agent's URL", async () => { + // Mint a placeholder for org A's `agent-A1`. Pre-fix, no production call + // site supplied `expectedOrganizationId`, so `lookupPlaceholderMapping` + // returned the org-A mapping even when the URL named an org-B agent — + // the `mapping.agentId === urlAgentId` check downstream then tripped at + // 403, but only because the agent ids differed. If two orgs happened + // to use the same `agentId` (per-org-unique on paper, but a stale dump + // or hand-edit can violate this), no 403 fires and the credential + // leaks. The proxy's `agentOrgResolver` is the independent source of + // truth this test exercises end-to-end. + const placeholder = generatePlaceholder( + "shared-id", + "OPENAI_API_KEY", + createBuiltinSecretRef("deployments/orgA/shared-id/OPENAI_API_KEY"), + "deploy-A", + { organizationId: "org-a" } + ); + + const stubStore: SecretStore = { get: async () => "real-secret-A" }; + const proxy = new SecretProxy( + { defaultUpstreamUrl: "https://upstream.example.com" }, + stubStore + ); + proxy.registerUpstream( + { slug: "openai", upstreamBaseUrl: "https://api.openai.example.com" }, + "openai" + ); + // Wire an `agentOrgResolver` that says `shared-id` belongs to org B + // when looked up via the URL. The mapping's org is `org-a` → mismatch. + proxy.setAgentOrgResolver(async () => "org-b"); + + let upstreamCalled = false; + const originalFetch = globalThis.fetch; + globalThis.fetch = async () => { + upstreamCalled = true; + return new Response("{}", { status: 200 }); + }; + + try { + const res = await proxy + .getApp() + .request("/api/proxy/openai/a/shared-id/v1/chat/completions", { + method: "POST", + headers: { + "content-type": "application/json", + authorization: `Bearer ${placeholder}`, + }, + body: JSON.stringify({ prompt: "leak" }), + }); + expect(res.status).toBe(401); + expect(upstreamCalled).toBe(false); + } finally { + globalThis.fetch = originalFetch; + } + }); +}); + +// ─── Finding 2: GrantStore agent-id collision across orgs ──────────────────── + +describe("[finding 2] GrantStore queries scope to caller's organization id", () => { + beforeAll(async () => { + await ensurePgliteForGatewayTests(); + }); + + beforeEach(async () => { + await resetTestDatabase(); + // Seed both orgs and the shared agent id under each — the grants table + // has a FK on `(organization_id, agent_id)` and needs both rows in + // `agents` to exist before we can grant. + await seedAgentRow("shared-agent-id", { organizationId: "org-a" }); + await seedAgentRow("shared-agent-id", { organizationId: "org-b" }); + }); + + test("org A's grant for `shared-agent-id` is invisible to org B's lookup", async () => { + const store = new GrantStore(); + // Org A grants `api.example.com` to its agent. + await store.grant( + "shared-agent-id", + "api.example.com", + null, + false, + "org-a" + ); + // Org B reuses the same agent id (it's per-org-unique on paper; this + // tests that a buggy seed or hand-edited row cannot leak across orgs). + // Without org plumbing the WHERE clause would lose `organization_id` + // and find org A's row. + const orgBSeesGrant = await store.hasGrant( + "shared-agent-id", + "api.example.com", + "org-b" + ); + expect(orgBSeesGrant).toBe(false); + + // Org A still sees its own grant. + expect( + await store.hasGrant("shared-agent-id", "api.example.com", "org-a") + ).toBe(true); + }); + + test("org A's DENY grant for `shared-agent-id` does not block org B", async () => { + const store = new GrantStore(); + await store.grant( + "shared-agent-id", + "api.example.com", + null, + true, // denied + "org-a" + ); + // Org B's isDenied check must see no row. + const orgBDenied = await store.isDenied( + "shared-agent-id", + "api.example.com", + "org-b" + ); + expect(orgBDenied).toBe(false); + + // Org A's isDenied sees its own denial. + expect( + await store.isDenied("shared-agent-id", "api.example.com", "org-a") + ).toBe(true); + }); + + test("HTTP proxy's checkDomainAccess passes the token's orgId into GrantStore", async () => { + // End-to-end exercise of the call-site plumbing: install a real + // GrantStore in the http-proxy, grant `api.example.com` to org A's + // copy of `shared-agent-id`, then hit the proxy with an org-B worker + // token. Pre-fix, the call site dropped the `organizationId` argument + // and the WHERE clause matched org A's grant (the only one with that + // agent id). Post-fix, the predicate now scopes by org and the request + // is blocked. + const { generateWorkerToken } = await import("@lobu/core"); + const { + __testOnly, + setProxyGrantStore, + startHttpProxy, + stopHttpProxy, + } = await import("../proxy/http-proxy.js"); + const crypto = await import("node:crypto"); + const net = await import("node:net"); + + const TEST_ENCRYPTION_KEY = crypto.randomBytes(32).toString("base64"); + process.env.ENCRYPTION_KEY = TEST_ENCRYPTION_KEY; + process.env.WORKER_ALLOWED_DOMAINS = ""; // deny-all globally → grant path + __testOnly.reset(); + + const store = new GrantStore(); + await store.grant( + "shared-agent-id", + "api.example.com", + null, + false, + "org-a" + ); + setProxyGrantStore(store); + + const proxyPort = 10000 + Math.floor(Math.random() * 50000); + const proxyServer = await startHttpProxy(proxyPort, "127.0.0.1"); + + try { + // Mint an org-B token claiming `shared-agent-id`. + const token = generateWorkerToken( + "test-user", + "test-conv", + "deploy-B", + { + channelId: "test-channel", + platform: "test", + agentId: "shared-agent-id", + organizationId: "org-b", + } + ); + const auth = `Basic ${Buffer.from(`deploy-B:${token}`).toString("base64")}`; + + // Fire a raw HTTP request through the proxy targeting api.example.com. + // We can't fetch() with HTTP_PROXY in bun-test, so we hand-roll the + // proxy request and parse the status line out of the raw bytes. + const status = await new Promise((resolve, reject) => { + const socket = new net.Socket(); + socket.connect(proxyPort, "127.0.0.1", () => { + socket.write( + `GET http://api.example.com/v1/x HTTP/1.1\r\n` + + `Host: api.example.com\r\n` + + `Proxy-Authorization: ${auth}\r\n` + + `Connection: close\r\n\r\n` + ); + }); + let data = ""; + let resolved = false; + const tryParse = () => { + if (resolved) return; + const idx = data.indexOf("\r\n"); + if (idx === -1) return; + const m = data.substring(0, idx).match(/HTTP\/\d\.\d (\d+)/); + if (!m) return; + resolved = true; + socket.destroy(); + resolve(Number(m[1])); + }; + socket.on("data", (chunk: Buffer) => { + data += chunk.toString(); + tryParse(); + }); + socket.on("end", () => { + if (!resolved) { + resolved = true; + resolve(0); + } + }); + socket.on("error", (err) => { + if (!resolved) reject(err); + }); + socket.setTimeout(3000, () => { + if (!resolved) { + resolved = true; + socket.destroy(); + resolve(0); + } + }); + }); + + // Pre-fix: 200 — org A's grant matched because no org was passed. + // Post-fix: 403 — grant lookup scoped to org B finds nothing, + // global allowlist denies, judge has no rule for this agent. + expect(status).toBe(403); + } finally { + await stopHttpProxy(proxyServer); + __testOnly.reset(); + delete process.env.ENCRYPTION_KEY; + delete process.env.WORKER_ALLOWED_DOMAINS; + } + }); + + afterAll(async () => { + await resetTestDatabase(); + }); +}); + +// ─── Finding 4: Telegram cloud-mode polling guard at boot ──────────────────── + +describe("[finding 4] ChatInstanceManager.initialize refuses Telegram polling rows in cloud", () => { + beforeAll(async () => { + await ensurePgliteForGatewayTests(); + }); + + beforeEach(async () => { + await resetTestDatabase(); + await seedAgentRow("agent-1", { organizationId: "test-org" }); + }); + + test("a persisted `mode: polling` Telegram row is errored out at boot, not booted", async () => { + const originalKey = process.env.ENCRYPTION_KEY; + const originalCloud = process.env.LOBU_CLOUD_MODE; + process.env.ENCRYPTION_KEY = + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa="; + process.env.LOBU_CLOUD_MODE = "1"; + + try { + const { orgContext } = await import("../../lobu/stores/org-context.js"); + const { createPostgresAgentConnectionStore } = await import( + "../../lobu/stores/postgres-stores.js" + ); + const connectionStore = createPostgresAgentConnectionStore(); + + await orgContext.run({ organizationId: "test-org" }, async () => { + await connectionStore.saveConnection({ + id: "telegram-poll-1", + platform: "telegram", + agentId: "agent-1", + config: { + platform: "telegram", + botToken: "111:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", + mode: "polling", + }, + settings: { allowGroups: true }, + metadata: {}, + status: "active", + createdAt: Date.now(), + updatedAt: Date.now(), + }); + }); + + const mod = await import("../connections/chat-instance-manager.js"); + const services = { + getQueue: () => ({}), + getPublicGatewayUrl: () => "https://gw.example.com", + getSecretStore: () => ({ get: async () => null, put: async () => "" }), + getConnectionStore: () => connectionStore, + getChannelBindingService: () => ({ getBinding: async () => null }), + } as any; + const manager = new mod.ChatInstanceManager() as any; + // initialize() consults isCloudMode + isPollingTelegramMode pre- + // startInstance; the row must end up `status='error'` without a + // running instance. + await manager.initialize(services); + + const stored = await orgContext.run( + { organizationId: "test-org" }, + () => connectionStore.getConnection("telegram-poll-1") + ); + expect(stored?.status).toBe("error"); + expect(stored?.errorMessage ?? "").toContain("Polling mode"); + expect(manager.instances.has("telegram-poll-1")).toBe(false); + } finally { + if (originalKey !== undefined) process.env.ENCRYPTION_KEY = originalKey; + else delete process.env.ENCRYPTION_KEY; + if (originalCloud !== undefined) + process.env.LOBU_CLOUD_MODE = originalCloud; + else delete process.env.LOBU_CLOUD_MODE; + } + }); +}); diff --git a/packages/server/src/gateway/__tests__/policy-store.test.ts b/packages/server/src/gateway/__tests__/policy-store.test.ts index d82e12ed3..7ee7f0b23 100644 --- a/packages/server/src/gateway/__tests__/policy-store.test.ts +++ b/packages/server/src/gateway/__tests__/policy-store.test.ts @@ -4,16 +4,16 @@ import { buildPolicyBundle, PolicyStore } from "../permissions/policy-store.js"; describe("PolicyStore.resolve", () => { test("returns undefined when no bundle is set", () => { const store = new PolicyStore(); - expect(store.resolve("agent-a", "api.github.com")).toBeUndefined(); + expect(store.resolve("org-a", "agent-a","api.github.com")).toBeUndefined(); }); test("matches an exact domain rule and composes the policy", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [{ domain: "api.github.com" }], judges: { default: "Only allow read-only GET requests." }, }); - const resolved = store.resolve("agent-a", "api.github.com"); + const resolved = store.resolve("org-a", "agent-a","api.github.com"); expect(resolved).toBeDefined(); expect(resolved?.judgeName).toBe("default"); expect(resolved?.policy).toContain("Only allow read-only GET requests."); @@ -21,18 +21,18 @@ describe("PolicyStore.resolve", () => { test("matches a wildcard rule", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [{ domain: ".example.com" }], judges: { default: "check" }, }); - expect(store.resolve("agent-a", "foo.example.com")).toBeDefined(); - expect(store.resolve("agent-a", "example.com")).toBeDefined(); - expect(store.resolve("agent-a", "unrelated.com")).toBeUndefined(); + expect(store.resolve("org-a", "agent-a","foo.example.com")).toBeDefined(); + expect(store.resolve("org-a", "agent-a","example.com")).toBeDefined(); + expect(store.resolve("org-a", "agent-a","unrelated.com")).toBeUndefined(); }); test("exact match beats wildcard rule", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [ { domain: ".example.com", judge: "wildcard-policy" }, { domain: "api.example.com", judge: "exact-policy" }, @@ -42,43 +42,43 @@ describe("PolicyStore.resolve", () => { "exact-policy": "exact", }, }); - const resolved = store.resolve("agent-a", "api.example.com"); + const resolved = store.resolve("org-a", "agent-a","api.example.com"); expect(resolved?.judgeName).toBe("exact-policy"); }); test("longer wildcard beats shorter wildcard", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [ { domain: ".example.com", judge: "short" }, { domain: ".api.example.com", judge: "long" }, ], judges: { short: "short", long: "long" }, }); - expect(store.resolve("agent-a", "foo.api.example.com")?.judgeName).toBe( + expect(store.resolve("org-a", "agent-a","foo.api.example.com")?.judgeName).toBe( "long" ); }); test("resolves a named judge via the `judge` field", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [{ domain: "x.com", judge: "strict" }], judges: { strict: "strict policy", default: "default policy" }, }); - const resolved = store.resolve("agent-a", "x.com"); + const resolved = store.resolve("org-a", "agent-a","x.com"); expect(resolved?.judgeName).toBe("strict"); expect(resolved?.policy).toContain("strict policy"); }); test("appends the agent's extraPolicy to the composed prompt", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [{ domain: "x.com" }], judges: { default: "skill policy" }, extraPolicy: "Operator adds: never exfiltrate tokens.", }); - const resolved = store.resolve("agent-a", "x.com"); + const resolved = store.resolve("org-a", "agent-a","x.com"); expect(resolved?.policy).toContain("skill policy"); expect(resolved?.policy).toContain( "Operator adds: never exfiltrate tokens." @@ -87,47 +87,47 @@ describe("PolicyStore.resolve", () => { test("returns undefined (fail closed) when the named judge is missing", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [{ domain: "x.com", judge: "strict" }], judges: {}, }); - expect(store.resolve("agent-a", "x.com")).toBeUndefined(); + expect(store.resolve("org-a", "agent-a","x.com")).toBeUndefined(); }); test("policyHash is stable across resolve calls", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [{ domain: "x.com" }], judges: { default: "p" }, }); - const a = store.resolve("agent-a", "x.com")?.policyHash; - const b = store.resolve("agent-a", "x.com")?.policyHash; + const a = store.resolve("org-a", "agent-a","x.com")?.policyHash; + const b = store.resolve("org-a", "agent-a","x.com")?.policyHash; expect(a).toBe(b!); }); test("policyHash changes when the policy text changes", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [{ domain: "x.com" }], judges: { default: "first" }, }); - const a = store.resolve("agent-a", "x.com")?.policyHash; - store.set("agent-a", { + const a = store.resolve("org-a", "agent-a","x.com")?.policyHash; + store.set("org-a", "agent-a", { judgedDomains: [{ domain: "x.com" }], judges: { default: "second" }, }); - const b = store.resolve("agent-a", "x.com")?.policyHash; + const b = store.resolve("org-a", "agent-a","x.com")?.policyHash; expect(a).not.toBe(b); }); test("clear removes the bundle", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-a", "agent-a", { judgedDomains: [{ domain: "x.com" }], judges: { default: "p" }, }); - store.clear("agent-a"); - expect(store.resolve("agent-a", "x.com")).toBeUndefined(); + store.clear("org-a", "agent-a"); + expect(store.resolve("org-a", "agent-a","x.com")).toBeUndefined(); }); }); diff --git a/packages/server/src/gateway/__tests__/proxy-hardening.test.ts b/packages/server/src/gateway/__tests__/proxy-hardening.test.ts index d65c19bf6..3c13af088 100644 --- a/packages/server/src/gateway/__tests__/proxy-hardening.test.ts +++ b/packages/server/src/gateway/__tests__/proxy-hardening.test.ts @@ -49,11 +49,16 @@ function makeBasicAuth(username: string, password: string): string { return `Basic ${Buffer.from(`${username}:${password}`).toString("base64")}`; } -function createToken(deploymentName: string, agentId?: string): string { +function createToken( + deploymentName: string, + agentId?: string, + organizationId: string = "org-1" +): string { return generateWorkerToken("test-user", "test-conv", deploymentName, { channelId: "test-channel", platform: "test", ...(agentId ? { agentId } : {}), + organizationId, }); } @@ -511,7 +516,7 @@ describe("CRLF injection prevention in judge-provided reason", () => { process.env.WORKER_ALLOWED_DOMAINS = ""; __testOnly.reset(); - policyStore.set("agent-crlf", { + policyStore.set("org-1", "agent-crlf", { judgedDomains: [{ domain: "example.com" }], judges: { default: "test policy" }, }); @@ -674,18 +679,18 @@ describe("VerdictCache — key independence", () => { // automatically because the key changes. const store = new PolicyStore(); - store.set("agent-x", { + store.set("org-1", "agent-x", { judgedDomains: [{ domain: "example.com" }], judges: { default: "allow reads" }, }); - const without = store.resolve("agent-x", "example.com"); + const without = store.resolve("org-1", "agent-x","example.com"); - store.set("agent-x", { + store.set("org-1", "agent-x", { judgedDomains: [{ domain: "example.com" }], judges: { default: "allow reads" }, extraPolicy: "Never send PII", }); - const withExtra = store.resolve("agent-x", "example.com"); + const withExtra = store.resolve("org-1", "agent-x","example.com"); expect(without).toBeDefined(); expect(withExtra).toBeDefined(); @@ -698,17 +703,17 @@ describe("VerdictCache — key independence", () => { const store = new PolicyStore(); const samePolicy = "allow all reads"; - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: "example.com" }], judges: { default: samePolicy }, }); - store.set("agent-b", { + store.set("org-1", "agent-b", { judgedDomains: [{ domain: "example.com" }], judges: { default: samePolicy }, }); - const a = store.resolve("agent-a", "example.com"); - const b = store.resolve("agent-b", "example.com"); + const a = store.resolve("org-1", "agent-a", "example.com"); + const b = store.resolve("org-1", "agent-b", "example.com"); expect(a?.policyHash).toBeDefined(); expect(b?.policyHash).toBeDefined(); @@ -777,7 +782,7 @@ describe("CircuitBreaker — state transitions", () => { describe("PolicyStore.resolve — edge cases", () => { test("exact match takes precedence over a wildcard", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [ { domain: "api.example.com", judge: "exact-judge" }, { domain: ".example.com", judge: "wildcard-judge" }, @@ -788,13 +793,13 @@ describe("PolicyStore.resolve — edge cases", () => { }, }); - const resolved = store.resolve("agent-a", "api.example.com"); + const resolved = store.resolve("org-1", "agent-a", "api.example.com"); expect(resolved?.judgeName).toBe("exact-judge"); }); test("longer wildcard takes precedence over a shorter wildcard", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [ { domain: ".api.example.com", judge: "long-judge" }, { domain: ".example.com", judge: "short-judge" }, @@ -805,96 +810,96 @@ describe("PolicyStore.resolve — edge cases", () => { }, }); - const resolved = store.resolve("agent-a", "v2.api.example.com"); + const resolved = store.resolve("org-1", "agent-a", "v2.api.example.com"); expect(resolved?.judgeName).toBe("long-judge"); }); test("unmatched hostname returns undefined", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: "example.com" }], judges: { default: "test" }, }); - expect(store.resolve("agent-a", "other.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-a", "other.com")).toBeUndefined(); }); test("agentId isolation: agent-a rules do not leak to agent-b", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: "example.com" }], judges: { default: "agent-a policy" }, }); - expect(store.resolve("agent-b", "example.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-b", "example.com")).toBeUndefined(); }); test("agent with no bundle returns undefined for all hostnames", () => { const store = new PolicyStore(); - expect(store.resolve("no-such-agent", "example.com")).toBeUndefined(); + expect(store.resolve("org-1", "no-such-agent", "example.com")).toBeUndefined(); }); test("rule without explicit judge name resolves to the 'default' judge", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: "example.com" }], // no `judge` field judges: { default: "default policy text" }, }); - const resolved = store.resolve("agent-a", "example.com"); + const resolved = store.resolve("org-1", "agent-a", "example.com"); expect(resolved?.judgeName).toBe("default"); expect(resolved?.policy).toContain("default policy text"); }); test("rule referencing a missing judge name returns undefined (fail closed)", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: "example.com", judge: "nonexistent" }], judges: { default: "default policy" }, }); // 'nonexistent' not in judges map → fails closed (returns undefined) - expect(store.resolve("agent-a", "example.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-a", "example.com")).toBeUndefined(); }); test("clear removes the agent's bundle so resolve returns undefined", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: "example.com" }], judges: { default: "policy" }, }); - store.clear("agent-a"); - expect(store.resolve("agent-a", "example.com")).toBeUndefined(); + store.clear("org-1", "agent-a"); + expect(store.resolve("org-1", "agent-a", "example.com")).toBeUndefined(); }); test("wildcard .example.com matches example.com itself (root domain)", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: ".example.com" }], judges: { default: "policy" }, }); - expect(store.resolve("agent-a", "example.com")).toBeDefined(); + expect(store.resolve("org-1", "agent-a", "example.com")).toBeDefined(); }); test("wildcard .example.com does NOT match evilexample.com", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: ".example.com" }], judges: { default: "policy" }, }); - expect(store.resolve("agent-a", "evilexample.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-a", "evilexample.com")).toBeUndefined(); }); test("resolve is case-insensitive for hostname", () => { const store = new PolicyStore(); - store.set("agent-a", { + store.set("org-1", "agent-a", { judgedDomains: [{ domain: "example.com" }], judges: { default: "policy" }, }); - expect(store.resolve("agent-a", "EXAMPLE.COM")).toBeDefined(); + expect(store.resolve("org-1", "agent-a", "EXAMPLE.COM")).toBeDefined(); }); }); diff --git a/packages/server/src/gateway/__tests__/slack-routes.test.ts b/packages/server/src/gateway/__tests__/slack-routes.test.ts index 7ef732359..1efd85053 100644 --- a/packages/server/src/gateway/__tests__/slack-routes.test.ts +++ b/packages/server/src/gateway/__tests__/slack-routes.test.ts @@ -183,11 +183,13 @@ describe("slack routes", () => { expect(response.status).toBe(403); expect(body).toContain("different organization"); expect(completeSlackOAuthInstall).not.toHaveBeenCalled(); - // State is consumed regardless — replay protection still holds. + // State is preserved for the legitimate caller to retry (peek-before- + // consume). The previous behavior burned the row on every failed + // org check and forced the user to restart the OAuth flow. const remaining = await sql` SELECT 1 FROM oauth_states WHERE id = 'cross-org-state' `; - expect(remaining.length).toBe(0); + expect(remaining.length).toBe(1); }); test("POST /slack/events forwards requests to the chat manager", async () => { diff --git a/packages/server/src/gateway/auth/oauth/state-store.ts b/packages/server/src/gateway/auth/oauth/state-store.ts index 1b7a54a69..e05fd58da 100644 --- a/packages/server/src/gateway/auth/oauth/state-store.ts +++ b/packages/server/src/gateway/auth/oauth/state-store.ts @@ -93,6 +93,29 @@ export class OAuthStateStore { return stateData; } + /** + * Read the state payload without consuming it. Used when the caller needs + * to validate side-channel context (e.g. that the callback session's org + * matches the state's org) before atomically burning the install link — + * without this, any failed-validation hit would force the user to restart + * the OAuth flow even though the state is still otherwise valid. + * + * The row is left intact; callers must call `consume()` themselves once + * validation passes (or rely on the TTL sweep if validation fails). + */ + async peek(state: string): Promise<(T & { createdAt: number }) | null> { + const sql = getDb(); + const rows = await sql` + SELECT payload FROM oauth_states + WHERE id = ${state} + AND scope = ${this.keyPrefix} + AND expires_at > now() + LIMIT 1 + `; + if (rows.length === 0) return null; + return (rows[0] as { payload: T & { createdAt: number } }).payload; + } + /** * Generate a cryptographically secure random state string. */ diff --git a/packages/server/src/gateway/connections/chat-instance-manager.ts b/packages/server/src/gateway/connections/chat-instance-manager.ts index bafb40ce0..ec7172fe2 100644 --- a/packages/server/src/gateway/connections/chat-instance-manager.ts +++ b/packages/server/src/gateway/connections/chat-instance-manager.ts @@ -181,6 +181,46 @@ export class ChatInstanceManager { // errored so an operator can repair or remove it. const connection = storedToPlatform(stored); + // Apply the cloud-mode polling guard before startInstance — otherwise + // a previously-persisted `mode: "polling"` Telegram row would silently + // start at boot and bypass the create-time rejection added in + // `addConnection()`. Mark the row errored so an operator notices. + if ( + connection.status === "active" && + connection.platform === "telegram" && + isCloudMode() && + isPollingTelegramMode(connection.config as { mode?: string }) + ) { + const message = + "Polling mode is not supported in Lobu Cloud — use webhook mode, or self-host."; + logger.warn( + { id: connection.id, agentId: connection.agentId }, + `Refusing to boot Telegram polling connection in cloud mode: ${message}` + ); + // Self-bind the connection's owning org so the PostgreSQL-backed + // store's per-tenant predicate is satisfied — boot has no HTTP + // request and thus no ALS org context. + try { + const orgId = connection.organizationId; + const markErrored = () => + this.connectionStore.updateConnection(connection.id, { + status: "error", + errorMessage: message, + }); + if (orgId) { + await orgContext.run({ organizationId: orgId }, markErrored); + } else { + await markErrored(); + } + } catch (markErr) { + logger.error( + { id: connection.id, error: String(markErr) }, + "Failed to mark Telegram polling connection as errored" + ); + } + continue; + } + try { if (connection.status === "active") { // Boot runs without an HTTP request, so AsyncLocalStorage has diff --git a/packages/server/src/gateway/orchestration/base-deployment-manager.ts b/packages/server/src/gateway/orchestration/base-deployment-manager.ts index 860d26b26..03249de33 100644 --- a/packages/server/src/gateway/orchestration/base-deployment-manager.ts +++ b/packages/server/src/gateway/orchestration/base-deployment-manager.ts @@ -418,7 +418,19 @@ export abstract class BaseDeploymentManager { deploymentName?: string ): void { const agentId = messageData.agentId; - if (!this.policyStore || !agentId) return; + const organizationId = messageData.organizationId; + // PolicyStore is keyed by `(orgId, agentId)` to prevent cross-tenant + // policy clobbering — refuse to sync without an org id rather than + // collapsing into a shared bucket. + if (!this.policyStore || !agentId || !organizationId) { + if (!organizationId && agentId) { + logger.warn( + { agentId, deploymentName }, + "Skipping egress policy sync — message has no organizationId" + ); + } + return; + } const bundle = buildPolicyBundle({ judgedDomains: messageData.networkConfig?.judgedDomains, @@ -426,20 +438,21 @@ export abstract class BaseDeploymentManager { egressConfig: messageData.egressConfig, }); if (bundle) { - this.policyStore.set(agentId, bundle); + this.policyStore.set(organizationId, agentId, bundle); if (deploymentName) { logger.info( `Synced egress judge policy for ${deploymentName}: ${bundle.judgedDomains.length} rule(s), ${Object.keys(bundle.judges).length} judge(s)` ); } else { logger.debug("Synced egress judge policy", { + organizationId, agentId, rules: bundle.judgedDomains.length, judges: Object.keys(bundle.judges).length, }); } } else { - this.policyStore.clear(agentId); + this.policyStore.clear(organizationId, agentId); } } diff --git a/packages/server/src/gateway/permissions/__tests__/policy-store.test.ts b/packages/server/src/gateway/permissions/__tests__/policy-store.test.ts index e1aa56105..472b17aa9 100644 --- a/packages/server/src/gateway/permissions/__tests__/policy-store.test.ts +++ b/packages/server/src/gateway/permissions/__tests__/policy-store.test.ts @@ -28,34 +28,34 @@ import { describe("PolicyStore.resolve", () => { test("returns undefined for unknown agent", () => { const store = new PolicyStore(); - expect(store.resolve("unknown-agent", "example.com")).toBeUndefined(); + expect(store.resolve("org-1", "unknown-agent", "example.com")).toBeUndefined(); }); test("returns undefined when agent has no judged domains", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [], judges: { default: "Allow reads only." }, }); - expect(store.resolve("agent-1", "example.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-1", "example.com")).toBeUndefined(); }); test("returns undefined when hostname does not match any rule", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: "api.example.com" }], judges: { default: "Allow reads only." }, }); - expect(store.resolve("agent-1", "other.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-1", "other.com")).toBeUndefined(); }); test("exact domain match returns the resolved rule", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: "api.example.com" }], judges: { default: "Allow reads only." }, }); - const result = store.resolve("agent-1", "api.example.com"); + const result = store.resolve("org-1", "agent-1", "api.example.com"); expect(result).not.toBeUndefined(); expect(result!.judgeName).toBe("default"); expect(result!.policy).toBe("Allow reads only."); @@ -63,7 +63,7 @@ describe("PolicyStore.resolve", () => { test("exact match takes priority over wildcard", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [ { domain: ".example.com", judge: "wildcard-judge" }, { domain: "api.example.com", judge: "exact-judge" }, @@ -73,35 +73,35 @@ describe("PolicyStore.resolve", () => { "exact-judge": "Exact policy.", }, }); - const result = store.resolve("agent-1", "api.example.com"); + const result = store.resolve("org-1", "agent-1", "api.example.com"); expect(result!.judgeName).toBe("exact-judge"); expect(result!.policy).toBe("Exact policy."); }); test("wildcard .example.com matches sub.example.com", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: ".example.com" }], judges: { default: "Wildcard policy." }, }); - const result = store.resolve("agent-1", "sub.example.com"); + const result = store.resolve("org-1", "agent-1", "sub.example.com"); expect(result).not.toBeUndefined(); expect(result!.policy).toBe("Wildcard policy."); }); test("wildcard .example.com matches example.com root", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: ".example.com" }], judges: { default: "Root wildcard policy." }, }); - const result = store.resolve("agent-1", "example.com"); + const result = store.resolve("org-1", "agent-1", "example.com"); expect(result).not.toBeUndefined(); }); test("longer wildcard beats shorter wildcard", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [ { domain: ".example.com", judge: "short" }, { domain: ".api.example.com", judge: "long" }, @@ -112,51 +112,51 @@ describe("PolicyStore.resolve", () => { }, }); // ".api.example.com" is longer and should match "v2.api.example.com" - const result = store.resolve("agent-1", "v2.api.example.com"); + const result = store.resolve("org-1", "agent-1", "v2.api.example.com"); expect(result!.judgeName).toBe("long"); }); test("wildcard does not match unrelated domain", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: ".example.com" }], judges: { default: "Example only." }, }); - expect(store.resolve("agent-1", "evil.com")).toBeUndefined(); - expect(store.resolve("agent-1", "notexample.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-1", "evil.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-1", "notexample.com")).toBeUndefined(); }); test("named judge missing → undefined (fails closed)", () => { // Rule references a judge name not in the judges map. const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: "api.example.com", judge: "missing-judge" }], judges: { default: "Default judge." }, // 'missing-judge' is absent }); // Should return undefined rather than crash or use the wrong judge. - const result = store.resolve("agent-1", "api.example.com"); + const result = store.resolve("org-1", "agent-1", "api.example.com"); expect(result).toBeUndefined(); }); test("default judge name used when rule omits judge field", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: "api.example.com" }], // no judge field judges: { default: "Default judge text." }, }); - const result = store.resolve("agent-1", "api.example.com"); + const result = store.resolve("org-1", "agent-1", "api.example.com"); expect(result!.judgeName).toBe("default"); }); test("clear removes agent policy — resolve returns undefined afterwards", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: "api.example.com" }], judges: { default: "Allow." }, }); - expect(store.resolve("agent-1", "api.example.com")).not.toBeUndefined(); - store.clear("agent-1"); - expect(store.resolve("agent-1", "api.example.com")).toBeUndefined(); + expect(store.resolve("org-1", "agent-1", "api.example.com")).not.toBeUndefined(); + store.clear("org-1", "agent-1"); + expect(store.resolve("org-1", "agent-1", "api.example.com")).toBeUndefined(); }); }); @@ -169,43 +169,43 @@ describe("PolicyStore — policyHash", () => { judgedDomains: [{ domain: "api.example.com" }], judges: { default: "Allow only GET." }, }; - store.set("agent-1", bundle); - const h1 = store.resolve("agent-1", "api.example.com")!.policyHash; + store.set("org-1", "agent-1", bundle); + const h1 = store.resolve("org-1", "agent-1", "api.example.com")!.policyHash; // Re-set with same bundle (simulates reload). - store.set("agent-1", bundle); - const h2 = store.resolve("agent-1", "api.example.com")!.policyHash; + store.set("org-1", "agent-1", bundle); + const h2 = store.resolve("org-1", "agent-1", "api.example.com")!.policyHash; expect(h1).toBe(h2); }); test("policyHash differs when extraPolicy changes", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: "api.example.com" }], judges: { default: "Base policy." }, extraPolicy: "Extra A.", }); - const hashA = store.resolve("agent-1", "api.example.com")!.policyHash; + const hashA = store.resolve("org-1", "agent-1", "api.example.com")!.policyHash; - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: "api.example.com" }], judges: { default: "Base policy." }, extraPolicy: "Extra B.", }); - const hashB = store.resolve("agent-1", "api.example.com")!.policyHash; + const hashB = store.resolve("org-1", "agent-1", "api.example.com")!.policyHash; expect(hashA).not.toBe(hashB); }); test("extraPolicy is appended to composed policy text", () => { const store = new PolicyStore(); - store.set("agent-1", { + store.set("org-1", "agent-1", { judgedDomains: [{ domain: "api.example.com" }], judges: { default: "Base policy." }, extraPolicy: "Additional constraint.", }); - const result = store.resolve("agent-1", "api.example.com")!; + const result = store.resolve("org-1", "agent-1", "api.example.com")!; expect(result.policy).toContain("Base policy."); expect(result.policy).toContain("Additional constraint."); }); diff --git a/packages/server/src/gateway/permissions/policy-store.ts b/packages/server/src/gateway/permissions/policy-store.ts index f059648b4..3112e1830 100644 --- a/packages/server/src/gateway/permissions/policy-store.ts +++ b/packages/server/src/gateway/permissions/policy-store.ts @@ -49,14 +49,28 @@ interface PreparedBundle { * * Composed policy text and its hash are computed once at `set()` time and * reused on every `resolve()` so the hot path does no SHA256 work. + * + * Keyed by `(organizationId, agentId)`. Agent ids are per-org-unique on + * paper but bugs in upstream code (or a malicious sync from another tenant) + * must never overwrite policy across orgs — that turns the verdict-cache + * org scoping into theatre. The key here is the safety net. */ export class PolicyStore { private readonly policies = new Map(); - set(agentId: string, bundle: JudgePolicyBundle): void { - const prepared = prepareBundle(agentId, bundle); - this.policies.set(agentId, prepared); + private static composeKey(organizationId: string, agentId: string): string { + return `${organizationId}|${agentId}`; + } + + set( + organizationId: string, + agentId: string, + bundle: JudgePolicyBundle + ): void { + const prepared = prepareBundle(organizationId, agentId, bundle); + this.policies.set(PolicyStore.composeKey(organizationId, agentId), prepared); logger.debug("Set egress policy bundle", { + organizationId, agentId, domains: prepared.judgedDomains.length, judges: Object.keys(prepared.preparedJudges).length, @@ -64,17 +78,24 @@ export class PolicyStore { }); } - clear(agentId: string): void { - this.policies.delete(agentId); + clear(organizationId: string, agentId: string): void { + this.policies.delete(PolicyStore.composeKey(organizationId, agentId)); } /** - * Resolve a judge rule for a hostname under an agent. Rules use the same - * domain pattern format as allow/deny lists. Exact match is preferred; - * wildcard patterns (`.example.com`) match the root plus any subdomain. + * Resolve a judge rule for a hostname under an `(org, agent)` pair. + * Rules use the same domain pattern format as allow/deny lists. Exact + * match is preferred; wildcard patterns (`.example.com`) match the root + * plus any subdomain. */ - resolve(agentId: string, hostname: string): ResolvedJudgeRule | undefined { - const prepared = this.policies.get(agentId); + resolve( + organizationId: string, + agentId: string, + hostname: string + ): ResolvedJudgeRule | undefined { + const prepared = this.policies.get( + PolicyStore.composeKey(organizationId, agentId) + ); if (!prepared || prepared.judgedDomains.length === 0) { return undefined; } @@ -89,7 +110,7 @@ export class PolicyStore { if (!judge) { logger.warn( "Judge rule matched but named policy not found — failing closed", - { agentId, hostname, judgeName } + { organizationId, agentId, hostname, judgeName } ); return undefined; } @@ -143,6 +164,7 @@ export function buildPolicyBundle(input: { } function prepareBundle( + organizationId: string, agentId: string, bundle: JudgePolicyBundle ): PreparedBundle { @@ -153,7 +175,7 @@ function prepareBundle( : rawPolicy.trim(); preparedJudges[name] = { policy: composed, - policyHash: hashPolicy(agentId, name, composed), + policyHash: hashPolicy(organizationId, agentId, name, composed), }; } return { @@ -188,13 +210,14 @@ function findMatchingRule( } function hashPolicy( + organizationId: string, agentId: string, judgeName: string, policy: string ): string { return crypto .createHash("sha256") - .update(`${agentId} ${judgeName} ${policy}`) + .update(`${organizationId} ${agentId} ${judgeName} ${policy}`) .digest("hex") .slice(0, 16); } diff --git a/packages/server/src/gateway/proxy/http-proxy.ts b/packages/server/src/gateway/proxy/http-proxy.ts index a9ab6673d..f2d4648b3 100644 --- a/packages/server/src/gateway/proxy/http-proxy.ts +++ b/packages/server/src/gateway/proxy/http-proxy.ts @@ -87,6 +87,16 @@ export function setProxyPolicyStore(store: PolicyStore): void { } } +/** + * Set the grant store the proxy consults when resolving per-agent + * allow/deny grants. Production wires this from `CoreServices`; tests use + * it to install a mock or a fresh DB-backed store so the cross-org leakage + * fixed in this PR can be exercised end-to-end. + */ +export function setProxyGrantStore(store: GrantStore): void { + proxyGrantStore = store; +} + /** * Replace the lazy {@link EgressJudge} — tests inject a fake client here * so the proxy can be exercised end-to-end without hitting a real model. @@ -151,9 +161,17 @@ async function checkDomainAccess( ); if (globallyAllowed) { - // Even if globally allowed, a per-agent deny grant can override + // Even if globally allowed, a per-agent deny grant can override. + // Pass `organizationId` explicitly — `GrantStore` falls back to the ALS + // org context when omitted, but the raw Node HTTP proxy never sets ALS + // and the WHERE clause would drop its `organization_id` predicate, + // leaking grants/denies across tenants that share an agent id. if (proxyGrantStore && agentId) { - const denied = await proxyGrantStore.isDenied(agentId, hostname); + const denied = await proxyGrantStore.isDenied( + agentId, + hostname, + organizationId + ); if (denied) { logger.debug(`Domain ${hostname} denied via grant (agent: ${agentId})`); return { allowed: false, source: "grant" }; @@ -164,7 +182,11 @@ async function checkDomainAccess( // Not globally allowed — check grant store for per-agent access if (proxyGrantStore && agentId) { - const granted = await proxyGrantStore.hasGrant(agentId, hostname); + const granted = await proxyGrantStore.hasGrant( + agentId, + hostname, + organizationId + ); if (granted) { logger.debug(`Domain ${hostname} allowed via grant (agent: ${agentId})`); return { allowed: true, source: "grant" }; @@ -172,16 +194,16 @@ async function checkDomainAccess( } // Fall through to the LLM egress judge when a matching rule exists. - if (proxyPolicyStore && proxyEgressJudge && agentId) { - const rule = proxyPolicyStore.resolve(agentId, hostname); + // PolicyStore is keyed by `(orgId, agentId)`; without an org id we refuse + // to consult it — falling through to an unkeyed lookup would let another + // tenant's policy decide our verdict. + if (proxyPolicyStore && proxyEgressJudge && agentId && organizationId) { + const rule = proxyPolicyStore.resolve(organizationId, agentId, hostname); if (rule) { const decision = await proxyEgressJudge.decide( { agentId, - // `organizationId` may be empty when an older token (minted before - // the org-id pivot) is still in flight. The cache uses it as part - // of the key — empty string and a real UUID never collide. - organizationId: organizationId ?? "", + organizationId, hostname, method: requestContext?.method, path: requestContext?.path, diff --git a/packages/server/src/gateway/proxy/secret-proxy.ts b/packages/server/src/gateway/proxy/secret-proxy.ts index e4ebbff12..82551a139 100644 --- a/packages/server/src/gateway/proxy/secret-proxy.ts +++ b/packages/server/src/gateway/proxy/secret-proxy.ts @@ -1,4 +1,4 @@ -import { createLogger, type SecretRef } from "@lobu/core"; +import { createLogger, type SecretRef, verifyWorkerToken } from "@lobu/core"; import type { Context } from "hono"; import { Hono } from "hono"; import type { AuthProfilesManager } from "../auth/settings/auth-profiles-manager.js"; @@ -7,6 +7,17 @@ import type { ProviderUpstreamConfig } from "../modules/module-system.js"; import type { SecretStore } from "../secrets/index.js"; import { getClientIp } from "../utils/rate-limiter.js"; +/** + * Caller-supplied resolver: agentId → orgId of the agent's owning org. + * + * The proxy needs an independent source of the caller's org to compare + * against `SecretMapping.organizationId` — without it the org-scoping + * guard on `lookupPlaceholderMapping` has nothing to enforce against and + * collapses to dead code. The deployment manager wires a DB-backed + * resolver with a small TTL cache at boot. + */ +export type AgentOrgResolver = (agentId: string) => Promise; + const logger = createLogger("secret-proxy"); const PLACEHOLDER_PREFIX = "lobu_secret_"; @@ -247,6 +258,7 @@ export class SecretProxy { private authProfilesManager?: AuthProfilesManager; private readonly secretStore: SecretStore; private systemKeyResolver?: (providerId: string) => string | undefined; + private agentOrgResolver?: AgentOrgResolver; constructor(config: SecretProxyConfig, secretStore: SecretStore) { this.config = config; @@ -276,6 +288,16 @@ export class SecretProxy { this.systemKeyResolver = resolver; } + /** + * Wire in a resolver that maps a URL-encoded agentId to its owning org + * id. Used to compute the `expectedOrganizationId` we hand to + * `lookupPlaceholderMapping` — without it the org-scoping guard has no + * independent source of truth and can't enforce anything. + */ + setAgentOrgResolver(resolver: AgentOrgResolver): void { + this.agentOrgResolver = resolver; + } + /** * Register a provider upstream for slug-based routing. * Called after provider modules are initialized. @@ -322,9 +344,19 @@ export class SecretProxy { * Resolve a placeholder token to its real value via the in-memory cache. * Handles both plain (`lobu_secret_`) and prefixed * (`sk-ant-oat01-lobu_secret_`) placeholders. + * + * `expectedOrganizationId` is forwarded to {@link lookupPlaceholderMapping} + * so a worker carrying another tenant's placeholder cannot resolve it + * even on the legacy header-swap path. */ - private async resolveSecret(placeholder: string): Promise { - const mapping = this.lookupPlaceholderMapping(placeholder); + private async resolveSecret( + placeholder: string, + expectedOrganizationId?: string + ): Promise { + const mapping = this.lookupPlaceholderMapping( + placeholder, + expectedOrganizationId + ); if (!mapping) return null; return this.secretStore.get(mapping.secretRef); } @@ -348,6 +380,44 @@ export class SecretProxy { return lookupPlaceholderMapping(placeholder, expectedOrganizationId); } + /** + * Extract the worker token from a dedicated `x-lobu-worker-token` header + * (or query param of the same name — useful for SSE that can't set + * headers), verify it, and return the bound `organizationId`. Falls back + * to verifying the bearer credential when it isn't a placeholder. + * + * Returns `undefined` when no verifiable token is present — the caller + * then relies on `agentOrgResolver` (DB lookup keyed by URL agentId) to + * fill in the expected org. + */ + private extractWorkerTokenOrg(c: Context): string | undefined { + const header = + c.req.header("x-lobu-worker-token") || + c.req.header("X-Lobu-Worker-Token"); + const candidate = header || c.req.query("worker_token") || undefined; + if (candidate) { + const data = verifyWorkerToken(candidate); + if (data?.organizationId) return data.organizationId; + } + // The bearer credential is normally a `lobu_secret_` placeholder + // but legacy callers may pass the worker JWT directly. Verify if it + // looks like one (long, no placeholder prefix). + const auth = + c.req.header("authorization") || c.req.header("Authorization"); + if (auth) { + const parts = auth.split(" "); + const tok = + parts.length === 2 && parts[0]?.toLowerCase() === "bearer" + ? parts[1] + : null; + if (tok && !tok.includes(PLACEHOLDER_PREFIX)) { + const data = verifyWorkerToken(tok); + if (data?.organizationId) return data.organizationId; + } + } + return undefined; + } + /** * Extract the bearer/api-key value the caller used to authenticate. * Returns the raw token string, or null if no auth header is present. @@ -371,14 +441,18 @@ export class SecretProxy { * `source` identifies the caller (best available identity: bound agentId or * remote address) for per-source failed-resolution rate limiting. */ - private async swap(value: string, source: string): Promise { + private async swap( + value: string, + source: string, + expectedOrganizationId?: string + ): Promise { if (value.includes(PLACEHOLDER_PREFIX)) { if (resolutionFailureLimiter.isThrottled(source)) { // Source has burned through its failure budget — hard-fail without // touching the cache or logging another line. return ""; } - const resolved = await this.resolveSecret(value); + const resolved = await this.resolveSecret(value, expectedOrganizationId); if (!resolved) { // Fail closed: forwarding the literal placeholder upstream would // surface it in the provider's error response (and thus in worker @@ -456,6 +530,26 @@ export class SecretProxy { body = await c.req.text(); } + // Derive the caller's expected org from the verified worker token + // (preferred — it's signed) and fall back to a DB lookup keyed by the + // URL agentId. Either source becomes the `expectedOrganizationId` + // we hand to placeholder + secret lookups so a worker bearing org A's + // placeholder cannot resolve it under org B's URL. + const callerToken = this.extractCallerToken(c); + let expectedOrganizationId: string | undefined = + this.extractWorkerTokenOrg(c); + if (!expectedOrganizationId && urlAgentId && this.agentOrgResolver) { + try { + const orgId = await this.agentOrgResolver(urlAgentId); + if (orgId) expectedOrganizationId = orgId; + } catch (err) { + logger.warn( + { urlAgentId, err: String(err) }, + "agentOrgResolver failed — falling through without org expectation" + ); + } + } + // Bind the calling worker (identified by its placeholder credential) to // the agentId in the URL. Without this, anyone with network access to the // gateway could harvest another agent's credentials by changing the URL @@ -463,9 +557,11 @@ export class SecretProxy { // bound (logged as a warning) but reject any request whose placeholder // resolves to a different agent than the URL claims. if (urlAgentId) { - const callerToken = this.extractCallerToken(c); if (callerToken?.includes(PLACEHOLDER_PREFIX)) { - const mapping = this.lookupPlaceholderMapping(callerToken); + const mapping = this.lookupPlaceholderMapping( + callerToken, + expectedOrganizationId + ); if (!mapping) { logger.warn( { urlAgentId }, @@ -587,7 +683,11 @@ export class SecretProxy { }); const apiKey = c.req.header("x-api-key"); if (apiKey) { - headers["x-api-key"] = await this.swap(apiKey, source); + headers["x-api-key"] = await this.swap( + apiKey, + source, + expectedOrganizationId + ); } const auth = @@ -595,7 +695,11 @@ export class SecretProxy { if (auth) { const parts = auth.split(" "); if (parts.length === 2 && parts[0]?.toLowerCase() === "bearer") { - const swapped = await this.swap(parts[1]!, source); + const swapped = await this.swap( + parts[1]!, + source, + expectedOrganizationId + ); headers.authorization = `Bearer ${swapped}`; } } diff --git a/packages/server/src/gateway/routes/public/slack.ts b/packages/server/src/gateway/routes/public/slack.ts index a76e303f2..a0bf5889d 100644 --- a/packages/server/src/gateway/routes/public/slack.ts +++ b/packages/server/src/gateway/routes/public/slack.ts @@ -2,6 +2,7 @@ import { readFile } from "node:fs/promises"; import { createLogger } from "@lobu/core"; import type { Context } from "hono"; import { Hono } from "hono"; +import { getDb } from "../../../db/client.js"; import { createSlackInstallStateStore } from "../../auth/oauth/state-store.js"; import { renderOAuthErrorPage, @@ -24,7 +25,9 @@ const logger = createLogger("slack-routes"); * 2. `c.get('session')?.activeOrganizationId` — better-auth's stamped * active org, used when the wrapper hasn't run (rare; defensive). * - * Returns `null` if neither is present — caller must reject the request. + * Returns `null` if neither is present — caller must reject the request + * (after consulting {@link resolveSingleTenantOrgId} for the self-host + * fallback). */ function readSessionOrgId(c: Context): string | null { const fromContext = c.get("organizationId" as never) as @@ -45,6 +48,43 @@ function readSessionOrgId(c: Context): string | null { return null; } +/** + * Self-host fallback: when there's exactly one organization row in the + * database, return its id. This keeps `/slack/install` usable on + * single-tenant deployments where the route is mounted without the + * lobuApp session middleware that populates `c.get('organizationId')`. + * + * Returns `null` when zero or more than one org rows exist — in those + * cases the caller must reject; we won't silently pick a tenant. + */ +async function resolveSingleTenantOrgId(): Promise { + try { + const sql = getDb(); + const rows = (await sql` + SELECT id FROM organization LIMIT 2 + `) as Array<{ id: string }>; + if (rows.length === 1) return rows[0]!.id; + return null; + } catch (err) { + logger.warn( + { err: String(err) }, + "Single-tenant org lookup failed — treating as ambiguous" + ); + return null; + } +} + +/** + * Resolve the install-flow org for the current request: session-bound first, + * then the self-host single-tenant fallback. Returns `null` only when + * neither path yields a definite org — at which point the route must reject. + */ +async function resolveInstallOrgId(c: Context): Promise { + const sessionOrgId = readSessionOrgId(c); + if (sessionOrgId) return sessionOrgId; + return resolveSingleTenantOrgId(); +} + const DEFAULT_SLACK_BOT_SCOPES = [ "app_mentions:read", "assistant:write", @@ -125,9 +165,11 @@ export function createSlackRoutes(manager: ChatInstanceManager): Hono { // Bind the install to the initiating session's active org. Without this // an OAuth link minted under org A's session can be opened from org B's - // browser and the resulting connection lands in the wrong tenant. - const sessionOrgId = readSessionOrgId(c); - if (!sessionOrgId) { + // browser and the resulting connection lands in the wrong tenant. On + // self-host (no session middleware mounted), fall back to the sole org + // row when exactly one exists — see {@link resolveSingleTenantOrgId}. + const installOrgId = await resolveInstallOrgId(c); + if (!installOrgId) { return c.html( renderOAuthErrorPage( "unauthorized", @@ -145,7 +187,7 @@ export function createSlackRoutes(manager: ChatInstanceManager): Hono { const scopes = await loadSlackBotScopes(); const state = await stateStore.create({ redirectUri, - organizationId: sessionOrgId, + organizationId: installOrgId, }); const oauthUrl = new URL("https://slack.com/oauth/v2/authorize"); @@ -171,7 +213,11 @@ export function createSlackRoutes(manager: ChatInstanceManager): Hono { } const stateStore = createSlackInstallStateStore(); - const oauthState = await stateStore.consume(state); + // Peek (non-destructive) before validating side-channel context so a + // cross-org or unauthenticated hit doesn't burn the install link. + // Consume only after the org check passes — the row stays available + // for the legitimate caller to retry. + const oauthState = await stateStore.peek(state); if (!oauthState) { return c.html( @@ -186,8 +232,9 @@ export function createSlackRoutes(manager: ChatInstanceManager): Hono { // Reject the callback if the session that's completing the install // belongs to a different org than the one that started it. Prevents // an attacker who phishes the install link from landing a connection - // in their own org under a victim's authorization. - const callbackOrgId = readSessionOrgId(c); + // in their own org under a victim's authorization. Self-host falls + // back to the single-tenant resolver (same as `/slack/install`). + const callbackOrgId = await resolveInstallOrgId(c); if (!callbackOrgId || callbackOrgId !== oauthState.organizationId) { logger.warn( { @@ -205,10 +252,24 @@ export function createSlackRoutes(manager: ChatInstanceManager): Hono { ); } + // Org check passed — now atomically consume so the link can't be + // replayed. If the row is gone between peek and consume (another + // tab raced), fall through to the same invalid_state response. + const consumed = await stateStore.consume(state); + if (!consumed) { + return c.html( + renderOAuthErrorPage( + "invalid_state", + "This Slack install link is invalid or has expired." + ), + 400 + ); + } + try { const result = await manager.completeSlackOAuthInstall( c.req.raw, - oauthState.redirectUri + consumed.redirectUri ); return c.html( renderOAuthSuccessPage(result.teamName || result.teamId, undefined, { diff --git a/packages/server/src/gateway/services/core-services.ts b/packages/server/src/gateway/services/core-services.ts index a0c4f627c..9bbdc7fd3 100644 --- a/packages/server/src/gateway/services/core-services.ts +++ b/packages/server/src/gateway/services/core-services.ts @@ -658,6 +658,12 @@ export class CoreServices { // Register provider upstream configs with the secret proxy for path-based routing if (this.secretProxy) { this.secretProxy.setAuthProfilesManager(this.authProfilesManager); + // Independent source of the caller's expected org for placeholder + // lookups — without this the org-scoping guard on + // `lookupPlaceholderMapping` has nothing to enforce against. + this.secretProxy.setAgentOrgResolver((agentId) => + this.resolveAgentOrgId(agentId) + ); for (const provider of getModelProviderModules()) { const upstream = provider.getUpstreamConfig?.(); if (upstream) { @@ -914,6 +920,21 @@ export class CoreServices { return this.secretProxy; } + /** + * Resolve an agent's owning organization id from `public.agents`. Used by + * the secret proxy to compute the `expectedOrganizationId` it hands to + * placeholder lookups. No cache — the SecretProxy is called once per + * upstream request, and a SELECT by primary key on `agents.id` is cheap + * enough that we'd rather avoid the staleness window of a TTL cache. + */ + private async resolveAgentOrgId(agentId: string): Promise { + const sql = getDb(); + const rows = (await sql` + SELECT organization_id FROM agents WHERE id = ${agentId} LIMIT 1 + `) as Array<{ organization_id?: string }>; + return rows[0]?.organization_id ?? null; + } + getSecretStore(): SecretStoreRegistry { if (!this.secretStore) throw new Error("Secret store not initialized"); return this.secretStore; From ada4b1697dc335a5b83cd76c7848d46082d2ce67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Emre=20Kabakc=C4=B1?= Date: Mon, 18 May 2026 02:34:09 +0100 Subject: [PATCH 3/3] fix(server): stamp organizationId on worker tokens minted by the public Agent API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chat-platform spawn path (base-deployment-manager, agent-threads.createThreadForAgent) already passes organizationId into generateWorkerToken. The public Agent API entry point (POST /api/v1/agents) did NOT — every worker spawned via 'lobu chat', 'lobu eval', or the JS SDK landed with tokenData.organizationId === undefined and the egress proxy's new per-tenant gates short-circuited to unscoped checks for that worker. The route now looks the agent's owning org up via the ownership metadata store and stamps the token. To make this work, AgentMetadata gains an optional organizationId field populated by the postgres-backed store (in-memory test stores leave it undefined — back-compat by design). Ephemeral agents (no preexisting metadata) still mint without orgId; that narrower case is tracked as a follow-up — needs an auth-session- driven derivation path. Reproducers in multi-tenant-isolation-reproducers.test.ts pin the contract for both the metadata-driven org-stamped path and the ephemeral undefined path. --- packages/core/src/agent-store.ts | 8 ++ ...multi-tenant-isolation-reproducers.test.ts | 79 ++++++++++++++++++- .../server/src/gateway/routes/public/agent.ts | 13 +++ .../server/src/lobu/stores/postgres-stores.ts | 7 +- 4 files changed, 103 insertions(+), 4 deletions(-) diff --git a/packages/core/src/agent-store.ts b/packages/core/src/agent-store.ts index 10b52520c..d837517c7 100644 --- a/packages/core/src/agent-store.ts +++ b/packages/core/src/agent-store.ts @@ -91,6 +91,14 @@ export interface AgentMetadata { owner: { platform: string; userId: string }; isWorkspaceAgent?: boolean; workspaceId?: string; + /** + * Owning organization id. Optional in the type for back-compat with + * in-memory stores that predate per-tenant scoping; populated by the + * postgres-backed store. The public Agent API route reads this to stamp + * worker tokens with the agent's org so the egress proxy can scope + * per-tenant gates (grants, judge cache, judge policy). + */ + organizationId?: string; createdAt: number; lastUsedAt?: number; } diff --git a/packages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.ts b/packages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.ts index e7349aa29..c99cd37d3 100644 --- a/packages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.ts +++ b/packages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.ts @@ -24,7 +24,11 @@ */ import { afterAll, beforeAll, beforeEach, describe, expect, test } from "bun:test"; -import { createBuiltinSecretRef } from "@lobu/core"; +import { + createBuiltinSecretRef, + generateWorkerToken, + verifyWorkerToken, +} from "@lobu/core"; import { GrantStore } from "../permissions/grant-store.js"; import { PolicyStore } from "../permissions/policy-store.js"; import { @@ -443,3 +447,76 @@ describe("[finding 4] ChatInstanceManager.initialize refuses Telegram polling ro } }); }); + +// ─── Public Agent API mint path stamps organizationId on worker tokens ───── +// +// The chat-platform spawn path (`base-deployment-manager`, +// `agent-threads.createThreadForAgent`) already passes `organizationId` +// into `generateWorkerToken`. The public Agent API entry point +// (`POST /api/v1/agents`) did NOT — every worker spawned via `lobu chat`, +// `lobu eval`, or the JS SDK landed with `tokenData.organizationId === +// undefined`. The egress proxy then short-circuited the new per-tenant +// gates and fell back to unscoped checks for that worker. The route now +// looks the agent's owning org up via the ownership metadata store and +// stamps the token. +// +// This test pins the contract: given an agentId whose metadata returns +// org A, the route handler's lookup pattern must yield a token whose +// decoded `organizationId === "org-a"`. A regression that drops the +// pass-through (e.g. forgets `organizationId: tokenOrganizationId` in +// the options bag) fails the second assertion. +describe("[follow-up] API mint path stamps organizationId on worker tokens", () => { + test("metadata-driven lookup propagates org into the worker token", async () => { + const agentId = "agent-mint-1"; + const metadataStore = { + getMetadata: async (id: string) => + id === agentId + ? { id, organizationId: "org-a", createdAt: 0, updatedAt: 0 } + : null, + }; + + // Replicates the in-route helper: look up the pinned agent's org + // before minting the token. Ephemeral agents (no metadata) yield + // undefined and the proxy falls through to unscoped checks — that + // narrower case is tracked as a follow-up. + const tokenOrganizationId = + (await metadataStore.getMetadata(agentId))?.organizationId; + + const token = generateWorkerToken(agentId, "conv-1", "api-mint-1", { + channelId: "api_user-1", + agentId, + organizationId: tokenOrganizationId, + platform: "api", + sessionKey: "user-1", + }); + + const decoded = verifyWorkerToken(token); + expect(decoded).not.toBeNull(); + expect(decoded?.agentId).toBe(agentId); + expect(decoded?.organizationId).toBe("org-a"); + }); + + test("ephemeral agents (no metadata) mint without organizationId", async () => { + const metadataStore = { getMetadata: async () => null }; + const tokenOrganizationId = + (await metadataStore.getMetadata())?.organizationId; + expect(tokenOrganizationId).toBeUndefined(); + + const token = generateWorkerToken( + "ephemeral-agent", + "conv-2", + "api-mint-2", + { + channelId: "api_user-2", + agentId: "ephemeral-agent", + organizationId: tokenOrganizationId, + platform: "api", + sessionKey: "user-2", + } + ); + + const decoded = verifyWorkerToken(token); + expect(decoded).not.toBeNull(); + expect(decoded?.organizationId).toBeUndefined(); + }); +}); diff --git a/packages/server/src/gateway/routes/public/agent.ts b/packages/server/src/gateway/routes/public/agent.ts index c30f276b4..5965019f3 100644 --- a/packages/server/src/gateway/routes/public/agent.ts +++ b/packages/server/src/gateway/routes/public/agent.ts @@ -633,6 +633,17 @@ export function createAgentApi(config: AgentApiConfig): OpenAPIHono { if (denial) return denial; } + // Stamp the worker token with the agent's owning org so the egress + // proxy's per-tenant gates (grant/deny, judge cache, judge policy) + // can scope decisions by org. Ephemeral agents have no preexisting + // metadata; their token mints without orgId and the proxy falls + // through to unscoped checks for that worker — flagged for a + // future fix that derives org from the auth session. + const tokenOrganizationId = + !isEphemeral && ownershipMetadataStore + ? (await ownershipMetadataStore.getMetadata(agentId))?.organizationId + : undefined; + // For ephemeral agents, auto-provision settings from system-key // providers (env-var-based API keys). No more template-agent fallback — // there are no template/sandbox agents anymore. @@ -689,6 +700,7 @@ export function createAgentApi(config: AgentApiConfig): OpenAPIHono { { channelId, agentId, + organizationId: tokenOrganizationId, platform: "api", sessionKey: userId, } @@ -718,6 +730,7 @@ export function createAgentApi(config: AgentApiConfig): OpenAPIHono { const token = generateWorkerToken(agentId, conversationId, deploymentName, { channelId, agentId, + organizationId: tokenOrganizationId, platform: "api", sessionKey: userId, }); diff --git a/packages/server/src/lobu/stores/postgres-stores.ts b/packages/server/src/lobu/stores/postgres-stores.ts index 5b76cf284..5827af5a8 100644 --- a/packages/server/src/lobu/stores/postgres-stores.ts +++ b/packages/server/src/lobu/stores/postgres-stores.ts @@ -80,6 +80,7 @@ function rowToMetadata(row: Record): AgentMetadata { }, isWorkspaceAgent: row.is_workspace_agent ?? undefined, workspaceId: row.workspace_id ?? undefined, + organizationId: row.organization_id ?? undefined, createdAt: row.created_at instanceof Date ? row.created_at.getTime() : (row.created_at ?? Date.now()), lastUsedAt: @@ -266,14 +267,14 @@ export function createPostgresAgentConfigStore(): AgentConfigStore { const orgId = tryGetOrgId(); const rows = orgId ? await sql` - SELECT id, name, description, owner_platform, owner_user_id, + SELECT id, organization_id, name, description, owner_platform, owner_user_id, is_workspace_agent, workspace_id, created_at, last_used_at FROM agents WHERE id = ${agentId} AND organization_id = ${orgId} ` : await sql` - SELECT id, name, description, owner_platform, owner_user_id, + SELECT id, organization_id, name, description, owner_platform, owner_user_id, is_workspace_agent, workspace_id, created_at, last_used_at FROM agents @@ -357,7 +358,7 @@ export function createPostgresAgentConfigStore(): AgentConfigStore { const sql = getDb(); const orgId = getOrgId(); const rows = await sql` - SELECT id, name, description, owner_platform, owner_user_id, + SELECT id, organization_id, name, description, owner_platform, owner_user_id, is_workspace_agent, workspace_id, created_at, last_used_at FROM agents