diff --git a/.github/workflows/ci-integration.yml b/.github/workflows/ci-integration.yml index f220eba96..2e4c18970 100644 --- a/.github/workflows/ci-integration.yml +++ b/.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 }} diff --git a/docs/security/identity-person-anchor.md b/docs/security/identity-person-anchor.md index 17cb65725..51e75ed40 100644 --- a/docs/security/identity-person-anchor.md +++ b/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` diff --git a/docs/security/service-role-client-allowlist.json b/docs/security/service-role-client-allowlist.json index efeeff67e..957be9cf4 100644 --- a/docs/security/service-role-client-allowlist.json +++ b/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"}, diff --git a/docs/security/storage-profiles-bucket.md b/docs/security/storage-profiles-bucket.md new file mode 100644 index 000000000..45b858426 --- /dev/null +++ b/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. diff --git a/packages/api/src/donor-portal/index.ts b/packages/api/src/donor-portal/index.ts index a65aa1543..a4408e3e6 100644 --- a/packages/api/src/donor-portal/index.ts +++ b/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 = {}; @@ -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; diff --git a/packages/api/src/profile/image-url.ts b/packages/api/src/profile/image-url.ts new file mode 100644 index 000000000..b2a97a7b1 --- /dev/null +++ b/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; +} diff --git a/packages/api/src/profile/index.ts b/packages/api/src/profile/index.ts index 769da1816..729572202 100644 --- a/packages/api/src/profile/index.ts +++ b/packages/api/src/profile/index.ts @@ -8,8 +8,13 @@ import { getAdminClient } from "@asym/database/supabase/admin"; import { type NextRequest, NextResponse } from "next/server"; import { z } from "zod"; +import { checkProfileImageUrl } from "./image-url"; import { findFullProfileById } from "./queries"; -import { ensureJsonBody, toErrorResponse } from "../shared/http-errors"; +import { + ApiHttpError, + ensureJsonBody, + toErrorResponse, +} from "../shared/http-errors"; import { findMissionaryByProfileId } from "../shared/queries"; const profilePatchSchema = z @@ -89,6 +94,38 @@ export async function PATCH(request: NextRequest) { requireAuth(auth); const ctx = auth as AuthenticatedContext; const patch = profilePatchSchema.parse(await ensureJsonBody(request)); + const [storedAvatar, storedCover] = await Promise.all([ + typeof patch.avatarUrl === "string" + ? supabaseAdmin + .from("profiles") + .select("avatar_url") + .eq("id", ctx.profileId) + .eq("tenant_id", ctx.tenantId) + .maybeSingle() + : null, + typeof patch.coverUrl === "string" + ? supabaseAdmin + .from("missionaries") + .select("cover_url") + .eq("profile_id", ctx.profileId) + .eq("tenant_id", ctx.tenantId) + .maybeSingle() + : null, + ]); + const lookupError = storedAvatar?.error ?? storedCover?.error; + if (lookupError) throw new ApiHttpError(500, lookupError.message); + // #2406: validated before any write; unchanged stored values are skipped. + const owner = { tenantId: ctx.tenantId, userId: ctx.userId }; + const writeAvatar = checkProfileImageUrl(patch.avatarUrl, { + ...owner, + purpose: "avatars", + stored: [storedAvatar?.data?.avatar_url], + }); + const writeCover = checkProfileImageUrl(patch.coverUrl, { + ...owner, + purpose: "covers", + stored: [storedCover?.data?.cover_url], + }); const now = new Date().toISOString(); const profileUpdate: Record = {}; const missionaryUpdate: Record = {}; @@ -106,7 +143,7 @@ export async function PATCH(request: NextRequest) { profileUpdate.phone = patch.phone || null; missionaryUpdate.phone = patch.phone || null; } - if (patch.avatarUrl !== undefined) { + if (writeAvatar) { profileUpdate.avatar_url = patch.avatarUrl || null; } @@ -125,7 +162,7 @@ export async function PATCH(request: NextRequest) { if (patch.location !== undefined) { missionaryUpdate.location = patch.location || null; } - if (patch.coverUrl !== undefined) { + if (writeCover) { missionaryUpdate.cover_url = patch.coverUrl || null; } if (patch.socialLinks !== undefined) { diff --git a/scripts/sql/supabase-compat-bootstrap.sql b/scripts/sql/supabase-compat-bootstrap.sql index aaf3ca156..7d5f70bc4 100644 --- a/scripts/sql/supabase-compat-bootstrap.sql +++ b/scripts/sql/supabase-compat-bootstrap.sql @@ -135,10 +135,16 @@ CREATE TABLE IF NOT EXISTS storage.buckets ( id TEXT PRIMARY KEY, name TEXT NOT NULL, public BOOLEAN DEFAULT false, + file_size_limit BIGINT, + allowed_mime_types TEXT[], created_at TIMESTAMPTZ DEFAULT NOW(), updated_at TIMESTAMPTZ DEFAULT NOW() ); +ALTER TABLE storage.buckets + ADD COLUMN IF NOT EXISTS file_size_limit BIGINT, + ADD COLUMN IF NOT EXISTS allowed_mime_types TEXT[]; + CREATE TABLE IF NOT EXISTS storage.objects ( id UUID PRIMARY KEY DEFAULT extensions.gen_random_uuid(), bucket_id TEXT NOT NULL REFERENCES storage.buckets(id), diff --git a/supabase/migrations/20261027100000_profiles_bucket_owner_folders.sql b/supabase/migrations/20261027100000_profiles_bucket_owner_folders.sql new file mode 100644 index 000000000..c152e6ceb --- /dev/null +++ b/supabase/migrations/20261027100000_profiles_bucket_owner_folders.sql @@ -0,0 +1,103 @@ +-- #2406 (folds in #2417): owner-scoped writes, owner-only object reads and +-- size/type limits for the `profiles` storage bucket. +-- +-- Rollback: supabase/migrations/rollback_20261027100000_profiles_bucket_owner_folders.sql +-- (it restores the pre-migration INSERT, UPDATE and SELECT policies, so pair +-- it with reverting the app commit). Contract and deploy notes: +-- docs/security/storage-profiles-bucket.md. +-- +-- Object keys follow {avatars|covers}/{tenantId}/{userId}/{rest} (the #2308 +-- upload route). An authenticated caller may insert only under its own +-- folder: purpose avatars or covers, tenant = the caller's profiles.tenant_id +-- (the demo tenant only for a super_admin whose tenant_id is NULL, mirroring +-- getAuthContext), user = auth.uid(), and a non-empty remainder. Owners may +-- update their own objects, but the updated row must satisfy the same folder +-- rule, so a legacy object can be moved into the folder but not rewritten in +-- place. "Public read profiles" gives way to an owner-only SELECT policy; the +-- bucket stays public, so public object URLs keep working without it. +-- "Owner delete profiles" is unchanged. +-- +-- The folder check uses string_to_array(name, '/') rather than +-- storage.foldername(), so it runs on hosted storage and on CI's compat +-- schema alike. +-- +-- The bucket's prior file_size_limit and allowed_mime_types are recorded in a +-- private table before they change (first record wins on re-apply) so the +-- rollback can restore them. No API role can reach that table. This +-- migration adds no grants. + +BEGIN; + +CREATE SCHEMA IF NOT EXISTS private; + +CREATE TABLE IF NOT EXISTS private.storage_bucket_limits_20261027100000 ( + bucket_id TEXT PRIMARY KEY, + file_size_limit BIGINT, + allowed_mime_types TEXT[], + recorded_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +REVOKE ALL ON TABLE private.storage_bucket_limits_20261027100000 FROM PUBLIC, anon, authenticated, service_role; + +INSERT INTO private.storage_bucket_limits_20261027100000 (bucket_id, file_size_limit, allowed_mime_types) +SELECT id, file_size_limit, allowed_mime_types +FROM storage.buckets +WHERE id = 'profiles' +ON CONFLICT (bucket_id) DO NOTHING; + +-- 5 MB (the upload route's cap) and the route's four image types. +UPDATE storage.buckets +SET file_size_limit = 5242880, + allowed_mime_types = ARRAY['image/jpeg', 'image/png', 'image/webp', 'image/gif'] +WHERE id = 'profiles'; + +DROP POLICY IF EXISTS "Authenticated upload profiles" ON storage.objects; +DROP POLICY IF EXISTS "Owner update profiles" ON storage.objects; +DROP POLICY IF EXISTS "Public read profiles" ON storage.objects; +DROP POLICY IF EXISTS "profiles owner folder insert" ON storage.objects; +DROP POLICY IF EXISTS "profiles owner folder update" ON storage.objects; +DROP POLICY IF EXISTS "profiles owner select" ON storage.objects; + +-- '00000000-0000-0000-0000-000000000001' is DEMO_TENANT_ID +-- (packages/auth/constants.ts). +CREATE POLICY "profiles owner folder insert" + ON storage.objects FOR INSERT TO authenticated + WITH CHECK ( + bucket_id = 'profiles' + AND (string_to_array(name, '/'))[1] IN ('avatars', 'covers') + AND (string_to_array(name, '/'))[2] IN ( + SELECT COALESCE( + p.tenant_id::text, + CASE WHEN p.role = 'super_admin' THEN '00000000-0000-0000-0000-000000000001' END + ) + FROM public.profiles p + WHERE p.user_id = (SELECT auth.uid()) + ) + AND (string_to_array(name, '/'))[3] = (SELECT auth.uid())::text + AND (string_to_array(name, '/'))[4] <> '' + ); + +CREATE POLICY "profiles owner folder update" + ON storage.objects FOR UPDATE TO authenticated + USING (bucket_id = 'profiles' AND owner = (SELECT auth.uid())) + WITH CHECK ( + bucket_id = 'profiles' + AND owner = (SELECT auth.uid()) + AND (string_to_array(name, '/'))[1] IN ('avatars', 'covers') + AND (string_to_array(name, '/'))[2] IN ( + SELECT COALESCE( + p.tenant_id::text, + CASE WHEN p.role = 'super_admin' THEN '00000000-0000-0000-0000-000000000001' END + ) + FROM public.profiles p + WHERE p.user_id = (SELECT auth.uid()) + ) + AND (string_to_array(name, '/'))[3] = (SELECT auth.uid())::text + AND (string_to_array(name, '/'))[4] <> '' + ); + +CREATE POLICY "profiles owner select" + ON storage.objects FOR SELECT TO authenticated + USING (bucket_id = 'profiles' AND owner = (SELECT auth.uid())); + +COMMIT; diff --git a/supabase/migrations/rollback_20261027100000_profiles_bucket_owner_folders.sql b/supabase/migrations/rollback_20261027100000_profiles_bucket_owner_folders.sql new file mode 100644 index 000000000..7775634de --- /dev/null +++ b/supabase/migrations/rollback_20261027100000_profiles_bucket_owner_folders.sql @@ -0,0 +1,52 @@ +-- Rollback for 20261027100000_profiles_bucket_owner_folders.sql (#2406). +-- Restores the pre-migration `profiles` storage policies (the three +-- originals exactly as 20250101000000_init_schema.sql defines them), sets the +-- bucket's file_size_limit and allowed_mime_types back to the values the +-- forward migration recorded, and drops the record table. +-- +-- The restored policies are the pre-migration upload and read rules, so ship +-- this only together with reverting the app commit. If the recorded row is +-- missing it raises and changes nothing; it never falls back to NULL limits. +-- One transaction, so a failure under plain `psql -f` leaves no partial state. + +BEGIN; + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 + FROM private.storage_bucket_limits_20261027100000 + WHERE bucket_id = 'profiles' + ) THEN + RAISE EXCEPTION 'rollback_20261027100000: no recorded limits for bucket profiles; nothing was changed'; + END IF; +END +$$; + +UPDATE storage.buckets AS b +SET file_size_limit = r.file_size_limit, + allowed_mime_types = r.allowed_mime_types +FROM private.storage_bucket_limits_20261027100000 AS r +WHERE b.id = 'profiles' + AND r.bucket_id = 'profiles'; + +DROP POLICY "profiles owner folder insert" ON storage.objects; +DROP POLICY "profiles owner folder update" ON storage.objects; +DROP POLICY "profiles owner select" ON storage.objects; + +CREATE POLICY "Public read profiles" + ON storage.objects FOR SELECT + USING (bucket_id = 'profiles'); + +CREATE POLICY "Authenticated upload profiles" + ON storage.objects FOR INSERT TO authenticated + WITH CHECK (bucket_id = 'profiles' AND auth.role() = 'authenticated'); + +CREATE POLICY "Owner update profiles" + ON storage.objects FOR UPDATE TO authenticated + USING (bucket_id = 'profiles' AND auth.uid() = owner) + WITH CHECK (bucket_id = 'profiles' AND auth.uid() = owner); + +DROP TABLE private.storage_bucket_limits_20261027100000; + +COMMIT; diff --git a/tests/unit/acceptance/issue-2155/postgres-ecr-gates.acceptance.test.ts b/tests/unit/acceptance/issue-2155/postgres-ecr-gates.acceptance.test.ts index 76cebbacb..bb7bd326c 100644 --- a/tests/unit/acceptance/issue-2155/postgres-ecr-gates.acceptance.test.ts +++ b/tests/unit/acceptance/issue-2155/postgres-ecr-gates.acceptance.test.ts @@ -310,6 +310,8 @@ describe("#2155 criterion 9: nothing else in ci-integration.yml changes", () => { ref: "#2312 Builder e518a845", hash: "52e534b2e20a3ae2" }, { ref: "#2402 Builder 28ac3c13", hash: "999bf6013d1b18ca" }, { ref: "#2364 Builder 0cf86552", hash: "4ac6a6bcd2006149" }, + // #2406: Builder 9563beea (develop 629cb71d merged) appends the issue-2406 real-stack path to tenant isolation + { ref: "#2406 Builder 9563beea", hash: "6f061d845914c44d" }, ] as const; it("triggers, permissions, concurrency, runs-on, if:, needs, services, steps match a reviewed ci-integration.yml", () => { expect( diff --git a/tests/unit/acceptance/issue-2201/merge-group.acceptance.test.ts b/tests/unit/acceptance/issue-2201/merge-group.acceptance.test.ts index 2c4d4af40..26063246e 100644 --- a/tests/unit/acceptance/issue-2201/merge-group.acceptance.test.ts +++ b/tests/unit/acceptance/issue-2201/merge-group.acceptance.test.ts @@ -201,6 +201,8 @@ describe("#2201 criteria 9-12: threat-model invariants pinned at b17ce79e", () = { ref: "#2312 Builder e518a845", hash: "3e026b8f31f34dda" }, { ref: "#2402 Builder 28ac3c13", hash: "7755cfddb8cb875c" }, { ref: "#2364 Builder 0cf86552", hash: "21073c70380a4f90" }, + // #2406: Builder 9563beea (develop 629cb71d merged) appends the issue-2406 real-stack path to tenant isolation + { ref: "#2406 Builder 9563beea", hash: "925cfd8de36c75a3" }, ] as const; it("nothing but on: and test-e2e's if: changes (fingerprint)", () => { const prints = [CI, CII].map((file) => { diff --git a/tests/unit/acceptance/issue-2250/static.acceptance.test.ts b/tests/unit/acceptance/issue-2250/static.acceptance.test.ts index d5ad688fa..fd5ba9d2b 100644 --- a/tests/unit/acceptance/issue-2250/static.acceptance.test.ts +++ b/tests/unit/acceptance/issue-2250/static.acceptance.test.ts @@ -40,6 +40,7 @@ const STEP_RUN = [ "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", ].join(" "); const STEP = "Verify tenant isolation (real stack)"; const REALSTACK = diff --git a/tests/unit/acceptance/issue-2256/harness.ts b/tests/unit/acceptance/issue-2256/harness.ts index 25a90d4c9..cd1bdbf23 100644 --- a/tests/unit/acceptance/issue-2256/harness.ts +++ b/tests/unit/acceptance/issue-2256/harness.ts @@ -71,8 +71,13 @@ export const READER_FUNCTIONS: Record = { expire_eve_retention_records: true, handle_new_user: true, }; -/** §1.3: the one RLS policy reader, "schema.table policy". */ -export const READER_POLICIES = ["public.profiles profiles staff tenant select"]; +/** §1.3 RLS policy readers, "schema.table policy", sorted. #2406 addendum 1 + * adds the two storage.objects folder policies (they read profiles.tenant_id). */ +export const READER_POLICIES = [ + "public.profiles profiles staff tenant select", + "storage.objects profiles owner folder insert", + "storage.objects profiles owner folder update", +]; /** §1.3 app rows (files). */ export const READER_APPS = [ "apps/admin/src/cms/auth/supabase-strategy.ts", diff --git a/tests/unit/acceptance/issue-2256/migration-text.acceptance.test.ts b/tests/unit/acceptance/issue-2256/migration-text.acceptance.test.ts index e6cf44ee6..ab2b0cf07 100644 --- a/tests/unit/acceptance/issue-2256/migration-text.acceptance.test.ts +++ b/tests/unit/acceptance/issue-2256/migration-text.acceptance.test.ts @@ -21,6 +21,7 @@ import { POLICY, READER_APPS, READER_FUNCTIONS, + READER_POLICIES, ROLLBACK, SEAMS, V2_MIGRATION, @@ -332,7 +333,8 @@ describe("#2256 census artifacts and docs", () => { ...CONSTRAINTS, ...Object.keys(READER_FUNCTIONS), ...READER_APPS, - "profiles staff tenant select", + // Every policy reader by name, incl. #2406 addendum 1's storage policies. + ...READER_POLICIES.map((p) => p.replace(/^\S+ /, "")), ]) expect(doc, n).toContain(n); const rollback = path.basename(ROLLBACK).replace(/\./g, "\\."); diff --git a/tests/unit/acceptance/issue-2308/static.acceptance.test.ts b/tests/unit/acceptance/issue-2308/static.acceptance.test.ts index 479840c0a..48b79a9e5 100644 --- a/tests/unit/acceptance/issue-2308/static.acceptance.test.ts +++ b/tests/unit/acceptance/issue-2308/static.acceptance.test.ts @@ -71,12 +71,13 @@ describe("#2308 static contracts (A)", () => { expect(result.error).toBeUndefined(); expect(result.status, `${result.stdout}${result.stderr}`).toBe(0); }); - // Re-frozen after develop d99a1fc4 shrank the table-preferences entry. - it("C12: frozen service-role allowlist is byte-unchanged from develop d99a1fc4", () => { + // Re-frozen after develop d99a1fc4 shrank the table-preferences entry, and + // again at #2406 Builder 9563beea (only `lines` fields moved; no new sites). + it("C12: frozen service-role allowlist is byte-unchanged from #2406 Builder 9563beea", () => { expect( createHash("sha256") .update(read("docs/security/service-role-client-allowlist.json")) .digest("hex"), - ).toBe("9c9875a789cb33d8b1b80d35428773c3cab8599b0ef4805e4e65d0241e060233"); + ).toBe("41b0324c81960fc9349c92b5048293ff7b856ce8d51d5c14ce2c22b4fb575d0c"); }); }); diff --git a/tests/unit/acceptance/issue-2406/harness.ts b/tests/unit/acceptance/issue-2406/harness.ts new file mode 100644 index 000000000..bead73eb3 --- /dev/null +++ b/tests/unit/acceptance/issue-2406/harness.ts @@ -0,0 +1,357 @@ +/** #2406 spec r2 (comment 6449278094, approved 6449292287): shared constants and + * the real-stack harness for criteria 2-10. Isolation follows #2402/#2258: one + * throwaway database per Stack (unique name per file and worker, from + * template0), never DDL on the CI database, lock/statement timeouts so a lock + * wait fails fast (the 40P01 lesson from #2256/#2260), DROP ... WITH (FORCE). + */ +import { spawnSync } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import { existsSync, readFileSync, readdirSync } from "node:fs"; + +import pg from "pg"; +import { expect } from "vitest"; + +export const MIG = "supabase/migrations"; +export const VERSION = "20261027100000"; +export const NAME = `${VERSION}_profiles_bucket_owner_folders.sql`; +export const FORWARD = `${MIG}/${NAME}`; +export const ROLLBACK = `${MIG}/rollback_${NAME}`; +export const INIT = `${MIG}/20250101000000_init_schema.sql`; +export const BOOTSTRAP = "scripts/sql/supabase-compat-bootstrap.sql"; +export const BACKUP = `private.storage_bucket_limits_${VERSION}`; +export const DOC = "docs/security/storage-profiles-bucket.md"; +export const DIR = "tests/unit/acceptance/issue-2406"; +export const REALSTACK = `${DIR}/profiles-storage.realstack.acceptance.test.ts`; +/** packages/auth/constants.ts DEMO_TENANT_ID (asserted equal in the static suite). */ +export const DEMO = "00000000-0000-0000-0000-000000000001"; +export const LIMIT = 5242880; +export const TYPES = ["image/gif", "image/jpeg", "image/png", "image/webp"]; +/** Stand-in for recorded hosted limits (criterion 10): non-NULL, unlike §1.4. */ +export const PRIOR = { size: "1048576", types: ["image/png"] }; +export const NEW_POLICIES = [ + "profiles owner folder insert", + "profiles owner folder update", + "profiles owner select", +]; +export const OLD_POLICIES = [ + "Authenticated upload profiles", + "Owner update profiles", + "Public read profiles", +]; +export const DELETE_POLICY = "Owner delete profiles"; + +export const read = (p: string) => + existsSync(p) ? readFileSync(p, "utf8") : ""; +/** SQL text without comments, so commented-out statements never count. */ +export const code = (s: string) => s.replace(/--[^\n]*|\/\*[\s\S]*?\*\//g, " "); +export const squash = (s: string) => s.replace(/\s+/g, " ").trim(); +export const migrated = () => + expect( + existsSync(FORWARD), + `${FORWARD} missing: #2406 not implemented on this tree`, + ).toBe(true); + +type Row = Record; +export type Result = { code: string | null; rows: Row[]; count: number }; +export type Role = "anon" | "authenticated"; +export type Snapshot = { policies: Row[]; buckets: Row[] }; + +/** Actors: tenants T/T2; U donor and M missionary in T; X in T2; S super + * admin without tenant; S2 super admin in T; N has auth.users but no profile; + * Z (addendum 1) donor whose profiles.tenant_id is NULL. */ +const ACTORS = "T T2 U M X S S2 N Z".split(" "); + +export class Stack { + readonly database = `acc2406_${randomUUID().replaceAll("-", "")}`; + readonly ids: Record = Object.fromEntries( + ACTORS.map((k) => [k, randomUUID()]), + ); + readonly profiles: Record = Object.fromEntries( + ACTORS.map((k) => [k, randomUUID()]), + ); + readonly admin: pg.Client; + readonly db: pg.Client; + readonly url: URL; + pre: Snapshot = { policies: [], buckets: [] }; + priorApplied = false; + created = false; + connected = false; + closed = false; + /** hosted=false: storage table grants only, enough for RLS to decide. + * hosted=true: criterion 8's hosted grants and private default privileges. */ + constructor( + url: string, + readonly hosted: boolean, + ) { + this.admin = new pg.Client({ connectionString: url }); + this.url = new URL(url); + this.url.pathname = `/${this.database}`; + this.db = new pg.Client({ connectionString: this.url.toString() }); + } + async rows(sql: string, params: unknown[] = []) { + return (await this.db.query(sql, params)).rows as Row[]; + } + /** psql with the DB URI as its first argument (never PGDATABASE). */ + psql(file: string) { + const r = spawnSync( + process.env.PSQL_BIN?.trim() || "psql", + [ + this.url.toString(), + "-X", + "-q", + "-v", + "ON_ERROR_STOP=1", + ...(file.endsWith("/20260214090000_foundation_1_schema.sql") + ? ["--single-transaction"] + : []), + "-f", + file, + ], + { encoding: "utf8", timeout: 60_000 }, + ); + // Never echo stderr: connection failures may contain credentials. + return { status: r.status, error: r.error?.name ?? null }; + } + async open() { + await this.admin.connect(); + try { + const adminDb = (await this.admin.query("select current_database() as d")) + .rows[0]!.d as string; + expect(adminDb, "throwaway name must differ from the CI DB").not.toBe( + this.database, + ); + await this.admin.query( + `CREATE DATABASE ${this.database} TEMPLATE template0`, + ); + this.created = true; + const files = [ + BOOTSTRAP, + ...readdirSync(MIG) + .filter((f) => /^\d{14}_.+\.sql$/.test(f)) + .sort() + .map((f) => `${MIG}/${f}`), + "supabase/seed.sql", + ]; + for (const file of files) { + if (file === FORWARD) await this.beforeForward(); + const r = this.psql(file); + expect( + r.status, + `migration/bootstrap failed: ${file} (${r.error ?? "psql"})`, + ).toBe(0); + } + if (!this.connected) await this.beforeForward(); + } catch (error) { + await this.close(); + throw error; + } + } + /** Grants, prior-limit fixture and the pre-migration catalog snapshot. */ + async beforeForward() { + await this.db.connect(); + this.connected = true; + await this.db.query( + "set lock_timeout = '10s'; set statement_timeout = '60s'", + ); + const [self] = await this.rows("select current_database() as d"); + expect(self!.d).toBe(this.database); + await this.db + .query(`GRANT USAGE ON SCHEMA storage TO anon, authenticated, service_role; + GRANT SELECT, INSERT, UPDATE, DELETE ON storage.objects TO anon, authenticated;`); + if (this.hosted) + await this.db + .query(`GRANT ALL ON storage.objects, storage.buckets TO anon, authenticated, service_role; + ALTER DEFAULT PRIVILEGES IN SCHEMA private GRANT ALL ON TABLES TO anon, authenticated;`); + const [cols] = await this.rows( + `select count(*)::int as n from information_schema.columns where table_schema='storage' + and table_name='buckets' and column_name in ('file_size_limit','allowed_mime_types')`, + ); + if (cols!.n === 2) { + await this.db.query( + "update storage.buckets set file_size_limit=$1, allowed_mime_types=$2 where id='profiles'", + [PRIOR.size, PRIOR.types], + ); + this.priorApplied = true; + } + this.pre = { + policies: await this.policies(), + buckets: await this.buckets(), + }; + } + async close() { + if (this.closed) return; + this.closed = true; + await (this.connected ? this.db.end() : Promise.resolve()).finally( + async () => { + try { + if (this.created) + await this.admin.query( + `DROP DATABASE ${this.database} WITH (FORCE)`, + ); + } finally { + await this.admin.end(); + } + }, + ); + } + policies() { + return this + .rows(`select policyname, permissive, roles::text[] as roles, cmd, qual, with_check + from pg_policies where schemaname='storage' and tablename='objects' order by policyname`); + } + buckets() { + return this.rows( + "select id, to_jsonb(b) as row from storage.buckets b order by id", + ); + } + async limits() { + const [r] = await this + .rows(`select file_size_limit::text as size, allowed_mime_types as types + from storage.buckets where id='profiles'`); + return { + size: (r?.size ?? null) as string | null, + types: r?.types ? [...(r.types as string[])].sort() : null, + }; + } + async regclass(name: string) { + return (await this.rows("select to_regclass($1)::text as r", [name]))[0]! + .r as string | null; + } + async privileges(table: string) { + return this.rows( + `select r as role, p as privilege from unnest(array['anon','authenticated','service_role']) r, + unnest(array['SELECT','INSERT','UPDATE','DELETE']) p where has_table_privilege(r::name, $1::text, p)`, + [table], + ); + } + /** Object name built from actor keys: path("avatars", "T", "U") + "/x.webp". */ + path(purpose: string, tenant: string, user: string) { + return `${purpose}/${this.ids[tenant] ?? tenant}/${this.ids[user] ?? user}`; + } + /** Per-test fixture: tenants, users, profiles and seeded profiles objects. */ + async fixture() { + const id = this.ids, + p = this.profiles; + const users = ACTORS.filter((k) => !k.startsWith("T")).flatMap((k) => [ + id[k], + p[k], + ]); + await this.db.query( + `insert into public.tenants(id,name,slug) + values ($1::uuid,'acc2406 T',$1::text),($2::uuid,'acc2406 T2',$2::text)`, + [id.T, id.T2], + ); + for (const sql of [ + "insert into auth.users(id) select unnest($1::uuid[])", + "delete from public.profiles where id = any($1::uuid[]) or user_id = any($1::uuid[])", + "delete from authz.memberships where user_id = any($1::uuid[])", + ]) + await this.db.query(sql, [users]); + const roles: [string, string | null, string][] = [ + ["U", id.T!, "donor"], + ["M", id.T!, "missionary"], + ["X", id.T2!, "donor"], + ["S", null, "super_admin"], + ["S2", id.T!, "super_admin"], + ["Z", null, "donor"], + ]; + for (const [actor, tenant, role] of roles) + await this.db.query( + "insert into public.profiles(id,user_id,tenant_id,role) values($1,$2,$3,$4)", + [p[actor], id[actor], tenant, role], + ); + await this.db.query( + `insert into storage.objects(bucket_id,name,owner) values + ('profiles',$1,$5),('profiles',$2,$5),('profiles',$3,$6),('profiles',$4,$6),('profiles','legacy/orphan.webp',null), + ('profiles',$7,$8)`, + [ + `${this.path("avatars", "T", "U")}/seed.webp`, + `avatars/${id.U}-123.webp`, + `${this.path("avatars", "T2", "X")}/seed.webp`, + `avatars/${id.X}-123.webp`, + id.U, + id.X, + `avatars/${id.Z}-123.webp`, + id.Z, + ], + ); + } + async tx(fn: () => Promise) { + await this.db.query("begin"); + try { + await this.fixture(); + await fn(); + } finally { + await this.db.query("rollback"); + } + } + /** Runs one statement as `role` with JWT sub = actor id (or raw sub/none). */ + async as( + role: Role, + actor: string | null, + sql: string, + params: unknown[] = [], + ): Promise { + await this.db.query("savepoint actor"); + try { + const sub = actor ? (this.ids[actor] ?? actor) : ""; + await this.db.query(`set local role ${role}`); + await this.db.query( + `select set_config('request.jwt.claim.sub',$1,true), set_config('request.jwt.claim.role',$2,true), + set_config('request.jwt.claims',$3,true)`, + [sub, role, JSON.stringify({ sub: sub || null, role })], + ); + const r = await this.db.query(sql, params); + await this.db.query("reset role; release savepoint actor"); + return { code: null, rows: r.rows as Row[], count: r.rowCount ?? 0 }; + } catch (error) { + await this.db.query( + "rollback to savepoint actor; release savepoint actor", + ); + return { + code: (error as { code?: string }).code ?? "error", + rows: [], + count: 0, + }; + } + } + async objects() { + const [r] = await this.rows( + "select count(*)::int as n from storage.objects where bucket_id='profiles'", + ); + return r!.n as number; + } + insert(role: Role, actor: string | null, name: string, returning = false) { + const owner = actor ? (this.ids[actor] ?? actor) : null; + return this.as( + role, + actor, + `insert into storage.objects(bucket_id,name,owner) values ('profiles',$1,$2)${returning ? " returning id" : ""}`, + [name, owner], + ); + } + update(actor: string, name: string, set: { name?: string }) { + return this.as( + "authenticated", + actor, + `update storage.objects set ${set.name ? "name=$2" : "metadata=jsonb_build_object('v',$2::text)"} + where bucket_id='profiles' and name=$1 returning name`, + [name, set.name ?? "1"], + ); + } + remove(actor: string, name: string) { + return this.as( + "authenticated", + actor, + "delete from storage.objects where bucket_id='profiles' and name=$1", + [name], + ); + } + list(role: Role, actor: string | null) { + return this.as( + role, + actor, + "select name, owner::text as owner from storage.objects where bucket_id='profiles' order by name", + ); + } +} diff --git a/tests/unit/acceptance/issue-2406/profile-url.acceptance.test.ts b/tests/unit/acceptance/issue-2406/profile-url.acceptance.test.ts new file mode 100644 index 000000000..941e5c021 --- /dev/null +++ b/tests/unit/acceptance/issue-2406/profile-url.acceptance.test.ts @@ -0,0 +1,612 @@ +/** #2406 spec r2 criteria 11-18 against the real PATCH /api/profile and + * PATCH /api/donor/portal handlers (real zod, auth role helpers, error mapping). + * Only getAuthContext, the admin client (an in-memory table recorder) and the + * audit logger are faked. Modules reload per test so a handler that reads + * NEXT_PUBLIC_SUPABASE_URL at import time sees each test's value (OQ9). + * Mutants: M7 "prefix" hosts, M8 "profiles-archive"/".." cases, M9 criterion + * 14 (writes before validating), M11 "another user's folder", M12 "another + * tenant's folder", M13 purpose swaps, M17 "super_admin with tenant T". + */ +import { existsSync } from "node:fs"; + +import { createClient } from "@supabase/supabase-js"; +import { NextRequest } from "next/server"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { DEMO } from "./harness"; + +import type * as AuthContext from "@asym/auth/context"; + +type Row = Record; +type Write = { table: string; op: string; values: unknown }; +const m = vi.hoisted(() => ({ + auth: {} as Row, + tables: {} as Record, + writes: [] as Write[], +})); + +/** Supabase-shaped query builder over m.tables; every write is recorded. */ +function fakeAdmin() { + return { + from(table: string) { + const filters: ((r: Row) => boolean)[] = []; + let write: Omit | null = null; + let mode: "many" | "single" | "maybe" = "many"; + const hit = () => + (m.tables[table] ?? []).filter((r) => filters.every((f) => f(r))); + const run = () => { + if (write) { + m.writes.push({ table, ...write }); + const rows = write.op === "update" ? hit() : []; + for (const r of rows) Object.assign(r, write.values); + return { data: rows.map((r) => ({ ...r })), error: null }; + } + const rows = hit().map((r) => ({ ...r })); + if (mode === "many") return { data: rows, error: null }; + if (rows.length === 1 || (mode === "maybe" && rows.length === 0)) + return { data: rows[0] ?? null, error: null }; + return { data: null, error: { code: "PGRST116", message: "no row" } }; + }; + const q: Row = {}; + const chain = + (fn: (...a: never[]) => unknown = () => undefined) => + (...a: never[]) => { + fn(...a); + return q; + }; + const where = (test: (a: unknown, b: unknown) => boolean) => + chain(((k: string, v: unknown) => + filters.push((r) => test(r[k] ?? null, v))) as never); + const op = (name: string) => + chain(((values: unknown) => (write = { op: name, values })) as never); + for (const name of "select order limit range gte gt lt lte not or filter".split( + " ", + )) + q[name] = chain(); + Object.assign(q, { + eq: where((a, b) => a === b), + neq: where((a, b) => a !== b), + is: where((a, b) => a === b), + in: where((a, b) => (b as unknown[]).includes(a)), + match: chain(((o: Row) => + Object.entries(o).forEach(([k, v]) => + filters.push((r) => r[k] === v), + )) as never), + update: op("update"), + insert: op("insert"), + upsert: op("upsert"), + delete: op("delete"), + single: chain(() => (mode = "single")), + maybeSingle: chain(() => (mode = "maybe")), + then: (ok: (v: unknown) => unknown, fail: (e: unknown) => unknown) => + Promise.resolve().then(run).then(ok, fail), + }); + return q; + }, + rpc: async () => ({ data: null, error: { message: "rpc not faked" } }), + }; +} + +vi.mock("@asym/database/supabase/admin", () => ({ + getAdminClient: () => ({ client: fakeAdmin(), error: null }), +})); +vi.mock("@asym/auth/context", async (importOriginal) => ({ + ...(await importOriginal()), + getAuthContext: async () => m.auth, +})); +vi.mock("@asym/lib/audit/logger", () => ({ + createAuditLogger: () => ({ log: async () => undefined }), +})); + +const ORIGIN = "https://acc2406.supabase.co"; +const P = `${ORIGIN}/storage/v1/object/public/profiles`; +// Fixed ids with hex letters so the case-change cases really differ. +const T = "aaaaaaaa-0000-4000-8000-00000000000a"; +const T2 = "bbbbbbbb-0000-4000-8000-00000000000b"; +const X = "eeeeeeee-0000-4000-8000-00000000000e"; +const A = { + M: { + userId: "cccccccc-0000-4000-8000-00000000000c", + tenantId: T, + role: "missionary", + }, + D: { + userId: "dddddddd-0000-4000-8000-00000000000d", + tenantId: T, + role: "donor", + }, + SA: { + userId: "fafafafa-0000-4000-8000-0000000000fa", + tenantId: DEMO, + role: "super_admin", + }, + SAT: { + userId: "fbfbfbfb-0000-4000-8000-0000000000fb", + tenantId: T, + role: "super_admin", + }, +} as const; +type Actor = keyof typeof A; +const PROFILE = Object.fromEntries( + Object.keys(A).map((k, i) => [k, `10000000-0000-4000-8000-00000000000${i}`]), +) as Record; +const F = "0b0e6c3e-7d1a-4b55-9b52-6a3f1d2c4e5f.webp"; +const UM = A.M.userId; +const UD = A.D.userId; +const own = (purpose: string, actor: Actor = "M", file = F) => + `${P}/${purpose}/${A[actor].tenantId}/${A[actor].userId}/${file}`; +const STORED = { + external: + "https://images.unsplash.com/photo-1500648767791-00dcc994a43e?w=400", + cover: `${P}/covers/${UM}-1.webp`, + donor: `${P}/avatars/${UD}-123.webp`, +}; + +function seed() { + m.writes = []; + m.tables = { + profiles: (Object.keys(A) as Actor[]).map((k) => ({ + id: PROFILE[k], + user_id: A[k].userId, + // The route reads profiles by ctx.tenantId (DEMO for SA), so rows use it too. + tenant_id: A[k].tenantId, + role: A[k].role, + email: `${k.toLowerCase()}@example.test`, + first_name: "First", + last_name: "Last", + full_name: "First Last", + display_name: "First Last", + phone: null, + avatar_url: k === "M" ? STORED.external : k === "D" ? STORED.donor : null, + })), + missionaries: [ + { + id: "20000000-0000-4000-8000-000000000001", + profile_id: PROFILE.M, + tenant_id: T, + cover_url: STORED.cover, + bio: null, + }, + ], + donors: [ + { + id: "30000000-0000-4000-8000-000000000001", + profile_id: PROFILE.D, + tenant_id: T, + name: "First Last", + email: "d@example.test", + avatar_url: STORED.donor, + }, + ], + }; +} +const row = (table: string, profileKey: string, id: string) => + m.tables[table]!.find((r) => r[profileKey] === id)!; +const wrote = (column: string) => + m.writes.some( + (w) => + typeof w.values === "object" && w.values !== null && column in w.values, + ); + +async function send(route: "profile" | "donor", actor: Actor, body: Row) { + m.auth = { + isAuthenticated: true, + email: null, + memberships: [], + userId: A[actor].userId, + tenantId: A[actor].tenantId, + profileId: PROFILE[actor], + role: A[actor].role, + profileRole: A[actor].role, + }; + const mod = + route === "profile" + ? await import("../../../../packages/api/src/profile") + : await import("../../../../packages/api/src/donor-portal"); + const path = route === "profile" ? "/api/profile" : "/api/donor/portal"; + const res = await mod.PATCH( + new NextRequest(`https://app.example.test${path}`, { + method: "PATCH", + body: JSON.stringify(body), + headers: { "content-type": "application/json" }, + }), + ); + return { status: res.status, body: (await res.json()) as Row }; +} + +beforeEach(() => { + vi.resetModules(); + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", ORIGIN); + seed(); +}); +afterEach(() => { + vi.unstubAllEnvs(); +}); + +async function accepted(route: "profile" | "donor", actor: Actor, body: Row) { + const r = await send(route, actor, body); + expect(r.status, JSON.stringify(r.body)).toBe(200); +} +async function rejected( + route: "profile" | "donor", + actor: Actor, + body: Row, + status = 400, +) { + const before = structuredClone(m.tables); + const r = await send(route, actor, body); + expect(r.status, JSON.stringify(r.body)).toBe(status); + expect(typeof r.body.error).toBe("string"); + expect(m.writes).toEqual([]); + expect(m.tables).toEqual(before); +} + +describe("#2406 PATCH /api/profile", () => { + it("#2406 criterion 11: own avatars/T/U URL returns 200 and is stored in profiles.avatar_url", async () => { + await accepted("profile", "M", { avatarUrl: own("avatars") }); + expect(row("profiles", "id", PROFILE.M).avatar_url).toBe(own("avatars")); + }); + it("#2406 criterion 11: missionary's own covers/T/U URL is stored in missionaries.cover_url", async () => { + const url = own("covers", "M", F.replace("webp", "jpg")); + await accepted("profile", "M", { coverUrl: url }); + expect(row("missionaries", "profile_id", PROFILE.M).cover_url).toBe(url); + }); + // Follow-up: 'Profile API: super_admin with no tenant can't save PATCH /api/profile' (addendum 1: criterion 11 third bullet is validator-only). + it("#2406 criterion 11: super_admin without a tenant may use avatars//U/", async () => { + await accepted("profile", "SA", { avatarUrl: own("avatars", "SA") }); + expect(row("profiles", "id", PROFILE.SA).avatar_url).toBe( + own("avatars", "SA"), + ); + }); + it("#2406 criterion 11 (addendum 1, validator-only): isOwnProfileImageUrl accepts avatars//U/ for a tenant-less super_admin resolved to DEMO", async () => { + const { isOwnProfileImageUrl } = + await import("../../../../packages/api/src/profile/image-url"); + const expected = { + purpose: "avatars", + tenantId: DEMO, + userId: A.SA.userId, + origin: ORIGIN, + } as const; + expect(isOwnProfileImageUrl(own("avatars", "SA"), expected)).toBe(true); + expect( + isOwnProfileImageUrl(own("avatars", "SA"), { ...expected, tenantId: T }), + ).toBe(false); + }); + it("#2406 criterion 11: supabase-js getPublicUrl for #2308-layout keys is accepted (avatar and cover)", async () => { + const bucket = createClient(ORIGIN, "anon-key").storage.from("profiles"); + const avatar = bucket.getPublicUrl(`avatars/${T}/${UM}/${F}`).data + .publicUrl; + const cover = bucket.getPublicUrl(`covers/${T}/${UM}/${F}`).data.publicUrl; + await accepted("profile", "M", { avatarUrl: avatar, coverUrl: cover }); + expect(row("profiles", "id", PROFILE.M).avatar_url).toBe(avatar); + expect(row("missionaries", "profile_id", PROFILE.M).cover_url).toBe(cover); + }); + it("#2406 criterion 11: the exact URL PR #2415's upload route returns is accepted", async (t) => { + const route = "packages/api/src/upload/image.ts"; + if (!existsSync(route)) + t.skip( + "PR #2415 (#2308 POST /api/upload/image) is not on this base; runs once it is", + ); + const { buildProfileImageKey } = (await import( + /* @vite-ignore */ `../../../../${route}` + )) as { buildProfileImageKey: (i: Row) => string }; + const key = buildProfileImageKey({ + purpose: "avatars", + tenantId: T, + userId: UM, + type: "image/webp", + }); + // The route returns storage.getPublicUrl(path).data.publicUrl. + const url = createClient(ORIGIN, "anon-key") + .storage.from("profiles") + .getPublicUrl(key).data.publicUrl; + await accepted("profile", "M", { avatarUrl: url }); + expect(row("profiles", "id", PROFILE.M).avatar_url).toBe(url); + }); + it("#2406 criterion 12: avatarUrl null and coverUrl null (missionary) return 200 and store NULL", async () => { + await accepted("profile", "M", { avatarUrl: null, coverUrl: null }); + expect(row("profiles", "id", PROFILE.M).avatar_url).toBeNull(); + expect(row("missionaries", "profile_id", PROFILE.M).cover_url).toBeNull(); + }); + + const mine = (purpose = "avatars") => `${purpose}/${T}/${UM}`; + const host = (h: string) => + `${h}/storage/v1/object/public/profiles/${mine()}/${F}`; + const BAD: [string, Actor, "avatarUrl" | "coverUrl", string][] = [ + [ + "another user's folder (M11)", + "M", + "avatarUrl", + `${P}/avatars/${T}/${X}/${F}`, + ], + [ + "another tenant's folder (M12)", + "M", + "avatarUrl", + `${P}/avatars/${T2}/${UM}/${F}`, + ], + [ + "super_admin with tenant T into the demo folder (M17)", + "SAT", + "avatarUrl", + own("avatars", "SA").replace(A.SA.userId, A.SAT.userId), + ], + [ + "avatarUrl with covers/ (M13)", + "M", + "avatarUrl", + `${P}/${mine("covers")}/${F}`, + ], + ["coverUrl with avatars/ (M13)", "M", "coverUrl", `${P}/${mine()}/${F}`], + [ + "unknown purpose documents/", + "M", + "avatarUrl", + `${P}/${mine("documents")}/${F}`, + ], + [ + "legacy object avatars/U-123.webp", + "M", + "avatarUrl", + `${P}/avatars/${UM}-123.webp`, + ], + ["legacy object at the bucket root", "M", "avatarUrl", `${P}/${UM}.webp`], + ["folder with no file", "M", "avatarUrl", `${P}/${mine()}/`], + [ + "upper-cased tenant segment", + "M", + "avatarUrl", + `${P}/avatars/${T.toUpperCase()}/${UM}/${F}`, + ], + [ + "upper-cased uid segment", + "M", + "avatarUrl", + `${P}/avatars/${T}/${UM.toUpperCase()}/${F}`, + ], + ["another origin", "M", "avatarUrl", host("https://evil.example.com")], + [ + "host sharing a string prefix (M7)", + "M", + "avatarUrl", + host(`${ORIGIN}.evil.example`), + ], + [ + "hostname extended by a prefix match (M7)", + "M", + "avatarUrl", + host(`${ORIGIN}evil.example`), + ], + [ + "http when configured https", + "M", + "avatarUrl", + host(ORIGIN.replace("https", "http")), + ], + ["a different port", "M", "avatarUrl", host(`${ORIGIN}:8443`)], + [ + "credentials in the URL", + "M", + "avatarUrl", + host(ORIGIN.replace("//", "//user:pass@")), + ], + [ + "username in the URL", + "M", + "avatarUrl", + host(ORIGIN.replace("//", "//user@")), + ], + [ + "another bucket", + "M", + "avatarUrl", + `${ORIGIN}/storage/v1/object/public/documents/${mine()}/${F}`, + ], + [ + "bucket name starting with profiles (M8)", + "M", + "avatarUrl", + `${ORIGIN}/storage/v1/object/public/profiles-archive/${mine()}/${F}`, + ], + [ + "/object/sign/ endpoint", + "M", + "avatarUrl", + `${ORIGIN}/storage/v1/object/sign/profiles/${mine()}/${F}`, + ], + [ + "/object/authenticated/ endpoint", + "M", + "avatarUrl", + `${ORIGIN}/storage/v1/object/authenticated/profiles/${mine()}/${F}`, + ], + [ + "/render/image/public/ endpoint", + "M", + "avatarUrl", + `${ORIGIN}/storage/v1/render/image/public/profiles/${mine()}/${F}`, + ], + ["a query string", "M", "avatarUrl", `${P}/${mine()}/${F}?width=64`], + ["a fragment", "M", "avatarUrl", `${P}/${mine()}/${F}#top`], + ["raw . segment", "M", "avatarUrl", `${P}/${mine()}/./${F}`], + [ + "raw .. segment resolving into own folder (M8)", + "M", + "avatarUrl", + `${P}/${mine()}/../${UM}/${F}`, + ], + [ + "raw .. segment from another user's folder", + "M", + "avatarUrl", + `${P}/avatars/${T}/${X}/../${UM}/${F}`, + ], + [ + "percent-encoded .. segment", + "M", + "avatarUrl", + `${P}/${mine()}/%2e%2e/${UM}/${F}`, + ], + ["percent-encoded . segment", "M", "avatarUrl", `${P}/${mine()}/%2E/${F}`], + ["encoded / in the file name", "M", "avatarUrl", `${P}/${mine()}/a%2F${F}`], + [ + "encoded / joining folder segments", + "M", + "avatarUrl", + `${P}/avatars%2F${T}/${UM}/${F}`, + ], + ["empty object path", "M", "avatarUrl", `${P}/`], + ["no object path", "M", "avatarUrl", P], + ["ftp: scheme", "M", "avatarUrl", host(ORIGIN.replace("https", "ftp"))], + ["javascript: scheme", "M", "avatarUrl", "javascript:alert(1)"], + ["empty string", "M", "avatarUrl", ""], + [ + "coverUrl: another user's folder", + "M", + "coverUrl", + `${P}/covers/${T}/${X}/${F}`, + ], + [ + "coverUrl: another origin", + "M", + "coverUrl", + `https://evil.example.com/storage/v1/object/public/profiles/${mine("covers")}/${F}`, + ], + ]; + it.each(BAD)( + "#2406 criterion 13: %s returns 400 { error }", + (_l, actor, field, value) => rejected("profile", actor, { [field]: value }), + ); + it.each(BAD)( + "#2406 criterion 14: %s alongside valid other fields changes no row and makes no write (M9)", + (_l, actor, field, value) => + rejected("profile", actor, { + firstName: "Changed", + phone: "555-0100", + bio: "New bio", + [field]: value, + }), + ); + + it("#2406 criterion 15: resending the stored external avatar and legacy cover unchanged returns 200 without rewriting them (OQ2)", async () => { + await accepted("profile", "M", { + firstName: "Changed", + avatarUrl: STORED.external, + coverUrl: STORED.cover, + }); + expect(row("profiles", "id", PROFILE.M).first_name).toBe("Changed"); + expect(row("profiles", "id", PROFILE.M).avatar_url).toBe(STORED.external); + expect(row("missionaries", "profile_id", PROFILE.M).cover_url).toBe( + STORED.cover, + ); + expect(wrote("avatar_url"), "avatar_url rewritten").toBe(false); + expect(wrote("cover_url"), "cover_url rewritten").toBe(false); + }); + it("#2406 criterion 15: resending a stored legacy-object avatar in this bucket unchanged returns 200 without rewrite", async () => { + await accepted("profile", "D", { avatarUrl: STORED.donor }); + expect(row("profiles", "id", PROFILE.D).avatar_url).toBe(STORED.donor); + expect(wrote("avatar_url")).toBe(false); + }); + it.each([ + ["a different external URL", "https://images.unsplash.com/photo-2?w=400"], + ["a different legacy-object URL", `${P}/avatars/${UM}-999.webp`], + ["another user's folder URL", `${P}/avatars/${T}/${X}/${F}`], + ])("#2406 criterion 15: changing to %s returns 400", (_l, value) => + rejected("profile", "M", { avatarUrl: value }), + ); + + it("#2406 criterion 16: NEXT_PUBLIC_SUPABASE_URL unset: a changed URL returns 503 { error } with no write (OQ9)", async () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", undefined); + await rejected( + "profile", + "M", + { firstName: "Changed", avatarUrl: own("avatars") }, + 503, + ); + }); + it("#2406 criterion 16: NEXT_PUBLIC_SUPABASE_URL unset: null still returns 200", async () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", undefined); + await accepted("profile", "M", { avatarUrl: null }); + expect(row("profiles", "id", PROFILE.M).avatar_url).toBeNull(); + }); + + it("#2406 criterion 17: a non-missionary with a valid own coverUrl still gets 403", async () => { + await rejected("profile", "D", { coverUrl: own("covers", "D") }, 403); + }); + it("#2406 criterion 17: a non-missionary with an invalid coverUrl gets 400 (validated before the role check)", async () => { + await rejected("profile", "D", { + coverUrl: "https://evil.example.com/cover.webp", + }); + }); +}); + +describe("#2406 criterion 18: PATCH /api/donor/portal avatarUrl (OQ3)", () => { + it("#2406 criterion 18 (c11): own avatars/T/U URL writes profiles.avatar_url and donors.avatar_url", async () => { + await accepted("donor", "D", { avatarUrl: own("avatars", "D") }); + expect(row("profiles", "id", PROFILE.D).avatar_url).toBe( + own("avatars", "D"), + ); + expect(row("donors", "profile_id", PROFILE.D).avatar_url).toBe( + own("avatars", "D"), + ); + }); + it("#2406 criterion 18 (c12): null clears both columns", async () => { + await accepted("donor", "D", { avatarUrl: null }); + expect(row("profiles", "id", PROFILE.D).avatar_url).toBeNull(); + expect(row("donors", "profile_id", PROFILE.D).avatar_url).toBeNull(); + }); + it("#2406 criterion 18 (c15): the stored legacy-object URL resent unchanged returns 200 without rewrite", async () => { + await accepted("donor", "D", { + phone: "555-0100", + avatarUrl: STORED.donor, + }); + expect(wrote("avatar_url")).toBe(false); + expect(row("donors", "profile_id", PROFILE.D).avatar_url).toBe( + STORED.donor, + ); + }); + const mineD = `avatars/${T}/${UD}`; + it.each([ + ["a different external URL", "https://images.unsplash.com/photo-2?w=400"], + ["another user's folder", `${P}/avatars/${T}/${UM}/${F}`], + ["another tenant's folder", `${P}/avatars/${T2}/${UD}/${F}`], + ["the covers/ purpose", `${P}/covers/${T}/${UD}/${F}`], + ["a different legacy-object URL", `${P}/avatars/${UD}-999.webp`], + [ + "a host sharing a string prefix", + `${ORIGIN}.evil.example/storage/v1/object/public/profiles/${mineD}/${F}`, + ], + ["a raw .. segment", `${P}/${mineD}/../${UD}/${F}`], + ["a query string", `${P}/${mineD}/${F}?x=1`], + ])( + "#2406 criterion 18 (c13/c14): %s returns 400 with no write", + (_l, value) => + rejected("donor", "D", { firstName: "Changed", avatarUrl: value }), + ); + // Addendum 1 criterion 8: a value stored in only one of the two columns is + // backfilled into the other; stored in both, neither column is written. + const backfill = async (profiles: string | null, donors: string | null) => { + row("profiles", "id", PROFILE.D).avatar_url = profiles; + row("donors", "profile_id", PROFILE.D).avatar_url = donors; + await accepted("donor", "D", { avatarUrl: own("avatars", "D") }); + expect(row("profiles", "id", PROFILE.D).avatar_url).toBe( + own("avatars", "D"), + ); + expect(row("donors", "profile_id", PROFILE.D).avatar_url).toBe( + own("avatars", "D"), + ); + }; + it("#2406 criterion 8 (addendum 1): donors X and profiles NULL, send X: both become X", () => + backfill(null, own("avatars", "D"))); + it("#2406 criterion 8 (addendum 1): profiles X and donors NULL, send X: both become X", () => + backfill(own("avatars", "D"), null)); + it("#2406 criterion 8 (addendum 1): both X, send X: avatar_url is in neither update payload", async () => { + await backfill(own("avatars", "D"), own("avatars", "D")); + expect(wrote("avatar_url")).toBe(false); + }); + it("#2406 criterion 18 (c16): NEXT_PUBLIC_SUPABASE_URL unset: changed URL 503 with no write, null 200", async () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", undefined); + await rejected("donor", "D", { avatarUrl: own("avatars", "D") }, 503); + await accepted("donor", "D", { avatarUrl: null }); + }); +}); diff --git a/tests/unit/acceptance/issue-2406/profiles-storage.realstack.acceptance.test.ts b/tests/unit/acceptance/issue-2406/profiles-storage.realstack.acceptance.test.ts new file mode 100644 index 000000000..0a88779e9 --- /dev/null +++ b/tests/unit/acceptance/issue-2406/profiles-storage.realstack.acceptance.test.ts @@ -0,0 +1,471 @@ +/** #2406 spec r2 real stack (throwaway DB; skips cleanly without + * TENANT_GUARD_IT_ADMIN_DATABASE_URL). Criteria 2-7 and 9 run twice: on + * storage table grants only, then on criterion 8's hosted grants plus private + * default privileges (applied before the forward migration). Criterion 10's + * round trips and the M5/M14 (plus M1-M4/M17 reference) mutant probes run on + * the first stack only, after the read-only criteria. + * Mutants: M1 c4 "uid", M2 c4 "tenant", M3 c4 "purpose", M4 c4 "three + * segments", M5 c6 "rename outside", M6 c2, M10 c10, M14 c7/c9, M15 c2 backup + * row, M16 c2/c8 privileges, M17 c5 "S2 into demo". Addendum 1 criterion 5: + * Z (donor, NULL tenant) has no folder at all, not even the demo one. + */ +import { randomUUID } from "node:crypto"; + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import * as H from "./harness"; + +const URL = process.env.TENANT_GUARD_IT_ADMIN_DATABASE_URL ?? ""; +type Case = [label: string, role: H.Role, actor: string | null, name: string]; +const file = (ext = "webp") => `${randomUUID()}.${ext}`; + +/** Runs allow/deny cases; returns the labels that did not behave. */ +async function inserts(s: H.Stack, allow: Case[], deny: Case[]) { + const issues: string[] = []; + for (const [label, role, actor, name] of allow) { + const r = await s.insert(role, actor, name, label.includes("RETURNING")); + if (r.code || r.count !== 1) issues.push(`allow ${label}: ${r.code}`); + } + for (const [label, role, actor, name] of deny) { + const before = await s.objects(); + const r = await s.insert(role, actor, name); + if (r.code !== "42501" || (await s.objects()) !== before) + issues.push(`deny ${label}: ${r.code ?? "allowed"}`); + } + return issues; +} +const allow3 = (s: H.Stack): Case[] => [ + [ + "U avatars", + "authenticated", + "U", + `${s.path("avatars", "T", "U")}/${file()}`, + ], + [ + "U covers", + "authenticated", + "U", + `${s.path("covers", "T", "U")}/${file("jpg")}`, + ], + [ + "U RETURNING id", + "authenticated", + "U", + `${s.path("avatars", "T", "U")}/${file("png")}`, + ], +]; +const deny4 = (s: H.Stack): Case[] => [ + [ + "uid segment != U", + "authenticated", + "U", + `${s.path("avatars", "T", "X")}/${file()}`, + ], + [ + "tenant segment != T", + "authenticated", + "U", + `${s.path("avatars", "T2", "U")}/${file()}`, + ], + [ + "purpose documents", + "authenticated", + "U", + `${s.path("documents", "T", "U")}/x.webp`, + ], + ["three segments", "authenticated", "U", s.path("avatars", "T", "U")], + ["empty remainder", "authenticated", "U", `${s.path("avatars", "T", "U")}/`], + ["legacy shape", "authenticated", "U", `avatars/${s.ids.U}-123.webp`], + ["bucket root", "authenticated", "U", "x.webp"], + ["role anon", "anon", null, `${s.path("avatars", "T", "U")}/${file()}`], + ["no sub", "authenticated", null, `${s.path("avatars", "T", "U")}/${file()}`], + [ + "sub without profile", + "authenticated", + "N", + `${s.path("avatars", "T", "N")}/${file()}`, + ], + [ + "sub without profile, demo", + "authenticated", + "N", + `${s.path("avatars", H.DEMO, "N")}/${file()}`, + ], + [ + "X into tenant T", + "authenticated", + "X", + `${s.path("avatars", "T", "X")}/${file()}`, + ], +]; +const allow5 = (s: H.Stack): Case[] => [ + [ + "S into demo", + "authenticated", + "S", + `${s.path("avatars", H.DEMO, "S")}/${file()}`, + ], + [ + "S2 into own tenant", + "authenticated", + "S2", + `${s.path("avatars", "T", "S2")}/${file()}`, + ], +]; +const deny5 = (s: H.Stack): Case[] => [ + [ + "S into tenant T", + "authenticated", + "S", + `${s.path("avatars", "T", "S")}/${file()}`, + ], + [ + "S2 into demo", + "authenticated", + "S2", + `${s.path("avatars", H.DEMO, "S2")}/${file()}`, + ], +]; +/** Addendum 1 criterion 5: a tenant-less non-super_admin has no folder. */ +const denyZ = (s: H.Stack): Case[] => [ + [ + "Z into tenant T", + "authenticated", + "Z", + `${s.path("avatars", "T", "Z")}/${file()}`, + ], + [ + "Z into demo avatars", + "authenticated", + "Z", + `${s.path("avatars", H.DEMO, "Z")}/${file()}`, + ], + [ + "Z into demo covers", + "authenticated", + "Z", + `${s.path("covers", H.DEMO, "Z")}/${file("jpg")}`, + ], +]; +/** Addendum 1 criterion 5: Z's owned legacy object cannot move into demo. */ +async function zUpdate(s: H.Stack) { + const issues: string[] = []; + const legacy = `avatars/${s.ids.Z}-123.webp`; + const to = `${s.path("avatars", H.DEMO, "Z")}/moved.webp`; + const r = await s.update("Z", legacy, { name: to }); + if (r.code !== "42501") + issues.push(`Z legacy into demo: ${r.code ?? "allowed"}`); + const [kept] = await s.rows( + "select count(*)::int as n from storage.objects where bucket_id='profiles' and name=$1 and owner=$2", + [legacy, s.ids.Z], + ); + if (kept!.n !== 1 || (await s.objects()) !== 6) + issues.push("Z legacy changed"); + return issues; +} + +/** Criterion 6 (OQ5): returns the cases that did not behave. */ +async function updates(s: H.Stack) { + const issues: string[] = []; + const seed = `${s.path("avatars", "T", "U")}/seed.webp`; + const legacy = `avatars/${s.ids.U}-123.webp`; + const own = await s.update("U", seed, {}); + if (own.code || own.count !== 1 || own.rows[0]?.name !== seed) + issues.push(`owner in-folder update: ${own.code ?? own.count}`); + for (const [label, to] of [ + ["rename outside folder", `${s.path("avatars", "T", "X")}/moved.webp`], + ["rename to legacy", `avatars/${s.ids.U}-999.webp`], + ] as const) + if ((await s.update("U", seed, { name: to })).code !== "42501") + issues.push(label); + const other = await s.update("X", seed, {}); + if (other.code || other.count !== 0) issues.push("non-owner update"); + if ((await s.update("U", legacy, {})).code !== "42501") + issues.push("legacy in-place update"); + const moved = await s.update("U", legacy, { + name: `${s.path("avatars", "T", "U")}/moved.webp`, + }); + if (moved.code || moved.count !== 1) issues.push("legacy move into folder"); + return issues; +} + +/** Criterion 7: owner-only SELECT, owner-only DELETE. */ +async function reads(s: H.Stack) { + const issues: string[] = []; + const anon = await s.list("anon", null); + if (anon.code || anon.count !== 0) + issues.push(`anon sees ${anon.count} rows`); + const seed = `${s.path("avatars", "T", "U")}/seed.webp`; + const legacy = `avatars/${s.ids.U}-123.webp`; + const mine = await s.list("authenticated", "U"); + if (mine.rows.some((r) => r.owner !== s.ids.U)) + issues.push("U sees others' rows"); + const names = mine.rows.map((r) => String(r.name)).sort(); + if (mine.code || names.join() !== [legacy, seed].sort().join()) + issues.push("U does not see exactly own rows"); + const foreign = await s.remove("X", seed); + if (foreign.code || foreign.count !== 0) issues.push("non-owner delete"); + for (const name of [legacy, seed]) { + const r = await s.remove("U", name); + if (r.code || r.count !== 1) issues.push(`owner delete ${name}`); + } + return issues; +} + +const profilesPolicy = (p: Record) => + /'profiles'/.test(`${p.qual} ${p.with_check}`); + +function criteria(hosted: boolean) { + const tag = hosted ? " [c8 hosted grants]" : ""; + const title = `#2406 real-stack${hosted ? " (criterion 8: hosted grants + private default privileges)" : ""}`; + describe.skipIf(!URL)(title, () => { + let s: H.Stack; + beforeAll(async () => { + s = new H.Stack(URL, hosted); + await s.open(); + }, 300_000); + afterAll(async () => { + if (s) await s.close(); + }); + const check = (name: string, fn: () => Promise) => + it(`#2406 criterion ${name}${tag}`, () => { + H.migrated(); + return s.tx(fn); + }); + + check( + "2: profiles bucket limits, other buckets unchanged, prior limits recorded, backup table unreachable (M6/M15/M16)", + async () => { + expect( + s.priorApplied, + "storage.buckets lacks the limit columns (§2.3 bootstrap)", + ).toBe(true); + const [b] = await s.rows( + "select public from storage.buckets where id='profiles'", + ); + expect(b!.public).toBe(true); + expect(await s.limits()).toEqual({ + size: String(H.LIMIT), + types: H.TYPES, + }); + const others = (rows: Record[]) => + rows.filter((r) => r.id !== "profiles"); + expect(others(await s.buckets())).toEqual(others(s.pre.buckets)); + expect( + await s.rows( + `select bucket_id, file_size_limit::text as size, allowed_mime_types as types from ${H.BACKUP}`, + ), + ).toEqual([{ bucket_id: "profiles", ...H.PRIOR }]); + expect(await s.privileges(H.BACKUP)).toEqual([]); + }, + ); + check( + "3: U inserts avatars/T/U/.webp and covers/T/U/.jpg, also with RETURNING id", + async () => { + expect(await inserts(s, allow3(s), [])).toEqual([]); + }, + ); + check( + "4: RLS refuses every off-layout or unauthenticated INSERT with 42501 and adds no row (M1-M4)", + async () => { + expect(await inserts(s, [], deny4(s))).toEqual([]); + }, + ); + check( + "5: super_admin without tenant uses demo only; super_admin with tenant never demo (OQ4, M17)", + async () => { + expect(await inserts(s, allow5(s), deny5(s))).toEqual([]); + }, + ); + check( + "5 (addendum 1): donor Z with NULL tenant: INSERT into avatars/T/Z, avatars/demo/Z and covers/demo/Z refused 42501, no row; legacy UPDATE into avatars/demo/Z refused, unchanged", + async () => { + expect(await inserts(s, [], denyZ(s))).toEqual([]); + expect(await zUpdate(s)).toEqual([]); + }, + ); + check( + "6: owner UPDATE stays in folder, non-owner 0 rows, legacy can only move in (OQ5, M5)", + async () => { + expect(await updates(s)).toEqual([]); + }, + ); + check( + "7: anon lists 0 rows, U sees only own rows, owner-only DELETE, delete policy unchanged (M14)", + async () => { + expect(await reads(s)).toEqual([]); + const del = (rows: Record[]) => + rows.find((p) => p.policyname === H.DELETE_POLICY); + expect(del(await s.policies())).toEqual(del(s.pre.policies)); + expect(del(s.pre.policies)).toBeDefined(); + }, + ); + check( + "9: exactly four profiles policies; other buckets' policies unchanged (M14)", + async () => { + const now = await s.policies(); + expect( + now + .filter(profilesPolicy) + .map((p) => p.policyname) + .sort(), + ).toEqual([...H.NEW_POLICIES, H.DELETE_POLICY].sort()); + expect(now.map((p) => p.policyname)).not.toContain( + "Public read profiles", + ); + const shape = Object.fromEntries( + now.map((p) => [p.policyname, [p.cmd, p.roles]]), + ); + expect(shape).toMatchObject({ + "profiles owner folder insert": ["INSERT", ["authenticated"]], + "profiles owner folder update": ["UPDATE", ["authenticated"]], + "profiles owner select": ["SELECT", ["authenticated"]], + }); + const rest = (rows: Record[]) => + rows.filter((p) => !profilesPolicy(p)); + expect(rest(now)).toEqual(rest(s.pre.policies)); + expect(rest(now).map((p) => p.policyname)).toEqual( + expect.arrayContaining([ + "Public read document-uploads", + "Public read email-assets", + ]), + ); + }, + ); + + if (hosted) return; + + // Probes: swap in a known mutant and show the criterion check flags it. + const tenantOf = `(select coalesce(p.tenant_id::text, case when p.role = 'super_admin' then '${H.DEMO}' end) + from public.profiles p where p.user_id = (select auth.uid()))`; + const part = { + bucket: "bucket_id = 'profiles'", + purpose: "split_part(name,'/',1) in ('avatars','covers')", + tenant: `split_part(name,'/',2) = ${tenantOf}`, + uid: "split_part(name,'/',3) = (select auth.uid())::text", + rest: "split_part(name,'/',4) <> ''", + }; + const ref = (drop: keyof typeof part | null, extra = "") => + Object.entries(part) + .filter(([k]) => k !== drop) + .map( + ([, v]) => + `(${v}${extra && v === part.tenant ? ` or ${extra}` : ""})`, + ) + .join(" and "); + const anySuper = `(split_part(name,'/',2) = '${H.DEMO}' and exists (select 1 from public.profiles p + where p.user_id = (select auth.uid()) and p.role = 'super_admin'))`; + it.each([ + ["reference policy (harness self-check)", ref(null), null], + [ + "M1 drops the uid segment check", + ref("uid"), + "deny uid segment != U: allowed", + ], + [ + "M2 drops the tenant segment check", + ref("tenant"), + "deny tenant segment != T: allowed", + ], + [ + "M3 accepts any purpose", + ref("purpose"), + "deny purpose documents: allowed", + ], + [ + "M4 accepts three segments", + ref("rest"), + "deny three segments: allowed", + ], + [ + "M17 demo tenant for any super_admin", + ref(null, anySuper), + "deny S2 into demo: allowed", + ], + ])("#2406 mutant probe: INSERT %s", (_label, expr, issue) => { + H.migrated(); + return s.tx(async () => { + await s.db.query( + `alter policy "profiles owner folder insert" on storage.objects with check (${expr})`, + ); + const found = await inserts( + s, + [...allow3(s), ...allow5(s)], + [...deny4(s), ...deny5(s)], + ); + if (issue) expect(found).toContain(issue); + else expect(found).toEqual([]); + }); + }); + it("#2406 mutant probes: M5 UPDATE WITH CHECK without folder, M14 SELECT without owner / public read kept", () => { + H.migrated(); + return s.tx(async () => { + await s.db.query("savepoint probe"); + await s.db + .query(`alter policy "profiles owner folder update" on storage.objects + with check (bucket_id = 'profiles' and owner = (select auth.uid()))`); + expect(await updates(s)).toContain("rename outside folder"); + await s.db.query("rollback to savepoint probe"); + await s.db.query( + `alter policy "profiles owner select" on storage.objects using (bucket_id = 'profiles')`, + ); + expect(await reads(s)).toContain("U sees others' rows"); + await s.db.query("rollback to savepoint probe"); + await s.db.query( + `create policy "Public read profiles" on storage.objects for select using (bucket_id = 'profiles')`, + ); + expect((await reads(s))[0]).toMatch(/^anon sees [1-9]/); + }); + }); + + describe("#2406 criterion 10: rollback (runs last; mutates the stack)", () => { + const run = (path: string) => expect(s.psql(path).status, path).toBe(0); + it("#2406 criterion 10: migrate -> rollback -> migrate; rollback restores the pre-migration catalog and recorded non-NULL limits, drops the backup (M10)", async () => { + H.migrated(); + expect(s.priorApplied).toBe(true); + run(H.ROLLBACK); + expect(await s.policies()).toEqual(s.pre.policies); + expect(await s.limits()).toEqual(H.PRIOR); + expect(await s.regclass(H.BACKUP)).toBeNull(); + run(H.FORWARD); + expect(await s.limits()).toEqual({ + size: String(H.LIMIT), + types: H.TYPES, + }); + expect((await s.policies()).filter(profilesPolicy).length).toBe(4); + }); + it("#2406 criterion 10: a run starting from NULL limits round-trips back to NULL", async () => { + H.migrated(); + run(H.ROLLBACK); + await s.db.query( + "update storage.buckets set file_size_limit=null, allowed_mime_types=null where id='profiles'", + ); + run(H.FORWARD); + expect(await s.limits()).toEqual({ + size: String(H.LIMIT), + types: H.TYPES, + }); + run(H.ROLLBACK); + expect(await s.limits()).toEqual({ size: null, types: null }); + expect(await s.policies()).toEqual(s.pre.policies); + run(H.FORWARD); + }); + it("#2406 criterion 10: rollback without the recorded row raises and changes nothing", async () => { + H.migrated(); + await s.db.query(`delete from ${H.BACKUP}`); + const before = { + policies: await s.policies(), + limits: await s.limits(), + }; + expect(s.psql(H.ROLLBACK).status, "rollback must fail").not.toBe(0); + expect({ + policies: await s.policies(), + limits: await s.limits(), + }).toEqual(before); + }); + }); + }); +} + +criteria(false); +criteria(true); diff --git a/tests/unit/acceptance/issue-2406/static.acceptance.test.ts b/tests/unit/acceptance/issue-2406/static.acceptance.test.ts new file mode 100644 index 000000000..24d18b544 --- /dev/null +++ b/tests/unit/acceptance/issue-2406/static.acceptance.test.ts @@ -0,0 +1,464 @@ +/** #2406 spec r2 static contract (no database): criterion 1 (version, rollback + * header), 9 (migration text: GRANT-free, single REVOKE target, policy names, + * string_to_array, (SELECT auth.uid())), 10 (rollback text), §2.3 bootstrap + * columns, 19/20 (Builder tests present), 21 (doc), 22 (CI append + pin), 23 + * (append-only init, allowlist lines only, service-role gate), §6 declared + * edits, and text mutants M6, M10, M14-M16 (each with a self-check that the + * predicate rejects a synthetic mutant). Runtime mutants are mapped in the + * real-stack and route files. + */ +import { spawnSync } from "node:child_process"; +import { createHash } from "node:crypto"; +import { existsSync, readFileSync, readdirSync } from "node:fs"; + +import { describe, expect, it } from "vitest"; + +import * as H from "./harness"; +import { DEMO_TENANT_ID } from "../../../../packages/auth/constants"; +import { + loadWorkflowDocument, + PINNED_STEP_BODIES, + workflowJobs, +} from "../../helpers/workflow-yaml"; + +const fwd = () => H.code(H.read(H.FORWARD)); +const back = () => H.code(H.read(H.ROLLBACK)); +const statements = (sql: string, re: RegExp) => + sql + .split(";") + .map(H.squash) + .filter((s) => re.test(s)); +const ALLOWLIST = "docs/security/service-role-client-allowlist.json"; +const STEP = "Verify tenant isolation (real stack)"; +const git = (...args: string[]) => spawnSync("git", args, { encoding: "utf8" }); + +/** + * PR base for the base-relative checks (criterion 1 order, 23 allowlist, §6 + * scope). Branch-agnostic on purpose: ACCEPTANCE_2406_BASE, else + * `git merge-base HEAD origin/develop`, never a pinned develop SHA, so files + * other PRs merge into develop never count as #2406 edits. Returns a skip + * reason when the base is unknown, when a merge queue commit may carry PRs + * queued ahead of this one, or when the forward migration is already on the + * base (#2406 merged: the scope belonged to its own PR, which CI checked). + */ +function prBase(): { base: string } | { skip: string } { + if (process.env.GITHUB_EVENT_NAME === "merge_group") + return { skip: "merge_group commits can include other queued PRs" }; + const pinned = process.env.ACCEPTANCE_2406_BASE?.trim(); + const r = pinned + ? git("rev-parse", "--verify", `${pinned}^{commit}`) + : git("merge-base", "HEAD", "origin/develop"); + if (r.status !== 0) + return { + skip: "no origin/develop (or ACCEPTANCE_2406_BASE) in this checkout", + }; + const base = r.stdout.trim(); + if (git("cat-file", "-e", `${base}:${H.FORWARD}`).status === 0) + return { skip: `#2406 already merged at base ${base}; scope was its PR's` }; + return { base }; +} +const withBase = (ctx: { skip: (note?: string) => void }) => { + const b = prBase(); + if ("skip" in b) ctx.skip(b.skip); + return (b as { base: string }).base; +}; + +/** M16 predicate: one REVOKE ALL on the backup table naming all four roles. */ +const revokesBackup = (sql: string) => { + const revokes = statements(sql, /^REVOKE\b/i); + return ( + revokes.length > 0 && + revokes.every((s) => + new RegExp( + `\\bON\\s+(?:TABLE\\s+)?${H.BACKUP.replace(".", "\\.")}\\s+FROM\\b`, + "i", + ).test(s), + ) && + revokes.some((s) => { + const roles = s + .split(/\bFROM\b/i)[1]! + .toLowerCase() + .split(",") + .map((r) => r.trim()); + return ( + /^REVOKE\s+ALL\b/i.test(s) && + ["public", "anon", "authenticated", "service_role"].every((r) => + roles.includes(r), + ) + ); + }) + ); +}; +/** M6 predicate: the profiles UPDATE sets exactly 5242880 and the four types. */ +const setsLimits = (sql: string) => { + const [u] = statements(sql, /^UPDATE\s+storage\.buckets\b/i); + if ( + !u || + !/WHERE\s+id\s*=\s*'profiles'\s*$/i.test(u) || + /\bpublic\s*=/i.test(u) + ) + return false; + const size = + u.match(/file_size_limit\s*=\s*(\d+(?:\s*\*\s*\d+)*)/i)?.[1] ?? ""; + // Addendum 1: every ARRAY entry must be one of the four quoted image types; + // any other entry of any kind (e.g. 'application/pdf') fails the predicate. + const entries = ( + u.match(/allowed_mime_types\s*=\s*ARRAY\s*\[([^\]]*)\]/i)?.[1] ?? "" + ) + .split(",") + .map((e) => e.trim().match(/^'(image\/[a-z+.-]+)'$/)?.[1] ?? ""); + return ( + size.split("*").reduce((n, f) => n * Number(f), 1) === H.LIMIT && + [...new Set(entries)].sort().join() === H.TYPES.join() && + entries.length === 4 + ); +}; +/** M15 predicate: the record INSERT keeps the first row on re-apply. */ +const recordsFirst = (sql: string) => { + const [ins] = statements( + sql, + new RegExp(`^INSERT\\s+INTO\\s+${H.BACKUP.replace(".", "\\.")}\\b`, "i"), + ); + return Boolean( + ins && + /FROM\s+storage\.buckets/i.test(ins) && + /ON\s+CONFLICT[^;]*DO\s+NOTHING/i.test(ins) && + !/DO\s+UPDATE/i.test(ins), + ); +}; +/** M10 predicate: rollback restores from the backup and never NULLs the limits. + * Addendum 1: an UPDATE storage.buckets must read both limit columns from the + * backup table (alias form `col = r.col ... FROM backup AS r`). */ +const restoresRecorded = (sql: string) => + new RegExp(`\\b${H.BACKUP.replace(".", "\\.")}\\b`).test(sql) && + statements(sql, /^UPDATE\s+storage\.buckets\b/i).some((u) => { + const r = u.match( + new RegExp( + `FROM\\s+${H.BACKUP.replace(".", "\\.")}\\s+(?:AS\\s+)?(\\w+)\\b`, + "i", + ), + )?.[1]; + return ( + !!r && + ["file_size_limit", "allowed_mime_types"].every((c) => + new RegExp(`\\b${c}\\s*=\\s*${r}\\.${c}\\b`, "i").test(u), + ) + ); + }) && + /RAISE\s+EXCEPTION/i.test(sql) && + !/(file_size_limit|allowed_mime_types)\s*=\s*NULL\b/i.test(sql) && + new RegExp( + `DROP\\s+TABLE\\s+(?:IF\\s+EXISTS\\s+)?${H.BACKUP.replace(".", "\\.")}`, + "i", + ).test(sql); +/** The three original CREATE POLICY statements, verbatim from init_schema. */ +const originals = () => + H.OLD_POLICIES.map( + (name) => + statements( + H.code(H.read(H.INIT)), + new RegExp(`^CREATE POLICY "${name}"`), + )[0]!, + ); + +describe("#2406 static contract", () => { + it("#2406 criterion 1: forward migration exists, header names the rollback, rollback exists", () => { + expect(H.read(H.FORWARD), H.FORWARD).not.toBe(""); + expect(H.read(H.ROLLBACK), H.ROLLBACK).not.toBe(""); + const header = H.read(H.FORWARD) + .split("\n") + .filter((l) => l.startsWith("--")) + .join("\n"); + expect(header).toContain(`rollback_${H.NAME}`); + expect(H.VERSION > "20261026171000", "above every §10 reservation").toBe( + true, + ); + }); + it("#2406 criterion 1: the version sorts above every forward migration on the PR base", (ctx) => { + const base = withBase(ctx); + const r = git("ls-tree", "--name-only", `${base}:${H.MIG}`); + expect(r.status).toBe(0); + const higher = r.stdout + .split("\n") + .filter((f) => /^\d{14}_/.test(f) && f.slice(0, 14) >= H.VERSION); + expect(higher, `base ${base}`).toEqual([]); + const local = readdirSync(H.MIG).filter( + (f) => /^\d{14}_/.test(f) && f.slice(0, 14) === H.VERSION, + ); + expect(local).toEqual([H.NAME]); + }); + it("#2406 criterion 9 / §2.1: migration text is GRANT-free and its only REVOKE targets the backup table (M16)", () => { + expect(fwd(), H.FORWARD).not.toBe(""); + expect(statements(fwd(), /\bGRANT\b/i)).toEqual([]); + expect(revokesBackup(fwd())).toBe(true); + expect( + revokesBackup( + `REVOKE ALL ON ${H.BACKUP} FROM PUBLIC, anon, service_role;`, + ), + ).toBe(false); + expect( + revokesBackup( + `REVOKE ALL ON ${H.BACKUP} FROM PUBLIC, anon, authenticated, service_role; REVOKE ALL ON storage.objects FROM anon;`, + ), + ).toBe(false); + }); + it("#2406 criterion 9 / §2.1: backup table shape, prior limits recorded first-wins before the bucket update (M15)", () => { + const sql = fwd(); + const [table] = statements( + sql, + new RegExp( + `^CREATE TABLE (IF NOT EXISTS )?${H.BACKUP.replace(".", "\\.")}`, + "i", + ), + ); + expect(table, "CREATE TABLE backup").toBeDefined(); + for (const col of [ + /bucket_id\s+TEXT\s+PRIMARY\s+KEY/i, + /file_size_limit\s+BIGINT/i, + /allowed_mime_types\s+TEXT\s*\[\]/i, + /recorded_at\s+TIMESTAMPTZ\s+NOT\s+NULL\s+DEFAULT\s+now\(\)/i, + ]) + expect(table).toMatch(col); + expect(recordsFirst(sql)).toBe(true); + expect( + recordsFirst( + sql.replace( + /DO\s+NOTHING/gi, + "DO UPDATE SET file_size_limit = EXCLUDED.file_size_limit", + ), + ), + ).toBe(false); + expect( + sql.search( + new RegExp(`INSERT\\s+INTO\\s+${H.BACKUP.replace(".", "\\.")}`, "i"), + ), + ).toBeLessThan(sql.search(/UPDATE\s+storage\.buckets/i)); + }); + it("#2406 criterion 2 / §1.4 text: limits are exactly 5242880 and the four image types, public untouched (M6)", () => { + expect(setsLimits(fwd())).toBe(true); + const ok = `UPDATE storage.buckets SET file_size_limit = 5242880, allowed_mime_types = ARRAY['image/jpeg','image/png','image/webp','image/gif'] WHERE id = 'profiles';`; + expect(setsLimits(ok)).toBe(true); + expect(setsLimits(ok.replace("5242880", "10485760"))).toBe(false); + expect( + setsLimits(ok.replace("'image/gif'", "'image/gif','image/svg+xml'")), + ).toBe(false); + // Addendum 1: a non-image entry of any kind must fail, not be ignored. + expect( + setsLimits(ok.replace("'image/gif'", "'image/gif','application/pdf'")), + ).toBe(false); + }); + it("#2406 criterion 9 / §2.1: drops the three originals, creates the three named policies TO authenticated, leaves delete alone (M14)", () => { + const sql = fwd(); + for (const name of H.OLD_POLICIES) + expect(sql).toMatch( + new RegExp( + `DROP POLICY (IF EXISTS )?"${name}" ON storage\\.objects`, + "i", + ), + ); + expect(statements(sql, /^CREATE POLICY "Public read profiles"/i)).toEqual( + [], + ); + for (const [name, cmd] of [ + [H.NEW_POLICIES[0], "INSERT"], + [H.NEW_POLICIES[1], "UPDATE"], + [H.NEW_POLICIES[2], "SELECT"], + ] as const) { + const [s] = statements(sql, new RegExp(`^CREATE POLICY "${name}"`, "i")); + expect(s, name).toMatch( + new RegExp( + `ON storage\\.objects (AS PERMISSIVE )?FOR ${cmd} TO authenticated\\b`, + "i", + ), + ); + expect(s).toContain("'profiles'"); + } + const [select] = statements(sql, /^CREATE POLICY "profiles owner select"/i); + expect(select).toMatch( + /owner\s*=\s*\(\s*SELECT\s+auth\.uid\(\)\s*\)|\(\s*SELECT\s+auth\.uid\(\)\s*\)\s*=\s*owner/i, + ); + expect(sql).not.toContain(H.DELETE_POLICY); + }); + it("#2406 §2.1: folder check uses string_to_array(name, '/'), never storage.foldername; auth.uid() only as (SELECT auth.uid())", () => { + const sql = fwd(); + expect(sql).toMatch(/string_to_array\(\s*name\s*,\s*'\/'\s*\)/i); + expect(sql).not.toMatch(/storage\.foldername/i); + expect(sql).toMatch(/\(\s*SELECT\s+auth\.uid\(\)\s*\)/i); + expect(sql.replace(/\(\s*SELECT\s+auth\.uid\(\)\s*\)/gi, "")).not.toMatch( + /auth\.uid\(\)/i, + ); + expect(sql).toMatch(/\bprivate\b/); + expect(sql).toMatch(/CREATE SCHEMA IF NOT EXISTS private/i); + }); + it("#2406 criterion 10 / §2.2: rollback drops the new policies, recreates the originals verbatim, restores recorded limits or raises (M10)", () => { + const sql = back(); + expect(sql, H.ROLLBACK).not.toBe(""); + for (const name of H.NEW_POLICIES) + expect(sql).toMatch( + new RegExp( + `DROP POLICY (IF EXISTS )?"${name}" ON storage\\.objects`, + "i", + ), + ); + const recreated = statements(sql, /^CREATE POLICY "/i); + for (const original of originals()) expect(recreated).toContain(original); + expect(sql).not.toContain(H.DELETE_POLICY); + expect(restoresRecorded(sql)).toBe(true); + expect( + restoresRecorded( + `${sql}\nUPDATE storage.buckets SET file_size_limit = NULL WHERE id = 'profiles';`, + ), + ).toBe(false); + // Addendum 1: without the restoring UPDATE the rollback must fail the predicate. + expect(sql).toMatch(/UPDATE\s+storage\.buckets\b/i); + expect( + restoresRecorded(sql.replace(/UPDATE\s+storage\.buckets\b[^;]*;/i, "")), + ).toBe(false); + expect(statements(sql, /\bGRANT\b/i)).toEqual([]); + }); + it("#2406 §2.3: compat bootstrap storage.buckets has file_size_limit BIGINT and allowed_mime_types TEXT[] (CREATE and ADD COLUMN)", () => { + const sql = H.code(H.read(H.BOOTSTRAP)); + const [create] = statements( + sql, + /^CREATE TABLE IF NOT EXISTS storage\.buckets\b/i, + ); + expect(create).toMatch(/file_size_limit BIGINT/i); + expect(create).toMatch(/allowed_mime_types TEXT\[\]/i); + for (const col of [ + "file_size_limit BIGINT", + "allowed_mime_types TEXT\\[\\]", + ]) + expect(sql).toMatch( + new RegExp( + `ALTER TABLE (IF EXISTS )?storage\\.buckets[^;]*ADD COLUMN IF NOT EXISTS ${col}`, + "i", + ), + ); + }); + it("#2406 criteria 19/20: Builder's validator and handler unit tests exist", () => { + expect(existsSync("packages/api/src/profile/image-url.ts")).toBe(true); + expect( + existsSync("tests/unit/packages/api/profile/image-url.test.ts"), + ).toBe(true); + for (const dir of ["profile", "donor-portal"]) { + const at = `tests/unit/packages/api/${dir}`; + const tests = readdirSync(at) + .filter((f) => f !== "image-url.test.ts") + .map((f) => H.read(`${at}/${f}`)); + expect( + tests.some((t) => /avatarUrl/.test(t) && /\b400\b/.test(t)), + `${at} handler test`, + ).toBe(true); + } + }); + it("#2406 criterion 21: contract doc has the four policies, both limits, the backup table, key layout, URL rule and the three sections", () => { + const doc = H.read(H.DOC); + expect(doc, H.DOC).not.toBe(""); + for (const term of [ + ...H.NEW_POLICIES, + H.DELETE_POLICY, + String(H.LIMIT), + ...H.TYPES, + H.BACKUP, + "/storage/v1/object/public/profiles/", + ]) + expect(doc).toContain(term); + expect(doc).toMatch( + /\{?(avatars\|covers|purpose)\}?\/\{tenantId\}\/\{(userId|uid)\}\//, + ); + for (const heading of [ + /^#+ .*pre-deploy/im, + /^#+ (?!.*pre-deploy).*deploy/im, + /^#+ .*rollback/im, + ]) + expect(doc).toMatch(heading); + }); + it("#2406 criterion 22: the real-stack path is appended to the tenant isolation step and the shared pin matches", () => { + expect(existsSync(H.REALSTACK)).toBe(true); + const steps = workflowJobs( + loadWorkflowDocument(".github/workflows/ci-integration.yml"), + ).migrate!.steps!; + const i = steps.findIndex((s) => s.name === STEP); + expect(String(steps[i]!.run).split(/\s+/).at(-1)).toBe(H.REALSTACK); + const { index, ...rest } = PINNED_STEP_BODIES.migrate![`name:${STEP}`]!; + expect(index).toBe(i); + expect(steps[i]).toEqual({ name: STEP, ...rest }); + }); + it("#2406 criterion 23: init_schema stays byte-identical to 2b028084", () => { + const hash = createHash("sha256") + .update(readFileSync(H.INIT)) + .digest("hex"); + expect(hash).toBe( + "c5cc1775d6914b613730822d920f4ec3ba189cdb5e2f98fadb8981238e76635a", + ); + }); + it("#2406 criterion 23: bun run verify:service-role-clients passes", () => { + const r = spawnSync( + "node", + ["scripts/verify/service-role-client-gate.mjs"], + { encoding: "utf8", timeout: 120_000 }, + ); + expect(r.status, `${r.stdout}\n${r.stderr}`.slice(-2000)).toBe(0); + }); + it("#2406 criterion 23: allowlist differs from the PR base only in profile/donor-portal lines; counts equal", (ctx) => { + const base = withBase(ctx); + const shown = git("show", `${base}:${ALLOWLIST}`); + expect(shown.status).toBe(0); + type Entry = Record; + const before = JSON.parse(shown.stdout) as { entries: Entry[] } & Entry; + const after = JSON.parse(readFileSync(ALLOWLIST, "utf8")) as typeof before; + const moved = [ + "packages/api/src/profile/index.ts", + "packages/api/src/donor-portal/index.ts", + ]; + const strip = (doc: typeof before) => ({ + ...doc, + entries: doc.entries.map((e) => + moved.includes(String(e.file)) ? { ...e, lines: "" } : e, + ), + }); + expect(strip(after)).toEqual(strip(before)); + for (const e of after.entries) + expect((e.lines as unknown[]).length).toBe(e.count); + }); + it("#2406 §6: the PR diff touches only declared edits (branch-agnostic merge-base)", (ctx) => { + const base = withBase(ctx); + const r = git("diff", "--name-only", "--no-renames", base, "HEAD"); + expect(r.status).toBe(0); + const declared = new Set([ + H.FORWARD, + H.ROLLBACK, + H.BOOTSTRAP, + H.DOC, + ALLOWLIST, + "docs/security/identity-person-anchor.md", + ...`packages/api/src/profile/image-url.ts packages/api/src/profile/index.ts + packages/api/src/donor-portal/settings-patch.ts packages/api/src/donor-portal/index.ts + .github/workflows/ci-integration.yml tests/unit/helpers/workflow-yaml.ts`.split( + /\s+/, + ), + ...[ + "issue-2250/static", + "issue-2155/postgres-ecr-gates", + "issue-2201/merge-group", + "issue-2308/static", + "issue-2402/static", + ].map((p) => `tests/unit/acceptance/${p}.acceptance.test.ts`), + ]); + const prefixes = [ + `${H.DIR}/`, + "tests/unit/acceptance/issue-2256/", + "tests/unit/packages/api/profile/", + "tests/unit/packages/api/donor-portal/", + ]; + const outside = r.stdout + .split("\n") + .filter( + (p) => p && !declared.has(p) && !prefixes.some((x) => p.startsWith(x)), + ); + expect(outside, `diff against ${base}`).toEqual([]); + }); + it("#2406 harness: DEMO matches packages/auth/constants DEMO_TENANT_ID", () => { + expect(H.DEMO).toBe(DEMO_TENANT_ID); + }); +}); diff --git a/tests/unit/helpers/workflow-yaml.ts b/tests/unit/helpers/workflow-yaml.ts index bcb40897f..7ff0e76b6 100644 --- a/tests/unit/helpers/workflow-yaml.ts +++ b/tests/unit/helpers/workflow-yaml.ts @@ -452,7 +452,7 @@ export const PINNED_STEP_BODIES: Record< }, "name:Verify tenant isolation (real stack)": { index: 16, - 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", env: { TENANT_GUARD_IT_ADMIN_DATABASE_URL: "${{ env.DATABASE_URL }}" }, }, "name:Verify authz EXECUTE grants (real stack)": { diff --git a/tests/unit/packages/api/donor-portal/patch-avatar-url.test.ts b/tests/unit/packages/api/donor-portal/patch-avatar-url.test.ts new file mode 100644 index 000000000..84bb5583a --- /dev/null +++ b/tests/unit/packages/api/donor-portal/patch-avatar-url.test.ts @@ -0,0 +1,137 @@ +import { NextRequest } from "next/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +type Row = Record; +const m = vi.hoisted(() => ({ + ctx: {} as Row, + stored: { profile: null as string | null, donor: null as string | null }, + writes: [] as { table: string; values: Row }[], +})); + +/** Only updates reach the admin client; reads go through the mocked service. */ +function fakeAdmin() { + return { + from(table: string) { + const q: Row = { + eq: () => q, + update: (values: Row) => (m.writes.push({ table, values }), q), + then: (ok: (v: unknown) => unknown) => + Promise.resolve({ data: null, error: null }).then(ok), + }; + return q; + }, + }; +} + +vi.mock("@asym/database/supabase/admin", () => ({ + getAdminClient: () => ({ client: fakeAdmin(), error: null }), +})); +vi.mock("@asym/auth/context", () => ({ + getAuthContext: async () => m.ctx, + requireAuth: () => undefined, + requireRole: () => undefined, +})); +vi.mock("@asym/lib/audit/logger", () => ({ + createAuditLogger: () => ({ log: async () => undefined }), +})); +vi.mock("../../../../../packages/api/src/donor-portal/service", () => ({ + resolveDonorPortalContext: async () => ({ + profile: { id: "p-1", avatar_url: m.stored.profile }, + donor: { id: "d-1", avatar_url: m.stored.donor }, + }), + getDonorPortalSnapshot: async () => ({}), +})); +vi.mock( + "../../../../../packages/api/src/profile/image-url", + async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + checkProfileImageUrl: vi.fn(actual.checkProfileImageUrl), + }; + }, +); + +import { PATCH } from "../../../../../packages/api/src/donor-portal"; +import { checkProfileImageUrl } from "../../../../../packages/api/src/profile/image-url"; + +import type * as ImageUrl from "../../../../../packages/api/src/profile/image-url"; + +const ORIGIN = "https://proj.supabase.co"; +const T = "aaaaaaaa-0000-4000-8000-00000000000a"; +const U = "dddddddd-0000-4000-8000-00000000000d"; +const P = `${ORIGIN}/storage/v1/object/public/profiles`; +const OWN = `${P}/avatars/${T}/${U}/x.webp`; + +async function patch(body: Row) { + m.ctx = { userId: U, tenantId: T, profileId: "p-1", role: "donor" }; + const res = await PATCH( + new NextRequest("https://app.test/api/donor/portal", { + method: "PATCH", + body: JSON.stringify(body), + }), + ); + return { status: res.status, body: (await res.json()) as Row }; +} +const avatarWrites = () => + m.writes.filter((w) => "avatar_url" in w.values).map((w) => w.table); + +beforeEach(() => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", ORIGIN); + vi.mocked(checkProfileImageUrl).mockClear(); + m.writes = []; + m.stored = { profile: `${P}/avatars/${U}-123.webp`, donor: null }; +}); + +describe("PATCH /api/donor/portal avatarUrl (#2406)", () => { + it("validates with ctx.tenantId, ctx.userId and both stored avatars, then writes both tables", async () => { + const r = await patch({ avatarUrl: OWN }); + expect(r.status).toBe(200); + expect(checkProfileImageUrl).toHaveBeenCalledWith(OWN, { + purpose: "avatars", + tenantId: T, + userId: U, + stored: [m.stored.profile, null], + }); + expect(avatarWrites()).toEqual(["profiles", "donors"]); + }); + + it.each([ + ["an external URL", "https://images.unsplash.com/photo-2?w=400"], + ["another user's folder", `${P}/avatars/${T}/someone-else/x.webp`], + ["the covers purpose", `${P}/covers/${T}/${U}/x.webp`], + ])("rejects %s with 400 before any write", async (_label, avatarUrl) => { + const r = await patch({ firstName: "Changed", phone: "1", avatarUrl }); + expect(r.status).toBe(400); + expect(typeof r.body.error).toBe("string"); + expect(m.writes).toEqual([]); + }); + + const X = "https://images.unsplash.com/photo-1?w=400"; + it.each([ + ["donors X, profiles NULL", { profile: null, donor: X }, { profiles: X }], + ["profiles X, donors NULL", { profile: X, donor: null }, { donors: X }], + ["both X", { profile: X, donor: X }, {}], + ] as const)( + "%s, resend X: only a column that differs gets X (OQ2, criterion 8)", + async (_label, stored, expected) => { + m.stored = { ...stored }; + const r = await patch({ phone: "555-0100", avatarUrl: X }); + expect(r.status).toBe(200); + expect(m.writes.length).toBeGreaterThan(0); + const avatars = m.writes + .filter((w) => "avatar_url" in w.values) + .map((w) => [w.table, w.values.avatar_url]); + expect(Object.fromEntries(avatars)).toEqual(expected); + }, + ); + + it("returns 503 with no write for a new URL when NEXT_PUBLIC_SUPABASE_URL is unset; null still clears", async () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", undefined); + const r = await patch({ firstName: "Changed", avatarUrl: OWN }); + expect(r.status).toBe(503); + expect(m.writes).toEqual([]); + expect((await patch({ avatarUrl: null })).status).toBe(200); + expect(avatarWrites()).toEqual(["profiles"]); + }); +}); diff --git a/tests/unit/packages/api/profile/image-url.test.ts b/tests/unit/packages/api/profile/image-url.test.ts new file mode 100644 index 000000000..bd74d426d --- /dev/null +++ b/tests/unit/packages/api/profile/image-url.test.ts @@ -0,0 +1,203 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + checkProfileImageUrl, + getProfileImageOrigin, + isOwnProfileImageUrl, +} from "../../../../../packages/api/src/profile/image-url"; + +const ORIGIN = "https://proj.supabase.co"; +const P = `${ORIGIN}/storage/v1/object/public/profiles`; +const T = "aaaaaaaa-0000-4000-8000-00000000000a"; +const T2 = "bbbbbbbb-0000-4000-8000-00000000000b"; +const U = "cccccccc-0000-4000-8000-00000000000c"; +const X = "eeeeeeee-0000-4000-8000-00000000000e"; +const DEMO = "00000000-0000-0000-0000-000000000001"; +const F = "0b0e6c3e-7d1a-4b55-9b52-6a3f1d2c4e5f.webp"; +const own = { purpose: "avatars", tenantId: T, userId: U } as const; +const valid = (url: string, expected: Partial = {}) => + isOwnProfileImageUrl(url, { ...own, origin: ORIGIN, ...expected }); + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +describe("isOwnProfileImageUrl", () => { + it("accepts the caller's own avatars and covers object URLs", () => { + expect(valid(`${P}/avatars/${T}/${U}/${F}`)).toBe(true); + expect(valid(`${P}/covers/${T}/${U}/x.jpg`, { purpose: "covers" })).toBe( + true, + ); + expect(valid(`${P}/avatars/${T}/${U}/nested/x.png`)).toBe(true); + }); + + it("accepts the demo tenant folder only when the caller's tenant is the demo tenant", () => { + const url = `${P}/avatars/${DEMO}/${U}/${F}`; + expect(valid(url, { tenantId: DEMO })).toBe(true); + expect(valid(url)).toBe(false); + }); + + it.each([ + ["another user's folder", `${P}/avatars/${T}/${X}/${F}`], + ["another tenant's folder", `${P}/avatars/${T2}/${U}/${F}`], + ["the other purpose", `${P}/covers/${T}/${U}/${F}`], + ["an unknown purpose", `${P}/documents/${T}/${U}/${F}`], + ["a legacy object", `${P}/avatars/${U}-123.webp`], + ["a bucket-root object", `${P}/${U}.webp`], + ["a folder with no file", `${P}/avatars/${T}/${U}/`], + ["an empty segment", `${P}/avatars/${T}/${U}//${F}`], + ["an upper-cased tenant", `${P}/avatars/${T.toUpperCase()}/${U}/${F}`], + ["an upper-cased uid", `${P}/avatars/${T}/${U.toUpperCase()}/${F}`], + ["an upper-cased purpose", `${P}/AVATARS/${T}/${U}/${F}`], + ])("rejects %s", (_label, url) => { + expect(valid(url)).toBe(false); + }); + + const path = `/storage/v1/object/public/profiles/avatars/${T}/${U}/${F}`; + it.each([ + ["another origin", `https://evil.example${path}`], + ["a host extending the origin", `${ORIGIN}.evil.example${path}`], + ["a host sharing a string prefix", `${ORIGIN}evil.example${path}`], + ["http for an https origin", `http://proj.supabase.co${path}`], + ["a different port", `${ORIGIN}:8443${path}`], + ["an explicit default port", `${ORIGIN}:443${path}`], + ["an upper-cased host", `https://PROJ.supabase.co${path}`], + ["credentials", `https://user:pass@proj.supabase.co${path}`], + ["a username", `https://user@proj.supabase.co${path}`], + ])("rejects %s", (_label, url) => { + expect(valid(url)).toBe(false); + }); + + const mine = `avatars/${T}/${U}`; + it.each([ + [ + "another bucket", + `${ORIGIN}/storage/v1/object/public/documents/${mine}/${F}`, + ], + [ + "a bucket that starts with profiles", + `${ORIGIN}/storage/v1/object/public/profiles-archive/${mine}/${F}`, + ], + [ + "the sign endpoint", + `${ORIGIN}/storage/v1/object/sign/profiles/${mine}/${F}`, + ], + [ + "the authenticated endpoint", + `${ORIGIN}/storage/v1/object/authenticated/profiles/${mine}/${F}`, + ], + [ + "the render endpoint", + `${ORIGIN}/storage/v1/render/image/public/profiles/${mine}/${F}`, + ], + ["a query string", `${P}/${mine}/${F}?width=64`], + ["an empty query", `${P}/${mine}/${F}?`], + ["a fragment", `${P}/${mine}/${F}#top`], + ["a raw . segment", `${P}/${mine}/./${F}`], + ["a raw .. segment", `${P}/${mine}/../${U}/${F}`], + [ + "a .. segment out of another folder", + `${P}/avatars/${T}/${X}/../${U}/${F}`, + ], + ["an encoded .. segment", `${P}/${mine}/%2e%2e/${U}/${F}`], + ["an encoded . segment", `${P}/${mine}/%2E/${F}`], + ["a mixed encoded .. segment", `${P}/${mine}/.%2e/${F}`], + ["an encoded / in the file", `${P}/${mine}/a%2F${F}`], + ["an encoded / joining segments", `${P}/avatars%2F${T}/${U}/${F}`], + ["a backslash", `${P}/${mine}\\${F}`], + ["an empty object path", `${P}/`], + ["no object path", P], + ["an ftp scheme", `ftp://proj.supabase.co${path}`], + ["a javascript scheme", "javascript:alert(1)"], + ["an empty string", ""], + ["a relative URL", `/storage/v1/object/public/profiles/${mine}/${F}`], + ])("rejects %s", (_label, url) => { + expect(valid(url)).toBe(false); + }); + + it("rejects everything when the caller's tenant or uid is empty", () => { + expect(valid(`${P}/avatars//${U}/${F}`, { tenantId: "" })).toBe(false); + expect(valid(`${P}/avatars/${T}//${F}`, { userId: "" })).toBe(false); + }); +}); + +describe("getProfileImageOrigin", () => { + it.each([ + ["the plain URL", ORIGIN], + ["a trailing slash", `${ORIGIN}/`], + ["a mixed-case host", "https://Proj.Supabase.CO"], + ["a mixed-case host and trailing slash", "https://PROJ.supabase.co/"], + ["surrounding whitespace", ` ${ORIGIN} `], + ])("normalizes %s to the parsed origin", (_label, configured) => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", configured); + const origin = getProfileImageOrigin(); + expect(origin).toBe(ORIGIN); + expect( + isOwnProfileImageUrl(`${P}/avatars/${T}/${U}/${F}`, { + ...own, + origin: origin!, + }), + ).toBe(true); + }); + + it.each([ + ["unset", undefined], + ["empty", ""], + ["not a URL", "proj.supabase.co"], + ["not http(s)", "ftp://proj.supabase.co"], + ])("returns null when the configured URL is %s", (_label, configured) => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", configured); + expect(getProfileImageOrigin()).toBeNull(); + }); +}); + +describe("checkProfileImageUrl", () => { + const input = { ...own, stored: [] as (string | null)[] }; + const status = (fn: () => unknown) => { + try { + fn(); + } catch (error) { + return (error as { status?: number }).status; + } + return "no error"; + }; + + it("skips an absent value and writes null without needing config", () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", undefined); + expect(checkProfileImageUrl(undefined, input)).toBe(false); + expect(checkProfileImageUrl(null, input)).toBe(true); + }); + + it("skips a value equal to a stored value, even one that would fail (OQ2)", () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", undefined); + const external = "https://images.unsplash.com/photo-1?w=400"; + expect( + checkProfileImageUrl(external, { ...input, stored: [null, external] }), + ).toBe(false); + }); + + it("writes a new own-folder URL and rejects others with 400", () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", ORIGIN); + expect(checkProfileImageUrl(`${P}/avatars/${T}/${U}/${F}`, input)).toBe( + true, + ); + expect( + status(() => checkProfileImageUrl(`${P}/avatars/${T}/${X}/${F}`, input)), + ).toBe(400); + expect( + status(() => + checkProfileImageUrl("https://images.unsplash.com/photo-2", { + ...input, + stored: ["https://images.unsplash.com/photo-1"], + }), + ), + ).toBe(400); + }); + + it("returns 503 for a new non-null URL when the public URL is not configured (OQ9)", () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", undefined); + expect( + status(() => checkProfileImageUrl(`${P}/avatars/${T}/${U}/${F}`, input)), + ).toBe(503); + }); +}); diff --git a/tests/unit/packages/api/profile/patch-image-url.test.ts b/tests/unit/packages/api/profile/patch-image-url.test.ts new file mode 100644 index 000000000..23f95ec13 --- /dev/null +++ b/tests/unit/packages/api/profile/patch-image-url.test.ts @@ -0,0 +1,163 @@ +import { NextRequest } from "next/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +type Row = Record; +const m = vi.hoisted(() => ({ + ctx: {} as Row, + rows: {} as Record, + writes: [] as { table: string; values: Row }[], +})); + +/** Reads return m.rows[table]; updates are recorded in m.writes. */ +function fakeAdmin() { + return { + from(table: string) { + let values: Row | null = null; + const result = () => { + if (values) m.writes.push({ table, values }); + return { data: values ? null : (m.rows[table] ?? null), error: null }; + }; + const q: Row = { + select: () => q, + eq: () => q, + update: (v: Row) => ((values = v), q), + single: async () => result(), + maybeSingle: async () => result(), + then: (ok: (v: unknown) => unknown) => + Promise.resolve(result()).then(ok), + }; + return q; + }, + }; +} + +vi.mock("@asym/database/supabase/admin", () => ({ + getAdminClient: () => ({ client: fakeAdmin(), error: null }), +})); +vi.mock("@asym/auth/context", () => ({ + getAuthContext: async () => m.ctx, + requireAuth: () => undefined, + hasContextRole: (ctx: Row, role: string) => ctx.role === role, +})); +vi.mock( + "../../../../../packages/api/src/profile/image-url", + async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + checkProfileImageUrl: vi.fn(actual.checkProfileImageUrl), + }; + }, +); + +import { PATCH } from "../../../../../packages/api/src/profile"; +import { checkProfileImageUrl } from "../../../../../packages/api/src/profile/image-url"; + +import type * as ImageUrl from "../../../../../packages/api/src/profile/image-url"; + +const ORIGIN = "https://proj.supabase.co"; +const T = "aaaaaaaa-0000-4000-8000-00000000000a"; +const U = "cccccccc-0000-4000-8000-00000000000c"; +const own = (purpose: string) => + `${ORIGIN}/storage/v1/object/public/profiles/${purpose}/${T}/${U}/x.webp`; +const STORED_AVATAR = "https://images.unsplash.com/photo-1?w=400"; +const STORED_COVER = `${ORIGIN}/storage/v1/object/public/profiles/covers/${U}-1.webp`; + +async function patch(body: Row, role = "missionary") { + m.ctx = { userId: U, tenantId: T, profileId: "p-1", role }; + const res = await PATCH( + new NextRequest("https://app.test/api/profile", { + method: "PATCH", + body: JSON.stringify(body), + }), + ); + return { status: res.status, body: (await res.json()) as Row }; +} +const written = (table: string) => + m.writes.filter((w) => w.table === table).map((w) => w.values); + +beforeEach(() => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", ORIGIN); + vi.mocked(checkProfileImageUrl).mockClear(); + m.writes = []; + m.rows = { + profiles: { id: "p-1", avatar_url: STORED_AVATAR }, + missionaries: { profile_id: "p-1", cover_url: STORED_COVER }, + }; +}); + +describe("PATCH /api/profile image URLs (#2406)", () => { + it("passes ctx.tenantId, ctx.userId and the stored values to the validator", async () => { + await patch({ avatarUrl: own("avatars"), coverUrl: own("covers") }); + expect(checkProfileImageUrl).toHaveBeenCalledWith(own("avatars"), { + purpose: "avatars", + tenantId: T, + userId: U, + stored: [STORED_AVATAR], + }); + expect(checkProfileImageUrl).toHaveBeenCalledWith(own("covers"), { + purpose: "covers", + tenantId: T, + userId: U, + stored: [STORED_COVER], + }); + }); + + it("writes new own-folder avatarUrl and coverUrl", async () => { + const r = await patch({ + avatarUrl: own("avatars"), + coverUrl: own("covers"), + }); + expect(r.status).toBe(200); + expect(written("profiles")[0]).toMatchObject({ + avatar_url: own("avatars"), + }); + expect(written("missionaries")[0]).toMatchObject({ + cover_url: own("covers"), + }); + }); + + it("rejects an invalid avatarUrl with 400 before writing other fields", async () => { + const r = await patch({ + firstName: "Changed", + bio: "New", + avatarUrl: "https://evil.example/x.webp", + }); + expect(r.status).toBe(400); + expect(typeof r.body.error).toBe("string"); + expect(m.writes).toEqual([]); + }); + + it("rejects an invalid coverUrl with 400 before the profile write and the role check", async () => { + for (const role of ["missionary", "donor"]) { + const r = await patch( + { firstName: "Changed", coverUrl: own("avatars") }, + role, + ); + expect(r.status).toBe(400); + } + expect(m.writes).toEqual([]); + }); + + it("does not rewrite unchanged stored values (OQ2)", async () => { + const r = await patch({ + firstName: "Changed", + avatarUrl: STORED_AVATAR, + coverUrl: STORED_COVER, + }); + expect(r.status).toBe(200); + expect(written("profiles")).toHaveLength(1); + expect(written("profiles")[0]).not.toHaveProperty("avatar_url"); + expect(written("missionaries")).toEqual([]); + }); + + it("returns 503 with no write for a new URL when NEXT_PUBLIC_SUPABASE_URL is unset; null still clears", async () => { + vi.stubEnv("NEXT_PUBLIC_SUPABASE_URL", undefined); + const r = await patch({ firstName: "Changed", avatarUrl: own("avatars") }); + expect(r.status).toBe(503); + expect(m.writes).toEqual([]); + const cleared = await patch({ avatarUrl: null }); + expect(cleared.status).toBe(200); + expect(written("profiles")[0]).toMatchObject({ avatar_url: null }); + }); +});