Don't rely on Request::getPayload() to populate the parsed body

[nicolas-grekas]

That's just not needed and creates issues like #121

• fixes a bug with invalid jsons.

[derrabus]

Sounds reasonable. Let's keep it simple.

[derrabus]

Please mind fabbot, btw. 😉

[nicolas-grekas]

fabbot 🍏

[derrabus]

Thank you Nicolas.

[hivokas]

Could someone please explain what was the problem with using getPayload()?

PR says "creates issues like #121", but I can't open #121, it shows 404 for me.

[derrabus]

Could someone please explain what was the problem with using getPayload()?

It appears, Laravel overrides this method with a bad return type. This has been fixed on their side, but since the logic behind getPayload() is trivial for our case, we've decided to inline it.

PR says "creates issues like #121", but I can't open #121, it shows 404 for me.

The issue tracker of this repository has been disabled because we're using the one over at https://github.com/symfony/symfony now.

[hivokas]

Thanks for explaining @derrabus.

[rdgout]

Sadly this PR breaks solutions in Laravel where it was possible to add input data with a middleware. This PR makes it so that it always uses the raw input (which we cannot mutate) over the input that was already defined by the request.
For example, we have a middleware that adds the oauth client_id and client_secret to an oauth/token request to keep it a secret. This change makes this impossible unless we don't use Content-Type: application/json which is a change we cannot control for clients.

Seems pretty silly to me not to use the existing input that was already parsed.

[derrabus]

Sadly this PR breaks solutions in Laravel

Seems like no matter what we do, we "break Laravel". 🤷🏻‍♂️

where it was possible to add input data with a middleware.

The Request class has an attribute bag for data that is derived from the input. Mutating the input bags is usually not a wise thing to do because you cannot tell apart data submitted by the client from data that you've added programmatically.

For instance, let's say you have a piece of middleware that injects the current user ID as property user_id into the payload if a user is authenticated. Next thing I'll do is send you an unauthenticated request with a user_id property and your app believes I'm that user. 💣

That being said, feel free to decorate the PsrHttpFactory service and mutate the generated PSR-7 request as much as you please.

For example, we have a middleware that adds the oauth client_id and client_secret to an oauth/token request to keep it a secret.

And that's a perfect example for data that should never be injected into the input bags. Use attributes for that.

Seems pretty silly to me

I'd like to ask you to keep a respectful tone on this issue tracker.

[rdgout]

Thanks for the speedy reply.

For instance, let's say you have a piece of middleware that injects the current user ID as property user_id into the payload if a user is authenticated. Next thing I'll do is send you an unauthenticated request with a user_id property and your app believes I'm that user. 💣

This is not an issue since it only happens for 1 specific request which is actually an unauthenticated request.
There is also no upside (or possibility) to actually changing these properties since it only ruins your chance at getting a token. There is no way to escalate your permissions or something otherwise.
I understand what you mean though but we have thought about that.

And that's a perfect example for data that should never be injected into the input bags. Use attributes for that.

I'll take a look at that. Thanks for the suggestion.

I'd like to ask you to keep a respectful tone on this issue tracker.

I don't think saying that I think something is silly is disrespectful but let's not continue down that avenue.

[wouterj]

I understand what you mean though but we have thought about that.

Note that one of the big security benefits of using a framework/third party libraries is that they will prevent many vulnerable spots without you having to think about them. It's good to hear you've thought this through, but I'm happy to know that teams that didn't think this through are not vulnerable either because of the behavior of the library :)

Anyway, it's a bit unfortunate that code somewhere was broken due to a change in the library, but this is nearly unavoidable. Especially with libraries used in 2 different frameworks, where the "best practices" of one aren't necessarily advocated in great detail in the docs of the other. I'm glad we could point you to an alternative way of achieving the expected behavior.

[rdgout]

So to give a small update, upon searching further the issue was not with Laravel but with oauth2-server. Our solution, adding the client_id, client_secret, while admittedly wasn't the most elegant did make sure that in the previous version our input was in the getParsedBody function. With this change it no longer did.
Sadly oauth2-server basically only allows input from getParsedBody.

My fix in the end, for anyone who may encounter this issue, was to add the client_id and client_secret as Basic Auth to the request since the package reads this input as a default when it can't be found in the body.

This made sure the client_id and client_secret are still secure because the input is never sent back to the client, which keeps them safe. This solution obviously only works if you're not using these headers for a different purpose!

[hivokas]

@rdgout I've actually also located this pull request because we're doing the exact same thing in the project (using Laravel Passport, and injecting client_id and client_secret in the middleware).

Here is a fix I came up with.

Before:

$request->request->add([
    'client_id' => $client->id,
    'client_secret' => $client->secret,
]);

After:

$request->request->add([
    'client_id' => $client->id,
    'client_secret' => $client->secret,
]);

// Laravel Passport stopped recognizing injected `client_id` and `client_secret`
// because of this change: https://github.com/symfony/psr-http-message-bridge/releases/tag/v2.3.1
// This is a workaround for that issue.
$contentProperty = (new \ReflectionClass($request))->getProperty('content');
if (\is_string($contentProperty->getValue($request)) && json_decode($contentProperty->getValue($request)) !== null) {
    $contentProperty->setValue($request, json_encode($request->getPayload()->all()));
}

I won't pretend it's a beautiful solution, but it works for us. Feature tests are highly recommended for such stuff haha.

[trevorgehman]

My fix in the end, for anyone who may encounter this issue, was to add the client_id and client_secret as Basic Auth to the request since the package reads this input as a default when it can't be found in the body.

Could you paste the code you used in your middleware to accomplish this @rdgout ?

[hivokas]

My fix in the end, for anyone who may encounter this issue, was to add the client_id and client_secret as Basic Auth to the request since the package reads this input as a default when it can't be found in the body.

Have you ended up with something like this in the middleware?

$request->headers->add([
    'Authorization' => 'Basic '.base64_encode($client->id.':'.$client->secret),
]);

I think it's much more elegant than my initial solution. I haven't thought about passing client id and secret as basic auth credentials.

[rdgout]

My fix in the end, for anyone who may encounter this issue, was to add the client_id and client_secret as Basic Auth to the request since the package reads this input as a default when it can't be found in the body.

Have you ended up with something like this in the middleware?

$request->headers->add([
    'Authorization' => 'Basic '.base64_encode($client->id.':'.$client->secret),
]);

I think it's much more elegant than my initial solution. I haven't thought about passing client id and secret as basic auth credentials.

That's exactly what I did. My initial solution was to use the request attributes and use the attributes in my custom grant, however this did not work when the token was renewed with the refresh_token grant.
Using the header solution actually always works.

Also to note regarding your earlier comment, I caught this issue because we actually do have feature tests that failed with this update 😄