Fix backend topic correctness and /topic CORS contracts

docs/plans/phase-02-backend-correctness-and-cors.md

@@ -1,60 +1,68 @@
-# Phase 02 - Backend Correctness and CORS
+# Phase 02 Checklist - Backend Correctness and CORS
 
-Status: Planned
+## Implementation intent (coupling target)
 
-## Goal
+This checklist is intentionally constrained to the original Phase 02 intent:
 
-Fix confirmed runtime defects in topic handling, authorization checks, and CORS declarations.
+1. Fix `topicDELETE` descendant-deletion logic in `src/lib/MongodbService.ts`.
+2. Fix `topicListGET` auth validation in `src/lib/MongodbService.ts`.
+3. Align referer normalization/matching contract between:
+   - `example.env`
+   - `src/lib/backend-utilities.ts`
+   - `src/lib/MongodbService.ts` topic creation flow
+4. Fix `/topic` CORS declared methods/headers in `src/functions/topic.ts`.
 
-## Scope
+Coupling note:
+- No governance/process-only items are included in this checklist.
+- Test execution steps are handled in implementation/validation flow per `docs/norms/implementation.md`.
 
-- Fix `topicDELETE` descendant-deletion logic in `src/lib/MongodbService.ts`.
-- Fix `topicListGET` auth validation in `src/lib/MongodbService.ts`.
-- Align referer normalization/matching contract between:
-  - `example.env`
-  - `src/lib/backend-utilities.ts`
-  - `src/lib/MongodbService.ts` topic creation flow
-- Fix `/topic` CORS declared methods/headers in `src/functions/topic.ts`.
+## Checklist QC Decisions (2026-03-02)
 
-## Inputs and evidence
+1. Issue: C07 mixed two independent behaviors (allow-headers contract and preflight/non-preflight consistency), violating atomic checklist rules.
+   Decision: Split into separate C07 and C08 items.
 
-- `topicDELETE` defects:
-  - Cursor truthiness check at `src/lib/MongodbService.ts:1451`.
-  - Incomplete ID extraction at `src/lib/MongodbService.ts:1469`.
-- `topicListGET` cursor misuse at `src/lib/MongodbService.ts:1327`.
-- Referer mismatch:
-  - Protocol-including config in `example.env:17`.
-  - Protocol-stripping normalization in `src/lib/backend-utilities.ts:227`.
-  - Matching call in `src/lib/backend-utilities.ts:246` and use in `src/lib/MongodbService.ts:1048`.
-- `/topic` CORS mismatch in `src/functions/topic.ts:44` vs implemented methods at `:79`, `:93`, `:101`.
+2. Issue: C03 incorrectly depended on C01 even though topic-list auth validation does not require topic-delete changes.
+   Decision: Remove the dependency so C03 can be executed independently.
 
-## Implementation steps
+3. Issue: C04 did not explicitly define how configured patterns are normalized before match, which weakened checkability and parity with referer normalization.
+   Decision: Require both request referer and each configured allow-origin pattern to be normalized via the same helper path before picomatch comparison.
 
-1. Fix topic descendant deletion:
-   - Check existence via `await cursor.hasNext()`/`findOne`.
-   - Collect all descendant IDs (topic + replies) correctly.
-2. Fix topic list auth query:
-   - Resolve user doc (`findOne` or `next`) before checks.
-3. Decide referer contract and enforce consistently:
-   - Normalize configured origins before matching or normalize neither side.
-   - Update docs/examples to match runtime behavior.
-4. Correct `/topic` CORS method/header declarations.
-5. Add/expand backend tests for each bug path.
+## Checklist
 
