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

ExceptionHandler depends explicitly on Eloquent or object with public email property #21041

Closed
CImrie opened this issue Sep 6, 2017 · 0 comments

Comments

@CImrie
Copy link

CImrie commented Sep 6, 2017

  • Laravel Version: 5.5.2
  • PHP Version: 7.0.22
  • Database Driver & Version: MySQL 5.7.19

Description:

If a user is authenticated and an error is thrown the default exception handler tries to attach user-specific context to whoops page. This causes the whole app to crash for non-eloquent user models.

This happens because Illuminate\Foundation\Exceptions\Handler line 152 references the email property on a user. The catch block only catches Exception classes. Because of changes to PHP7 error handling, this is no longer sufficient when a Throwable is thrown. http://php.net/manual/en/language.errors.php7.php

As the Error hierarchy does not inherit from Exception, code that uses catch (Exception $e) { ... } blocks to handle uncaught exceptions in PHP 5 will find that these Errors are not caught by these blocks. Either a catch (Error $e) { ... } block or a set_exception_handler() handler is required.

Steps To Reproduce:

You need to meet the following conditions to see the issue:

  • An exception must be thrown in the request cycle
  • You must be logged in
  • You are using an authenticatable object that does not have a public $email property

I use Doctrine for authentication but I can reproduce this issue without any database.

Example Code to Reproduce

In App/User.php

namespace App;

use Illuminate\Contracts\Auth\Access\Authorizable;
use Illuminate\Contracts\Auth\Authenticatable;
use Illuminate\Contracts\Auth\CanResetPassword;
use Illuminate\Notifications\Notifiable;

class User implements Authenticatable, Authorizable, CanResetPassword {
    use Notifiable, \Illuminate\Auth\Authenticatable, \Illuminate\Foundation\Auth\Access\Authorizable, \Illuminate\Auth\Passwords\CanResetPassword;

    protected $email;

    /**
     * The attributes that are mass assignable.
     *
     * @var array
     */
    protected $fillable = [
        'name', 'email', 'password',
    ];

    /**
     * The attributes that should be hidden for arrays.
     *
     * @var array
     */
    protected $hidden = [
        'password', 'remember_token',
    ];

    public function getKeyName()
    {
	return 'email';
    }
}

In my web.php routes file:

Route::get('/', function () {
	Auth::login(new User());
	throw new Exception('test');
});

Then simply navigate to the '/' path of your project.

Fix

If the catch block on Line 154 of Illuminate\Foundation\Exceptions\Handler is widened to \Throwable instead of \Exception classes, this is resolved.

I will try to write a test to catch this issue and submit a PR shortly

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

No branches or pull requests

2 participants