Security fixes: P0-P2 complete (22 fixes)

P0 (7): Auth-bypass removed, migrations fixed, plugin-upload disabled, RLS FORCE+WITH CHECK, plugin double-registration fixed, persistent volume, domain removed
P1 (11): User/tenant model, Redis centralized, worker separated, transactional outbox, XSS fixed, DMS chunked streaming, permissions unified, password reset, metrics secured, config/docs fixed, cross-tenant FK
P2 (4): Contact model normalized, cross-imports reduced 94%, commands+state machines for contacts/dms/mail/calendar, SPA path-traversal

8 new migrations, 99 unit tests, 13 commands, 8 contracts, 72 files changed

app/core/permissions.py

@@ -16,7 +16,7 @@ import uuid
 from typing import Any
 
 import redis.asyncio as aioredis
-from sqlalchemy import select
+from sqlalchemy import select, func
 from sqlalchemy.ext.asyncio import AsyncSession
 
 from app.config import get_settings
@@ -30,6 +30,9 @@ logger = logging.getLogger(__name__)
 CACHE_TTL = 300  # 5 minutes
 CACHE_PREFIX = "resolved"
 
+# Severity ordering for field permissions: highest wins
+_FIELD_PERM_SEVERITY = {"hidden": 3, "readonly": 2, "read": 1}
+
 
 def _matches_permission(granted: str, required: str) -> bool:
     """Check if a granted permission matches the required permission.
@@ -44,7 +47,6 @@ def _matches_permission(granted: str, required: str) -> bool:
         return True
     g_parts = granted.split(":")
     r_parts = required.split(":")
-    # Wildcard * matches any single segment, but remaining segments must still match
     if len(g_parts) != len(r_parts):
         return False
     for i, g_part in enumerate(g_parts):
@@ -88,6 +90,87 @@ def _normalize_permissions(permissions: Any) -> set[str]:
     return result
 
 
+def _merge_field_permissions(
+    existing: dict[str, dict[str, str]],
+    incoming: dict[str, Any],
+) -> None:
+    """Merge incoming field permissions into existing dict.
+
+    Uses 'strictest right wins': hidden > readonly > read.
+    When a field already exists, the more restrictive (higher severity) value wins.
+    """
+    for module, fields in incoming.items():
+        if not isinstance(fields, dict):
+            continue
+        if module not in existing:
+            existing[module] = {}
+        for field, perm in fields.items():
+            if not isinstance(perm, str):
+                continue
+            perm_lower = perm.lower()
+            if perm_lower not in _FIELD_PERM_SEVERITY:
+                # Unknown permission level — skip with warning
+                logger.warning(
+                    "Unknown field permission level '%s' for %s.%s — skipping",
+                    perm, module, field,
+                )
+                continue
+            current = existing[module].get(field)
+            if current is None:
+                existing[module][field] = perm_lower
+            else:
+                # Strictest (highest severity) wins
+                if _FIELD_PERM_SEVERITY[perm_lower] > _FIELD_PERM_SEVERITY.get(current, 0):
+                    existing[module][field] = perm_lower
+
+
+async def _get_current_permission_version(
+    db: AsyncSession,
+    user_id: uuid.UUID,
+    tenant_id: uuid.UUID,
+) -> int:
+    """Get the current max permission_version from DB for cache validation.
+
+    Uses a SAVEPOINT so that a failure here does not abort the outer transaction.
+    """
+    async with db.begin_nested():
+        # Check role version
+        ut_q = select(UserTenant.role_id).where(
+            UserTenant.user_id == user_id,
+            UserTenant.tenant_id == tenant_id,
+        )
+        ut_result = await db.execute(ut_q)
+        role_id = ut_result.scalar_one_or_none()
+
+        max_version = 0
+        if role_id is not None:
+            role_q = select(Role.permission_version).where(Role.id == role_id)
+            role_result = await db.execute(role_q)
+            role_ver = role_result.scalar_one_or_none()
+            if role_ver is not None:
+                max_version = max(max_version, role_ver)
+
+        # Check group versions
+        ug_q = select(UserGroup.group_id).where(
+            UserGroup.user_id == user_id,
+            UserGroup.tenant_id == tenant_id,
+        )
+        ug_result = await db.execute(ug_q)
+        group_ids = [row[0] for row in ug_result.all()]
+
+        if group_ids:
+            groups_q = select(func.max(Group.permission_version)).where(
+                Group.id.in_(group_ids),
+                Group.deleted_at.is_(None),
+            )
+            groups_result = await db.execute(groups_q)
+            group_max = groups_result.scalar()
+            if group_max is not None:
+                max_version = max(max_version, group_max)
+
+        return max_version
+
+
 async def resolve_permissions(
     db: AsyncSession,
     user_id: uuid.UUID,
@@ -104,18 +187,24 @@ async def resolve_permissions(
             "version": int,                # permission_version for cache invalidation
         }
     """
-    # Check system admin first
-    # If a previous query in this session failed, the transaction may be aborted.
-    # Rollback to recover before executing our query.
+    # Use SAVEPOINT for the initial query so a failure doesn't abort
+    # the outer transaction.
     try:
