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