Follow-ups from #108 review: auth gating, PDF dedup, password validation

[rbonill]

Follow-ups from the review of #108. Each commit lands one self-contained change so they can be reviewed independently or cherry-picked.

What's in here

1. Restore admin auth on map image upload and competition PDFs (503c65b4)

• routes/api/lineMaps.js — POST /image/:map was publicRouter; back on adminRouter.
• routes/api/mazeMaps.js — the four competition-targets-pdf / scoresheet GET+POST endpoints were publicRouter; back on adminRouter.
• All call sites are in admin-only map editor pages (public/javascripts/admin/mapEditor/maze_2026.js:618, 703, 715, 1124, 1137), so this is transparent to legit callers but blocks the unauthenticated paths.
• Also collapses the duplicated cells-to-array conversion in the two POST handlers into a single normalizeMapCells helper, and switches the remaining console.log to logger.info to match the rest of the codebase.

2. Stop deduplicating cognitive targets by color code (3205c1e5)

• helper/competitionTargetsPDF.js#extractTargets keyed targetsMap by COGNITIVE_<colorCode> (and equivalent for letter victims), so two victims sharing a ring pattern collapsed into a single entry on the printable answer sheet — and because each entry carries a position-specific victimLetter, later victims also lost their letter labels.
• Switched to a flat array; every placement gets its own letter circle on the PDF. Sort order preserved.
• This is the issue Codex flagged as P2.

3. Tidy password endpoint and logging consistency (4eb5eba6)

• /api/users/me/password now type-guards current / new / confirm and tolerates a missing body, returning a clean 400 instead of a 500 on malformed input. Codex P2.
• models/user.js#updatePassword gets a one-line comment noting it relies on the pre('save') hook to re-hash. Without it the simplification reads as if the password were stored in plaintext, which would be wrong but plausible-looking enough to warrant a future cleanup PR by someone unfamiliar.
• package.json regains its trailing newline.

What's intentionally not here

These items from the review need design input or a data migration and are better handled separately:

• Cyan vs Blue cognitive ring color. The maze rules (rules/maze/1_Field.adoc:134-138) specify Blue = +2 but the generator renders Cyan; either a recolor or a K/R/Y/G/B rename + asset migration. Affects 3,225 committed PNGs.
• H/S/U/Red/Yellow/Green still in the VICTIMS enum in models/mazeMap.js:12. 2026 rules dropped them; needs a one-shot data migration on existing maps before removing.
• No validator for Cognitive victims missing rings. A pre-save check (victims.X === 'Cognitive' ⇒ cognitiveTargets.X.rings.ring1..ring5 populated) would catch silent misconfiguration.
• No tests for the score calculator changes or the score-sheet rules refactor.

Happy to follow up on any of those — wanted to keep this PR small and reviewable first.

Test plan

• Map editor in admin can still GET/POST competition-targets-pdf and scoresheet (cookie auth carries through).
• An unauthenticated request to those endpoints returns 401/redirect rather than rendering.
• An unauthenticated POST /api/maps/line/image/:map is rejected.
• PDF for a map containing two cognitive targets with the same ring pattern shows both, each with its own victim letter.
• POST /api/users/me/password with a body missing new returns 400 (was 500).
• Existing valid /me/password flows still work.

🤖 Generated with Claude Code

4. Reject Cognitive victims without populated rings (59f086af)

• models/mazeMap.js — adds a tileSchema.pre('validate') hook that refuses to save a tile whose victims.<side> is 'Cognitive' while cognitiveTargets.<side> is missing. Without this, a misconfigured map would save fine but silently award 0 kits at competition (since getVictimMaxKits returns 0 when rings are absent) while still granting SVI points.

---

Note: this branch replaces #109, which was closed automatically when the branch was renamed from rbonill/cognitive-targets-observations to rbonill/cognitive-targets-followups. Same commits, plus the new validator.

[rbonill]

