feat(uploads): ADR-002 Phase 1b-a — signed-URL mint + ACL + audit log by samxu01 · Pull Request #216 · Team-Commonly/commonly

samxu01 · 2026-04-18T19:03:56Z

Plumbing for closing the GET authorization gap on /api/uploads/:fileName
without breaking every during rollout. Ships the mechanism; the
follow-up (Phase 1b-b) removes public-read and requires a valid token or
header auth on GET.

Backend:

backend/services/attachmentAccess.ts — signAttachmentToken /
verifyAttachmentToken (JWT with pur:'upload' purpose claim, 5-min TTL,
bound to fileName+userId); canReadAttachment runs the Phase-1 URL-substring
scan across owner, profile pictures, Post.image/content, PG messages
content. File name is shape-guarded before any DB query (ReDoS guard on
the Mongo $regex and parity with the PG ILIKE).
backend/routes/uploads.ts — GET /:fileName/url mints a signed URL after
auth + rate-limit (30/min/user) + ACL. Audit-logs each successful mint
fire-and-forget so audit DB failures don't 500 the mint.
backend/models/AuditLog.ts + backend/services/auditService.ts — minimal
append-only audit record.

Frontend:

frontend/src/utils/signedAttachmentUrl.ts — async helper that calls the
mint endpoint, caches per fileName until shortly before TTL expiry, and
coalesces concurrent calls for the same file.

Also:

Move REVIEW.md to repo root so Claude self-review picks it up, and
update the CLAUDE.md pointer.
Self-review caught a ReDoS path (fileName flowed directly into Mongo
$regex); fix + test included in this commit.

Tests: 14 cases on attachmentAccess (sign/verify + ACL fan-out + ReDoS
guard), 5 on the mint route, 7 on the frontend helper. All pass.

https://claude.ai/code/session_01JGkwSJkDKerKRsnzUe8Mxe

Plumbing for closing the GET authorization gap on /api/uploads/:fileName without breaking every <img src> during rollout. Ships the mechanism; the follow-up (Phase 1b-b) removes public-read and requires a valid token or header auth on GET. Backend: - backend/services/attachmentAccess.ts — signAttachmentToken / verifyAttachmentToken (JWT with pur:'upload' purpose claim, 5-min TTL, bound to fileName+userId); canReadAttachment runs the Phase-1 URL-substring scan across owner, profile pictures, Post.image/content, PG messages content. File name is shape-guarded before any DB query (ReDoS guard on the Mongo $regex and parity with the PG ILIKE). - backend/routes/uploads.ts — GET /:fileName/url mints a signed URL after auth + rate-limit (30/min/user) + ACL. Audit-logs each successful mint fire-and-forget so audit DB failures don't 500 the mint. - backend/models/AuditLog.ts + backend/services/auditService.ts — minimal append-only audit record. Frontend: - frontend/src/utils/signedAttachmentUrl.ts — async helper that calls the mint endpoint, caches per fileName until shortly before TTL expiry, and coalesces concurrent calls for the same file. Also: - Move REVIEW.md to repo root so Claude self-review picks it up, and update the CLAUDE.md pointer. - Self-review caught a ReDoS path (fileName flowed directly into Mongo $regex); fix + test included in this commit. Tests: 14 cases on attachmentAccess (sign/verify + ACL fan-out + ReDoS guard), 5 on the mint route, 7 on the frontend helper. All pass. https://claude.ai/code/session_01JGkwSJkDKerKRsnzUe8Mxe

samxu01

Solid Phase 1b-a landing overall — JWT sign/verify, fire-and-forget audit, frontend helper with cache+inflight coalescing, ReDoS shape guard, and good test coverage (14 + 5 + 7). The split into 1b-a (plumbing) and 1b-b (flip) in the ADR is the right sequencing.

Blocking before merge:

