From f8d136a521eb3597c5e301f622ab6de4ef564f94 Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Thu, 24 Sep 2026 14:39:06 -0700 Subject: [PATCH] =?UTF-8?q?fix(r3):=20fold=20heid's=20bug=20hunt=20?= =?UTF-8?q?=E2=80=94=20no=20link=20offers=20a=20pair=20that=20404s,=20NUL?= =?UTF-8?q?=20booth=20names,=20a=20FIFO=20marker,=20encoded=20view-state?= =?UTF-8?q?=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- booth/app.py | 63 +++++++++++++++++-------- booth/templates/base.html | 4 +- booth/templates/compare.html | 5 +- docs/contracts/r3_compare.contract.md | 16 +++++-- tests/mutations/r3.toml | 67 +++++++++++++++++++++------ tests/test_compare.py | 45 ++++++++++++++++++ tests/test_compare_browser.py | 16 +++++++ 7 files changed, 176 insertions(+), 40 deletions(-) diff --git a/booth/app.py b/booth/app.py index 7555bcc..4184932 100644 --- a/booth/app.py +++ b/booth/app.py @@ -466,8 +466,12 @@ def record_view(booth: Path) -> None: # the utime is not decoration: the marker must read as NOW or the whole # mechanism is a file nobody's clock looks at. 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, - os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW, 0o644) + os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW | os.O_NONBLOCK, 0o644) try: os.utime(fd) finally: @@ -1124,7 +1128,9 @@ def create_app( candidate = data_dir / name try: 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") # resolved.parent must be the data dir itself — blocks symlink escape + nesting. 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") if isinstance(a, str) and a and isinstance(b, str) and b: try: - ring = review_chain(booth_items(resolve_booth(name))) + booth = resolve_booth(name) + ring = _compare_ring(booth, booth_items(booth)) except HTTPException: 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] group = {"key": item.group, "k": members.index(f) + 1, "n": len(members)} open_now = open_marks(marks) + cring = _compare_ring(booth, items) return templates.TemplateResponse( request, "view.html", { **common, @@ -1985,10 +1993,12 @@ def create_app( "is_last": pos == len(ring) - 1, "tray": [x for x in film if x["flagged"]], "back_url": f"/b/{quote(name, safe='')}/#item-{item.url}", - # R3 C2: this item against the NEXT in the ring (itself in a - # ring of one), keyed by rel like every compare URL + # R3 C2: this item against the NEXT in the compare ring + # (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='/')}" - 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 @@ -2007,6 +2017,28 @@ def create_app( 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: """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 @@ -2014,14 +2046,7 @@ def create_app( 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 renders a doc, and compare takes media only.""" - if not isinstance(rel, str) or not 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(): + if not _in_booth(booth, rel): raise HTTPException(status_code=404, detail="no such item") if rel not in ring: 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 unknown value reads as the default and never as a 422. - The filmstrip is the review ring in RING ORDER (the item order filtered - to media — the review's `film`). + The filmstrip is the compare ring in RING ORDER (the review's `film`: + the item order filtered to media, less what compare cannot open). """ booth = resolve_booth(name) items = booth_items(booth) - ring = review_chain(items) - a = _compare_side(booth, ring, a) - b = _compare_side(booth, ring, b) + a = _compare_side(booth, review_chain(items), a) + b = _compare_side(booth, review_chain(items), b) + ring = _compare_ring(booth, items) # A look records both — below the 404s, so only a real pair counts. record_view(booth) record_seen(booth, a, items) diff --git a/booth/templates/base.html b/booth/templates/base.html index 986768a..1a34f85 100644 --- a/booth/templates/base.html +++ b/booth/templates/base.html @@ -859,7 +859,9 @@ .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 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 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). */ diff --git a/booth/templates/compare.html b/booth/templates/compare.html index 4c574ed..c18ffbf 100644 --- a/booth/templates/compare.html +++ b/booth/templates/compare.html @@ -110,7 +110,10 @@ var i = href.indexOf('?'); if (i < 0) return href; 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 (!linked) parts.push('link=0'); diff --git a/docs/contracts/r3_compare.contract.md b/docs/contracts/r3_compare.contract.md index 63ea0bf..6d3f60f 100644 --- a/docs/contracts/r3_compare.contract.md +++ b/docs/contracts/r3_compare.contract.md @@ -57,9 +57,14 @@ assumptions: - **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 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, - caption, blurred, flagged and thumb. It also carries the ring as a filmstrip - in RING ORDER (the view route's `film`, one line in the route's docstring), + caption, blurred, flagged and thumb. It also carries the compare ring as a + 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`, 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 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 - server-computed link (the steps, the filmstrip, the flag landing) stays inside - it. +- **INV-2 — ring only.** Both sides are media in the review ring that pass the + view route's containment. Every server-computed link (the steps, the + 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, through the existing route and the existing in-place path. - **INV-4 — JS-off parity.** Without JS (and so without the head script that diff --git a/tests/mutations/r3.toml b/tests/mutations/r3.toml index 3dd35b1..b30c0c2 100644 --- a/tests/mutations/r3.toml +++ b/tests/mutations/r3.toml @@ -10,11 +10,9 @@ label = "C1 the conjunction loses containment (an outside symlink in the ring op file = "booth/app.py" test = "tests/test_compare.py::test_an_outside_symlink_in_the_ring_is_404" old = ''' - if not str(target).startswith(str(booth) + os.sep) or not target.is_file(): - raise HTTPException(status_code=404, detail="no such item") - if rel not in ring:''' + return str(target).startswith(str(booth) + os.sep) and target.is_file()''' new = ''' - if rel not in ring:''' + return target.is_file()''' [[mutation]] 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 = ''' booth = resolve_booth(name) items = booth_items(booth) - ring = review_chain(items) - a = _compare_side(booth, ring, a)''' + a = _compare_side(booth, review_chain(items), a)''' new = ''' booth = resolve_booth(name) record_view(booth) items = booth_items(booth) - ring = review_chain(items) - a = _compare_side(booth, ring, a)''' + a = _compare_side(booth, review_chain(items), a)''' [[mutation]] 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)" file = "booth/app.py" test = "tests/test_compare.py::test_the_review_offers_compare_with_the_next_item" -old = ''' f"&b={quote(ring[(pos + 1) % len(ring)], safe='/')}"),''' -new = ''' f"&b={quote(ring[min(pos + 1, len(ring) - 1)], safe='/')}"),''' +old = ''' f"&b={quote(cring[(cring.index(f) + 1) % len(cring)], safe='/')}"),''' +new = ''' f"&b={quote(cring[min(cring.index(f) + 1, len(cring) - 1)], safe='/')}"),''' # ---- 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" test = "tests/test_compare.py::test_an_outside_symlink_in_the_ring_is_404" old = ''' - if not str(target).startswith(str(booth) + os.sep) or not target.is_file(): - raise HTTPException(status_code=404, detail="no such item")''' + return str(target).startswith(str(booth) + os.sep) and target.is_file()''' new = ''' - if not str(target).startswith(str(booth)) or not target.is_file(): - raise HTTPException(status_code=404, detail="no such item")''' + return str(target).startswith(str(booth)) and target.is_file()''' [[mutation]] 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 = '''