Illuminate\Http\JsonResponse::setData() falsely assumes that toJson() calls json_encode

[nagmat84]

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

Description

Details

The method Illuminate\Http\JsonResponse::setData() calls $data->toJson() on the provided $data, if $data is an instance of Jsonable. Otherwise, Illuminate\Http\JsonResponse::setData() uses json_encode directly. Later, Illuminate\Http\JsonResponse::setData() uses json_last_error() in order to check whether there has been any error.

/**
 * {@inheritdoc}
 *
 * @return static
 */
public function setData($data = [])
{
  $this->original = $data;

  if ($data instanceof Jsonable) {
    $this->data = $data->toJson($this->encodingOptions);
  } elseif ($data instanceof JsonSerializable) {
    $this->data = json_encode($data->jsonSerialize(), $this->encodingOptions);
  } elseif ($data instanceof Arrayable) {
    $this->data = json_encode($data->toArray(), $this->encodingOptions);
  } else {
    $this->data = json_encode($data, $this->encodingOptions);
  }

  if (! $this->hasValidJson(json_last_error())) {
    throw new InvalidArgumentException(json_last_error_msg());
  }

  return $this->update();
}

This is problematic, because this approach silently (and falsely) assumes that toJson uses json_last_error() internally. Please consider, that $data might use a completely different implementation. For example, a trivial object could simply return a statically coded string.

In this case, json_last_error() may not be cleared, but return a previous, stale error message from some completely unrelated former JSON operation (e.g. see #42106).

Proposed Solution

As setData is going to throw an exception anyway, the easiest solution would be to use the option JSON_THROW_ON_ERROR and let json_encode throw the exception. Consequently, setData throws an exception which is not caused by the current operation.

/**
 * {@inheritdoc}
 *
 * @return static
 */
public function setData($data = [])
{
  $this->original = $data;

  if ($data instanceof Jsonable) {
    $this->data = $data->toJson($this->encodingOptions | JSON_THROW_ON_ERROR);
  } elseif ($data instanceof JsonSerializable) {
    $this->data = json_encode($data->jsonSerialize(), $this->encodingOptions | JSON_THROW_ON_ERROR);
  } elseif ($data instanceof Arrayable) {
    $this->data = json_encode($data->toArray(), $this->encodingOptions | JSON_THROW_ON_ERROR);
  } else {
    $this->data = json_encode($data, $this->encodingOptions | JSON_THROW_ON_ERROR);
  }

  return $this->update();
}

This only makes the assumption that toJson will throw an exception, if anything goes wrong, which is a much slighter assumption than the previous one. This has also the nice benefit, that setData throws a much more specific JsonException than a generic InvalidArgumentException.

[driesvints]

Hi there, thanks for the clear issues. It's indeed atm assumed that toJson always returns valid JSON. That's done intentionally. Also see the default encoding options we use from Symfony:

I think if you want to change this behavior that you best attempt a PR. Thanks

[nagmat84]

Hi there, thanks for the clear issues. It's indeed atm assumed that toJson always returns valid JSON. That's done intentionally.

That is not the point of the issue. Please read carefully again.

Of course, toJson has to return a valid JSON string. For example, if toJson returns the static string '{}' this would be perfectly valid.

The problem is that setData calls json_last_error even though it is not guaranteed that json_encode has been invoked previously.

[driesvints]

@nagmat84 I see, thanks for clarifying that. I also see there's no way to clear json_last_error.

To proceed with this can you provide me with a value that's invalid for json_encode()?

[nagmat84]

I am not quite sure, why you need such a value, because the problem is caused by a toJson not using json_encode, but anyway here it is:

json_encode(['a' => acos(2)]);
$result = json_last_error();  // $result equals 7

[driesvints]

Going to re-open to get @taylorotwell's thoughts on this. Thanks

[nagmat84]

I believe the description is precise to the point. What exactly is unclear about it? Anway:

1. Create a Laravel App

2. Create a class Foo which implements Jsonable like this

   class Foo implements Jsonable {
     public toJson($options = 0): string {
       return '{}';
     }
   }

3. Create controller and a controller method like this

   class FooController extends Controller {
     public function getFoo() {
       // The next line is only there to mimic a previously failed JSON method
       // as it might have happened anywhere in the middleware
       json_encode('');
       return new Foo();
     }
   }

[driesvints]

@nagmat84 nothing is unclear here. Just want to get Taylor's thoughts. Using JSON_THROW_ON_ERROR will involve a breaking change because a different exception will be thrown.

[driesvints]

@nagmat84 what if we call json_decode('[]'); at the beginning of the method to clear the errors? That way the method could stay in place mostly. We can then revise and use JSON_THROW_ON_ERROR maybe in Laravel v10.

[nagmat84]

Objectively speaking this would work.

Personally, I don't like it. For me it feels like a (nasty) work-around. However, this is also due to the fact that I don't like the way how error are handled in Laravel at all. IMHO, using custom exceptions is the right way, see #40021 and #40020. But that is off-topic right now and nothing which can be solved without breaking backward compatibility.

[driesvints]

Thanks, I sent in a PR for this: #42125