fix(teams): four defects the live rig found in the forum
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>
This commit is contained in:
@@ -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'))
|
||||
|
||||
Reference in New Issue
Block a user