Remove authentication headers from webhook payloads

CHANGELOG.rst

@@ -48,6 +48,10 @@ Changed
 * Renamed reference to the RBAC backend/plugin from ``enterprise`` to ``default``. Updated st2api
   validation to use the new value when checking RBAC configuration. Removed other references to
   enterprise for RBAC related contents. (improvement)
+* Remove authentication headers ``St2-Api-Key``, ``X-Auth-Token`` and ``Cookie`` from webhook payloads to
+  prevent them from being stored in the database. (security bug fix) #4983
+
+  Contributed by @potato and @knagy
 
 Fixed
 ~~~~~

st2api/st2api/controllers/v1/webhooks.py

@@ -19,6 +19,7 @@
 urljoin = urlparse.urljoin
 
 from st2common import log as logging
+from st2common.constants.auth import HEADER_API_KEY_ATTRIBUTE_NAME, HEADER_ATTRIBUTE_NAME
 from st2common.constants.triggers import WEBHOOK_TRIGGER_TYPES
 from st2common.models.api.trace import TraceContext
 from st2common.models.api.trigger import TriggerAPI
@@ -126,6 +127,7 @@ def post(self, hook, webhook_body_api, headers, requester_user):
                                                           permission_type=permission_type)
 
         headers = self._get_headers_as_dict(headers)
+        headers = self._filter_authentication_headers(headers)
 
         # If webhook contains a trace-tag use that else create create a unique trace-tag.
         trace_context = self._create_trace_context(trace_tag=headers.pop(TRACE_TAG_HEADER, None),
@@ -219,6 +221,10 @@ def _get_headers_as_dict(self, headers):
             headers_dict[key] = value
         return headers_dict
 
+    def _filter_authentication_headers(self, headers):
+        auth_headers = [HEADER_API_KEY_ATTRIBUTE_NAME, HEADER_ATTRIBUTE_NAME, 'Cookie']
+        return {key: value for key, value in headers.items() if key not in auth_headers}
+
     def _log_request(self, msg, headers, body, log_method=LOG.debug):
         headers = self._get_headers_as_dict(headers)
         body = str(body)

st2api/tests/unit/controllers/v1/test_webhooks.py

@@ -320,6 +320,26 @@ def get_webhook_trigger(name, url):
         self.assertTrue(controller._is_valid_hook('with_leading_trailing_slash'))
         self.assertTrue(controller._is_valid_hook('with/mixed/slash'))
 
+    @mock.patch.object(TriggerInstancePublisher, 'publish_trigger', mock.MagicMock(
+        return_value=True))
+    @mock.patch.object(WebhooksController, '_is_valid_hook', mock.MagicMock(
+        return_value=True))
+    @mock.patch.object(HooksHolder, 'get_triggers_for_hook', mock.MagicMock(
+        return_value=[DUMMY_TRIGGER_DICT]))
+    @mock.patch('st2common.transport.reactor.TriggerDispatcher.dispatch')
+    def test_authentication_headers_should_be_removed(self, dispatch_mock):
+        headers = {
+            'Content-Type': 'application/x-www-form-urlencoded',
+            'St2-Api-Key': 'foobar',
+            'X-Auth-Token': 'deadbeaf',
+            'Cookie': 'foo=bar'
+        }
+
+        self.app.post('/v1/webhooks/git', WEBHOOK_1, headers=headers)
+        self.assertNotIn('St2-Api-Key', dispatch_mock.call_args[1]['payload']['headers'])
+        self.assertNotIn('X-Auth-Token', dispatch_mock.call_args[1]['payload']['headers'])
+        self.assertNotIn('Cookie', dispatch_mock.call_args[1]['payload']['headers'])
+
     def __do_post(self, hook, webhook, expect_errors=False, headers=None):
         return self.app.post_json('/v1/webhooks/' + hook,
                                   params=webhook,