From 98c4a66da128e89bba8fbc1e258f5a2e500b3bc8 Mon Sep 17 00:00:00 2001 From: Timothy Allen Date: Fri, 2 Oct 2026 21:21:08 +0200 Subject: [PATCH] Answer 400 for a request value of the wrong type A value such as chapter_ids that is not a list of numbers gave a 500. --- bindery/web.py | 73 +++++++++++++++++++++++++++++++---------- tests/test_api_flows.py | 34 +++++++++++++++++++ 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/bindery/web.py b/bindery/web.py index bbdae5d..1cd33ee 100644 --- a/bindery/web.py +++ b/bindery/web.py @@ -92,6 +92,46 @@ def _json() -> dict[str, Any]: return data if isinstance(data, dict) else {} +# Values of a request body. A value of the wrong type is an error of the request, so it gets a 400. + + +def _list(data: dict[str, Any], name: str) -> list[Any]: + value = data.get(name) + if value is None: + return [] + if not isinstance(value, list): + raise ValueError(f"{name} must be a list") + return value + + +def _int(value: Any, name: str) -> int: + try: + return int(value) + except (TypeError, ValueError): + raise ValueError(f"{name} must be a whole number") from None + + +def _ids(data: dict[str, Any], name: str = "chapter_ids") -> list[int]: + return [_int(v, name) for v in _list(data, name)] + + +def _rules(data: dict[str, Any]) -> list[dict[str, Any]]: + rules = _list(data, "rules") + if not all(isinstance(r, dict) for r in rules): + raise ValueError("rules must be a list of rules") + return rules + + +def _names(data: dict[str, Any], name: str) -> list[str] | None: + """A list of text values, or None if the request leaves it out.""" + if data.get(name) is None: + return None + values = _list(data, name) + if not all(isinstance(v, str) for v in values): + raise ValueError(f"{name} must be a list of names") + return values + + # Login gate and errors @@ -259,9 +299,8 @@ def stories() -> Any: @app.post("/api/stories") def add_stories() -> Any: - urls = _json().get("urls", []) - if isinstance(urls, str): - urls = re.split(r"\s+", urls) + data = _json() + urls = re.split(r"\s+", data["urls"]) if isinstance(data.get("urls"), str) else _list(data, "urls") queued, errors = orchestrator.queue_urls([str(u) for u in urls]) return {"jobs": queued, "errors": errors} @@ -294,15 +333,19 @@ def download(story_id: int) -> Any: data = _json() # A rebuild that the user asks for also builds and sends a book that did not change. return orchestrator.queue_story( - story_id, data.get("mode", DownloadMode.NEW.value), data.get("chapter_ids"), data.get("force_fields"), True + story_id, str(data.get("mode", DownloadMode.NEW.value)), _ids(data), _names(data, "force_fields"), True ) @app.put("/api/stories//settings") def story_settings(story_id: int) -> Any: data = _json() + days = data.get("recheck_days") orchestrator.update_story_settings( - story_id, data.get("locked_fields"), data.get("auto_update"), data.get("recheck_days") + story_id, + _names(data, "locked_fields"), + data.get("auto_update"), + None if days is None else _int(days, "recheck_days"), ) return orchestrator.story_details(story_id)["story"] @@ -330,23 +373,19 @@ def approve_upload(story_id: int) -> Any: @app.post("/api/stories//chapters") def chapters_included(story_id: int) -> Any: data = _json() - return orchestrator.set_chapters_included( - story_id, [int(c) for c in data.get("chapter_ids", [])], bool(data.get("included", True)) - ) + return orchestrator.set_chapters_included(story_id, _ids(data), bool(data.get("included", True))) @app.post("/api/stories//chapters/move") def move_chapters(story_id: int) -> Any: data = _json() after = data.get("after") - return orchestrator.move_chapters( - story_id, [int(c) for c in data.get("chapter_ids", [])], None if after is None else int(after) - ) + return orchestrator.move_chapters(story_id, _ids(data), None if after is None else _int(after, "after")) @app.post("/api/stories//chapters/delete") def delete_chapters(story_id: int) -> Any: - return orchestrator.delete_chapters(story_id, [int(c) for c in _json().get("chapter_ids", [])]) + return orchestrator.delete_chapters(story_id, _ids(_json())) @app.get("/api/stories//epub") @@ -377,7 +416,7 @@ def story_cover(story_id: int) -> Any: def rules(story_id: int | None = None) -> Any: if request.method == "PUT": data = _json() - return orchestrator.save_rules(story_id, data.get("rules", []), data.get("paragraphs", ...)) + return orchestrator.save_rules(story_id, _rules(data), data.get("paragraphs", ...)) return orchestrator.get_rules(story_id) @@ -387,8 +426,8 @@ def rules_preview(story_id: int | None = None) -> Any: data = _json() return orchestrator.preview_rules( story_id, - data.get("rules", []), - int(data.get("chapter_id", 0)), + _rules(data), + _int(data.get("chapter_id", 0), "chapter_id"), bool(data.get("source")), data.get("paragraphs"), ) @@ -406,7 +445,7 @@ def chapter_diff(chapter_id: int) -> Any: @app.post("/api/chapters//revision") def chapter_revision(chapter_id: int) -> Any: - return orchestrator.set_chapter_revision(chapter_id, int(_json().get("rev", 0))) + return orchestrator.set_chapter_revision(chapter_id, _int(_json().get("rev", 0), "rev")) @app.delete("/api/chapters//revisions/") @@ -430,7 +469,7 @@ def library_search() -> Any: @app.post("/api/library/import") def library_import() -> Any: data = _json() - books = [b for b in data.get("books", []) if isinstance(b, dict) and "id" in b] + books = [{**b, "id": _int(b["id"], "id")} for b in _list(data, "books") if isinstance(b, dict) and "id" in b] return orchestrator.queue_import(books, bool(data.get("check", True))) diff --git a/tests/test_api_flows.py b/tests/test_api_flows.py index 524cfcf..6fd7f93 100644 --- a/tests/test_api_flows.py +++ b/tests/test_api_flows.py @@ -152,3 +152,37 @@ def test_unexpected_backend_error_returns_json(client, monkeypatch, caplog): assert response.status_code == 500 assert response.json == {"error": "Bindery hit an unexpected error. The log has the details."} assert "RuntimeError: broken" in caplog.text + + +@pytest.mark.parametrize( + ("method", "path", "body", "error"), + [ + ("post", "stories", {"urls": 5}, "urls must be a list"), + ("post", "stories/{sid}/chapters", {"chapter_ids": 5}, "chapter_ids must be a list"), + ("post", "stories/{sid}/chapters/move", {"chapter_ids": [[1]]}, "chapter_ids must be a whole number"), + ("post", "stories/{sid}/chapters/move", {"chapter_ids": [1], "after": [2]}, "after must be a whole number"), + ("post", "stories/{sid}/chapters/delete", {"chapter_ids": "all"}, "chapter_ids must be a list"), + ("post", "stories/{sid}/download", {"mode": "recheck", "chapter_ids": ["x"]}, "chapter_ids must be a whole"), + ("post", "stories/{sid}/download", {"force_fields": "title"}, "force_fields must be a list"), + ("post", "stories/{sid}/download", {"force_fields": [["title"]]}, "force_fields must be a list of names"), + ("put", "stories/{sid}/settings", {"locked_fields": [{}]}, "locked_fields must be a list of names"), + ("put", "stories/{sid}/settings", {"recheck_days": [7]}, "recheck_days must be a whole number"), + ("put", "rules", {"rules": ["a"]}, "rules must be a list of rules"), + ("post", "rules/preview", {"rules": [], "chapter_id": None}, "chapter_id must be a whole number"), + ("post", "chapters/{cid}/revision", {"rev": {}}, "rev must be a whole number"), + ("post", "library/import", {"books": 7}, "books must be a list"), + ("post", "library/import", {"books": [{"id": "x"}]}, "id must be a whole number"), + ], +) +def test_a_value_of_the_wrong_type_is_a_bad_request(client, story, method, path, body, error): + sid, cid = story + response = getattr(client, method)("/api/" + path.format(sid=sid, cid=cid), json=body, headers=H) + assert response.status_code == 400 + assert error in response.json["error"] + + +def test_chapter_requests_without_ids_name_what_to_select(client, story): + sid, _ = story + for body in ({}, {"chapter_ids": None}): + response = client.post(f"/api/stories/{sid}/chapters/delete", json=body, headers=H) + assert (response.status_code, response.json["error"]) == (400, "Select the chapters to delete")