diff --git a/app/app.py b/app/app.py index 4273b41..a3fb45c 100644 --- a/app/app.py +++ b/app/app.py @@ -181,6 +181,23 @@ def create_app(config=None): app.config['AUTO_CREATE_TABLES'] = os.getenv('AUTO_CREATE_TABLES', 'true').lower() == 'true' app.config['ENABLE_DISCORD_BOT'] = os.getenv('ENABLE_DISCORD_BOT', 'true').lower() == 'true' + # Where the rate limiter keeps its counters (SEC-WEB-004). + # + # `memory://` is what Flask-Limiter falls back to when nothing is set, + # and it is the correct choice here: Waitress serves this application + # from one process, so one set of counters in that process is all there + # is to share. Naming it changes nothing at runtime and two things + # otherwise — it stops being an accident, and it becomes settable to a + # Redis URI on the day the deployment gains a second process, which is + # the day in-memory counters would start letting through N times the + # configured limit without anyone noticing. + # + # What this does not fix: the counters are keyed on an IP address that + # is forgeable while TRUSTED_PROXY is unresolved (OPS-002). Shared + # storage for a forgeable key buys nothing, which is why that one is + # the prerequisite and not this. + app.config['RATELIMIT_STORAGE_URI'] = os.getenv('RATELIMIT_STORAGE_URI', 'memory://') + # --- caller overrides win --------------------------------------------- if config: app.config.update(config) @@ -239,6 +256,22 @@ def create_app(config=None): login_manager.init_app(app) csrf.init_app(app) limiter.init_app(app) + + # Said once, at startup, because the failure mode is silent: counters in + # process memory are lost on every restart and are not shared, so a + # second worker would double every limit and nothing would report it. + if app.config['RATELIMIT_STORAGE_URI'].startswith('memory://'): + app.logger.info( + 'Rate limiting counters are held in process memory. Correct for a ' + 'single-process deployment; set RATELIMIT_STORAGE_URI to a shared ' + 'backend before running more than one worker (SEC-WEB-004).' + ) + else: + app.logger.info( + 'Rate limiting counters are held in a shared backend (%s).', + app.config['RATELIMIT_STORAGE_URI'].split('://', 1)[0], + ) + babel.init_app(app, locale_selector=i18n.select_locale) # Exposed to every template so the language switcher can render itself diff --git a/app/extensions.py b/app/extensions.py index c6ad0e1..b141de9 100644 --- a/app/extensions.py +++ b/app/extensions.py @@ -17,7 +17,11 @@ csrf = CSRFProtect() # stays available. See app/i18n.py for how a locale is chosen. babel = Babel() -# Rate limiter for brute-force protection +# Rate limiter for brute-force protection. +# +# No storage is named here on purpose: it comes from RATELIMIT_STORAGE_URI in +# app.config, which create_app fills from the environment and defaults to +# `memory://` (SEC-WEB-004). Naming it in both places is how the two drift. limiter = Limiter(key_func=get_remote_address, default_limits=["200 per day", "50 per hour"]) diff --git a/tests/test_security_headers.py b/tests/test_security_headers.py index 1deb61c..9cd2321 100644 --- a/tests/test_security_headers.py +++ b/tests/test_security_headers.py @@ -75,3 +75,41 @@ class TestTemplateEscaping: with app.app_context(): assert nl2br('') == '' assert nl2br(None) == '' + + +class TestRateLimitStorage: + """SEC-WEB-004 — the counters were in process memory by accident. + + Flask-Limiter falls back to `memory://` when nothing names a storage, so + the choice was never made: it was simply what happened. For one Waitress + process that answer is correct, which is exactly why it needed writing + down — a second worker would double every limit, silently, and the + configuration would look untouched. + """ + + def test_the_storage_is_named_rather_than_defaulted(self, app): + assert app.config['RATELIMIT_STORAGE_URI'] == 'memory://' + + def test_the_environment_can_move_it(self, monkeypatch, tmp_path): + """The point of naming it: the day the deployment gains a second + process, this is a one-line change and not a code change.""" + from app.app import create_app + + monkeypatch.setenv('RATELIMIT_STORAGE_URI', 'redis://cache.example.test:6379') + + application = create_app( + { + 'SECRET_KEY': 'test-secret-not-used-anywhere-real', + 'SQLALCHEMY_DATABASE_URI': f'sqlite:///{tmp_path / "t.sqlite"}', + 'TESTING': True, + 'WTF_CSRF_ENABLED': False, + 'FORCE_HTTPS': False, + 'SESSION_COOKIE_SECURE': False, + 'ENABLE_DISCORD_BOT': False, + 'AUTO_CREATE_TABLES': False, + 'CORS_ALLOWED_ORIGINS': '', + 'RATELIMIT_ENABLED': False, + } + ) + + assert application.config['RATELIMIT_STORAGE_URI'] == 'redis://cache.example.test:6379'