From d8732d1e80c87baedc3f5a808994330fbce9d178 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sun, 9 May 2021 08:37:37 +0900 Subject: [PATCH 1/2] Fix #330 Potentially request.body can be None when using a custom adapter --- slack_bolt/request/async_request.py | 7 +++--- slack_bolt/request/internals.py | 4 ---- slack_bolt/request/request.py | 6 ++--- tests/slack_bolt/request/test_request.py | 23 +++++++++++++++++++ tests/slack_bolt_async/request/__init__.py | 0 .../request/test_async_request.py | 21 +++++++++++++++++ 6 files changed, 51 insertions(+), 10 deletions(-) create mode 100644 tests/slack_bolt/request/test_request.py create mode 100644 tests/slack_bolt_async/request/__init__.py create mode 100644 tests/slack_bolt_async/request/test_async_request.py diff --git a/slack_bolt/request/async_request.py b/slack_bolt/request/async_request.py index 97ceab159..ad671ccc2 100644 --- a/slack_bolt/request/async_request.py +++ b/slack_bolt/request/async_request.py @@ -9,7 +9,6 @@ build_normalized_headers, extract_content_type, error_message_raw_body_required_in_http_mode, - error_message_unknown_request_body_type, ) @@ -45,7 +44,7 @@ def __init__( if mode == "http": # HTTP Mode - if not isinstance(body, str): + if body is not None and not isinstance(body, str): raise BoltError(error_message_raw_body_required_in_http_mode()) self.raw_body = body if body is not None else "" else: @@ -60,12 +59,14 @@ def __init__( self.query = parse_query(query) self.headers = build_normalized_headers(headers) self.content_type = extract_content_type(self.headers) + if isinstance(body, str): self.body = parse_body(self.raw_body, self.content_type) elif isinstance(body, dict): self.body = body else: - raise BoltError(error_message_unknown_request_body_type()) + self.body = {} + self.context = build_async_context( AsyncBoltContext(context if context else {}), self.body ) diff --git a/slack_bolt/request/internals.py b/slack_bolt/request/internals.py index 4b70df1b3..2fd0c430e 100644 --- a/slack_bolt/request/internals.py +++ b/slack_bolt/request/internals.py @@ -183,10 +183,6 @@ def error_message_raw_body_required_in_http_mode() -> str: return "`body` must be a raw string data when running in the HTTP server mode" -def error_message_unknown_request_body_type() -> str: - return "`body` must be either str or dict" - - def debug_multiple_response_urls_detected() -> str: return ( "`response_urls` in the body has multiple URLs in it. " diff --git a/slack_bolt/request/request.py b/slack_bolt/request/request.py index 5d48d7a35..d91e78ba6 100644 --- a/slack_bolt/request/request.py +++ b/slack_bolt/request/request.py @@ -9,7 +9,6 @@ build_context, extract_content_type, error_message_raw_body_required_in_http_mode, - error_message_unknown_request_body_type, ) @@ -44,7 +43,7 @@ def __init__( """ if mode == "http": # HTTP Mode - if not isinstance(body, str): + if body is not None and not isinstance(body, str): raise BoltError(error_message_raw_body_required_in_http_mode()) self.raw_body = body if body is not None else "" else: @@ -59,12 +58,13 @@ def __init__( self.query = parse_query(query) self.headers = build_normalized_headers(headers) self.content_type = extract_content_type(self.headers) + if isinstance(body, str): self.body = parse_body(self.raw_body, self.content_type) elif isinstance(body, dict): self.body = body else: - raise BoltError(error_message_unknown_request_body_type()) + self.body = {} self.context = build_context(BoltContext(context if context else {}), self.body) self.lazy_only = bool(self.headers.get("x-slack-bolt-lazy-only", [False])[0]) diff --git a/tests/slack_bolt/request/test_request.py b/tests/slack_bolt/request/test_request.py new file mode 100644 index 000000000..b6a81ecca --- /dev/null +++ b/tests/slack_bolt/request/test_request.py @@ -0,0 +1,23 @@ +from slack_bolt.request.request import BoltRequest + + +class TestRequest: + def setup_method(self): + pass + + def teardown_method(self): + pass + + def test_all_none_inputs_http(self): + req = BoltRequest(body=None, headers=None, query=None, context=None) + assert req is not None + assert req.raw_body == "" + assert req.body == {} + + def test_all_none_inputs_socket_mode(self): + req = BoltRequest( + body=None, headers=None, query=None, context=None, mode="socket_mode" + ) + assert req is not None + assert req.raw_body == "" + assert req.body == {} diff --git a/tests/slack_bolt_async/request/__init__.py b/tests/slack_bolt_async/request/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/slack_bolt_async/request/test_async_request.py b/tests/slack_bolt_async/request/test_async_request.py new file mode 100644 index 000000000..e69a242cf --- /dev/null +++ b/tests/slack_bolt_async/request/test_async_request.py @@ -0,0 +1,21 @@ +import pytest + +from slack_bolt.request.async_request import AsyncBoltRequest + + +class TestAsyncRequest: + @pytest.mark.asyncio + async def test_all_none_values_http(self): + req = AsyncBoltRequest(body=None, headers=None, query=None, context=None) + assert req is not None + assert req.raw_body == "" + assert req.body == {} + + @pytest.mark.asyncio + async def test_all_none_values_socket_mode(self): + req = AsyncBoltRequest( + body=None, headers=None, query=None, context=None, mode="socket_mode" + ) + assert req is not None + assert req.raw_body == "" + assert req.body == {} From caac693aa21b92e3b21776b99fdaf94bbd864fdc Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sun, 9 May 2021 09:07:21 +0900 Subject: [PATCH 2/2] Add scenario tests --- tests/scenario_tests/test_app.py | 37 +++++++++- .../scenario_tests_async/test_app_dispatch.py | 72 +++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 tests/scenario_tests_async/test_app_dispatch.py diff --git a/tests/scenario_tests/test_app.py b/tests/scenario_tests/test_app.py index 5d91dd577..8d3a4a3db 100644 --- a/tests/scenario_tests/test_app.py +++ b/tests/scenario_tests/test_app.py @@ -3,7 +3,7 @@ from slack_sdk.oauth.installation_store import FileInstallationStore from slack_sdk.oauth.state_store import FileOAuthStateStore -from slack_bolt import App, Say +from slack_bolt import App, Say, BoltRequest from slack_bolt.authorization import AuthorizeResult from slack_bolt.error import BoltError from slack_bolt.oauth import OAuthFlow @@ -187,3 +187,38 @@ def test_installation_store_conflicts(self): installation_store=store1, ) assert app.installation_store is store1 + + def test_none_body(self): + app = App(signing_secret="valid", client=self.web_client) + + req = BoltRequest(body=None, headers={}, mode="http") + response = app.dispatch(req) + # request verification failure + assert response.status == 401 + assert response.body == '{"error": "invalid request"}' + + req = BoltRequest(body=None, headers={}, mode="socket_mode") + response = app.dispatch(req) + # request verification is skipped for Socket Mode + assert response.status == 404 + assert response.body == '{"error": "unhandled request"}' + + def test_none_body_no_middleware(self): + app = App( + signing_secret="valid", + client=self.web_client, + ssl_check_enabled=False, + ignoring_self_events_enabled=False, + request_verification_enabled=False, + token_verification_enabled=False, + url_verification_enabled=False, + ) + req = BoltRequest(body=None, headers={}, mode="http") + response = app.dispatch(req) + assert response.status == 404 + assert response.body == '{"error": "unhandled request"}' + + req = BoltRequest(body=None, headers={}, mode="socket_mode") + response = app.dispatch(req) + assert response.status == 404 + assert response.body == '{"error": "unhandled request"}' diff --git a/tests/scenario_tests_async/test_app_dispatch.py b/tests/scenario_tests_async/test_app_dispatch.py new file mode 100644 index 000000000..34396a2d1 --- /dev/null +++ b/tests/scenario_tests_async/test_app_dispatch.py @@ -0,0 +1,72 @@ +import asyncio + +import pytest +from slack_sdk.web.async_client import AsyncWebClient + +from slack_bolt.app.async_app import AsyncApp +from slack_bolt.request.async_request import AsyncBoltRequest +from tests.mock_web_api_server import ( + setup_mock_web_api_server, + cleanup_mock_web_api_server, +) +from tests.utils import remove_os_env_temporarily, restore_os_env + + +class TestAsyncAppDispatch: + signing_secret = "secret" + valid_token = "xoxb-valid" + mock_api_server_base_url = "http://localhost:8888" + web_client = AsyncWebClient(token=valid_token, base_url=mock_api_server_base_url) + + @pytest.fixture + def event_loop(self): + old_os_env = remove_os_env_temporarily() + try: + setup_mock_web_api_server(self) + loop = asyncio.get_event_loop() + yield loop + loop.close() + cleanup_mock_web_api_server(self) + finally: + restore_os_env(old_os_env) + + @pytest.mark.asyncio + async def test_none_body(self): + app = AsyncApp( + client=self.web_client, + signing_secret=self.signing_secret, + ) + + req = AsyncBoltRequest(body=None, headers={}, mode="http") + response = await app.async_dispatch(req) + # request verification failure + assert response.status == 401 + assert response.body == '{"error": "invalid request"}' + + req = AsyncBoltRequest(body=None, headers={}, mode="socket_mode") + response = await app.async_dispatch(req) + # request verification is skipped for Socket Mode + assert response.status == 404 + assert response.body == '{"error": "unhandled request"}' + + @pytest.mark.asyncio + async def test_none_body_no_middleware(self): + app = AsyncApp( + client=self.web_client, + signing_secret=self.signing_secret, + ssl_check_enabled=False, + ignoring_self_events_enabled=False, + request_verification_enabled=False, + # token_verification_enabled=False, + url_verification_enabled=False, + ) + + req = AsyncBoltRequest(body=None, headers={}, mode="http") + response = await app.async_dispatch(req) + assert response.status == 404 + assert response.body == '{"error": "unhandled request"}' + + req = AsyncBoltRequest(body=None, headers={}, mode="socket_mode") + response = await app.async_dispatch(req) + assert response.status == 404 + assert response.body == '{"error": "unhandled request"}'