fix(teams): four defects the live rig found in the phase 4 forum #154

wtclaude · 2026-08-18T14:54:48Z

wtclaude commented

2026-08-18 14:54:48 +00:00

What & why

Phase 4 merged (website#153) without a live-rig walk. This is the walk, and what it
found. Four defects, none of which could fail a unit test, and three of which
break the feature for the operator rather than for the code.

1. The uploads acknowledgement was a one-way door 🔴

The worst of the four. 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 that value being present rather than on the mode being selected.

So after enabling uploads, the operator could never change a forum setting again.
Unticking "Enable Team forums" came back 400 "Enabling uploads requires
acknowledging the current notice" — the thing they would reach for in a hurry was
exactly the thing refused.

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. Both halves verified live.

2. 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 — that 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.

3. The embed sat beside its link, and nothing capped its size

An <img> is inline, so it landed next to the URL rather than beneath it as §5.5.3
specifies; and a remote image is whatever size its host decided, so one post could
blow the column 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.

4. The panel's buttons had no button styling

btn-ghost is a modifier — every other call site in this repo 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. Same class of mistake as the
Material one in Android M12: the modifier carries no base. Form controls also now
use input/textarea, and the body uses prose (serif was a class I invented and
this codebase does not have).

Plus one thing the walk improved rather than fixed

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

How it was tested

The full rig: real uo-link sidecar (protocol 4, guild board served from its
store) → website with module-uo installed → real MariaDB, browsed as a real leader,
a real admin, and a real unlinked guest.

What the walk proved, beyond the four defects:

Both slots render on the module's page, in order and independently — core's
activity feed and core's forum, inside module-uo's guild page. Turning forums off
removes the forum section entirely and leaves the feed untouched.
The 404 guard, against a genuine member: every forum route 404s while off, and
the grant route still answers. A caller with no access gets 404, never 403.
Acceptance 3 against a real row: same post, disabled → remote, identical
stored MD5, different rendering.
Acceptance 1 end to end: a leader granted an unlinked account access; it reads
the forum with reason: 'grant', holds zero team_members rows, and is absent
from the roster and both counts.
Acceptance 4: uploads without the acknowledgement is refused 400 — and the
refusal is atomic, so teams_forums_enabled in the same PUT is not written either.
Magic-byte sniffing: a PHP payload sent as image/png was rejected 400 and
removed from disk; the real PNG stored, with its attribution row.
The acknowledgement dialog: Enable uploads stays inert with one box ticked,
and the recorded acknowledgement names the admin and the time.
The §2.5.1 staff leadership override resolving into forum authority — isLeader
true at read time while the projection still says is_leader = 0.

npm test in server/ — 975 pass, 0 fail (3 new regression tests: the one-way
door, the upload-path linkify, and prose-with-a-slash). client/ — 224 pass.

Checklist

I have read CONTRIBUTING.md.
The change builds and existing tests/checks pass locally.
I have added or updated tests/docs where it makes sense.
My commits are reasonably scoped with clear messages.

AI-assisted contributions (required)

AI tools were used. Tool(s): Claude Code. I have reviewed and understand
every change, and take responsibility for it. AI-authored commits are
marked with a Co-Authored-By trailer.

License

I agree that my contribution is licensed under this project's license
(GNU GPL v3.0 or later), and I have the right to contribute it.

## What & why Phase 4 merged (website#153) without a live-rig walk. This is the walk, and what it found. **Four defects, none of which could fail a unit test**, and three of which break the feature for the operator rather than for the code. ### 1. The uploads acknowledgement was a one-way door 🔴 The worst of the four. 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 that value being *present* rather than on the mode being *selected*. So after enabling uploads, the operator could never change a forum setting again. Unticking "Enable Team forums" came back **400 "Enabling uploads requires acknowledging the current notice"** — the thing they would reach for in a hurry was exactly the thing refused. 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`. Both halves verified live. ### 2. 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 — that 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. ### 3. The embed sat beside its link, and nothing capped its size An `<img>` is inline, so it landed next to the URL rather than beneath it as §5.5.3 specifies; and a remote image is whatever size its host decided, so one post could blow the column 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. ### 4. The panel's buttons had no button styling `btn-ghost` is a **modifier** — every other call site in this repo 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`. Same class of mistake as the Material one in Android M12: *the modifier carries no base.* Form controls also now use `input`/`textarea`, and the body uses `prose` (`serif` was a class I invented and this codebase does not have). ### Plus one thing the walk improved rather than fixed The post body now re-sanitises client-side like every other body-HTML surface here, with `ADD_ATTR: ['referrerpolicy']`. That argument is load-bearing: DOMPurify's default allowlist carries `loading` but **not** `referrerpolicy`, so a plain `sanitize()` would silently strip the one attribute limiting what a remote embed leaks to the host serving it — the privacy property the admin help text promises an operator. ## How it was tested **The full rig**: real uo-link sidecar (protocol 4, guild board served from its store) → website with `module-uo` installed → real MariaDB, browsed as a real leader, a real admin, and a real unlinked guest. What the walk proved, beyond the four defects: - **Both slots render on the module's page**, in order and independently — core's activity feed and core's forum, inside `module-uo`'s guild page. Turning forums off removes the forum section entirely and leaves the feed untouched. - **The 404 guard**, against a genuine member: every forum route 404s while off, and the grant route still answers. A caller with no access gets 404, never 403. - **Acceptance 3 against a real row**: same post, `disabled` → `remote`, **identical stored MD5**, different rendering. - **Acceptance 1 end to end**: a leader granted an unlinked account access; it reads the forum with `reason: 'grant'`, holds zero `team_members` rows, and is absent from the roster and both counts. - **Acceptance 4**: `uploads` without the acknowledgement is refused 400 — and the refusal is atomic, so `teams_forums_enabled` in the same PUT is not written either. - **Magic-byte sniffing**: a PHP payload sent as `image/png` was rejected 400 **and removed from disk**; the real PNG stored, with its attribution row. - **The acknowledgement dialog**: `Enable uploads` stays inert with one box ticked, and the recorded acknowledgement names the admin and the time. - The §2.5.1 staff leadership override resolving into forum authority — `isLeader` true at read time while the projection still says `is_leader = 0`. `npm test` in `server/` — **975 pass, 0 fail** (3 new regression tests: the one-way door, the upload-path linkify, and prose-with-a-slash). `client/` — 224 pass. ## Checklist - [x] I have read [CONTRIBUTING.md](CONTRIBUTING.md). - [x] The change builds and existing tests/checks pass locally. - [x] I have added or updated tests/docs where it makes sense. - [x] My commits are reasonably scoped with clear messages. ## AI-assisted contributions (required) - [x] AI tools were used. Tool(s): `Claude Code`. I have reviewed and understand every change, and take responsibility for it. AI-authored commits are marked with a `Co-Authored-By` trailer. ## License - [x] I agree that my contribution is licensed under this project's license (**GNU GPL v3.0 or later**), and I have the right to contribute it.

wtclaude added 1 commit 2026-08-18 14:54:50 +00:00

fix(teams): four defects the live rig found in the forum

PR Checks / bot-install (pull_request) Successful in 22s

Details

PR Checks / server-tests (pull_request) Successful in 34s

Details

PR Checks / client-build (pull_request) Successful in 8m48s

Details

5baada08ef

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>

whitlocktech merged commit 763de66ebb into edge

2026-08-18 15:21:57 +00:00

whitlocktech deleted branch fix/teams-phase4-live-rig

2026-08-18 15:21:57 +00:00

whitlocktech referenced this issue from a commit

2026-08-18 15:22:00 +00:00

Merge pull request 'fix(teams): four defects the live rig found in the phase 4 forum' (#154) from fix/teams-phase4-live-rig into edge

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: RunicGateway/website#154