Task #524: Fix critical/high object-storage authorization findings

Scope: MR-H1, MR-H2, MR-M7 from .local/security/manual-review.md. Changes: - New lib/objectAuthz.ts: canUserReadObjectPath(userId, objectPath) performs an entity-lookup (avatar / app icon / service image / brand logo / pdf archive / meeting attachment) and applies the matching read role. Admin override; orphan paths denied. - routes/storage.ts: GET /api/storage/objects/* now calls canUserReadObjectPath BEFORE getObjectEntityFile and returns 404 on deny so existence is not leaked (MR-H1 fix). - routes/executive-meetings.ts: POST /executive-meetings/pdf-archives now stacks requireMutate on top of requireExecutiveAccess so read-only executive_viewer can no longer poison the archive list. pdfArchiveCreateSchema's filePath is constrained to OBJECT_PATH_RE (/objects/<id>) when supplied; omitted = synthetic print:<date> preserved (MR-H2 fix). - lib/objectAcl.ts: removed the empty enum + throwing factory that formed the MR-M7 trap. Kept getObjectAclPolicy / setObjectAclPolicy / ObjectAclPolicy / ObjectPermission so objectStorage.ts compiles. canAccessObject is now a deny-all shim with @deprecated pointer. Tests added (artifacts/api-server/tests/storage-object-authz.test.mjs): 1. Unauthenticated GET /api/storage/objects/* -> 401 2. Authed user GET orphan path -> 404 3. POST /pdf-archives by executive_viewer -> 403 4. POST /pdf-archives with free-form filePath -> 400 5. POST /pdf-archives by mutator with no filePath -> 201 (regression) 6. GET /pdf-archives by executive_viewer -> 200 (regression) Test results: all 6 new tests pass; 318/320 of the full api-server suite pass. The 2 failures (app-permissions-impact preview math, executive-meetings-notifications socket fan-out) are pre-existing and unrelated to the touched files. Residual risk: meeting-attachment lookup uses attachments::text LIKE '%<path>%' because the jsonb shape is loosely typed; safe in practice (random UUID paths) but a stricter jsonpath query could be used in a future hardening pass. Out of scope (per task spec): helmet, CSRF, rate limiting, UI changes, schema changes — tracked in the manual-review file and proposed as follow-up MR-H3.
2026-05-13 09:54:35 +00:00
parent 57f7a7aba2
commit 553aec1256
5 changed files with 415 additions and 92 deletions
@@ -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:<date>` 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");
+});