fix(r3): fold heid's bug hunt — no link offers a pair that 404s, NUL booth names, a FIFO marker, encoded view-state names

Navigation was built from the review ring while the compare GET also demands
containment, so an outside symlink (which stays in the ring) was offered by
the strip, the steps, the review's Compare control and the flag landing, and
404ed on arrival. Every one is now built from the compare ring (the review
ring filtered by the same conjunction, _in_booth).

Two pre-existing gaps compare inherits, fixed at the source: resolve_booth
caught only OSError, so a NUL in the booth segment was a 500; record_view
opened its marker blocking, so a planted FIFO hung every look. Plus: the page
treats %73ide=a as side=a, and the subgrid engine floor is stated. Two
findings refuted (a chorded click mid-drag never fires pointerup, measured;
booth_items never yields an unquotable rel). r3.toml: 57 rows.
This commit is contained in:
vh
2026-09-24 14:39:06 -07:00
parent 23f1bdb41f
commit f8d136a521
7 changed files with 176 additions and 40 deletions
+44 -19
View File
@@ -466,8 +466,12 @@ def record_view(booth: Path) -> None:
# the utime is not decoration: the marker must read as NOW or the whole # the utime is not decoration: the marker must read as NOW or the whole
# mechanism is a file nobody's clock looks at. # mechanism is a file nobody's clock looks at.
try: try:
# O_NONBLOCK: a FIFO planted at the marker would otherwise block this
# open forever with no reader — it cannot raise, so the swallow below
# never sees it, and the look costs the page after all (r3 heid bug
# hunt, hulda). Non-blocking, a reader-less FIFO fails with ENXIO.
fd = os.open(booth / VIEW_MARKER, fd = os.open(booth / VIEW_MARKER,
os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW, 0o644) os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW | os.O_NONBLOCK, 0o644)
try: try:
os.utime(fd) os.utime(fd)
finally: finally:
@@ -1124,7 +1128,9 @@ def create_app(
candidate = data_dir / name candidate = data_dir / name
try: try:
resolved = candidate.resolve() resolved = candidate.resolve()
except OSError: except (OSError, ValueError):
# ValueError: an embedded NUL (`/b/g%00/…`) is not an OSError, and
# hostile input is a 404, never a 500 (r3 heid bug hunt, hulda)
raise HTTPException(status_code=404, detail="no such booth") raise HTTPException(status_code=404, detail="no such booth")
# resolved.parent must be the data dir itself — blocks symlink escape + nesting. # resolved.parent must be the data dir itself — blocks symlink escape + nesting.
if resolved.parent != data_dir or not resolved.is_dir(): if resolved.parent != data_dir or not resolved.is_dir():
@@ -1547,7 +1553,8 @@ def create_app(
a, b = form.get("a"), form.get("b") a, b = form.get("a"), form.get("b")
if isinstance(a, str) and a and isinstance(b, str) and b: if isinstance(a, str) and a and isinstance(b, str) and b:
try: try:
ring = review_chain(booth_items(resolve_booth(name))) booth = resolve_booth(name)
ring = _compare_ring(booth, booth_items(booth))
except HTTPException: except HTTPException:
ring = [] ring = []
if a in ring and b in ring: if a in ring and b in ring:
@@ -1964,6 +1971,7 @@ def create_app(
members = [r for r in ring if by_rel[r].group == item.group] members = [r for r in ring if by_rel[r].group == item.group]
group = {"key": item.group, "k": members.index(f) + 1, "n": len(members)} group = {"key": item.group, "k": members.index(f) + 1, "n": len(members)}
open_now = open_marks(marks) open_now = open_marks(marks)
cring = _compare_ring(booth, items)
return templates.TemplateResponse( return templates.TemplateResponse(
request, "view.html", { request, "view.html", {
**common, **common,
@@ -1985,10 +1993,12 @@ def create_app(
"is_last": pos == len(ring) - 1, "is_last": pos == len(ring) - 1,
"tray": [x for x in film if x["flagged"]], "tray": [x for x in film if x["flagged"]],
"back_url": f"/b/{quote(name, safe='')}/#item-{item.url}", "back_url": f"/b/{quote(name, safe='')}/#item-{item.url}",
# R3 C2: this item against the NEXT in the ring (itself in a # R3 C2: this item against the NEXT in the compare ring
# ring of one), keyed by rel like every compare URL # (itself in a ring of one), keyed by rel like every compare
# URL. This item passed the containment check above, so it
# is in the compare ring.
"compare_url": (f"/b/{quote(name, safe='')}/compare?a={quote(f, safe='/')}" "compare_url": (f"/b/{quote(name, safe='')}/compare?a={quote(f, safe='/')}"
f"&b={quote(ring[(pos + 1) % len(ring)], safe='/')}"), f"&b={quote(cring[(cring.index(f) + 1) % len(cring)], safe='/')}"),
}) })
# .md renders, .txt/.log show as text — viewable in-booth, no download # .md renders, .txt/.log show as text — viewable in-booth, no download
@@ -2007,6 +2017,28 @@ def create_app(
url=f"/b/{quote(name, safe='')}/{quote(f, safe='/')}", status_code=307 url=f"/b/{quote(name, safe='')}/{quote(f, safe='/')}", status_code=307
) )
def _in_booth(booth: Path, rel) -> bool:
"""The view route's rule for a path: it resolves, stays inside the
booth, and is a file. NEVER RAISES — it is asked once per ring item."""
if not isinstance(rel, str) or not rel:
return False
try:
target = (booth / rel).resolve()
# the separator matters: a sibling booth `g-extra` shares `g`'s prefix
return str(target).startswith(str(booth) + os.sep) and target.is_file()
except (OSError, ValueError):
# ValueError: an embedded NUL — hostile input, never a 500
return False
def _compare_ring(booth: Path, items) -> list[str]:
"""THE COMPARE RING (R3 C1): the review ring — item order, media only —
filtered to what a compare can open. `booth_items` follows symlinks, so
a link pointing outside the booth is in the review ring and compare
404s it; every compare link, the review's Compare control and the flag
landing are built from THIS list, so none offers a pair that 404s (heid
bug hunt, 3 of 4)."""
return [r for r in review_chain(items) if _in_booth(booth, r)]
def _compare_side(booth: Path, ring: list[str], rel) -> str: def _compare_side(booth: Path, ring: list[str], rel) -> str:
"""One side of a compare, or a 404 (R3 C1). A CONJUNCTION: the view """One side of a compare, or a 404 (R3 C1). A CONJUNCTION: the view
route's resolve / containment / is_file check, AND membership of the route's resolve / containment / is_file check, AND membership of the
@@ -2014,14 +2046,7 @@ def create_app(
symlinks, so a link pointing outside the booth is IN the ring and only symlinks, so a link pointing outside the booth is IN the ring and only
containment refuses it. The view's check alone is not enough — it containment refuses it. The view's check alone is not enough — it
renders a doc, and compare takes media only.""" renders a doc, and compare takes media only."""
if not isinstance(rel, str) or not rel: if not _in_booth(booth, rel):
raise HTTPException(status_code=404, detail="no such item")
try:
target = (booth / rel).resolve()
except (OSError, ValueError):
# ValueError: an embedded NUL — hostile input, a 404, never a 500
raise HTTPException(status_code=404, detail="no such item")
if not str(target).startswith(str(booth) + os.sep) or not target.is_file():
raise HTTPException(status_code=404, detail="no such item") raise HTTPException(status_code=404, detail="no such item")
if rel not in ring: if rel not in ring:
raise HTTPException(status_code=404, detail="no such item") raise HTTPException(status_code=404, detail="no such item")
@@ -2043,14 +2068,14 @@ def create_app(
load, keeps them. They are `str`, not an int or a Literal, so an load, keeps them. They are `str`, not an int or a Literal, so an
unknown value reads as the default and never as a 422. unknown value reads as the default and never as a 422.
The filmstrip is the review ring in RING ORDER (the item order filtered The filmstrip is the compare ring in RING ORDER (the review's `film`:
to media — the review's `film`). the item order filtered to media, less what compare cannot open).
""" """
booth = resolve_booth(name) booth = resolve_booth(name)
items = booth_items(booth) items = booth_items(booth)
ring = review_chain(items) a = _compare_side(booth, review_chain(items), a)
a = _compare_side(booth, ring, a) b = _compare_side(booth, review_chain(items), b)
b = _compare_side(booth, ring, b) ring = _compare_ring(booth, items)
# A look records both — below the 404s, so only a real pair counts. # A look records both — below the 404s, so only a real pair counts.
record_view(booth) record_view(booth)
record_seen(booth, a, items) record_seen(booth, a, items)
+3 -1
View File
@@ -859,7 +859,9 @@
.viewer.review.compare{grid-template-rows:auto minmax(0,1fr) auto auto} .viewer.review.compare{grid-template-rows:auto minmax(0,1fr) auto auto}
/* The sides share ONE set of rows (subgrid): a caption under A takes its /* The sides share ONE set of rows (subgrid): a caption under A takes its
height from both stages alike, never from A's alone — two stages of height from both stages alike, never from A's alone — two stages of
different sizes would draw the same picture at two scales in Fit. */ different sizes would draw the same picture at two scales in Fit. The
engine floor is subgrid (Chromium 117, Firefox 71, Safari 16); an older
engine drops the rows and the sides size on their own. */
/* The separator is a 1px column GAP showing the body's background, never a /* The separator is a 1px column GAP showing the body's background, never a
border on B: a border comes out of one side's width alone, and the two border on B: a border comes out of one side's width alone, and the two
stages must be the same size to the pixel (heid code-review, hulda). */ stages must be the same size to the pixel (heid code-review, hulda). */
+4 -1
View File
@@ -110,7 +110,10 @@
var i = href.indexOf('?'); var i = href.indexOf('?');
if (i < 0) return href; if (i < 0) return href;
var parts = href.slice(i + 1).split('#')[0].split('&').filter(function (p) { var parts = href.slice(i + 1).split('#')[0].split('&').filter(function (p) {
return p && !/^(side|link)(=|$)/.test(p); /* by the DECODED name: `%73ide=a` is `side=a` to the server */
var n = p.split('=')[0];
try { n = decodeURIComponent(n.replace(/\+/g, ' ')); } catch (e) {}
return p && n !== 'side' && n !== 'link';
}); });
if (active === 'a') parts.push('side=a'); if (active === 'a') parts.push('side=a');
if (!linked) parts.push('link=0'); if (!linked) parts.push('link=0');
+11 -5
View File
@@ -57,9 +57,14 @@ assumptions:
- **A look records both.** `record_view(booth)` once, and `record_seen` for `a` - **A look records both.** `record_view(booth)` once, and `record_seen` for `a`
and then for `b`, below the 404s and gated on the records, as the view route and then for `b`, below the 404s and gated on the records, as the view route
gates it. Both calls never raise. gates it. Both calls never raise.
- **The compare ring** is the review ring filtered by that same conjunction:
item order, media only, less anything compare would 404 (an outside symlink
stays in the review ring). EVERY compare link is built from it: the strip,
the steps, the review's Compare control and the `back=compare` landing. So
no navigation offers a pair that 404s, and a step walks over such an item.
- The response carries, per side: the rel, its quoted url, ordinal, kind, - The response carries, per side: the rel, its quoted url, ordinal, kind,
caption, blurred, flagged and thumb. It also carries the ring as a filmstrip caption, blurred, flagged and thumb. It also carries the compare ring as a
in RING ORDER (the view route's `film`, one line in the route's docstring), filmstrip in RING ORDER (the view route's `film`, one line in the route's docstring),
the linked and per-side step targets (C3), the back link (the review of `a`, the linked and per-side step targets (C3), the back link (the review of `a`,
which is also where `Esc` goes), and `ord_width`. which is also where `Esc` goes), and `ord_width`.
@@ -256,9 +261,10 @@ assumptions:
- **INV-1 — rel identity.** The pair is two rels, in the URL, always. Nothing - **INV-1 — rel identity.** The pair is two rels, in the URL, always. Nothing
about the pair is stored, and no ordinal ever addresses an item. about the pair is stored, and no ordinal ever addresses an item.
- **INV-2 — ring only.** Both sides are media in the review ring. Every - **INV-2 — ring only.** Both sides are media in the review ring that pass the
server-computed link (the steps, the filmstrip, the flag landing) stays inside view route's containment. Every server-computed link (the steps, the
it. filmstrip, the review's Compare control, the flag landing) stays inside the
compare ring (C1).
- **INV-3 — no new storage and no new mark.** The judgment is the existing flag, - **INV-3 — no new storage and no new mark.** The judgment is the existing flag,
through the existing route and the existing in-place path. through the existing route and the existing in-place path.
- **INV-4 — JS-off parity.** Without JS (and so without the head script that - **INV-4 — JS-off parity.** Without JS (and so without the head script that
+53 -14
View File
@@ -10,11 +10,9 @@ label = "C1 the conjunction loses containment (an outside symlink in the ring op
file = "booth/app.py" file = "booth/app.py"
test = "tests/test_compare.py::test_an_outside_symlink_in_the_ring_is_404" test = "tests/test_compare.py::test_an_outside_symlink_in_the_ring_is_404"
old = ''' old = '''
if not str(target).startswith(str(booth) + os.sep) or not target.is_file(): return str(target).startswith(str(booth) + os.sep) and target.is_file()'''
raise HTTPException(status_code=404, detail="no such item")
if rel not in ring:'''
new = ''' new = '''
if rel not in ring:''' return target.is_file()'''
[[mutation]] [[mutation]]
label = "C1 the conjunction loses the ring (a doc or a sidecar opens as a side)" label = "C1 the conjunction loses the ring (a doc or a sidecar opens as a side)"
@@ -50,14 +48,12 @@ test = "tests/test_compare.py::test_a_look_records_both_seen"
old = ''' old = '''
booth = resolve_booth(name) booth = resolve_booth(name)
items = booth_items(booth) items = booth_items(booth)
ring = review_chain(items) a = _compare_side(booth, review_chain(items), a)'''
a = _compare_side(booth, ring, a)'''
new = ''' new = '''
booth = resolve_booth(name) booth = resolve_booth(name)
record_view(booth) record_view(booth)
items = booth_items(booth) items = booth_items(booth)
ring = review_chain(items) a = _compare_side(booth, review_chain(items), a)'''
a = _compare_side(booth, ring, a)'''
[[mutation]] [[mutation]]
label = "C6 compare does not carry data-booth (Reveal all and its restore bail)" label = "C6 compare does not carry data-booth (Reveal all and its restore bail)"
@@ -118,8 +114,8 @@ new = ''' side_a = side not in ("", "b")'''
label = "C2 the review's Compare does not wrap (the last item compares with itself)" label = "C2 the review's Compare does not wrap (the last item compares with itself)"
file = "booth/app.py" file = "booth/app.py"
test = "tests/test_compare.py::test_the_review_offers_compare_with_the_next_item" test = "tests/test_compare.py::test_the_review_offers_compare_with_the_next_item"
old = ''' f"&b={quote(ring[(pos + 1) % len(ring)], safe='/')}"),''' old = ''' f"&b={quote(cring[(cring.index(f) + 1) % len(cring)], safe='/')}"),'''
new = ''' f"&b={quote(ring[min(pos + 1, len(ring) - 1)], safe='/')}"),''' new = ''' f"&b={quote(cring[min(cring.index(f) + 1, len(cring) - 1)], safe='/')}"),'''
# ---- C5: the regions and the JS-off flag landing # ---- C5: the regions and the JS-off flag landing
@@ -480,11 +476,9 @@ label = "C1 containment is a bare prefix (a sibling booth sharing the name opens
file = "booth/app.py" file = "booth/app.py"
test = "tests/test_compare.py::test_an_outside_symlink_in_the_ring_is_404" test = "tests/test_compare.py::test_an_outside_symlink_in_the_ring_is_404"
old = ''' old = '''
if not str(target).startswith(str(booth) + os.sep) or not target.is_file(): return str(target).startswith(str(booth) + os.sep) and target.is_file()'''
raise HTTPException(status_code=404, detail="no such item")'''
new = ''' new = '''
if not str(target).startswith(str(booth)) or not target.is_file(): return str(target).startswith(str(booth)) and target.is_file()'''
raise HTTPException(status_code=404, detail="no such item")'''
[[mutation]] [[mutation]]
label = "C1 the strip is not in ring order" label = "C1 the strip is not in ring order"
@@ -511,6 +505,51 @@ test = "tests/test_compare_browser.py::test_a_flags_A_in_place_and_the_stages_su
old = '''<a class="film-f{% if x.flagged %} is-flagged{% endif %}''' old = '''<a class="film-f{% if x.flagged %} is-flagged{% endif %}'''
new = '''<a class="film-f''' new = '''<a class="film-f'''
# ---- the heid bug-hunt fold (01M3ANEPHTDMPP4Q18Z075181W)
[[mutation]]
label = "C1 navigation is built from the review ring (it offers an outside symlink that 404s)"
file = "booth/app.py"
test = "tests/test_compare.py::test_no_navigation_offers_a_pair_that_404s"
old = '''
return [r for r in review_chain(items) if _in_booth(booth, r)]'''
new = '''
return list(review_chain(items))'''
[[mutation]]
label = "a NUL in the booth segment is a 500 (ValueError is not an OSError)"
file = "booth/app.py"
test = "tests/test_compare.py::test_hostile_booth_names_are_404_not_500"
old = '''
resolved = candidate.resolve()
except (OSError, ValueError):'''
new = '''
resolved = candidate.resolve()
except OSError:'''
[[mutation]]
label = "a FIFO planted at .viewed hangs the look (a blocking open)"
file = "booth/app.py"
test = "tests/test_compare.py::test_a_planted_fifo_marker_cannot_hang_a_look"
old = '''os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW | os.O_NONBLOCK, 0o644)'''
new = '''os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW, 0o644)'''
[[mutation]]
label = "C2 an encoded view-state name survives the rewrite (%73ide=a outlives X)"
file = "booth/templates/compare.html"
test = "tests/test_compare_browser.py::test_an_encoded_view_state_name_is_still_view_state"
old = '''
return p && n !== 'side' && n !== 'link';'''
new = '''
return p && !/^(side|link)(=|$)/.test(p);'''
# Refuted, not rowed (bug hunt): "a right-click mid-drag ends the pan" — a
# second button pressed and released during a drag arrives as chorded
# `pointermove` events, never a `pointerup` (measured 3/3 in the test browser,
# the pan continuing each time). "A non-UTF-8 name 500s compare in `quote()`" —
# booth_items never yields a rel that quote() cannot encode (test_flow's
# test_ordinals_count_rendered_items_only).
#
# Accepted, not rowed: the mode's `onChange: settleAll` (re-deciding which stage # Accepted, not rowed: the mode's `onChange: settleAll` (re-deciding which stage
# can pan) is redundant with compare's ResizeObserver — 1:1 drops the stage's # can pan) is redundant with compare's ResizeObserver — 1:1 drops the stage's
# padding, so every mode change resizes the stage's content box and the # padding, so every mode change resizes the stage's content box and the
+45
View File
@@ -120,6 +120,51 @@ def test_an_outside_symlink_in_the_ring_is_404(tmp_path):
assert c.get(f"/b/g/compare?a=p.png&b={rel}").status_code == 404, rel assert c.get(f"/b/g/compare?a=p.png&b={rel}").status_code == 404, rel
def test_no_navigation_offers_a_pair_that_404s(tmp_path):
"""An outside symlink stays in the review ring, and compare 404s it. So no
compare link may offer it: not the strip, not a step, not the review's
Compare control, not the JS-off flag landing (heid bug hunt, 3 of 4)."""
b = _booth(tmp_path, "g", {"a.png": PNG, "c.png": PNG})
outside = tmp_path / "elsewhere.png"
outside.write_bytes(PNG)
(b / "b-link.png").symlink_to(outside)
c = _client(tmp_path)
body = c.get("/b/g/compare?a=a.png&b=c.png").text
assert list(_frames(body)) == ["a.png", "c.png"], _frames(body)
for h in _compare_links(body):
q = parse_qs(urlsplit(h).query)
assert "b-link.png" not in (q["a"][0], q["b"][0]), h
assert _step(body, "a-next") == ("c.png", "c.png") # steps over it
assert _compare_href(c.get("/b/g/view?f=a.png").text) == ("a.png", "c.png")
r = c.post("/b/g/flag", data={"target": "a.png", "on": "1", "back": "compare",
"a": "a.png", "b": "b-link.png"})
assert r.headers["location"] == "/b/g/#item-a.png", r.headers["location"]
def test_hostile_booth_names_are_404_not_500(tmp_path):
"""A NUL in the booth segment makes Path.resolve raise ValueError, which
is not an OSError: it must still be a 404 (heid bug hunt, hulda)."""
_four(tmp_path)
c = _client(tmp_path)
for path in ("/b/g%00/compare?a=p.png&b=q.png", "/b/g%00/view?f=p.png", "/b/g%00/"):
assert c.get(path).status_code == 404, path
def test_a_planted_fifo_marker_cannot_hang_a_look(tmp_path):
"""Recording a look never costs the page: a FIFO planted at `.viewed` must
not block the open that touches it (heid bug hunt, hulda)."""
import os
import threading
b = _four(tmp_path)
os.mkfifo(b / ".viewed")
got = []
t = threading.Thread(target=lambda: got.append(
_client(tmp_path).get("/b/g/compare?a=p.png&b=q.png").status_code), daemon=True)
t.start()
t.join(10)
assert got == [200], "a planted FIFO held the look open"
def test_a_look_records_both_seen(tmp_path): def test_a_look_records_both_seen(tmp_path):
"""A compare GET is a look at both sides; a 404 records nothing.""" """A compare GET is a look at both sides; a 404 records nothing."""
import json import json
+16
View File
@@ -419,6 +419,22 @@ def test_a_save_keeps_the_active_side(browser, live):
assert picked == ("m-2-forge-s23.png", "r-1-dock-s11.png") and side == "a", (picked, side) assert picked == ("m-2-forge-s23.png", "r-1-dock-s11.png") and side == "a", (picked, side)
def test_an_encoded_view_state_name_is_still_view_state(browser, live):
"""`%73ide=a` IS `side=a` to the server, so the page must treat it as view
state too: after X, a reload shows the side X chose, not the stale one
the encoded parameter still named (heid bug hunt, hulda)."""
base, root = live
_bakeoff(root)
page = browser.new_page(viewport={"width": 1440, "height": 900})
_open(page, f"{base}/b/g/compare?a=m-1-dock-s11.png&b=r-1-dock-s11.png&%73ide=a")
first = page.evaluate("document.querySelector('.cmp-side.is-active').dataset.side")
page.keyboard.press("x")
page.reload(wait_until="networkidle")
after = page.evaluate("document.querySelector('.cmp-side.is-active').dataset.side")
page.close()
assert (first, after) == ("a", "b"), (first, after)
def test_without_js_every_judgment_and_step_still_works(browser, live): def test_without_js_every_judgment_and_step_still_works(browser, live):
"""INV-4. Scripts off: the pair renders in two Fit stages, the step and """INV-4. Scripts off: the pair renders in two Fit stages, the step and
strip links navigate, both flag forms are there and a flag lands back on strip links navigate, both flag forms are there and a flag lands back on