Port NoExceptionTracebackHook from Ironic

Change-Id: I160d602bb39a82c2ffb41d18389f793ae9269f87
Partial-Bug: 1411871

magnum/api/config.py

@@ -24,6 +24,7 @@ app = {
     'hooks': [
         hooks.ContextHook(),
         hooks.RPCHook(),
+        hooks.NoExceptionTracebackHook(),
     ],
     'acl_public_routes': [
         '/'

magnum/api/hooks.py

@@ -72,4 +72,41 @@ class RPCHook(hooks.PecanHook):
     """Attach the rpcapi object to the request so controllers can get to it."""
 
     def before(self, state):
-        state.request.rpcapi = conductor_api.API(context=state.request.context)
+        state.request.rpcapi = conductor_api.API(context=state.request.context)
+
+
+class NoExceptionTracebackHook(hooks.PecanHook):
+    """Workaround rpc.common: deserialize_remote_exception.
+    deserialize_remote_exception builds rpc exception traceback into error
+    message which is then sent to the client. Such behavior is a security
+    concern so this hook is aimed to cut-off traceback from the error message.
+    """
+    # NOTE(max_lobur): 'after' hook used instead of 'on_error' because
+    # 'on_error' never fired for wsme+pecan pair. wsme @wsexpose decorator
+    # catches and handles all the errors, so 'on_error' dedicated for unhandled
+    # exceptions never fired.
+    def after(self, state):
+        # Omit empty body. Some errors may not have body at this level yet.
+        if not state.response.body:
+            return
+
+        # Do nothing if there is no error.
+        if 200 <= state.response.status_int < 400:
+            return
+
+        json_body = state.response.json
+        # Do not remove traceback when server in debug mode (except 'Server'
+        # errors when 'debuginfo' will be used for traces).
+        if cfg.CONF.debug and json_body.get('faultcode') != 'Server':
+            return
+
+        faultsting = json_body.get('faultstring')
+        traceback_marker = 'Traceback (most recent call last):'
+        if faultsting and (traceback_marker in faultsting):
+            # Cut-off traceback.
+            faultsting = faultsting.split(traceback_marker, 1)[0]
+            # Remove trailing newlines and spaces if any.
+            json_body['faultstring'] = faultsting.rstrip()
+            # Replace the whole json. Cannot change original one beacause it's
+            # generated on the fly.
+            state.response.json = json_body

magnum/tests/api/base.py

@@ -27,6 +27,7 @@ import pecan
 import pecan.testing
 from six.moves.urllib import parse as urlparse
 
+from magnum.api import hooks
 from magnum.db import api as dbapi
 from magnum.tests.db import base
 
@@ -67,6 +68,9 @@ class FunctionalTest(base.DbTestCase):
                 'template_path': '%s/api/templates' % root_dir,
                 'enable_acl': enable_acl,
                 'acl_public_routes': ['/', '/v1'],
+                'hooks': [
+                    hooks.NoExceptionTracebackHook(),
+                ],
             },
         }
 

magnum/tests/api/test_hooks.py

@@ -13,18 +13,24 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import mock
+import json
 
+import mock
+from oslo.config import cfg
+from oslo import messaging
+
+from magnum.api.controllers import root
 from magnum.api import hooks
 from magnum.common import context
+from magnum.tests.api import base as api_base
 from magnum.tests import base
 from magnum.tests import fakes
 
 
-class TestHooks(base.BaseTestCase):
+class TestContextHook(base.BaseTestCase):
 
     def setUp(self):
-        super(TestHooks, self).setUp()
+        super(TestContextHook, self).setUp()
         self.app = fakes.FakeApp()
 
     def test_context_hook_before_method(self):
@@ -57,3 +63,77 @@ class TestHooks(base.BaseTestCase):
         self.assertEqual(fakes.fakeAuthTokenHeaders['X-Auth-Token'],
                          ctx.auth_token)
         self.assertEqual('assert_this', ctx.auth_token_info)
+
+
+class TestNoExceptionTracebackHook(api_base.FunctionalTest):
+
+    TRACE = [u'Traceback (most recent call last):',
+             u'  File "/opt/stack/magnum/magnum/openstack/common/rpc/amqp.py",'
+             ' line 434, in _process_data\\n   **args)',
+             u'  File "/opt/stack/magnum/magnum/openstack/common/rpc/'
+             'dispatcher.py", line 172, in dispatch\\n   result ='
+             ' getattr(proxyobj, method)(ctxt, **kwargs)']
+    MSG_WITHOUT_TRACE = "Test exception message."
+    MSG_WITH_TRACE = MSG_WITHOUT_TRACE + "\n" + "\n".join(TRACE)
+
+    def setUp(self):
+        super(TestNoExceptionTracebackHook, self).setUp()
+        p = mock.patch.object(root.Root, 'convert')
+        self.root_convert_mock = p.start()
+        self.addCleanup(p.stop)
+
+    def test_hook_exception_success(self):
+        self.root_convert_mock.side_effect = Exception(self.MSG_WITH_TRACE)
+
+        response = self.get_json('/', path_prefix='', expect_errors=True)
+
+        actual_msg = json.loads(response.json['error_message'])['faultstring']
+        self.assertEqual(self.MSG_WITHOUT_TRACE, actual_msg)
+
+    def test_hook_remote_error_success(self):
+        test_exc_type = 'TestException'
+        self.root_convert_mock.side_effect = messaging.rpc.RemoteError(
+            test_exc_type, self.MSG_WITHOUT_TRACE, self.TRACE)
+
+        response = self.get_json('/', path_prefix='', expect_errors=True)
+
+        # NOTE(max_lobur): For RemoteError the client message will still have
+        # some garbage because in RemoteError traceback is serialized as a list
+        # instead of'\n'.join(trace). But since RemoteError is kind of very
+        # rare thing (happens due to wrong deserialization settings etc.)
+        # we don't care about this garbage.
+        expected_msg = ("Remote error: %s %s"
+                        % (test_exc_type, self.MSG_WITHOUT_TRACE) + "\n[u'")
+        actual_msg = json.loads(response.json['error_message'])['faultstring']
+        self.assertEqual(expected_msg, actual_msg)
+
+    def test_hook_without_traceback(self):
+        msg = "Error message without traceback \n but \n multiline"
+        self.root_convert_mock.side_effect = Exception(msg)
+
+        response = self.get_json('/', path_prefix='', expect_errors=True)
+
+        actual_msg = json.loads(response.json['error_message'])['faultstring']
+        self.assertEqual(msg, actual_msg)
+
+    def test_hook_server_debug_on_serverfault(self):
+        cfg.CONF.set_override('debug', True)
+        self.root_convert_mock.side_effect = Exception(self.MSG_WITH_TRACE)
+
+        response = self.get_json('/', path_prefix='', expect_errors=True)
+
+        actual_msg = json.loads(
+            response.json['error_message'])['faultstring']
+        self.assertEqual(self.MSG_WITHOUT_TRACE, actual_msg)
+
+    def test_hook_server_debug_on_clientfault(self):
+        cfg.CONF.set_override('debug', True)
+        client_error = Exception(self.MSG_WITH_TRACE)
+        client_error.code = 400
+        self.root_convert_mock.side_effect = client_error
+
+        response = self.get_json('/', path_prefix='', expect_errors=True)
+
+        actual_msg = json.loads(
+            response.json['error_message'])['faultstring']
+        self.assertEqual(self.MSG_WITH_TRACE, actual_msg)