From 73bead41e486ef03fc9cb85585327a51b1befab9 Mon Sep 17 00:00:00 2001 From: Rendall Date: Mon, 2 Mar 2026 19:22:27 +0200 Subject: [PATCH 1/2] Fix topic backend correctness and CORS contracts --- .../phase-02-backend-correctness-and-cors.md | 98 ++++++++++--------- example.env | 9 +- src/functions/topic.ts | 8 +- src/lib/MongodbService.ts | 80 +++++++-------- src/lib/backend-utilities.ts | 18 +++- src/tests/backend/utilities.test.ts | 6 +- 6 files changed, 123 insertions(+), 96 deletions(-) diff --git a/docs/plans/phase-02-backend-correctness-and-cors.md b/docs/plans/phase-02-backend-correctness-and-cors.md index 6c8b8aa..71b881d 100644 --- a/docs/plans/phase-02-backend-correctness-and-cors.md +++ b/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`. diff --git a/example.env b/example.env index 8fc68b3..de1dae9 100644 --- a/example.env +++ b/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 diff --git a/src/functions/topic.ts b/src/functions/topic.ts index fccea28..4c86493 100644 --- a/src/functions/topic.ts +++ b/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(resolve => resolve(success200OK)) + return new Promise(resolve => + resolve({ ...success200OK, headers }) + ) default: return new Promise(resolve => resolve(error405MethodNotAllowed)) } diff --git a/src/lib/MongodbService.ts b/src/lib/MongodbService.ts index d70eb78..fc1d579 100644 --- a/src/lib/MongodbService.ts +++ b/src/lib/MongodbService.ts @@ -37,6 +37,7 @@ import { isEmail, isTokenClaim, isUser, + normalizeAllowedOrigin, normalizeUrl, toAdminSafeUser, toPublicSafeUser, @@ -1045,18 +1046,20 @@ 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 +1327,9 @@ export class MongodbService extends AbstractDbService { } const users: Collection = (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 +1432,7 @@ export class MongodbService extends AbstractDbService { const discussions: Collection = ( 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 +1445,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(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 => { diff --git a/src/lib/backend-utilities.ts b/src/lib/backend-utilities.ts index 81aa63d..457fdf2 100644 --- a/src/lib/backend-utilities.ts +++ b/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 diff --git a/src/tests/backend/utilities.test.ts b/src/tests/backend/utilities.test.ts index 232da97..c0771e8 100644 --- a/src/tests/backend/utilities.test.ts +++ b/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) }) }) From 59a6712710707ecba895ca352f797f380f5f576a Mon Sep 17 00:00:00 2001 From: Rendall Date: Mon, 2 Mar 2026 19:32:36 +0200 Subject: [PATCH 2/2] Update format --- src/lib/MongodbService.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib/MongodbService.ts b/src/lib/MongodbService.ts index fc1d579..36ad312 100644 --- a/src/lib/MongodbService.ts +++ b/src/lib/MongodbService.ts @@ -1047,7 +1047,9 @@ export class MongodbService extends AbstractDbService { } const allowedOrigins = getAllowedOrigins() - const normalizedAllowedOrigins = allowedOrigins.map(normalizeAllowedOrigin) + const normalizedAllowedOrigins = allowedOrigins.map( + normalizeAllowedOrigin + ) const isAllowed = isAllowedReferer(newTopic.referer, allowedOrigins) if (!isAllowed) {