-        user_q = select(User.is_system_admin).where(User.id == user_id)
-        user_result = await db.execute(user_q)
-        is_system_admin = user_result.scalar() or False
+        async with db.begin_nested():
+            user_q = select(User.is_system_admin).where(User.id == user_id)
+            user_result = await db.execute(user_q)
+            is_system_admin = user_result.scalar() or False
     except Exception:
-        await db.rollback()
-        user_q = select(User.is_system_admin).where(User.id == user_id)
-        user_result = await db.execute(user_q)
-        is_system_admin = user_result.scalar() or False
+        logger.warning(
+            "SAVEPOINT failed for is_system_admin query (user=%s), retrying without savepoint",
+            user_id,
+            exc_info=True,
+        )
+        # Last-resort fallback: still use savepoint to isolate
+        async with db.begin_nested():
+            user_q = select(User.is_system_admin).where(User.id == user_id)
+            user_result = await db.execute(user_q)
+            is_system_admin = user_result.scalar() or False
 
     if is_system_admin:
         return {
@@ -126,13 +215,14 @@ async def resolve_permissions(
             "version": 0,  # system admin doesn't need version tracking
         }
 
-    # Load UserTenant to get role_id
-    ut_q = select(UserTenant).where(
-        UserTenant.user_id == user_id,
-        UserTenant.tenant_id == tenant_id,
-    )
-    ut_result = await db.execute(ut_q)
-    user_tenant = ut_result.scalar_one_or_none()
+    # Load UserTenant to get role_id — use SAVEPOINT
+    async with db.begin_nested():
+        ut_q = select(UserTenant).where(
+            UserTenant.user_id == user_id,
+            UserTenant.tenant_id == tenant_id,
+        )
+        ut_result = await db.execute(ut_q)
+        user_tenant = ut_result.scalar_one_or_none()
 
     allowed: set[str] = set()
     denied: set[str] = set()
@@ -141,72 +231,75 @@ async def resolve_permissions(
 
     # Load role permissions
     if user_tenant and user_tenant.role_id:
-        role_q = select(Role).where(Role.id == user_tenant.role_id)
-        role_result = await db.execute(role_q)
-        role = role_result.scalar_one_or_none()
+        async with db.begin_nested():
+            role_q = select(Role).where(Role.id == user_tenant.role_id)
+            role_result = await db.execute(role_q)
+            role = role_result.scalar_one_or_none()
+
         if role:
             allowed |= _normalize_permissions(role.permissions)
             denied |= _normalize_permissions(role.denied_permissions)
-            max_version = max(max_version, role.permission_version)
-            # Merge field permissions
+            max_version = max(max_version, role.permission_version or 0)
+            # Merge field permissions using strictest-wins
             if role.field_permissions:
-                for module, fields in role.field_permissions.items():
-                    if isinstance(fields, dict):
-                        if module not in field_perms:
-                            field_perms[module] = {}
-                        field_perms[module].update(fields)
+                _merge_field_permissions(field_perms, role.field_permissions)
+
+    # Also check built-in role string on UserTenant for backward compatibility
+    if user_tenant is not None and user_tenant.role_id is None:
+        legacy_role = user_tenant.role
 
-    # Also check legacy role string on User for backward compatibility
-    if user_tenant is None or user_tenant.role_id is None:
-        legacy_q = select(User.role).where(User.id == user_id)
-        legacy_result = await db.execute(legacy_q)
-        legacy_role = legacy_result.scalar_one_or_none()
         if legacy_role == "admin":
             allowed.add("*:*")
         elif legacy_role == "editor":
-            allowed |= {"contacts:read", "contacts:write", "contacts:read", "contacts:write",
-                        "users:read", "roles:read", "audit:read", "attachments:read",
-                        "attachments:write", "workflows:read", "workflows:write",
-                        "sequences:read", "sequences:write", "addresses:read", "addresses:write",
-                        "taxes:read", "taxes:write", "currencies:read", "currencies:write",
-                        "notifications:read", "notifications:write", "import_export:read",
-                        "import_export:write",
-                        "user_preferences:read", "user_preferences:write"}
+            allowed |= {
+                "contacts:read", "contacts:write",
+                "users:read", "roles:read", "audit:read",
+                "attachments:read", "attachments:write",
+                "workflows:read", "workflows:write",
+                "sequences:read", "sequences:write",
+                "addresses:read", "addresses:write",
+                "taxes:read", "taxes:write",
+                "currencies:read", "currencies:write",
+                "notifications:read", "notifications:write",
+                "import_export:read", "import_export:write",
+                "user_preferences:read", "user_preferences:write",
+            }
         elif legacy_role == "viewer":
-            allowed |= {"contacts:read", "contacts:read", "users:read", "roles:read",
-                        "audit:read", "attachments:read", "workflows:read", "sequences:read",
-                        "addresses:read", "taxes:read", "currencies:read",
-                        "notifications:read", "import_export:read",
-                        "user_preferences:read", "user_preferences:write"}
+            allowed |= {
+                "contacts:read", "users:read", "roles:read",
+                "audit:read", "attachments:read", "workflows:read",
+                "sequences:read", "addresses:read", "taxes:read",
+                "currencies:read", "notifications:read",
+                "import_export:read",
+                "user_preferences:read", "user_preferences:write",
+            }
 
     # Load group permissions
-    ug_q = select(UserGroup).where(
-        UserGroup.user_id == user_id,
-        UserGroup.tenant_id == tenant_id,
-    )
-    ug_result = await db.execute(ug_q)
-    user_groups = ug_result.scalars().all()
+    async with db.begin_nested():
+        ug_q = select(UserGroup).where(
+            UserGroup.user_id == user_id,
+            UserGroup.tenant_id == tenant_id,
+        )
+        ug_result = await db.execute(ug_q)
+        user_groups = ug_result.scalars().all()
 
     if user_groups:
         group_ids = [ug.group_id for ug in user_groups]
-        groups_q = select(Group).where(
-            Group.id.in_(group_ids),
-            Group.deleted_at.is_(None),
-        )
-        groups_result = await db.execute(groups_q)
-        groups = groups_result.scalars().all()
+        async with db.begin_nested():
+            groups_q = select(Group).where(
+                Group.id.in_(group_ids),
+                Group.deleted_at.is_(None),
+            )
+            groups_result = await db.execute(groups_q)
+            groups = groups_result.scalars().all()
 
         for group in groups:
             allowed |= _normalize_permissions(group.permissions)
             denied |= _normalize_permissions(group.denied_permissions)
-            max_version = max(max_version, group.permission_version)
-            # Merge field permissions
+            max_version = max(max_version, group.permission_version or 0)
+            # Merge field permissions using strictest-wins
             if group.field_permissions:
-                for module, fields in group.field_permissions.items():
-                    if isinstance(fields, dict):
-                        if module not in field_perms:
-                            field_perms[module] = {}
-                        field_perms[module].update(fields)
+                _merge_field_permissions(field_perms, group.field_permissions)
 
     # Apply deny list
     resolved = allowed - denied
@@ -226,15 +319,42 @@ async def get_cached_permissions(
     user_id: uuid.UUID,
     tenant_id: uuid.UUID,
 ) -> dict[str, Any]:
-    """Get resolved permissions from Redis cache or resolve from DB."""
+    """Get resolved permissions from Redis cache or resolve from DB.
+
+    Validates the cached permission_version against the current DB version.
+    If they differ, the cache entry is stale and will be re-resolved.
+    """
     cache_key = f"{CACHE_PREFIX}:{user_id}:{tenant_id}"
 
     raw = await redis.get(cache_key)
     if raw is not None:
         data = json.loads(raw)
-        return data
+        cached_version = data.get("version", -1)
 
-    # Cache miss — resolve from DB
+        # Validate cached version against current DB version
+        try:
+            current_version = await _get_current_permission_version(db, user_id, tenant_id)
+        except Exception:
+            logger.warning(
+                "Failed to query current permission_version for cache validation "
+                "(user=%s, tenant=%s) — using cached data",
+                user_id, tenant_id,
+                exc_info=True,
+            )
+            current_version = cached_version  # assume cache is valid if we can't check
+
+        if cached_version == current_version:
+            return data
+
+        # Version mismatch — invalidate stale cache and re-resolve
+        logger.info(
+            "Permission cache version mismatch for user=%s tenant=%s "
+            "(cached=%s, current=%s) — re-resolving",
+            user_id, tenant_id, cached_version, current_version,
+        )
+        await redis.delete(cache_key)
+
+    # Cache miss or stale — resolve from DB
     resolved = await resolve_permissions(db, user_id, tenant_id)
 
     # Store in cache (convert sets to lists for JSON)
@@ -263,11 +383,33 @@ async def invalidate_all_user_permissions(
     redis: aioredis.Redis,
     tenant_id: uuid.UUID,
 ) -> None:
-    """Invalidate permission cache for all users in a tenant (e.g. after role/group change)."""
+    """Invalidate permission cache for all users in a tenant (e.g. after role/group change).
+
+    Uses SCAN (non-blocking) instead of KEYS to avoid blocking Redis.
+    """
     pattern = f"{CACHE_PREFIX}:*:{tenant_id}"
-    keys = await redis.keys(pattern)
-    if keys:
-        await redis.delete(*keys)
+    batch_size = 200
+    cursor: int | bytes | str = 0
+    deleted_count = 0
+
+    while True:
+        cursor, keys = await redis.scan(
+            cursor=cursor,
+            match=pattern,
+            count=batch_size,
+        )
+        if keys:
+            await redis.delete(*keys)
+            deleted_count += len(keys)
+        # SCAN returns cursor as bytes or int depending on redis-py version
+        cursor_int = int(cursor) if cursor else 0
+        if cursor_int == 0:
+            break
+
+    logger.info(
+        "Invalidated %d permission cache entries for tenant=%s",
+        deleted_count, tenant_id,
+    )
 
 
 def check_permission(resolved: dict[str, Any], required: str) -> bool: