feat(api): restrict avatarUrl to GCS public-image bucket in shared router

[pstaylor-patrick]

Summary

Stacked behind #163 per @taterhead247's review feedback. Extracts the shared-router half of sweep round 2's R6 (security) so it can be discussed and merged independently of the Me app deployment.

Base branch is feat/me (the head of #163). The diff this PR shows is exactly the R6 host-allow-list code that used to live in #163's sweep-2 commit.

Context

The current schema in packages/api/src/router/me/index.ts accepts any well-formed URL for avatarUrl. An authenticated user can PATCH the field to an attacker-controlled host, and other apps that consume the avatar value through this shared router (without their own host filter) become a tracking-pixel / SSRF surface when they pre-fetch the URL server-side.

This PR adds a regex refine restricting avatarUrl to storage.googleapis.com/f3-public-images[-staging]/* at the shared router, plus positive and negative tests.

Trade-off the team should weigh before merging

F3 currently has user avatar URLs and region logos pointing at a variety of hosts -- Slack, region WordPress sites, etc. Enforcing a strict host allow-list at the shared router will break write paths for non-GCS-hosted images on every app that shares this router (not just the Me app).

@taterhead247 raised this concern on #163. Mitigations the team can pick from:

1. Backfill non-conforming avatar URLs into the GCS public-image bucket before merging.
2. Loosen the allow-list to also accept the legacy hosts in use today (Slack, configured region WP domains).
3. Drop this PR and keep the restriction only in apps that upload (the Me app already has its own host check at apps/me/src/app/api/profile/route.ts).

What's NOT in this PR (lives in #163)

• The Me app's own host check (apps/me/src/app/api/profile/route.ts) -- @taterhead247 explicitly endorsed keeping that.
• R7 (atomic JSONB merge for users.meta) from sweep round 2 -- that's a race-condition fix unrelated to host policy.

Test plan

• Team alignment on which mitigation strategy applies (or whether to drop the shared-router restriction entirely)
• If keeping: confirm no production avatar URLs in users.avatarUrl would be rejected by the new pattern
• If keeping: confirm region logos (if they go through this router) won't be impacted

Verification

• pnpm lint, pnpm typecheck, pnpm format:check all pass
• New tests added: positive case (GCS URL accepted), negative cases (attacker host rejected, wrong-bucket rejected)

Related

• Depends on Add F3 Me profile manager app #163 (base branch is feat/me); will rebase onto dev if/when Add F3 Me profile manager app #163 lands or is unstacked.
• Original review comment: Add F3 Me profile manager app #163 (comment)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Avatar image URLs are now restricted to approved cloud storage locations; unsupported domains or buckets will be rejected.
  • Submissions using disallowed avatar URLs now produce a clear validation error so users know to update the image link.

[taterhead247]

Team is interested in doing this. But would require every image to be migrated into GCP. Option 2 for allow list could ease us in.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: afb85418-3c95-4ec7-bc12-ca03bf7091f9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The /me router now restricts avatar URL updates to Google Cloud Storage URLs from the f3-public-images bucket (with optional -staging suffix). A validation helper and Zod schema refinement enforce this pattern, with tests verifying both valid GCS public-image URLs and rejection of other domains or buckets.

Changes

Avatar URL Restriction

Layer / File(s)	Summary
Avatar URL validation contract and schema enforcement packages/api/src/router/me/index.ts	Adds ALLOWED_AVATAR_HOST_PATTERN and isAllowedAvatarUrl helper to validate avatarUrl against GCS public-image bucket pattern, then wires it into profileUpdateSchema via Zod refine() with updated description.
Avatar URL validation tests packages/api/src/router/me/me.test.ts	Updates valid avatarUrl test to use GCS public-image URL with user ID, and adds new test to verify rejection of non-allowed domains and other GCS buckets.

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• F3-Nation/f3-nation#287: Also modifies avatarUrl allowlists/validation at the API boundary; closely related to host/bucket allowlist logic.

Suggested reviewers

• dnishiyama
• evanpetzoldt

🐰 A bucket so fine, the public images align,
I nibble on bytes and tidy the line,
Only GCS paths may now take the stage,
Avatars march in from the canonical page,
Hooray for checked URLs — hop, validate, and shine! 📸✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat(api): restrict avatarUrl to GCS public-image bucket in shared router' accurately and specifically describes the main change: adding a validation constraint to restrict avatar URLs to a GCS bucket.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/me-shared-avatar-host-allowlist

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/api/src/router/me/me.test.ts (1)

307-336: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Add a positive f3-public-images-staging test.

The contract now allows both prod and staging buckets, but this coverage only proves prod passes and non-allowed URLs fail. A regression in the optional -staging branch would slip through unnoticed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/api/src/router/me/me.test.ts` around lines 307 - 336, Add a positive
test case mirroring "should update avatarUrl with a valid GCS public-image URL"
but using the staging bucket hostname/path: construct a URL like
`https://storage.googleapis.com/f3-public-images-staging/user-avatars/${testUserId}.jpg`,
call createDirectClient() and client.me.updateProfile({ avatarUrl: url }),
assert result.user.avatarUrl equals the staging URL, and perform the same
cleanup by resetting avatarUrl to null; place this alongside the existing tests
so both prod and staging buckets are covered.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/api/src/router/me/index.ts`:
- Around line 63-72: The avatarUrl zod refine currently rejects non-GCS URLs via
isAllowedAvatarUrl which will break PATCH for users with legacy avatar hosts;
update the refine to only enforce isAllowedAvatarUrl when the account has passed
the legacy-avatar migration (or when a global migration flag is enabled),
otherwise allow legacy URLs that are on a temporary allow-list; implement a
small helper (e.g., isLegacyAvatarAllowed or use an environment flag
LEGACY_AVATAR_ALLOWLIST/ENABLE_LEGACY_AVATARS) and change the refine on
avatarUrl to: accept null/empty, accept GCS via isAllowedAvatarUrl, or accept
legacy-host URLs when isLegacyAvatarAllowed(session.user.id or the URL) returns
true so existing profiles aren’t rejected until migrated.

---

Outside diff comments:
In `@packages/api/src/router/me/me.test.ts`:
- Around line 307-336: Add a positive test case mirroring "should update
avatarUrl with a valid GCS public-image URL" but using the staging bucket
hostname/path: construct a URL like
`https://storage.googleapis.com/f3-public-images-staging/user-avatars/${testUserId}.jpg`,
call createDirectClient() and client.me.updateProfile({ avatarUrl: url }),
assert result.user.avatarUrl equals the staging URL, and perform the same
cleanup by resetting avatarUrl to null; place this alongside the existing tests
so both prod and staging buckets are covered.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4ff641e2-a6dc-4e75-addf-a62aaca1878f

📥 Commits

Reviewing files that changed from the base of the PR and between 6317aa5 and bb6e3d9.

📒 Files selected for processing (2)

• packages/api/src/router/me/index.ts
• packages/api/src/router/me/me.test.ts

[coderabbitai]

♻️ Duplicate comments (1)

packages/api/src/router/me/index.ts (1)

63-67: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

This refine can block PATCH for users still carrying legacy avatar hosts.

If a client posts an unchanged legacy avatarUrl along with unrelated profile edits, this now fails validation and rejects the full update. Consider gating strict enforcement behind migration completion or a temporary legacy allow-list.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/api/src/router/me/index.ts` around lines 63 - 67, The avatarUrl zod
refine using isAllowedAvatarUrl rejects legacy-host URLs and thus breaks PATCH
when clients resubmit an unchanged legacy avatar; modify the validation to
permit legacy URLs until migration finishes by either (1) adding a temporary
legacy allow-list check (e.g., legacyAllowedAvatarHosts) into the refine or (2)
gating the strict isAllowedAvatarUrl enforcement behind a migration flag (e.g.,
migrationComplete or allowLegacyAvatars) so that avatarUrl validation only
requires isAllowedAvatarUrl when migrationComplete is true; alternatively, if
you have access to the current user record in the update flow, skip strict
validation when the incoming avatarUrl equals the stored avatarUrl to allow
unchanged legacy values to pass.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@packages/api/src/router/me/index.ts`:
- Around line 63-67: The avatarUrl zod refine using isAllowedAvatarUrl rejects
legacy-host URLs and thus breaks PATCH when clients resubmit an unchanged legacy
avatar; modify the validation to permit legacy URLs until migration finishes by
either (1) adding a temporary legacy allow-list check (e.g.,
legacyAllowedAvatarHosts) into the refine or (2) gating the strict
isAllowedAvatarUrl enforcement behind a migration flag (e.g., migrationComplete
or allowLegacyAvatars) so that avatarUrl validation only requires
isAllowedAvatarUrl when migrationComplete is true; alternatively, if you have
access to the current user record in the update flow, skip strict validation
when the incoming avatarUrl equals the stored avatarUrl to allow unchanged
legacy values to pass.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 11dd4482-b33c-48fb-af04-4f7745c44edb

📥 Commits

Reviewing files that changed from the base of the PR and between bb6e3d9 and bf433bf.

📒 Files selected for processing (2)

• packages/api/src/router/me/index.ts
• packages/api/src/router/me/me.test.ts

[pstaylor-patrick]

🟡 Parked as draft — avatar-migration strategy + script added

Picking up @taterhead247's concern ("would require every image to be migrated") and CodeRabbit's "gate behind a legacy-avatar migration." They're right: the new validation is write-path only, so the risk isn't reads — it's that a user whose stored avatar_url is a legacy/external URL would hit a 400 the moment they edit any profile field (the client round-trips the current avatar). The fix is to migrate existing avatars into the bucket before enabling strict validation.

Yes, this scripts out cleanly — and it's a data backfill, not a Drizzle schema migration (no DDL), so it lives with the other one-off packages/db/src/* scripts and dodges the migration-strip rebase footgun entirely.

What I added (commit e994a5c)

• @acme/shared/app/avatar — single source of truth for the allowlist (ALLOWED_AVATAR_HOST_PATTERN + isAllowedAvatarUrl). The me router now imports it instead of an inline copy, so the API rule and the migration can never drift.
• packages/db/src/backfill-avatar-urls.ts — for every user whose avatar_url doesn't match the allowlist: fetch the image (SSRF-guarded: http(s) only, blocks private/loopback hosts; 15s timeout; 15 MB cap) → re-encode to the same 512×512 JPEG the upload route produces → upload to the canonical user-avatars/{userId}.jpg → rewrite avatar_url. Reuses @acme/storage (resizeImage + uploadFile) so it's byte-for-byte consistent with live uploads.
• Scripts: `pnpm db:backfill:avatars` (dry-run default) · `-- --execute` to apply · `--null-failed` to clear dead URLs · `--limit N` for a canary batch.

Why it's still draft — decisions for you

1. Dead/unfetchable URLs — default leaves them + reports; `--null-failed` nulls them (→ default avatar). Pick the policy before flipping strict validation on.
2. Org logos — orgs.logo_url likely has the same legacy-host situation (there's a parallel upload-logo route). In scope here, or a follow-up?
3. Run order — backfill (prod dry-run → execute) must land before the validation is enforced, else affected users get blocked mid-flight.

Gates locally: typecheck + lint green (db/api/shared). Left as draft pending your call on the above.

[pstaylor-patrick]

🤖 Automated readiness pass

• Still a draft — left intentionally undrafted; not marked ready-for-review and not merged.
• Rebased onto the latest origin/dev (1207db8). Resolved a package.json conflict (kept both the typecheck --no-daemon and the new db:backfill:avatars script) and regenerated pnpm-lock.yaml for the new @acme/db → @acme/storage workspace dep.
• Fixed a rebase-induced break: @acme/storage renamed resizeImage → prepareImageForStorage; updated the backfill script's import/call (signature is identical). Typecheck now passes.
• CI green: build, typecheck, lint, test-coverage, format-check all passing on the rebased head.
• Threads clear: addressed the CodeRabbit legacy-avatar concern (handled by design via the companion db:backfill:avatars migration + shared allowlist); thread resolved.
• Code-review sweep: clean, 0 criticals / 0 warnings.

Head: c6f9104

[taterhead247]

@pstaylor-patrick , let's talk about this next time you're at a Tuesday standup. I still think we need to lock down URLs in our database. I'm not sure this completely solves that. If helps secure Me. But people can still add malicious urls using the standard api users call. And we can't just add the allow list to the users.crupdate, because developers would only be allowed to set the url to that of our bucket, but most (all) of them don't have access to upload images to the bucket.