Profile photo storage: scope uploads to the owner's folder, add bucket limits, and restrict profile image URLs to our storage (#2406)

.github/workflows/ci-integration.yml

@@ -98,7 +98,7 @@ jobs:
       - name: Verify tenant isolation (real stack)
         env:
           TENANT_GUARD_IT_ADMIN_DATABASE_URL: ${{ env.DATABASE_URL }}
-        run: bun x vitest run tests/unit/acceptance/issue-2250/tenant-guard.realstack.acceptance.test.ts tests/unit/acceptance/issue-2269/donations-embed.realstack.acceptance.test.ts tests/unit/acceptance/issue-2256/person-anchor.realstack.acceptance.test.ts tests/unit/acceptance/issue-2260/donor-feed-preferences.realstack.acceptance.test.ts tests/unit/acceptance/issue-2258/current-user-assignments.realstack.acceptance.test.ts tests/unit/acceptance/issue-2312/public-reads.realstack.acceptance.test.ts tests/unit/acceptance/issue-2402/crm-table-preferences.realstack.acceptance.test.ts tests/unit/acceptance/issue-2364/missionaries-reads.realstack.acceptance.test.ts
+        run: bun x vitest run tests/unit/acceptance/issue-2250/tenant-guard.realstack.acceptance.test.ts tests/unit/acceptance/issue-2269/donations-embed.realstack.acceptance.test.ts tests/unit/acceptance/issue-2256/person-anchor.realstack.acceptance.test.ts tests/unit/acceptance/issue-2260/donor-feed-preferences.realstack.acceptance.test.ts tests/unit/acceptance/issue-2258/current-user-assignments.realstack.acceptance.test.ts tests/unit/acceptance/issue-2312/public-reads.realstack.acceptance.test.ts tests/unit/acceptance/issue-2402/crm-table-preferences.realstack.acceptance.test.ts tests/unit/acceptance/issue-2364/missionaries-reads.realstack.acceptance.test.ts tests/unit/acceptance/issue-2406/profiles-storage.realstack.acceptance.test.ts
       - name: Verify authz EXECUTE grants (real stack)
         env:
           TENANT_GUARD_IT_ADMIN_DATABASE_URL: ${{ env.DATABASE_URL }}

docs/security/identity-person-anchor.md

@@ -97,8 +97,16 @@ today. Per spec In 3, this is escalated in the PR and tracked in #2267; this
 slice changes no behavior. The frozen reader set, pinned by the #2256 acceptance
 tests, is:
 
-- RLS policy: `public.profiles` `profiles staff tenant select`. Staff of the
-  profile's `tenant_id` may read the profile row.
+- RLS policies:
+  - `public.profiles` `profiles staff tenant select`. Staff of the profile's
+    `tenant_id` may read the profile row.
+  - `storage.objects` `profiles owner folder insert` and
+    `profiles owner folder update`, added by #2406. Each binds the object's
+    tenant folder segment to the caller's own `profiles.tenant_id`, looked up
+    by `user_id = auth.uid()`. Each fails closed when the caller has no
+    profile, or has a NULL tenant and isn't a `super_admin`. The `super_admin`
+    demo fallback mirrors `getAuthContext` and is tracked in #2267; if #2267
+    removes or moves it, these two policies change in the same PR.
 - Functions (SECURITY DEFINER unless marked INVOKER). Most bind an actor or
   referenced profile to a tenant by requiring `profiles.tenant_id` to match:
   - `enforce_contribution_operation_tenant_refs`

docs/security/service-role-client-allowlist.json

@@ -51,8 +51,8 @@
     {"file":"packages/api/src/donate/outbox.ts","kind":"admin_client","count":1,"lines":[23],"path_ref":"P2","reason":"No-JWT path P2 from #2250 still uses the service-role client. Frozen by Q4; follow-up #2261.","follow_up":"#2261"},
     {"file":"packages/api/src/donations/index.ts","kind":"admin_client","count":1,"lines":[17],"path_ref":"U staff","reason":"Staff server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2263.","follow_up":"#2263"},
     {"file":"packages/api/src/donor-portal/billing.ts","kind":"operation_admin_context","count":1,"lines":[9],"path_ref":"U member/portal","reason":"withOperation handler uses the supabaseAdmin client it is handed. Frozen by Q4; follow-up #2263.","follow_up":"#2263"},