-## Risk and mitigation
+- [x] C01 `[backend]` In `src/lib/MongodbService.ts`, update `topicDELETE` topic existence check to resolve a document (`findOne`/`next`) rather than relying on cursor truthiness.
+- [x] C02 `[backend]` In `src/lib/MongodbService.ts`, update `topicDELETE` descendant deletion to collect IDs from graph results as `topic.id` plus `replies[].id` before deletion. Depends on: C01.
+- [x] C03 `[backend]` In `src/lib/MongodbService.ts`, fix `topicListGET` auth lookup to resolve a user document before policy checks.
+- [x] C04 `[backend]` In `src/lib/backend-utilities.ts` and `src/lib/MongodbService.ts`, enforce one referer-matching contract by normalizing both request referer and each configured allow-origin pattern before picomatch comparison.
+- [x] C05 `[docs]` Update `example.env` comments/examples so `ALLOW_ORIGIN` documentation matches the implemented referer-matching contract. Depends on: C04.
+- [x] C06 `[backend]` In `src/functions/topic.ts`, align `Access-Control-Allow-Methods` with implemented handlers (`GET,POST,PUT,DELETE,OPTIONS`).
+- [x] C07 `[backend]` In `src/functions/topic.ts`, align `/topic` `Access-Control-Allow-Headers` with headers actually consumed for auth/topic creation flows.
+- [x] C08 `[backend]` In `src/functions/topic.ts`, keep CORS header behavior consistent across preflight and non-preflight responses by using the same header construction path for all responses. Depends on: C06, C07.
 
-- Risk: referer policy change can alter who may create topics.
-- Mitigation:
-  - Add explicit tests for allowed and denied referers.
-  - Document final pattern format and examples.
+## Behavior Slices
 
-## Acceptance criteria
+- Goal: Correct topic deletion and topic list authorization behavior in backend service paths.
+  Items: C01, C02, C03
+  Type: behavior
 
-- Confirmed defects reproduced before change and fixed after.
-- New tests cover bug scenarios and pass.
-- No regression in existing topic/comment API tests.
+- Goal: Make topic referer authorization behavior internally consistent across code and user-facing configuration docs.
+  Items: C04, C05
+  Type: behavior
 
-## Rollback
+- Goal: Align `/topic` CORS declarations with implemented route behavior.
+  Items: C06, C07, C08
+  Type: behavior
 
-- Revert phase PR.
-- If partial rollback needed, keep non-behavioral CORS header fixes separate from policy logic changes.
+## Checklist integration pass (2026-03-02)
+
+- Verified each checklist item remains directly mapped to the four implementation intents above (no added scope).
+- Verified dependency graph is coherent after QC decisions:
+  - C02 depends on C01.
+  - C05 depends on C04.
+  - C08 depends on C06 and C07.
+- Verified every checklist item belongs to exactly one behavior slice.
+
+## Checklist sanity pass (2026-03-02)
+
+- No remaining blockers in checklist design.
+- Items are atomic, imperative, checkable, and scoped per `docs/norms/checklist.md`.
+- Proceeding to implementation under `docs/norms/implementation.md`.

example.env

@@ -8,10 +8,11 @@ SIMPLE_COMMENT_MODERATOR_PASSWORD=easyToRememberHardToGuessLikeABC123
 
 JWT_SECRET=secret-key-looks-like-aXvEQ572fvOMvQQ36K2i2PE0bwEMg9qpqBWlnPhsa5OMF1vl9NyI3TxRH0DK 
 
-# ALLOW_ORIGIN holds the URLs that are allowed to fetch from and post to the Simple Comment API.
-# If browser request comes from a `referer` that is not in this list, the request will be rejected.
-# Separate multiple origins by commas, e.g. ALLOW_ORIGIN=https://blog.example.com,https//:blog.another-example.com
-# Accepts wildcard glob patterns like https://example.com/blog/*.html will match https://example.com/blog/2022-11-15.html
+# ALLOW_ORIGIN holds the allowed referer patterns for topic creation.
+# The incoming referer and configured patterns are normalized before matching:
+# protocol, query/hash, and trailing directory index are stripped.
+# Separate multiple origins by commas, e.g. ALLOW_ORIGIN=https://blog.example.com,blog.another-example.com
+# Accepts wildcard glob patterns like blog.example.com/*.html to match https://blog.example.com/2022-11-15.html
 # More information on glob patterns: https://github.com/micromatch/picomatch#globbing-features
 
 ALLOW_ORIGIN=https://blog.example.com,https://example.com

src/functions/topic.ts

