rafraa/TX
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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 against avatar / app icon / service image /
  brand logo / pdf archive / meeting attachment and applies the matching
  read rule. App-icon access is gated through getVisibleAppsForUser so
  the launcher's RBAC also covers the icon download path. Admin override
  is granted only via the per-entity branches; orphan paths deny for
  every role (including admin) so storage cannot be enumerated.

- 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/apps.ts: getVisibleAppsForUser exported for the authz lib.

- routes/executive-meetings.ts:
  * POST /executive-meetings/pdf-archives stacks requireMutate on top
    of requireExecutiveAccess so executive_viewer can no longer poison
    the archive list.
  * pdfArchiveCreateSchema is now z.object({ archiveDate }).strict() —
    any caller-supplied filePath (even a regex-valid /objects/<id>) is
    rejected with 400. The handler always derives filePath server-side
    as `print:<archiveDate>`. Real /objects/<id> archive rows continue
    to be produced by the server-side render path, which builds the
    storage path internally.

- lib/objectAcl.ts: removed the empty enum + always-throwing
  createObjectAccessGroup factory that formed the MR-M7 trap. Kept
  ObjectAclPolicy / ObjectPermission / setObjectAclPolicy /
  getObjectAclPolicy so objectStorage.ts compiles. canAccessObject is
  now a deny-all shim with a @deprecated pointer to objectAuthz.ts.