-    {"file":"packages/api/src/donor-portal/index.ts","kind":"admin_client","count":1,"lines":[46],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
-    {"file":"packages/api/src/donor-portal/index.ts","kind":"operation_admin_context","count":1,"lines":[71],"path_ref":"U member/portal","reason":"withOperation handler uses the supabaseAdmin client it is handed. Frozen by Q4; follow-up #2263.","follow_up":"#2263"},
+    {"file":"packages/api/src/donor-portal/index.ts","kind":"admin_client","count":1,"lines":[47],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
+    {"file":"packages/api/src/donor-portal/index.ts","kind":"operation_admin_context","count":1,"lines":[72],"path_ref":"U member/portal","reason":"withOperation handler uses the supabaseAdmin client it is handed. Frozen by Q4; follow-up #2263.","follow_up":"#2263"},
     {"file":"packages/api/src/donor-portal/route-helpers.ts","kind":"admin_client","count":1,"lines":[24],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
     {"file":"packages/api/src/email/assets.ts","kind":"admin_client","count":1,"lines":[141],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
     {"file":"packages/api/src/email/settings-store.ts","kind":"admin_client","count":1,"lines":[53],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
@@ -84,7 +84,7 @@
     {"file":"packages/api/src/posts/index.ts","kind":"admin_client","count":2,"lines":[28,77],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
     {"file":"packages/api/src/posts/post.ts","kind":"admin_client","count":2,"lines":[45,142],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
     {"file":"packages/api/src/posts/reaction-route-handlers.ts","kind":"admin_client","count":1,"lines":[47],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
-    {"file":"packages/api/src/profile/index.ts","kind":"admin_client","count":2,"lines":[41,83],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
+    {"file":"packages/api/src/profile/index.ts","kind":"admin_client","count":2,"lines":[46,88],"path_ref":"U member/portal","reason":"Member/portal server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2265.","follow_up":"#2265"},
     {"file":"packages/api/src/reads/dashboard-stats.ts","kind":"admin_client","count":2,"lines":[50,151],"path_ref":"U staff","reason":"Staff server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2263.","follow_up":"#2263"},
     {"file":"packages/api/src/reads/donor-history.ts","kind":"admin_client","count":2,"lines":[68,152],"path_ref":"U staff","reason":"Staff server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2263.","follow_up":"#2263"},
     {"file":"packages/api/src/reads/member-care.ts","kind":"admin_client","count":1,"lines":[339],"path_ref":"U staff","reason":"Staff server path uses the service-role client with app-level tenant checks. Frozen by Q4; follow-up #2263.","follow_up":"#2263"},

docs/security/storage-profiles-bucket.md

@@ -0,0 +1,123 @@
+# `profiles` storage bucket contract
+
+The `profiles` bucket holds avatar and cover photos. Migration
+`supabase/migrations/20261027100000_profiles_bucket_owner_folders.sql` (#2406,
+folds in #2417) scopes writes to the owner's folder, limits object reads
+through the API to the owner, and sets size and type limits. Rollback:
+`supabase/migrations/rollback_20261027100000_profiles_bucket_owner_folders.sql`.
+
+## Key layout
+
+```
+{avatars|covers}/{tenantId}/{userId}/{rest}
+```
+
+- `avatars` or `covers` is the purpose.
+- `tenantId` is the caller's `profiles.tenant_id`. A `super_admin` whose
+  `tenant_id` is NULL uses `DEMO_TENANT_ID`
+  (`00000000-0000-0000-0000-000000000001`, `packages/auth/constants.ts`); a
+  `super_admin` with a tenant uses that tenant and never the demo one. This
+  mirrors `getAuthContext`.
+- `userId` is `auth.uid()` (`ctx.userId`).
+- `rest` is non-empty. The upload route (`POST /api/upload/image`, #2308)
+  writes `{uuid}.{ext}` with `upsert: false`.
+
+## Policies on `storage.objects`
+
+Exactly four policies cover the bucket. All are `TO authenticated`; `anon`
+has none.
+
+| Policy                         | Command | Rule                                                                                                 |
+| ------------------------------ | ------- | ---------------------------------------------------------------------------------------------------- |
+| `profiles owner folder insert` | INSERT  | WITH CHECK: the name is in the caller's own folder (layout above)                                    |
+| `profiles owner folder update` | UPDATE  | USING: `owner = auth.uid()`. WITH CHECK: still the owner, and the name is in the caller's own folder |
+| `profiles owner select`        | SELECT  | USING: `owner = auth.uid()`                                                                          |
+| `Owner delete profiles`        | DELETE  | USING: `owner = auth.uid()` (unchanged since init_schema)                                            |
+
+- The folder check splits the name with `string_to_array(name, '/')`, not
+  `storage.foldername()`, and wraps `auth.uid()` as `(SELECT auth.uid())`.
+- Uploading a new object needs INSERT only. Storage API `remove` and `move`
+  need SELECT on the owner's own rows, which `profiles owner select` gives.
+- An owned object outside the layout (a legacy path) can be deleted or moved
+  into the owner's folder, but not updated in place.
+- The bucket stays `public`, so public object URLs are served without any
+  SELECT policy. Listing, signed URLs and downloads go through RLS.
+- Direct browser uploads (the dormant `useBackendProcessing={false}` branch
+  of `ImageUpload`) must use this layout and `upsert: false`.
+- The two folder policies read `profiles.tenant_id`, so they are in the
+  sanctioned reader set in `docs/security/identity-person-anchor.md` (#2267).
+
+## Bucket limits
+
+| Column               | Value                                                |
+| -------------------- | ---------------------------------------------------- |
+| `file_size_limit`    | `5242880` (5 MB, the upload route's cap)             |
+| `allowed_mime_types` | `image/jpeg`, `image/png`, `image/webp`, `image/gif` |
+
+Storage enforces these on upload. If the route's cap changes, change the
+bucket limit in the same PR.
+
+### Recorded prior limits
+
+Before changing the limits, the migration copies the bucket's previous
+`file_size_limit` and `allowed_mime_types` into
+`private.storage_bucket_limits_20261027100000` (`bucket_id`,
+`file_size_limit`, `allowed_mime_types`, `recorded_at`). The first record is
+kept if the migration runs again. `ALL` is revoked from `PUBLIC`, `anon`,
+`authenticated` and `service_role`, and nothing is granted, so no API role
+can read it. The rollback restores these values and drops the table.
+
+## Profile image URL rule
+
+`PATCH /api/profile` (`avatarUrl`, `coverUrl`) and `PATCH /api/donor/portal`
+(`avatarUrl`) accept a value only when it is:
+
+- `null`, which clears the field;
+- equal to the currently stored value, which is accepted and not rewritten;
+- exactly `{origin}/storage/v1/object/public/profiles/{purpose}/{tenantId}/{userId}/{rest}`,
+  where `origin` is the origin of `NEXT_PUBLIC_SUPABASE_URL`, `purpose` is
+  `avatars` for `avatarUrl` and `covers` for `coverUrl`, `tenantId` is
+  `ctx.tenantId` and `userId` is `ctx.userId`.
+
+Comparisons are exact strings. The URL must be canonical, with no query,
+fragment, credentials, `.` or `..` segments (raw or percent-encoded), encoded
+`/` or empty segments. Anything else returns 400 `{ error }` and writes
+nothing. With `NEXT_PUBLIC_SUPABASE_URL` unset, a new non-null URL returns 503. The validator is `packages/api/src/profile/image-url.ts`; it runs before
+any write.
+
+## Pre-deploy listing
+
+Before the migration is applied to a hosted environment, the factory runs a
+read-only listing of `profiles` objects (`name`, `metadata->>'mimetype'`,
+`(metadata->>'size')::bigint`) that are outside the layout
+(`name !~ '^(avatars|covers)/[^/]+/[^/]+/.+'`), not one of the four types,
+or over 5242880 bytes, plus the bucket's current limits and policy list. The
+result goes in the factory handoff note, not in the repository. Nothing is
+moved or deleted; any cleanup is a separate, owner-approved step.
+
+## Deploy
+
+- The migration depends only on `20250101000000_init_schema.sql` and creates
+  the `private` schema if it is missing.
+- Either order works with the upload route: its keys pass the new INSERT
+  policy, and its upload needs no SELECT.
+- `NEXT_PUBLIC_SUPABASE_URL` in each app must equal the URL storage public
+  links are built from.
+- After deploy, for missionary avatar, missionary cover and donor avatar:
+  upload a photo (201); open its public URL signed out (200); reload the
+  profile; save a non-storage URL and another user's folder URL through the
+  API (both 400); `list()` the bucket signed out and another user's folder
+  signed in (both empty); upload an over-limit file and a non-image type (both
+  rejected by storage).
+
+## Rollback
+
+The rollback file drops the three new policies, recreates
+`Public read profiles`, `Authenticated upload profiles` and
+`Owner update profiles` exactly as init_schema defines them, sets both limit
+columns back to the recorded values, and drops the record table, all in one
+transaction. If the recorded row is missing it raises and changes nothing; it
+never sets the limits to NULL by default. Because it restores the earlier
+upload and read policies, apply it only together with reverting the app
+commit. If the recorded values differ from the factory's pre-deploy listing,
+restore the listed values.

packages/api/src/donor-portal/index.ts

@@ -10,6 +10,7 @@ import { NextResponse } from "next/server";
 import { getDemoDonorPortalSnapshot } from "./demo-snapshot";
 import { getDonorPortalSnapshot, resolveDonorPortalContext } from "./service";
 import { donorPortalPatchSchema } from "./settings-patch";
+import { checkProfileImageUrl } from "../profile/image-url";
 import { ensureJsonBody, toErrorResponse } from "../shared/http-errors";
 import { withOperation } from "../shared/with-operation";
 
@@ -71,11 +72,19 @@ export const PATCH = withOperation(
   async ({ supabaseAdmin, auth, request }) => {
     const ctx = auth as AuthenticatedContext;
     const patch = donorPortalPatchSchema.parse(await ensureJsonBody(request));
-    const { donor } = await resolveDonorPortalContext(
+    const { profile, donor } = await resolveDonorPortalContext(
       supabaseAdmin,
       ctx.profileId,
       ctx.tenantId,
     );
+    // #2406: validated before any write. Each column takes the accepted value
+    // unless it already holds it (OQ2), so a resent value still backfills.
+    checkProfileImageUrl(patch.avatarUrl, {
+      purpose: "avatars",
+      tenantId: ctx.tenantId,
+      userId: ctx.userId,
+      stored: [profile.avatar_url, donor.avatar_url],
+    });
 
     const now = new Date().toISOString();
     const profileUpdate: Record<string, unknown> = {};
@@ -96,8 +105,13 @@ export const PATCH = withOperation(
       donorUpdate.phone = patch.phone || null;
     }
     if (patch.avatarUrl !== undefined) {
-      profileUpdate.avatar_url = patch.avatarUrl || null;
-      donorUpdate.avatar_url = patch.avatarUrl || null;
+      const avatarUrl = patch.avatarUrl || null;
+      if (profile.avatar_url !== avatarUrl) {
+        profileUpdate.avatar_url = avatarUrl;
+      }
+      if (donor.avatar_url !== avatarUrl) {
+        donorUpdate.avatar_url = avatarUrl;
+      }
     }
     if (patch.preferredContact !== undefined) {
       donorUpdate.preferred_contact = patch.preferredContact;

packages/api/src/profile/image-url.ts

@@ -0,0 +1,110 @@
+import { getSupabasePublicConfig } from "@asym/database/supabase/config";
+
+import { ApiHttpError } from "../shared/api-http-error";
+
+export type ProfileImagePurpose = "avatars" | "covers";
+
+const PUBLIC_BUCKET_PATH = "/storage/v1/object/public/profiles/";
+
+/**
+ * Origin of the configured `NEXT_PUBLIC_SUPABASE_URL` (the base storage public
+ * URLs are built from), or null when it is unset or not an http(s) URL.
+ */
+export function getProfileImageOrigin(): string | null {
+  const configured = getSupabasePublicConfig().url;
+  if (!configured) return null;
+  try {
+    const { origin, protocol } = new URL(configured);
+    return protocol === "https:" || protocol === "http:" ? origin : null;
+  } catch {
+    return null;
+  }
+}
+
+function isNormalizedSegment(segment: string): boolean {
+  if (!segment || /%(2f|5c)/i.test(segment)) return false;
+  try {
+    const decoded = decodeURIComponent(segment);
+    return decoded !== "." && decoded !== "..";
+  } catch {
+    return false;
+  }
+}
+
+/**
+ * True only for an exact public object URL of this project's `profiles`
+ * bucket inside the caller's own folder:
+ * `{origin}/storage/v1/object/public/profiles/{purpose}/{tenantId}/{userId}/{rest}`.
+ * All comparisons are exact strings. The URL must already be in canonical
+ * form (no query, fragment, credentials, default port, dot segments or
+ * encoded slashes), so what is stored is exactly what storage serves.
+ */
+export function isOwnProfileImageUrl(
+  url: string,
+  expected: {
+    purpose: ProfileImagePurpose;
+    tenantId: string;
+    userId: string;
+    origin: string;
+  },
+): boolean {
+  if (!url || /[?#\\]/.test(url)) return false;
+  let parsed: URL;
+  try {
+    parsed = new URL(url);
+  } catch {
+    return false;
+  }
+  if (
+    parsed.href !== url ||
+    parsed.origin !== expected.origin ||
+    parsed.username ||
+    parsed.password ||
+    !parsed.pathname.startsWith(PUBLIC_BUCKET_PATH)
+  ) {
+    return false;
+  }
+  const [purpose, tenantId, userId, ...rest] = parsed.pathname
+    .slice(PUBLIC_BUCKET_PATH.length)
+    .split("/");
+  return (
+    Boolean(expected.tenantId && expected.userId) &&
+    purpose === expected.purpose &&
+    tenantId === expected.tenantId &&
+    userId === expected.userId &&
+    rest.length > 0 &&
+    rest.every(isNormalizedSegment)
+  );
+}
+
+/**
+ * Applies the #2406 rule to one image URL field before any write. Returns
+ * whether the value must be written: `undefined` (absent) and a value equal
+ * to a stored value (OQ2) are not written; `null` clears the field. A new
+ * value must be an own-folder public URL of the `profiles` bucket (400), and
+ * cannot be checked without the public Supabase URL (503).
+ */
+export function checkProfileImageUrl(
+  value: string | null | undefined,
+  input: {
+    purpose: ProfileImagePurpose;
+    tenantId: string;
+    userId: string;
+    stored: readonly (string | null | undefined)[];
+  },
+): boolean {
+  if (value === undefined) return false;
+  if (value === null) return true;
+  if (input.stored.includes(value)) return false;
+  const origin = getProfileImageOrigin();
+  if (!origin) {
+    throw new ApiHttpError(503, "Profile image storage is not configured");
+  }
+  if (!isOwnProfileImageUrl(value, { ...input, origin })) {
+    throw new ApiHttpError(
+      400,
+      `Image URL must be an uploaded ${input.purpose === "avatars" ? "avatar" : "cover"} in your own profile folder`,
+    );
+  }
+  return true;
+}