[9.x] Manually populate POST request body with JSON data only when required

[LukeTowers]

This fixes a 6 year old bug introduced in #7052 where GET requests would have GET data populated in the POST body property leading to issues around Request::post() and $request->getParsedBody() returning GET values when called on GET requests.

This is a resubmit of #17087 & #36708, and fixes #22805. Credit to @dan-har for the initial solution and @mixlion for updating it for >=6.x.

The original PR was meant to support POST requests where their content-type was set to application/json (instead of the typical application/x-www-form-urlencoded), but it introduced a subtle and dangerous bug because while $request->getInputSource() does return the JSON data for JSON requests, it also returns the GET data for GET requests. This commit solves the underlying issue without breaking compatibility with the original functionality.

This PR is submitted as a direct result from a security issue reported to me by users of @wintercms & @octobercms and was submitted after a discussion with @taylorotwell via email.

I request that this fix also be backported to 6.x @driesvints

[driesvints]

Okay, I just went through the code and this indeed seems okay to me. There's one small concern though:

Both json and getInputSource are returning an instance of Symfony\Component\HttpFoundation\ParameterBag while $newRequest->request requires an Symfony\Component\HttpFoundation\InputBag instead. This is already happening at the moment so it might not be too big of a problem. But still wondering if we should assign the proper type here.

@LukeTowers I'm afraid we cannot make this change on 6.x as I can definitely see apps break over this while expecting the current behavior. But it seems reasonable to do this on the next major release.

Would be cool if you could add a test as well.

[LukeTowers]

@driesvints InputBag was only added 15 months ago in 5.1 (symfony/http-foundation@5fa0a07) and extends ParameterBag. If there's a need for $request->json() to be an InputBag instead then I would say that's a recent change and one that should happen regardless.

I'm not sure I can see how existing apps could break over this change in a negative way, could you provide me an example of what came to your mind? The whole reason I want this backported is because it "breaks" apps in that it fixes potential security issues (I don't think a single developer out there expects Request::post() to return GET values during a GET request).

@driesvints Tests have been added

[driesvints]

@LukeTowers okay, I guess that the type thing is fine then.

So, the reason I think this is a breaking change is because before this PR you could do the following for a GET request:

$request->request->all();

And that would give back the params from the query string. I definitely agree with you that this needs fixing but there might be people relying on the current behavior.

[LukeTowers]

@driesvints

While a bit less obvious that it's a bad idea to use $request->request->all() for GET parameters than Request::post() I'm not seeing anyone that does use it in that way from a quick code search over GitHub.

I am however seeing lots of usage of how it's intended to be used, for POST parameters; along with a few that could be impacted by not realizing that it's including GET params.

[driesvints]

@LukeTowers let's see what Taylor says.

[taylorotwell]

Why not just change the post method to always return null if the HTTP method isn't POST?

[taylorotwell]

I still am not sure this fixes your "vulnerability" - can a malicious user not simply send a Content-Type header of application/json to trick the framework into populating request?

[LukeTowers]

@taylorotwell

Why not just change the post method to always return null if the HTTP method isn't POST?

Already doing that in our projects as a stop gap.

I still am not sure this fixes your "vulnerability" - can a malicious user not simply send a Content-Type header of application/json to trick the framework into populating request?

The main concern is CSRF attacks triggered unintentionally. AFAIK you can't make a users browser send a different content type header for GET requests for things like regular links and img sources.

[taylorotwell]

@LukeTowers Can we make the change to have post always return null if the HTTP method isn't POST instead of the change currently in this PR? Does that totally solve your problem?

[LukeTowers]

@taylorotwell while it could possibly solve my specific problem, I'm not sure that's all that helpful overall given the amount of people out there using $request->request->all() to access POST values instead of $request->post() (see https://github.com/search?q=%22request-%3Erequest-%3Eall%28%29%22+language%3APHP+language%3APHP&type=Code) since it would still leave those users at risk and fundamentally would still be an incorrect implementation at a deep level of the framework.

[LukeTowers]

@driesvints will we be able to get this backported to 6.x as well?

[driesvints]

Hey @LukeTowers. No we won't be backporting this one. We're not super confident that it won't break anything so this one will only target the next version.