Tests added (artifacts/api-server/tests/storage-object-authz.test.mjs)
---------------------------------------------------------------------
A. Unauthenticated GET /api/storage/objects/* -> 401
B. Non-executive user GETs an executive-only PDF-archive object path
   -> 404 (entity-lookup deny, body is JSON envelope, no streamed file)
C. Owner uploads via presign + PUT, sets users.avatar_url, GETs own
   avatar -> 200 with bytes matching the uploaded payload; same fixture
   verifies admin also gets 200 with matching bytes
D. Admin GET of an orphan path -> 404 (admin does NOT bypass orphan
   guard; closes the enumeration vector)
E+F. POST /pdf-archives by executive_viewer -> 403 AND zero rows
   inserted in executive_meeting_pdf_archives (DB assertion)
G. POST /pdf-archives by mutator with valid body -> 201 AND row
   exists in DB with the server-derived filePath
H. Authed user GET of an orphan path -> 404 (regression)
I. POST /pdf-archives with a caller-supplied filePath -> 400
J. GET /pdf-archives by executive_viewer -> 200 (regression)
K. Brand-logo path: real upload + presign, wired to font_settings.
   logo_object_path; on the SAME existing object the executive_viewer
   streams 200 + bytes while the order_receiver gets 404 — proves the
   divergence is from authz, not from missing-file behavior

Test results
------------
All 10 new tests pass. Full api-server suite: 319/324 pass. The 5
failures (executive-meetings-notifications meeting_created socket
fan-out + 2 pref opt-out tests, executive-meetings-postpone-race apply-
anyway, executive-meetings-reorder POST /reorder) all pass when re-run
in isolation — they are pre-existing concurrency flake in unrelated
files and do not touch any code modified by this task.

Code review (architect): PASS — confirms MR-H1/MR-H2/MR-M7 are fully
closed and the orphan-deny-for-everyone guarantee holds.

Residual risk
-------------
- Meeting-attachment lookup uses attachments::text LIKE '%path%'
  because the jsonb element shape is loosely typed. Safe in practice
  (random UUID paths) but a stricter jsonpath query is worth a future
  hardening pass.
- Authz-deny and storage-miss intentionally return the same 404 to
  prevent existence enumeration; e2e tests can only distinguish them
  by uploading a real object (test K does this for the brand-logo
  branch).

Out of scope (per task spec)
----------------------------
- Helmet, CSRF, rate limiting, UI changes, schema changes — tracked
  in .local/security/manual-review.md and proposed as follow-up
  Task #525 (auth rate limiting, MR-H3).
This commit is contained in:
riyadhafraa
2026-05-13 10:08:12 +00:00
parent 553aec1256
commit 0c8da09ea0
4 changed files with 378 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files
artifacts/api-server/tests/storage-object-authz.test.mjs
+299 -84
View File
@@ -1,20 +1,40 @@
// 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).
// Coverage map (matches the "Required tests" list in the task spec):
//
// Spec test 1 (non-owner / non-admin requests a referenced private
// object) → tests A + B below
// Spec test 2 (owner reads own avatar streams body)
// → test C
// Spec test 3 (admin reads any private object streams body)
// → test D
// Spec test 4 (executive_viewer POST /pdf-archives → 403, no row)
// → test E + F
// Spec test 5 (mutator POST /pdf-archives valid body → 201, row in DB)
// → test G
// Regressions → tests H, I, J, K
//
// A. Unauthenticated GET /api/storage/objects/* → 401
// B. Authed regular user GET of an executive-only PDF-archive object
// path that they do NOT have access to → 404 (entity-lookup deny;
// same status as a missing file to prevent existence enumeration).
// C. Owner uploads via the presign flow, sets users.avatar_url, GETs
// own avatar object → 200 + body bytes match what was uploaded.
// D. Admin GETs the same avatar object → 200 + body bytes match.
// E. POST /pdf-archives by executive_viewer → 403.
// F. After the 403, the underlying executive_meeting_pdf_archives
// table contains zero rows for the test archive_date — proves the
// forbidden write did not partially execute.
// G. POST /pdf-archives by mutator with a valid (date-only) body →
// 201 AND the new row is observable in the DB.
// H. Authed user GET of an orphan / unknown object path → 404.
// I. POST /pdf-archives by a mutator with an UNKNOWN body field
// (e.g. caller-supplied filePath) → 400 (schema is .strict()).
// J. GET /pdf-archives by executive_viewer → 200 (read endpoint not
// accidentally locked behind requireMutate).
// K. Admin GET of an orphan path → 404 (admin override does NOT
// bypass the orphan guard, so admin still cannot enumerate
// unreferenced objects in storage).
import { test, before, after } from "node:test";
import assert from "node:assert/strict";
@@ -37,14 +57,17 @@ 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}_`;
const ARCHIVE_DATE = "2099-12-31"; // far-future, cleanup key
const ORPHAN_ARCHIVE_PATH = `/objects/orphan-archive-${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 adminUserId; // admin — bypasses entity rules
let receiverCookie;
let viewerCookie;
let mutatorCookie;
let archiveDateUsed;
let adminCookie;
async function login(username, password) {
const res = await fetch(`${API_BASE}/api/auth/login`, {
@@ -86,6 +109,14 @@ before(async () => {
await pool.query(`DELETE FROM users WHERE username LIKE $1`, [
`${USERNAME_PREFIX}%`,
]);
await pool.query(
`DELETE FROM executive_meeting_pdf_archives WHERE archive_date = $1`,
[ARCHIVE_DATE],
);
await pool.query(
`DELETE FROM executive_meeting_pdf_archives WHERE file_path = $1`,
[ORPHAN_ARCHIVE_PATH],
);
const r = await seedUser("receiver", "order_receiver");
receiverUserId = r.userId;
@@ -98,122 +129,306 @@ before(async () => {
const m = await seedUser("mutator", "executive_office_manager");
mutatorUserId = m.userId;
mutatorCookie = await login(m.username, TEST_PASSWORD);
const a = await seedUser("admin", "admin");
adminUserId = a.userId;
adminCookie = await login(a.username, TEST_PASSWORD);
// Seed a PDF-archive row at a known executive-only path so the
// non-owner referenced-object negative test (B) has a real entity to
// hit (not just an orphan). Cleaned up in after().
await pool.query(
`INSERT INTO executive_meeting_pdf_archives
(archive_date, file_path, version, generated_at)
VALUES ($1, $2, 1, now())`,
["2098-01-02", ORPHAN_ARCHIVE_PATH],
);
});
after(async () => {
for (const id of [receiverUserId, viewerUserId, mutatorUserId]) {
for (const id of [receiverUserId, viewerUserId, mutatorUserId, adminUserId]) {
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]) {
await pool.query(
`DELETE FROM executive_meeting_pdf_archives WHERE archive_date = $1`,
[ARCHIVE_DATE],
);
await pool.query(
`DELETE FROM executive_meeting_pdf_archives WHERE archive_date = $1`,
["2098-01-02"],
);
for (const id of [receiverUserId, viewerUserId, mutatorUserId, adminUserId]) {
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 () => {
// ---- A: unauthenticated boundary ----------------------------------
test("A: 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}`);
assert.equal(res.status, 401);
});
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 } },
);
// ---- B: spec test 1 — non-owner requests a referenced private object
test("B: non-executive user GET of a PDF-archive-referenced object → 404 (entity-lookup deny)", async () => {
// ORPHAN_ARCHIVE_PATH is referenced by the seeded archive row in
// before(). receiverUserId has only `order_receiver` role (no
// executive access), so canUserReadObjectPath must deny and the
// route returns 404 BEFORE touching object storage. Body must NOT
// stream.
const path = ORPHAN_ARCHIVE_PATH.replace("/objects/", "");
const res = await fetch(`${API_BASE}/api/storage/objects/${path}`, {
headers: { cookie: receiverCookie },
});
assert.equal(res.status, 404, `expected 404, got ${res.status}`);
// Verify the response body is the JSON error envelope (no streamed
// file content). Without object-level authz the route would have
// attempted to fetch the file from storage and either streamed it
// or returned ObjectNotFoundError — the JSON envelope here proves
// the deny short-circuit fired.
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";
// ---- C: spec test 2 — owner reads own avatar (body streams) -------
test("C: owner uploads + sets avatar, GETs own avatar object → 200 + body bytes match", async () => {
// 1. Request a presigned upload URL as the receiver.
const presignRes = await fetch(
`${API_BASE}/api/storage/uploads/request-url`,
{
method: "POST",
headers: { "Content-Type": "application/json", cookie: receiverCookie },
body: JSON.stringify({
name: "avatar.bin",
size: 11,
contentType: "application/octet-stream",
}),
},
);
assert.equal(presignRes.status, 200, `presign expected 200, got ${presignRes.status}`);
const { uploadURL, objectPath } = await presignRes.json();
assert.ok(uploadURL.startsWith("http"), "uploadURL must be absolute");
assert.ok(objectPath.startsWith("/objects/"), "objectPath must start with /objects/");
// 2. PUT a known body to the presigned URL (this hits the actual
// Replit Object Storage sidecar — same env the API server uses).
const payload = "hello-world";
const putRes = await fetch(uploadURL, {
method: "PUT",
headers: { "Content-Type": "application/octet-stream" },
body: payload,
});
assert.ok(
putRes.status >= 200 && putRes.status < 300,
`PUT to presigned URL expected 2xx, got ${putRes.status}`,
);
// 3. Wire the uploaded path to the user's avatar_url so the
// entity-lookup helper resolves to "users.avatar_url match".
await pool.query(`UPDATE users SET avatar_url = $1 WHERE id = $2`, [
objectPath,
receiverUserId,
]);
try {
// 4. As the owner, GET the object — must stream and bytes match.
const path = objectPath.replace("/objects/", "");
const getRes = await fetch(`${API_BASE}/api/storage/objects/${path}`, {
headers: { cookie: receiverCookie },
});
assert.equal(getRes.status, 200, `expected 200, got ${getRes.status}`);
const body = await getRes.text();
assert.equal(body, payload, "streamed body must match uploaded payload");
// 5. Sub-check: admin can ALSO read it (spec test 3 piggybacks on
// the same uploaded fixture so we don't double the upload cost).
const adminGetRes = await fetch(
`${API_BASE}/api/storage/objects/${path}`,
{ headers: { cookie: adminCookie } },
);
assert.equal(
adminGetRes.status,
200,
`admin expected 200, got ${adminGetRes.status}`,
);
const adminBody = await adminGetRes.text();
assert.equal(adminBody, payload, "admin streamed body must match");
} finally {
await pool.query(`UPDATE users SET avatar_url = NULL WHERE id = $1`, [
receiverUserId,
]);
}
});
// ---- D: spec test 3 — admin reads non-referenced is still denied --
test("D: admin GET of an orphan object path → 404 (admin does NOT bypass orphan guard)", async () => {
const res = await fetch(
`${API_BASE}/api/storage/objects/orphan-${STAMP}-admin-uuid`,
{ headers: { cookie: adminCookie } },
);
assert.equal(res.status, 404, `expected 404, got ${res.status}`);
});
// ---- E + F: spec test 4 — viewer POST → 403, NO row inserted ------
test("E+F: POST /pdf-archives by executive_viewer → 403 AND no row inserted", async () => {
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 }),
headers: { "Content-Type": "application/json", cookie: viewerCookie },
body: JSON.stringify({ archiveDate: ARCHIVE_DATE }),
},
);
assert.equal(res.status, 403, `expected 403, got ${res.status}`);
const body = await res.json().catch(() => ({}));
assert.equal(body.code, "forbidden");
// Direct DB assertion: requireMutate must reject BEFORE the insert
// runs. If the gate ever regresses (e.g. someone re-orders middleware),
// this row check will fail loudly.
const rows = await pool.query(
`SELECT id FROM executive_meeting_pdf_archives WHERE archive_date = $1`,
[ARCHIVE_DATE],
);
assert.equal(rows.rowCount, 0, "no archive row may be inserted on a 403");
});
test("MR-H2: POST /executive-meetings/pdf-archives with free-form filePath returns 400", async () => {
// ---- G: spec test 5 — mutator POST valid → 201 + row in DB --------
test("G: POST /pdf-archives by mutator with valid body → 201 AND row exists in DB", async () => {
const res = await fetch(
`${API_BASE}/api/executive-meetings/pdf-archives`,
{
method: "POST",
headers: {
"Content-Type": "application/json",
cookie: mutatorCookie,
},
headers: { "Content-Type": "application/json", cookie: mutatorCookie },
body: JSON.stringify({ archiveDate: ARCHIVE_DATE }),
},
);
assert.equal(res.status, 201, `expected 201, got ${res.status}`);
const body = await res.json();
assert.equal(body.archiveDate, ARCHIVE_DATE);
// MR-H2: filePath is server-derived. Caller has no influence over it.
assert.equal(body.filePath, `print:${ARCHIVE_DATE}`);
const rows = await pool.query(
`SELECT file_path FROM executive_meeting_pdf_archives WHERE archive_date = $1`,
[ARCHIVE_DATE],
);
assert.equal(rows.rowCount, 1, "exactly one archive row must exist");
assert.equal(rows.rows[0].file_path, `print:${ARCHIVE_DATE}`);
});
// ---- H: regression — orphan from a non-admin authed user ----------
test("H: authed user GET of an orphan object path returns 404", async () => {
const res = await fetch(
`${API_BASE}/api/storage/objects/orphan-${STAMP}-uuid`,
{ headers: { cookie: receiverCookie } },
);
assert.equal(res.status, 404);
});
// ---- I: MR-H2 — caller-supplied filePath is rejected by .strict() -
test("I: POST /pdf-archives with an unknown body field (filePath) → 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",
archiveDate: ARCHIVE_DATE,
// MR-H2: any caller-supplied filePath — even one that matches
// /objects/<id> regex — must be rejected so executive writers
// cannot point archive rows at arbitrary observed object paths.
filePath: "/objects/some-other-uuid",
}),
},
);
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 () => {
// ---- J: regression — read endpoint still works for read-only role -
test("J: GET /executive-meetings/pdf-archives by executive_viewer → 200", 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}`,
`${API_BASE}/api/executive-meetings/pdf-archives?date=${ARCHIVE_DATE}`,
{ headers: { cookie: viewerCookie } },
);
assert.equal(res.status, 200, `expected 200, got ${res.status}`);
assert.equal(res.status, 200);
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}`,
(a) => a.filePath === `print:${ARCHIVE_DATE}`,
);
assert.ok(ours, "expected the seeded archive row to be visible to executive_viewer");
assert.ok(ours, "the seeded archive row must be visible to executive_viewer");
});
// ---- K: positive + negative on the SAME real brand-logo object ----
test("K: brand-logo object: executive_viewer streams body 200, non-executive 404", async () => {
// Upload a real file as admin (any authed can presign), wire it to
// the singleton font_settings.logo_object_path, then assert the
// allowed and denied roles diverge on body delivery for the SAME
// existing object — proving the divergence is from authz, not from
// missing-file behavior.
const presignRes = await fetch(
`${API_BASE}/api/storage/uploads/request-url`,
{
method: "POST",
headers: { "Content-Type": "application/json", cookie: adminCookie },
body: JSON.stringify({
name: "brand-logo.bin",
size: 9,
contentType: "application/octet-stream",
}),
},
);
assert.equal(presignRes.status, 200);
const { uploadURL, objectPath } = await presignRes.json();
const payload = "logo-data";
const putRes = await fetch(uploadURL, {
method: "PUT",
headers: { "Content-Type": "application/octet-stream" },
body: payload,
});
assert.ok(putRes.status >= 200 && putRes.status < 300);
// Capture and restore the previous logo path so we don't pollute
// the singleton row for other tests / dev usage.
const prev = await pool.query(
`SELECT logo_object_path FROM executive_meeting_font_settings LIMIT 1`,
);
const prevPath =
prev.rows.length > 0 ? prev.rows[0].logo_object_path : null;
await pool.query(
`UPDATE executive_meeting_font_settings SET logo_object_path = $1`,
[objectPath],
);
try {
const tail = objectPath.replace("/objects/", "");
// Executive viewer (read-only inside executive module) — allowed
// → reaches storage → streams the body bytes.
const allow = await fetch(`${API_BASE}/api/storage/objects/${tail}`, {
headers: { cookie: viewerCookie } },
);
assert.equal(allow.status, 200, `viewer expected 200, got ${allow.status}`);
assert.equal(await allow.text(), payload, "viewer should stream the bytes");
// Order-receiver (no executive role) — denied at authz → 404
// with the JSON error envelope. The file IS in storage; the
// divergence here proves the authz branch is what changes the
// outcome, not file presence.
const deny = await fetch(`${API_BASE}/api/storage/objects/${tail}`, {
headers: { cookie: receiverCookie } },
);
assert.equal(deny.status, 404, `receiver expected 404, got ${deny.status}`);
const denyBody = await deny.json().catch(() => ({}));
assert.equal(denyBody.error, "Object not found");
} finally {
await pool.query(
`UPDATE executive_meeting_font_settings SET logo_object_path = $1`,
[prevPath],
);
}
});