fix(viewer): chronicle hygiene — XSS payload bodies + internal-metadata leak (#731, #732) - #740
Merged
Merged
Conversation
…rnal kind labels/You— artifact (#731, #732) #731 (XSS partial sanitization): neutralizeMarkup stripped <script> TAGS but left the inner BODY as text ('<script>alert(1)</script>' -> 'alert(1)'), which rode into the chronicle as a player action. Now excise the bodies of script-class/embedded-content tags (script/style/iframe/object/embed/svg/math/template/noscript/...) BEFORE the generic tag strip, so nothing of the payload survives. Benign emphasis prose (<b>hello</b> -> hello) is preserved. #732 (chronicle metadata leak): a recentEvents history row carries the engine's internal SessionLogEntry.kind (narration|dialogue|roll|system|combat). Only narration/action/roll/ dialog had LogEntry branches, so a 'dialogue'/'combat' row fell to the default branch and rendered the raw kind STRING as an uppercase label, with text NOT sanitized. And a player action rendered 'You—…', a formatting artifact reading like DM narration. Now: - new 'dialogue' branch renders sanitized in-world speech (no kind label, scaffolding stripped); - the default branch NEVER renders the raw internal kind (cleanRowLabel drops kind tokens); - player 'You' actions render as clean second-person prose with no 'You—' chrome. Does NOT touch the #720/#727 engine_logged/dedup path or the _pick_campaign/_action_actor campaign-resolution code (#735). Viewer render-only; engine stays sole writer. Tests: viewer/tests/test_chronicle_hygiene.py (19 cases, real shipped fns under Node). Verified: full viewer suite 475 passed / 1 skipped; fast_gate Tier-0 188 passed.
|
Warning Review limit reached
More reviews will be available in 3 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Comment |
100yenadmin
added a commit
that referenced
this pull request
Jun 14, 2026
…nsition meta-text (#752) (#890) * fix(viewer): bound chronicle a11y footprint + suppress inter-beat transition meta-text (#752) The 2026-06-15 confirm sweep flagged #752 MAJOR by 3 of 5 personas (newbie, adversarial, narrative). Verbatim: "Chronicle log grows into one massive block — later beats invisible in a11y tree" / "Oversized chronicle log pushes Actions section out of a11y tree entirely" / "buries action buttons — player can't tell if DM is done" / "hiding all action controls from screen reader". Plus adversarial: "DUPLICATE DM narration for the opening beat" and "Engine META-TEXT transition phrases LEAK into the chronicle between player beats". FELT MECHANISM: the screen-reader / QA blind-player reads the page via a LINEAR, length-capped accessibility snapshot (qa/playwright/palette_server.js does ariaSnapshot().slice(0,9000)) rendered in DOM order. The Chronicle (role="log") renders BEFORE the action palette + composer, so a long run of multi-paragraph DM beats fills the whole snapshot budget and the action controls are sliced off the snapshot ENTIRELY. #402 anchored the bar VISUALLY (a sticky DOM sibling) but a linear capped snapshot can't be fixed by visual positioning. FIX (viewer-only, READ-ONLY — engine stays sole writer): 1. a11y bound: cap the chronicle's ACCESSIBILITY footprint independently of its visual row count — only the most-recent CHRONICLE_A11Y_TAIL (8) rendered rows stay in the a11y tree; older rendered rows are aria-hidden (still fully VISIBLE for sighted scroll-back, full history in the Quest Journal). The latest beat is always exposed; the bound only engages once the rendered list exceeds the tail. Plus a named role="region" aria-label="Actions" so AT can target the controls. New pure exported chronicleRowAriaHidden(i,total) + CHRONICLE_A11Y_TAIL. 2. opening-beat / player-echo DEDUP: pinned the exactly-once contract (the existing #727/#740/#405 dedup already collapses the opening + the Continue/free-text echo; tests lock it so it can't regress). 3. META-TEXT leak: new high-confidence _BEAT_TRANSITION pattern in sanitizeNarration suppresses inter-beat transition stage-directions ("Moving on to the next beat", "Transitioning to the next scene", "Scene transition", "End of beat", "Beginning the next beat", "between the beats", "move to the next part of the story") while preserving real fiction ("heart skips a beat", "tavern scene", "smooth transition from the parapet to the rope", "between the beats of the drum"). Tests (JSX harness, real .jsx transpiled under Node): - viewer/tests/test_chronicle_a11y_bound.py (3): tail constant tight; action controls stay in the a11y tree after 30 rows (only the tail exposed); short chronicle exposes every row. - viewer/tests/test_chronicle_dedup_opening.py (13): opening once across bands; Continue/free-text no double-echo; wrapper + inter-beat transition meta-text suppressed; surrounding real prose preserved; in-world beat/scene/transition prose survives. fast_gate.sh PASS (215 engine). Full viewer suite 596 passed / 1 skipped. * fix(viewer): tighten _BEAT_TRANSITION to stop over-stripping real fiction (review fix) Adversarial review found the meta-text regex silently deleted real fiction sentences ('the end of the act left them breathless', 'by the close of the scene, three lay dead', 'beginning the act of contrition'). Root: the end-of/beginning-the arms used act/chapter/part (real-fiction words) + an optional forward qualifier, so they matched descriptive prose. FIX: those two arms now use only the engine's own struct terms (beat|scene) AND require the phrase to be TERMINAL (the meta note IS the short sentence, ending on the struct) — descriptive fiction embeds the phrase mid-sentence with trailing prose, so it's spared. The leak cases ('End of beat.', 'Beginning the next beat.') still suppress. +5 survive-case tests (the exact over-strip examples). Story quality is the north star — no silent fiction deletion. --------- Co-authored-by: Eva <arncalso@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two player-facing chronicle-hygiene bugs from the v1.0.4-rc1 RRI @
fa97b34(adversarial/newbie/veteran/narrative personas). Both live on the chat/chronicle render path (viewer/openworlds+viewer/server.pysurface builders). Viewer render-only — the engine stays sole writer; no wire-contract change.#731 — XSS partial sanitization (inner text leaks as a player action)
neutralizeMarkup(app.jsx) stripped<script>tags but kept the body as text:"<script>alert(1)</script>"→"alert(1)", which rode into the chronicle rendered as the player's action — an injection/spoofing surface. The old comment even claimed "this is NOT an XSS fix."Fix: excise the bodies of script-class / embedded-content tags (
script|style|iframe|object|embed|svg|math|template|noscript|xml|applet|frame|frameset) before the generic tag strip, so nothing of the payload survives. Benign emphasis prose is preserved (<b>hello</b>→hello).#732 — internal metadata + scaffolding leak into the chronicle
A
recentEventshistory-band row carries the engine's internalSessionLogEntry.kind(narration|dialogue|roll|system|combat).LogEntryonly branched onnarration/action/roll/dialog, so adialogue/combatrow fell to the default branch, which rendered{entry.label || kind}— leaking the literal "dialogue"/"combat" kind string as an uppercase label, with its text not passed throughsanitizeNarration(so story-craft scaffolding leaked too). Player actions also rendered "You—…", a formatting artifact that reads like DM narration.Fix:
dialogueLogEntry branch → renders sanitized in-world speech (no kind label; scaffolding stripped via the existingsanitizeNarrationguard).cleanRowLabeldrops internal kind tokens; renders prose only).Youactions render as clean second-person prose with noYou—chrome (a named NPC actor keeps its styledName — actionattribution).Root cause (file:line)
viewer/openworlds/app.jsx:162—neutralizeMarkuptag-only regex kept tag bodies as text ([reliability][P0] Active PC silently switches between beats — the zero_critical blocker #731).viewer/openworlds/screen-table.jsx:~1462—LogEntrydefault branch renderedentry.label || kind(raw internal kind) + unsanitized text;actionbranch renderedwho("You") with an em-dash ([security][major] XSS partial sanitization — inner text leaks into chronicle as a player action #732).Scope / constraints honored
engine_logged/dedup path (opening-duplication is already fixed)._pick_campaign/_action_actorcampaign-resolution ([reliability][P0] Active PC silently switches between beats — the zero_critical blocker #735 owns that).Tests
viewer/tests/test_chronicle_hygiene.py— 19 cases, exercising the REAL shippedneutralizeMarkupandLogEntryunder Node (transpiled via the bundled Babel; no reimplementation). Asserts a<script>alert(1)</script>move renders inert/empty, adialogue/combatrow never surfaces its kind label, dialogue text is sanitized, and a player action has noYou—artifact.Verification
viewer/tests: 475 passed, 1 skipped, 48 subtests (full suite, no regressions).qa/fast_gate.sh: Tier-0 PASS — 188 engine + seat-path.