From b454fb478b89f6570a032ead9146e3eba4c10f30 Mon Sep 17 00:00:00 2001 From: User Date: Thu, 16 Jul 2026 18:38:40 +0200 Subject: [PATCH] fix(backend): harden principal child isolation --- .superpowers/sdd/task-5-report.md | 21 ++++++++++++ backend/src/auth/principal.ts | 26 ++++++++++++-- backend/src/pi/pi-process-manager.ts | 3 +- backend/src/routes/sessions.ts | 10 +++--- backend/src/tht/tht-runner.ts | 13 +++---- backend/test/auth.test.ts | 23 +++++++++++++ backend/test/pi-spawn-args.test.ts | 25 ++++++++++++++ backend/test/routes-sessions.test.ts | 21 ++++++++++++ backend/test/tht-runner.test.ts | 51 +++++++++++++++++++++------- 9 files changed, 167 insertions(+), 26 deletions(-) diff --git a/.superpowers/sdd/task-5-report.md b/.superpowers/sdd/task-5-report.md index 831f8acc..cebde76c 100644 --- a/.superpowers/sdd/task-5-report.md +++ b/.superpowers/sdd/task-5-report.md @@ -48,3 +48,24 @@ enforced, new sessions had no trusted principal binding, and settings were globa - Existing dependency-injected route fakes without `sessionShow` retain a narrow test seam; production `ThtRunner` always has that method, so deployed requests cannot bypass the repository authorization check. + +## Review follow-up + +### RED + +Focused regressions initially failed exactly at the three review findings: stale ambient +display names survived into both `tht` and Pi child environments; mutation/document runner +methods dropped the selected workspace; and `expandLocalHome` did not exist. + +### GREEN + +- Child environments now remove all four `THT_PRINCIPAL_*` keys from their cloned base + environment before applying the exact request principal. Regression tests prove an absent + display name does not inherit a stale ambient value in either child path. +- `setName`, `setGroup`, `archive`, `unarchive`, and `documents` now take and retain an + optional workspace. The rename route regression proves `session show` authorization and + the mutation use the same non-default workspace. +- Local principal paths expand `~`/`~/...`; existing local home and identity file modes are + repaired to POSIX `0700`/`0600` when applicable, with Windows left unchanged. +- Focused suite: `74 passed`; full backend suite: `213 passed` across `22` files, followed by + TypeScript typecheck, production build, and diff check. diff --git a/backend/src/auth/principal.ts b/backend/src/auth/principal.ts index b065c989..a58aff1f 100644 --- a/backend/src/auth/principal.ts +++ b/backend/src/auth/principal.ts @@ -1,4 +1,4 @@ -import { mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; import { randomUUID } from "node:crypto"; @@ -10,6 +10,25 @@ export interface PrincipalContext { isAdmin: boolean; } +const principalEnvKeys = [ + "THT_PRINCIPAL_ISSUER", "THT_PRINCIPAL_SUBJECT", "THT_PRINCIPAL_DISPLAY_NAME", "THT_PRINCIPAL_IS_ADMIN", +] as const; + +export function clearPrincipalEnvironment(env: NodeJS.ProcessEnv): void { + for (const key of principalEnvKeys) delete env[key]; +} + +export function expandLocalHome(path: string, home = homedir()): string { + if (path === "~") return home; + if (path.startsWith("~/")) return join(home, path.slice(2)); + return path; +} + +function harden(path: string, mode: number): void { + if (process.platform === "win32") return; + try { chmodSync(path, mode); } catch { /* best-effort parity with harness local storage */ } +} + const invalid = (value: string) => value.length === 0 || value.length > 512 || /[\u0000-\u001f\u007f]/.test(value); function required(value: unknown): string | undefined { @@ -34,12 +53,14 @@ export function upstreamPrincipal(headers: Record): PrincipalCo } export function localPrincipal(): PrincipalContext { - const home = process.env.THT_HOME ?? join(homedir(), ".thothii"); + const home = expandLocalHome(process.env.THT_HOME ?? join(homedir(), ".thothii")); const identityPath = join(home, "identity.json"); mkdirSync(home, { recursive: true, mode: 0o700 }); + harden(home, 0o700); try { const stored = JSON.parse(readFileSync(identityPath, "utf8")); if (stored?.issuer === "local" && typeof stored.subject === "string" && /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(stored.subject)) { + harden(identityPath, 0o600); return { issuer: "local", subject: stored.subject, isAdmin: false }; } throw new Error("invalid local identity"); @@ -48,6 +69,7 @@ export function localPrincipal(): PrincipalContext { const principal = { issuer: "local", subject: randomUUID() }; try { writeFileSync(identityPath, JSON.stringify(principal) + "\n", { mode: 0o600, flag: "wx" }); + harden(identityPath, 0o600); return { ...principal, isAdmin: false }; } catch (writeError: any) { // Another local request won the identity creation race; always converge on its UUID. diff --git a/backend/src/pi/pi-process-manager.ts b/backend/src/pi/pi-process-manager.ts index ba12a4bc..c9aa47f3 100644 --- a/backend/src/pi/pi-process-manager.ts +++ b/backend/src/pi/pi-process-manager.ts @@ -5,7 +5,7 @@ import { SessionBridge } from "../bridge/session-bridge.js"; import type { ThtRunner } from "../tht/tht-runner.js"; import { buildPiChildEnv, canonicalPiProvider } from "./provider-credentials.js"; import { secretValue } from "../config/secret-bundle.js"; -import { principalEnvironment, type PrincipalContext } from "../auth/principal.js"; +import { clearPrincipalEnvironment, principalEnvironment, type PrincipalContext } from "../auth/principal.js"; export interface SessionRuntime { rpc: RpcClient; @@ -55,6 +55,7 @@ export class PiProcessManager { credentialFile: this.cfg.modelApiKeyFile, additions: { THT_SESSION: sessionId, THT_AUTHOR: author }, }); + clearPrincipalEnvironment(env); if (principal) Object.assign(env, principalEnvironment(principal)); // The Thoth gate executes the deterministic `tht` CLI as a Pi tool. Give only // this managed session process the adapter values already loaded by the core diff --git a/backend/src/routes/sessions.ts b/backend/src/routes/sessions.ts index e62f000f..1aab8a13 100644 --- a/backend/src/routes/sessions.ts +++ b/backend/src/routes/sessions.ts @@ -397,7 +397,7 @@ export function sessionRoutes( try { const settings = await d.getSettings(principal); if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - await runnerFor(principal).setName(id, (req.body as any).name); + await runnerFor(principal).setName(id, (req.body as any).name, settings.workspace); } catch { return storageFailure(reply); } return reply.code(204).send(); }); @@ -407,7 +407,7 @@ export function sessionRoutes( try { const settings = await d.getSettings(principal); if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - await runnerFor(principal).setGroup(id, (req.body as any).group); + await runnerFor(principal).setGroup(id, (req.body as any).group, settings.workspace); } catch { return storageFailure(reply); } return reply.code(204).send(); }); @@ -417,7 +417,7 @@ export function sessionRoutes( try { const settings = await d.getSettings(principal); if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - await runnerFor(principal).archive(id); + await runnerFor(principal).archive(id, settings.workspace); } catch { return storageFailure(reply); } return reply.code(204).send(); }); @@ -427,7 +427,7 @@ export function sessionRoutes( try { const settings = await d.getSettings(principal); if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - await runnerFor(principal).unarchive(id); + await runnerFor(principal).unarchive(id, settings.workspace); } catch { return storageFailure(reply); } return reply.code(204).send(); }); @@ -458,7 +458,7 @@ export function sessionRoutes( try { const settings = await d.getSettings(principal); if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - return await runnerFor(principal).documents(id); + return await runnerFor(principal).documents(id, settings.workspace); } catch { return storageFailure(reply); } }); } diff --git a/backend/src/tht/tht-runner.ts b/backend/src/tht/tht-runner.ts index 5a9524ad..0ad4f28a 100644 --- a/backend/src/tht/tht-runner.ts +++ b/backend/src/tht/tht-runner.ts @@ -1,7 +1,7 @@ import { spawn } from "node:child_process"; import { existsSync } from "node:fs"; import { join } from "node:path"; -import { principalEnvironment, type PrincipalContext } from "../auth/principal.js"; +import { clearPrincipalEnvironment, principalEnvironment, type PrincipalContext } from "../auth/principal.js"; export interface ThtConfig { thtBin: string; @@ -68,6 +68,7 @@ export class ThtRunner { return new Promise((resolve) => { const env: NodeJS.ProcessEnv = { ...process.env }; delete env.THT_DATA_ROOT; + clearPrincipalEnvironment(env); if (this.cfg.dataRoot !== undefined) env.THT_DATA_ROOT = this.cfg.dataRoot; if (this.principal) Object.assign(env, principalEnvironment(this.principal)); const ch = spawn(this.cfg.thtBin, this.buildArgv(args, workspace), { @@ -159,15 +160,15 @@ export class ThtRunner { closeSession(id: string, workspace?: string) { return this.ok(["session", "close", id], workspace); } failSession(id: string, workspace?: string) { return this.ok(["session", "fail", id], workspace); } reopenSession(id: string, workspace?: string) { return this.ok(["session", "reopen", id], workspace); } - setName(id: string, name: string) { return this.ok(["session", "set-name", id, "--name", name]); } - setGroup(id: string, group: string) { return this.ok(["session", "set-group", id, "--group", group]); } - archive(id: string) { return this.ok(["session", "archive", id]); } - unarchive(id: string) { return this.ok(["session", "unarchive", id]); } + setName(id: string, name: string, workspace?: string) { return this.ok(["session", "set-name", id, "--name", name], workspace); } + setGroup(id: string, group: string, workspace?: string) { return this.ok(["session", "set-group", id, "--group", group], workspace); } + archive(id: string, workspace?: string) { return this.ok(["session", "archive", id], workspace); } + unarchive(id: string, workspace?: string) { return this.ok(["session", "unarchive", id], workspace); } async deleteSession(id: string, workspace?: string) { const { code, stderr } = await this.run(["session", "delete", id], workspace); if (code !== 0) throw new Error(`tht session delete exit ${code}: ${stderr.trim()}`); } - documents(id: string) { return this.json(["session", "documents", id, "--json"]); } + documents(id: string, workspace?: string) { return this.json(["session", "documents", id, "--json"], workspace); } preferencesGet(workspace?: string) { return this.json>(["session", "preferences", "get", "--json"], workspace); } async preferencesSet(preferences: Record, workspace?: string): Promise { diff --git a/backend/test/auth.test.ts b/backend/test/auth.test.ts index 16a79777..ffd77c9c 100644 --- a/backend/test/auth.test.ts +++ b/backend/test/auth.test.ts @@ -1,6 +1,10 @@ import { test, expect } from "vitest"; import Fastify from "fastify"; import { authPreHandler, getPrincipal } from "../src/auth/auth.js"; +import { chmodSync, mkdtempSync, rmSync, statSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { expandLocalHome, localPrincipal } from "../src/auth/principal.js"; test("local mode resolves a stable local principal", async () => { const app = Fastify(); @@ -46,3 +50,22 @@ test("upstream mode accepts only normalized proxy principal headers", async () = issuer: "portal", subject: "42", displayName: "Alice", isAdmin: true, }); }); + +test("local identity expands tilde homes and restores private POSIX permissions", () => { + expect(expandLocalHome("~/thoth-test", "/home/tester")).toBe("/home/tester/thoth-test"); + expect(expandLocalHome("~", "/home/tester")).toBe("/home/tester"); + const home = mkdtempSync(join(tmpdir(), "thoth-principal-")); + chmodSync(home, 0o755); + const previous = process.env.THT_HOME; + process.env.THT_HOME = home; + try { + localPrincipal(); + if (process.platform !== "win32") { + expect(statSync(home).mode & 0o777).toBe(0o700); + expect(statSync(join(home, "identity.json")).mode & 0o777).toBe(0o600); + } + } finally { + if (previous === undefined) delete process.env.THT_HOME; else process.env.THT_HOME = previous; + rmSync(home, { recursive: true, force: true }); + } +}); diff --git a/backend/test/pi-spawn-args.test.ts b/backend/test/pi-spawn-args.test.ts index 2488f09d..45ed3621 100644 --- a/backend/test/pi-spawn-args.test.ts +++ b/backend/test/pi-spawn-args.test.ts @@ -32,3 +32,28 @@ test("production spawnFn launches `pi --mode rpc` with no --approve (pi 0.73 dro expect(args).not.toContain("--approve"); mgr.teardown("s1"); }); + +test("Pi child replaces stale principal env and omits absent display names", async () => { + const saved = Object.fromEntries([ + "THT_PRINCIPAL_ISSUER", "THT_PRINCIPAL_SUBJECT", "THT_PRINCIPAL_DISPLAY_NAME", "THT_PRINCIPAL_IS_ADMIN", + ].map((key) => [key, process.env[key]])); + Object.assign(process.env, { + THT_PRINCIPAL_ISSUER: "stale", THT_PRINCIPAL_SUBJECT: "stale", THT_PRINCIPAL_DISPLAY_NAME: "stale", + THT_PRINCIPAL_IS_ADMIN: "true", + }); + try { + (nodeSpawn as any).mockClear(); + const mgr = new PiProcessManager(loadConfig({})); + await mgr.spawnFor("s-principal", { principal: { issuer: "portal", subject: "42", isAdmin: false } }); + const env = (nodeSpawn as any).mock.calls[0][2].env; + expect(env).toMatchObject({ + THT_PRINCIPAL_ISSUER: "portal", THT_PRINCIPAL_SUBJECT: "42", THT_PRINCIPAL_IS_ADMIN: "false", + }); + expect(env).not.toHaveProperty("THT_PRINCIPAL_DISPLAY_NAME"); + mgr.teardown("s-principal"); + } finally { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; else process.env[key] = value; + } + } +}); diff --git a/backend/test/routes-sessions.test.ts b/backend/test/routes-sessions.test.ts index fc34eec5..8416b28c 100644 --- a/backend/test/routes-sessions.test.ts +++ b/backend/test/routes-sessions.test.ts @@ -1282,6 +1282,27 @@ test("POST /sessions/:id/rename calls setName", async () => { expect(arg).toEqual({ id: "s1", name: "N" }); }); +test("rename authorizes and mutates through the same selected workspace", async () => { + const workspaces: string[] = []; + const app = buildApp(loadConfig({ AUTH_MODE: "upstream", THT_HARNESS_DIR: "../harness" }), { + thtRunner: { + withPrincipal: () => ({ + sessionShow: async (_id: string, workspace: string) => { workspaces.push(`show:${workspace}`); return { id: "s1" }; }, + setName: async (_id: string, _name: string, workspace: string) => { workspaces.push(`set:${workspace}`); }, + }), + } as any, + getSettings: () => ({ workspace: "tenant-a" }) as any, + }); + const res = await app.inject({ + method: "POST", url: "/sessions/s1/rename", payload: { name: "N" }, + headers: { + "x-thoth-principal-issuer": "portal", "x-thoth-principal-subject": "42", "x-thoth-is-admin": "false", + }, + }); + expect(res.statusCode).toBe(204); + expect(workspaces).toEqual(["show:tenant-a", "set:tenant-a"]); +}); + test("POST /sessions/:id/group calls setGroup", async () => { let arg: any; const app = mutApp({ setGroup: async (id: string, group: string) => { arg = { id, group }; } }); diff --git a/backend/test/tht-runner.test.ts b/backend/test/tht-runner.test.ts index 59f22aa5..1553beb2 100644 --- a/backend/test/tht-runner.test.ts +++ b/backend/test/tht-runner.test.ts @@ -83,6 +83,31 @@ test("run omits ambient THT_DATA_ROOT when config does not provide one", async ( } }); +test("principal-bound tht child replaces stale principal env and omits an absent display name", async () => { + const saved = Object.fromEntries([ + "THT_PRINCIPAL_ISSUER", "THT_PRINCIPAL_SUBJECT", "THT_PRINCIPAL_DISPLAY_NAME", "THT_PRINCIPAL_IS_ADMIN", + ].map((key) => [key, process.env[key]])); + Object.assign(process.env, { + THT_PRINCIPAL_ISSUER: "stale-issuer", THT_PRINCIPAL_SUBJECT: "stale-subject", + THT_PRINCIPAL_DISPLAY_NAME: "Stale Name", THT_PRINCIPAL_IS_ADMIN: "true", + }); + try { + (spawn as any).mockClear(); + const runner = new ThtRunner({ thtBin: "tht", harnessDir: "/h", configPath: "config/tht.yaml" }) + .withPrincipal({ issuer: "portal", subject: "42", isAdmin: false }); + await runner.run(["session", "list", "--json"]); + const env = (spawn as any).mock.calls[0][2].env; + expect(env).toMatchObject({ + THT_PRINCIPAL_ISSUER: "portal", THT_PRINCIPAL_SUBJECT: "42", THT_PRINCIPAL_IS_ADMIN: "false", + }); + expect(env).not.toHaveProperty("THT_PRINCIPAL_DISPLAY_NAME"); + } finally { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; else process.env[key] = value; + } + } +}); + test("run with exit != 0 propagates error with stderr", async () => { const r = new ThtRunner({ thtBin: "tht", harnessDir: "/h", configPath: "config/tht.yaml" }); r.run = async () => ({ code: 1, stdout: "", stderr: "ERRORE: boom" }); @@ -137,23 +162,25 @@ test("setName builds the right argv", async () => { const calls: string[][] = []; const r = new ThtRunner({ thtBin: "tht", harnessDir: "/h", configPath: "config/tht.yaml" }); r.run = async (args) => { calls.push(args); return { code: 0, stdout: "", stderr: "" }; }; - await r.setName("sid", "Mio nome"); + await r.setName("sid", "Mio nome", "tenant-a"); expect(calls[0]).toEqual(["session", "set-name", "sid", "--name", "Mio nome"]); }); -test("setGroup / archive / unarchive / deleteSession build argv", async () => { - const calls: string[][] = []; +test("mutation and document commands retain their requested workspace", async () => { + const calls: Array<{ args: string[]; workspace?: string }> = []; const r = new ThtRunner({ thtBin: "tht", harnessDir: "/h", configPath: "config/tht.yaml" }); - r.run = async (args) => { calls.push(args); return { code: 0, stdout: "", stderr: "" }; }; - await r.setGroup("sid", "G1"); - await r.archive("sid"); - await r.unarchive("sid"); - await r.deleteSession("sid"); + r.run = async (args, workspace) => { calls.push({ args, workspace }); return { code: 0, stdout: "[]", stderr: "" }; }; + await r.setGroup("sid", "G1", "tenant-a"); + await r.archive("sid", "tenant-a"); + await r.unarchive("sid", "tenant-a"); + await r.documents("sid", "tenant-a"); + await r.deleteSession("sid", "tenant-a"); expect(calls).toEqual([ - ["session", "set-group", "sid", "--group", "G1"], - ["session", "archive", "sid"], - ["session", "unarchive", "sid"], - ["session", "delete", "sid"], + { args: ["session", "set-group", "sid", "--group", "G1"], workspace: "tenant-a" }, + { args: ["session", "archive", "sid"], workspace: "tenant-a" }, + { args: ["session", "unarchive", "sid"], workspace: "tenant-a" }, + { args: ["session", "documents", "sid", "--json"], workspace: "tenant-a" }, + { args: ["session", "delete", "sid"], workspace: "tenant-a" }, ]); });