-
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 8 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 |
---|---|---|
|
@@ -139,7 +139,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); | ||
} | ||
|
@@ -220,12 +220,20 @@ protected function prepareJsonResponse($request, Exception $e) | |
|
||
$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, [ | ||
if (config('app.debug')) { | ||
$response = [ | ||
'message' => $e->getMessage(), | ||
'file' => $e->getFile(), | ||
'line' => $e->getLine(), | ||
'trace' => $e->getTrace(), | ||
]; | ||
} else { | ||
$response = [ | ||
'message' => $this->isHttpException($e) ? $e->getMessage() : 'Server Error', | ||
]; | ||
} | ||
|
||
return response(json_encode($response, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES), $status, array_merge($headers, [ | ||
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. So we actually can only return JSON here, why not just: return new JsonResponse($response, $status, $headers, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES);
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. @lucasmichot - that line of code is basically what was already in the master branch. But I agree your way seems cleaner - so I've updated the PR. |
||
'Content-Type' => 'application/json', | ||
])); | ||
} | ||
|
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.
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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - we only return the detailed information if
debug
is true.