Rate-limit ordering bug (routes/uploads.ts:137) — mintRateLimit runs before auth, so the keyGenerator always sees req.userId === undefined and collapses every caller to an IP bucket. The comment promises "30/min/user"; the code implements "30/min/IP". Users on shared NATs share one bucket. Swap middleware order.

Blocking before the Phase 1b-b flip (not necessarily this PR, but before public-read is removed):

Post.findOne false-deny (services/attachmentAccess.ts:115) — when a file is referenced by multiple posts (public + pod-scoped), Mongo's non-deterministic findOne can return a pod-scoped row and deny a viewer who actually has access via a public post. Fan out to .find().limit(N) and check each.
PG LIMIT 1 false-deny (services/attachmentAccess.ts:166) — same shape; pick up to N distinct pods and check membership against any.
Profile-picture URL shape mismatch (services/attachmentAccess.ts:112) — urlFragment is relative, but uploads.ts:96 returns an absolute URL after upload and userController.ts:47 stores whatever the client sends. The equality match silently misses most avatars. Normalize on read or match against both shapes.

Nits:

Unbounded frontend cache (signedAttachmentUrl.ts:33) — fine for 1b-a, worth an LRU cap before heavy call-site migration in 1b-b.

Once #1 is fixed, this PR is safe to ship as 1b-a plumbing; #2–#4 must be resolved before 1b-b flips GET to deny-by-default, or legitimate viewers will see broken images.

Generated by Claude Code

samxu01 · 2026-04-18T19:11:41Z

