FormRequestServiceProvider tries to copy non-JSON request as JSON

[nagmat84]

• Laravel Version: 8.83.7
• PHP Version: 8.0.16-r1

Description:

Summary

During bootstrapping, the FormRequestServiceProvider makes a deep copy of the Request object. While doing so, any Request is treated as if it contained JSON payload which leads to the side-effect that json_last_error is set for non-JSON requests. As other parts of Laravel check for json_last_error being clear later on, this has undesired effects.

Details

At some point during bootstrapping, the backtrace looks basically like this

Request.php:384, Illuminate\Http\Request->json()
Request.php:435, Illuminate\Http\Request::createFrom()
...
/Providers/FormRequestServiceProvider.php:33-37}()
...
Kernel.php:111, Illuminate\Foundation\Http\Kernel->handle()
index.php:74, {main}()

and the Request object makes a deep copy of itself in Illuminate\Http\Request::createFrom().

/**
 * Create a new request instance from the given Laravel request.
 *
 * @param  \Illuminate\Http\Request  $from
 * @param  \Illuminate\Http\Request|null  $to
 * @return static
 */
public static function createFrom(self $from, $to = null)
{
  $request = $to ?: new static;

  $files = $from->files->all();

  $files = is_array($files) ? array_filter($files) : $files;

  $request->initialize(
    $from->query->all(),
    $from->request->all(),
    $from->attributes->all(),
    $from->cookies->all(),
    $files,
    $from->server->all(),
    $from->getContent()
  );

  $request->headers->replace($from->headers->all());

  $request->setJson($from->json());

  if ($session = $from->getSession()) {
    $request->setLaravelSession($session);
  }

  $request->setUserResolver($from->getUserResolver());

  $request->setRouteResolver($from->getRouteResolver());

  return $request;
}

The problematic line is $request->setJson($from->json()) and the invocation of $from->json(). This getter method is not side-effect free.

/**
 * Get the JSON payload for the request.
 *
 * @param  string|null  $key
 * @param  mixed  $default
 * @return \Symfony\Component\HttpFoundation\ParameterBag|mixed
 */
public function json($key = null, $default = null)
{
  if (! isset($this->json)) {
    $this->json = new ParameterBag((array) json_decode($this->getContent(), true));
  }

  if (is_null($key)) {
    return $this->json;
  }

  return data_get($this->json->all(), $key, $default);
}

For non-JSON requests, this method calls json_decode on content which is not JSON. As a smaller cosmetic defect, it also modifies the original request $from by setting $this->json to an new, empty ParameterBag.

Proposed solution

Remove $request->setJson($from->json()) from Illuminate\Http\Request::createFrom().

Calling this method is not necessary at all for making a deep copy. Everything which is needed is already copied by $request->initialize(...) some line earlier, especially the content of the request. If a JSON interpretation of the content is required later in the application, then json() can (and will) still be called on the deep copy which leads to the exact same result. Bonus points:

• no unconditional call to json_decode for non-JSON requests
• no side-effect on json_get_last_error
• slightly higher efficiency by skipping an unnecessary step
• no modification of the original Request object

[driesvints]

Can you give a clear way to reproduce this in a fresh Laravel app?

[nagmat84]

1. Create a new Laravel App
2. Create a route and a controller method for the root
3. Make a AJAX request to the route which is non-JSON, for example POST some data encoded as multipart/form-data
4. During the middleware Laravel makes a deep-copy of the Request object and also tries to copy the content as JSON. This silently fails, but leaves a stale error code in json_last_error

[driesvints]

I'll need a clear explanation of what actually breaks. There's still no bug here. Having json_last_error set isn't a bug imo.

[driesvints]

I don't think there's a big issue here tbh. You're free to make a proposal through a PR if you want. Thanks

[nagmat84]

It is a bug in combination with #42107, because this is one of the code lines which set json_last_error and then later break setData (see #42107). Of course one could argue that this is an error in setData.

However, and this are the real bugs, the copy-method

• is not side-effect free,
• modifies the original object, and
• tries to unconditionally interpret some content as JSON which is not JSON

Luckily, json_decode stops early after it has encountered the first parsing error. Simply assume you upload a huge file (e.g. a media file) and then you let json_decode process it. Surely, json_decode will simply stop after the first few bytes, but it is still something you should not do.