Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

\Illuminate\Database\Eloquent\Concerns\PreventsCircularRecursion::withoutRecursion() method crashes on mocked models #52727

Closed
sanfair opened this issue Sep 10, 2024 · 4 comments

Comments

@sanfair
Copy link

sanfair commented Sep 10, 2024

Laravel Version

11.22.0

PHP Version

8.3.8

Database Driver & Version

No response

Description

Method \Illuminate\Database\Eloquent\Concerns\PreventsCircularRecursion::withoutRecursion() will crash if it is invoked from eval.

Method definition

protected function withoutRecursion($callback, $default = null)
{
$trace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT, 2);
$onceable = Onceable::tryFromTrace($trace, $callback);
$stack = static::getRecursiveCallStack($this);
if (array_key_exists($onceable->hash, $stack)) {
return is_callable($stack[$onceable->hash])
? static::setRecursiveCallValue($this, $onceable->hash, call_user_func($stack[$onceable->hash]))
: $stack[$onceable->hash];
}
try {
static::setRecursiveCallValue($this, $onceable->hash, $default);
return call_user_func($onceable->callable);
} finally {
static::clearRecursiveCallValue($this, $onceable->hash);
}
}

That happens because \Illuminate\Support\Onceable::tryFromTrace() will return null when the second frame in the trace stack is eval.

A more appropriate example is when the \Illuminate\Database\Eloquent\Model::toArray() method is called on a model mocked by mockery/mockery.


Real-world example.

Stack:

  • Laravel v11.22.0
  • Inertia
  • Pest/PHPUnit

Our feature test:

  1. Arrange a mocked user and use it for authentication.
  2. Make an HTTP request.
  3. Assert
    • that a method on the model is called (in our case, one of Laravel cashier's methods).
    • that the server responds with an inertia component.

The exception happens when Ineria middleware tries to pass the mocked user model to the front end.


Steps To Reproduce

I prepared two Pest tests that demonstrate this bug:

test('that `Model::withoutRecursion()` method can be called using eval', function () {
    $model = new class extends \Illuminate\Database\Eloquent\Model
    {
        public function toArray(): array
        {
            // Added to prevent IDE to complain about "Undefined variable '$value'" on line 25.
            $result = [];

            eval(<<<'EVAL'

            $result = $this->withoutRecursion(
                fn () => array_merge($this->attributesToArray(), $this->relationsToArray()),
                fn () => $this->attributesToArray(),
            );

            EVAL
            );

            return $result;
        }
    };

    expect($model->toArray())->toEqual([]);
});

test('that `Model::toArray()` method work on mocked models', function () {
    $model = Mockery::mock(\App\Models\User::class)->makePartial();

    expect($model->toArray())->toEqual([]);
});

Repo: https://github.com/sanfair/laravel-prevent-circular-recursion-bug
Commit with tests: sanfair/laravel-prevent-circular-recursion-bug@89b173c

@sanfair
Copy link
Author

sanfair commented Sep 10, 2024

Error messages from tests for context.

image

$ php artisan test

   FAIL  Tests\Unit\ExampleTest
  ⨯ that Model::withoutRecursion() method can be called using eval                                                                                                               0.01s
  ⨯ that Model::toArray() method work on mocked models                                                                                                                           0.02s
  ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   FAILED  Tests\Unit\ExampleTest > that `Model::withoutRecursion()` method can be called using eval                                                                        TypeError
  Illuminate\Database\Eloquent\Model::clearRecursiveCallValue(): Argument #2 ($hash) must be of type string, null given, called in /home/eugene/laravel-prevent-circular-recursion-bug/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php on line 44

  at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php:94
     90▕      * @param  string  $hash
     91▕      * @param  mixed  $value
     92▕      * @return mixed
     93▕      */
  ➜  94▕     protected static function setRecursiveCallValue($object, string $hash, $value)
     95▕     {
     96▕         static::getRecursionCache()->offsetSet(
     97▕             $object,
     98▕             tap(static::getRecursiveCallStack($object), fn (&$stack) => $stack[$hash] = $value),

      +2 vendor frames
  3   tests/Unit/ExampleTest.php:12
  4   tests/Unit/ExampleTest.php:25

  ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   FAILED  Tests\Unit\ExampleTest > that `Model::toArray()` method work on mocked models                                                                                    TypeError
  Mockery_0_App_Models_User::clearRecursiveCallValue(): Argument #2 ($hash) must be of type string, null given, called in /home/eugene/laravel-prevent-circular-recursion-bug/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php on line 44

  at vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php:40
     36▕                 : $stack[$onceable->hash];
     37▕         }
     38▕
     39▕         try {
  ➜  40▕             static::setRecursiveCallValue($this, $onceable->hash, $default);
     41▕
     42▕             return call_user_func($onceable->callable);
     43▕         } finally {
     44▕             static::clearRecursiveCallValue($this, $onceable->hash);

      +2 vendor frames
  3   tests/Unit/ExampleTest.php:31


  Tests:    2 failed (0 assertions)
  Duration: 0.08s

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@maximetassy
Copy link
Contributor

maximetassy commented Sep 25, 2024

Hi, I'm facing the same issue with laravel 11.24.1 on mocked model.
In order to fix this issue, maybe it could just use the same null verification that is used in once(callable $callback) helper method ?
https://github.com/laravel/framework/blob/11.x/src/Illuminate/Support/helpers.php#L237
Something like :

protected function withoutRecursion($callback, $default = null)
{
    $trace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT, 2);

    $onceable = Onceable::tryFromTrace($trace, $callback);

    if (is_null($onceable)) {
        return call_user_func($callback);
    }
    
    $stack = static::getRecursiveCallStack($this);
    // ...

@crynobone
Copy link
Member

This seem to be fixed via #52943

Please submit new bug report if you still have any issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants