fix(edge): accept never demotes, the migration splits into safe-now functions and apply-last lockdown

Round-2 security review fixes:

N1 (Important) — accept_invite could DEMOTE: invites only ever carry role
admin|member, so an owner accepting a stray member-invite went
owner->member, potentially zeroing the account's owners (an admin could
engineer this to bypass the owner-removal rule). Replaced the blind
role-overwriting upsert with a read-then-branch: after the atomic claim,
read the signer's existing membership (.ilike, case-insensitive); no row
inserts at invite.role; existing role >= invite role leaves the row
untouched (invite still consumed); invite role higher updates only the
role column. Reverts the invite to 'pending' on any write failure, same
as before.

N2 (Important) — deploy-order hard dependency: leave/remove_member call
delete_owner_guarded, which lived in the migration gated "APPLY ONLY
AFTER the edge function is live" — so those two actions would 500 for
the entire rollout window. Split the migration:
  - 20260801_membership_functions.sql: get_invite_by_token +
    delete_owner_guarded + all revokes/grants + the new unique index
    (N3). Additive only, safe to apply immediately, BEFORE the edge
    function deploys.
  - 20260802_account_membership_lockdown.sql: only the 8 drop-policy
    statements, keeping the "APPLY ONLY AFTER" gate.
Deleted the old combined 20260801_account_membership_lockdown.sql.

