-
Notifications
You must be signed in to change notification settings - Fork 11k
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.5] Ensure uncaught exceptions are returned in JSON if required #18732
Changes from all commits
cd69e50
655d3ce
44166d6
4a4a6f6
bc16cc7
7b33de4
f04172f
84506c1
bf5d8c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
use Psr\Log\LoggerInterface; | ||
use Illuminate\Http\Response; | ||
use Illuminate\Routing\Router; | ||
use Illuminate\Http\JsonResponse; | ||
use Illuminate\Support\Facades\Auth; | ||
use Illuminate\Http\RedirectResponse; | ||
use Illuminate\Auth\AuthenticationException; | ||
|
@@ -139,7 +140,7 @@ public function render($request, Exception $e) | |
return $this->convertValidationExceptionToResponse($e, $request); | ||
} | ||
|
||
return $request->expectsJson() && config('app.debug') | ||
return $request->expectsJson() | ||
? $this->prepareJsonResponse($request, $e) | ||
: $this->prepareResponse($request, $e); | ||
} | ||
|
@@ -212,22 +213,28 @@ protected function prepareResponse($request, Exception $e) | |
* | ||
* @param \Illuminate\Http\Request $request | ||
* @param \Exception $e | ||
* @return \Symfony\Component\HttpFoundation\Response | ||
* @return \Illuminate\Http\JsonResponse | ||
*/ | ||
protected function prepareJsonResponse($request, Exception $e) | ||
{ | ||
$status = $this->isHttpException($e) ? $e->getStatusCode() : 500; | ||
|
||
$headers = $this->isHttpException($e) ? $e->getHeaders() : []; | ||
|
||
return response(json_encode([ | ||
'message' => $e->getMessage(), | ||
'file' => $e->getFile(), | ||
'line' => $e->getLine(), | ||
'trace' => $e->getTrace(), | ||
], JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES), $status, array_merge($headers, [ | ||
'Content-Type' => 'application/json', | ||
])); | ||
if (config('app.debug')) { | ||
$response = [ | ||
'message' => $e->getMessage(), | ||
'file' => $e->getFile(), | ||
'line' => $e->getLine(), | ||
'trace' => $e->getTrace(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't think we should response a different json in debug mode and the production mode. Further more, the exception code (getCode()) may have much more valuable. May be we should just hide the trace for some security consideration. Also we should consider the translation for the message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Taylor already added JSON stuff for debug in the master branch. This PR is not adding it. We are just putting JSON for production when it was requested via AJAX. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should just hide the trace in production. Or replace it with the exception's getCode. I think the translation for the message is really important. Could you add these code ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Translation of exceptions should probably be a different PR though - as that is a separate to what is occurring in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, but it is relate these code, about the exception. It doesn't matter how the code work. Please consider the exception code, the translation. The file name and the exception line shown in production may not be safe for the server. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct - we only return the detailed information if |
||
]; | ||
} else { | ||
$response = [ | ||
'message' => $this->isHttpException($e) ? $e->getMessage() : 'Server Error', | ||
]; | ||
} | ||
|
||
return new JsonResponse($response, $status, $headers, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
<?php | ||
|
||
namespace Illuminate\Tests\Foundation; | ||
|
||
use Exception; | ||
use Mockery as m; | ||
use PHPUnit\Framework\TestCase; | ||
use Illuminate\Container\Container; | ||
use Illuminate\Config\Repository as Config; | ||
use Illuminate\Foundation\Exceptions\Handler; | ||
use Symfony\Component\HttpKernel\Exception\HttpException; | ||
|
||
class FoundationExceptionsHandlerTest extends TestCase | ||
{ | ||
protected $config; | ||
|
||
protected $container; | ||
|
||
protected $handler; | ||
|
||
protected $request; | ||
|
||
public function setUp() | ||
{ | ||
$this->config = m::mock(Config::class); | ||
|
||
$this->request = m::mock('StdClass'); | ||
|
||
$this->container = Container::setInstance(new Container); | ||
|
||
$this->container->singleton('config', function () { | ||
return $this->config; | ||
}); | ||
|
||
$this->container->singleton('Illuminate\Contracts\Routing\ResponseFactory', function () { | ||
return new \Illuminate\Routing\ResponseFactory( | ||
m::mock(\Illuminate\Contracts\View\Factory::class), | ||
m::mock(\Illuminate\Routing\Redirector::class) | ||
); | ||
}); | ||
|
||
$this->handler = new Handler($this->container); | ||
} | ||
|
||
public function tearDown() | ||
{ | ||
m::close(); | ||
} | ||
|
||
public function testReturnsHtmlPageWithStackTraceWhenHtmlRequestAndDebugTrue() | ||
{ | ||
$this->config->shouldReceive('get')->with('app.debug', null)->twice()->andReturn(true); | ||
$this->request->shouldReceive('expectsJson')->once()->andReturn(false); | ||
|
||
$response = $this->handler->render($this->request, new Exception('My custom error message'))->getContent(); | ||
|
||
$this->assertContains('<!DOCTYPE html>', $response); | ||
$this->assertContains('<h1>Whoops, looks like something went wrong.</h1>', $response); | ||
$this->assertContains('My custom error message', $response); | ||
$this->assertContains('::main()', $response); | ||
$this->assertNotContains('"message":', $response); | ||
} | ||
|
||
public function testReturnsJsonWithStackTraceWhenAjaxRequestAndDebugTrue() | ||
{ | ||
$this->config->shouldReceive('get')->with('app.debug', null)->once()->andReturn(true); | ||
$this->request->shouldReceive('expectsJson')->once()->andReturn(true); | ||
|
||
$response = $this->handler->render($this->request, new Exception('My custom error message'))->getContent(); | ||
|
||
$this->assertNotContains('<!DOCTYPE html>', $response); | ||
$this->assertContains('"message": "My custom error message"', $response); | ||
$this->assertContains('"file":', $response); | ||
$this->assertContains('"line":', $response); | ||
$this->assertContains('"trace":', $response); | ||
} | ||
|
||
public function testReturnsJsonWithoutStackTraceWhenAjaxRequestAndDebugFalseAndExceptionMessageIsMasked() | ||
{ | ||
$this->config->shouldReceive('get')->with('app.debug', null)->once()->andReturn(false); | ||
$this->request->shouldReceive('expectsJson')->once()->andReturn(true); | ||
|
||
$response = $this->handler->render($this->request, new Exception('This error message should not be visible'))->getContent(); | ||
|
||
$this->assertContains('"message": "Server Error"', $response); | ||
$this->assertNotContains('<!DOCTYPE html>', $response); | ||
$this->assertNotContains('This error message should not be visible', $response); | ||
$this->assertNotContains('"file":', $response); | ||
$this->assertNotContains('"line":', $response); | ||
$this->assertNotContains('"trace":', $response); | ||
} | ||
|
||
public function testReturnsJsonWithoutStackTraceWhenAjaxRequestAndDebugFalseAndHttpExceptionErrorIsShown() | ||
{ | ||
$this->config->shouldReceive('get')->with('app.debug', null)->once()->andReturn(false); | ||
$this->request->shouldReceive('expectsJson')->once()->andReturn(true); | ||
|
||
$response = $this->handler->render($this->request, new HttpException(403, 'My custom error message'))->getContent(); | ||
|
||
$this->assertContains('"message": "My custom error message"', $response); | ||
$this->assertNotContains('<!DOCTYPE html>', $response); | ||
$this->assertNotContains('"message": "Server Error"', $response); | ||
$this->assertNotContains('"file":', $response); | ||
$this->assertNotContains('"line":', $response); | ||
$this->assertNotContains('"trace":', $response); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe only one condition could be used here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasmichot - I'm not sure what do you mean? If it is a HttpException - we will use the exception code we were given (i.e.
403
). But if it is not a HttpException - then we need to cast it as a500
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that we can actually make this check only once:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to Taylor I guess? Those lines were what he wrote already in the master branch that currently exists. I'll leave it as is for this PR to keep changes to a minimum and we can review after?