From f99bddcdf0bb2fe6bb529be813279c010e12525a Mon Sep 17 00:00:00 2001 From: mptyl Date: Sun, 16 Aug 2026 18:03:36 +0200 Subject: [PATCH] fix(auth): restore permission boundary safeguards --- backend/src/auth/authorization.ts | 20 +++++++++++++++ backend/src/routes/pi-management.ts | 14 +++++++++- backend/src/routes/sessions.ts | 3 ++- backend/src/routes/sql.ts | 13 +++++++--- backend/test/authorization.test.ts | 20 ++++++++++++++- backend/test/routes-pi-management.test.ts | 14 +++++----- backend/test/routes-sessions.test.ts | 25 ++++++++++++++++++ backend/test/routes-sql-meta.test.ts | 31 +++++++++++++++++++++++ 8 files changed, 126 insertions(+), 14 deletions(-) diff --git a/backend/src/auth/authorization.ts b/backend/src/auth/authorization.ts index bef58c8b..bec563e4 100644 --- a/backend/src/auth/authorization.ts +++ b/backend/src/auth/authorization.ts @@ -22,3 +22,23 @@ export function requirePermission( if (hasPermission(principal, permission)) return principal; return reply.code(403).send({ code: "auth_forbidden", error: "This operation is not permitted" }); } + +/** Permit non-browser clients and browsers whose declared origin matches the request host. */ +export function requireSameOriginOrNonBrowser( + request: FastifyRequest, + reply: FastifyReply, +): FastifyReply | undefined { + const origin = request.headers.origin; + if (origin === undefined) return undefined; + if (typeof origin !== "string" || typeof request.headers.host !== "string") { + return reply.code(403).send({ code: "auth_forbidden", error: "This operation is not permitted" }); + } + try { + const supplied = new URL(origin); + const expected = new URL(`${request.protocol}://${request.headers.host}`); + if (supplied.origin === expected.origin) return undefined; + } catch { + // Invalid browser origins are forbidden below. + } + return reply.code(403).send({ code: "auth_forbidden", error: "This operation is not permitted" }); +} diff --git a/backend/src/routes/pi-management.ts b/backend/src/routes/pi-management.ts index 1021bd4c..8a3838e6 100644 --- a/backend/src/routes/pi-management.ts +++ b/backend/src/routes/pi-management.ts @@ -1,5 +1,9 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; -import { isPrincipalContext, requirePermission } from "../auth/authorization.js"; +import { + isPrincipalContext, + requirePermission, + requireSameOriginOrNonBrowser, +} from "../auth/authorization.js"; import { PiManagementError, type PiManagementService } from "../pi/management.js"; export function piManagementRoutes( @@ -26,6 +30,10 @@ async function run( ): Promise { const principal = requirePermission(request, reply, "pi.manage"); if (!isPrincipalContext(principal)) return principal; + if (principal.issuer === "local" && isWrite(request.method)) { + const csrfDenied = requireSameOriginOrNonBrowser(request, reply); + if (csrfDenied) return csrfDenied; + } try { return await action(); } catch (error) { @@ -36,3 +44,7 @@ async function run( return reply.code(503).send({ code: "pi_management_unavailable", error: "Pi management is unavailable" }); } } + +function isWrite(method: string): boolean { + return method === "POST" || method === "PUT" || method === "PATCH" || method === "DELETE"; +} diff --git a/backend/src/routes/sessions.ts b/backend/src/routes/sessions.ts index f2596eca..955bde81 100644 --- a/backend/src/routes/sessions.ts +++ b/backend/src/routes/sessions.ts @@ -102,7 +102,8 @@ export function sessionRoutes( admissionLeases.set(req, release); }); app.addHook("preHandler", async (req, reply) => { - if (req.url === "/runtime/prewarm" || req.url === "/sessions" || req.url.startsWith("/sessions/")) { + const pathname = req.url.split("?", 1)[0]; + if (pathname === "/runtime/prewarm" || pathname === "/sessions" || pathname.startsWith("/sessions/")) { const principal = requirePermission(req, reply, "session.use"); if (!isPrincipalContext(principal)) return principal; } diff --git a/backend/src/routes/sql.ts b/backend/src/routes/sql.ts index c9a13517..c547fdeb 100644 --- a/backend/src/routes/sql.ts +++ b/backend/src/routes/sql.ts @@ -3,7 +3,7 @@ import type { ThtRunner } from "../tht/tht-runner.js"; import type { PrincipalContext } from "../auth/principal.js"; import type { Settings } from "../settings/settings-store.js"; import type { WorkspaceRegistry } from "../workspaces/registry.js"; -import { isPrincipalContext, requirePermission } from "../auth/authorization.js"; +import { hasPermission, isPrincipalContext, requirePermission } from "../auth/authorization.js"; export function sqlRoutes(app: FastifyInstance, deps: { tht: ThtRunner; getSettings: (principal: PrincipalContext) => Promise; @@ -13,11 +13,16 @@ export function sqlRoutes(app: FastifyInstance, deps: { const runner = deps.tht as any; return typeof runner.withPrincipal === "function" ? runner.withPrincipal(principal) : runner; }; + const readPrincipal = (principal: PrincipalContext): PrincipalContext => ({ + ...principal, + // The harness still consumes this compatibility bit; it must never exceed session.read_all. + isAdmin: hasPermission(principal, "session.read_all"), + }); const isNotFound = (error: unknown) => /not found|non trovata|inesistente|404/i.test( error instanceof Error ? error.message : String(error), ); const locate = async (principal: PrincipalContext, id: string, legacyWorkspace?: string) => { - const runner = runnerFor(principal); + const runner = runnerFor(readPrincipal(principal)); if (typeof runner.sessionShow !== "function") return { manifest: {}, workspace: legacyWorkspace }; const registry = deps.workspaceRegistry as Partial; const revisions = typeof registry.listRetainedSnapshots === "function" @@ -65,7 +70,7 @@ export function sqlRoutes(app: FastifyInstance, deps: { return reply.code(503).send({ error: "session storage is unavailable" }); } try { - return await runnerFor(principal).sqlPreview(id, { limit, offset }, workspace); + return await runnerFor(readPrincipal(principal)).sqlPreview(id, { limit, offset }, workspace); } catch (error: any) { return reply.code(500).send({ error: error.message ?? String(error) }); } @@ -87,7 +92,7 @@ export function sqlRoutes(app: FastifyInstance, deps: { return reply.code(503).send({ error: "session storage is unavailable" }); } try { - return await runnerFor(principal).sqlExport(id, workspace); + return await runnerFor(readPrincipal(principal)).sqlExport(id, workspace); } catch (error: any) { return reply.code(500).send({ error: error.message ?? String(error) }); } diff --git a/backend/test/authorization.test.ts b/backend/test/authorization.test.ts index 57281987..cacfed39 100644 --- a/backend/test/authorization.test.ts +++ b/backend/test/authorization.test.ts @@ -1,7 +1,7 @@ import { expect, test } from "vitest"; import Fastify from "fastify"; import { authPreHandler, getPrincipal } from "../src/auth/auth.js"; -import { hasPermission } from "../src/auth/authorization.js"; +import { hasPermission, requireSameOriginOrNonBrowser } from "../src/auth/authorization.js"; import type { PrincipalContext } from "../src/auth/principal.js"; import type { Permission } from "../src/auth/types.js"; @@ -54,3 +54,21 @@ test("compatibility adapters derive roles and permissions before routes inspect expect(elevated.json()).toMatchObject({ roles: ["admin"], isAdmin: true }); expect(malformed.json()).toMatchObject({ roles: ["user"], isAdmin: false }); }); + +test("same-origin guard permits non-browser and same-origin calls but rejects a cross-origin browser", async () => { + const app = Fastify(); + app.get("/guard", async (request, reply) => { + const denied = requireSameOriginOrNonBrowser(request, reply); + return denied ?? { ok: true }; + }); + + expect((await app.inject({ method: "GET", url: "/guard" })).statusCode).toBe(200); + expect((await app.inject({ + method: "GET", url: "/guard", headers: { host: "127.0.0.1:8080", origin: "http://127.0.0.1:8080" }, + })).statusCode).toBe(200); + const denied = await app.inject({ + method: "GET", url: "/guard", headers: { host: "127.0.0.1:8080", origin: "https://evil.example" }, + }); + expect(denied.statusCode).toBe(403); + expect(denied.json()).toEqual({ code: "auth_forbidden", error: "This operation is not permitted" }); +}); diff --git a/backend/test/routes-pi-management.test.ts b/backend/test/routes-pi-management.test.ts index 5a2234e8..d1d98a9a 100644 --- a/backend/test/routes-pi-management.test.ts +++ b/backend/test/routes-pi-management.test.ts @@ -105,9 +105,9 @@ test("loopback-only AUTH_MODE=none may read the sanitized Pi status", async () = } }); -// Route authorization is permission-based; loopback none-mode derives its local admin principal -// from trusted installation configuration rather than a browser Origin check. -test("loopback-only management accepts cross-origin writes for its local administrator", async () => { +// A local implicit administrator has pi.manage, but a browser origin still cannot borrow that +// authority to mutate local configuration or trigger provider work. +test("loopback-only management rejects cross-origin writes for its local administrator", async () => { const service = fakeService(); const app = appWith(service); try { @@ -121,10 +121,10 @@ test("loopback-only management accepts cross-origin writes for its local adminis headers: { host: "127.0.0.1:8080", origin: "https://evil.example" }, }); - expect(configured.statusCode).toBe(200); - expect(smoke.statusCode).toBe(200); - expect(service.configure).toHaveBeenCalledOnce(); - expect(service.test).toHaveBeenCalledOnce(); + expect(configured.statusCode).toBe(403); + expect(smoke.statusCode).toBe(403); + expect(service.configure).not.toHaveBeenCalled(); + expect(service.test).not.toHaveBeenCalled(); } finally { await app.close(); } diff --git a/backend/test/routes-sessions.test.ts b/backend/test/routes-sessions.test.ts index 8d36693e..167bda4f 100644 --- a/backend/test/routes-sessions.test.ts +++ b/backend/test/routes-sessions.test.ts @@ -10,6 +10,8 @@ import { SseHub } from "../src/sse/sse-hub.js"; import { MaintenanceBarrier } from "../src/runtime/maintenance-gate.js"; import { PiProcessManager } from "../src/pi/pi-process-manager.js"; import { validateDeclarativePiConfig } from "../src/pi/managed-config.js"; +import Fastify from "fastify"; +import { sessionRoutes } from "../src/routes/sessions.js"; const FAKE = path.resolve("../harness/tests/fake_pi/fake_pi_rpc.mjs"); const SCRIPT = path.resolve("../harness/tests/fake_pi/scripts/f1_disambiguation.json"); @@ -99,6 +101,29 @@ test("upstream requests without a principal fail before a Pi runtime can be crea expect(created).toBe(false); }); +test("GET /sessions with a query still requires session.use", async () => { + const app = Fastify(); + app.addHook("preHandler", async (request) => { + request.principal = { issuer: "oidc", subject: "no-role", roles: [], permissions: [], isAdmin: false }; + }); + sessionRoutes(app, { + mgr: { get: () => undefined } as any, + tht: { sessionList: async () => [] } as any, + hub: {} as any, + getSettings: async () => ({}), + readiness: {} as any, + listModels: async () => [], + workspaceRegistry: { list: async () => [] } as any, + workspaceRuntimeSupport: () => true, + maintenanceBarrier: new MaintenanceBarrier(), + }); + + const response = await app.inject({ method: "GET", url: "/sessions?scope=mine" }); + + expect(response.statusCode).toBe(403); + expect(response.json()).toEqual({ code: "auth_forbidden", error: "This operation is not permitted" }); +}); + test("maintenance rejects new and resumed session admission without interrupting running sessions", async () => { const maintenanceBarrier = new MaintenanceBarrier(); await maintenanceBarrier.activate(); diff --git a/backend/test/routes-sql-meta.test.ts b/backend/test/routes-sql-meta.test.ts index a85e364f..f21dfefb 100644 --- a/backend/test/routes-sql-meta.test.ts +++ b/backend/test/routes-sql-meta.test.ts @@ -1,6 +1,8 @@ import { test, expect, vi } from "vitest"; +import Fastify from "fastify"; import { buildApp } from "../src/app.js"; import { loadConfig } from "../src/config.js"; +import { sqlRoutes } from "../src/routes/sql.js"; // --------------------------------------------------------------------------- // SQL routes @@ -29,6 +31,35 @@ test("POST /sessions/:id/sql/preview returns rows from injected thtRunner stub", expect(res.json()).toEqual(previewResult); }); +test("SQL lookup and execution derive harness admin compatibility from session.read_all", async () => { + const seen: boolean[] = []; + const app = Fastify(); + app.addHook("preHandler", async (request) => { + request.principal = { + issuer: "portal", subject: "inconsistent", roles: ["user"], permissions: ["session.use"], isAdmin: true, + }; + }); + const runner = { + sessionShow: async () => ({ id: "s1" }), + sqlPreview: async () => ({ columns: [], rows: [], execution_ms: 0, truncated: false }), + }; + sqlRoutes(app, { + tht: { + withPrincipal: (principal: { isAdmin: boolean }) => { + seen.push(principal.isAdmin); + return runner; + }, + } as any, + getSettings: async () => ({ workspace: "legacy" }), + workspaceRegistry: { list: async () => [] } as any, + }); + + const response = await app.inject({ method: "POST", url: "/sessions/s1/sql/preview", payload: {} }); + + expect(response.statusCode).toBe(200); + expect(seen).toEqual([false, false]); +}); + test("POST /sessions/:id/sql/preview passes limit and offset to thtRunner", async () => { let captured: any; const app = buildApp(loadConfig({ THT_HARNESS_DIR: "../harness" }), {