Skip to content

Fix Web API error masking in production mode #2371

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

Merged
merged 1 commit into from
Mar 1, 2016
Merged

Fix Web API error masking in production mode #2371

merged 1 commit into from
Mar 1, 2016

Conversation

skolodyazhnyy
Copy link

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes

In this PR I have changed exception masking for Web API. Current implementation hide Stack trace and may cause fatal errors for custom exception types because it tries to reinstantiate exception with different messages. In my proposed implementation I'm wrapping original exception into WebapiExceptionReport which carry reportId in the property and original exception as previous exception.

Ways to reproduce:

A. Losing stack trace

  1. Make sure developer mode disabled
  2. Make an API query where a critical exception occurs. You may want to change API implementation in order to force an exception.

Expected result
I can see WebAPI report ID in the API response and exception stack trace in the log file.

Actual result
I can see WebAPI report ID in the API response but exception stack trace always refer to crtitical method call:

[2015-11-14 00:14:31] main.CRITICAL: exception 'ReflectionException' with message 'Report ID: webapi-56467ce7f2048; Message: Class string does not exist' in /var/www/lib/internal/Magento/Framework/Webapi/ErrorProcessor.php:194
Stack trace:
#0 /var/www/lib/internal/Magento/Framework/Webapi/ErrorProcessor.php(139): Magento\Framework\Webapi\ErrorProcessor->_critical(Object(ReflectionException))
#1 /var/www/app/code/Magento/Webapi/Controller/Rest.php(163): Magento\Framework\Webapi\ErrorProcessor->maskException(Object(ReflectionException))
#2 /var/www/var/generation/Magento/Webapi/Controller/Rest/Interceptor.php(24): Magento\Webapi\Controller\Rest->dispatch(Object(Magento\Framework\App\Request\Http))
#3 /var/www/lib/internal/Magento/Framework/App/Http.php(115): Magento\Webapi\Controller\Rest\Interceptor->dispatch(Object(Magento\Framework\App\Request\Http))
#4 /var/www/lib/internal/Magento/Framework/App/Bootstrap.php(258): Magento\Framework\App\Http->launch()
#5 /var/www/index.php(45): Magento\Framework\App\Bootstrap->run(Object(Magento\Framework\App\Http))
#6 {main} [] []

B. Fatal error during exception creation

  1. Make sure developer mode is disabled
  2. Define a new exception type which has custom constructor, for example:
class UnexpectedPropertyTypeException extends Exception
{
    public function __construct(array $expectedTypes, $actualType)
    {
        parent::__construct(sprintf("Unexpected property type %s, expected types: %s", $actualType, implode(', ', $expectedTypes)));
    }
}
  1. Throw this exception somewhere in your API implementation

Expected result
We should see exception message in the log

Actual result
Fatal error: Argument 1 passed to ...::__construct() must be of the type array, string given

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@Vinai Vinai added the PS label Nov 25, 2015
@Vinai
Copy link
Contributor

Vinai commented Nov 25, 2015

Thank you for your contribution!
All tests pass, the contributor license agreement is signed, code looks okay: ready for review by core team.

@okorshenko okorshenko self-assigned this Feb 17, 2016
@vrann
Copy link
Contributor

vrann commented Feb 24, 2016

Change makes sense, thanks for contribution!

@vrann vrann added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept labels Feb 24, 2016
@okorshenko
Copy link
Contributor

@skolodyazhnyy, because of integrity tests failure, we will modify the code of this PR. But thank you for contribution.

@magento-team magento-team merged commit c60b3a7 into magento:develop Mar 1, 2016
magento-team pushed a commit that referenced this pull request Mar 1, 2016

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@okorshenko
Copy link
Contributor

@skolodyazhnyy your Pull Request merged to develop branch. Thank you for contribution!

magento-engcom-team pushed a commit that referenced this pull request Apr 13, 2018
MAGETWO-86554: Customer is redirected to 404 from Catalog page if switches to the Store with another root Category
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants