From c0a18ac735cb691127ed4260685438dc307c2925 Mon Sep 17 00:00:00 2001 From: Stefan Date: Mon, 14 Sep 2026 17:22:54 +0200 Subject: [PATCH] Wettlauf beim Reservieren, 500er bei unsinnigen Eingaben MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drei P3-Funde aus der Durchsicht vom 14.09.: Reservieren: Statusprüfung und Schreiben waren getrennt - zwei gleichzeitige Anfragen konnten beide passieren, die zweite überschrieb Name und Token der ersten, ohne dass die es erfuhr. Jetzt entscheidet ein UPDATE mit Status-Bedingung; der Verlierer bekommt None und die Route meldet "schon weg" (Seite) bzw. 409 (API). /?limit=abc lieferte jedem anonymen Besucher einen internen Serverfehler, limit=-1 hiess in SQLite "alles". _limit_lesen() fällt bei Unsinn auf die Seitengrösse zurück und deckelt bei 500. Erfassen/Bearbeiten: int(category_id) und die Pydantic-Prüfung warfen im Handler - 500 statt Fehlermeldung. Jetzt Meldung; ausserdem werden die Angaben VOR den Bildern geprüft, damit bei abgelehnten Angaben keine verwaisten Bilddateien liegen bleiben. Vier neue Tests, jeder einmal gegen den alten Code gelaufen und dabei rot geworden. 84 lokal grün (HEIC-Test braucht pillow-heif, Docker). Co-Authored-By: Claude Fable 5 --- Plan-Verbesserungen.md | 19 +++++++++-- app/crud.py | 28 +++++++++++----- app/routers/items.py | 5 +++ app/routers/pages.py | 68 +++++++++++++++++++++++++++++--------- tests/test_reservierung.py | 16 +++++++++ tests/test_seiten.py | 23 +++++++++++++ 6 files changed, 133 insertions(+), 26 deletions(-) diff --git a/Plan-Verbesserungen.md b/Plan-Verbesserungen.md index 2aed07d..b13cacd 100644 --- a/Plan-Verbesserungen.md +++ b/Plan-Verbesserungen.md @@ -146,7 +146,7 @@ verlässt sich das Image derzeit stillschweigend. Proxy-IP eintragen; dort ausserdem den Hinweis ergänzen, dass der veröffentlichte Port nur für den Proxy erreichbar sein darf. -### P3 — Öffentlicher 500er über `?limit=abc` +### P3 — Öffentlicher 500er über `?limit=abc` ✅ ERLEDIGT 14.09.2026 `pages.py` (`galerie`, `liste_ausschnitt`) rechnet `int(request.query_params.get("limit") or SEITE)` ohne Prüfung — `/?limit=x` @@ -155,7 +155,7 @@ bedeutet in SQLite „alles". Die API-Seite macht es mit `Query(ge=1, le=200)` längst richtig; dieselbe Grenze hier nachziehen (ungültig → Standardwert). -### P3 — Betreiber-Formulare: 500 statt Fehlermeldung +### P3 — Betreiber-Formulare: 500 statt Fehlermeldung ✅ ERLEDIGT 14.09.2026 In `erfassen` und `bearbeiten` (`pages.py`) wird `int(category_id)` ohne Prüfung gerechnet, und `ItemAnlegen(...)` wirft bei ungültigen Werten eine @@ -164,7 +164,7 @@ ValidationError **im** Handler — beides ergibt einen 500er statt der sonst HTML-Formulare (Dropdown, `maxlength`) verhindern es auf dem normalen Weg — es bricht aber das Muster, das `reservieren` mit try/except vormacht. -### P3 — Reservieren: Prüfen und Setzen sind nicht atomar +### P3 — Reservieren: Prüfen und Setzen sind nicht atomar ✅ ERLEDIGT 14.09.2026 `reservieren` prüft erst `status == available` und schreibt dann. Zwei gleichzeitige Anfragen können beide die Prüfung passieren; der zweite @@ -205,3 +205,16 @@ funktionierende Rate-Limits, aber alle Besucher teilen sich die Zähler über die Proxy-IP. Das ist das kleinere Übel gegenüber frei erfindbaren Absenderadressen. **Beim nächsten Deployment die Proxy-IP eintragen und einmal prüfen, dass `request.client.host` die echte Besucher-IP zeigt.** + +### Umsetzung der drei P3 am 14.09.2026 + +| Punkt | Ergebnis | +|---|---| +| `?limit=abc` | `_limit_lesen()` in `pages.py`: ungültig → Seitengrösse, Deckel 500 | +| Betreiber-Formulare | Angaben werden **vor** dem Bildspeichern geprüft; ValueError → Meldung statt 500 (und keine verwaisten Bilddateien mehr) | +| Reservieren | `crud.reservieren()` schreibt per `UPDATE … WHERE status='available'`; der Verlierer des Wettlaufs bekommt None → „schon weg" | + +Vier neue Tests decken genau diese Fälle ab; alle vier wurden nach der +Hausregel einmal gegen den alten Code laufen gelassen und sind dabei +nachweislich rot geworden. Suite: **85 Tests** (84 lokal grün, der +HEIC-Test braucht `pillow-heif` und läuft im Docker-Testimage). diff --git a/app/crud.py b/app/crud.py index e01b21d..a8df227 100644 --- a/app/crud.py +++ b/app/crud.py @@ -8,7 +8,7 @@ from __future__ import annotations import secrets -from sqlalchemy import func, select +from sqlalchemy import func, select, update from sqlalchemy.orm import Session, selectinload from app.models import Groesse, Item, ItemBild, Kategorie, Status, jetzt @@ -135,17 +135,29 @@ def item_loeschen(db: Session, item: Item) -> list[str]: # --------------------------------------------------------- Reservierung --- -def reservieren(db: Session, item: Item, name: str) -> str: +def reservieren(db: Session, item: Item, name: str) -> str | None: """Reserviert und gibt das Token zurück, mit dem sich das rückgängig - machen lässt. Das Token verlässt die Anwendung nur dieses eine Mal.""" + machen lässt. Das Token verlässt die Anwendung nur dieses eine Mal. + + Prüfen und Setzen stehen in EINEM UPDATE mit Status-Bedingung: zwei + gleichzeitige Anfragen könnten sonst beide die Prüfung in der Route + passieren, und die zweite überschriebe Name und Token der ersten - + ohne dass die es je erfährt. So gewinnt genau eine; die andere + bekommt None und der Aufrufer meldet "schon weg".""" token = secrets.token_urlsafe(32) - item.status = Status.reserved.value - item.reserved_by = name - item.reservation_token = token - item.updated_at = jetzt() + betroffen = db.execute( + update(Item) + .where(Item.id == item.id, Item.status == Status.available.value) + .values( + status=Status.reserved.value, + reserved_by=name, + reservation_token=token, + updated_at=jetzt(), + ) + ).rowcount db.commit() db.refresh(item) - return token + return token if betroffen else None def freigeben(db: Session, item: Item) -> Item: diff --git a/app/routers/items.py b/app/routers/items.py index 3cb26cc..8a7dec4 100644 --- a/app/routers/items.py +++ b/app/routers/items.py @@ -154,6 +154,11 @@ def item_reservieren( status_code=409, detail="Dieses Kleidungsstück ist nicht mehr verfügbar." ) token = crud.reservieren(db, item, daten.reserved_by) + if token is None: + # Zwischen Prüfung und Schreiben war jemand schneller. + raise HTTPException( + status_code=409, detail="Dieses Kleidungsstück ist nicht mehr verfügbar." + ) return ReservierenAus(item=_als_antwort(item), reservation_token=token) diff --git a/app/routers/pages.py b/app/routers/pages.py index b6bef90..c4dd972 100644 --- a/app/routers/pages.py +++ b/app/routers/pages.py @@ -127,9 +127,24 @@ def _liste_daten(db: Session, request: Request, limit: int) -> dict: } +def _limit_lesen(request: Request) -> int: + """?limit= kommt aus der URL und ist damit Nutzereingabe. + + Ohne die Prüfung liefert /?limit=abc jedem anonymen Besucher einen + internen Serverfehler, und limit=-1 heisst in SQLite "alles". Der + Deckel ist bewusst grosszügiger als die 200 der API: "Mehr anzeigen" + wächst in 24er-Schritten und soll den ganzen Bestand erreichen können. + """ + try: + wert = int(request.query_params.get("limit", "")) + except ValueError: + return SEITE + return max(1, min(wert, 500)) + + @router.get("/", response_class=HTMLResponse) def galerie(request: Request, db: Session = Depends(get_db)): - limit = int(request.query_params.get("limit") or SEITE) + limit = _limit_lesen(request) return vorlagen.TemplateResponse( request, "galerie.html", _umgebung(request, db, kategorien=crud.kategorien(db), groessen=crud.groessen(db), **_liste_daten(db, request, limit)), @@ -139,7 +154,7 @@ def galerie(request: Request, db: Session = Depends(get_db)): @router.get("/teil/liste", response_class=HTMLResponse) def liste_ausschnitt(request: Request, db: Session = Depends(get_db)): """Nur die Liste - von HTMX beim Filtern und Nachladen geholt.""" - limit = int(request.query_params.get("limit") or SEITE) + limit = _limit_lesen(request) return vorlagen.TemplateResponse( request, "_liste.html", _umgebung(request, db, **_liste_daten(db, request, limit)) ) @@ -181,6 +196,9 @@ def reservieren( return _weiter(f"/kleid/{item_id}", "Bitte nur einen Namen angeben.", "fehler") token = crud.reservieren(db, item, geprueft.reserved_by) + if token is None: + # Zwischen Prüfung und Schreiben war jemand schneller. + return _weiter(f"/kleid/{item_id}", "Das ist leider schon weg.", "fehler") freigabe = request.url_for("freigeben_per_link", item_id=item.id) return vorlagen.TemplateResponse( request, "reserviert.html", _umgebung(request, db, item=item, freigabe_url=f"{freigabe}?token={token}"), @@ -319,16 +337,28 @@ def erfassen( security.betreiber_noetig(request) _csrf_oder_fehler(request, csrf) + # Erst die Angaben prüfen, dann die Bilder speichern: scheitert die + # Prüfung, lägen sonst schon Dateien ohne Eintrag auf der Platte. + # int() und die Pydantic-Prüfung werfen beide ValueError - ohne das + # Abfangen würde daraus ein 500er statt einer Fehlermeldung. Die + # HTML-Formulare verhindern das zwar auf dem normalen Weg, aber ein + # Handgriff an der Anfrage darf keinen Serverfehler auslösen. + try: + daten = ItemAnlegen( + title=title, description=description, size=size, + category_id=int(category_id), gender=gender, season=season, + condition=condition, + ) + except ValueError: + return _weiter("/erfassen", + "Bitte die Angaben prüfen - Kategorie wählen, " + "Titel höchstens 100 Zeichen.", "fehler") + try: namen = _bilder_speichern(dateien) except images.BildFehler as e: return _weiter("/erfassen", str(e), "fehler") - daten = ItemAnlegen( - title=title, description=description, size=size, - category_id=int(category_id), gender=gender, season=season, - condition=condition, - ) item = crud.item_anlegen(db, daten) for name in namen: crud.bild_anhaengen(db, item, name) @@ -405,19 +435,27 @@ def bearbeiten( if item is None: raise HTTPException(status_code=404, detail="Nicht gefunden.") + war_entwurf = item.status == Status.draft.value + # Wie beim Erfassen: erst prüfen, dann Bilder - und ValueError (int, + # Pydantic) als Meldung statt als 500er. + try: + daten = ItemAendern( + title=title, description=description, size=size, + category_id=int(category_id), gender=gender, season=season, + condition=condition, + # Ein Entwurf wird durchs Nachtragen sichtbar. + status=Status.available if war_entwurf else None, + ) + except ValueError: + return _weiter(f"/kleid/{item_id}/bearbeiten", + "Bitte die Angaben prüfen - Kategorie wählen, " + "Titel höchstens 100 Zeichen.", "fehler") + try: namen = _bilder_speichern(dateien) except images.BildFehler as e: return _weiter(f"/kleid/{item_id}/bearbeiten", str(e), "fehler") - war_entwurf = item.status == Status.draft.value - daten = ItemAendern( - title=title, description=description, size=size, - category_id=int(category_id), gender=gender, season=season, - condition=condition, - # Ein Entwurf wird durchs Nachtragen sichtbar. - status=Status.available if war_entwurf else None, - ) try: crud.item_aendern(db, item, daten) except IntegrityError: diff --git a/tests/test_reservierung.py b/tests/test_reservierung.py index 0d03ea7..e889049 100644 --- a/tests/test_reservierung.py +++ b/tests/test_reservierung.py @@ -89,3 +89,19 @@ def test_name_ohne_werbung(client, item_id): json={"reserved_by": "Jetzt billig kaufen http://spam.example"}, ) assert antwort.status_code == 422 + + +def test_wettlauf_beim_reservieren_gewinnt_nur_einer(client, item_id, db_sitzung): + """Zwei Anfragen können beide die Statusprüfung in der Route passieren, + bevor eine schreibt. Das UPDATE mit Status-Bedingung lässt trotzdem nur + eine gewinnen - die zweite bekommt None und überschreibt nichts.""" + from app import crud + + item = crud.item_holen(db_sitzung, item_id) + erster = crud.reservieren(db_sitzung, item, "A") + zweiter = crud.reservieren(db_sitzung, item, "B") + + assert erster is not None + assert zweiter is None + assert item.reserved_by == "A" + assert item.reservation_token is not None diff --git a/tests/test_seiten.py b/tests/test_seiten.py index b69aa8c..bb3244a 100644 --- a/tests/test_seiten.py +++ b/tests/test_seiten.py @@ -263,3 +263,26 @@ def test_farbschema_skript_laeuft_vor_dem_zeichnen(client): def test_umschalter_ist_vorhanden(client): assert 'id="schema-knopf"' in client.get("/").text + + +# ------------------------------------------------- Unsinnige Eingaben --- + +def test_unsinniges_limit_ist_kein_serverfehler(gast): + """?limit= kommt aus der URL - Buchstaben oder negative Werte dürfen + keinen 500er auslösen, sondern fallen auf die Seitengrösse zurück.""" + assert gast.get("/?limit=abc").status_code == 200 + assert gast.get("/teil/liste?limit=-5").status_code == 200 + + +def test_unsinnige_kategorie_gibt_meldung_statt_500(client, kategorie_id): + antwort = erfasse(client, kategorie_id, category_id="abc") + assert antwort.status_code == 200 + assert "Bitte die Angaben prüfen" in antwort.text + + +def test_zu_langer_titel_gibt_meldung_statt_500(client, kategorie_id): + """maxlength im HTML schützt nur den Browserweg - die Anwendung muss + es selbst abfangen.""" + antwort = erfasse(client, kategorie_id, title="x" * 101) + assert antwort.status_code == 200 + assert "Bitte die Angaben prüfen" in antwort.text