From 5baada08ef05329ca18e61a5326fd136a0dd230d Mon Sep 17 00:00:00 2001
From: wtclaude <claude@whitlocktech.net>
Date: Tue, 18 Aug 2026 09:54:05 -0500
Subject: [PATCH] fix(teams): four defects the live rig found in the forum
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

None of these could fail a unit test, and three of them break the feature for the
operator rather than for the code.

**The uploads acknowledgement was a one-way door.** A settings form sends every
field it owns, so once `teams_forum_images` was `uploads`, every later save
re-sent `uploads` — and the gate fired on the VALUE being present rather than on
the mode being SELECTED. The operator could never change a forum setting again,
and the thing they would reach for in a hurry, switching the forum off, was
exactly what came back 400. The gate now passes when an acknowledgement for the
version in force is already on record AND uploads is already the stored mode:
there is no new consent to take. A transition INTO uploads still asks, and a
reworded notice is still caught by assertSettingsWritable.

**An uploaded image could never become a picture.** `uploads` mode hands the
composer `/uploads/<name>.png`, the composer puts it in the body as text — the
author never writes markup, which is the whole design — and the renderer only
rewrites ANCHORS. The linkifier matched absolute http(s) URLs only, so the write
path could not produce the anchor the read path looks for, even though
`isEmbeddableImageUrl` had accepted those paths since the first commit. The two
halves disagreed and only a real upload showed it.

**The embed sat beside its link, not beneath it**, because an <img> is inline, and
nothing capped a remote image to the column — one post from a host serving a
4000px file would have blown the layout out. Core now emits `class="forum-embed"`
and the stylesheet owns both. A class rather than an inline style because the
style would then have to survive the client's DOMPurify pass, and its CSS
sanitiser is a larger thing to reason about than one class name.

**The panel's buttons had no button styling.** `btn-ghost` is a MODIFIER — every
other call site in this codebase pairs it with the base `btn` — so alone it
contributed colours and no geometry, and the controls rendered as bare boxes.
Small inline actions use `pill`, which is what the rest of the admin surface uses
for exactly these. Same class of mistake as the Material one in the Android M12
phase: the modifier carries no base.

Also: the post body now re-sanitises client-side like every other body-HTML
surface on this site, with `ADD_ATTR: ['referrerpolicy']`. That argument is
load-bearing — DOMPurify's default allowlist carries `loading` but not
`referrerpolicy`, so a plain sanitize() call silently strips the one attribute
limiting what a remote embed leaks to the host serving it, which is the privacy
property the admin help text promises.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 client/src/modules/TeamForumPanel.jsx         | 50 +++++++++-------
 client/src/styles/theme.css                   | 12 ++++
 .../model/teams/teamForumSettings.model.js    | 33 +++++++---
 .../src/router/v1/admin/admin.controller.js   |  2 +-
 server/src/utils/forumHtml.js                 | 24 +++++++-
 server/test/teamForum.test.js                 | 60 ++++++++++++++++---
 6 files changed, 141 insertions(+), 40 deletions(-)

diff --git a/client/src/modules/TeamForumPanel.jsx b/client/src/modules/TeamForumPanel.jsx
index d961654..a823bdb 100644
--- a/client/src/modules/TeamForumPanel.jsx
+++ b/client/src/modules/TeamForumPanel.jsx
@@ -1,5 +1,6 @@
 import { useCallback, useEffect, useState } from 'react'
 import { useSearchParams } from 'react-router-dom'
+import DOMPurify from 'dompurify'
 import { api } from '../api/client.js'
 import { useAuth } from '../contexts/AuthContext.jsx'
 import { useSite } from '../contexts/SiteContext.jsx'
