Fix comment auditing and upload rules

Recognize Firebase UIDs separately from system actors and audit callable
comment creations. Limit Storage validation to one reservation lookup
and add
the composite comments index.

Author: Llewellyn500

firestore.indexes.json

@@ -28,6 +28,15 @@
         { "fieldPath": "createdAt", "order": "DESCENDING" }
       ]
     },
+    {
+      "collectionGroup": "comments",
+      "queryScope": "COLLECTION",
+      "fields": [
+        { "fieldPath": "articleId", "order": "ASCENDING" },
+        { "fieldPath": "status", "order": "ASCENDING" },
+        { "fieldPath": "pinned", "order": "ASCENDING" }
+      ]
+    },
     {
       "collectionGroup": "commentReplies",
       "queryScope": "COLLECTION",

functions/src/index.ts

@@ -1063,11 +1063,17 @@ function stripUndefinedDeep(value: unknown): unknown {
   return value;
 }
 
+function isFirebaseUserUid(actorUid: string) {
+  // Firestore auth context has no "user" type. Client SDK writes often
+  // arrive as api_key/unknown with the Firebase UID in authId.
+  return Boolean(actorUid) && !actorUid.includes("@");
+}
+
 function isSystemAuditActor(actorUid: string, authType?: string) {
+  if (isFirebaseUserUid(actorUid)) return false;
   return (
     authType === "service_account" ||
     authType === "system" ||
-    authType === "api_key" ||
     /gserviceaccount\.com$/i.test(actorUid)
   );
 }
@@ -1203,6 +1209,9 @@ async function auditAuthenticatedClientWrite(params: {
   after: admin.firestore.DocumentData | undefined;
 }) {
   if (!params.actorUid) return;
+  // Admin SDK / scheduled jobs have no signed-in user here. Callables that
+  // act for a person log that person themselves via writeServerAuditLog.
+  if (isSystemAuditActor(params.actorUid, params.authType)) return;
   const operation = !params.before ? "create" : !params.after ? "delete" : "update";
   const snapshot = params.after || params.before;
   const actorHint =
@@ -5056,6 +5065,13 @@ export const createComment = onCall(
       }));
       if (reservationRef) transaction.delete(reservationRef);
     });
+    await writeServerAuditLog({
+      actorUid: uid,
+      action: "comment.create",
+      category: "comments",
+      details: `create comment record ${commentRef.id}`,
+      targetId: commentRef.id,
+    });
     return { success: true, commentId: commentRef.id };
   }
 );
@@ -5150,6 +5166,13 @@ export const createCommentReply = onCall(
       }));
       if (reservationRef) transaction.delete(reservationRef);
     });
+    await writeServerAuditLog({
+      actorUid: uid,
+      action: "comment_reply.create",
+      category: "comments",
+      details: `create comment_reply record ${replyRef.id}`,
+      targetId: replyRef.id,
+    });
     return { success: true, replyId: replyRef.id };
   }
 );

storage.rules

@@ -96,16 +96,20 @@ service firebase.storage {
     }
 
     match /comments/{userId}/{reservationId}/{fileName} {
+      function reservation() {
+        return firestore.get(/databases/(default)/documents/commentUploadReservations/$(reservationId)).data;
+      }
       allow get: if true;
+      // Storage rules may read at most two Firestore documents. Account checks
+      // stay on reserveCommentUploads / createComment so this rule only reads
+      // the reservation.
       allow create: if request.auth != null &&
         request.auth.uid == userId &&
-        (isActiveReaderAccount() || isStaffUser()) &&
         reservationId.matches('[a-zA-Z0-9_-]{20,64}') &&
         fileName.matches('[0-3]\\.(webp|jpg)') &&
-        firestore.exists(/databases/(default)/documents/commentUploadReservations/$(reservationId)) &&
-        firestore.get(/databases/(default)/documents/commentUploadReservations/$(reservationId)).data.uid == userId &&
-        firestore.get(/databases/(default)/documents/commentUploadReservations/$(reservationId)).data.expiresAt >= request.time &&
-        fileName in firestore.get(/databases/(default)/documents/commentUploadReservations/$(reservationId)).data.fileNames &&
+        reservation().uid == userId &&
+        reservation().expiresAt >= request.time &&
+        fileName in reservation().fileNames &&
         request.resource.size <= 2 * 1024 * 1024 &&
         request.resource.contentType.matches('image/(webp|jpeg)');
       // Attachment objects are content-addressed by a random name. Edits must