fix: four defects the U4 bug-hunt panel found in code it did not add
All four pre-date U4 and sit in files it touched, which is why a diff-scoped robustness lens saw them. They are separated from the unit's own commit so the feature history stays readable; the release tags both. * A booth name reached a JS string context. The confirm dialogs interpolated the name into a string literal inside `onsubmit`. Jinja's autoescape is HTML-attribute escaping, not JS-string escaping: the browser decodes the entity back to a quote before the JS parser sees it, so a name crafted to close the string executed on submit. Booth names are agent-authored — making a folder under the data dir is the whole API — so this was a live path, not a theoretical one. The name now travels as a data attribute to a delegated handler, where escaping is escaping. * An unreadable `links.md` returned 500 for the whole booth page. `is_file()` then an unguarded `read_text()`. The board is one tile on that page, and a page that will not load is worse than one missing a tile — the posture `read_blurred`, `marks_for` and `read_manifest` already take. * The index order had no tie-breaker, which violates the deterministic-order invariant. Equal-mtime booths fell back to whatever `iterdir()` yielded, and two booths landed by one `rsync` batch share an mtime exactly. Now `(mtime, name)` reverse: newest first, then name. The operator refers to cards positionally, so a sequence that moves between renders misfiles his judgment rather than crashing. * `/b/<n>/marks.json` reported damage as empty success. `booth marks` exits 3 on an unreadable file precisely so a caller can tell "not yet" from "broken"; the HTTP mirror — the only reader a remote session has — returned the same empty list for both. It now carries `error` and `detail`. The status stays 200 deliberately: reads are lenient here, and a pinned status code is a promise to remote clients this fix has no business breaking. Each has a regression test. 410 tests.
This commit is contained in:
@@ -14,6 +14,7 @@ test names are that contract's.
|
||||
import json
|
||||
import os
|
||||
import pathlib
|
||||
import re
|
||||
import stat
|
||||
import subprocess
|
||||
import time
|
||||
@@ -996,3 +997,102 @@ def test_a_VALID_but_empty_marks_document_does_NOT_hold(tmp_path):
|
||||
|
||||
assert hold_reason(*hold_read(booth)) is None
|
||||
assert sweep_once(tmp_path, ttl_seconds=3600) == ["b"]
|
||||
|
||||
|
||||
# ---- pre-existing defects the bug-hunt panel surfaced ----------------------
|
||||
|
||||
|
||||
def test_marks_json_says_so_when_the_file_is_damaged(tmp_path):
|
||||
"""Gróa's strongest solo. `booth marks` exits 3 on an unreadable file so a
|
||||
caller can tell "not yet" from "broken"; the HTTP mirror — the ONLY reader a
|
||||
remote session has — reported the same damage as an empty success. One
|
||||
question, two surfaces, two answers, which is the thing U4 exists to stop.
|
||||
|
||||
The status stays 200 on purpose: reads are lenient here, and a pinned
|
||||
status is a promise to remote clients this fix has no business breaking.
|
||||
`test_a_corrupt_marks_file_gives_the_browser_a_409_not_a_500` pins it.
|
||||
"""
|
||||
app = create_app(tmp_path, ttl_hours=24, start_sweeper=False)
|
||||
c = TestClient(app)
|
||||
booth = tmp_path / "b"
|
||||
_touch(booth / "a.png")
|
||||
|
||||
clean = c.get("/b/b/marks.json").json()
|
||||
assert "error" not in clean
|
||||
|
||||
(booth / ".marks.json").write_text("{{{ not json")
|
||||
damaged = c.get("/b/b/marks.json")
|
||||
|
||||
assert damaged.status_code == 200
|
||||
assert "cannot be read" in damaged.json()["error"]
|
||||
assert damaged.json()["open"] == []
|
||||
|
||||
|
||||
def test_an_unreadable_links_file_does_not_take_down_the_booth_page(client):
|
||||
"""Hulda, pre-existing. `is_file()` then an unguarded `read_text()` was a
|
||||
500 waiting on a mode change or an EIO. The board is one tile on that page,
|
||||
and a page that will not load is worse than one missing a tile — the same
|
||||
posture `read_blurred`, `marks_for` and `read_manifest` already take."""
|
||||
c, data = client
|
||||
booth = data / "links"
|
||||
_touch(booth / "a.png")
|
||||
(booth / "links.md").write_text("- [x](http://x/) <sub>· y · 2026-01-01 00:00</sub>\n")
|
||||
os.chmod(booth / "links.md", 0)
|
||||
try:
|
||||
assert c.get("/b/links/").status_code == 200
|
||||
finally:
|
||||
os.chmod(booth / "links.md", 0o644)
|
||||
|
||||
|
||||
def test_the_index_order_has_a_tie_breaker(tmp_path):
|
||||
"""CLAUDE.md invariant 6. Sorting on mtime alone left equal-mtime booths in
|
||||
whatever order `iterdir()` yielded, which is not a rule — and two booths
|
||||
landed by one `rsync` batch share an mtime exactly. The operator refers to
|
||||
cards positionally, so a sequence that moves between renders misfiles his
|
||||
judgment rather than crashing."""
|
||||
when = time.time() - 100
|
||||
for n in ("charlie", "alpha", "bravo"):
|
||||
_touch(tmp_path / n / "a.png", when=when)
|
||||
os.utime(tmp_path / n, (when, when))
|
||||
|
||||
runs = {tuple(b["name"] for b in list_booths(tmp_path, ttl_seconds=86400))
|
||||
for _ in range(5)}
|
||||
|
||||
assert len(runs) == 1, "the same state must render the same sequence"
|
||||
assert runs.pop() == ("charlie", "bravo", "alpha"), "newest first, then name"
|
||||
|
||||
|
||||
def test_a_booth_name_cannot_reach_a_js_string_context(client):
|
||||
"""Hulda, solo, pre-existing and live. The confirm dialogs interpolated the
|
||||
booth NAME into a JS string literal inside `onsubmit`. Jinja's autoescape is
|
||||
HTML-attribute escaping, not JS-string escaping: the browser decodes `'`
|
||||
back to `'` before the JS parser sees it, so a booth named `'+alert(1)+'`
|
||||
executed on submit. Booth names are agent-authored — making a folder is the
|
||||
whole API — so that is a live path.
|
||||
"""
|
||||
c, data = client
|
||||
# A canary spelled so that nothing in the repo's prose can collide with it —
|
||||
# the first version of this test matched the fix's OWN comment explaining
|
||||
# what it fixed, which is a passing test measuring the wrong thing.
|
||||
hostile = "'+xssCanary7+'"
|
||||
_touch(data / hostile / "a.png")
|
||||
_touch(data / "kept-one" / "a.png")
|
||||
_touch(data / "kept-one" / KEEP_MARKER)
|
||||
|
||||
html = c.get("/").text
|
||||
|
||||
# The ATTRIBUTE form, not the bare word — the replacement's own comment
|
||||
# explains what it replaced and says "onsubmit" while doing so.
|
||||
assert "onsubmit=" not in html, "no inline handler may carry a name at all"
|
||||
assert "onclick=" not in html
|
||||
|
||||
# The name DOES appear as visible text, correctly escaped, and that is the
|
||||
# point: HTML-escaping is the right escaping for an HTML text node. What
|
||||
# must not happen is the name reaching a place where the JS parser reads it.
|
||||
scripts = re.findall(r"<script\b[^>]*>(.*?)</script>", html, re.S)
|
||||
assert scripts, "the page does ship script, so this check is not vacuous"
|
||||
for block in scripts:
|
||||
assert "xssCanary7" not in block.replace("'", "'"), "the name is in a script"
|
||||
|
||||
assert 'data-confirm="wipe"' in html, "the name travels as data, where escaping is escaping"
|
||||
assert ">'+xssCanary7+'<" in html, "and still renders as the name it is"
|
||||
|
||||
Reference in New Issue
Block a user