feat(ui): explain the impersonation warning where a phone user can read it#494
feat(ui): explain the impersonation warning where a phone user can read it#494sanity wants to merge 6 commits into
Conversation
Review — Light tier + adversarial readTier: Light. Additive UI + fixture + tests, no protocol/contract/wire surface, no WASM change (both migration checks pass untouched). But the modal wiring is non-trivial logic on a trust surface, so it got a full adversarial read rather than a rubber stamp. External model unavailable — Codex is out of credit; Gemini CLI remains deprecated. Claude-subagent substitution, same as #489. Recorded so the weaker signal is visible. Verdict: no BLOCKING findings. Three IMPORTANT, addressed before publish. Clean, and verified rather than assumed
Fixed before publish
Worth carrying forwardA badge's Also noted, not a finding: On the 320px testIt was vacuous as first written and passed against the reintroduced clipping bug: the fixture's generated names are too short to overflow at 320px, so the assertion measured a condition that could not occur. Found only by mutating. It now forces the overflow and asserts the name is actually clamping as a premise, and the collision was re-demonstrated on the static release harness — the one CI uses — not just through a dev server. [AI-assisted - Claude] |
Three review findings on #494. **The modal's arguments were pinned by nothing.** The member list and the conversation author line each carry a whitespace-insensitive argument pin; the third surface had only a check that the function name appeared. Two swaps compile, pass every other test, and are invisible in the fixture because the impostor and the viewer both have `privilege_in_view` of `None`. The subtle one is the 4th argument: a viewer who is themselves badged — every moderator in Freenet Official, the room this feature exists for — would open an impostor's modal and be shown the PRIVILEGED-clash sentence, "Name conflict … Both are real; check the member ID". That is the wording for a legitimate collision, displayed for a genuine impersonation, exonerating the impostor to precisely the people who would otherwise ban them. Both swaps verified RED, then restored. **The surface list said "two".** It is three now, and the comment that says so is the one whose whole point is not being stale — it explicitly listed the member-info modal among the surfaces the warning is NOT rendered on. `impersonation_warning_is_wired_into_both_render_surfaces` was likewise named for two of three and pinned nothing about the modal; renamed to `..._into_every_render_surface` and extended to cover it, so dropping the modal fails the same test that would catch dropping either of the others. **More than half the demo runs were shouting.** Measured exhaustively over the real pools (46 x 23 = 1,058 handles): `I`->`l` covers 6.5%, `l`->`I` covers 42.1%, and the remaining 51.4% fell through to uppercasing. So most `cargo make dev-example` runs showed `BOB SMITH (MEMBER)` flagged against `Bob Smith (Member)` — a correct tier-1 match, since the case-insensitive fold is exactly that, but one a developer reasonably reads as a false positive. A fixture that teaches the reader to distrust the badge inverts its own purpose. A letter-for-digit branch from the fold's own ASCII table now runs first; all 544 of those handles contain at least one of `o/e/s/a/t`, so it covers the remainder and the demo shows `B0b Smith (Member)`. A sweep pins that the uppercase fallback is unreachable for generated handles. Minor, same review: - The advice added at `ImpersonationWarning::tooltip` said to grep for `getByRole` locators. Since this PR makes the sentence VISIBLE TEXT, `getByText`, `hasText` and `toContainText` are collision surfaces too — a class that did not exist while it lived only in `title`/`aria-label`. - The imitated-name chip was asserted visible and non-empty but never compared to the deputy's actual name, so rendering the FLAGGED member's own name passed — and under the bar-class branches the two differ by one character, so it survived inspection by eye as well. Now compared. Verified RED against exactly that mutation: expected `Charlie Williams (Member)`, received `CharIie Williams (Member)`. - The chip is `inline-block` with no wrapping, so a long spaceless nickname overflowed `max-w-md` at 320px. Added `max-w-full break-words`. Also fixed a pin of my own that this exposed: the visible-text scrape took a fixed 900-character window, and adding a comment inside the block pushed the element it checks out of range. A pin whose reach depends on how much prose sits above it is not a pin; it is now scoped between anchors. 655 Rust tests pass; Playwright 93 passed / 6 skipped / 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
Three review findings on #494. **The modal's arguments were pinned by nothing.** The member list and the conversation author line each carry a whitespace-insensitive argument pin; the third surface had only a check that the function name appeared. Two swaps compile, pass every other test, and are invisible in the fixture because the impostor and the viewer both have `privilege_in_view` of `None`. The subtle one is the 4th argument: a viewer who is themselves badged — every moderator in Freenet Official, the room this feature exists for — would open an impostor's modal and be shown the PRIVILEGED-clash sentence, "Name conflict … Both are real; check the member ID". That is the wording for a legitimate collision, displayed for a genuine impersonation, exonerating the impostor to precisely the people who would otherwise ban them. Both swaps verified RED, then restored. **The surface list said "two".** It is three now, and the comment that says so is the one whose whole point is not being stale — it explicitly listed the member-info modal among the surfaces the warning is NOT rendered on. `impersonation_warning_is_wired_into_both_render_surfaces` was likewise named for two of three and pinned nothing about the modal; renamed to `..._into_every_render_surface` and extended to cover it, so dropping the modal fails the same test that would catch dropping either of the others. **More than half the demo runs were shouting.** Measured exhaustively over the real pools (46 x 23 = 1,058 handles): `I`->`l` covers 6.5%, `l`->`I` covers 42.1%, and the remaining 51.4% fell through to uppercasing. So most `cargo make dev-example` runs showed `BOB SMITH (MEMBER)` flagged against `Bob Smith (Member)` — a correct tier-1 match, since the case-insensitive fold is exactly that, but one a developer reasonably reads as a false positive. A fixture that teaches the reader to distrust the badge inverts its own purpose. A letter-for-digit branch from the fold's own ASCII table now runs first; all 544 of those handles contain at least one of `o/e/s/a/t`, so it covers the remainder and the demo shows `B0b Smith (Member)`. A sweep pins that the uppercase fallback is unreachable for generated handles. Minor, same review: - The advice added at `ImpersonationWarning::tooltip` said to grep for `getByRole` locators. Since this PR makes the sentence VISIBLE TEXT, `getByText`, `hasText` and `toContainText` are collision surfaces too — a class that did not exist while it lived only in `title`/`aria-label`. - The imitated-name chip was asserted visible and non-empty but never compared to the deputy's actual name, so rendering the FLAGGED member's own name passed — and under the bar-class branches the two differ by one character, so it survived inspection by eye as well. Now compared. Verified RED against exactly that mutation: expected `Charlie Williams (Member)`, received `CharIie Williams (Member)`. - The chip is `inline-block` with no wrapping, so a long spaceless nickname overflowed `max-w-md` at 320px. Added `max-w-full break-words`. Also fixed a pin of my own that this exposed: the visible-text scrape took a fixed 900-character window, and adding a comment inside the block pushed the element it checks out of range. A pin whose reach depends on how much prose sits above it is not a pin; it is now scoped between anchors. 655 Rust tests pass; Playwright 93 passed / 6 skipped / 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
bf6df97 to
26d4a8f
Compare
The ⚠ impersonation warning (#489) rendered on two surfaces, and on both of them its explanation lived only in a `title=` attribute. `title=` NEVER FIRES ON TOUCH, so on a phone the feature conveyed "there is a ⚠ here" and stopped — and the reader's natural next move, tapping the name, opened a member-info modal that said nothing about it. The badge had also never been seen in a browser by anyone. Example data seeds a spoof nickname so the 🛡 shield is visible in dev builds, but nothing in it produced two members whose names fold together, so `cargo make dev-example` could not show a ⚠ at all. Wire the warning into the modal through `impersonation_warning_for_display` — the same entry point the member list and the conversation author line use, so this surface cannot show a badge the others do not — and render its sentence as VISIBLE TEXT rather than only as a tooltip. The imitated name is shown here and nowhere else, as its own DOM node: safe for the same reason `appointer_names` is, since a comma inside an attacker-chosen nickname forges structure in a flat string but cannot span two elements. The chip's label is derived from the tooltip's own leading clause, so this surface cannot invent wording that drifts from the single definition in `ImpersonationWarning::tooltip`, and it follows that function's privileged/unprivileged branch for free. This pays for the whole badge map, which `deputy_badge_for_viewer` deliberately avoids. That cost is documented at the call site: the modal is opened by a deliberate click and renders one member, and the alternative is not a cheaper warning but no explanation at all on a phone. The shield still uses the single-target helper, so its Playwright-pinned behaviour stays on its own path. Example data gains an impostor: a plain member (NOT a second deputy, which would break `member-info-deputy-tag.spec.ts`'s one-shield assertion) whose nickname is DERIVED from the deputy's at build time, because those names are re-rolled every run and a hardcoded `lan Clarke` would collide only on the runs where the generator happened to pick `Ian Clarke`. They get a guaranteed message, so the conversation always has a ⚠ author line directly below the deputy's 🛡 one. - `the_example_impostor_always_collides_with_the_deputy` pins that every branch of `confusable_variant` is a tier-1 collision, against literals covering all three branches AND 500 draws from the real generator. - `every_example_room_shows_the_impersonation_warning` asserts the fixture reaches its own premise in all three rooms: the deputy is badged, the checker is non-empty, and exactly one member — a plain member, not a deputy — is flagged. - `modal_renders_the_impersonation_warning_as_visible_text` pins the shared entry point and, specifically, that the sentence is rendered as text and not only as `title=`. Both new behaviours were verified by mutation, not by assumption: breaking `confusable_variant` so the names no longer fold turns both example-data tests RED, and deleting the visible sentence turns the modal pin RED. Restored, all 649 UI tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
Five tests: the impostor's member row carries the badge and the deputy's does not, the conversation author line carries it, the modal explains it in VISIBLE text, the deputy's modal shows the shield with no warning, and it survives a 320px viewport. The last two are the ones a unit test structurally cannot reach. `title=` never fires on touch, so only a browser can assert a phone reader can get at the explanation; and the clipping regression this feature already hit once (a `truncate` on the row button eating the badge) is invisible to any assertion that is not a real layout at a real width. The 320px case asserts the glyph's box sits inside its row's box rather than merely being 'visible', because the original bug presented as a badge pushed outside its own row. The member-list badge has no data-testid yet (P20, in members.rs), so the member-list locators match the glyph inside span.member-icon and name the testid they should switch to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
A mutation check on the new 320px Playwright assertion reported a pass against a bug that was definitely present — the dx serve watcher had not rebuilt, so the browser was still running the previous binary. That inverts the result of the one technique that proves a browser test is not vacuous, so it is worth more than a comment in a spec file. Also records two neighbours found the same way: a stale dx earlier on PATH (0.7.3 against the crate's 0.7.9) serves a 404 page that reads like a broken change, and port 8082 being the suite's default means a server left running by another worktree gets tested instead of yours. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
Adding a flagged member to example data broke an unrelated spec, in a way
worth recording because the mechanism is not obvious.
`getByRole` matches the accessible name by case-insensitive SUBSTRING, and a
member row's accessible name includes the `aria-label` of every badge inside
it. The impersonation warning's label reads "…but their name CLOSEly
resembles one", so every row carrying a ⚠ now also answers to
`getByRole("button", { name: "Close" })`. The Export Identity spec then
failed on a strict-mode violation at its Close button, pointing at a member
row, with nothing to suggest the impersonation feature was involved.
`exact: true` is the fix and is also just correct for a button whose whole
label is "Close". Found by running the full suite rather than only the new
spec — the new spec passed throughout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
…pecs The comment explaining this collision was in a spec file, which is not where anyone about to reword the warning will be looking. It belongs at the string itself. An element's accessible name is composed from its descendants, so this text becomes part of the accessible name of every row and header containing the badge, and Playwright matches accessible names by case-insensitive substring unless the caller passes `exact: true`. That is how "closely resembles" made every flagged member row answer to a locator for the Close button. Notes the second-order trap too: the sentence branches on the flagged member's own privilege, so a collision can exist in one wording and not the other and look intermittent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
Three review findings on #494. **The modal's arguments were pinned by nothing.** The member list and the conversation author line each carry a whitespace-insensitive argument pin; the third surface had only a check that the function name appeared. Two swaps compile, pass every other test, and are invisible in the fixture because the impostor and the viewer both have `privilege_in_view` of `None`. The subtle one is the 4th argument: a viewer who is themselves badged — every moderator in Freenet Official, the room this feature exists for — would open an impostor's modal and be shown the PRIVILEGED-clash sentence, "Name conflict … Both are real; check the member ID". That is the wording for a legitimate collision, displayed for a genuine impersonation, exonerating the impostor to precisely the people who would otherwise ban them. Both swaps verified RED, then restored. **The surface list said "two".** It is three now, and the comment that says so is the one whose whole point is not being stale — it explicitly listed the member-info modal among the surfaces the warning is NOT rendered on. `impersonation_warning_is_wired_into_both_render_surfaces` was likewise named for two of three and pinned nothing about the modal; renamed to `..._into_every_render_surface` and extended to cover it, so dropping the modal fails the same test that would catch dropping either of the others. **More than half the demo runs were shouting.** Measured exhaustively over the real pools (46 x 23 = 1,058 handles): `I`->`l` covers 6.5%, `l`->`I` covers 42.1%, and the remaining 51.4% fell through to uppercasing. So most `cargo make dev-example` runs showed `BOB SMITH (MEMBER)` flagged against `Bob Smith (Member)` — a correct tier-1 match, since the case-insensitive fold is exactly that, but one a developer reasonably reads as a false positive. A fixture that teaches the reader to distrust the badge inverts its own purpose. A letter-for-digit branch from the fold's own ASCII table now runs first; all 544 of those handles contain at least one of `o/e/s/a/t`, so it covers the remainder and the demo shows `B0b Smith (Member)`. A sweep pins that the uppercase fallback is unreachable for generated handles. Minor, same review: - The advice added at `ImpersonationWarning::tooltip` said to grep for `getByRole` locators. Since this PR makes the sentence VISIBLE TEXT, `getByText`, `hasText` and `toContainText` are collision surfaces too — a class that did not exist while it lived only in `title`/`aria-label`. - The imitated-name chip was asserted visible and non-empty but never compared to the deputy's actual name, so rendering the FLAGGED member's own name passed — and under the bar-class branches the two differ by one character, so it survived inspection by eye as well. Now compared. Verified RED against exactly that mutation: expected `Charlie Williams (Member)`, received `CharIie Williams (Member)`. - The chip is `inline-block` with no wrapping, so a long spaceless nickname overflowed `max-w-md` at 320px. Added `max-w-full break-words`. Also fixed a pin of my own that this exposed: the visible-text scrape took a fixed 900-character window, and adding a comment inside the block pushed the element it checks out of range. A pin whose reach depends on how much prose sits above it is not a pin; it is now scoped between anchors. 655 Rust tests pass; Playwright 93 passed / 6 skipped / 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
26d4a8f to
1de6659
Compare
Problem
#489 shipped the ⚠ impersonation warning on two surfaces, and on both of them the explanation lived only in a
title=attribute.title=never fires on touch. So on a phone the feature conveyed "there is a ⚠ here" and stopped, and the reader's natural next move — tapping the name — opened a member-info modal that said nothing about it at all.Separately, nobody had ever seen the badge in a browser.
example_data.rsseeds a spoof nickname so the 🛡 shield is visible in dev builds, but nothing in it produced two members whose names fold together, socargo make dev-examplecould not render a ⚠ under any circumstances.Approach
The modal now explains the warning in visible text. It goes through
impersonation_warning_for_displaywithprivilege_in_view— the same entry point the member list and the conversation author line use — so this surface cannot show a badge the others do not, and it inherits the privileged/unprivileged tooltip branch rather than restating it. The chip's label is derived from the tooltip's own leading clause ("Impersonation warning" / "Name conflict") instead of being written by hand, so this surface cannot invent wording that drifts from the single definition inImpersonationWarning::tooltip. The imitated name is shown here and nowhere else, as its own DOM node — safe for the same reasonappointer_namesis, since a comma inside an attacker-chosen nickname forges structure in a flat string but cannot span two elements.This does pay for the whole badge map, which
deputy_badge_for_viewerdeliberately avoids. That cost is documented at the call site: the modal opens on a deliberate click and renders one member, and the alternative is not a cheaper warning but no explanation at all on a phone. The shield still uses the single-target helper so its Playwright-pinned behaviour stays on its own path.Example data gains an impostor so the badge is visible in dev builds and reachable from a browser test. It is a plain member, not a second deputy — a second deputy would break
member-info-deputy-tag.spec.ts, which asserts exactly one shield, and it would be the wrong shape anyway since the point is a member with no authority wearing a moderator's name. Its nickname is derived from the deputy's at fixture-build time rather than hardcoded, because those names are re-rolled every run and a literallan Clarkewould collide only on the runs where the generator happened to pickIan Clarke.Testing
Five Playwright tests (member-list row, conversation author line, the modal's visible explanation, the deputy's modal, and a 320px layout case) plus three Rust tests. Every new behaviour was checked by mutation rather than by trusting a green.
The 320px test was vacuous as first written, and passed against the clipping bug. This is the most useful thing in this PR for whoever reads it next. Reintroducing the regression that
7c2511e8fixed (truncateback on the row button) and measuring the real DOM:The fixture's generated nicknames are short, so at 320px the row never overflows, nothing can be clipped, and the assertion was measuring a condition that never occurs. The test now forces the overflow itself and carries a premise assertion that the name is actually being clamped, so it fails loudly rather than silently going vacuous again.
Two further corrections came out of the same exercise, both worth knowing:
truncateback on the button stops it being a flex container, which makes both child spans inline, andscrollWidth/clientWidthare 0 for inline elements — so the mutation check failed with "the name is not being clamped", which is true but describes a symptom rather than the clipping. The property is now asserted first and the mutation fails with "the ⚠ is clipped past the right edge of its row".Adding a flagged member to example data broke an unrelated spec, and the mechanism is worth recording.
getByRolematches the accessible name by case-insensitive substring, and a member row's accessible name includes thearia-labelof every badge inside it. The warning's label reads "…but their name closely resembles one", so every row carrying a ⚠ now also answers togetByRole("button", { name: "Close" }). The Export Identity spec failed on a strict-mode violation at its Close button, pointing at a member row, with nothing to suggest this feature was involved. Fixed withexact: true, which is also simply correct for a button whose entire label is "Close". Found only by running the whole suite — the new spec passed throughout.Also verified: breaking
confusable_variantso the names stop folding turns both example-data tests red, and deleting the modal's visible sentence turns the modal pin red.every_example_room_shows_the_impersonation_warningasserts the fixture reaches its own premise in all three rooms — deputy badged, checker non-empty, exactly one member flagged, and that member not a badged deputy — which was re-run after the guard changes in #489 landed rather than assumed to still hold.AGENTS.mdgains a short note on telling whether a UI test measured the build you think it did:dx serveserved a stale build during this work and reported a mutation check as passing, which inverts the one technique that proves a browser test is not vacuous.Full suite green: 649 Rust tests, 5/5 Playwright,
cargo fmtand prettier clean.[AI-assisted - Claude]