-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
[5.1] Errors in views cause problems when running in queue daemon worker #11892
Comments
Got the same issue. This is a MAJOR security leak in Laravel! |
I disagree. |
Oh, wait, I see. This is actually a known issue. |
On reading again, it's not. I was thinking this was the security bug we still have with
This is an error in your code, not ours. |
Oh wow. Bugs reported by many people and you close it? Get a fucking grip. On Tue, 2 Feb 2016 02:45 Graham Campbell notifications@github.com wrote:
|
@GrahamCampbell, the problem is not if he has or has not an error in his code. 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. 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. 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. :-) |
Proposed fix? |
@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... |
Will try to re-create in a bit. |
Pushing up fixes. |
@jaketoolson I don't appreciate your comments, and it doesn't help when they're not even true. |
@GrahamCampbell then do what you always do, close the thread without offering any help. |
@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 :) |
OK just let me know as soon as you know :) On Feb 1, 2016, 2:33 PM -0600, Jake Toolsonnotifications@github.com, wrote:
|
@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('test@example.com')
->subject('One');
});
Mail::queue('email.test', ['error' => true, 'foo' => 'Two'], function ($message) {
$message->to('test@example.com')
->subject('Two');
});
Mail::queue('email.test', ['error' => false, 'foo' => 'Three'], function ($message) {
$message->to('test@example.com')
->subject('Three');
});
Mail::queue('email.test', ['error' => false, 'foo' => 'Four'], function ($message) {
$message->to('test@example.com')
->subject('Four');
});
Mail::queue('email.test', ['error' => false, 'foo' => 'Five'], function ($message) {
$message->to('test@example.com')
->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 This is tested with |
Hmm. Anyone see anything I'm missing on what may be causing this? On Feb 1, 2016, 2:36 PM -0600, ocklenotifications@github.com, 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. |
What version of PHP are you on? On Feb 2, 2016, 1:25 AM -0600, Robert Clancy (Robbo)notifications@github.com, wrote:
|
This has been fixed and tagged on Laravel 4.2 through 5.2. |
Tested with 5.1.29 and can confirm it's fixed, thanks! |
Great! Thanks, Taylor! |
This had been a frustrating issue. Glad for the fix. |
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 |
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? |
but this is not fixed in Laravel Framework version 5.0.35 |
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. |
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.2. Send mail to queue using some data that will identify the content as being the 1st email, not triggering error in view.
3. Send mail to queue using some data that will identify the content as being the 2nd email, and triggering error in view.
4. Send mail to queue using some data that will identify the content as being the 3rd email, not triggering error in view.
5. Send mail to queue using some data that will identify the content as being the 4th email, not triggering error in view.
6. 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:
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.
The text was updated successfully, but these errors were encountered: