Framework: [5.1] Errors in views cause problems when running in queue daemon worker

[ockle]

I've run into the same problem as described in #9881, so please read that as this follows on, but with more findings, reproduction steps and discussion of a fixe.

As identified in a comment, if an error occurs when rendering a view while in a queue daemon worker, subsequent renderings use a previous rendering result. Seeing as the queue workers are most commonly used for mail and it's easiest to reproduce like that, we will.

I have tested using the beanstalkd driver, but I presume any other than sync will work.

To reproduce:
1, Create an email view using blade @section and @yield, with a method for making it error on demand e.g. trying to echo an undefined variable based on data passed.

1. Send mail to queue using some data that will identify the content as being the 1st email, _not_ triggering error in view.
2. Send mail to queue using some data that will identify the content as being the 2nd email, _and_ triggering error in view.
3. Send mail to queue using some data that will identify the content as being the 3rd email, _not_ triggering error in view.
4. Send mail to queue using some data that will identify the content as being the 4th email, _not_ triggering error in view.
5. Run php artisan queue:work --daemon --tries=1

Expected result:
1st, 3rd and 4th emails received, content correct and different in all three.

Actual result:
1st email correct, 3rd email correct, 4th email has same content as 3rd email.

I've gone beyond what was figured in #9881 and chased this error through the framework and ascertained that the issue is due to the error in the rendering of the view in the 2nd email causing a problem with the section array/section stack in Illuminate\View\Factory. It seems that an error in a view in a previous job effectively get "left over" and causes problems with view rendering in future jobs.

A user-implementable workaround, which also proves the cause, is to add:

Queue::looping(function () {
    View::flushSections();
});

to e.g. the AppServiceProvider boot method. With this in place, the sections are forcibly flushed after every job, and the steps above can be re-run with the expected results achieved.

Exactly how best to approach fixing this in the framework, I am not sure, hence no pull request, but hopefully there is enough information here to be able to replicate this and work to getting it fixed, as this can potentially lead to inadvertently to sending emails with the sensitive information to the wrong recipients.

[robclancy]

Got the same issue. This is a MAJOR security leak in Laravel!

[GrahamCampbell]

Got the same issue. This is a MAJOR security leak in Laravel!

I disagree.

[GrahamCampbell]

Oh, wait, I see. This is actually a known issue.

[GrahamCampbell]

On reading again, it's not. I was thinking this was the security bug we still have with @parent.

e.g. trying to echo an undefined variable based on data passed.

This is an error in your code, not ours.

[robclancy]

Oh wow. Bugs reported by many people and you close it? Get a fucking grip.

On Tue, 2 Feb 2016 02:45 Graham Campbell [email protected] wrote:

On reading again, it's not. I was thinking this was the security bug we
still have with @parent.

e.g. trying to echo an undefined variable based on data passed.

This is an error in your code, not ours.

—
Reply to this email directly or view it on GitHub
https://github.com/laravel/framework/issues/11892#issuecomment-178062371
.

[vladrusu]

@GrahamCampbell, the problem is not if he has or has not an error in his code.
Yes, he has. Errors happens.
I think you're missing the point here. The problem is not that we have an undefined variable in our code. The problem is how Laravel REACTS to code errors in views (you know, errors get their way in our code from time to time, we're humans).

If the steps mentioned by @ockle are reproducible, to me it seems like a clear Laravel bug.

An error in a view should trigger a fatal error no matter what. The email should not be sent, period.
What does Laravel when sending mails in a queue and gets an error in the view? It still sends the mail, but with the last successful rendered email view. That's insane.

The security implications are HIGH, because you might very well send sensitive data to other users (think of "Welcome user X. Here are your username/password.", sent to user Y.
That's very SERIOUS.

And the worst part? I ran into this bug two times already, when the system sent by mistake same email bodies to different users. Have the "possibility" to send the same user registration credentials to different users is not nice. :-)

[GrahamCampbell]

Proposed fix?

[jaketoolson]

@robclancy, the only way you can get @GrahamCampbell to do anything is if there are extra whitespaces. Otherwise, he closes the PR early and asks questions later. @taylorotwell one of these days we'd like to not have to deal with him...

[taylorotwell]

Will try to re-create in a bit.

[taylorotwell]

Pushing up fixes.

[GrahamCampbell]

@jaketoolson I don't appreciate your comments, and it doesn't help when they're not even true.

[jaketoolson]

@GrahamCampbell then do what you always do, close the thread without offering any help.

[jaketoolson]

@taylorotwell thank you. I am testing both locally and on our dev servers. Finger crossed as this would aleviate one of the strangest quirks we encountered and ignored :)

[taylorotwell]

OK just let me know as soon as you know :)

On Feb 1, 2016, 2:33 PM -0600, Jake [email protected], wrote:

