From 9e769b0206ff038222a5400d63a05f40c62cbd9d Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 12 Apr 2023 10:51:47 +0100 Subject: [PATCH 1/5] raise a ConfigError on an invalid app_service_config_files --- synapse/config/appservice.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 00182090b2ea..94081078ff16 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -42,8 +42,11 @@ def load_appservices( """Returns a list of Application Services from the config files.""" if not isinstance(config_files, list): # type-ignore: this function gets arbitrary json value; we do use this path. - logger.warning("Expected %s to be a list of AS config files.", config_files) # type: ignore[unreachable] - return [] + raise ConfigError( + "Expected '%s' to be a list of AS config files:" + % (config_files) + "app_service_config_files" + ) # Dicts of value -> filename seen_as_tokens: Dict[str, str] = {} From 9cc02810870e5a8c6619845b27cab5f2465ec7b5 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 12 Apr 2023 10:52:53 +0100 Subject: [PATCH 2/5] changelog --- changelog.d/15425.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15425.bugfix diff --git a/changelog.d/15425.bugfix b/changelog.d/15425.bugfix new file mode 100644 index 000000000000..fd104a63b3bd --- /dev/null +++ b/changelog.d/15425.bugfix @@ -0,0 +1 @@ +Synapse now correctly fails to start if the config option `app_service_config_files` is not a list. \ No newline at end of file From b5cfe5b64f1160bacb293c16f8667aa451603b5b Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 12 Apr 2023 11:08:40 +0100 Subject: [PATCH 3/5] Move config check to read_config --- synapse/config/appservice.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 94081078ff16..eb497310b979 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -33,6 +33,14 @@ class AppServiceConfig(Config): def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.app_service_config_files = config.get("app_service_config_files", []) + if not isinstance(self.app_service_config_files, list): + # type-ignore: this function gets arbitrary json value; we do use this path. + raise ConfigError( + "Expected '%s' to be a list of AS config files:" + % (self.app_service_config_files), + "app_service_config_files" + ) + self.track_appservice_user_ips = config.get("track_appservice_user_ips", False) @@ -40,13 +48,6 @@ def load_appservices( hostname: str, config_files: List[str] ) -> List[ApplicationService]: """Returns a list of Application Services from the config files.""" - if not isinstance(config_files, list): - # type-ignore: this function gets arbitrary json value; we do use this path. - raise ConfigError( - "Expected '%s' to be a list of AS config files:" - % (config_files) - "app_service_config_files" - ) # Dicts of value -> filename seen_as_tokens: Dict[str, str] = {} From 04d17552b08c7b350847fd47c7a3584a8985951a Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 12 Apr 2023 11:09:02 +0100 Subject: [PATCH 4/5] Add test --- synapse/config/appservice.py | 2 +- tests/config/test_appservice.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 tests/config/test_appservice.py diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index eb497310b979..657ec4c8e729 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -38,7 +38,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: raise ConfigError( "Expected '%s' to be a list of AS config files:" % (self.app_service_config_files), - "app_service_config_files" + "app_service_config_files", ) self.track_appservice_user_ips = config.get("track_appservice_user_ips", False) diff --git a/tests/config/test_appservice.py b/tests/config/test_appservice.py new file mode 100644 index 000000000000..088a44a11dc7 --- /dev/null +++ b/tests/config/test_appservice.py @@ -0,0 +1,32 @@ +# Copyright 2023 Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from synapse.config.appservice import AppServiceConfig, ConfigError + +from tests.unittest import TestCase + + +class AppServiceConfigTest(TestCase): + def test_invalid_app_service_config_files(self) -> None: + for invalid_value in ["foobar", 1, None, True, False, {}]: + with self.assertRaises(ConfigError): + AppServiceConfig().read_config( + {"app_service_config_files": invalid_value} + ) + + def test_valid_app_service_config_files(self) -> None: + AppServiceConfig().read_config({"app_service_config_files": []}) + AppServiceConfig().read_config( + {"app_service_config_files": ["/not/a/real/path", "/not/a/real/path/2"]} + ) From 3947dd03970f6450d4604b0731a3e25fb84a902e Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 12 Apr 2023 11:41:19 +0100 Subject: [PATCH 5/5] Ensure list also contains strings --- synapse/config/appservice.py | 4 +++- tests/config/test_appservice.py | 10 +++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 657ec4c8e729..fd89960e7261 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -33,7 +33,9 @@ class AppServiceConfig(Config): def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.app_service_config_files = config.get("app_service_config_files", []) - if not isinstance(self.app_service_config_files, list): + if not isinstance(self.app_service_config_files, list) or not all( + type(x) is str for x in self.app_service_config_files + ): # type-ignore: this function gets arbitrary json value; we do use this path. raise ConfigError( "Expected '%s' to be a list of AS config files:" diff --git a/tests/config/test_appservice.py b/tests/config/test_appservice.py index 088a44a11dc7..d2d1a40dfcb8 100644 --- a/tests/config/test_appservice.py +++ b/tests/config/test_appservice.py @@ -19,7 +19,15 @@ class AppServiceConfigTest(TestCase): def test_invalid_app_service_config_files(self) -> None: - for invalid_value in ["foobar", 1, None, True, False, {}]: + for invalid_value in [ + "foobar", + 1, + None, + True, + False, + {}, + ["foo", "bar", False], + ]: with self.assertRaises(ConfigError): AppServiceConfig().read_config( {"app_service_config_files": invalid_value}