Skip to content

FormRequest: body-only input on write methods, with opt-in query merge - #339

Closed
tmgbedu wants to merge 1 commit into
mainfrom
task/formrequest-body-only-input
Closed

tmgbedu wants to merge 1 commit into
mainfrom
task/formrequest-body-only-input

Conversation

@tmgbedu

@tmgbedu tmgbedu commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Since #336 (ADR 053), Validated[Model] merged query-string params into the input of every request that has a JSON body. That meant POST /login?password=… validated, so credentials could end up in access logs. It also meant PATCH /profile?role=admin put role into model_fields_set, which is a mass-assignment risk.

This PR changes that. ADR 056 amends ADR 053.

  • Write methods (POST, PUT, PATCH, and a DELETE with a body) now validate the body only. Query keys are ignored and never reach model_fields_set.
  • Safe methods (GET, HEAD, OPTIONS) and body-less DELETE behave as before: they read the query, and merge it with an object body.
  • Opt-in: merge_query: ClassVar[bool] = False on FormRequest. Set it to True to get the old {**query, **body} merge back (the body wins). The name follows the existing stop_on_first_failure setting. The ADR explains why this name was picked over query_on_write and accepts_query.
  • Per request: I didn't add a new hook. prepare_for_validation already gets the request, so a model can merge the query itself with cls.query_input(validator.request.query_params). A test covers this.
  • Embedded models: if a model's embed key is absent or null in a write body, it validates {} and reports its required rules under its prefix. With merge_query = True, or on a safe method, it reads the query as before.
  • request_input(params, body, *, method) now needs method as a keyword, so a direct caller can't fall back to the merge by accident.
  • Unchanged: the CSRF guard (a body-less write still returns "": ["Field required"]), model_type errors for non-object bodies, aliases and repeated keys, error shapes, and the OpenAPI query documentation.

Tests

  • POST/PUT/PATCH with a body plus query params ignore the query by default. This includes /login?password=….
  • With merge_query = True, the query is merged and the body wins, on all three methods.
  • model_fields_set no longer contains query keys on writes.
  • A DELETE with a body reads the body only. A GET with a body still merges the query.
  • Embedded cases: an absent key on a POST gives search.term required. An opted-in model reads the query and its errors are combined with the other models'. An absent key on a GET reads the query.
  • The per-request merge through prepare_for_validation.
  • I ran the 7 default-behaviour tests against the old logic and they all fail there.

uv run pytest --ignore=tests/masoniteorm/postgres --cov: 3726 passed, 92.75% coverage. ruff is clean. basedpyright reports 0 errors.

Behaviour change

Apps that relied on query params filling body fields on write methods must set merge_query = True on those FormRequests.

🤖 Generated with Claude Code

POST/PUT/PATCH (and DELETE with a body) no longer merge the query string
into the FormRequest input, so credentials in URLs are not accepted and
query keys cannot reach model_fields_set. Safe methods and body-less
DELETE keep reading the query. FormRequest.merge_query = True restores
the {**query, **body} merge for models that want $request->all() parity.

ADR 056 amends ADR 053.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@tmgbedu

tmgbedu commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Closing. Mass assignment is protected by the model (fillable), not by where FormRequest reads its input. ADR 053's query + body merge (Laravel $request->all() parity) stays as is.

@tmgbedu tmgbedu closed this Oct 31, 2026
@claude-tmgbedu
claude-tmgbedu deleted the task/formrequest-body-only-input branch October 31, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant