Page MenuHomePhabricator

REST: clarify the relationship between "post" and "body" parameters
Open, In Progress, HighPublic

Description

Currently, the REST framework supports both "post" and "body" as PARAM_SOURCE values returned by getParamSettings. They behave similarly, but not identically:

"post" parameters:

  • support form data requests bodies, but not JSON request bodies
  • are accessible through getValidatedParams, but not getValidatedBody
  • do not support complex values (arrays), but do support multi-value

"body" parameters:

  • support both form data requests bodies as well as JSON request bodies
  • are not accessible through getValidatedParams, but through getValidatedBody
  • do support complex values (arrays) as well as "multi-value" strings

This is somewhat confusing, especially if they are mixed. We should try to clarify this situation.

Proposal

Deprecate the use of "post" parameters in the REST API. Use "body" parameters only. Support form data only if getSupportedRequestTypes() returns the relevant MIME types.

Search for code that still uses "post" parameters: https://codesearch.wmcloud.org/search/?q=SOURCE+%3D%3E+%27post%27

Original Proposal

  1. Do not allow "post" and "body" parameters to be mixed. A handler can only use one of these two kinds of param sources.
  2. Do not support form data requests if "body" is used.
  3. Allow "post" params to function with JSON requests
  4. Potentially, we might introduce a getBodyParamSettings method, to allow body fields and path params to have overlapping names.

This way, endpoints that only need simple data from POST requests can opt into supporting form-data (in addition to JSON) by using "post" parameters. Such endpoints would handle body fields just like query parameters, which is typical for HTML form handling.

This however means that no complex fields can be added, not even optionally. Handlers that want to support complex data in the request body (now or in the future) would use "body" parameters. Such handlers would require a JSON body (or a custom implementation of parseBodyData), and would use getValidatedBody to access the request data.

NOTE: TokenAwareHandlerTrait::getTokenParamDefinition() will need to be updated to take the source as a parameter.

Event Timeline

daniel updated the task description. (Show Details)

The concept of "post" parameters makes more sense once you realize their name is a reference to $_POST, not POST bodies in general. So in that sense extending them to JSON fields would actually be more confusing.

Given that form data handling is not that useful in the REST API in general, maybe just deprecate post parameters for that API entirely?

The concept of "post" parameters makes more sense once you realize their name is a reference to $_POST, not POST bodies in general. So in that sense extending them to JSON fields would actually be more confusing.

Given that form data handling is not that useful in the REST API in general, maybe just deprecate post parameters for that API entirely?

That's what I did initially, and ended up causeing a train blocker: T362817: PHP Deprecated: The "post" source is deprecated, use "body" instead [Called from MediaWiki\Rest\Validator\ParamValidatorCallbacks::getValue]. This is my attempt of being more careful and retainign backwards compatibility.

I agree that form data processing isn't what you'd typicalyl use a REST API for, but apparently people are indeed doing it: https://codesearch.wmcloud.org/things/?q=SOURCE%20%3D%3E%20%27post%27. A prominent use case seems to be OAuth, though I haven't investigated how or why.

Change #1024459 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] REST: introduce getBodyParamSettings

https://gerrit.wikimedia.org/r/1024459

I'd deprecate "post", and pass "body" parameters to the BodyValidator (which is based on the mime type and could be a FormDataBodyValidator, so form data support is easy to preserve although old code would have to be updated).

The OAuth 2 spec does make use of application/x-www-form-urlencoded for some endpoints, no form data though I think.

I'd deprecate "post", and pass "body" parameters to the BodyValidator (which is based on the mime type and could be a FormDataBodyValidator, so form data support is easy to preserve although old code would have to be updated).

The OAuth 2 spec does make use of application/x-www-form-urlencoded for some endpoints, no form data though I think.

The "body" source already supports taking info from $_POST, because PSR-7 wants $_POST to be available through RequestInterface::getParsedBody(). So that would be automatic.

But the handler would have to use getValidatedBody instead of getValidatedParams().

It's doable. The conceptual question is, I think, wether we want to deprecate the idea of form data as "parameters", and make handlers think of it as "request body" instead. The former is the traditional approach in PHP, the latter is technically more correct and mroe in line with OAS.

I was originally going in that direction. I filed this ticket to explore the alternative approach, allowing both perspectives on form data ($_POST) requests.

As @Tgr points out, the term "post" is confusing because it could be taken to mean either the HTTP POST method or $_POST.

Would it help, as part of the solution, to deprecate (and then quickly stop supporting) the "post" naming and change it to "form" instead? That'd be a one-to-one switch that we could easily make in affected code. If we then need to support it perpetually, its meaning is hopefully more obvious.

At this point I think we should just go ahead and kill "post". We just need to be a bit more careful than I was initially.

Change #1024459 merged by jenkins-bot:

[mediawiki/core@master] REST: introduce getBodyParamSettings

https://gerrit.wikimedia.org/r/1024459

Change #1028884 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] REST: introduce getSupportedRequestTypes()

https://gerrit.wikimedia.org/r/1028884

Change #1028885 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] DNM: REST: Deprecate using "post" as the parameter source

https://gerrit.wikimedia.org/r/1028885

Change #1028884 merged by jenkins-bot:

[mediawiki/core@master] REST: introduce getSupportedRequestTypes()

https://gerrit.wikimedia.org/r/1028884

daniel changed the task status from Open to In Progress.May 16 2024, 2:58 PM
daniel claimed this task.
daniel moved this task from Needs Further Discussion to In Progress on the MW-Interfaces-Team board.
daniel lowered the priority of this task from High to Medium.May 16 2024, 3:06 PM
daniel raised the priority of this task from Medium to High.Thu, May 30, 3:42 PM

Change #1037847 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/extensions/OAuth@master] Use getValidatedBody to get POST data.

https://gerrit.wikimedia.org/r/1037847

Change #1038390 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/extensions/GrowthExperiments@master] REST: Use getValidatedBody to get POST data.

https://gerrit.wikimedia.org/r/1038390

Change #1038390 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] REST: Use getValidatedBody to get POST data.

https://gerrit.wikimedia.org/r/1038390

Change #1043182 had a related patch set uploaded (by Fgoodwin; author: Fgoodwin):

[mediawiki/extensions/CognitiveProcessDesigner@master] Use body parameter instead of post parameter

https://gerrit.wikimedia.org/r/1043182

Change #1037847 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Use getValidatedBody to get POST data.

https://gerrit.wikimedia.org/r/1037847

Change #1043823 had a related patch set uploaded (by Fgoodwin; author: Fgoodwin):

[mediawiki/extensions/Checklists@master] Use body parameters instead of post parameters

https://gerrit.wikimedia.org/r/1043823

Change #1043182 had a related patch set uploaded (by Daniel Kinzler; author: Fgoodwin):

[mediawiki/extensions/CognitiveProcessDesigner@master] Use body parameter instead of post parameter

https://gerrit.wikimedia.org/r/1043182

Change #1043823 had a related patch set uploaded (by Daniel Kinzler; author: Fgoodwin):

[mediawiki/extensions/Checklists@master] Use body parameters instead of post parameters

https://gerrit.wikimedia.org/r/1043823

Change #1046727 had a related patch set uploaded (by Fgoodwin; author: Fgoodwin):

[mediawiki/extensions/TuleapWikiFarm@master] Use body parameters instead of post parameters

https://gerrit.wikimedia.org/r/1046727

Change #1046739 had a related patch set uploaded (by Fgoodwin; author: Fgoodwin):

[mediawiki/extensions/ContentStabilization@master] Use body parameters instead of post parameters

https://gerrit.wikimedia.org/r/1046739