From f4e0808a87961424fadbe7b5901727ce5ef0f7ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20H=C3=B6nig?= Date: Wed, 30 May 2018 11:29:13 +0200 Subject: [PATCH] botserver: Validate token before accepting message. Previously, the botserver would accept any message sent to it. This was a security hazard, since an attacker could impersonate arbitrary users with arbitrary messages. We only want the Zulip instance where a bot is registered to be able to send out messages for that bot. To do this, this commits adds a check for the security token associated with each outgoing webhook bot. For each bot, its token is stored in the botserverrc file. The server sends the token along with each message. --- zulip_botserver/tests/test.conf | 2 ++ zulip_botserver/tests/test_server.py | 35 +++++++++++++++++++++-- zulip_botserver/zulip_botserver/server.py | 10 +++++-- 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/zulip_botserver/tests/test.conf b/zulip_botserver/tests/test.conf index b59691f..5a316f5 100644 --- a/zulip_botserver/tests/test.conf +++ b/zulip_botserver/tests/test.conf @@ -2,7 +2,9 @@ key=value email=helloworld-bot@zulip.com site=http://localhost +token=abcd1234 [giphy] key=value2 email=giphy-bot@zulip.com site=http://localhost +token=abcd1234 diff --git a/zulip_botserver/tests/test_server.py b/zulip_botserver/tests/test_server.py index f0b221c..604ae0e 100644 --- a/zulip_botserver/tests/test_server.py +++ b/zulip_botserver/tests/test_server.py @@ -26,13 +26,15 @@ class BotServerTests(BotServerTestCase): 'email': 'helloworld-bot@zulip.com', 'key': '123456789qwertyuiop', 'site': 'http://localhost', + 'token': 'abcd1234', } } self.assert_bot_server_response(available_bots=available_bots, bots_config=bots_config, event=dict(message={'content': "@**test** test message"}, bot_email='helloworld-bot@zulip.com', - trigger='mention'), + trigger='mention', + token='abcd1234'), expected_response="beep boop", check_success=True) @@ -43,17 +45,20 @@ class BotServerTests(BotServerTestCase): 'email': 'helloworld-bot@zulip.com', 'key': '123456789qwertyuiop', 'site': 'http://localhost', + 'token': 'abcd1234', }, 'help': { 'email': 'help-bot@zulip.com', 'key': '123456789qwertyuiop', 'site': 'http://localhost', + 'token': 'abcd1234', } } self.assert_bot_server_response(available_bots=available_bots, event=dict(message={'content': "@**test** test message"}, bot_email='helloworld-bot@zulip.com', - trigger='mention'), + trigger='mention', + token='abcd1234'), expected_response="beep boop", bots_config=bots_config, check_success=True) @@ -64,6 +69,7 @@ class BotServerTests(BotServerTestCase): 'email': 'helloworld-bot@zulip.com', 'key': '123456789qwertyuiop', 'site': 'http://localhost', + 'token': 'abcd1234', }, } self.assert_bot_server_response(available_bots=['helloworld'], @@ -72,6 +78,24 @@ class BotServerTests(BotServerTestCase): bots_config=bots_config, check_success=False) + def test_wrong_bot_token(self) -> None: + available_bots = ['helloworld'] + bots_config = { + 'helloworld': { + 'email': 'helloworld-bot@zulip.com', + 'key': '123456789qwertyuiop', + 'site': 'http://localhost', + 'token': 'abcd1234', + } + } + self.assert_bot_server_response(available_bots=available_bots, + bots_config=bots_config, + event=dict(message={'content': "@**test** test message"}, + bot_email='helloworld-bot@zulip.com', + trigger='mention', + token='wrongtoken'), + check_success=False) + @mock.patch('logging.error') @mock.patch('zulip_bots.lib.StateHandler') def test_wrong_bot_credentials(self, mock_StateHandler: mock.Mock, mock_LoggingError: mock.Mock) -> None: @@ -81,6 +105,7 @@ class BotServerTests(BotServerTestCase): 'email': 'helloworld-bot@zulip.com', 'key': '123456789qwertyuiop', 'site': 'http://localhost', + 'token': 'abcd1234', } } # TODO: The following passes mypy, though the six stubs don't match the @@ -93,7 +118,8 @@ class BotServerTests(BotServerTestCase): available_bots=available_bots, event=dict(message={'content': "@**test** test message"}, bot_email='helloworld-bot@zulip.com', - trigger='mention'), + trigger='mention', + token='abcd1234'), bots_config=bots_config)) @mock.patch('sys.argv', ['zulip-bot-server', '--config-file', '/foo/bar/baz.conf']) @@ -115,11 +141,13 @@ class BotServerTests(BotServerTestCase): 'email': 'helloworld-bot@zulip.com', 'key': 'value', 'site': 'http://localhost', + 'token': 'abcd1234', }, 'giphy': { 'email': 'giphy-bot@zulip.com', 'key': 'value2', 'site': 'http://localhost', + 'token': 'abcd1234', } } assert json.dumps(bot_conf1, sort_keys=True) == json.dumps(expected_config1, sort_keys=True) @@ -129,6 +157,7 @@ class BotServerTests(BotServerTestCase): 'email': 'helloworld-bot@zulip.com', 'key': 'value', 'site': 'http://localhost', + 'token': 'abcd1234', } } assert json.dumps(bot_conf2, sort_keys=True) == json.dumps(expected_config2, sort_keys=True) diff --git a/zulip_botserver/zulip_botserver/server.py b/zulip_botserver/zulip_botserver/server.py index 7bea153..a599abd 100644 --- a/zulip_botserver/zulip_botserver/server.py +++ b/zulip_botserver/zulip_botserver/server.py @@ -6,7 +6,7 @@ import os from flask import Flask, request from importlib import import_module from typing import Any, Dict, Union, List, Optional -from werkzeug.exceptions import BadRequest +from werkzeug.exceptions import BadRequest, Unauthorized from zulip import Client from zulip_bots import lib @@ -22,6 +22,7 @@ def read_config_file(config_file_path: str, bot_name: Optional[str]=None) -> Dic "email": parser.get(section, 'email'), "key": parser.get(section, 'key'), "site": parser.get(section, 'site'), + "token": parser.get(section, 'token'), } if bot_name is not None: logging.warning("Single bot mode is enabled") @@ -102,16 +103,21 @@ bots_config = {} # type: Dict[str, Dict[str, str]] @app.route('/', methods=['POST']) -def handle_bot() -> Union[str, BadRequest]: +def handle_bot() -> Union[str, BadRequest, Unauthorized]: event = request.get_json(force=True) for bot_name, config in bots_config.items(): if config['email'] == event['bot_email']: bot = bot_name + bot_config = config break else: return BadRequest("Cannot find a bot with email {} in the Botserver " "configuration file. Do the emails in your botserverrc " "match the bot emails on the server?".format(event['bot_email'])) + if bot_config['token'] != event['token']: + return Unauthorized("Request token does not match token found for bot {} in the " + "Botserver configuration file. Do the outgoing webhooks in " + "Zulip point to the right Botserver?".format(event['bot_email'])) lib_module = app.config.get("BOTS_LIB_MODULES", {})[bot] bot_handler = app.config.get("BOT_HANDLERS", {})[bot] message_handler = app.config.get("MESSAGE_HANDLERS", {})[bot]