For non-developer context: this PR is the cleanup follow-up to #108 (the cognitive-targets feature). Findings came out of running our /review playbook against #108 — the same review I posted here. In plain English, here's what changes and why:

• Restored access controls. Three categories of endpoints had their authentication accidentally removed during the cognitive-targets work: uploading a map background image, generating the printable cognitive-targets answer sheet, and generating the score sheet. As-is, anyone with a map ID could fetch the answer key for a live competition. They're back to admin-only — no impact on the editor (it already runs admin-authenticated), but the open door is now closed.

• Fixed the printable answer sheet. The PDF was deduplicating cognitive targets by their ring pattern, so two physical victims that happened to share the same colors collapsed into one entry — and the second victim's letter (A, B, C…) silently disappeared from the sheet referees print. Each victim now gets its own entry with its own letter circle, so referees can match printed sheet to physical victim cleanly.

• Made the password change endpoint robust. Submitting a malformed request (e.g., a missing field) returned an internal-error 500 instead of a clean validation message. Users now get a meaningful 400 response telling them what's wrong.

• Added a safety check on cognitive victims. If a map says "this position has a cognitive victim" but doesn't include the ring pattern, the database now refuses to save it instead of letting it through silently. Prevents a class of invisible misconfigurations that would only show up at competition as "this victim mysteriously gives 0 kits."

• Small cleanups. Logging consistency, restored a missing newline, factored out a small piece of duplicated code.

Nothing here changes scoring math, victim semantics, or competition behavior — it just hardens the surface around what's already working. The bigger rules-alignment items (the Cyan-vs-Blue color question for cognitive-target rings, retiring the old H/S/U/Red/Yellow/Green victim types from the schema, automated tests on the score sheet generator) are deliberately deferred to their own PRs since each needs design input or a data migration.

[mrshu]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 59f086af2c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[rbonill]

@federicomaniglio @rrrobo — these are the #108 follow-ups, with one important course-correction on top of the original PR.

The auth-scope assumption was wrong. Commit 503c65b4 was titled "Restore admin auth on map image upload and competition PDFs", but the four scoresheet/competition-PDF endpoints in routes/api/mazeMaps.js were never on adminRouter to begin with — they were intentionally public per @rrrobo's c10552c2 "support cognitive pdf from pub service" (the unauthenticated /service/editor/maze/* flow renders the same admin editor with pubService: true and POSTs map data to those endpoints to render PDFs). Moving them to adminRouter would break the public service editor with no fallback. New commit bd46e26e reverts those four to publicRouter. For lineMaps /image/:map, the original security concern (anonymous overwrite of saved maps' preview images) is real, so it stays public but now content-guards: the dummy id '000000000000000000000000' (which the public service editor uses) is allowed without auth, anything else requires admin.

The other course-correction is the new pre('save') Cognitive-victim-ring check. Originally it was a tileSchema.pre('validate') hook that drifted from the file's existing convention (all other tile-level invariants live in the parent mazeMapSchema.pre('save') loop with x/y/z coordinates in the error). It also paired badly with reality: any pre-existing prod doc already in the rejected state would become uneditable until a DBA fix. Commit 89fd8c5b folds the check into the existing pre-save loop matching the surrounding error-message format, and adds scripts/find-invalid-cognitive-victims.js — a read-only audit that exits non-zero if any docs are in the lockout state, so an operator can gate the deploy on a clean DB.

Plus a defensive guard on extractTargets for malformed rings: {} shapes that were crashing PDF generation with TypeError: NaN is not iterable, helper consolidation extended to the third byte-identical PUT /:map site, and narrative comments trimmed.

Medium-tier review pass = three independent reviewers in parallel covering correctness, maintainability, and data-edge cases. Findings folded into commits bd46e26e through bbf878a9.

No automated test suite exists in this repo, so the test plan is manual — same checklist as the original PR description, plus: (1) verify the public service editor still generates score sheets and target PDFs, (2) verify a non-admin POST to /api/maps/line/image/:realMapId returns 403, (3) run find-invalid-cognitive-victims.js against staging before deploy.