diff --git a/docs/adr/053-validated-reads-query-and-body.md b/docs/adr/053-validated-reads-query-and-body.md index 5ef9afc1..d1249833 100644 --- a/docs/adr/053-validated-reads-query-and-body.md +++ b/docs/adr/053-validated-reads-query-and-body.md @@ -4,6 +4,8 @@ Date: 2026-10-31 Extends the FastAPI integration of [ADR 048](048-async-first-validation.md). +Status: amended by [ADR 056](056-validated-write-methods-read-the-body-only.md). Write methods with a body now read the body only, unless the model sets `merge_query = True`. + ## Problem `Validated[Model]` declared its input as a required `Body()`, so a `FormRequest` could only validate a JSON body. A `GET` endpoint with filters (`/lexemes?window=today&page=2`) could not use the same class: FastAPI answered `422 body: Field required` before the dependency ran, even when every query parameter was valid. Applications fell back to binding the model with `Annotated[Model, Query()]`, which only runs pydantic, so `authorize`, `prepare_for_validation`, the fluent rules and `after` were silently skipped. @@ -34,6 +36,8 @@ Option 3. ### Query input on write methods +Amended by [ADR 056](056-validated-write-methods-read-the-body-only.md): this merge now needs `merge_query = True`. + Write methods with a JSON body still merge the query, as Laravel's `$request->all()` does. As a result, a declared field can be set from the URL: `PATCH /profile?role=admin` with `{"name": "n"}` validates with `role == "admin"`, and `role` is in `model_fields_set`, so `update(data.model_dump(exclude_unset=True))` writes it. This gives the client no new power. The same client can put `role` in the body, the model's declared fields remain the only assignable fields, and the cross-site path is closed because the merge needs a JSON body. URLs end up in access logs and proxies, so a sensitive field (a role or a password) must not be accepted from the query. Exclude the field from the model, or reject it in `prepare_for_validation` or `after`. Keeping the merge lets one model take its filters from the URL and its payload from the body. ### OpenAPI diff --git a/docs/adr/056-validated-write-methods-read-the-body-only.md b/docs/adr/056-validated-write-methods-read-the-body-only.md new file mode 100644 index 00000000..50e74c7f --- /dev/null +++ b/docs/adr/056-validated-write-methods-read-the-body-only.md @@ -0,0 +1,49 @@ +# 056. `Validated` write methods read the body only, with an opt-in query merge + +Date: 2026-10-31 + +Amends [ADR 053](053-validated-reads-query-and-body.md). + +## Problem + +Since ADR 053, `Validated[Model]` merges the query string into the input of every request that has a JSON body, on every method. Two consequences on write methods: + +- **Secrets in URLs.** `POST /login?password=secret` with `{"email": "a@b.c"}` validates. The client learns that the URL is an accepted channel, and the password ends up in access logs, proxy logs and browser history. +- **Mass assignment from the URL.** `PATCH /profile?role=admin` with `{"name": "n"}` validates with `role == "admin"` and `role` in `model_fields_set`, so `update(data.model_dump(exclude_unset=True))` writes it. ADR 053 asked developers to keep sensitive fields off such models, which is easy to forget. The PR #336 review flagged it. + +Write payloads belong in the body. The query string is the natural input of safe methods, where ADR 053 keeps working well. + +## Alternatives + +1. **Keep the merge, document the risk (ADR 053).** Safe only when every model is written with the risk in mind. +2. **Drop query input on write methods with no way back.** Breaks applications that rely on Laravel's `$request->all()` parity, for example a `POST /search?page=2` with filters in the body. +3. **Body only on write methods by default, with a class-level opt-in.** A `FormRequest` that wants the merge says so once, next to its fields. Per-request control is already possible with `prepare_for_validation`, so no extra hook is needed. +4. **A per-request hook (`merges_query(request) -> bool`).** More API for a rare need that `prepare_for_validation` covers. + +## Decision + +Option 3. + +- **Safe methods** (`GET`, `HEAD`, `OPTIONS`) keep ADR 053's behaviour: the query input alone without a body, `{**query, **body}` with an object body. +- **Body-less `DELETE`** keeps reading the query input. A `DELETE` with a body is a write and reads the body only. +- **Write methods with a body** (`POST`, `PUT`, `PATCH`, a `DELETE` with a body, and any other method) validate the body alone. Query keys are ignored, so they never reach `model_fields_set`. +- **Opt-in:** `merge_query: ClassVar[bool] = False` on `FormRequest`. With `merge_query = True`, write methods validate `{**query_input, **body}` (the body wins on a clash), as ADR 053 did. The name says what happens (the query is merged under the body) and follows the existing `stop_on_first_failure` class setting. `query_on_write` and `accepts_query` were rejected: the first hides that the body wins, the second suggests the query is accepted alone. +- **Per request:** `prepare_for_validation` receives the `Validator` with the request, so a model can merge the query only when it wants: `validator.data = {**cls.query_input(validator.request.query_params), **validator.data}`. +- **Embedded models.** With several body parameters, a model whose embed key is absent or `null` in a present body validates an empty object on write methods, so its `required` rules report under its prefix (`search.term`). With `merge_query = True`, or on a safe method, it reads the query input as before. +- Unchanged: a body-less write method still reports `"": ["Field required"]` (per embed key when embedded) before `authorize` runs, so the CSRF protection of ADR 053 stands. A non-object body (a list, a string, a JSON `null`) still fails with `model_type` under `""`. Aliases, repeated keys for sequences, the error shapes and the OpenAPI query parameters of query methods are unchanged. + +### Implementation + +- `form_request.py` adds `SAFE_METHODS = {"GET", "HEAD", "OPTIONS"}`. `QUERY_METHODS` (safe methods plus `DELETE`) still decides which body-less requests read the query. +- `FormRequest.request_input(params, body=ABSENT, *, method)` takes the HTTP method as a required keyword. Without a body it returns `query_input`. A non-object body passes through. An object body is merged with the query input when `merge_query` is set or the method is safe, and returned alone otherwise. The keyword is required so a direct caller cannot fall back to the merge by accident. +- The `Validated` dependency passes `request.method` and turns a missing or `null` embed key in a present body into `{}`. +- `document_query_input` is unchanged. Write methods never listed query parameters. With `merge_query = True` the merged fields are still documented only by the body schema. + +### Behaviour changes + +- `POST`/`PUT`/`PATCH` (and a `DELETE` with a body) no longer read the query string unless the model sets `merge_query = True`. A field sent only in the URL is now missing: a required field reports its `required` message, an optional field keeps its default. +- On those methods, an embedded model whose key is absent from the body no longer reads the query; it reports its `required` rules. + +## Validation + +`tests/validation/test_fastapi_validation.py` covers: `POST`, `PUT` and `PATCH` with a JSON body and query parameters ignoring the query by default; the same routes with a `merge_query` model merging the query with the body winning; `model_fields_set` without query keys on a `PATCH`; a `DELETE` with a body reading the body only; `GET` still reading the query and merging it with a body; the per-request merge in `prepare_for_validation`; an embedded model with an absent key on a `POST` reporting its `required` rules by default and reading the query when it opts in. The body-less CSRF tests, the non-object body tests and the OpenAPI tests of ADR 053 still pass. The full framework suite passes with coverage above the threshold, and `ruff` and `basedpyright` are clean. diff --git a/docs/adr/AGENTS.md b/docs/adr/AGENTS.md index 86b6967c..ef7b5ac2 100644 --- a/docs/adr/AGENTS.md +++ b/docs/adr/AGENTS.md @@ -46,3 +46,4 @@ Read this index first, then open the records relevant to the change. Before impl | [051](051-nullable-column-casts.md) | 2026-10-31 | Nullable annotations infer the cast of their inner type | `Caster.normalize_type` only knew bare types, so `int \| None`, `Optional[int]` and `Union[int, None]` fell through to the `"str"` cast and nullable columns (e.g. a `HasMany` foreign key) were read back as strings, breaking eager-load key matching. `Caster.build` (replacing `build_casts`) removes `None` from a top-level union, also around or inside `Annotated`, maps the inner type to its usual cast and records the field as nullable. For nullable fields only, `Caster.get` turns `None` into `None` (or the `Field` default) and `Caster.set` writes `None`; bare annotations keep their behaviour. Unions of several non-`None` types get a pass-through `BaseCast`. Relationship key matching stays type-strict. | | [052](052-sqlite-savepoint-isolation.md) | 2026-10-31 | SQLite savepoints start a real transaction | pysqlite/aiosqlite defer `BEGIN` until the first DML, so a savepoint opened first in an outer transaction became a top-level transaction and `RELEASE` committed it, leaking rows past `DatabaseTransaction`/`RefreshDatabase` rollbacks. `SQliteConnection` listens to the engine `savepoint` event and runs `BEGIN` when the driver connection is not in a transaction. On the shared `StaticPool` connection, a lost `BEGIN` race (`OperationalError` while a transaction is already open) is ignored, so concurrent nested transactions still succeed as on `main`. SQLAlchemy's `isolation_level=None` + `BEGIN` on every `begin` recipe was rejected because concurrent transactions on `SQLiteConfig`'s shared `StaticPool` connection then fail with `cannot start a transaction within a transaction`. | | [053](053-validated-reads-query-and-body.md) | 2026-10-31 | `Validated` reads the query string and the body | `Validated[Model]` declared a required `Body()`, so a `GET` with filters got `422 body: Field required` and apps bound models with `Query()`, skipping `authorize`, rules and `after`. The body parameter now defaults to `None`. Without a body, `GET`/`HEAD`/`OPTIONS`/`DELETE` validate `query_input`, while `POST`/`PUT`/`PATCH` still report `"": Field required` (per embed key when embedded), so a body-less cross-site form `POST` cannot pass on the query string (CSRF). `DELETE` always needs a CORS preflight. With a body, `request_input` merges `{**query, **body}` for an object; any other body, a JSON `null` included, passes through and fails with `model_type`. `query_input` keeps the keys pydantic accepts (`validation_alias`, `AliasChoices`, single-key `AliasPath`, the name with `validate_by_name`/`populate_by_name`). `collections.abc` `Sequence`/`Set` fields (not `str`/`bytes`) get every repeated value. Declared fields can be set from the URL on write methods with a JSON body, as with Laravel's `$request->all()`: keep sensitive fields off such models. `document_query_input` (installed by `FastAPIProvider`) replaces `requestBody` with `in: query` parameters on query-method routes. Chosen over a separate `ValidatedQuery` so one `FormRequest` serves `GET` and `POST`. | +| [056](056-validated-write-methods-read-the-body-only.md) | 2026-10-31 | `Validated` write methods read the body only, with an opt-in query merge | Amends ADR 053, whose `{**query, **body}` merge on every method let `POST /login?password=…` validate (secrets in logs) and `PATCH /profile?role=admin` set `role` in `model_fields_set` (mass assignment). `request_input(params, body, *, method)` now merges the query only on `GET`/`HEAD`/`OPTIONS` (`SAFE_METHODS`) or when the model sets `merge_query: ClassVar[bool] = True` (named like `stop_on_first_failure`; the body wins). `POST`/`PUT`/`PATCH` and a `DELETE` with a body validate the body alone; a body-less `DELETE` still reads the query. An embedded model whose key is absent or `null` in a write body validates `{}` (its `required` rules under its prefix) unless it opts in. Per-request merging uses `prepare_for_validation` with `cls.query_input(validator.request.query_params)`, so no new hook. Body-less writes still report `"": Field required` (CSRF), non-object bodies still fail with `model_type`, OpenAPI is unchanged. | diff --git a/fastapi_startkit/src/fastapi_startkit/validation/form_request.py b/fastapi_startkit/src/fastapi_startkit/validation/form_request.py index 0e376d8f..679cbda7 100644 --- a/fastapi_startkit/src/fastapi_startkit/validation/form_request.py +++ b/fastapi_startkit/src/fastapi_startkit/validation/form_request.py @@ -38,7 +38,8 @@ RECORDED = "fastapi_startkit.validation.recorded" PENDING_ERRORS = "fastapi_startkit.validation.errors" VALIDATED_KEY = "__validated_key__" -QUERY_METHODS = frozenset({"GET", "HEAD", "OPTIONS", "DELETE"}) +SAFE_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) +QUERY_METHODS = SAFE_METHODS | {"DELETE"} TEXT_TYPES = (str, bytes, bytearray) ABSENT: Any = object() @@ -92,6 +93,7 @@ def rule_fields(cls) -> Iterator[tuple[str, Field]]: yield name, metadata stop_on_first_failure: ClassVar[bool] = False + merge_query: ClassVar[bool] = False @classmethod def authorize(cls, validator: Validator) -> bool | Awaitable[bool]: @@ -162,10 +164,10 @@ def query_input(cls, params: QueryParams) -> dict[str, Any]: } @classmethod - def request_input(cls, params: QueryParams, body: Any = ABSENT) -> Any: + def request_input(cls, params: QueryParams, body: Any = ABSENT, *, method: str) -> Any: if body is ABSENT: return cls.query_input(params) - if isinstance(body, Mapping): + if isinstance(body, Mapping) and (cls.merge_query or method in SAFE_METHODS): return {**cls.query_input(params), **body} return body @@ -312,9 +314,10 @@ async def dependency(**parameters: Any) -> Any: if not raw and request.method not in QUERY_METHODS: errors.add(key if embedded else "", "Field required") else: - body = parameters[key] if raw and not (embedded and parameters[key] is None) else ABSENT + body = ABSENT if not raw else {} if embedded and parameters[key] is None else parameters[key] + data = model.request_input(request.query_params, body, method=request.method) try: - instance = await model.validate_input(model.request_input(request.query_params, body), context, request) + instance = await model.validate_input(data, context, request) except ValidationException as error: errors.merge(error.bag, prefix=key if embedded else None) if errors.any() and (route is None or route.last is dependency): diff --git a/fastapi_startkit/tests/validation/test_fastapi_validation.py b/fastapi_startkit/tests/validation/test_fastapi_validation.py index 0ea74750..03e00c53 100644 --- a/fastapi_startkit/tests/validation/test_fastapi_validation.py +++ b/fastapi_startkit/tests/validation/test_fastapi_validation.py @@ -96,6 +96,26 @@ class Profile(FormRequest): role: str = "member" +class MergedProfile(Profile): + merge_query = True + + +class MergedSearch(Search): + merge_query = True + + +class Login(FormRequest): + email: Annotated[str, Email().required()] + password: Annotated[str, Field().required()] + + +class SearchOnRequest(Search): + @classmethod + def prepare_for_validation(cls, validator: Validator) -> None: + if validator.request is not None and validator.request.headers.get("x-merge-query"): + validator.data = {**cls.query_input(validator.request.query_params), **validator.data} + + class Paging(FormRequest): model_config = ConfigDict(populate_by_name=True) @@ -187,10 +207,30 @@ async def hidden(query: Validated[Search]): async def transfer(data: Validated[Transfer]): return data.model_dump() + @app.fastapi.post("/profile") + @app.fastapi.put("/profile") @app.fastapi.patch("/profile") async def profile(data: Validated[Profile]): return {"set": sorted(data.model_fields_set), **data.model_dump()} + @app.fastapi.post("/merged/profile") + @app.fastapi.put("/merged/profile") + @app.fastapi.patch("/merged/profile") + async def merged_profile(data: Validated[MergedProfile]): + return {"set": sorted(data.model_fields_set), **data.model_dump()} + + @app.fastapi.post("/merged/search/billing") + async def merged_search_billing(query: Validated[MergedSearch], billing: Validated[Billing]): + return {"term": query.term, "billing": billing.name} + + @app.fastapi.post("/login") + async def login(data: Validated[Login]): + return {"email": data.email} + + @app.fastapi.post("/search/on-request") + async def search_on_request(data: Validated[SearchOnRequest]): + return {"term": data.term} + @app.fastapi.get("/paging") async def paging(query: Validated[Paging]): return query.model_dump() @@ -461,8 +501,8 @@ async def test_validated_runs_authorize(self): denied.assert_status(403) async def test_validated_and_validated_body_errors_are_combined(self): - valid = await self.post("/search/billing?term=paris", json={"billing": {"name": "abcd"}}) - invalid = await self.post("/search/billing?term=p", json={"billing": {"name": "a"}}, headers=JSON) + valid = await self.post("/merged/search/billing?term=paris", json={"billing": {"name": "abcd"}}) + invalid = await self.post("/merged/search/billing?term=p", json={"billing": {"name": "a"}}, headers=JSON) valid.assert_status(200) self.assertEqual(valid.json(), {"term": "paris", "billing": "abcd"}) @@ -470,7 +510,7 @@ async def test_validated_and_validated_body_errors_are_combined(self): self.assertEqual( invalid.json()["errors"], { - "search.term": ["The term field must be at least 2 characters."], + "merged_search.term": ["The term field must be at least 2 characters."], "billing.name": ["The name field must be at least 3 characters."], }, ) @@ -481,11 +521,70 @@ async def test_body_values_override_query_values_of_the_same_field(self): response.assert_status(200) self.assertEqual(response.json(), {"email": "body@example.com"}) - async def test_query_values_fill_fields_missing_from_the_body(self): + async def test_query_is_ignored_on_write_methods_by_default(self): response = await self.post("/requests?email=query@example.com", json={}) + response.assert_status(422) + self.assertEqual(response.json()["errors"], {"email": ["The email field is required."]}) + + async def test_credentials_in_the_query_are_not_read_on_post(self): + response = await self.post("/login?password=secret", json={"email": "a@example.com"}) + + response.assert_status(422) + self.assertEqual(response.json()["errors"], {"password": ["The password field is required."]}) + + async def test_post_put_and_patch_validate_the_body_only(self): + for method in ("POST", "PUT", "PATCH"): + response = await self.client.request(method, "/profile?name=query&role=admin", json={"name": "n"}) + + self.assertEqual(response.status_code, 200, method) + self.assertEqual(response.json(), {"set": ["name"], "name": "n", "role": "member"}, method) + + async def test_merge_query_merges_the_query_with_the_body_winning(self): + for method in ("POST", "PUT", "PATCH"): + response = await self.client.request(method, "/merged/profile?name=query&role=admin", json={"name": "n"}) + + self.assertEqual(response.status_code, 200, method) + self.assertEqual(response.json(), {"set": ["name", "role"], "name": "n", "role": "admin"}, method) + + async def test_merge_query_fills_fields_missing_from_the_body(self): + response = await self.patch("/merged/profile?name=query", json={}) + response.assert_status(200) - self.assertEqual(response.json(), {"email": "query@example.com"}) + self.assertEqual(response.json(), {"set": ["name"], "name": "query", "role": "member"}) + + async def test_prepare_for_validation_can_merge_the_query_per_request(self): + merged = await self.post("/search/on-request?term=paris", json={}, headers={"x-merge-query": "1"}) + ignored = await self.post("/search/on-request?term=paris", json={}, headers=JSON) + + merged.assert_status(200) + self.assertEqual(merged.json(), {"term": "paris"}) + ignored.assert_status(422) + self.assertEqual(ignored.json()["errors"], {"term": ["The term field is required."]}) + + async def test_delete_with_a_body_validates_the_body_only(self): + response = await self.client.request("DELETE", "/listing?page=3&q=abc", json={"page": 2}) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json(), {"window": "today", "q": "", "page": 2, "tags": [], "archived": False}) + + async def test_get_with_a_body_still_merges_the_query(self): + response = await self.client.request("GET", "/listing?page=3&q=abc", json={"page": 2}) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json(), {"window": "today", "q": "abc", "page": 2, "tags": [], "archived": False}) + + async def test_embedded_model_missing_from_a_write_body_ignores_the_query(self): + response = await self.post("/search/billing?term=paris", json={"billing": {"name": "abcd"}}, headers=JSON) + + response.assert_status(422) + self.assertEqual(response.json()["errors"], {"search.term": ["The term field is required."]}) + + async def test_embedded_model_missing_from_a_get_body_reads_the_query(self): + response = await self.client.request("GET", "/search/notes?term=paris", json={"note": "hi"}) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json(), {"term": "paris", "note": "hi"}) async def test_openapi_documents_the_request_body_as_optional(self): schema = (await self.get("/openapi.json")).json() @@ -518,17 +617,11 @@ async def test_non_form_body_is_rejected_even_with_a_valid_query(self): response.assert_status(422) self.assertEqual(list(response.json()["errors"]), [""]) - async def test_json_body_is_merged_with_the_query_on_write_methods(self): - response = await self.post("/transfer?to=friend", json={"amount": 5}) - - response.assert_status(200) - self.assertEqual(response.json(), {"to": "friend", "amount": 5}) - - async def test_query_values_of_declared_fields_are_set_on_write_methods(self): + async def test_query_values_of_declared_fields_are_not_set_on_write_methods(self): response = await self.patch("/profile?role=admin&unknown=1", json={"name": "n"}) response.assert_status(200) - self.assertEqual(response.json(), {"set": ["name", "role"], "name": "n", "role": "admin"}) + self.assertEqual(response.json(), {"set": ["name"], "name": "n", "role": "member"}) async def test_json_null_body_is_rejected_as_a_non_object_body(self): post = await self.post(