diff --git a/artifacts/api-server/src/lib/objectAcl.ts b/artifacts/api-server/src/lib/objectAcl.ts index 0d824fdb..479f8f60 100644 --- a/artifacts/api-server/src/lib/objectAcl.ts +++ b/artifacts/api-server/src/lib/objectAcl.ts @@ -1,70 +1,44 @@ import { File } from "@google-cloud/storage"; -const ACL_POLICY_METADATA_KEY = "custom:aclPolicy"; - -// Can be flexibly defined according to the use case. +// ===================================================================== +// DEPRECATED — DO NOT USE FOR ACCESS DECISIONS +// ===================================================================== // -// Examples: -// - USER_LIST: the users from a list stored in the database; -// - EMAIL_DOMAIN: the users whose email is in a specific domain; -// - GROUP_MEMBER: the users who are members of a specific group; -// - SUBSCRIBER: the users who are subscribers of a specific service / content -// creator. -export enum ObjectAccessGroupType {} +// This file used to expose a generic ACL subsystem (`ObjectAccessGroupType`, +// `createObjectAccessGroup`, `canAccessObject`) carried over from a starter +// blueprint. The subsystem was never wired into any route — the empty enum +// and throwing factory were a security trap (MR-M7 in +// `.local/security/manual-review.md`): any future caller would have either +// silently 403'd every download, or "fixed" the failure by re-opening the +// route and re-introducing MR-H1 deliberately. +// +// Authorization for `GET /api/storage/objects/*` now lives in +// `lib/objectAuthz.ts` and uses an entity-lookup model (per-entity DB +// query → per-entity read role). DO NOT add new callers of this file +// for permission decisions. +// +// What remains here is the bare minimum needed to keep +// `objectStorage.downloadObject` working: it consults the cached ACL +// metadata only to decide between `Cache-Control: public` and +// `Cache-Control: private` headers. Because `setObjectAclPolicy` is no +// longer called from anywhere in the app, `getObjectAclPolicy` will +// always return null on production data, so every response is sent +// with `Cache-Control: private` — the safe default. +// ===================================================================== -export interface ObjectAccessGroup { - type: ObjectAccessGroupType; - // The logic id that identifies qualified group members. Format depends on the - // ObjectAccessGroupType — e.g. a user-list DB id, an email domain, a group id. - id: string; -} +const ACL_POLICY_METADATA_KEY = "custom:aclPolicy"; export enum ObjectPermission { READ = "read", WRITE = "write", } -export interface ObjectAclRule { - group: ObjectAccessGroup; - permission: ObjectPermission; -} - -// Stored as object custom metadata under "custom:aclPolicy" (JSON string). +// Retained as a type so callers (objectStorage.ts) keep compiling. The +// `aclRules`/`group` machinery has been removed; only the +// owner/visibility hint is kept for the cache-header decision. export interface ObjectAclPolicy { owner: string; visibility: "public" | "private"; - aclRules?: Array; -} - -function isPermissionAllowed( - requested: ObjectPermission, - granted: ObjectPermission, -): boolean { - if (requested === ObjectPermission.READ) { - return [ObjectPermission.READ, ObjectPermission.WRITE].includes(granted); - } - return granted === ObjectPermission.WRITE; -} - -abstract class BaseObjectAccessGroup implements ObjectAccessGroup { - constructor( - public readonly type: ObjectAccessGroupType, - public readonly id: string, - ) {} - - public abstract hasMember(userId: string): Promise; -} - -function createObjectAccessGroup( - group: ObjectAccessGroup, -): BaseObjectAccessGroup { - switch (group.type) { - // Implement per access group type, e.g.: - // case "USER_LIST": - // return new UserListAccessGroup(group.id); - default: - throw new Error(`Unknown access group type: ${group.type}`); - } } export async function setObjectAclPolicy( @@ -75,7 +49,6 @@ export async function setObjectAclPolicy( if (!exists) { throw new Error(`Object not found: ${objectFile.name}`); } - await objectFile.setMetadata({ metadata: { [ACL_POLICY_METADATA_KEY]: JSON.stringify(aclPolicy), @@ -91,47 +64,22 @@ export async function getObjectAclPolicy( if (!aclPolicy) { return null; } - return JSON.parse(aclPolicy as string); + try { + return JSON.parse(aclPolicy as string) as ObjectAclPolicy; + } catch { + return null; + } } -export async function canAccessObject({ - userId, - objectFile, - requestedPermission, -}: { +/** + * @deprecated Use `canUserReadObjectPath` from `lib/objectAuthz.ts` + * instead. This shim only exists so callers that historically imported + * it keep type-checking; it always denies. + */ +export async function canAccessObject(_args: { userId?: string; objectFile: File; requestedPermission: ObjectPermission; }): Promise { - const aclPolicy = await getObjectAclPolicy(objectFile); - if (!aclPolicy) { - return false; - } - - if ( - aclPolicy.visibility === "public" && - requestedPermission === ObjectPermission.READ - ) { - return true; - } - - if (!userId) { - return false; - } - - if (aclPolicy.owner === userId) { - return true; - } - - for (const rule of aclPolicy.aclRules || []) { - const accessGroup = createObjectAccessGroup(rule.group); - if ( - (await accessGroup.hasMember(userId)) && - isPermissionAllowed(requestedPermission, rule.permission) - ) { - return true; - } - } - return false; } diff --git a/artifacts/api-server/src/lib/objectAuthz.ts b/artifacts/api-server/src/lib/objectAuthz.ts new file mode 100644 index 00000000..f5a50622 --- /dev/null +++ b/artifacts/api-server/src/lib/objectAuthz.ts @@ -0,0 +1,127 @@ +import { + db, + usersTable, + appsTable, + servicesTable, + executiveMeetingFontSettingsTable, + executiveMeetingPdfArchivesTable, + executiveMeetingsTable, + rolesTable, +} from "@workspace/db"; +import { eq, inArray, sql } from "drizzle-orm"; +import { getEffectiveRoleIds } from "../middlewares/auth"; + +// Roles that gate read access to the Executive Meetings module. +// Mirrors EXECUTIVE_READ_ROLES in middlewares/auth.ts (kept private here +// to avoid exporting that list more widely than necessary). +const EXECUTIVE_READ_ROLES: ReadonlyArray = [ + "admin", + "executive_ceo", + "executive_office_manager", + "executive_coord_lead", + "executive_coordinator", + "executive_viewer", +]; + +async function getRoleNamesForUser(userId: number): Promise> { + const ids = await getEffectiveRoleIds(userId); + if (ids.length === 0) return new Set(); + const rows = await db + .select({ name: rolesTable.name }) + .from(rolesTable) + .where(inArray(rolesTable.id, ids)); + return new Set(rows.map((r) => r.name)); +} + +/** + * Per-entity authorization for a private object path served by + * `GET /api/storage/objects/*`. Returns true iff the given user is + * allowed to read the object; false for orphan objects (never + * referenced by any known entity) and for users who lack the entity's + * read role. The route translates `false` into a 404 to avoid leaking + * which `/objects/` paths exist. + * + * Authorization model (per MR-H1 / MR-M7 in .local/security/manual-review.md): + * - admin : allow + * - users.avatar_url == path : allow any authenticated user + * - apps.image_url == path : allow any authenticated user + * - services.image_url == path : allow any authenticated user + * - executive_meeting_font_settings.logo_* : require executive read role + * - executive_meeting_pdf_archives.file_path : require executive read role + * - executive_meetings.attachments contains path : require executive read role + * - else (orphan / unknown) : deny + * + * Ordering note: a single object path can in principle be referenced + * from multiple entities. We check the broadest-permission entities + * first (avatar / app icon / service image) so that any "public-ish" + * usage wins. Executive-only entities are checked last and only matter + * if no broader reference was found. + */ +export async function canUserReadObjectPath( + userId: number, + objectPath: string, +): Promise { + if (!objectPath.startsWith("/objects/")) return false; + + const roles = await getRoleNamesForUser(userId); + if (roles.has("admin")) return true; + + // 1. Avatars — visible to anyone authenticated (used by the @-mention + // picker, the user directory, and meeting attendee headshots). + const avatar = await db + .select({ id: usersTable.id }) + .from(usersTable) + .where(eq(usersTable.avatarUrl, objectPath)) + .limit(1); + if (avatar.length > 0) return true; + + // 2. App icons — the launcher tile is shown to every authenticated user. + const app = await db + .select({ id: appsTable.id }) + .from(appsTable) + .where(eq(appsTable.imageUrl, objectPath)) + .limit(1); + if (app.length > 0) return true; + + // 3. Service images — the services list is exposed to any authed user + // (`GET /services` is gated by requireAuth only). + const svc = await db + .select({ id: servicesTable.id }) + .from(servicesTable) + .where(eq(servicesTable.imageUrl, objectPath)) + .limit(1); + if (svc.length > 0) return true; + + const hasExecutiveAccess = EXECUTIVE_READ_ROLES.some((r) => roles.has(r)); + + // 4. Executive brand logo — only executive-module roles see the PDF. + const logo = await db + .select({ id: executiveMeetingFontSettingsTable.id }) + .from(executiveMeetingFontSettingsTable) + .where(eq(executiveMeetingFontSettingsTable.logoObjectPath, objectPath)) + .limit(1); + if (logo.length > 0) return hasExecutiveAccess; + + // 5. Executive PDF archives — only executive-module roles. + const archive = await db + .select({ id: executiveMeetingPdfArchivesTable.id }) + .from(executiveMeetingPdfArchivesTable) + .where(eq(executiveMeetingPdfArchivesTable.filePath, objectPath)) + .limit(1); + if (archive.length > 0) return hasExecutiveAccess; + + // 6. Meeting attachments — jsonb array. We do a substring match on the + // serialized form because the per-element shape is loosely typed + // (see executive-meetings.ts:2078). Object paths are random UUIDs + // so substring collisions are negligible. + const meeting = await db.execute( + sql`SELECT id FROM executive_meetings WHERE attachments::text LIKE ${`%${objectPath}%`} LIMIT 1`, + ); + // drizzle-orm's execute() return shape varies by driver; pg returns + // { rows: [...] }. Be defensive. + const rows = (meeting as unknown as { rows?: unknown[] }).rows ?? []; + if (rows.length > 0) return hasExecutiveAccess; + + // Orphan path — no entity references it. Treat as not-found. + return false; +} diff --git a/artifacts/api-server/src/routes/executive-meetings.ts b/artifacts/api-server/src/routes/executive-meetings.ts index c56cb8ec..d5c4234b 100644 --- a/artifacts/api-server/src/routes/executive-meetings.ts +++ b/artifacts/api-server/src/routes/executive-meetings.ts @@ -304,9 +304,20 @@ const duplicateSchema = z.object({ // frontend and server share the same contract for POST /reorder. const reorderSchema = ExecutiveMeetingsReorderBody; +// MR-H2: `filePath` is constrained to either a server-generated +// `/objects/` path (matching OBJECT_PATH_RE — same regex used by +// the brand-logo upload schema below) or omitted (in which case the +// route derives a synthetic `print:` value server-side). +// Free-form strings are rejected so a caller cannot inject arbitrary +// paths into the archive list and chain them with the storage route. const pdfArchiveCreateSchema = z.object({ archiveDate: dateSchema, - filePath: z.string().trim().max(500).optional(), + filePath: z + .string() + .trim() + .max(500) + .regex(/^\/objects\/[A-Za-z0-9_\-./]+$/, "expected /objects/ path") + .optional(), }); // #RRGGBB hex color. @@ -3289,6 +3300,11 @@ router.get( router.post( "/executive-meetings/pdf-archives", requireExecutiveAccess, + // MR-H2: writing an archive row is a mutation; the read-only + // `executive_viewer` role must NOT be able to create archives or + // poison the archive list. Mirrors the gating on every other write + // endpoint in this module (see e.g. line 609). + requireMutate, async (req, res): Promise => { const data = parseBody(res, pdfArchiveCreateSchema, req.body); if (!data) return; diff --git a/artifacts/api-server/src/routes/storage.ts b/artifacts/api-server/src/routes/storage.ts index e9ababfd..cd6bfb74 100644 --- a/artifacts/api-server/src/routes/storage.ts +++ b/artifacts/api-server/src/routes/storage.ts @@ -5,6 +5,7 @@ import { RequestUploadUrlResponse, } from "@workspace/api-zod"; import { ObjectStorageService, ObjectNotFoundError } from "../lib/objectStorage"; +import { canUserReadObjectPath } from "../lib/objectAuthz"; import { requireAuth } from "../middlewares/auth"; const router: IRouter = Router(); @@ -89,6 +90,18 @@ router.get("/storage/objects/*path", requireAuth, async (req: Request, res: Resp const raw = req.params.path; const wildcardPath = Array.isArray(raw) ? raw.join("/") : raw; const objectPath = `/objects/${wildcardPath}`; + + // MR-H1: object-level authorization. Look up which entity (if any) + // references this path and apply that entity's read rule. Orphan + // paths and unauthorized callers BOTH get the same 404 response so + // we do not leak which `/objects/` paths exist in storage. + const userId = req.session.userId!; + const allowed = await canUserReadObjectPath(userId, objectPath); + if (!allowed) { + res.status(404).json({ error: "Object not found" }); + return; + } + const objectFile = await objectStorageService.getObjectEntityFile(objectPath); const response = await objectStorageService.downloadObject(objectFile); diff --git a/artifacts/api-server/tests/storage-object-authz.test.mjs b/artifacts/api-server/tests/storage-object-authz.test.mjs new file mode 100644 index 00000000..9897af63 --- /dev/null +++ b/artifacts/api-server/tests/storage-object-authz.test.mjs @@ -0,0 +1,219 @@ +// Tests for Task #524 — MR-H1 / MR-H2 / MR-M7. +// +// Coverage: +// 1. Unauthenticated GET /api/storage/objects/* → 401 (auth boundary). +// 2. Authenticated user GET of an orphan / unknown object path → 404 +// (entity-lookup authorization in lib/objectAuthz.ts denies orphans +// so we don't leak which object UUIDs exist in storage). +// 3. POST /api/executive-meetings/pdf-archives by an executive_viewer +// (read-only role) → 403 (requireMutate gate added per MR-H2). +// 4. POST /api/executive-meetings/pdf-archives by a mutator role with +// a free-form filePath → 400 (schema OBJECT_PATH_RE constraint +// added per MR-H2). +// 5. POST /api/executive-meetings/pdf-archives by a mutator role with +// no filePath → 201 (regression: synthetic `print:` path +// still works, archive list is not blocked for legitimate users). +// 6. GET /api/executive-meetings/pdf-archives by an executive_viewer +// → 200 (regression: read endpoint still works for read-only roles). + +import { test, before, after } from "node:test"; +import assert from "node:assert/strict"; +import pg from "pg"; + +const API_BASE = process.env.TEST_API_BASE ?? "http://localhost:8080"; +const DATABASE_URL = process.env.DATABASE_URL; +if (!DATABASE_URL) { + throw new Error("DATABASE_URL must be set to run these tests"); +} + +const TEST_PASSWORD = "TestPass123!"; +// bcrypt cost-10 hash of TEST_PASSWORD (matches the value used by the +// other executive-meetings test fixtures so we don't pay bcrypt cost +// per test seed). +const TEST_PASSWORD_HASH = + "$2b$10$Bs636ukPMyz01nKrsi.5m.JlDXSN22AVCvn8cgPWWDbo5yJRQX2vu"; + +const pool = new pg.Pool({ connectionString: DATABASE_URL }); + +const STAMP = `${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 6)}`; +const USERNAME_PREFIX = `objauthz_${STAMP}_`; + +let receiverUserId; // order_receiver only — no executive role +let viewerUserId; // executive_viewer — read-only inside executive module +let mutatorUserId; // executive_office_manager — full mutator +let receiverCookie; +let viewerCookie; +let mutatorCookie; +let archiveDateUsed; + +async function login(username, password) { + const res = await fetch(`${API_BASE}/api/auth/login`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ username, password }), + }); + assert.equal(res.status, 200, `login expected 200, got ${res.status}`); + const setCookie = res.headers.get("set-cookie"); + assert.ok(setCookie, "login response missing set-cookie header"); + const cookie = setCookie + .split(",") + .map((c) => c.split(";")[0].trim()) + .find((c) => c.startsWith("connect.sid=")); + assert.ok(cookie, "no connect.sid cookie returned by login"); + return cookie; +} + +async function seedUser(suffix, roleName) { + const username = `${USERNAME_PREFIX}${suffix}`; + const u = await pool.query( + `INSERT INTO users (username, email, password_hash, display_name_en, + preferred_language, is_active) + VALUES ($1, $2, $3, 'ObjAuthz Test', 'en', true) + RETURNING id`, + [username, `${username}@ex.com`, TEST_PASSWORD_HASH], + ); + const userId = u.rows[0].id; + await pool.query( + `INSERT INTO user_roles (user_id, role_id) + SELECT $1, id FROM roles WHERE name = $2 + ON CONFLICT DO NOTHING`, + [userId, roleName], + ); + return { userId, username }; +} + +before(async () => { + await pool.query(`DELETE FROM users WHERE username LIKE $1`, [ + `${USERNAME_PREFIX}%`, + ]); + + const r = await seedUser("receiver", "order_receiver"); + receiverUserId = r.userId; + receiverCookie = await login(r.username, TEST_PASSWORD); + + const v = await seedUser("viewer", "executive_viewer"); + viewerUserId = v.userId; + viewerCookie = await login(v.username, TEST_PASSWORD); + + const m = await seedUser("mutator", "executive_office_manager"); + mutatorUserId = m.userId; + mutatorCookie = await login(m.username, TEST_PASSWORD); +}); + +after(async () => { + for (const id of [receiverUserId, viewerUserId, mutatorUserId]) { + if (!id) continue; + await pool.query(`DELETE FROM user_roles WHERE user_id = $1`, [id]); + await pool.query(`DELETE FROM user_groups WHERE user_id = $1`, [id]); + } + // Clean up any pdf_archive rows created by this run so we don't pollute + // shared dev databases. We tagged created rows by the unique + // archiveDateUsed (a far-future date computed below). + if (archiveDateUsed) { + await pool.query( + `DELETE FROM executive_meeting_pdf_archives WHERE archive_date = $1`, + [archiveDateUsed], + ); + } + for (const id of [receiverUserId, viewerUserId, mutatorUserId]) { + if (!id) continue; + await pool.query(`DELETE FROM users WHERE id = $1`, [id]); + } + await pool.end(); +}); + +test("MR-H1: unauthenticated GET /api/storage/objects/* returns 401", async () => { + const res = await fetch(`${API_BASE}/api/storage/objects/anything-uuid`); + assert.equal(res.status, 401, `expected 401, got ${res.status}`); +}); + +test("MR-H1: authed user GET of an orphan object path returns 404 (entity-lookup deny)", async () => { + // This UUID is not referenced by any entity row in any seeded test + // database, so canUserReadObjectPath returns false and the route + // emits a 404 BEFORE touching object storage. The same status (404) + // is what an actually-missing file would return — that's intentional + // and prevents existence enumeration. + const res = await fetch( + `${API_BASE}/api/storage/objects/orphan-${STAMP}-uuid`, + { headers: { cookie: receiverCookie } }, + ); + assert.equal(res.status, 404, `expected 404, got ${res.status}`); + const body = await res.json().catch(() => ({})); + assert.equal(body.error, "Object not found"); +}); + +test("MR-H2: POST /executive-meetings/pdf-archives by executive_viewer (read-only) returns 403", async () => { + // Use a far-future date so we don't collide with real fixture rows + // and so the after-hook can clean up by date alone. + archiveDateUsed = "2099-12-31"; + const res = await fetch( + `${API_BASE}/api/executive-meetings/pdf-archives`, + { + method: "POST", + headers: { + "Content-Type": "application/json", + cookie: viewerCookie, + }, + body: JSON.stringify({ archiveDate: archiveDateUsed }), + }, + ); + assert.equal(res.status, 403, `expected 403, got ${res.status}`); + const body = await res.json().catch(() => ({})); + assert.equal(body.code, "forbidden"); +}); + +test("MR-H2: POST /executive-meetings/pdf-archives with free-form filePath returns 400", async () => { + const res = await fetch( + `${API_BASE}/api/executive-meetings/pdf-archives`, + { + method: "POST", + headers: { + "Content-Type": "application/json", + cookie: mutatorCookie, + }, + body: JSON.stringify({ + archiveDate: archiveDateUsed, + // Path-traversal attempt that passes max(500) but must be + // rejected by the OBJECT_PATH_RE regex. + filePath: "../../etc/passwd", + }), + }, + ); + assert.equal(res.status, 400, `expected 400, got ${res.status}`); +}); + +test("regression: POST /executive-meetings/pdf-archives by mutator with no filePath returns 201 (synthetic print: path)", async () => { + const res = await fetch( + `${API_BASE}/api/executive-meetings/pdf-archives`, + { + method: "POST", + headers: { + "Content-Type": "application/json", + cookie: mutatorCookie, + }, + body: JSON.stringify({ archiveDate: archiveDateUsed }), + }, + ); + assert.equal(res.status, 201, `expected 201, got ${res.status}`); + const body = await res.json(); + assert.equal(body.archiveDate, archiveDateUsed); + assert.equal(body.filePath, `print:${archiveDateUsed}`); + assert.equal(typeof body.version, "number"); +}); + +test("regression: GET /executive-meetings/pdf-archives by executive_viewer (read-only) returns 200", async () => { + const res = await fetch( + `${API_BASE}/api/executive-meetings/pdf-archives?date=${archiveDateUsed}`, + { headers: { cookie: viewerCookie } }, + ); + assert.equal(res.status, 200, `expected 200, got ${res.status}`); + const body = await res.json(); + assert.ok(Array.isArray(body.archives), "expected archives array"); + // The row inserted by the previous test must be visible to the + // read-only role — confirms requireMutate did NOT leak onto the read + // endpoint. + const ours = body.archives.find( + (a) => a.filePath === `print:${archiveDateUsed}`, + ); + assert.ok(ours, "expected the seeded archive row to be visible to executive_viewer"); +});