From 5954dfc4fe5855238ff4d291ebc486542fa7d5dd Mon Sep 17 00:00:00 2001 From: Eva Date: Wed, 10 Jun 2026 04:41:51 +0700 Subject: [PATCH] =?UTF-8?q?fix(viewer):=20chronicle=20hygiene=20=E2=80=94?= =?UTF-8?q?=20strip=20XSS=20payload=20bodies=20+=20drop=20internal=20kind?= =?UTF-8?q?=20labels/You=E2=80=94=20artifact=20(#731,=20#732)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #731 (XSS partial sanitization): neutralizeMarkup stripped ' -> '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 (hello -> 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. --- viewer/openworlds/app.jsx | 37 +++-- viewer/openworlds/screen-table.jsx | 66 +++++++- viewer/tests/test_chronicle_hygiene.py | 208 +++++++++++++++++++++++++ 3 files changed, 297 insertions(+), 14 deletions(-) create mode 100644 viewer/tests/test_chronicle_hygiene.py diff --git a/viewer/openworlds/app.jsx b/viewer/openworlds/app.jsx index bc243981..41e32afe 100644 --- a/viewer/openworlds/app.jsx +++ b/viewer/openworlds/app.jsx @@ -146,19 +146,38 @@ function openWorldsSyncHashForScreen(id, opts) { } } -// #342: neutralize markup in player free-text BEFORE it is sent to the engine or echoed into the -// chronicle. The adversarial run (#324 v2) found that submitting "", "{{ }}", or +// #342 + #731: neutralize markup in player free-text BEFORE it is sent to the engine or echoed into +// the chronicle. The #324 v2 adversarial run found that submitting "", "{{ }}", or // "…" sent the raw markup straight to the DM (it stalled 35s+) and rode along in the local -// echo. React already escapes on *display* (it never renders raw HTML), so this is NOT an XSS fix — -// it is a robustness fix: a hostile/odd free-text turn must not be able to wedge the DM or the loop. -// We strip angle-bracket tags and defang template-style "{{ … }}" / "}}" runs to plain text, collapse -// whitespace, and cap absurd length — keeping ordinary apostrophes, quotes, punctuation, and emoji -// intact so a normal in-character line is untouched. Viewer-side only; the engine stays sole writer. +// echo. React escapes on *display*, but #342's tag-only strip still leaked the script BODY as text: +// "" → "alert(1)" rendered as the player's action — an injection/spoofing +// surface (#731, v1.0.4-rc1 RRI). The fix below excises the BODIES of script-class / embedded-content +// tags (their content is never in-world prose), then strips remaining angle-bracket tags and defangs +// template-style "{{ … }}" / "}}" runs to plain text, collapses whitespace, and caps absurd length — +// keeping ordinary apostrophes, quotes, punctuation, emoji, and benign emphasis prose intact so a +// normal in-character line is untouched. Viewer-side only; the engine stays sole writer. window.neutralizeMarkup = window.neutralizeMarkup || function neutralizeMarkup(raw) { if (typeof raw !== "string") return ""; let t = raw; - // Drop anything that looks like an HTML/XML tag (incl. bodies are kept as text - // once their tags are removed). Do it twice so "<>" style nesting can't leave a stray bracket. + // #731 (v1.0.4-rc1 adversarial RRI): excise the *bodies* of script-class / embedded-content tags + // FIRST — before the generic tag strip below. Previously only the tags were removed, so + // "" left "alert(1)" as text, which rode into the chronicle as a player + // action (an injection/spoofing surface). For these elements the CONTENT is never in-world prose, + // so drop the whole "…" span — open tag, body, and close tag — leaving nothing. + // Also catch a self-closed/orphaned open tag of the same class. Case-insensitive; the `[\s\S]` + // body match spans newlines. Ordinary emphasis tags (//…) are NOT in this list — their text + // is legitimate prose and is preserved by the generic tag strip that follows. + t = t.replace( + /<(script|style|iframe|object|embed|svg|math|template|noscript|xml|applet|frame|frameset)\b[\s\S]*?<\/\1\s*>/gi, + " ", + ); + t = t.replace( + /<\/?(script|style|iframe|object|embed|svg|math|template|noscript|xml|applet|frame|frameset)\b[^>]*>/gi, + " ", + ); + // Drop anything that looks like an HTML/XML tag (incl. … bodies are kept as text once their + // tags are removed — that is legitimate emphasis prose). Do it twice so "<>" style nesting can't + // leave a stray bracket. t = t.replace(/<\/?[a-zA-Z][^>]*>/g, " ").replace(/<\/?[a-zA-Z][^>]*>/g, " "); // Defang stray angle brackets that weren't part of a full tag. t = t.replace(/[<>]/g, " "); diff --git a/viewer/openworlds/screen-table.jsx b/viewer/openworlds/screen-table.jsx index 0f79ba3e..31720115 100644 --- a/viewer/openworlds/screen-table.jsx +++ b/viewer/openworlds/screen-table.jsx @@ -1403,8 +1403,44 @@ function ConditionRow({ icon, name, who, detail, tone }) { ); } +// #732 (v1.0.4-rc1 RRI — chronicle hygiene): the engine's INTERNAL session-log kind names +// (SessionLogEntry.kind = narration | dialogue | roll | system | combat) and a stray bare +// label are bookkeeping, NEVER a player-facing attribution. A recentEvents history row carries +// its raw kind, and before this guard a `dialogue`/`combat` row fell to LogEntry's default branch +// and rendered the literal "dialogue"/"combat" string as an uppercase label. `cleanRowLabel` +// returns a label ONLY when it is a real in-world speaker name, dropping any internal kind token +// (and the trivial "You", whose attribution the prose carries — see the action branch below). +const _INTERNAL_KIND_LABELS = new Set([ + "narration", "dialogue", "dialog", "roll", "system", "combat", "action", + "began", "meeting", "arrival", "faction_move", +]); +function cleanRowLabel(label) { + const s = String(label == null ? "" : label).trim(); + if (!s) return ""; + if (_INTERNAL_KIND_LABELS.has(s.toLowerCase())) return ""; + return s; +} + function LogEntry({ entry }) { const kind = entry.kind || "narration"; + // #732: an engine `dialogue` row (recentEvents history band) is DM-authored speech — render it as + // sanitized in-world prose through the same guard as narration, so it can't (a) surface its raw + // kind label via the default branch nor (b) leak story-craft scaffolding the narration path strips. + if (entry.kind === "dialogue") { + const text = sanitizeNarration(entry.text); + if (!text) return null; + const speaker = cleanRowLabel(entry.who || entry.label); + return ( +
+ {speaker ? ( + + {speaker} + + ) : null} + {text} +
+ ); + } if (entry.kind === "narration") { // #335: render-path-complete guard. Every narration source (the /chat tail AND // engine recentEvents) flows through this one branch, so sanitizing here means a @@ -1429,10 +1465,22 @@ function LogEntry({ entry }) { ); } if (entry.kind === "action") { + // #732: the player's OWN action ("You") rendered "You—I draw my sword." — the "You—" prefix is a + // formatting artifact that reads like DM narration scaffolding. The player already knows they are + // the actor, so render their action as clean second-person prose with no attribution chrome. + // A non-self actor (an NPC the engine names) keeps its styled "Name — action" attribution. + const rawWho = String(entry.who == null ? "" : entry.who).trim().toLowerCase(); + const isSelf = rawWho === "you" || rawWho === ""; + const actorLabel = isSelf ? "" : cleanRowLabel(entry.who); + if (!actorLabel) { + return ( +
{entry.text}
+ ); + } return (
- {entry.who} + {actorLabel} — {entry.text} @@ -1459,12 +1507,20 @@ function LogEntry({ entry }) {
); } + // #732: the catch-all for any other (e.g. future) kind. NEVER fall back to rendering the raw + // internal `kind` string as a label — that is exactly the "dialogue"/"combat" leak. Show a real + // in-world label only (cleanRowLabel drops internal kind tokens), and the row text otherwise. + const fallbackLabel = cleanRowLabel(entry.label); + const fallbackText = entry.text || entry.detail; + if (!fallbackText) return null; return (
- - {entry.label || kind} - - {entry.text || entry.detail} + {fallbackLabel ? ( + + {fallbackLabel} + + ) : null} + {fallbackText}
); } diff --git a/viewer/tests/test_chronicle_hygiene.py b/viewer/tests/test_chronicle_hygiene.py new file mode 100644 index 00000000..abffd912 --- /dev/null +++ b/viewer/tests/test_chronicle_hygiene.py @@ -0,0 +1,208 @@ +"""Chronicle-hygiene behavior tests for the player-facing story scroll (#731 + #732). + +These exercise the REAL shipped browser functions (no reimplementation): + + • #731 (XSS partial-sanitization): `neutralizeMarkup` (viewer/openworlds/app.jsx) is the + guard that cleans player free-text BEFORE it is echoed into the chronicle / sent to the DM. + The v1.0.4-rc1 adversarial RRI found `` had its TAGS stripped but + the INNER TEXT ("alert(1)") leaked through and rendered as a player action — an + injection/spoofing surface. The guard must strip dangerous tag *bodies* (script/style/etc.) + so nothing of the payload survives, and HTML-escape is moot because the result must be inert + TEXT either way. + + • #732 (chronicle metadata leak): `LogEntry` (viewer/openworlds/screen-table.jsx) renders one + chronicle row. A `recentEvents` history-band row carries the engine's INTERNAL session-log + `kind` ("narration" | "dialogue" | "roll" | "system" | "combat"). Only narration/action/roll/ + dialog had branches, so a `dialogue`/`combat` row fell to the default branch and rendered the + raw kind STRING ("dialogue"/"combat") as an uppercase label, with its text NOT passed through + sanitizeNarration (so scaffolding leaked too). And a player action rendered "You—…", a + formatting artifact that reads like DM narration. The chronicle must render clean prose only: + never an internal kind label, never the "You—" artifact, and the dialogue path must sanitize. + +Both functions live in browser JS; mirroring the sibling JS-behavior tests we transpile/eval the +ACTUAL `.jsx` under Node so the test tracks shipped behavior. Skipped if Node is not on PATH. +""" +from __future__ import annotations + +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +HERE = Path(__file__).resolve().parent +OPENWORLDS = HERE.parent / "openworlds" +APP_JSX = OPENWORLDS / "app.jsx" +SCREEN_TABLE = OPENWORLDS / "screen-table.jsx" +BABEL = OPENWORLDS / "vendor" / "babel-standalone-7.29.0.min.js" + + +def _node() -> str: + node = shutil.which("node") + if not node: + pytest.skip("node not on PATH; skipping JS-behavior test") + return node + + +# --------------------------------------------------------------------------------------------- # +# #731 — neutralizeMarkup: strip dangerous tag BODIES, never leak inner text into the chronicle. +# --------------------------------------------------------------------------------------------- # +def _extract_fn(src: str, name: str) -> str: + marker = f"function {name}(" + start = src.index(marker) + depth = 0 + for i in range(start, len(src)): + c = src[i] + if c == "{": + depth += 1 + elif c == "}": + depth -= 1 + if depth == 0: + return src[start : i + 1] + raise AssertionError(f"could not brace-match {name}()") + + +def _neutralize(text) -> str: + src = APP_JSX.read_text(encoding="utf-8") + fn = _extract_fn(src, "neutralizeMarkup") + snippet = ( + fn + + "\nconst __t = " + + json.dumps(text) + + ";\nprocess.stdout.write(String(neutralizeMarkup(__t)));\n" + ) + proc = subprocess.run([_node(), "-e", snippet], capture_output=True, text=True, timeout=30) + if proc.returncode != 0: + raise AssertionError(f"node failed: {proc.stderr}") + return proc.stdout + + +def test_731_script_payload_inner_text_does_not_leak(): + # The exact RRI move: a ") + assert "alert" not in out + assert out.strip() == "" + + +def test_731_script_body_does_not_ride_along_with_real_prose(): + # A payload glued to a legit action must drop the payload while keeping the action prose. + out = _neutralize("I draw my sword") + assert "document.cookie" not in out + assert "cookie" not in out + assert "I draw my sword" in out + + +@pytest.mark.parametrize("tag", ["script", "style", "iframe", "object", "embed", "svg", "math", "template"]) +def test_731_dangerous_tag_bodies_are_excised(tag): + out = _neutralize(f"<{tag}>PAYLOAD_{tag}safe text") + assert f"PAYLOAD_{tag}" not in out + assert "PAYLOAD" not in out + assert "safe text" in out + + +def test_731_event_handler_attribute_vector_is_inert(): + # An never had a body, but the attribute payload must not survive as text. + out = _neutralize("") + assert "alert" not in out + assert "onerror" not in out + + +def test_731_ordinary_emphasis_prose_is_preserved_as_text(): + # The fix must NOT over-strip: a benign inline tag around real words keeps the words (the tag + # itself is removed, the prose reads clean). This is a player typing markup by habit, not an attack. + out = _neutralize("I say hello to the guard") + assert "" not in out and "" not in out + assert "hello" in out + assert "I say" in out and "to the guard" in out + + +def test_731_case_and_whitespace_variants_of_script_are_stripped(): + for raw in ( + "", + "", + "alert(3)", + ): + out = _neutralize(raw) + assert "alert" not in out, f"leaked from: {raw!r} -> {out!r}" + + +# --------------------------------------------------------------------------------------------- # +# #732 — LogEntry: no internal kind label, no "You—" artifact, dialogue text is sanitized. +# --------------------------------------------------------------------------------------------- # +def _render_log_entries(entries: list[dict]) -> list[str]: + """Transpile screen-table.jsx with the bundled Babel, capture each LogEntry's createElement + tree, and return the flattened visible TEXT of each rendered row.""" + program = ( + "const fs = require('fs'); const vm = require('vm');\n" + + "const Babel = require(%s);\n" % json.dumps(str(BABEL)) + + "const src = fs.readFileSync(%s, 'utf8');\n" % json.dumps(str(SCREEN_TABLE)) + + "const code = Babel.transform(src, { presets: ['react'], filename: 'screen-table.jsx' }).code;\n" + + "function h(type, props, ...children){ return { type: (typeof type==='function'?(type.name||'C'):type)," + + " props: props||{}, children: children.flat(Infinity).filter(c=>c!=null) }; }\n" + + "const React = { useState:()=>[null,()=>{}], useRef:()=>({}), useCallback:f=>f, useEffect:()=>{}," + + " createElement:h, Fragment:'F' };\n" + + "const sb = { React }; sb.window = sb; vm.createContext(sb); vm.runInContext(code, sb);\n" + + "const LogEntry = sb.window.LogEntry;\n" + + "if (typeof LogEntry !== 'function') throw new Error('LogEntry not exported on window');\n" + + "function textOf(n){ if(n==null) return ''; if(typeof n==='string'||typeof n==='number') return String(n);" + + " if(Array.isArray(n)) return n.map(textOf).join(''); if(n.children) return n.children.map(textOf).join('');" + + " return ''; }\n" + + "const entries = " + json.dumps(entries) + ";\n" + + "const out = entries.map((e) => textOf(LogEntry({ entry: e })));\n" + + "process.stdout.write(JSON.stringify(out));\n" + ) + proc = subprocess.run( + [_node(), "--input-type=commonjs"], input=program, text=True, capture_output=True + ) + if proc.returncode != 0: + raise AssertionError(f"node failed: {proc.stderr}") + return json.loads(proc.stdout) + + +def test_732_dialogue_row_does_not_surface_its_kind_label(): + # A recentEvents dialogue row with NO speaker label previously rendered the raw kind string + # "dialogue" as an uppercase label. The player must never see the internal kind name. + (rendered,) = _render_log_entries([{"kind": "dialogue", "text": '"Hold there."'}]) + assert "dialogue" not in rendered.lower() + assert "Hold there" in rendered # the in-world line survives + + +def test_732_combat_row_does_not_surface_its_kind_label(): + (rendered,) = _render_log_entries([{"kind": "combat", "text": "A blade flashes."}]) + assert "combat" not in rendered.lower() + assert "A blade flashes" in rendered + + +def test_732_unknown_internal_kind_never_renders_its_raw_name(): + # Any internal/scaffolding kind name (e.g. a prelude "meeting"/"arrival" note or a future kind) + # must not leak as a visible label. + for kind in ("narration", "dialogue", "combat", "began", "meeting", "arrival", "faction_move"): + (rendered,) = _render_log_entries([{"kind": kind, "text": "The world turns."}]) + assert kind.lower() not in rendered.lower(), f"kind label leaked for {kind!r}: {rendered!r}" + assert "The world turns" in rendered + + +def test_732_player_action_has_no_you_dash_artifact(): + # A player action row rendered "You—I draw my sword." — the "You—" prefix reads like DM + # narration scaffolding. The player's own action must render as clean prose with no "You—" artifact. + (rendered,) = _render_log_entries([{"kind": "action", "who": "You", "text": "I draw my sword."}]) + assert "You—" not in rendered + assert "I draw my sword" in rendered + + +def test_732_dialogue_text_is_sanitized_like_narration(): + # The dialogue path must run sanitizeNarration so story-craft scaffolding can't ride a + # dialogue-kind row into the chronicle (the narration path already strips this — #347). + (rendered,) = _render_log_entries( + [{"kind": "dialogue", "text": "The lock holds after three failed social checks.", "label": "DM"}] + ) + assert "social checks" not in rendered.lower() + assert "The lock holds" in rendered + + +def test_732_narration_row_is_unchanged_clean_prose(): + # Regression guard: an ordinary narration row still renders its prose, no label, no artifact. + (rendered,) = _render_log_entries([{"kind": "narration", "text": "Rain gathers on the cobbles."}]) + assert rendered == "Rain gathers on the cobbles."