@taylorotwell(https://github.com/taylorotwell)I am testing both locally and on our dev servers. Finger crossed as this would aleviate one of the strangest quirks we encountered and ignored :)

—
Reply to this email directly orview it on GitHub(https://github.com/laravel/framework/issues/11892#issuecomment-178174107).

[ockle]

@taylorotwell Thanks for looking at this, however, your fix does not seem to solve the issue :(

Here's a some stripped out reproduction steps for a fresh install of L5.2:

routes.php:

/*Queue::looping(function () {
    View::flushSections();
});*/

Route::group(['middleware' => ['web']], function () {
    Route::get('/test', function () {
        Mail::queue('email.test', ['error' => false, 'foo' => 'One'], function ($message) {
            $message->to('[email protected]')
                ->subject('One');
        });
        Mail::queue('email.test', ['error' => true, 'foo' => 'Two'], function ($message) {
            $message->to('[email protected]')
                ->subject('Two');
        });
        Mail::queue('email.test', ['error' => false, 'foo' => 'Three'], function ($message) {
            $message->to('[email protected]')
                ->subject('Three');
        });
        Mail::queue('email.test', ['error' => false, 'foo' => 'Four'], function ($message) {
            $message->to('[email protected]')
                ->subject('Four');
        });
        Mail::queue('email.test', ['error' => false, 'foo' => 'Five'], function ($message) {
            $message->to('[email protected]')
                ->subject('Five');
        });
    });
});

view/email/layout.blade.php:

Layout

@yield('content')

views/email/test.blade.php:

@extends('email.layout')

@section('content')
    {{ $foo }}

    @if ($error)
        {{ $bar }}
    @endif
@endsection

Set mail driver to log. Now visit /test. The run php artisan queue:work --daemon --tries=1. Once that has processed all the jobs, stop it and check the log. The last three emails will all have content "Three". Now uncomment that bit at the top of the posted routes.php and rerun the test. Now last three emails have the correct content.

This is tested with "laravel/framework": "5.2.*@dev" which has pulled in 8116de4640fc67b60cb9c418b5f25b4948d69448 and verified View.php is patched.

[taylorotwell]

Hmm. Anyone see anything I'm missing on what may be causing this?

On Feb 1, 2016, 2:36 PM -0600, [email protected], wrote:

@taylorotwell(https://github.com/taylorotwell)Thanks for looking at this, however, your fix does not seem to solve the issue :(

Here's a some stripped out reproduction steps for a fresh install of L5.2:

routes.php:

/_Queue::looping(function () {View::flushSections();});_/Route::group(['middleware'=>['web']],function() {Route::get('/test',function() {Mail::queue('email.test', ['error'=>false,'foo'=>'One'],function($message) {$message->to('[email protected]')->subject('One');});Mail::queue('email.test', ['error'=>true,'foo'=>'Two'],function($message) {$message->to('[email protected]')->subject('Two');});Mail::queue('email.test', ['error'=>false,'foo'=>'Three'],function($message) {$message->to('[email protected]')->subject('Three');});Mail::queue('email.test', ['error'=>false,'foo'=>'Four'],function($message) {$message->to('[email protected]')->subject('Four');});Mail::queue('email.test', ['error'=>false,'foo'=>'Five'],function($message) {$message->to('[email protected]')->subject('Five');});});});

view/email/layout.blade.php:

Layout@yield('content')

views/email/test.blade.php:

@extends('email.layout')@section('content'){{$foo}}@if($error){{$bar}}@endif@endsection

Set mail driver to log. Now visit /test. The runphp artisan queue:work --daemon --tries=1. Once that has processed all the jobs, stop it and check the log. The last three emails will all have content "Three". Now uncomment that bit at the top of the posted routes.php and rerun the test. Now last three emails have the correct content.

This is tested with"laravel/framework": "5.2.*@dev"which has pulled in8116de4640fc67b60cb9c418b5f25b4948d69448and verified View.php is patched.

—
Reply to this email directly orview it on GitHub(https://github.com/laravel/framework/issues/11892#issuecomment-178175923).

[robclancy]

I have been thinking a lot about this and the fact that the sections are left over from the last successful email left and not the email that has the exception in it means the issue is occurring before this point. However this makes no sense because firstly it only happens when the exception happens and secondly now when the error does happen the sections are flushed so even if it isn't fixing the root cause it should still be fixing it.

We need this fixed now so will be taking the test code above and working until I find the issue.

[taylorotwell]

What version of PHP are you on?

On Feb 2, 2016, 1:25 AM -0600, Robert Clancy (Robbo)[email protected], wrote:

I have been thinking a lot about this and the fact that the sections are left over from the last successful email left and not the email that has the exception in it means the issue is occurring before this point. However this makes no sense because firstly it only happens when the exception happens and secondly now when the error does happen the sections are flushed so even if it isn't fixing the root cause it should still be fixing it.

We need this fixed now so will be taking the test code above and working until I find the issue.

—
Reply to this email directly orview it on GitHub(https://github.com/laravel/framework/issues/11892#issuecomment-178422217).

[taylorotwell]

This has been fixed and tagged on Laravel 4.2 through 5.2.

[ockle]

Tested with 5.1.29 and can confirm it's fixed, thanks!

[vladrusu]

Great! Thanks, Taylor!

[zogot]

This had been a frustrating issue. Glad for the fix.

[pedro-lucas]

I get the same error when the Swift_Mail class emit error on connection with SMTP Server (timeout). I think this error is about send email using queue and not isolated to blade templates.

Laravel framework version: 5.1.29

[lifeofguenter]

Seeing that this is a major security issue and not something necessarily you'd expect, it would be great to know if there were some kind of channel where issues like these get communicated?

[pavinthan]

but this is not fixed in Laravel Framework version 5.0.35

[driesvints]

I'm locking this issue because it either has gone off-topic, become a dumping ground for things which shouldn't be in an issue tracker or is just too old.