@@ -41,9 +41,9 @@ const service: MongodbService = new MongodbService(
 
 const getAllowHeaders = (event: APIGatewayEvent) => {
   const allowHeaders = {
-    "Access-Control-Allow-Methods": "GET,OPTIONS",
+    "Access-Control-Allow-Methods": "GET,POST,PUT,DELETE,OPTIONS",
     "Access-Control-Allow-Credentials": "true",
-    "Access-Control-Allow-Headers": "Cookie,Referrer-Policy",
+    "Access-Control-Allow-Headers": "Cookie,Authorization",
   }
   const eventHeaders = toDefinedHeaders(event.headers)
   const allowedOriginHeaders = getAllowOriginHeaders(
@@ -101,7 +101,9 @@ export const handler = async (event: APIGatewayEvent) => {
       case "DELETE":
         return service.topicDELETE(targetId, authUserId)
       case "OPTIONS":
-        return new Promise<Success>(resolve => resolve(success200OK))
+        return new Promise<Success>(resolve =>
+          resolve({ ...success200OK, headers })
+        )
       default:
         return new Promise<Error>(resolve => resolve(error405MethodNotAllowed))
     }

src/lib/MongodbService.ts

@@ -37,6 +37,7 @@ import {
   isEmail,
   isTokenClaim,
   isUser,
+  normalizeAllowedOrigin,
   normalizeUrl,
   toAdminSafeUser,
   toPublicSafeUser,
@@ -1045,18 +1046,22 @@ export class MongodbService extends AbstractDbService {
         return error403UserNotAuthorized
       }
 
-      const isAllowed = isAllowedReferer(newTopic.referer, getAllowedOrigins())
+      const allowedOrigins = getAllowedOrigins()
+      const normalizedAllowedOrigins = allowedOrigins.map(
+        normalizeAllowedOrigin
+      )
+      const isAllowed = isAllowedReferer(newTopic.referer, allowedOrigins)
 
       if (!isAllowed) {
         const normalizedReferer = normalizeUrl(newTopic.referer)
 
-        if (getAllowedOrigins().length > 1) {
-          const body = `The referer '${normalizedReferer}' does not match any of the expected patterns. Allowed patterns: '${getAllowedOrigins().join(
+        if (normalizedAllowedOrigins.length > 1) {
+          const body = `The referer '${normalizedReferer}' does not match any of the expected patterns. Allowed patterns: '${normalizedAllowedOrigins.join(
             " or "
           )}'. Please ensure the referer matches one of the allowed patterns.`
           return { ...error403Forbidden, body }
         } else {
-          const body = `The referer '${normalizedReferer}' does not match the expected pattern: '${getAllowedOrigins().join()}'. Please ensure the referer matches the allowed pattern.`
+          const body = `The referer '${normalizedReferer}' does not match the expected pattern: '${normalizedAllowedOrigins.join()}'. Please ensure the referer matches the allowed pattern.`
           return { ...error403Forbidden, body }
         }
       }
@@ -1324,9 +1329,9 @@ export class MongodbService extends AbstractDbService {
     }
 
     const users: Collection<User> = (await this.getDb()).collection("users")
-    const authUser = await users.find({ id: authUserId }).limit(1)
+    const authUser = authUserId ? await users.findOne({ id: authUserId }) : null
 
-    if (!authUser && !policy.canPublicReadDiscussion) {
+    if (authUserId && !authUser && !policy.canPublicReadDiscussion) {
       return error404UserUnknown
     }
 
@@ -1429,9 +1434,7 @@ export class MongodbService extends AbstractDbService {
     const discussions: Collection<Comment | Topic> = (
       await this.getDb()
     ).collection("comments")
-    const cursor = await discussions.find({ id: topicId }).limit(1)
-
-    const foundTopic = await cursor.next()
+    const foundTopic = await discussions.findOne({ id: topicId })
 
     if (!foundTopic || isComment(foundTopic)) {
       return {
@@ -1444,43 +1447,44 @@ export class MongodbService extends AbstractDbService {
       return error403UserNotAuthorized
     }
 
-    // If we delete a topic that has replies it will orphan
-    // them, so first check for even one
-    const replyCursor = await discussions.find({ parentId: topicId }).limit(1)
-
-    if (replyCursor) {
-      // well, shit. delete all of the replies
-      const getReplies = id => [
-        { $match: { id } },
-        {
-          $graphLookup: {
-            from: "comments",
-            startWith: "$id",
-            connectFromField: "id",
-            connectToField: "parentId",
-            as: "replies",
-          },
+    const getReplies = id => [
+      { $match: { id } },
+      {
+        $graphLookup: {
+          from: "comments",
+          startWith: "$id",
+          connectFromField: "id",
+          connectToField: "parentId",
+          as: "replies",
         },
-      ]
+      },
+    ]
 
+    type TopicDescendants = {
+      id: TopicId
+      replies?: { id: CommentId }[]
+    }
+
+    try {
       const rawReplies = await discussions
-        .aggregate(getReplies(topicId))
+        .aggregate<TopicDescendants>(getReplies(topicId))
         .toArray()
-      const commentIds = rawReplies.map(c => c.id)
+      const commentIds = Array.from(
+        new Set(
+          rawReplies.flatMap(replyThread => [
+            replyThread.id,
+            ...(replyThread.replies ?? []).map(reply => reply.id),
+          ])
+        )
+      )
 
       await discussions.deleteMany({ id: { $in: commentIds } })
+      return success202TopicDeleted
+    } catch (e) {
+      return authUser.isAdmin
+        ? { ...error500UpdateError, body: e }
+        : error500UpdateError
     }
-
-    // entire comment can be deleted without trouble
-    // but we don't want anyone to reply in the meantime, so lock it:
-    return discussions
-      .findOneAndDelete({ id: topicId })
-      .then(() => success202TopicDeleted)
-      .catch(e => {
-        return authUser.isAdmin
-          ? { ...error500UpdateError, body: e }
-          : error500UpdateError
-      })
   }
 
   authDELETE = (): Promise<Success> => {

src/lib/backend-utilities.ts

@@ -217,7 +217,11 @@ export const addHeaders = (
 
 const getOrigin = (headers: Headers) => getHeaderValue(headers, "origin")
 
-export const getAllowedOrigins = () => allowOrigin.split(",")
+export const getAllowedOrigins = () =>
+  allowOrigin
+    .split(",")
+    .map(origin => origin.trim())
+    .filter(origin => origin.length > 0)
 
 /** Normalize a url for allowedReferer test
  * - strips hash and query parameters
@@ -232,6 +236,9 @@ export const normalizeUrl = (url: string) =>
     removeDirectoryIndex: true,
   })
 
+export const normalizeAllowedOrigin = (allowedOriginPattern: string) =>
+  normalizeUrl(allowedOriginPattern.trim())
+
 /**
  * Return true if `url` matches any of the patterns in `allowedPatterns`
  * Strips any trailing '/' from the referring url
@@ -246,7 +253,14 @@ export const normalizeUrl = (url: string) =>
 export const isAllowedReferer = (
   url: string,
   allowedPatterns: string[] = getAllowedOrigins()
-) => picomatch.isMatch(normalizeUrl(url), allowedPatterns)
+) =>
+  picomatch.isMatch(
+    normalizeUrl(url),
+    [
+      ...allowedPatterns.map(pattern => pattern.trim()),
+      ...allowedPatterns.map(normalizeAllowedOrigin),
+    ].filter(pattern => pattern.length > 0)
+  )
 
 /** Returns the proper headers for Access-Control-Allow-Origin
  * as set in .env and as determined by the Request Origin header

src/tests/backend/utilities.test.ts

@@ -225,7 +225,7 @@ describe("isAllowedReferer with advanced patterns", () => {
     expect(isAllowedReferer(url, allowedOriginArray)).toBe(false)
   })
 
-  // never use these as allowed origins
+  // normalization now allows these variants to match
   test.each([
     "https://example.com", // protocol is stripped by normalization
     "http://example.com", // protocol is stripped by normalization
@@ -234,8 +234,8 @@ describe("isAllowedReferer with advanced patterns", () => {
     "example.com/index.html", // directory index is stripped by normalization
     "example.com?foo=bar", // query parameters are stripped by normalization
     "example.com#foo", // hash is stripped by normalization
-  ])("allowed origin %s will not match", (url: string) => {
-    expect(isAllowedReferer(url, [url])).toBe(false)
+  ])("allowed origin %s should match after normalization", (url: string) => {
+    expect(isAllowedReferer(url, [url])).toBe(true)
   })
 })