From 39c719ee58e61c505b836dbded2cfcdbf5b5be91 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 7 Oct 2025 18:09:27 -0500 Subject: [PATCH 1/9] Move open registration config validation --- synapse/app/homeserver.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 3424cdbdb81..166e3865b49 100644 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -330,6 +330,22 @@ def load_or_generate_config(argv_options: List[str]) -> HomeServerConfig: config = HomeServerConfig.load_or_generate_config( "Synapse Homeserver", argv_options ) + + if ( + config.registration.enable_registration + and not config.registration.enable_registration_without_verification + ): + if ( + not config.captcha.enable_registration_captcha + and not config.registration.registrations_require_3pid + and not config.registration.registration_requires_token + ): + raise ConfigError( + "You have enabled open registration without any verification. This is a known vector for " + "spam and abuse. If you would like to allow public registration, please consider adding email, " + "captcha, or token-based verification. Otherwise this check can be removed by setting the " + "`enable_registration_without_verification` config option to `true`." + ) except ConfigError as e: sys.stderr.write("\n") for f in format_config_error(e): @@ -380,22 +396,6 @@ def setup( if config.server.gc_seconds: synapse.metrics.MIN_TIME_BETWEEN_GCS = config.server.gc_seconds - if ( - config.registration.enable_registration - and not config.registration.enable_registration_without_verification - ): - if ( - not config.captcha.enable_registration_captcha - and not config.registration.registrations_require_3pid - and not config.registration.registration_requires_token - ): - raise ConfigError( - "You have enabled open registration without any verification. This is a known vector for " - "spam and abuse. If you would like to allow public registration, please consider adding email, " - "captcha, or token-based verification. Otherwise this check can be removed by setting the " - "`enable_registration_without_verification` config option to `true`." - ) - hs = SynapseHomeServer( config.server.server_name, config=config, From 3b282c45681f30aa59416cd35c219f8ab1df2591 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 7 Oct 2025 18:49:12 -0500 Subject: [PATCH 2/9] Add changelog --- changelog.d/19027.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/19027.misc diff --git a/changelog.d/19027.misc b/changelog.d/19027.misc new file mode 100644 index 00000000000..46fdab91e4f --- /dev/null +++ b/changelog.d/19027.misc @@ -0,0 +1 @@ +Move open registration config validation from `setup(...)` to `load_or_generate_config(...)`. From 4ac0690c2850525afdfa98e3a9c31795f01ae1f8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 8 Oct 2025 18:20:54 -0500 Subject: [PATCH 3/9] Move open registration validation to `HomeServerConfig` --- synapse/app/homeserver.py | 21 +------ synapse/config/_base.py | 10 +--- synapse/config/homeserver.py | 39 +++++++++++- tests/config/test_registration_config.py | 75 ++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 29 deletions(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 166e3865b49..10ac2924369 100644 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -330,22 +330,6 @@ def load_or_generate_config(argv_options: List[str]) -> HomeServerConfig: config = HomeServerConfig.load_or_generate_config( "Synapse Homeserver", argv_options ) - - if ( - config.registration.enable_registration - and not config.registration.enable_registration_without_verification - ): - if ( - not config.captcha.enable_registration_captcha - and not config.registration.registrations_require_3pid - and not config.registration.registration_requires_token - ): - raise ConfigError( - "You have enabled open registration without any verification. This is a known vector for " - "spam and abuse. If you would like to allow public registration, please consider adding email, " - "captcha, or token-based verification. Otherwise this check can be removed by setting the " - "`enable_registration_without_verification` config option to `true`." - ) except ConfigError as e: sys.stderr.write("\n") for f in format_config_error(e): @@ -367,7 +351,7 @@ def setup( freeze: bool = True, ) -> SynapseHomeServer: """ - Create and setup a Synapse homeserver instance given a configuration. + Create and setup the main Synapse homeserver instance given a configuration. Args: config: The configuration for the homeserver. @@ -385,10 +369,9 @@ def setup( if config.worker.worker_app: raise ConfigError( - "You have specified `worker_app` in the config but are attempting to start a non-worker " + "You have specified `worker_app` in the config but are attempting to setup a non-worker " "instance. Please use `python -m synapse.app.generic_worker` instead (or remove the option if this is the main process)." ) - sys.exit(1) events.USE_FROZEN_DICTS = config.server.use_frozen_dicts synapse.util.caches.TRACK_MEMORY_USAGE = config.caches.track_memory_usage diff --git a/synapse/config/_base.py b/synapse/config/_base.py index 6de4c12c960..d6f4b34f94d 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -842,15 +842,7 @@ def load_or_generate_config( ): return None - obj.parse_config_dict( - config_dict, - config_dir_path=config_dir_path, - data_dir_path=data_dir_path, - allow_secrets_in_config=config_args.secrets_in_config, - ) - obj.invoke_all("read_arguments", config_args) - - return obj + return cls.load_config(description, argv_options) def parse_config_dict( self, diff --git a/synapse/config/homeserver.py b/synapse/config/homeserver.py index 5d7089c2e6a..88a317cf74e 100644 --- a/synapse/config/homeserver.py +++ b/synapse/config/homeserver.py @@ -18,7 +18,9 @@ # [This file includes modifications made by New Vector Limited] # # -from ._base import RootConfig +from typing import List, Type, TypeVar + +from ._base import ConfigError, RootConfig from .account_validity import AccountValidityConfig from .api import ApiConfig from .appservice import AppServiceConfig @@ -64,8 +66,14 @@ from .voip import VoipConfig from .workers import WorkerConfig +THomeServerConfig = TypeVar("THomeServerConfig", bound="HomeServerConfig") + class HomeServerConfig(RootConfig): + """ + Top-level config object for Synapse homeserver (main process and workers). + """ + config_classes = [ ModulesConfig, ServerConfig, @@ -113,3 +121,32 @@ class HomeServerConfig(RootConfig): # This must be last, as it checks for conflicts with other config options. MasConfig, ] + + @classmethod + def load_config( + cls: Type[THomeServerConfig], description: str, argv: List[str] + ) -> THomeServerConfig: + """ + See `RootConfig.load_config`. + + Note: This is where to put cross-config validation checks. + """ + config = super().load_config(description, argv) + + if ( + config.registration.enable_registration + and not config.registration.enable_registration_without_verification + ): + if ( + not config.captcha.enable_registration_captcha + and not config.registration.registrations_require_3pid + and not config.registration.registration_requires_token + ): + raise ConfigError( + "You have enabled open registration without any verification. This is a known vector for " + "spam and abuse. If you would like to allow public registration, please consider adding email, " + "captcha, or token-based verification. Otherwise this check can be removed by setting the " + "`enable_registration_without_verification` config option to `true`." + ) + + return config diff --git a/tests/config/test_registration_config.py b/tests/config/test_registration_config.py index a8520c91d13..7d4e6642410 100644 --- a/tests/config/test_registration_config.py +++ b/tests/config/test_registration_config.py @@ -19,6 +19,8 @@ # # +import argparse + import synapse.app.homeserver from synapse.config import ConfigError from synapse.config.homeserver import HomeServerConfig @@ -116,3 +118,76 @@ def test_refuse_to_start_if_open_registration_and_no_verification(self) -> None: ["-c", self.config_file] ) synapse.app.homeserver.setup(homeserver_config) + + def test_load_config_error_if_open_registration_and_no_verification(self) -> None: + """ + Test that `HomeServerConfig.load_config(...)` raises an exception when we detect open + registration. + """ + self.generate_config() + self.add_lines_to_config( + [ + " ", + "enable_registration: true", + "registrations_require_3pid: []", + "enable_registration_captcha: false", + "registration_requires_token: false", + ] + ) + + # Test that allowing open registration without verification raises an error + with self.assertRaises(ConfigError): + _homeserver_config = HomeServerConfig.load_config( + description="test", argv=["-c", self.config_file] + ) + + def test_load_or_generate_config_error_if_open_registration_and_no_verification( + self, + ) -> None: + """ + Test that `HomeServerConfig.load_or_generate_config(...)` raises an exception when we detect open + registration. + """ + self.generate_config() + self.add_lines_to_config( + [ + " ", + "enable_registration: true", + "registrations_require_3pid: []", + "enable_registration_captcha: false", + "registration_requires_token: false", + ] + ) + + # Test that allowing open registration without verification raises an error + with self.assertRaises(ConfigError): + _homeserver_config = HomeServerConfig.load_or_generate_config( + description="test", argv=["-c", self.config_file] + ) + + def test_load_config_with_parser_error_if_open_registration_and_no_verification( + self, + ) -> None: + """ + Test that `HomeServerConfig.load_config_with_parser(...)` raises an exception when we detect open + registration. + """ + self.generate_config() + self.add_lines_to_config( + [ + " ", + "enable_registration: true", + "registrations_require_3pid: []", + "enable_registration_captcha: false", + "registration_requires_token: false", + ] + ) + + # Test that allowing open registration without verification raises an error + with self.assertRaises(ConfigError): + config_parser = argparse.ArgumentParser(description="test") + HomeServerConfig.add_arguments_to_parser(config_parser) + + _homeserver_config = HomeServerConfig.load_config_with_parser( + parser=config_parser, argv=["-c", self.config_file] + ) From 260f137ff870c50928caba83fa72fa4403d333f2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 8 Oct 2025 18:31:30 -0500 Subject: [PATCH 4/9] Fix tests --- synapse/config/_base.py | 12 ++++++++++-- synapse/config/_base.pyi | 6 +++--- synapse/config/homeserver.py | 17 ++++++++++------- tests/config/test_registration_config.py | 12 ++++++++---- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index d6f4b34f94d..ff56073740a 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -545,18 +545,22 @@ def generate_config( @classmethod def load_config( - cls: Type[TRootConfig], description: str, argv: List[str] + cls: Type[TRootConfig], description: str, argv_options: List[str] ) -> TRootConfig: """Parse the commandline and config files Doesn't support config-file-generation: used by the worker apps. + Args: + description: TODO + argv_options: The options passed to Synapse. Usually `sys.argv[1:]`. + Returns: Config object. """ config_parser = argparse.ArgumentParser(description=description) cls.add_arguments_to_parser(config_parser) - obj, _ = cls.load_config_with_parser(config_parser, argv) + obj, _ = cls.load_config_with_parser(config_parser, argv_options) return obj @@ -609,6 +613,10 @@ def load_config_with_parser( Used for workers where we want to add extra flags/subcommands. + Note: This is the common denominator for loading config and is also used by + `load_config` and `load_or_generate_config`. Which means it's the class to + override if you want to add additional config validation logic. + Args: parser argv_options: The options passed to Synapse. Usually `sys.argv[1:]`. diff --git a/synapse/config/_base.pyi b/synapse/config/_base.pyi index 5e036352062..924ea0d685e 100644 --- a/synapse/config/_base.pyi +++ b/synapse/config/_base.pyi @@ -156,11 +156,11 @@ class RootConfig: ) -> str: ... @classmethod def load_or_generate_config( - cls: Type[TRootConfig], description: str, argv: List[str] + cls: Type[TRootConfig], description: str, argv_options: List[str] ) -> Optional[TRootConfig]: ... @classmethod def load_config( - cls: Type[TRootConfig], description: str, argv: List[str] + cls: Type[TRootConfig], description: str, argv_options: List[str] ) -> TRootConfig: ... @classmethod def add_arguments_to_parser( @@ -168,7 +168,7 @@ class RootConfig: ) -> None: ... @classmethod def load_config_with_parser( - cls: Type[TRootConfig], parser: argparse.ArgumentParser, argv: List[str] + cls: Type[TRootConfig], parser: argparse.ArgumentParser, argv_options: List[str] ) -> Tuple[TRootConfig, argparse.Namespace]: ... def generate_missing_files( self, config_dict: dict, config_dir_path: str diff --git a/synapse/config/homeserver.py b/synapse/config/homeserver.py index 88a317cf74e..156f8b35dd3 100644 --- a/synapse/config/homeserver.py +++ b/synapse/config/homeserver.py @@ -18,7 +18,8 @@ # [This file includes modifications made by New Vector Limited] # # -from typing import List, Type, TypeVar +import argparse +from typing import List, Tuple, Type, TypeVar from ._base import ConfigError, RootConfig from .account_validity import AccountValidityConfig @@ -123,15 +124,17 @@ class HomeServerConfig(RootConfig): ] @classmethod - def load_config( - cls: Type[THomeServerConfig], description: str, argv: List[str] - ) -> THomeServerConfig: + def load_config_with_parser( + cls: Type[THomeServerConfig], + parser: argparse.ArgumentParser, + argv_options: List[str], + ) -> Tuple[THomeServerConfig, argparse.Namespace]: """ - See `RootConfig.load_config`. + See `RootConfig.load_config_with_parser`. Note: This is where to put cross-config validation checks. """ - config = super().load_config(description, argv) + config, config_args = super().load_config_with_parser(parser, argv_options) if ( config.registration.enable_registration @@ -149,4 +152,4 @@ def load_config( "`enable_registration_without_verification` config option to `true`." ) - return config + return config, config_args diff --git a/tests/config/test_registration_config.py b/tests/config/test_registration_config.py index 7d4e6642410..b7c808c8ecb 100644 --- a/tests/config/test_registration_config.py +++ b/tests/config/test_registration_config.py @@ -101,6 +101,10 @@ def test_session_lifetime_must_not_be_exceeded_by_smaller_lifetimes(self) -> Non ) def test_refuse_to_start_if_open_registration_and_no_verification(self) -> None: + """ + Test that our utilities to start the main Synapse homeserver process refuses + to start if we detect open registration. + """ self.generate_config() self.add_lines_to_config( [ @@ -113,7 +117,7 @@ def test_refuse_to_start_if_open_registration_and_no_verification(self) -> None: ) # Test that allowing open registration without verification raises an error - with self.assertRaises(ConfigError): + with self.assertRaises(SystemExit): homeserver_config = synapse.app.homeserver.load_or_generate_config( ["-c", self.config_file] ) @@ -138,7 +142,7 @@ def test_load_config_error_if_open_registration_and_no_verification(self) -> Non # Test that allowing open registration without verification raises an error with self.assertRaises(ConfigError): _homeserver_config = HomeServerConfig.load_config( - description="test", argv=["-c", self.config_file] + description="test", argv_options=["-c", self.config_file] ) def test_load_or_generate_config_error_if_open_registration_and_no_verification( @@ -162,7 +166,7 @@ def test_load_or_generate_config_error_if_open_registration_and_no_verification( # Test that allowing open registration without verification raises an error with self.assertRaises(ConfigError): _homeserver_config = HomeServerConfig.load_or_generate_config( - description="test", argv=["-c", self.config_file] + description="test", argv_options=["-c", self.config_file] ) def test_load_config_with_parser_error_if_open_registration_and_no_verification( @@ -189,5 +193,5 @@ def test_load_config_with_parser_error_if_open_registration_and_no_verification( HomeServerConfig.add_arguments_to_parser(config_parser) _homeserver_config = HomeServerConfig.load_config_with_parser( - parser=config_parser, argv=["-c", self.config_file] + parser=config_parser, argv_options=["-c", self.config_file] ) From 36325884cc7d81de7e85d5810fede566e95e1bc8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 8 Oct 2025 18:39:30 -0500 Subject: [PATCH 5/9] Add dedicated `validate_config()` pattern --- synapse/config/_base.py | 18 ++++++++++++++++-- synapse/config/homeserver.py | 32 ++++++++------------------------ 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index ff56073740a..b0f6c7f3746 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -614,8 +614,8 @@ def load_config_with_parser( Used for workers where we want to add extra flags/subcommands. Note: This is the common denominator for loading config and is also used by - `load_config` and `load_or_generate_config`. Which means it's the class to - override if you want to add additional config validation logic. + `load_config` and `load_or_generate_config`. Which is why we call + `validate_config()` here. Args: parser @@ -650,6 +650,10 @@ def load_config_with_parser( obj.invoke_all("read_arguments", config_args) + # Now that we finally have the full config sections parsed, allow subclasses to + # do some extra validation across the entire config. + obj.validate_config() + return obj, config_args @classmethod @@ -911,6 +915,16 @@ def reload_config_section(self, section_name: str) -> Config: existing_config.root = None return existing_config + def validate_config(self) -> None: + """ + Additional config validation across all config sections. + + Override this in subclasses to add extra validation. + + Raises: + ConfigError: if the config is invalid. + """ + def read_config_files(config_files: Iterable[str]) -> Dict[str, Any]: """Read the config files and shallowly merge them into a dict. diff --git a/synapse/config/homeserver.py b/synapse/config/homeserver.py index 156f8b35dd3..803b74ad626 100644 --- a/synapse/config/homeserver.py +++ b/synapse/config/homeserver.py @@ -18,8 +18,6 @@ # [This file includes modifications made by New Vector Limited] # # -import argparse -from typing import List, Tuple, Type, TypeVar from ._base import ConfigError, RootConfig from .account_validity import AccountValidityConfig @@ -67,8 +65,6 @@ from .voip import VoipConfig from .workers import WorkerConfig -THomeServerConfig = TypeVar("THomeServerConfig", bound="HomeServerConfig") - class HomeServerConfig(RootConfig): """ @@ -123,27 +119,17 @@ class HomeServerConfig(RootConfig): MasConfig, ] - @classmethod - def load_config_with_parser( - cls: Type[THomeServerConfig], - parser: argparse.ArgumentParser, - argv_options: List[str], - ) -> Tuple[THomeServerConfig, argparse.Namespace]: - """ - See `RootConfig.load_config_with_parser`. - - Note: This is where to put cross-config validation checks. - """ - config, config_args = super().load_config_with_parser(parser, argv_options) - + def validate_config( + self, + ) -> None: if ( - config.registration.enable_registration - and not config.registration.enable_registration_without_verification + self.registration.enable_registration + and not self.registration.enable_registration_without_verification ): if ( - not config.captcha.enable_registration_captcha - and not config.registration.registrations_require_3pid - and not config.registration.registration_requires_token + not self.captcha.enable_registration_captcha + and not self.registration.registrations_require_3pid + and not self.registration.registration_requires_token ): raise ConfigError( "You have enabled open registration without any verification. This is a known vector for " @@ -151,5 +137,3 @@ def load_config_with_parser( "captcha, or token-based verification. Otherwise this check can be removed by setting the " "`enable_registration_without_verification` config option to `true`." ) - - return config, config_args From 92fd2250b6e54d8aec777eef0849e89255b95e06 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 8 Oct 2025 18:56:19 -0500 Subject: [PATCH 6/9] Update changelog --- changelog.d/19027.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/19027.misc b/changelog.d/19027.misc index 46fdab91e4f..727f3ee5ffc 100644 --- a/changelog.d/19027.misc +++ b/changelog.d/19027.misc @@ -1 +1 @@ -Move open registration config validation from `setup(...)` to `load_or_generate_config(...)`. +Introduce `RootConfig.validate_config()` which can be subclassed in `HomeServerConfig` to do cross-config class validation. From f39fd82b77accef35388402ba1936df17a1fcb1e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 8 Oct 2025 19:00:32 -0500 Subject: [PATCH 7/9] Ignore open registration in test --- tests/config/test_load.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/config/test_load.py b/tests/config/test_load.py index b72365b6e3a..c1b787346ef 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -99,7 +99,14 @@ def test_load_succeeds_if_macaroon_secret_key_missing(self) -> None: def test_disable_registration(self) -> None: self.generate_config() self.add_lines_to_config( - ["enable_registration: true", "disable_registration: true"] + [ + "enable_registration: true", + "disable_registration: true", + # We're not worried about open registration in this test. This test is + # focused on making sure that enable/disable_registration properly + # override each other. + "enable_registration_without_verification: true", + ] ) # Check that disable_registration clobbers enable_registration. config = HomeServerConfig.load_config("", ["-c", self.config_file]) From 3faa7ea5b830687daa86e7f66492058945249033 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 9 Oct 2025 12:20:34 -0500 Subject: [PATCH 8/9] Partial clarify no extra changes should be made Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- synapse/config/_base.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index b0f6c7f3746..d78c9a8f74c 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -919,7 +919,8 @@ def validate_config(self) -> None: """ Additional config validation across all config sections. - Override this in subclasses to add extra validation. + Override this in subclasses to add extra validation once all + config option values have been populated. Raises: ConfigError: if the config is invalid. From 5df163b80b005e13bc50bd8dacff1c5b904f7570 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 9 Oct 2025 12:25:43 -0500 Subject: [PATCH 9/9] Better clarify no further modifications should be made See https://github.com/element-hq/synapse/pull/19027#pullrequestreview-3317956338 --- synapse/config/_base.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index d78c9a8f74c..5d0560e0f2e 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -919,9 +919,12 @@ def validate_config(self) -> None: """ Additional config validation across all config sections. - Override this in subclasses to add extra validation once all + Override this in subclasses to add extra validation. This is called once all config option values have been populated. + XXX: This should only validate, not modify the configuration, as the final + config state is required for proper validation across all config sections. + Raises: ConfigError: if the config is invalid. """