+// ADR-002 Phase 1b: signed-URL mint endpoint. Declared before the bare
+// `:fileName` GET so Express doesn't match `/:fileName/url` as a fileName
+// containing a slash.
+router.get('/:fileName/url', mintRateLimit, auth, async (req: AuthReq, res: Res) => {


Middleware ordering — rate-limit key is always the IP today.

mintRateLimit runs before auth, so when keyGenerator evaluates req.userId || req.ip || 'anon', req.userId is always undefined (auth hasn't run yet). Every caller collapses to an IP-based bucket, which means:

Users on the same office/college NAT (or behind an L7 proxy where req.ip is the shared ingress) share a single 30/min bucket.

The comment above (30 mints/min/user) isn't actually what the code implements.

Fix is just swapping the order: router.get('/:fileName/url', auth, mintRateLimit, ...). The tests don't catch this because the auth stub and the rate limiter both use the same header/IP in isolation.

Generated by Claude Code

Fixed in 299a0d2 — swapped to auth, mintRateLimit so the keyGenerator sees req.userId.

Generated by Claude Code

samxu01 · 2026-04-18T19:11:51Z

+  const profileUser = await User.findOne({ profilePicture: urlFragment }).select('_id').lean();
+  if (profileUser) return true;
+
+  const post = await Post.findOne({


False-deny bug: findOne picks one referencing post; if Mongo returns a pod-scoped post first, a legit viewer referenced in a different post gets denied.

Concrete failure: a file is pasted into (a) a public post with podId: null, and (b) a pod-scoped post in pod A. findOne is free to return either — no ordering is specified. If it returns the pod-A post and the viewer isn't a member of pod A, this branch falls through to the messages check and eventually returns false, even though the public post grants access to everyone.

Once Phase 1b-b flips GET to deny-by-default, this becomes a user-visible "broken image" intermittently.

Suggested fix: fan out, not pick-one.

const posts = await Post.find({ $or: [...] }).select('_id podId').limit(10).lean(); for (const p of posts) { if (!p.podId) return true; const member = await Pod.findOne({ _id: p.podId, members: userId }).select('_id').lean(); if (member) return true; }

Same shape applies to findMessagePodReferencingFile below (see related comment).

Generated by Claude Code

Fixed in 299a0d2 — fan-out via Post.find(...).limit(20), any public referencing post allows, then a single Pod.findOne({ _id: { $in: scopedPodIds }, members: userId }) covers all scoped pods. Regression test added: "allows read when the file is referenced in BOTH a hidden pod AND a public post (fan-out)".

Generated by Claude Code

samxu01 · 2026-04-18T19:11:55Z

+    if (!pool?.query) return null;
+    const needle = `%${urlFragment}%`;
+    const { rows } = await pool.query(
+      'SELECT pod_id FROM messages WHERE content ILIKE $1 LIMIT 1',


Same LIMIT 1 false-deny here. SELECT pod_id ... LIMIT 1 with no ORDER BY returns an arbitrary single pod when the file is referenced in multiple pods' messages. If the viewer is a member of pod B but PG happens to return pod A first, access is denied despite pod B granting it.

Fix: fetch distinct pods up to a bounded N and check membership against any.

SELECT DISTINCT pod_id FROM messages WHERE content ILIKE $1 LIMIT 20

Then:

const pod = await Pod.findOne({ _id: { $in: rows.map(r => r.pod_id) }, members: userId }) .select('_id').lean(); if (pod) return true;

One query instead of N, and it's correct across multi-pod references.

Generated by Claude Code

Fixed in 299a0d2 — SELECT DISTINCT pod_id ... LIMIT 20, then one Pod.findOne({ _id: { $in: podIds }, members: userId }). Regression test added: "allows read across multiple referencing pods when viewer is a member of any".

Generated by Claude Code

samxu01 · 2026-04-18T19:12:02Z

+
+  const urlFragment = `/api/uploads/${fileName}`;
+
+  const profileUser = await User.findOne({ profilePicture: urlFragment }).select('_id').lean();


Profile-picture ACL check probably misses most users due to URL shape mismatch.

urlFragment is the relative path /api/uploads/${fileName}.

backend/routes/uploads.ts:96 (unchanged by this PR) returns the absolute URL ${protocol}://${host}/api/uploads/${fileName} to the client after upload.

controllers/userController.ts:47 writes whatever the client sends into user.profilePicture.

So in practice user.profilePicture is often stored as https://api-dev.commonly.me/api/uploads/xyz.png, and this equality match against the relative /api/uploads/xyz.png fragment fails to find the user → access denied, even though the file really is someone's avatar.

Suggested fix: normalize on read.

const profileUser = await User.findOne({ profilePicture: { $in: [urlFragment, `http://${host}${urlFragment}`, `https://${host}${urlFragment}`] } }).select('_id').lean();

Or better: store profile pictures in a normalized shape going forward (Phase 2's structured attachments model is the right home for this).

Generated by Claude Code

Fixed in 299a0d2 — matcher is now a substring $regex on the URL fragment (regex-escaped), so both the relative /api/uploads/<f> and absolute https://api-dev.commonly.me/api/uploads/<f> shapes match. Regression test added: "matches profile pictures stored as absolute URLs (substring regex)". Write-time normalization agreed as Phase 2 work.

Generated by Claude Code

samxu01 · 2026-04-18T19:12:06Z

+  expiresAtMs: number;
+}
+
+const cache: Map<string, CacheEntry> = new Map();


Nit: cache is an unbounded Map. A long-lived SPA that scrolls through hundreds of feeds will grow this indefinitely — entries are only pruned when explicitly re-requested (via the freshness check in getSignedAttachmentUrl), but files that are requested once and never again stay forever.

For Phase 1b-a this is fine in practice (TTL=5min, feed sizes are small), but a bounded LRU (e.g. cap at 500 entries) would be safer once call sites get migrated in 1b-b. Not blocking.

Generated by Claude Code

Fixed in 299a0d2 — bounded at 500 with FIFO eviction via Map insertion order. Test added verifies the 501st entry evicts the 1st while the 250th still hits cache.

Generated by Claude Code

… profile-pic URL shape, bounded cache Four real bugs caught in review + one bounded-cache nit, all landed here. backend/routes/uploads.ts - Swap `auth` before `mintRateLimit`. With the previous order the rate limiter evaluated `req.userId || req.ip` before auth populated `req.userId`, so every caller collapsed to an IP-based bucket and NAT'd users (offices, colleges, L7 proxies) shared one 30/min limit. backend/services/attachmentAccess.ts - Post ACL: `findOne` → `find(...).limit(20)`. `findOne` arbitrarily picked one referencing post; if Mongo returned a pod-scoped post the viewer couldn't see, the separate public post that would have granted access was missed. Fan-out checks every returned post for a public (no podId) match, then a single `Pod.findOne({ _id: { $in: [...] } })` covers all scoped pods in one query. - PG message scan: `SELECT pod_id ... LIMIT 1` → `SELECT DISTINCT pod_id ... LIMIT 20`. Same false-deny shape: if a file was referenced across pods A and B and PG returned A first, a viewer in B was denied. - Profile pictures: substring regex on profilePicture (was: exact match against relative `/api/uploads/<f>`). POST /api/uploads returns an absolute URL, and User.profilePicture stores whatever the client sends — usually `https://api-dev.commonly.me/api/uploads/<f>`, which would never equal the relative fragment. Substring match on the regex-escaped URL fragment covers both shapes without a backfill. frontend/src/utils/signedAttachmentUrl.ts - Bound the cache at 500 entries with FIFO eviction via Map insertion order. A long-lived SPA scrolling through many feeds would otherwise grow the cache indefinitely. Tests updated: Post fan-out regression (BOTH hidden pod AND public post reference the file — must allow), PG multi-pod regression (viewer in pod B, PG returns pod A first — must allow), absolute-URL profile-pic match, cache-eviction boundary (501st entry evicts the 1st while keeping the 250th). 37/37 tests pass (29 backend + 8 frontend). Types check clean. https://claude.ai/code/session_01JGkwSJkDKerKRsnzUe8Mxe

…+ rate limit Unit tests mock Mongo queries, the PG pool, and the auth layer — they can't catch regressions that depend on real Mongoose regex semantics, ObjectId coercion, or middleware-ordering interactions with the rate limiter. This integration test exercises the mint endpoint end-to-end against a real MongoMemoryServer (User, Post, Pod, File, AuditLog) with a mocked PG pool for the message fan-out path. Covers the real paths unit tests can't: - Owner short-circuit on File.uploadedBy - Public post reference — any authed viewer mints - Pod-scoped post, viewer not in pod → 403 - Profile picture stored as absolute URL → real substring regex match (regression for fix #4) - PG messages reference file across multiple pods, viewer is in one → mint succeeds (regression for fix #3) - Rate limiter: 31st mint within the window → 429 (regression for fix #1 — auth must run before mintRateLimit so keyGen sees req.userId) - Audit log row lands after successful mint Slots into the existing `INTEGRATION_TEST=true` tier in tests.yml — no new CI infra, picked up by the existing Test workflow on every PR. https://claude.ai/code/session_01JGkwSJkDKerKRsnzUe8Mxe

CodeQL's query only recognises `express-rate-limit` when it's the FIRST middleware on a route. The earlier fix that swapped `auth` before `mintRateLimit` (so the keyGen could read `req.userId`) made CodeQL miss the limiter and fire a High-severity alert on this PR. Resolution: move `mintRateLimit` back to first position and key the limiter on a SHA-256 hash of the `Authorization` header instead of `req.userId`. Same per-user isolation for legitimate bearer-token callers (one bucket per token), no NAT collision, and no dependency on middleware ordering. Unauthenticated requests fall through to `req.ip`-keyed buckets as before. Why hash the header: keeps the raw bearer out of the rate-limiter's internal key space; truncated to 16 hex chars for a compact bucket id. Ephemeral — express-rate-limit's in-memory store lives only as long as the window (60s). Tests updated: integration test comment corrected — it's no longer "keyGen sees userId"; it's "per-token bucket." 29/29 unit tests pass, TS check clean. Integration test continues to drive 30 mints with the same bearer to trigger the 429. https://claude.ai/code/session_01JGkwSJkDKerKRsnzUe8Mxe

Two gaps surfaced landing ADR-002 Phase 1b-a (#216): 1. "Integration" tests don't actually hit real services. CI wires mongo:7 + postgres:16 service containers but setupMongoDb() uses MongoMemoryServer anyway. Inline review had to catch bugs (ACL false-denies, profile-pic URL shape) that a real-DB Tier would have caught in CI. 2. Deploys are cloud-agent-hostile. The documented CLAUDE.md flow requires a local docker daemon, local gcloud auth, and an uncommitted values-private.yaml at a hard-coded laptop path. A Claude session on claude.ai/code can't trigger or observe a real-cluster deployment, so verification falls on whoever pulls the next image tag. ADR-009 proposes: - Four named test tiers (unit / service / cluster / dev-env) with explicit PR gating. The current "integration" tier is renamed to "service" and actually uses the CI service containers; cluster smoke moves from post-merge-only to path-gated on PRs. - Workflow-triggered GKE deploys via Workload Identity Federation. Merge-to-main auto-deploys dev + runs Tier 3 HTTP probes + auto-rollback on probe failure. Tag push gates prod behind a GitHub-environment approval. No SA JSON keys in secrets. - Retires values-private.yaml: non-secret config moves to committed values-dev.yaml / values-prod.yaml; secrets go through ESO (which already owns api-keys). One uncommitted file goes away. Six implementation phases, starting with the Tier 1 rename because it unblocks the rest. Phases 3 and 5 require GCP-side WIF setup outside the repo. Draft — not merging until the team sanity-checks the tier renames and the WIF setup plan. https://claude.ai/code/session_01JGkwSJkDKerKRsnzUe8Mxe

samxu01 force-pushed the claude/adr2-continued-wfVlI branch from b1464c6 to 3f02f67 Compare April 18, 2026 19:07

samxu01 commented Apr 18, 2026

View reviewed changes

github-advanced-security AI found potential problems Apr 19, 2026

View reviewed changes

Comment thread backend/routes/uploads.ts Fixed

samxu01 force-pushed the claude/adr2-continued-wfVlI branch from 810d0b7 to 915bbc7 Compare April 19, 2026 18:00

samxu01 merged commit dd1b310 into main Apr 19, 2026
9 checks passed

samxu01 mentioned this pull request Apr 19, 2026

docs: ADR-009 test tiers + CI/CD to GKE (+ WIF runbook, Path A) #217

Merged

7 tasks

lilyshen0722 mentioned this pull request Apr 21, 2026

ci(gitleaks): allowlist uploads signed-URL integration test #220

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(uploads): ADR-002 Phase 1b-a — signed-URL mint + ACL + audit log - #216

feat(uploads): ADR-002 Phase 1b-a — signed-URL mint + ACL + audit log#216
samxu01 merged 4 commits into
mainfrom
claude/adr2-continued-wfVlI

samxu01 commented Apr 18, 2026

Uh oh!

samxu01 left a comment

Uh oh!

samxu01 Apr 18, 2026

Uh oh!

samxu01 Apr 19, 2026

Uh oh!

samxu01 Apr 18, 2026

Uh oh!

samxu01 Apr 19, 2026

Uh oh!

samxu01 Apr 18, 2026

Uh oh!

samxu01 Apr 19, 2026

Uh oh!

samxu01 Apr 18, 2026

Uh oh!

samxu01 Apr 19, 2026

Uh oh!

samxu01 Apr 18, 2026

Uh oh!

samxu01 Apr 19, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		const urlFragment = `/api/uploads/${fileName}`;

		const profileUser = await User.findOne({ profilePicture: urlFragment }).select('_id').lean();

Uh oh!

Conversation

samxu01 commented Apr 18, 2026

Uh oh!

samxu01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants