From 751eecb7713ef638efdcb093649290c3daa660a3 Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Mon, 21 Sep 2026 08:40:26 -0700 Subject: [PATCH] fix(booth): the reveal button was inert; add kept-lane wipe and in-booth keep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three operator reports, one of them a real defect I had claimed was working. THE REVEAL BUTTON DID NOTHING, for a day. Its handler sat after the content block's closing tag, and a child template's out-of-block content is silently DISCARDED by Jinja. The button rendered. The handler never reached the browser. Two commits and a README paragraph said click-to-reveal worked, and the suite passed the entire time because nothing asserted against the SERVED page -- the template really did contain the code. Two guards, both confirmed to FAIL when the defect is reintroduced rather than merely added and assumed protective: * test_reveal_handler_actually_reaches_the_served_page greps the response * test_no_orphaned_markup_after_the_content_block guards the structure While moving it, caught a second instance of the same class: the explanatory comment I wrote for the fix contained a literal Jinja endblock tag, which Jinja would have parsed as a real tag and used to close the block early. KEPT-LANE ร—. Wiping a kept booth required release-then-find-it-in-the-other- lane. That protected nothing and cost a hunt -- the board you just released is loose in a feed that turns over, and you have to go find it to finish a job you had already decided on. Protection now lives in the confirmation, which names the booth and says KEPT, instead of in the number of lanes you must traverse. Release stays as the reversible option. IN-BOOTH KEEP. `โ˜† keep` / `โ˜… kept โ€” release` beside "Wipe now", so promoting does not mean navigating back to the index. The booth page did not previously know its own kept state; it does now. Both post a `next` field to stay put -- and `next` is a form field, so it is attacker-controlled: only same-site absolute paths are honoured, with `//host`, schemes and backslashes refused, tested. 173 tests pass. --- services/booth/README.md | 31 +++++++- services/booth/booth/app.py | 20 ++++- services/booth/booth/templates/base.html | 14 ++++ services/booth/booth/templates/booth.html | 30 ++++++-- services/booth/booth/templates/index.html | 29 ++++--- services/booth/tests/test_booth.py | 92 +++++++++++++++++++++++ 6 files changed, 194 insertions(+), 22 deletions(-) diff --git a/services/booth/README.md b/services/booth/README.md index 693f0ca..0abb538 100644 --- a/services/booth/README.md +++ b/services/booth/README.md @@ -70,6 +70,16 @@ figures across mixed kinds and will fail if you don't. tells the truth about whether anything here is blurred. - **Reveal is per-viewer and never persisted.** Click ๐Ÿ‘ reveal; a reload re-hides. With JS off it stays blurred, which is the safe direction to fail. + + โš  This button shipped INERT on 2026-09-20 and stayed that way for a day. Its + handler sat after the content block's closing tag, and a child template's + out-of-block content is silently DISCARDED by Jinja โ€” the button rendered, + the handler never reached the browser, and two commits plus this README said + it worked. The suite passed throughout because nothing asserted against the + served page. `test_reveal_handler_actually_reaches_the_served_page` now greps + the HTTP RESPONSE, and `test_no_orphaned_markup_after_the_content_block` + guards the structure. Both were confirmed to FAIL when the defect is + reintroduced, which is the only way to know a guard guards anything. - **Covers inherit it.** If a booth's cover image is blurred, the index card's thumb is blurred too โ€” otherwise the front page undoes the censoring. - **Inline docs are blurred too**, not just images and video. That branch puts @@ -77,8 +87,25 @@ figures across mixed kinds and will fail if you don't. ## Keeping a booth (round trip, both directions) -`โ˜…` on an ephemeral card promotes it to the kept lane; `release` in the kept -lane sends it back. Equivalent CLI: `booth keep ` / `booth unkeep `. +Three places, all doing the same thing: + +- **Index, ephemeral card** โ€” `โ˜…` promotes to the kept lane. +- **Index, kept card** โ€” `release` demotes, and `ร—` now WIPES DIRECTLY. The old + rule was release-then-find-it-in-the-other-lane; that protected nothing and + cost a hunt, because the board you just released is loose in a feed that + turns over. Protection lives in the confirmation now, which names the booth + and says KEPT. +- **Inside a booth** โ€” `โ˜† keep` / `โ˜… kept โ€” release`, beside *Wipe now*, so you + do not have to go back to the index. These post a `next` field to stay on the + page; `next` is a form field and therefore attacker-controlled, so only + same-site absolute paths are honoured (`//host`, schemes and backslashes are + refused). + +Equivalent CLI: `booth keep ` / `booth unkeep `. + +โš  Release BUMPS the directory mtime, so a released board's age resets and it +survives another full TTL. Unkeep-and-wait is a 24h delay, not a delete โ€” which +is exactly why the direct `ร—` was worth adding. โš  Until 2026-09-19 the UI only went one way โ€” the kept lane could release, but an ephemeral booth could only be kept from a shell. The `/keep` route and the diff --git a/services/booth/booth/app.py b/services/booth/booth/app.py index 624bccb..4f59e32 100644 --- a/services/booth/booth/app.py +++ b/services/booth/booth/app.py @@ -704,6 +704,9 @@ def create_app( **base_ctx, "name": name, "name_url": quote(name, safe=""), + # The page could not previously tell keep from release, so it + # offered neither and you had to go back to the index. + "kept": is_kept(booth), # links.md is rendered AS the board below, so it must not also # appear as a markdown doc tile โ€” that would show the same # content twice, once interactive and once not. @@ -992,16 +995,25 @@ def create_app( toggle_pin(resolve_booth(name), entry) return RedirectResponse(url=f"/b/{quote(name, safe='')}/", status_code=303) + def _safe_next(nxt: str) -> str: + """Where to land after keep/unkeep. Defaults to the index; a booth page + can ask to stay put. Only same-site absolute paths are honoured โ€” `//` + and any scheme are refused, because a redirect target taken from a form + field is an open redirect if you do not check it.""" + if nxt.startswith("/") and not nxt.startswith("//") and "\\" not in nxt: + return nxt + return "/" + @app.post("/b/{name}/keep") - def booth_keep(name: str): + def booth_keep(name: str, next: str = Form("/")): (resolve_booth(name) / KEEP_MARKER).touch() - return RedirectResponse(url="/", status_code=303) + return RedirectResponse(url=_safe_next(next), status_code=303) @app.post("/b/{name}/unkeep") - def booth_unkeep(name: str): + def booth_unkeep(name: str, next: str = Form("/")): # missing_ok: releasing an already-released board is a no-op, not a 500. (resolve_booth(name) / KEEP_MARKER).unlink(missing_ok=True) - return RedirectResponse(url="/", status_code=303) + return RedirectResponse(url=_safe_next(next), status_code=303) @app.post("/b/{name}/blur") def booth_blur(name: str, f: str = Form(...), on: str = Form("1")): diff --git a/services/booth/booth/templates/base.html b/services/booth/booth/templates/base.html index 5b77584..ffc6bef 100644 --- a/services/booth/booth/templates/base.html +++ b/services/booth/booth/templates/base.html @@ -253,6 +253,20 @@ /* โ˜… keep, mirroring .wipe on the other shoulder of the card. Same hover-to-reveal language as .release in the kept lane. */ .keepit{position:absolute;top:.5rem;left:.5rem;margin:0;opacity:0;transition:opacity .12s} + /* ร— on a KEPT card. Same shoulder as the ephemeral ร—, deliberately tinted so + it does not read as the same weight of action. */ + .wipe-kept{position:absolute;top:.5rem;right:.5rem;margin:0;opacity:0;transition:opacity .12s} + .card-kept:hover .wipe-kept,.wipe-kept:focus-within{opacity:1} + .wipe-kept button{font:inherit;line-height:1;cursor:pointer;border:1px solid var(--line); + border-radius:.3rem;padding:.02rem .3rem;background:var(--bg);color:var(--muted)} + .wipe-kept button:hover{background:var(--aus-red,#ff6b6b);color:var(--fg-on-accent,#fff); + border-color:var(--aus-red,#ff6b6b)} + /* keep / release from inside a booth, beside "Wipe now" */ + .keep-lg{display:inline-block;margin:0 .4rem 0 0} + .keep-lg button{font:inherit;font-size:.8rem;line-height:1;padding:.32rem .6rem; + cursor:pointer;border:1px solid var(--line);border-radius:.3rem; + background:var(--bg);color:var(--fg)} + .keep-lg button:hover{background:var(--aus-blue);color:var(--fg-on-accent)} .card:hover .keepit,.keepit:focus-within{opacity:1} .keepit button{font:inherit;font-size:.9rem;line-height:1;padding:.1rem .34rem; cursor:pointer;border:1px solid var(--line);border-radius:.3rem; diff --git a/services/booth/booth/templates/booth.html b/services/booth/booth/templates/booth.html index 9830a0f..4958aea 100644 --- a/services/booth/booth/templates/booth.html +++ b/services/booth/booth/templates/booth.html @@ -23,6 +23,19 @@ {# A durable multi-writer board gets no one-click wipe โ€” same rule as the kept lane on the index. Remove rows with the per-row ร—, or release the board from the index and wipe it from there. #} + {# Promote or release without going back to the index. `next` keeps you on + this page instead of bouncing you to /. #} + {% if kept %} +
+ + +
+ {% else %} +
+ + +
+ {% endif %} {% if not board %}
@@ -285,17 +298,22 @@ refresh(); })(); - -{% endblock %} - +{% endblock %} diff --git a/services/booth/booth/templates/index.html b/services/booth/booth/templates/index.html index efabb84..3e58a2b 100644 --- a/services/booth/booth/templates/index.html +++ b/services/booth/booth/templates/index.html @@ -40,17 +40,26 @@ {{ b.name }}
{{ b.count }} item{{ '' if b.count == 1 else 's' }} ยท kept ยท โฌ‡ zip
- {# Still no ร— here โ€” a one-click wipe next to the durable stuff is a - footgun. But "deliberate" must not mean "impossible from the UI", - which is what it meant before: the only routes out were ssh or a - hand-written API call. Release drops the sentinel and the board moves - to the ephemeral lane, where the ร— already lives. Two deliberate - acts, both reachable, and the first one is reversible. + {# There IS a ร— here now (operator, 2026-09-21). The old rule was + release-then-find-it-in-the-other-lane, on the theory that two + deliberate acts protect durable boards. In practice it protects + nothing and costs a hunt: the board you just released is loose in a + feed that turns over, and you have to go find it to finish the job + you had already decided on. - The confirm says "wipe it from there" rather than "let it expire" on - purpose: releasing BUMPS the directory mtime, so the board's age - resets and it survives another full TTL. Unkeep-and-wait is a 24h - delay, not a delete. #} + The protection now lives in the CONFIRMATION, not in the number of + lanes you must traverse โ€” this one names the booth and says the word + KEPT, where the ephemeral ร— just asks. A deliberate act, one click, + reachable. + + Release still exists and is still the reversible option. Note it + BUMPS the directory mtime, so the board's age resets and it survives + another full TTL โ€” unkeep-and-wait is a 24h delay, not a delete, + which is exactly why a direct ร— was worth adding. #} + + +
diff --git a/services/booth/tests/test_booth.py b/services/booth/tests/test_booth.py index ecc89e4..47ab88c 100644 --- a/services/booth/tests/test_booth.py +++ b/services/booth/tests/test_booth.py @@ -1487,3 +1487,95 @@ def test_blur_toggle_posts_the_opposite_state(client): body = c.get("/b/bo/").text assert 'name="on" value="0"' in body, "a blurred item must offer un-blur" assert "โ—‰ blurred" in body + + +# --------------------------------------------------------------------------- +# Operator-reported, 2026-09-21: "the reveal button doesn't do anything". +# It rendered and was inert โ€” the handler sat after {%- endblock -%} in a child +# template, which Jinja DISCARDS. Two commits and a README claimed +# click-to-reveal worked. The suite passed the whole time because nothing +# asserted against the SERVED page. +# --------------------------------------------------------------------------- + + +def test_reveal_handler_actually_reaches_the_served_page(client): + """The regression that matters. Assert the handler is IN THE RESPONSE, not + that the template file contains the text โ€” the template contained it fine + and the browser never saw it.""" + c, root = client + d = root / "bo" + d.mkdir() + _png(d / "x.png") + set_blurred(d, "x.png", True) + + body = c.get("/b/bo/").text + assert 'class="reveal"' in body, "the button must render" + assert "classList.toggle('revealed')" in body, ( + "the handler must reach the page โ€” a button with no handler is a dead " + "control, which is exactly what shipped" + ) + + +def test_no_orphaned_markup_after_the_content_block(client): + """Structural guard for the same defect class: anything a child template + puts outside a block is silently dropped, so the only safe number of + closing content-block tags is one, at the very end.""" + tpl = (pathlib.Path(__file__).parent.parent + / "booth" / "templates" / "booth.html").read_text() + after = tpl[tpl.rindex("{% endblock %}") + len("{% endblock %}"):] + assert after.strip() == "", ( + f"content after the final endblock is discarded by Jinja: {after[:120]!r}" + ) + + +def test_booth_page_offers_keep_when_ephemeral_and_release_when_kept(client): + """Operator: 'adding a keep button inside a booth'. Both states, because a + control that always says the same thing cannot be driving off real state.""" + c, root = client + d = root / "bo" + d.mkdir() + _png(d / "x.png") + + body = c.get("/b/bo/").text + assert "โ˜† keep" in body and "release" not in body.split("boothhead")[1][:900] + + c.post("/b/bo/keep", data={"next": "/b/bo/"}, follow_redirects=False) + body = c.get("/b/bo/").text + assert "โ˜… kept โ€” release" in body + + +def test_keep_from_inside_a_booth_stays_on_the_booth_page(client): + """Without `next` the route redirects to /, which throws you out of the + booth you were reading.""" + c, root = client + (root / "bo").mkdir() + r = c.post("/b/bo/keep", data={"next": "/b/bo/"}, follow_redirects=False) + assert r.headers["location"] == "/b/bo/" + r = c.post("/b/bo/keep", follow_redirects=False) # no next + assert r.headers["location"] == "/" + + +def test_next_refuses_an_open_redirect(client): + """`next` comes from a form field, so it is attacker-controlled input.""" + c, root = client + (root / "bo").mkdir() + for evil in ("//evil.example/x", "https://evil.example/x", "\\\\evil"): + r = c.post("/b/bo/keep", data={"next": evil}, follow_redirects=False) + assert r.headers["location"] == "/", f"{evil!r} must not be honoured" + + +def test_kept_lane_offers_a_direct_wipe_beside_release(client): + """Operator: 'allow an x to delete next to release so i don't have to + release and then find it to delete it.'""" + c, root = client + d = root / "bo" + d.mkdir() + _png(d / "x.png") + (d / KEEP_MARKER).touch() + + body = c.get("/").text + assert 'class="wipe wipe-kept"' in body, "kept card must offer a direct ร—" + assert 'action="/b/bo/unkeep"' in body, "release must still be there too" + # ...and it actually wipes. + c.post("/b/bo/delete", follow_redirects=False) + assert not d.exists()