N3 (Minor) — added uq_account_owners_account_lower_wallet, a unique index
on (account_id, lower(wallet_address)), to the functions migration. Closes
the latent trap where the one known checksummed production row could gain
a lowercase sibling (e.g. via accept_invite's insert path) and make
.ilike()/.maybeSingle() lookups start matching 2 rows and 500 forever.
Verified zero duplicate-case pairs exist in prod today; existing data is
NOT normalized (no FK cascade from users to make that safe here).

N4 (Minor) — the ERC-1271 verifyMessage catch-all was returning 503 for
malformed signatures too, not just RPC outages. Added isWellFormedSignature
(0x + even-length hex, >=132 chars) checked before any verifier call;
malformed signatures now fail fast as 401 BAD_SIGNATURE. 503
VERIFY_UNAVAILABLE is now reserved for errors thrown by the ERC-1271 RPC
call path after shape validation already passed.

pnpm test:web stays green (271/271).

Author: MaxBrych

apps/expo/supabase/functions/org-membership/index.ts

@@ -2,11 +2,13 @@
  * Supabase Edge Function: org-membership
  *
  * The ONLY write path for org membership once
- * 20260801_account_membership_lockdown.sql is applied: creating/revoking
- * invites, accepting/declining them, leaving an org, removing a member,
- * updating account fields, and creating a new account. Also serves two
- * privileged reads (list_invites, has_pending_invite) that the lockdown
- * migration closes off from anon-key access.
+ * supabase/migrations/20260802_account_membership_lockdown.sql is applied
+ * (which itself requires 20260801_membership_functions.sql to already be
+ * live — see that migration's header): creating/revoking invites,
+ * accepting/declining them, leaving an org, removing a member, updating
+ * account fields, and creating a new account. Also serves two privileged
+ * reads (list_invites, has_pending_invite) that the lockdown migration
+ * closes off from anon-key access.
  *
  * Auth: every request carries a signed message
  * ("roebel-org-v1:<action>:<wallet>:<timestampSec>:<payloadHash>") signed
@@ -59,6 +61,14 @@ const MAX_MESSAGE_AGE_SECONDS = 300;
 const WALLET_RE = /^0x[0-9a-fA-F]{40}$/;
 const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
 const EMAIL_RE = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+// Loose shape check, not a real signature parse: 0x + even-length hex,
+// at least 65 bytes (132 chars total — a plain ECDSA sig; ERC-6492-wrapped
+// smart-account sigs are longer). Used to reject garbage BEFORE calling any
+// verifier, so a malformed signature reads as 401 BAD_SIGNATURE, never a
+// 503 that implies the RPC itself is the problem.
+const SIGNATURE_SHAPE_RE = /^0x[0-9a-fA-F]+$/;
+
+const ROLE_RANK: Record<string, number> = { owner: 3, admin: 2, member: 1 };
 
 const INVITE_ROLES = ['admin', 'member'] as const;
 const ACCOUNT_TYPES = ['personal', 'organisation'] as const;
@@ -164,6 +174,14 @@ function isHttpsUrl(v: string): boolean {
   return v.startsWith('https://');
 }
 
+function isWellFormedSignature(sig: string): boolean {
+  if (!SIGNATURE_SHAPE_RE.test(sig)) return false;
+  const hexLen = sig.length - 2; // strip '0x'
+  if (hexLen % 2 !== 0) return false;
+  if (sig.length < 132) return false;
+  return true;
+}
+
 // Mirrors apps/web/src/lib/slug.ts generateSlug / apps/expo/lib/supabase-accounts.ts
 // generateSlug byte-for-byte.
 function baseSlugify(input: string): string {
@@ -339,22 +357,53 @@ async function handleAcceptInvite(
     return fail('INVITE_GONE', 409, 'invite was already resolved by a concurrent request');
   }
 
-  // Upsert WITHOUT ignoreDuplicates: an existing member accepting a
-  // higher-role invite must have their role updated, not silently no-op'd.
-  const { error: ownerErr } = await admin.from('account_owners').upsert(
-    {
+  // Never demote. Invites only ever carry role admin|member — a blind
+  // upsert of invite.role would let an owner who mistakenly (or an admin
+  // who deliberately) re-accepts a stray member-invite get knocked down to
+  // 'member', potentially zeroing the account's owner count and letting an
+  // admin engineer a way around the owner-removal rule. Read the signer's
+  // existing membership first (case-insensitive — a checksummed row may
+  // still exist at rest) and only ever raise the role, never lower it.
+  const { data: existingRow, error: existingErr } = await admin
+    .from('account_owners')
+    .select('role')
+    .eq('account_id', invite.account_id)
+    .ilike('wallet_address', signer)
+    .maybeSingle();
+  if (existingErr) {
+    await admin.from('invite_tokens').update({ status: 'pending' }).eq('id', inviteId).eq('status', 'accepted');
+    return fail('INTERNAL', 500, existingErr.message);
+  }
+
+  if (!existingRow) {
+    const { error: insertErr } = await admin.from('account_owners').insert({
       account_id: invite.account_id,
       wallet_address: signer,
       role: invite.role,
       invited_by: invite.invited_by,
-    },
-    { onConflict: 'account_id,wallet_address' },
-  );
-  if (ownerErr) {
-    // Best-effort revert: don't leave the invite stuck 'accepted' with no
-    // membership to show for it.
-    await admin.from('invite_tokens').update({ status: 'pending' }).eq('id', inviteId).eq('status', 'accepted');
-    return fail('INTERNAL', 500, ownerErr.message);
+    });
+    if (insertErr) {
+      // Best-effort revert: don't leave the invite stuck 'accepted' with no
+      // membership to show for it.
+      await admin.from('invite_tokens').update({ status: 'pending' }).eq('id', inviteId).eq('status', 'accepted');
+      return fail('INTERNAL', 500, insertErr.message);
+    }
+  } else {
+    const existingRank = ROLE_RANK[(existingRow as { role: string }).role] ?? 0;
+    const inviteRank = ROLE_RANK[invite.role] ?? 0;
+    if (inviteRank > existingRank) {
+      const { error: updateErr } = await admin
+        .from('account_owners')
+        .update({ role: invite.role })
+        .eq('account_id', invite.account_id)
+        .ilike('wallet_address', signer);
+      if (updateErr) {
+        await admin.from('invite_tokens').update({ status: 'pending' }).eq('id', inviteId).eq('status', 'accepted');
+        return fail('INTERNAL', 500, updateErr.message);
+      }
+    }
+    // else: existing role already outranks or equals the invite role —
+    // leave it untouched. The invite is still consumed (stays 'accepted').
   }
 
   if (invite.invited_wallet) {
@@ -759,6 +808,12 @@ serve(async (req: Request) => {
   if (typeof signature !== 'string' || signature.length === 0) {
     return fail('BAD_REQUEST', 400, 'signature required');
   }
+  // Shape-check BEFORE calling any verifier: a malformed signature is a bad
+  // request, not a verifier outage — reject it as 401 here so it never
+  // reaches the try/catch below and gets misread as 503 VERIFY_UNAVAILABLE.
+  if (!isWellFormedSignature(signature)) {
+    return fail('BAD_SIGNATURE', 401, 'signature malformed');
+  }
 
   const ts = Number(timestampSec);
   const ageSec = Math.abs(Date.now() / 1000 - ts);

supabase/migrations/20260801_account_membership_lockdown.sql

@@ -0,0 +1,74 @@
+-- 20260801_membership_functions.sql
+-- Additive only — SAFE to apply immediately, BEFORE the edge function deploys.
+--
+-- Split off the previously-combined 20260801_account_membership_lockdown.sql
+-- (deploy-order fix, security review round 2): the org-membership edge
+-- function's leave/remove_member handlers call delete_owner_guarded, so
+-- that function (and get_invite_by_token, and the account_owners
+-- case-insensitive uniqueness guard below) must exist BEFORE the edge
+-- function goes live — not gated behind the same "apply only after" fence
+-- as the policy lockdown in 20260802_account_membership_lockdown.sql, which
+-- would otherwise 500 every leave/remove_member call for the entire deploy
+-- window between "edge function live" and "this migration applied".
+
+-- ── RPC: bearer-token invite lookup ─────────────────────────────────────────
+-- The ONLY anon-callable function in this pair of migrations. Knowledge of
+-- the token is the credential (bearer semantics); it takes no wallet
+-- parameter. All other membership reads/writes go through the
+-- signature-verified org-membership edge function (service role). Rule: no
+-- anon-granted function may take a wallet parameter as an authorization
+-- input — owner wallets are public in account_owners, so such a parameter
+-- is attacker-controlled.
+create or replace function public.get_invite_by_token(p_token text)
+returns invite_tokens
+language sql security definer set search_path = public, pg_temp stable as $$
+  select * from invite_tokens where token = p_token limit 1;
+$$;
+
+revoke all on function public.get_invite_by_token(text) from public;
+grant execute on function public.get_invite_by_token(text) to anon, authenticated;
+
+-- ── Guarded owner deletion ───────────────────────────────────────────────────
+-- Closes a TOCTOU race in the org-membership edge function's leave/
+-- remove_member handlers: without this, two concurrent calls against the
+-- same account could both read owner_count > 1 and both proceed, leaving
+-- the account with zero owners. `for update` locks the account's
+-- account_owners rows before counting, serializing the count-then-delete
+-- against any other concurrent call on the same account_id. service_role
+-- grant ONLY — the edge function is the only caller, never anon/authenticated
+-- (same wallet-parameter-is-attacker-controlled rule as above, but here the
+-- caller already had to pass signature verification before this runs).
+create or replace function public.delete_owner_guarded(p_account_id uuid, p_wallet text)
+returns text
+language plpgsql security definer set search_path = public, pg_temp as $$
+declare v_deleted_role text; v_owner_count int;
+begin
+  perform 1 from account_owners where account_id = p_account_id for update;
+  select role into v_deleted_role from account_owners
+    where account_id = p_account_id and lower(wallet_address) = lower(p_wallet);
+  if v_deleted_role is null then return 'not_a_member'; end if;
+  select count(*) into v_owner_count from account_owners
+    where account_id = p_account_id and role = 'owner';
+  if v_deleted_role = 'owner' and v_owner_count <= 1 then return 'last_owner'; end if;
+  delete from account_owners
+    where account_id = p_account_id and lower(wallet_address) = lower(p_wallet);
+  return 'deleted';
+end $$;
+
+revoke all on function public.delete_owner_guarded(uuid,text) from public;
+grant execute on function public.delete_owner_guarded(uuid,text) to service_role;
+
+-- ── Case-insensitive wallet uniqueness guard ─────────────────────────────────
+-- At least one production account_owners row stores a checksummed wallet
+-- address (pre-dates this lockdown's lower-casing convention). The edge
+-- function's lookups now use .ilike() to tolerate it, but that only papers
+-- over the read side — if a write path ever inserted a *lowercase* sibling
+-- row for the same (account_id, wallet) pair (e.g. accept_invite), ilike
+-- lookups (and .maybeSingle() in particular) would start matching 2 rows
+-- and 500 forever. This index makes that structurally impossible going
+-- forward. Verified zero duplicate-case pairs exist in production today, so
+-- this is safe to apply immediately. Existing data is NOT normalized —
+-- account_owners.wallet_address has no FK cascade from users that would
+-- make a case rewrite safe to do blindly here.
+create unique index if not exists uq_account_owners_account_lower_wallet
+  on account_owners (account_id, lower(wallet_address));

supabase/migrations/20260801_membership_functions.sql

@@ -0,0 +1,22 @@
+-- 20260802_account_membership_lockdown.sql
+-- Closes SECURITY_FINDINGS_2026-07-28 §1 (+ the forgeable/enumerable invite_tokens
+-- corollary) and §2. Anon-key writes on accounts/account_owners/invite_tokens are
+-- replaced by the functions in 20260801_membership_functions.sql (which must
+-- already be applied) + the signature-verified org-membership edge function
+-- (service role).
+-- ⚠️ APPLY ONLY AFTER the org-membership edge function and rewired clients are live.
+
+-- ── Policy lockdown ─────────────────────────────────────────────────────────
+-- accounts: reads stay public; every write becomes service-role/RPC only.
+drop policy if exists "accounts_insert" on accounts;                -- 005:25
+drop policy if exists "accounts_update" on accounts;                -- 005:26  (finding §2)
+drop policy if exists "accounts_delete" on accounts;                -- 20260504_accounts_delete_policy.sql:15
+
+-- account_owners: reads stay public (keystone + UI rely on it); writes locked.
+drop policy if exists "account_owners_insert" on account_owners;    -- 005:42  (finding §1)
+drop policy if exists "account_owners_delete" on account_owners;    -- 005:43
+
+-- invite_tokens: fully locked; bearer lookup goes through get_invite_by_token.
+drop policy if exists "invite_tokens_select" on invite_tokens;      -- 011:58 (enumeration hole)
+drop policy if exists "invite_tokens_insert" on invite_tokens;      -- 011:59 (forgery hole)
+drop policy if exists "invite_tokens_update" on invite_tokens;      -- 011:60