@@ -118,7 +119,7 @@ export default function TeamForumPanel({ externalId, moduleId }) {
           Announcements
         </h2>
         {forum.canPost && !composing && (
-          <button type="button" className="btn-ghost sans" onClick={() => setComposing(true)}>
+          <button type="button" className="pill" onClick={() => setComposing(true)}>
             Post an announcement
           </button>
         )}
@@ -225,7 +226,7 @@ function GuestManager({ slug }) {
 
   if (!open) {
     return (
-      <button type="button" className="btn-ghost sans" onClick={() => setOpen(true)} style={{ marginTop: 10 }}>
+      <button type="button" className="pill" onClick={() => setOpen(true)} style={{ marginTop: 10 }}>
         Forum guests
       </button>
     )
@@ -235,7 +236,7 @@ function GuestManager({ slug }) {
     <section style={{ marginTop: 12, padding: 12, border: '1px solid var(--rule, #ccc)', borderRadius: 6 }}>
       <header style={{ display: 'flex', justifyContent: 'space-between', alignItems: 'baseline' }}>
         <h3 className="sans" style={{ margin: 0, fontSize: '0.95rem' }}>Forum guests</h3>
-        <button type="button" className="btn-ghost sans" onClick={() => setOpen(false)}>Close</button>
+        <button type="button" className="pill" onClick={() => setOpen(false)}>Close</button>
       </header>
       <p className="sans dim" style={{ fontSize: '0.8rem', margin: '6px 0 10px' }}>
         Guests read and post in this forum without being members of the Team. They do not appear on the
@@ -247,7 +248,7 @@ function GuestManager({ slug }) {
         {(data?.guests || []).map((g) => (
           <li key={g.userId} className="sans" style={{ fontSize: '0.88rem', display: 'flex', gap: 8 }}>
             <span>{g.username}</span>
-            <button type="button" className="btn-ghost sans" onClick={() => revoke(g.userId)}>Remove</button>
+            <button type="button" className="pill" onClick={() => revoke(g.userId)}>Remove</button>
           </li>
         ))}
         {data && data.guests.length === 0 && (
@@ -257,14 +258,14 @@ function GuestManager({ slug }) {
 
       <form onSubmit={add} style={{ display: 'flex', gap: 8 }}>
         <input
-          className="sans"
+          className="input"
           value={username}
           onChange={(e) => setUsername(e.target.value)}
           placeholder="Account name"
           maxLength={32}
           required
         />
-        <button type="submit" className="btn sans">Add</button>
+        <button type="submit" className="btn btn-primary btn-sq">Add</button>
       </form>
       {error && <p className="sans" style={{ color: 'var(--danger, crimson)', fontSize: '0.85rem' }}>{error}</p>}
     </section>
@@ -274,7 +275,7 @@ function GuestManager({ slug }) {
 function ThreadView({ thread, canModerate, onBack, onModerate }) {
   return (
     <section style={{ marginTop: 26 }}>
-      <button type="button" className="btn-ghost sans" onClick={onBack} style={{ marginBottom: 10 }}>
+      <button type="button" className="pill" onClick={onBack} style={{ marginBottom: 10 }}>
         ← All announcements
       </button>
       <h2 className="display" style={{ fontSize: '1.15rem', color: 'var(--head)', margin: '0 0 4px' }}>
@@ -288,24 +289,33 @@ function ThreadView({ thread, canModerate, onBack, onModerate }) {
       {thread.posts.map((post) => (
         <article key={post.id} style={{ marginBottom: 16 }}>
           {/*
-            Rendered server-side under the operator's image policy, which is why
-            this is dangerouslySetInnerHTML and not a sanitizer call here. The body
-            was sanitised on write with the forum's own profile — one in which
-            `img` is never allowed — and any <img> in it was emitted by core's own
-            renderer with a fixed attribute set. A client-side sanitiser would have
-            to strip exactly the tag core just decided to add.
+            Sanitised on write with the forum's own profile, rendered server-side
+            under the operator's image policy, and re-sanitised here — the same
+            defence-in-depth every other body-HTML surface on this site applies
+            (FiveOnFriday, NewsletterIssue, the rich-text block).
+
+            `ADD_ATTR: ['referrerpolicy']` is load-bearing and not a preference.
+            DOMPurify's default allowlist carries `loading` but NOT
+            `referrerpolicy`, so a plain sanitize() call silently strips the one
+            attribute that limits what a remote embed leaks to the host serving it
+            — the privacy property the admin help text promises an operator. The
+            <img> itself is core's own output with a fixed attribute set, so
+            nothing here is widening what an author can write.
           */}
           {/* eslint-disable-next-line react/no-danger */}
-          <div className="serif" dangerouslySetInnerHTML={{ __html: post.body }} />
+          <div
+            className="prose"
+            dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(post.body || '', { ADD_ATTR: ['referrerpolicy'] }) }}
+          />
         </article>
       ))}
 
       {canModerate && (
         <div style={{ display: 'flex', gap: 8, marginTop: 12 }}>
-          <button type="button" className="btn-ghost sans" onClick={() => onModerate(thread.pinned ? 'unpin' : 'pin')}>
+          <button type="button" className="pill" onClick={() => onModerate(thread.pinned ? 'unpin' : 'pin')}>
             {thread.pinned ? 'Unpin' : 'Pin'}
           </button>
-          <button type="button" className="btn-ghost sans" onClick={() => onModerate(thread.status === 'hidden' ? 'unhide' : 'hide')}>
+          <button type="button" className="pill" onClick={() => onModerate(thread.status === 'hidden' ? 'unhide' : 'hide')}>
             {thread.status === 'hidden' ? 'Unhide' : 'Hide'}
           </button>
         </div>
@@ -352,7 +362,7 @@ function Composer({ slug, imageMode, onCancel, onPosted }) {
   return (
     <form onSubmit={submit} style={{ display: 'grid', gap: 8, marginTop: 12 }}>
       <input
-        className="sans"
+        className="input"
         value={title}
         onChange={(e) => setTitle(e.target.value)}
         placeholder="Title"
@@ -360,7 +370,7 @@ function Composer({ slug, imageMode, onCancel, onPosted }) {
         required
       />
       <textarea
-        className="sans"
+        className="textarea"
         value={body}
         onChange={(e) => setBody(e.target.value)}
         placeholder="Write your announcement. Paste an image URL on its own line to share a picture."
@@ -374,8 +384,8 @@ function Composer({ slug, imageMode, onCancel, onPosted }) {
       )}
       {error && <p className="sans" style={{ color: 'var(--danger, crimson)', fontSize: '0.85rem' }}>{error}</p>}
       <div style={{ display: 'flex', gap: 8 }}>
-        <button type="submit" className="btn sans" disabled={busy}>Post</button>
-        <button type="button" className="btn-ghost sans" onClick={onCancel}>Cancel</button>
+        <button type="submit" className="btn btn-primary btn-sq" disabled={busy}>Post</button>
+        <button type="button" className="pill" onClick={onCancel}>Cancel</button>
       </div>
     </form>
   )
diff --git a/client/src/styles/theme.css b/client/src/styles/theme.css
index dd24ae3..62c4cf3 100644
--- a/client/src/styles/theme.css
+++ b/client/src/styles/theme.css
@@ -298,6 +298,18 @@ button[disabled] {
 }
 
 /* ===== Rich prose (wiki / newsletter body) ===== */
+.forum-embed {
+  /* The image a Team-forum post's URL renders as, in `remote`/`uploads` mode.
+     Emitted by the server (utils/forumHtml.js), never by an author — which is
+     what makes the operator's image policy enforceable. Block, so it sits
+     beneath its link rather than beside it; capped, because a remote image is
+     whatever size its host decided and one post must not blow out the column. */
+  display: block;
+  margin-top: 8px;
+  max-width: 100%;
+  height: auto;
+  border-radius: var(--radius-input);
+}
 .prose {
   color: var(--text);
   font-size: 1.06rem;
diff --git a/server/src/model/teams/teamForumSettings.model.js b/server/src/model/teams/teamForumSettings.model.js
index b3d69dc..044c183 100644
--- a/server/src/model/teams/teamForumSettings.model.js
+++ b/server/src/model/teams/teamForumSettings.model.js
@@ -91,16 +91,33 @@ async function ackState() {
  * Returns `{ ok }` or `{ ok: false, error, status }`, matching the model result
  * shape the Teams controllers already translate.
  */
-function assertAcknowledged(nextMode, acknowledge) {
+async function assertAcknowledged(nextMode, acknowledge) {
   if (nextMode !== 'uploads') return { ok: true }
-  if (String(acknowledge ?? '') !== ACK_VERSION) {
-    return {
-      ok: false,
-      status: 400,
-      error: `Enabling uploads requires acknowledging the current notice (version ${ACK_VERSION}).`,
-    }
+  if (String(acknowledge ?? '') === ACK_VERSION) return { ok: true }
+
+  // **The gate is on SELECTING uploads, not on the value being present.**
+  //
+  // A settings form sends every field it owns, so once uploads is on, every later
+  // save re-sends `uploads` — turning the forum off, switching back to `remote`,
+  // any of it. Demanding a fresh acknowledgement for those would make the mode a
+  // one-way door: the operator could never change a forum setting again, and the
+  // one thing they would most want to do in a hurry (switch the forum off) would
+  // be the thing refused. Found on the live rig, where unticking "Enable Team
+  // forums" came back 400.
+  //
+  // So an acknowledgement already ON RECORD, for the version in force, while
+  // uploads is ALREADY the stored mode, is what this request needs — there is no
+  // new consent to take. A transition INTO uploads still needs the checkbox, and
+  // a stale acknowledgement is caught by assertSettingsWritable, which is the
+  // separate rule for a reworded notice.
+  const [state, current] = await Promise.all([ackState(), imageMode()])
+  if (current === 'uploads' && state.given && !state.stale) return { ok: true }
+
+  return {
+    ok: false,
+    status: 400,
+    error: `Enabling uploads requires acknowledging the current notice (version ${ACK_VERSION}).`,
   }
-  return { ok: true }
 }
 
 /**
diff --git a/server/src/router/v1/admin/admin.controller.js b/server/src/router/v1/admin/admin.controller.js
index 4c6602e..cec162f 100644
--- a/server/src/router/v1/admin/admin.controller.js
+++ b/server/src/router/v1/admin/admin.controller.js
@@ -609,7 +609,7 @@ async function updateSettings(req, res) {
     }
     // THE GATE (§5.5.5). Server-side, and rejected 400 with the admin UI's
     // checkbox bypassed — a checkbox is how the gate is presented, never the gate.
-    const gate = forumSettings.assertAcknowledged(nextImageMode, req.body.acknowledge)
+    const gate = await forumSettings.assertAcknowledged(nextImageMode, req.body.acknowledge)
     if (!gate.ok) return res.status(gate.status).json({ message: gate.error })
   }
   {
diff --git a/server/src/utils/forumHtml.js b/server/src/utils/forumHtml.js
index d6e0b4c..9047d7d 100644
--- a/server/src/utils/forumHtml.js
+++ b/server/src/utils/forumHtml.js
@@ -82,7 +82,19 @@ const IMAGE_EXTENSIONS = ['.png', '.jpg', '.jpeg', '.gif', '.webp', '.avif']
 // being shown, not offered.
 const NO_LINKIFY = new Set(['a', 'code', 'pre'])
 
-const BARE_URL = /\bhttps?:\/\/[^\s<>"']+/g
+// Absolute http(s) URLs, and root-relative `/uploads/…` paths.
+//
+// The second alternative is not a nicety. In `uploads` mode the composer hands
+// the author a path like `/uploads/1787…-ab12.png`, puts it in the body as TEXT
+// (the author never writes markup — that is the whole design), and the renderer
+// only ever rewrites ANCHORS. Without this branch the write path cannot produce
+// the anchor the read path looks for, so an uploaded image could never become a
+// picture — even though `isEmbeddableImageUrl` was written to accept exactly
+// these paths. The two halves disagreed, and only a real upload showed it.
+//
+// Deliberately narrow: `/uploads/` and nothing else, so ordinary prose that
+// happens to contain a slash is left alone.
+const BARE_URL = /\bhttps?:\/\/[^\s<>"']+|(?:^|(?<=[\s(]))\/uploads\/[A-Za-z0-9._~-]+(?:\/[A-Za-z0-9._~-]+)*/g
 
 /**
  * Sanitise a forum post body. Runs on WRITE; the stored value is already safe and
@@ -180,7 +192,15 @@ function renderForumBody(storedHtml, mode) {
     // A fixed attribute set, every time. `no-referrer` limits what leaks to the
     // third-party host — it cannot prevent the request itself, which is the
     // privacy cost stated in the admin help text rather than hidden.
-    return `${anchor}<img src="${href}" loading="lazy" referrerpolicy="no-referrer" alt="">`
+    //
+    // `class` rather than an inline style, for two things the live rig showed:
+    // the embed has to sit BENEATH the link (§5.5.3) and an <img> is inline, so
+    // without it the picture lands beside the URL; and a remote image is any size
+    // its host chooses, so it needs a max-width or one post can blow the column
+    // out. Both live in core's stylesheet (`.forum-embed`) because a style
+    // attribute would then have to survive the client's DOMPurify pass, and its
+    // CSS sanitiser is a larger thing to reason about than one class name.
+    return `${anchor}<img class="forum-embed" src="${href}" loading="lazy" referrerpolicy="no-referrer" alt="">`
   })
 }
 
diff --git a/server/test/teamForum.test.js b/server/test/teamForum.test.js
index fea740d..da8914f 100644
--- a/server/test/teamForum.test.js
+++ b/server/test/teamForum.test.js
@@ -67,23 +67,45 @@ test('an unexpected stored image mode reads as disabled rather than as itself',
 
 // ── the acknowledgement gate (§5.5.5) ──────────────────────────────────────
 
-test('acceptance 4: uploads mode is rejected without a matching acknowledgement', () => {
+test('acceptance 4: uploads mode is rejected without a matching acknowledgement', async () => {
   // Server-side, with the admin UI's checkbox bypassed — a checkbox is how the
-  // gate is presented and never the gate.
-  const refused = forumSettings.assertAcknowledged('uploads', undefined)
+  // gate is presented and never the gate. Nothing is on record and the stored
+  // mode is not uploads, so this is a genuine transition INTO it.
+  const refused = await forumSettings.assertAcknowledged('uploads', undefined)
   assert.equal(refused.ok, false)
   assert.equal(refused.status, 400)
 
   // A STALE version is not an acknowledgement either.
-  assert.equal(forumSettings.assertAcknowledged('uploads', '0').ok, false)
-  assert.equal(forumSettings.assertAcknowledged('uploads', forumSettings.ACK_VERSION).ok, true)
+  assert.equal((await forumSettings.assertAcknowledged('uploads', '0')).ok, false)
+  assert.equal((await forumSettings.assertAcknowledged('uploads', forumSettings.ACK_VERSION)).ok, true)
 })
 
-test('the other two image modes need no acknowledgement', () => {
+test('the other two image modes need no acknowledgement', async () => {
   // `remote` gets a non-blocking advisory instead: nothing comes to rest on the
   // operator's disk, which is the thing the acknowledgement is about.
-  assert.equal(forumSettings.assertAcknowledged('remote', undefined).ok, true)
-  assert.equal(forumSettings.assertAcknowledged('disabled', undefined).ok, true)
+  assert.equal((await forumSettings.assertAcknowledged('remote', undefined)).ok, true)
+  assert.equal((await forumSettings.assertAcknowledged('disabled', undefined)).ok, true)
+})
+
+test('uploads is not a one-way door — a later save needs no fresh acknowledgement', async () => {
+  // Found on the live rig. A settings form sends every field it owns, so with
+  // uploads on, unticking "Enable Team forums" re-sends `uploads` and came back
+  // 400 — the operator could never change a forum setting again, least of all the
+  // one they would reach for in a hurry.
+  store.teams_forum_images = 'uploads'
+  store.teams_forum_uploads_ack = forumSettings.ACK_VERSION
+
+  assert.equal((await forumSettings.assertAcknowledged('uploads', undefined)).ok, true)
+
+  // Still a gate where consent is genuinely absent: a stale acknowledgement means
+  // the wording moved, and that DOES need re-consent.
+  store.teams_forum_uploads_ack = '0'
+  assert.equal((await forumSettings.assertAcknowledged('uploads', undefined)).ok, false)
+
+  // And a transition INTO uploads from another mode still asks.
+  store.teams_forum_images = 'remote'
+  store.teams_forum_uploads_ack = forumSettings.ACK_VERSION
+  assert.equal((await forumSettings.assertAcknowledged('uploads', undefined)).ok, false)
 })
 
 test('a reworded notice freezes forum settings but does NOT disable uploads', async () => {
@@ -127,9 +149,12 @@ test('acceptance 3: the stored HTML is identical in every image mode', () => {
   const remote = renderForumBody(stored, 'remote')
 
   assert.equal(disabled, stored)                       // byte-for-byte
-  assert.match(remote, /<img src="https:\/\/example\.com\/banner\.png"/)
+  assert.match(remote, /<img [^>]*src="https:\/\/example\.com\/banner\.png"/)
   assert.match(remote, /loading="lazy"/)
   assert.match(remote, /referrerpolicy="no-referrer"/)
+  // Carries core's own class, which is what puts the picture BENEATH its link
+  // (an <img> is inline) and caps it to the column. Found on the live rig.
+  assert.match(remote, /class="forum-embed"/)
   // The link survives in both. A blocked or dead image degrades to the URL the
   // author actually wrote.
   assert.ok(remote.includes('<a href="https://example.com/banner.png"'))
@@ -159,6 +184,23 @@ test('http URLs and non-image URLs stay plain links', () => {
   assert.ok(!notAnImage.includes('<img'))
 })
 
+test('an UPLOADED image becomes a picture — the composer’s own path', () => {
+  // Found on the live rig, not by any unit test here. `uploads` mode hands the
+  // composer a root-relative path, the composer puts it in the body as TEXT, and
+  // the renderer only rewrites ANCHORS — so the write path has to produce one, or
+  // an uploaded image can never render. isEmbeddableImageUrl accepted these paths
+  // from day one; nothing made an anchor out of them.
+  const stored = cleanForumBody('<p>/uploads/1787-abc.png</p>')
+  assert.match(stored, /<a href="\/uploads\/1787-abc\.png"/)
+  assert.match(renderForumBody(stored, 'uploads'), /<img [^>]*src="\/uploads\/1787-abc\.png"/)
+  assert.equal(renderForumBody(stored, 'disabled'), stored)
+})
+
+test('ordinary prose containing a slash is not turned into a link', () => {
+  // The upload branch is deliberately narrow — `/uploads/` and nothing else.
+  assert.ok(!cleanForumBody('<p>meet at /the docks tonight</p>').includes('<a href'))
+})
+
 test('a URL inside code or pre is shown, not offered', () => {
   const stored = cleanForumBody('<pre>https://example.com/a.png</pre>')
   assert.ok(!stored.includes('<a href'))