diff --git a/hullwork/cli.py b/hullwork/cli.py index e9fdc1d..d93dd00 100644 --- a/hullwork/cli.py +++ b/hullwork/cli.py @@ -34,6 +34,7 @@ db, doctor, lease, + operator, outcomes, page, propose, @@ -44,6 +45,7 @@ triage, work, ) +from hullwork import decisions as decide from hullwork import upstream as upstream_module from hullwork.config import ConfigError, Settings, get_settings from hullwork.credentials import PushCapability @@ -897,37 +899,25 @@ def _cmd_rotate( def approve(session: Session, slug: str, item_id: int) -> Item: """Let an agent attempt one amber item. One item, named explicitly, by a human. - A command rather than an endpoint, for the reason registration is one: the operator already has - the host, and an approval endpoint would be a permanent attack surface for something done by one - person a handful of times. There is deliberately no `--all`. + **The decision itself moved to `decisions.py` in item 166**, because the page grew a button and + a route would otherwise import a command from here — the shape item 162 removed from `sandbox/`. + What stays is this signature, which takes a slug because a person types a slug, and the mapping + from the decision's refusal to this module's. + + The comment that used to live here argued that approval should be a command and never an + endpoint: *"the operator already has the host, and an approval endpoint would be a permanent + attack surface for something done by one person a handful of times."* Item 166 reversed it, with + the ground it stood on: the operator no longer *has* the host in any useful sense — reading the + page happens on a laptop and acting meant SSH, `docker compose exec` and this command, which + measured out at two items waiting twenty-one hours on an instance somebody was looking at. The + attack surface is still real, which is why the endpoint needs a second credential that never + appears in a URL. """ - project = _require(session, slug) - item = ( - session.query(Item) - .filter(Item.id == item_id, Item.project_id == project.id) - .one_or_none() - ) - if item is None: - raise CommandError(f"'{slug}' has no item {item_id}") - - if item.state is not ItemState.WAITING_APPROVAL: - # Naming the state it found is the difference between a refusal and a puzzle. An item that - # is already `ready`, or that a human closed, is the common case here. - raise CommandError( - f"item {item_id} is '{item.state.value}', not '{ItemState.WAITING_APPROVAL.value}' — " - f"only an item waiting for approval can be approved" - ) - try: - transition(item, ItemState.READY) - except IllegalTransitionError as exc: - # Red reaches here only if a manifest was edited underneath a queued item. The state machine - # refuses it whatever this command thinks, which is the point of enforcing it there. + return decide.approve(session, _require(session, slug), item_id) + except decide.DecisionError as exc: raise CommandError(str(exc)) from exc - session.commit() - return item - def requeue(session: Session, slug: str, item_id: int) -> Item: """Put a `human-only` item back in the queue when what stopped it was the environment. Item 093. @@ -1211,6 +1201,49 @@ def _cmd_page_token( return 0 +def _cmd_operator_key( + args: argparse.Namespace, session: Session, settings: Settings, out: TextIO +) -> int: + """Mint the credential that **acts** on the read-only page. Item 166. + + **A second credential rather than a promotion of the first**, and the difference is the whole + security model: the page token is a bearer string in a URL — a saved page, a screenshot of the + address bar, a link mailed to a colleague — so it reads everything and may never spend money. + This one is pasted into a form once and exchanged for a session cookie, so it never lands + anywhere a URL lands. + + Refuses to replace an existing key without `--rotate`, for the reason `page-token` does: a + second person running this to "get in" would lock out the first, and the failure would read as + the buttons being broken rather than as a key having changed underneath them. + """ + existing = operator.configured(session) + if existing and not args.rotate: + raise CommandError( + "this instance already has an operator key, and it cannot be shown again — it was " + "printed once and only its hash is stored.\n" + " To replace it: hullwork operator-key --rotate. Every session open right now ends " + "the moment you do." + ) + + key = operator.issue_key(session) + print("Rotated. Every session that was open has ended." if existing else + "The page can now be acted on.", file=out) + print("\n This key is shown once and cannot be recovered:\n", file=out) + print(f" {key}\n", file=out) + print( + " Paste it into the page's login, once per browser. Unlike the page URL it is **not** a\n" + " link and must never become one: it is the difference between somebody reading this\n" + " instance and somebody spending its budget.\n" + "\n" + " What a session may then do: approve one amber item, or hand one to a human. Nothing\n" + " else on the page changes anything, and there is no approve-everything.\n" + "\n" + " Sessions last 12 hours. To end them all at once, rotate.", + file=out, + ) + return 0 + + def _cmd_status( args: argparse.Namespace, session: Session, settings: Settings, out: TextIO ) -> int: @@ -3036,6 +3069,26 @@ def build_parser() -> argparse.ArgumentParser: ) page_token.set_defaults(func=_cmd_page_token) + operator_key = subparsers.add_parser( + "operator-key", + help="mint the credential that acts on the read-only page", + description=( + "The page reads with a token in its URL, which is why it may not act: a URL is a thing " + "that gets saved, screenshotted and forwarded. This mints a second credential that " + "never appears in a URL — pasted into a login once per browser, exchanged for a " + "session cookie — and it is what the two buttons on an amber item require.\n\n" + "Until this command runs there are no buttons, and every route that would change " + "something answers 404 the way an unknown path does.\n\n" + "Shown once, stored as a hash." + ), + ) + operator_key.add_argument( + "--rotate", + action="store_true", + help="replace the existing key, ending every session that is open right now", + ) + operator_key.set_defaults(func=_cmd_operator_key) + pruning = subparsers.add_parser( "prune", help="forget the raw bodies of old deliveries, keeping every row" ) diff --git a/hullwork/decisions.py b/hullwork/decisions.py new file mode 100644 index 0000000..39700e6 --- /dev/null +++ b/hullwork/decisions.py @@ -0,0 +1,98 @@ +"""The two decisions a human makes about an amber item, and nothing else. Item 166. + +**Here because two callers need them and neither should own them.** `approve` lived in `cli.py`, and +the route that item 166 added would have had to import it from there — a module that owns a command, +imported by a module that owns a route. That is the exact shape item 162 spent an item removing from +`sandbox/`, for the reason that survived it: a name two modules reach for does not belong to either. + +Both take the **project**, already looked up, rather than a slug. The lookup is the caller's +business — `cli` refuses with an exit code, a route refuses with a status — and passing the slug +would have dragged one of those vocabularies into the other. + +The pair is deliberately small and deliberately closed. `LEGAL[WAITING_APPROVAL]` is +`{READY, HUMAN_ONLY, DONE}`, and the third is not here: an item a human closes by hand is closed in +the forge, where the issue is, and the sweep reads it back. What a human decides *about an attempt* +is only ever these two — let it try, or take it away from it. +""" + +from __future__ import annotations + +from sqlalchemy.orm import Session + +from hullwork.models import Item, ItemState, Project +from hullwork.states import IllegalTransitionError, transition + + +class DecisionError(Exception): + """The decision cannot be made, and the message says which state was found instead. + + Naming the state is the difference between a refusal and a puzzle: an item already `ready`, or + one a human closed last week, is the common case at this door rather than the exception. + """ + + +def _the_item(session: Session, project: Project, item_id: int) -> Item: + item = ( + session.query(Item) + .filter(Item.id == item_id, Item.project_id == project.id) + .one_or_none() + ) + if item is None: + msg = f"'{project.slug}' has no item {item_id}" + raise DecisionError(msg) + return item + + +def _move(item: Item, target: ItemState, *, only_from: ItemState, verb: str) -> Item: + if item.state is not only_from: + msg = ( + f"item {item.id} is '{item.state.value}', not '{only_from.value}' — " + f"only an item waiting for approval can be {verb}" + ) + raise DecisionError(msg) + try: + transition(item, target) + except IllegalTransitionError as exc: + # Red reaches here only if a manifest was edited underneath a queued item. The state machine + # refuses it whatever this function thinks, which is the point of enforcing it there. + raise DecisionError(str(exc)) from exc + return item + + +def approve(session: Session, project: Project, item_id: int) -> Item: + """Let an agent attempt one amber item. One item, named explicitly, by a human. + + **There is deliberately no `--all` and no equivalent.** One approval is one attempt, which costs + money and opens a pull request somebody has to read; a button that approves a queue is a button + that spends a budget. + """ + item = _move( + _the_item(session, project, item_id), + ItemState.READY, + only_from=ItemState.WAITING_APPROVAL, + verb="approved", + ) + session.commit() + return item + + +def hand_to_human(session: Session, project: Project, item_id: int) -> Item: + """Take an amber item away from the agent: a person will do this one. + + **Not `rejected`, and the state machine is why.** `LEGAL[WAITING_APPROVAL]` does not contain + `REJECTED` — that state means *a reviewer closed a pull request*, and it feeds + `counted.rejected` keyed by the reason on that pull request's labels. Calling this "reject" + would file a decision about **whether to attempt** into the tally that counts **review** + decisions, and the number would drift with nobody able to see why. + + `human-only` is the honest name and it already existed: it is what a lane says when the code + location is somewhere an agent is not allowed to go. + """ + item = _move( + _the_item(session, project, item_id), + ItemState.HUMAN_ONLY, + only_from=ItemState.WAITING_APPROVAL, + verb="handed to a human", + ) + session.commit() + return item diff --git a/hullwork/main.py b/hullwork/main.py index a3a94f9..8ba3cf1 100644 --- a/hullwork/main.py +++ b/hullwork/main.py @@ -5,18 +5,21 @@ from collections.abc import AsyncIterator, Iterator from contextlib import asynccontextmanager, suppress from typing import Annotated, Any, Literal +from urllib.parse import parse_qsl -from fastapi import Depends, FastAPI, HTTPException, Response, status +from fastapi import Depends, FastAPI, HTTPException, Query, Request, Response, status from fastapi.responses import HTMLResponse, RedirectResponse from pydantic import BaseModel from sqlalchemy.orm import Session, sessionmaker -from hullwork import __version__, page, readiness +from hullwork import __version__, operator, page, readiness +from hullwork import decisions as decide from hullwork.config import ConfigError, Settings, get_settings from hullwork.db import get_engine, make_session_factory from hullwork.forge.factory import make_forge from hullwork.ingest import sweep from hullwork.logging import configure_logging +from hullwork.models import Item from hullwork.readiness import record_sweep_ok from hullwork.telemetry import ( configure_error_reporting, @@ -300,6 +303,7 @@ def page_instance( ) def page_instance_index( token: str, + request: Request, session: Annotated[Session, Depends(_readiness_session)], settings: Annotated[Settings, Depends(get_settings)], ) -> HTMLResponse: @@ -307,7 +311,12 @@ def page_instance_index( if not page.opens(session, token): raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") return HTMLResponse( - page.instance(session, settings, error_reporting=_reporting_enabled), + page.instance( + session, + settings, + error_reporting=_reporting_enabled, + acting=_acting(session, request), + ), headers=page.HEADERS, ) @@ -320,12 +329,16 @@ def page_instance_index( ) def page_items( token: str, + request: Request, session: Annotated[Session, Depends(_readiness_session)], + in_: Annotated[str | None, Query(alias="in")] = None, ) -> HTMLResponse: """Every item, most recent first. Item 123.""" if not page.opens(session, token): raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") - return HTMLResponse(page.items(session), headers=page.HEADERS) + return HTMLResponse( + page.items(session, only=in_, acting=_acting(session, request)), headers=page.HEADERS + ) @app.get( @@ -380,6 +393,7 @@ def page_project( def page_item( token: str, item_id: int, + request: Request, session: Annotated[Session, Depends(_readiness_session)], settings: Annotated[Settings, Depends(get_settings)], ) -> HTMLResponse: @@ -391,7 +405,199 @@ def page_item( """ if not page.opens(session, token): raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") - rendered = page.item(session, settings, item_id) + rendered = page.item(session, settings, item_id, acting=_acting(session, request)) if rendered is None: raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") return HTMLResponse(rendered, headers=page.HEADERS) + + +#: The most a form on this page can be. Both fields are 43-character tokens; anything approaching +#: this is not a browser filling in the login. +_FORM_CEILING = 4096 + + +async def _field(request: Request, name: str) -> str | None: + """One field out of an `application/x-www-form-urlencoded` body, or `None`. + + **Hand-parsed to keep a dependency out of the receiver.** FastAPI's `Form()` requires + `python-multipart`, and this is the half of Hullwork that listens on a network — every package + it imports is surface. The forms here are written in this file and post urlencoded, which + `urllib.parse` has always understood, so the dependency would buy nothing but multipart support + that nothing here sends. + + The body is read after the length check rather than before it, for the reason the webhook + endpoint does the same: a declared length is refusable without allocating what it declares. + """ + if "application/x-www-form-urlencoded" not in request.headers.get("content-type", ""): + return None + declared = request.headers.get("content-length") + if declared and declared.isdigit() and int(declared) > _FORM_CEILING: + return None + body = await request.body() + if len(body) > _FORM_CEILING: + return None + for key, value in parse_qsl(body.decode("utf-8", "replace"), keep_blank_values=True): + if key == name: + return value + return None + + +def _acting(session: Session, request: Request) -> page.Acting: + """What this request may do, from the cookie it brought. Item 166. + + Called by every page view, and it is the only place authority is decided. The renderer receives + the answer and never the cookie, so a view cannot accidentally treat a *read* token as + authority. + """ + return page.Acting( + csrf=operator.acting(session, request.cookies.get(operator.COOKIE)), + offered=operator.configured(session), + ) + + +def _to_page(token: str, tail: str = "") -> RedirectResponse: + """Back where the operator was, as a `303`. Item 166. + + **`303` and not `302`**, because the browser must turn a POST into a GET: a `302` leaves some + clients re-posting the form on refresh, which for `approve` would mean a second attempt. + """ + return RedirectResponse( + f"{page.PREFIX}/{token}/{tail}", + status_code=status.HTTP_303_SEE_OTHER, + headers=page.HEADERS, + ) + + +@app.post(f"{page.PREFIX}/{{token}}/login", tags=["page"], include_in_schema=False) +async def page_login( + token: str, + request: Request, + session: Annotated[Session, Depends(_readiness_session)], +) -> RedirectResponse: + """Exchange the operator key for a session cookie. Item 166. + + **The one route that accepts a secret in a body, and it answers the same either way.** A wrong + key redirects to the page exactly as a right one does: an attacker with the read link learns + nothing from the response about whether a key was right, and the operator finds out by whether + the buttons are there. No error page, because an error page is an oracle. + + `Secure` is read off the request rather than hardcoded. Hardcoding it on would silently break + the plain-HTTP tailnet deployment this runs on — the cookie would never be sent and the login + would look broken; hardcoding it off would be wrong the day a TLS proxy is put in front. On the + tailnet the transport is encrypted by WireGuard even without it. + """ + if not page.opens(session, token): + raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") + + key = await _field(request, "key") + issued = operator.log_in(session, key) if key else None + redirect = _to_page(token) + if issued is not None: + cookie, _csrf = issued + redirect.set_cookie( + operator.COOKIE, + cookie, + max_age=int(operator.LIFETIME.total_seconds()), + httponly=True, + samesite="strict", + secure=request.url.scheme == "https", + path=page.PREFIX, + ) + return redirect + + +@app.post(f"{page.PREFIX}/{{token}}/logout", tags=["page"], include_in_schema=False) +async def page_logout( + token: str, + request: Request, + session: Annotated[Session, Depends(_readiness_session)], +) -> RedirectResponse: + """End this session. Item 166. + + CSRF-protected like the decisions are: a forced logout is a nuisance rather than a breach, but a + route that skips the check is a route somebody later copies. + """ + if not page.opens(session, token): + raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") + cookie = request.cookies.get(operator.COOKIE) + if operator.csrf_ok(operator.acting(session, cookie), await _field(request, "csrf")): + operator.log_out(session, cookie) + redirect = _to_page(token) + redirect.delete_cookie(operator.COOKIE, path=page.PREFIX) + return redirect + + +def _decide( + token: str, + item_id: int, + session: Session, + csrf: str | None, + what: str, + *, + cookie: str | None, +) -> RedirectResponse: + """The shared body of the two decisions: authorise, act, and go back to the item. Item 166. + + **Four guards, in this order, and each one is the whole of a different threat:** + + 1. the page token, or `404` — the same answer an unknown path gets; + 2. a session, or `404` **again** rather than `401`: an instance with no operator key and one + whose cookie is wrong must be indistinguishable, or the read link becomes a way to ask + whether this instance can be acted on at all; + 3. the CSRF token, or `403` — reachable only by something that already holds a valid session + cookie, so there is nothing left to hide from it; + 4. the state machine, in `decisions`, which is what refuses an item that is not amber. + """ + if not page.opens(session, token): + raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") + + expected = operator.acting(session, cookie) + if expected is None: + raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") + if not operator.csrf_ok(expected, csrf): + raise HTTPException(status.HTTP_403_FORBIDDEN, "Forbidden") + + found = session.get(Item, item_id) + if found is None: + raise HTTPException(status.HTTP_404_NOT_FOUND, "Not Found") + act = decide.approve if what == "approve" else decide.hand_to_human + try: + act(session, found.project, item_id) + except decide.DecisionError as exc: + # The item's own page is where the reason belongs, and it is already rendering the state + # that caused this. `409` says the request was well formed and the world disagreed. + raise HTTPException(status.HTTP_409_CONFLICT, str(exc)) from exc + log.info("decided from the page", extra={"item": item_id, "decision": what}) + return _to_page(token, f"items/{item_id}") + + +@app.post( + f"{page.PREFIX}/{{token}}/items/{{item_id}}/approve", tags=["page"], include_in_schema=False +) +async def page_approve( + token: str, + item_id: int, + request: Request, + session: Annotated[Session, Depends(_readiness_session)], +) -> RedirectResponse: + """Let the agent attempt this item. `POST` only, and that is asserted by a test. + + A `GET` that approves is a URL that approves — from an image tag, a prefetch, a chat unfurling a + link somebody pasted. This costs money and opens a pull request, so it cannot be a link. + """ + return _decide(token, item_id, session, await _field(request, "csrf"), "approve", + cookie=request.cookies.get(operator.COOKIE)) + + +@app.post( + f"{page.PREFIX}/{{token}}/items/{{item_id}}/human", tags=["page"], include_in_schema=False +) +async def page_hand_to_human( + token: str, + item_id: int, + request: Request, + session: Annotated[Session, Depends(_readiness_session)], +) -> RedirectResponse: + """Take this item away from the agent: a person will do it. `POST` only, same reasoning.""" + return _decide(token, item_id, session, await _field(request, "csrf"), "human", + cookie=request.cookies.get(operator.COOKIE)) diff --git a/hullwork/models.py b/hullwork/models.py index f9739a1..5e0d139 100644 --- a/hullwork/models.py +++ b/hullwork/models.py @@ -657,6 +657,67 @@ class PageAccess(Base): created_at: Mapped[datetime] = mapped_column(UtcDateTime(), default=_now) +class OperatorKey(Base): + """The credential that **acts**. Item 166. One row, id 1, or none at all. + + **Separate from `PageAccess` on purpose, and that separation is the whole security model.** The + page token is a bearer credential that lives in a URL — a saved page, a screenshot of the + address bar, a link mailed to a colleague — so it reads everything and may never spend money. + This one never appears in a URL: it is pasted into a form once, exchanged for a session, and + after that only the session cookie travels. + + **None at all is the default, and it means the buttons do not exist.** An instance that upgrades + into this item is byte-identical to the one before it until somebody runs + `hullwork operator-key`. + + Generated, never chosen. 32 random bytes hashed with SHA-256, for the reason already written + beside the page token: against 32 random bytes a KDF buys nothing. A human-chosen password would + need scrypt or argon2, a new dependency, and a guessing-rate story — three problems this does + not have. + """ + + __tablename__ = "operator_key" + + id: Mapped[int] = mapped_column(Integer, primary_key=True) + + #: SHA-256, compared in constant time. See `security.hash_token`. + key_hash: Mapped[str] = mapped_column(String(64)) + + created_at: Mapped[datetime] = mapped_column(UtcDateTime(), default=_now) + + +class OperatorSession(Base): + """One browser that has proved it holds the operator key. Item 166. + + **Rows rather than a signed cookie, so that revoking is deleting.** A signed cookie cannot be + withdrawn without rotating the signing key, which logs out everything at once and gives the + operator no way to end one session — and the moment a laptop is lost, "everything at once" is + the only option anybody has. Deleting a row is the whole of it, and + `hullwork operator-key --rotate` deletes them all. + + The token is hashed like every other credential here. The **CSRF token is not**: it is not a + credential, it is a value the server hands out and expects back on the same session, and it is + compared in constant time for tidiness rather than for secrecy. + """ + + __tablename__ = "operator_session" + + id: Mapped[int] = mapped_column(Integer, primary_key=True) + + #: SHA-256 of the value in the cookie. + token_hash: Mapped[str] = mapped_column(String(64), index=True) + + #: Handed to the browser, returned in a hidden field, and never in a URL. + csrf: Mapped[str] = mapped_column(String(64)) + + created_at: Mapped[datetime] = mapped_column(UtcDateTime(), default=_now) + + #: When this stops being accepted. An absolute expiry rather than an idle timeout: an idle + #: timeout has to be written on every request, which turns a read of the page into a write to + #: the database — and the receiver's sweep already contends for that lock. + expires_at: Mapped[datetime] = mapped_column(UtcDateTime()) + + class DispatcherLease(Base): """Who is dispatching, and when they last said so. One row, id 1. Item 075, DR-0009. diff --git a/hullwork/operator.py b/hullwork/operator.py new file mode 100644 index 0000000..7120746 --- /dev/null +++ b/hullwork/operator.py @@ -0,0 +1,126 @@ +"""Who may change something on the page, and for how long. Item 166. + +**Two credentials, and the split is the point.** `page.opens` answers *may this request read*, and +its credential is a bearer token in a URL. This module answers *may this request act*, and its +credential never appears in a URL at all: it is pasted into a form once, exchanged for a session, +and after that only a cookie travels. A reader handed the read link stays safe to hand it to. + +Nothing here enumerates. A wrong key, an expired session, and an instance with no operator key +configured all produce the same `None`, so a caller can only answer `404` — the same answer as a +wrong page token — and probing learns nothing about whether this instance can be acted on at all. +""" + +from __future__ import annotations + +import hmac +from datetime import UTC, datetime, timedelta + +from sqlalchemy import delete, select +from sqlalchemy.orm import Session + +from hullwork.models import OperatorKey, OperatorSession +from hullwork.security import generate_token, hash_token, verify_token + +#: The cookie the browser sends back. Scoped to the page prefix rather than to `/`: nothing else +#: this application serves has any use for it, and the webhook endpoint least of all. +COOKIE = "hullwork_operator" + +#: How long a session lasts. Long enough that an operator working through a morning's queue does not +#: log in twice; short enough that a browser left open on a train stops being a key by tomorrow. +LIFETIME = timedelta(hours=12) + + +def configured(session: Session) -> bool: + """Whether this instance has an operator key at all — which is whether the buttons exist.""" + return session.scalars(select(OperatorKey).limit(1)).first() is not None + + +def issue_key(session: Session) -> str: + """Generate the operator key, store its hash, and **end every session that exists**. + + Rotating is overwriting the one row, which is why there is one. Dropping the sessions with it is + not tidiness: the reason to rotate is that the old key might be in somebody else's hands, and a + live session issued by it would outlive the rotation by up to `LIFETIME`. + """ + key = generate_token() + row = session.scalars(select(OperatorKey).limit(1)).first() + if row is None: + session.add(OperatorKey(id=1, key_hash=hash_token(key))) + else: + row.key_hash = hash_token(key) + row.created_at = datetime.now(UTC) + session.execute(delete(OperatorSession)) + session.commit() + return key + + +def log_in(session: Session, key: str) -> tuple[str, str] | None: + """Exchange the operator key for `(cookie value, csrf token)`, or `None` if it is not the key. + + `None` covers both *wrong key* and *no key configured*, deliberately: the caller cannot tell + them apart and so cannot leak the difference. + """ + row = session.scalars(select(OperatorKey).limit(1)).first() + if row is None or not verify_token(key, row.key_hash): + return None + + token = generate_token() + csrf = generate_token() + session.add( + OperatorSession( + token_hash=hash_token(token), + csrf=csrf, + expires_at=datetime.now(UTC) + LIFETIME, + ) + ) + session.commit() + return token, csrf + + +def _row_for(session: Session, token: str | None) -> OperatorSession | None: + if not token: + return None + row = session.scalars( + select(OperatorSession).where(OperatorSession.token_hash == hash_token(token)) + ).first() + if row is None: + return None + expires = row.expires_at + if expires.tzinfo is None: # SQLite hands back naïve datetimes on some drivers. + expires = expires.replace(tzinfo=UTC) + if expires <= datetime.now(UTC): + # Expired rows are deleted on the way past rather than by a sweep: this is the only moment + # anything is known to be looking at them, and a table of dead sessions is a table somebody + # eventually has to explain. + session.delete(row) + session.commit() + return None + return row + + +def acting(session: Session, token: str | None) -> str | None: + """The session's CSRF token if this cookie may act, else `None`. + + The one authorisation question this module answers, and the only one a route has to ask. + """ + row = _row_for(session, token) + return None if row is None else row.csrf + + +def csrf_ok(expected: str | None, supplied: str | None) -> bool: + """Constant-time comparison of the CSRF pair, and `False` if either side is missing.""" + if not expected or not supplied: + return False + return hmac.compare_digest(expected, supplied) + + +def log_out(session: Session, token: str | None) -> None: + """End this one session. + + A missing or unknown token is not an error: logging out twice is fine, and so is logging out of + a session that expired while the page was open. + """ + row = _row_for(session, token) + if row is not None: + session.delete(row) + session.commit() diff --git a/hullwork/page.py b/hullwork/page.py index 2515a87..17250cc 100644 --- a/hullwork/page.py +++ b/hullwork/page.py @@ -32,6 +32,7 @@ import html import re +from dataclasses import dataclass from datetime import UTC, datetime from typing import TYPE_CHECKING from urllib.parse import urlsplit @@ -237,6 +238,11 @@ def _link(url: str | None, text: str | None = None) -> str: .count { font: 650 1.75rem/1 var(--mono); font-variant-numeric: tabular-nums; letter-spacing: -.03em; color: var(--c, var(--ink)); } .count.zero { color: var(--faint); font-weight: 400; } +/* Item 166: a non-zero count is a link to the items it counted. It keeps the number's weight and + colour — the underline is what says it can be clicked, and only on hover so the board still reads + as figures rather than as a menu. */ +a.count { display: inline-block; text-decoration: none; } +a.count:hover, a.count:focus-visible { text-decoration: underline; } /* A tinted number is still a number: the column heading above it carries the meaning. */ .age { display: block; font-size: .78rem; color: var(--muted); margin-top: .35rem; } .col.owed { border-color: color-mix(in oklab, var(--waiting) 45%, var(--rule)); } @@ -285,6 +291,33 @@ def _link(url: str | None, text: str | None = None) -> str: summary:hover { color: var(--ink); } details > pre { border-left: 2px solid var(--rule); } code { font-family: var(--mono); font-size: .9em; } + +/* Item 166. A form is the only way to change something here, so it has to look like part of the + page rather than like a browser default from 1998. `.linkish` is a button that reads as a link, + for sign-out, where a button would claim more weight than the action has. */ +form.inline { display: inline; } +button.linkish { + background: none; border: 0; padding: 0; font: inherit; color: inherit; + text-decoration: underline; cursor: pointer; +} +.decide { display: flex; gap: .6rem; flex-wrap: wrap; margin: .8rem 0 0; } +.decide button { + font: inherit; padding: .45rem .9rem; border-radius: 6px; cursor: pointer; + border: 1px solid var(--rule); background: var(--card); color: var(--fg); +} +.decide button.go { border-color: var(--passed); } +.decide form { margin: 0; } +.login { margin: .8rem 0 0; display: flex; gap: .5rem; flex-wrap: wrap; align-items: center; } +.login input { + font: inherit; font-family: var(--mono); padding: .4rem .5rem; min-width: 22rem; + border: 1px solid var(--rule); border-radius: 6px; background: var(--card); color: var(--fg); +} +.next { border-left: 3px solid var(--waiting); padding-left: .8rem; margin: 1rem 0; } +/* An item the dispatcher will never pick up. Beside the state rather than instead of it: the state + is still the truth, this is what the state cannot say on its own. */ +.stuck { font: 600 .7rem/1 var(--sans); letter-spacing: .04em; text-transform: uppercase; + color: var(--refused); border: 1px solid currentColor; border-radius: var(--r-chip); + padding: .15rem .35rem; margin-left: .35rem; } """ @@ -304,8 +337,49 @@ def _link(url: str | None, text: str | None = None) -> str: ) -def _document(title: str, body: str) -> str: - """The whole page. No script, no external asset, one inlined stylesheet.""" +@dataclass(frozen=True) +class Acting: + """What the request being rendered may do. Item 166. + + **Decided in `operator`, rendered here, and defaulting to what the page was before it existed.** + An instance with no operator key produces `Acting()` on every request, and every branch below + then takes the path it took in 0.1.0a6 — which is the acceptance criterion that keeps this item + from changing an instance nobody asked to change. + """ + + #: The session's CSRF token when this request may act. `None` is *read-only*, and it is the + #: answer for a wrong cookie, an expired session and an instance with no key alike. + csrf: str | None = None + + #: Whether an operator key exists, which is whether offering a login is honest. Without this the + #: page would either show a login on an instance that can never accept one, or hide the one + #: affordance the operator is looking for. + offered: bool = False + + +#: A request that may read and nothing else — the default everywhere, and the whole of what this +#: page was before item 166. +READING = Acting() + + +def _document(title: str, body: str, *, acting: Acting = READING, up: str = "") -> str: + """The whole page. No script, no external asset, one inlined stylesheet. + + `up` is how far this view is from `/page//`, because **every URL here is relative on + purpose** — that is what keeps the token out of the HTML, so a saved page or a screenshot of the + source carries no key. A form is a URL like any other: from `items/28` the sign-out has to post + to `../logout`, and hardcoding `logout` would have posted to `items/logout` and 404'd. + """ + footing = ( + "read-only. This URL is the credential: anyone who has it can read " + "everything on this page. Rotate it with hullwork page-token --rotate." + if acting.csrf is None + else "signed in, so two buttons on an amber item work and nothing else " + "does. The URL is still only a read credential: this browser holds the other one. " + f'
' + f'' + '
' + ) return ( "\n" '' @@ -314,10 +388,7 @@ def _document(title: str, body: str) -> str: f'' f"{_h(title)}\n" f"{body}\n" - "\n" + f"\n" "\n" ) @@ -381,19 +452,29 @@ def _the_credential_split(session: Session) -> str: #: they are why the page exists — `pr-open` gets its own rather than being folded into "open", #: because that queue not draining is item 138's review debt, which is the product's own failure #: mode and belongs in the reader's face rather than in a report. -_COLUMNS: tuple[tuple[str, str, tuple[ItemState, ...], bool], ...] = ( - ("Arrived", "c-idle", (ItemState.NEW, ItemState.TRIAGED, ItemState.REOPENED), False), - ("Waiting on you", "c-waiting", (ItemState.WAITING_APPROVAL, ItemState.HUMAN_ONLY), True), - ("Queued", "c-idle", (ItemState.READY,), False), - ("Working", "c-working", (ItemState.IN_PROGRESS,), False), - ("Waiting on review", "c-waiting", (ItemState.PR_OPEN,), True), +#: +#: Each column also carries a **key**, because item 166 made the counts links. The operator read +#: *"Waiting on you 2"* off this board and asked *"¿y ahora qué?"*: a number with no name behind +#: it, answerable only by leaving the page, opening the list and scanning 28 rows. The key is what +#: `items?in=…` filters on, so a count leads to the items it counted. +_COLUMNS: tuple[tuple[str, str, str, tuple[ItemState, ...], bool], ...] = ( + ("Arrived", "arrived", "c-idle", + (ItemState.NEW, ItemState.TRIAGED, ItemState.REOPENED), False), + ("Waiting on you", "waiting", "c-waiting", + (ItemState.WAITING_APPROVAL, ItemState.HUMAN_ONLY), True), + ("Queued", "queued", "c-idle", (ItemState.READY,), False), + ("Working", "working", "c-working", (ItemState.IN_PROGRESS,), False), + ("Waiting on review", "review", "c-waiting", (ItemState.PR_OPEN,), True), ( - "Closed", "c-passed", + "Closed", "closed", "c-passed", (ItemState.DONE, ItemState.REJECTED, ItemState.FAILED, ItemState.NOT_REPRODUCIBLE), False, ), ) +#: The column keys, for the list view to look up without importing the display tuple's shape. +_IN: dict[str, tuple[ItemState, ...]] = {key: states for _, key, _, states, _ in _COLUMNS} + #: The six steps, in the order a reader watches them happen. _PHASES: tuple[tuple[str, AttemptPhase], ...] = ( ("baseline", AttemptPhase.BASELINE), @@ -424,6 +505,28 @@ def _ago(when: datetime | None) -> str: return "not recorded" # pragma: no cover - the loop above is exhaustive +#: The states the dispatcher will ever pick an item up from. `work.py` selects `READY` items whose +#: project is active; everything else is waiting on a person or already finished. +_DISPATCHABLE = (ItemState.READY, ItemState.WAITING_APPROVAL) + + +def _stuck(item: _Item) -> str | None: + """Why this item can **never** be attempted, or `None` if nothing stops it. Item 166. + + **The count was right and the reader was still misled.** `simplecheck` was disabled on + 2026-08-07 and item 15 stayed in `ready`, so the board kept counting it under *Queued* with + an age that kept climbing — while `work.py` selects on `Project.active.is_(True)` and would + never look at it again. A queue that cannot drain has to say so where it is displayed, not in + the release notes of the command that disabled the project. + """ + if item.state in _DISPATCHABLE and not item.project.active: + return ( + f"the project '{item.project.slug}' is disabled, so the dispatcher will never " + f"pick this up — re-register it to change that" + ) + return None + + def _board(session: Session) -> str: """Where everything is, and how long the oldest has been there. @@ -431,7 +534,7 @@ def _board(session: Session) -> str: clock on the transition. """ cells = [] - for title, tone, states, owed in _COLUMNS: + for title, key, tone, states, owed in _COLUMNS: items = list(session.scalars(select(_Item).where(_Item.state.in_(states))).all()) oldest = min((i.state_since for i in items if i.state_since is not None), default=None) count = len(items) @@ -442,10 +545,17 @@ def _board(session: Session) -> str: if oldest is not None else "age not recorded" ) + # **A count of zero is not a link**, because there is nothing behind it and a link that + # lands on "no items match" teaches a reader that the page is broken rather than that the + # queue is empty. + counter = ( + f'0' + if not count + else f'{count}' + ) cells.append( f'
' - f"

{_h(title)}

" - f'{count}' + f"

{_h(title)}

{counter}" f'{age}
' ) return f'
{"".join(cells)}
' @@ -575,7 +685,9 @@ def _violations_in(seal: object) -> bool: return bool(seal.get("violations")) -def instance(session: Session, settings: Settings, *, error_reporting: bool) -> str: +def instance( + session: Session, settings: Settings, *, error_reporting: bool, acting: Acting = READING +) -> str: """What `hullwork status` says, for somebody who does not have a terminal on this host. **Every number comes from the function `status` calls**, never from a second query written for @@ -639,8 +751,15 @@ def instance(session: Session, settings: Settings, *, error_reporting: bool) -> prices = spend.Prices.from_settings(settings) body = ( "

hullwork

" - '

Read-only. Nothing here changes anything. ' - 'Projects · ' + # **The opening line stops being a lie when a session can act** (item 166). It said + # "Nothing here changes anything" for two versions and it was true; saying it while two + # buttons work would be worse than saying nothing. + + ( + '

Read-only. Nothing here changes anything. ' + if acting.csrf is None + else '

Signed in: an amber item can be decided here. ' + ) + + 'Projects · ' 'Items and their evidence

' + (f"

Problems

" if problems else "") # **The three bands come first, and they are the page** (item 143). What follows them is @@ -649,6 +768,16 @@ def instance(session: Session, settings: Settings, *, error_reporting: bool) -> # is for arrives daily. + f"

Now

{_now(session, prices)}" + f"

Where everything is

{_board(session)}" + # A count is a link now, so the front page needs the way in to be here too rather + # than only on an item: the operator arrives at this board, not at item 28. + + ( + '
' + '' + '
' + if acting.offered and acting.csrf is None + else '' + ) + _disagreements(session, settings) + f"

State

{table}
" + (f"

Attempts

" if attempts else "") @@ -657,7 +786,7 @@ def instance(session: Session, settings: Settings, *, error_reporting: bool) -> + _the_credential_split(session) + _what_this_instance_allows(settings) ) - return _document("Hullwork — this instance", body) + return _document("Hullwork — this instance", body, acting=acting) #: How many rows a list shows. Bounded because an instance that has been running for a year has @@ -716,7 +845,7 @@ def _project_columns(session: Session, project_id: int) -> str: board disagreeing about what "waiting on you" means would make both useless. """ cells = [] - for title, tone, states, owed in _COLUMNS: + for title, _key, tone, states, owed in _COLUMNS: count = len( list( session.scalars( @@ -976,22 +1105,27 @@ def artefact( ) -def items(session: Session) -> str: - """Every item this instance has, most recent first. The view a reviewer lands on.""" +def items(session: Session, *, only: str | None = None, acting: Acting = READING) -> str: + """Every item this instance has, most recent first. The view a reviewer lands on. + + `only` is one of the board's column keys, which is what makes a count on the front page lead + somewhere. An unrecognised key shows everything rather than nothing: this arrives from a URL, + and a typo in a hand-edited address should not read as an empty instance. + """ from sqlalchemy import func from sqlalchemy.orm import joinedload from hullwork.models import Attempt, Item - total = session.scalar(select(func.count()).select_from(Item)) or 0 - rows = list( - session.scalars( - select(Item) - .options(joinedload(Item.project)) - .order_by(Item.last_seen.desc()) - .limit(MAX_ITEMS) - ).all() - ) + states = _IN.get(only) if only else None + counting = select(func.count()).select_from(Item) + listing = select(Item).options(joinedload(Item.project)) + if states is not None: + counting = counting.where(Item.state.in_(states)) + listing = listing.where(Item.state.in_(states)) + + total = session.scalar(counting) or 0 + rows = list(session.scalars(listing.order_by(Item.last_seen.desc()).limit(MAX_ITEMS)).all()) pulls: dict[int, str] = {} if rows: for item_id, ref in session.execute( @@ -1009,11 +1143,14 @@ def items(session: Session) -> str: if row.id in pulls else (_h(row.forge_issue_ref) if row.forge_issue_ref else "—") ) + state = _h(row.state.value) + ( + ' never' if _stuck(row) else "" + ) body_rows.append( "" f'#{row.id}' f"{_h(row.project.slug)}" - f"{_h(row.state.value)}" + f"{state}" f"{_h(row.lane.value)}" f"{_h(row.title.splitlines()[0] if row.title else '')}" f"{_h(row.last_seen)}" @@ -1021,6 +1158,7 @@ def items(session: Session) -> str: "" ) + scope = "" if states is None else f" in {_h(only)}" if rows: table = ( '' @@ -1031,20 +1169,25 @@ def items(session: Session) -> str: # The bound, stated. Silently showing 200 of 4,000 is how a page teaches a reader that an # instance has done less than it has. shown = ( - f"Showing all {total} item(s)." + f"Showing all {total} item(s)" if total <= MAX_ITEMS - else f"Showing the {len(rows)} most recently seen of {total} item(s)." + else f"Showing the {len(rows)} most recently seen of {total} item(s)" ) else: table = "" - shown = "No items yet. Nothing has arrived from the error tracker on this instance." + shown = ( + "Nothing here now" + if states is not None + else "No items yet. Nothing has arrived from the error tracker on this instance" + ) + everything = '' if states is None else ' · All items' body = ( "

Items

" - f'

{_h(shown)} Most recently seen first. ' - 'Instance

' + table + f'

{shown}{scope}. Most recently seen first. ' + f'Instance{everything}

' + table ) - return _document("Hullwork — items", body) + return _document("Hullwork — items", body, acting=acting) def _above_the_fold(attempt: Attempt, prices: Prices | None) -> str: @@ -1088,7 +1231,85 @@ def _above_the_fold(attempt: Attempt, prices: Prices | None) -> str: return '

' + " · ".join(facts) + "

" -def item(session: Session, settings: Settings, item_id: int) -> str | None: +#: What each state is waiting for, in the second person, because the reader is the one waiting. +#: +#: **Item 166 exists because the page hedged where the state does not.** On an amber item it printed +#: *"Either this item is waiting for the dispatcher, or its lane says a human takes it"* — and the +#: state answers that. The operator read the board, saw two items twenty-one hours old, and asked +#: *"¿y ahora qué?"*; this table is the answer, on the item, in one sentence. +_WAITING_FOR: dict[ItemState, str] = { + ItemState.NEW: "triage, which happens on the next sweep. Nothing to do.", + ItemState.TRIAGED: "its lane to be decided, which happens on the next sweep. Nothing to do.", + ItemState.WAITING_APPROVAL: ( + "**you**. Its lane is amber: an agent may attempt it, but only once somebody says so. " + "Approving costs one attempt — money on the wire and a pull request for a person to read." + ), + ItemState.HUMAN_ONLY: ( + "**a person**, and no agent will touch it. Either its lane says so, or somebody decided so " + "here." + ), + ItemState.READY: "the dispatcher, which takes one item at a time. Nothing to do.", + ItemState.IN_PROGRESS: "the attempt running now. The phases are on the front page.", + ItemState.PR_OPEN: ( + "**a reviewer**. Merging it accepts the fix; closing it with a label refuses it, and the " + "label is the reason this instance records." + ), + ItemState.REOPENED: "triage again: it came back after being closed.", +} + + +def _next_action(found: Item, acting: Acting, *, up: str) -> str: + """What is blocking this item and what a person can do about it, right here. Item 166.""" + stuck = _stuck(found) + waiting = _WAITING_FOR.get(found.state) + parts: list[str] = [] + if waiting: + parts.append(f"

Waiting for {_own_prose(waiting)}

") + if stuck: + parts.append(f'

But {_h(stuck)}.

') + if found.state is ItemState.WAITING_APPROVAL and not stuck: + parts.append(_decide(found, acting, up=up)) + if not parts: + return "" + return f'' + + +def _decide(found: Item, acting: Acting, *, up: str) -> str: + """The two buttons, a login, or the command — whichever this request has earned. + + Three states and each one is honest about itself: a signed-in operator gets the buttons; an + instance with a key and no session gets a login; an instance with **no operator key** gets the + command, because that is the only way to act on it and pretending otherwise would send a reader + looking for a login that cannot exist. + """ + if acting.csrf is not None: + forms = "".join( + f'' + f'' + f'' + for route, label, extra in ( + ("approve", "Let the agent try it", ' class="go"'), + ("human", "I will take this one", ""), + ) + ) + return f'
{forms}
' + if acting.offered: + return ( + f'' + '' + '' + ) + return ( + '

This instance has no operator key, so nothing here can act. Either run ' + f"hullwork approve {_h(found.project.slug)} {_h(found.id)} on the host, or " + "give the page a key with hullwork operator-key.

" + ) + + +def item( + session: Session, settings: Settings, item_id: int, *, acting: Acting = READING +) -> str | None: """One item and every attempt on it. `None` when there is no such item, which the route 404s. The facts first, because a reviewer decides whether this is worth reading before reading it; @@ -1160,14 +1381,17 @@ def item(session: Session, settings: Settings, item_id: int) -> str | None: f"

#{_h(found.id)} {_h(title)}

" f'

All items · Instance

' f"
itemprojectstatelane
{table}
" + + _next_action(found, acting, up="../") + ( f'

{_h(_NOT_STORED)}

' + "".join(blocks) if blocks - else '

No attempts. Either this item is waiting for the dispatcher, or ' - "its lane says a human takes it.

" + # **No longer "either … or".** What it is waiting for is stated above, from the state, + # and this line now says only the thing the state cannot: that there is no evidence + # trail yet because nothing has run. + else '

No attempts yet, so there is no evidence to read here.

' ) ) - return _document(f"Hullwork — item {found.id}", body) + return _document(f"Hullwork — item {found.id}", body, acting=acting, up="../") def _issue_link(settings: Settings, found: Item) -> str: diff --git a/hullwork/upstream.py b/hullwork/upstream.py index 6f85ba2..a7a69cc 100644 --- a/hullwork/upstream.py +++ b/hullwork/upstream.py @@ -113,8 +113,9 @@ | { f"cli:{name}" for name in ( - "approve", "config", "doctor", "gateway", "init", "lease", "page-token", "projects", - "propose", "prune", "republish", "requeue", "status", "sweep", "try", "work", + "approve", "config", "doctor", "gateway", "init", "lease", "operator-key", + "page-token", "projects", "propose", "prune", "republish", "requeue", "status", + "sweep", "try", "work", ) } ) diff --git a/migrations/versions/a4e21b8c56df_a_credential_that_acts.py b/migrations/versions/a4e21b8c56df_a_credential_that_acts.py new file mode 100644 index 0000000..da486e3 --- /dev/null +++ b/migrations/versions/a4e21b8c56df_a_credential_that_acts.py @@ -0,0 +1,69 @@ +"""a credential that acts + +Revision ID: a4e21b8c56df +Revises: c1f60d4a8b73 +Create Date: 2026-08-07 + +Item 166. Two tables: the credential that may change something, and the sessions it issues. + +**Neither replaces `page_access`, and the split is the security model rather than tidiness.** The page +token is a bearer credential that lives in a URL — a saved page, a screenshot of the address bar, a +link mailed to a colleague — so it reads everything and may never spend money. `operator_key` never +appears in a URL: it is pasted into a form once, exchanged for a session, and after that only the +session cookie travels. A reader handed the read link is still safe to hand it to. + +**No row is the default, and an upgrade therefore adds no buttons.** Until somebody runs +`hullwork operator-key`, the page renders what it rendered before this revision and every mutating +route answers `404` — the same answer as a wrong page token, so probing cannot even learn whether an +instance has an operator key. + +Sessions are rows so that revoking is deleting. A signed cookie cannot be withdrawn without rotating +the signing key, which ends every session at once and leaves no way to end one; the morning a laptop +goes missing, that is the only lever anybody has. + +`expires_at` is absolute rather than an idle timeout: an idle timeout has to be written on every +request, which turns reading the page into a write, and the receiver's sweep already contends for that +lock (`database is locked`, item 134). + +No application imports, so this revision keeps describing the schema as it was when it was written. +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = 'a4e21b8c56df' +down_revision: str | None = 'c1f60d4a8b73' +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + op.create_table( + 'operator_key', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('key_hash', sa.String(length=64), nullable=False), + sa.Column('created_at', sa.DateTime(timezone=True), nullable=False), + sa.PrimaryKeyConstraint('id'), + ) + op.create_table( + 'operator_session', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('token_hash', sa.String(length=64), nullable=False), + sa.Column('csrf', sa.String(length=64), nullable=False), + sa.Column('created_at', sa.DateTime(timezone=True), nullable=False), + sa.Column('expires_at', sa.DateTime(timezone=True), nullable=False), + sa.PrimaryKeyConstraint('id'), + ) + # Looked up by hash on every request that carries the cookie, which is every render of the page + # for a logged-in operator. + op.create_index( + op.f('ix_operator_session_token_hash'), 'operator_session', ['token_hash'], unique=False + ) + + +def downgrade() -> None: + op.drop_index(op.f('ix_operator_session_token_hash'), table_name='operator_session') + op.drop_table('operator_session') + op.drop_table('operator_key') diff --git a/tests/test_page_surface.py b/tests/test_page_surface.py index 5d9560d..85df98d 100644 --- a/tests/test_page_surface.py +++ b/tests/test_page_surface.py @@ -188,15 +188,40 @@ def test_the_policy_forbids_script_because_there_is_none(db: Session, client: Te assert " None: - """Read-only, asserted by walking the application's own routes rather than by trusting a - decorator to stay a `get` through the next refactor.""" +#: The only routes under the page prefix that may change anything. Item 166 added them and this +#: tuple is the whole of the exception: everything else stays `GET`-only. +_MAY_POST = ( + f"{page.PREFIX}/{{token}}/login", + f"{page.PREFIX}/{{token}}/logout", + f"{page.PREFIX}/{{token}}/items/{{item_id}}/approve", + f"{page.PREFIX}/{{token}}/items/{{item_id}}/human", +) + + +def test_only_the_four_named_routes_under_the_prefix_accept_a_post(client: TestClient) -> None: + """**This test used to say `GET`-only, and item 166 is why it does not any more.** + + The invariant it was protecting was never "no POST" — it was *no accidental mutation surface*, + asserted by walking the application's own routes rather than by trusting a decorator to stay a + `get` through the next refactor. That still holds, and it is now specific: four routes may take + a POST, they are named here, and a fifth appearing fails this test on the day it is written. + + A view acquiring a POST by accident is what this catches, and it is worth catching: the token is + a bearer credential in a URL, so a mutating route that only checks the token would let anybody + holding a saved link spend money. + """ from hullwork.main import app for route in app.routes: path = getattr(route, "path", "") - if path.startswith(page.PREFIX): - assert getattr(route, "methods", set()) <= {"GET", "HEAD"}, path + if not path.startswith(page.PREFIX): + continue + methods: set[str] = getattr(route, "methods", set()) + allowed = {"GET", "HEAD", "POST"} if path in _MAY_POST else {"GET", "HEAD"} + assert methods <= allowed, path + # And every route named above exists: a typo here would silently stop asserting anything. + paths = {getattr(route, "path", "") for route in app.routes} + assert set(_MAY_POST) <= paths def test_no_credential_of_any_kind_is_in_the_page(db: Session, client: TestClient) -> None: diff --git a/tests/test_the_page_can_be_acted_on.py b/tests/test_the_page_can_be_acted_on.py new file mode 100644 index 0000000..e911773 --- /dev/null +++ b/tests/test_the_page_can_be_acted_on.py @@ -0,0 +1,400 @@ +"""Two buttons on a page whose URL is a credential. Item 166. + +The page was read-only for two versions and the argument for keeping it that way is written into +`approve` itself: *"an approval endpoint would be a permanent attack surface for something done by +one person a handful of times."* That argument was answered rather than ignored — the ground it +stood on ("the operator already has the host") stopped being true — so what these tests assert is +the thing that makes the reversal safe: **the read token gains no authority at all.** + +Every test below is one sentence of the threat model: + +* the URL is a bearer credential, so it must not be able to spend money; +* an instance nobody has given a key to must be exactly what it was before this item; +* and nothing may answer differently depending on whether a key exists, because that answer is + itself worth having. +""" + +from __future__ import annotations + +import io +from collections.abc import Iterator +from pathlib import Path + +import pytest +from alembic import command +from alembic.config import Config +from fastapi.testclient import TestClient +from sqlalchemy.orm import Session + +from hullwork import operator, page +from hullwork.config import get_settings +from hullwork.db import make_engine, make_session_factory +from hullwork.models import Item, ItemKind, ItemState, Lane, Project +from hullwork.security import generate_token, hash_token + +ROOT = Path(__file__).resolve().parents[1] + +#: The read credential. Long enough to be real, and fixed so the tests can name URLs. +TOKEN = "a-page-token-long-enough-to-be-real-x" # noqa: S105 + + +@pytest.fixture +def db(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[Session]: + """A migrated database with one project and one amber item waiting for a decision.""" + url = f"sqlite:///{tmp_path / 'acted-on.db'}" + monkeypatch.setenv("HULLWORK_DATABASE_URL", url) + monkeypatch.setenv("HULLWORK_BASE_URL", "https://hullwork.example") + get_settings.cache_clear() + + cfg = Config() + cfg.set_main_option("script_location", str(ROOT / "migrations")) + cfg.cmd_opts = None + command.upgrade(cfg, "head") + + with make_session_factory(make_engine(url))() as session: + session.add( + Project( + slug="p", + forge="forgejo", + repo="easybyte/p", + webhook_secret_hash="not-a-real-hash", # noqa: S106 - fixture + manifest={}, + ) + ) + session.commit() + session.add( + Item( + project_id=1, + fingerprint="fp-1", + state=ItemState.WAITING_APPROVAL, + lane=Lane.AMBER, + kind=ItemKind.BUG, + title="OperationalError: locked", + ) + ) + session.commit() + page.issue(session, hash_token(TOKEN)) + yield session + get_settings.cache_clear() + + +@pytest.fixture +def client() -> TestClient: + from hullwork.main import app + + # Redirects are the subject of two tests below, so they are never followed automatically. + return TestClient(app, follow_redirects=False) + + +def _key(db: Session) -> str: + """Give this instance an operator key, the way the operator does.""" + return operator.issue_key(db) + + +def _sign_in(db: Session, client: TestClient) -> str: + """Log in and return the CSRF token the page would put in its forms.""" + key = _key(db) + answered = client.post(f"/page/{TOKEN}/login", data={"key": key}) + assert answered.status_code == 303 + csrf = operator.acting(db, client.cookies.get(operator.COOKIE)) + assert csrf is not None + return csrf + + +def _state(db: Session) -> ItemState: + db.expire_all() + found = db.get(Item, 1) + assert found is not None + return found.state + + +# --- an instance nobody gave a key to ----------------------------------------------------------- + + +def test_with_no_operator_key_the_page_says_it_is_read_only( + db: Session, client: TestClient +) -> None: + """**The acceptance criterion that protects everybody who did not ask for this.** An upgrade + that added buttons to a running instance would be this item changing somebody else's security + posture on their behalf.""" + front = client.get(f"/page/{TOKEN}/").text + item_page = client.get(f"/page/{TOKEN}/items/1").text + + assert "Nothing here changes anything" in front + assert "read-only" in front + assert "Sign in to decide" not in front + assert "Sign in to decide" not in item_page + assert "Sign in', "") + + +def test_with_no_operator_key_the_deciding_routes_are_not_there( + db: Session, client: TestClient +) -> None: + """`404`, and the **same** `404` an unknown path gets — not `401` and not `403`. + + A `403` would say *this instance can be acted on and you are not allowed*, which is a fact worth + withholding from somebody who has found a saved link. + """ + unknown = client.get("/nope") + approved = client.post(f"/page/{TOKEN}/items/1/approve", data={"csrf": "anything"}) + + assert approved.status_code == unknown.status_code == 404 + assert approved.json() == unknown.json() + assert _state(db) is ItemState.WAITING_APPROVAL + + +def test_the_item_page_offers_the_command_when_there_is_no_key( + db: Session, client: TestClient +) -> None: + """It says how to act rather than pretending nothing can be done — the complaint that started + this item was *"¿y ahora qué?"*, and the answer without a key is a command.""" + rendered = client.get(f"/page/{TOKEN}/items/1").text + + assert "hullwork approve p 1" in rendered + assert "hullwork operator-key" in rendered + + +# --- a key, but no session ---------------------------------------------------------------------- + + +def test_a_key_offers_a_login_and_still_no_buttons(db: Session, client: TestClient) -> None: + _key(db) + + front = client.get(f"/page/{TOKEN}/").text + item_page = client.get(f"/page/{TOKEN}/items/1").text + + assert "Sign in to decide" in front + assert "Sign in to decide" in item_page + assert "Let the agent try it" not in item_page + + +def test_the_read_token_alone_cannot_decide_anything(db: Session, client: TestClient) -> None: + """**The whole point of the two-credential split, stated as a test.** Somebody holding the URL — + a saved page, a screenshot, a forwarded link — posts the form and nothing happens.""" + _key(db) + + approved = client.post(f"/page/{TOKEN}/items/1/approve", data={"csrf": "guessed"}) + + assert approved.status_code == 404 + assert _state(db) is ItemState.WAITING_APPROVAL + + +def test_a_wrong_key_answers_exactly_like_the_right_one(db: Session, client: TestClient) -> None: + """No error page, because an error page is an oracle: an attacker with the read link would + otherwise have a place to guess keys and be told when one is wrong.""" + _key(db) + + wrong = client.post(f"/page/{TOKEN}/login", data={"key": "not-the-key"}) + + assert wrong.status_code == 303 + assert operator.COOKIE not in wrong.cookies + assert operator.acting(db, None) is None + + +# --- a session ---------------------------------------------------------------------------------- + + +def test_signing_in_shows_the_two_buttons_and_nothing_else(db: Session, client: TestClient) -> None: + csrf = _sign_in(db, client) + + rendered = client.get(f"/page/{TOKEN}/items/1").text + + assert "Let the agent try it" in rendered + assert "I will take this one" in rendered + assert csrf in rendered, "the form has to carry the token the server will check" + assert "Nothing here changes anything" not in client.get(f"/page/{TOKEN}/").text + + +def test_approving_from_the_page_makes_the_item_ready(db: Session, client: TestClient) -> None: + csrf = _sign_in(db, client) + + answered = client.post(f"/page/{TOKEN}/items/1/approve", data={"csrf": csrf}) + + assert answered.status_code == 303 + assert answered.headers["location"].endswith("/items/1") + assert _state(db) is ItemState.READY + + +def test_handing_it_to_a_human_is_human_only_and_not_rejected( + db: Session, client: TestClient +) -> None: + """**`rejected` would have corrupted a number.** That state means a reviewer closed a pull + request and it feeds `counted.rejected` by reason; a decision about whether to *attempt* has no + business in the tally that counts *review*.""" + csrf = _sign_in(db, client) + + answered = client.post(f"/page/{TOKEN}/items/1/human", data={"csrf": csrf}) + + assert answered.status_code == 303 + assert _state(db) is ItemState.HUMAN_ONLY + + +def test_a_wrong_csrf_token_changes_nothing(db: Session, client: TestClient) -> None: + """`403` here rather than `404`: this is reachable only by something already holding a valid + session cookie, so there is nothing left to hide from it.""" + _sign_in(db, client) + + answered = client.post(f"/page/{TOKEN}/items/1/approve", data={"csrf": generate_token()}) + + assert answered.status_code == 403 + assert _state(db) is ItemState.WAITING_APPROVAL + + +def test_a_missing_csrf_token_changes_nothing(db: Session, client: TestClient) -> None: + _sign_in(db, client) + + answered = client.post(f"/page/{TOKEN}/items/1/approve", data={}) + + assert answered.status_code == 403 + assert _state(db) is ItemState.WAITING_APPROVAL + + +def test_deciding_twice_refuses_the_second_time(db: Session, client: TestClient) -> None: + """The state machine is the last guard, and it is the one that cannot be forgotten: a double + submit — a refresh, an impatient click — must not buy two attempts.""" + csrf = _sign_in(db, client) + + first = client.post(f"/page/{TOKEN}/items/1/approve", data={"csrf": csrf}) + second = client.post(f"/page/{TOKEN}/items/1/approve", data={"csrf": csrf}) + + assert first.status_code == 303 + assert second.status_code == 409 + assert _state(db) is ItemState.READY + + +def test_the_cookie_is_httponly_and_samesite_strict(db: Session, client: TestClient) -> None: + """`SameSite=Strict` is the first half of the CSRF defence: a cross-site POST does not carry it. + + `Secure` is **not** asserted, and that is deliberate: it is read off the request scheme, so the + test client's `http` correctly produces a cookie without it. Hardcoding it on would have made + the login silently impossible on the plain-HTTP tailnet deployment this runs on. + """ + key = _key(db) + + answered = client.post(f"/page/{TOKEN}/login", data={"key": key}) + + header = answered.headers["set-cookie"].lower() + assert "httponly" in header + assert "samesite=strict" in header + assert f"path={page.PREFIX}" in header + + +def test_rotating_the_key_ends_the_session_that_was_open(db: Session, client: TestClient) -> None: + """Measured rather than asserted in prose: the reason to rotate is that the old key may be in + somebody else's hands, and a live session issued by it would outlive the rotation.""" + csrf = _sign_in(db, client) + assert client.post(f"/page/{TOKEN}/items/1/approve", data={"csrf": csrf}).status_code == 303 + + operator.issue_key(db) + + # Asserted on the front page rather than on the item: the approval above moved it out of + # `waiting-approval`, and the login only appears beside a decision that is still open. + after = client.get(f"/page/{TOKEN}/").text + assert "Sign in to decide" in after + assert "Nothing here changes anything" in after, "no session, so it is read-only again" + + +def test_signing_out_ends_it_too(db: Session, client: TestClient) -> None: + csrf = _sign_in(db, client) + + client.post(f"/page/{TOKEN}/logout", data={"csrf": csrf}) + + assert "Sign in to decide" in client.get(f"/page/{TOKEN}/").text + + +# --- the three complaints that started the item ------------------------------------------------- + + +def test_the_board_counts_lead_to_the_items_they_counted(db: Session, client: TestClient) -> None: + """*"Waiting on you 2 → ¿y ahora qué?"* — a count with no name behind it was a dead end.""" + front = client.get(f"/page/{TOKEN}/").text + + assert 'href="items?in=waiting"' in front + listed = client.get(f"/page/{TOKEN}/items?in=waiting").text + assert "items/1" in listed + assert "in=waiting" not in listed or "All items" in listed + + +def test_a_zero_count_is_not_a_link(db: Session, client: TestClient) -> None: + """A link that lands on "nothing here" teaches a reader that the page is broken.""" + front = client.get(f"/page/{TOKEN}/").text + + assert 'href="items?in=working"' not in front + + +def test_the_item_says_what_it_is_waiting_for_without_hedging( + db: Session, client: TestClient +) -> None: + """The line this replaces was *"Either this item is waiting for the dispatcher, or its lane says + a human takes it"* — on an item whose state answers that question exactly.""" + rendered = client.get(f"/page/{TOKEN}/items/1").text + + assert "Waiting for" in rendered + assert "Either this item is waiting" not in rendered + + +def test_an_item_whose_project_is_disabled_says_it_will_never_run( + db: Session, client: TestClient +) -> None: + """**Measured on the live instance on 2026-08-07.** `simplecheck` was disabled, item 15 stayed + `ready`, and the board went on counting it under *Queued* with a climbing age while `work.py` + would never look at it again.""" + found = db.get(Item, 1) + assert found is not None + found.state = ItemState.READY + project = db.get(Project, 1) + assert project is not None + project.active = False + db.commit() + + rendered = client.get(f"/page/{TOKEN}/items/1").text + listed = client.get(f"/page/{TOKEN}/items").text + + assert "is disabled, so the dispatcher will never pick this up" in rendered + assert "never" in listed + + +def test_an_unknown_filter_shows_everything_rather_than_nothing( + db: Session, client: TestClient +) -> None: + """It arrives from a URL, and a typo in a hand-edited address should not read as an empty + instance.""" + listed = client.get(f"/page/{TOKEN}/items?in=nonsense").text + + assert "items/1" in listed + + +# --- the CLI side ------------------------------------------------------------------------------- + + +def test_the_command_refuses_to_replace_a_key_without_rotate(db: Session) -> None: + """The failure it prevents: a second person running this to "get in" locks out the first, and it + reads as the buttons being broken rather than as a key having changed.""" + from hullwork.cli import main as cli_main + + out = io.StringIO() + assert cli_main(["operator-key"], out=out) == 0 + assert "shown once" in out.getvalue() + + # `main` turns a `CommandError` into an exit code and a line on stderr rather than a traceback, + # so the refusal is asserted where an operator would see it. + again = io.StringIO() + assert cli_main(["operator-key"], out=again) != 0 + assert again.getvalue() == "", "the refusal belongs on stderr, not in the output" + + +def test_the_key_is_printed_once_and_only_its_hash_is_stored(db: Session) -> None: + from hullwork.cli import main as cli_main + from hullwork.models import OperatorKey + + out = io.StringIO() + assert cli_main(["operator-key"], out=out) == 0 + + printed = [line.strip() for line in out.getvalue().splitlines() if line.strip()] + key = next(line for line in printed if len(line) > 40 and " " not in line) + + db.expire_all() + stored = db.get(OperatorKey, 1) + assert stored is not None + assert stored.key_hash == hash_token(key) + assert key not in stored.key_hash