diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3ca0de1b7b..43d317b942 100644 --- a/CHANGELOG.rst +++ b/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 ~~~~~ diff --git a/st2api/st2api/controllers/v1/webhooks.py b/st2api/st2api/controllers/v1/webhooks.py index f14d5e6b8a..dd31abbb5f 100644 --- a/st2api/st2api/controllers/v1/webhooks.py +++ b/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) diff --git a/st2api/tests/unit/controllers/v1/test_webhooks.py b/st2api/tests/unit/controllers/v1/test_webhooks.py index c537366dc1..487830a092 100644 --- a/st2api/tests/unit/controllers/v1/test_webhooks.py +++ b/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,