From 3bfd81c84ce9dd1eecc75a10c2cc868f3032b766 Mon Sep 17 00:00:00 2001 From: NguyenND Date: Fri, 21 Aug 2026 13:14:37 -0400 Subject: [PATCH] Aug 21 - Fixed Forwarded host --- app/__init__.py | 11 +++- deploy/nginx/jqc_lts.conf | 71 ++++++++++++++++++++++++ deploy/nginx/jqc_multi.conf | 72 ++++++++++++++++++++++++ tests/test_forwarded_host.py | 104 +++++++++++++++++++++++++++++++++++ 4 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 deploy/nginx/jqc_lts.conf create mode 100644 deploy/nginx/jqc_multi.conf create mode 100644 tests/test_forwarded_host.py diff --git a/app/__init__.py b/app/__init__.py index f0db58c..255f005 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -85,8 +85,17 @@ def create_app(config_name='default'): # Unwrap X-Forwarded-For / X-Forwarded-Proto set by Nginx so Flask sees # the real client IP (needed for rate limiting and fail2ban logging) and # the real scheme (needed for HTTPS URL generation in emails). + # + # MT-24: x_host is deliberately 0. With x_host=1, `request.host` was taken + # from the X-Forwarded-Host header — and nginx forwards unrecognised client + # headers upstream, so any client could supply that header and choose which + # tenant database the request bound to. Nginx already sets `Host $host` + # from the real SNI/Host, so HTTP_HOST is the trustworthy source and + # X-Forwarded-Host adds nothing but an attacker-controlled input. + # The nginx configs also pin X-Forwarded-Host explicitly (defence in depth); + # neither layer alone is relied upon. See deploy/nginx/README.md. from werkzeug.middleware.proxy_fix import ProxyFix - app.wsgi_app = ProxyFix(app.wsgi_app, x_for=1, x_proto=1, x_host=1) + app.wsgi_app = ProxyFix(app.wsgi_app, x_for=1, x_proto=1, x_host=0) db.init_app(app) login_manager.init_app(app) diff --git a/deploy/nginx/jqc_lts.conf b/deploy/nginx/jqc_lts.conf new file mode 100644 index 0000000..0095151 --- /dev/null +++ b/deploy/nginx/jqc_lts.conf @@ -0,0 +1,71 @@ +# /etc/nginx/sites-available/jqc_lts +# ───────────────────────────────────────────────────────────────────────────── +# Tenant-zero (LT Services). An exact server_name beats the *.jqc.app wildcard, +# so this block wins for lts.jqc.app. +# +# Same MT-24 requirement as jqc_multi.conf: X-Forwarded-Host must be pinned. +# ───────────────────────────────────────────────────────────────────────────── + +server { + listen 443 ssl; + http2 on; + server_name lts.jqc.app; + + client_max_body_size 50M; + client_body_timeout 120s; + proxy_connect_timeout 120s; + proxy_send_timeout 120s; + proxy_read_timeout 120s; + + location / { + proxy_pass http://127.0.0.1:8000; + + proxy_set_header Host $host; + # MT-24 — pin, never inherit from the client. + proxy_set_header X-Forwarded-Host $host; + proxy_set_header X-Forwarded-Port $server_port; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + + proxy_request_buffering on; + proxy_buffer_size 16k; + proxy_buffers 8 32k; + proxy_busy_buffers_size 64k; + proxy_redirect off; + } + + location /static { + alias /home/jqc/janitorial_qc/app/static; + expires 7d; + add_header Cache-Control "public, immutable"; + access_log off; + } + + # MT-24: the /uploads alias is REMOVED. + # + # It published app/static/uploads at a second, shorter public path. Nothing + # in the application generates /uploads/... URLs — LocalBackend builds + # /static/uploads/... via url_for('static'), and the S3 backend issues + # presigned URLs — so this route served no traffic the app depends on while + # widening the unauthenticated surface for tenant photographs. + # + # Restore it ONLY if access logs show real traffic on /uploads/ from an + # older client build: + # grep -c ' /uploads/' /home/jqc/logs/janitorial-qc-access.log + # Check before removing, not after. + + access_log /home/jqc/logs/janitorial-qc-access.log; + error_log /home/jqc/logs/janitorial-qc-error.log; + + ssl_certificate /etc/letsencrypt/live/lts.jqc.app/fullchain.pem; + ssl_certificate_key /etc/letsencrypt/live/lts.jqc.app/privkey.pem; + include /etc/letsencrypt/options-ssl-nginx.conf; + ssl_dhparam /etc/letsencrypt/ssl-dhparams.pem; +} + +server { + listen 80; + server_name lts.jqc.app; + return 301 https://$host$request_uri; +} diff --git a/deploy/nginx/jqc_multi.conf b/deploy/nginx/jqc_multi.conf new file mode 100644 index 0000000..987e781 --- /dev/null +++ b/deploy/nginx/jqc_multi.conf @@ -0,0 +1,72 @@ +# /etc/nginx/sites-available/jqc_multi +# ───────────────────────────────────────────────────────────────────────────── +# Tenant subdomains: *.jqc.app → 127.0.0.1:8000 +# +# MT-24. The critical line in this file is: +# +# proxy_set_header X-Forwarded-Host $host; +# +# Nginx forwards unrecognised client request headers upstream. Without this +# line a client could send its own X-Forwarded-Host, and (when the app trusted +# it) choose which tenant database the request bound to — pre-authentication, +# from the open internet. Pinning it to $host makes the header say what nginx +# observed, never what the client claimed. The app no longer trusts the header +# at all (ProxyFix x_host=0), so this is the second of two independent layers. +# +# Keep both. Either alone closes the hole; both together mean a future change +# to one does not silently reopen it. +# ───────────────────────────────────────────────────────────────────────────── + +# ── Shared proxy header set ────────────────────────────────────────────────── +# Included by every location that proxies, so a new location cannot forget one. +# (Requires: include snippets in /etc/nginx/snippets/ or inline as below.) + +server { + listen 443 ssl; + http2 on; + server_name *.jqc.app; + + client_max_body_size 50M; + + # Static assets. NOTE: this serves the app's static tree directly, which + # includes app/static/uploads on a local-storage deploy — every photo is + # then readable by anyone who knows or guesses its URL, with no session + # check. Filenames are random UUIDs so they are not enumerable, but they + # are also not access-controlled and never expire. + # This is retired per tenant at R2 cutover, when media moves to short-lived + # presigned URLs. Until then, treat it as a known exposure. + location /static/ { + alias /home/jqc/janitorial_qc/app/static/; + expires 30d; + } + + location / { + proxy_pass http://127.0.0.1:8000; + + proxy_set_header Host $host; + # MT-24 — pin, never inherit from the client. See header comment. + proxy_set_header X-Forwarded-Host $host; + proxy_set_header X-Forwarded-Port $server_port; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + + proxy_read_timeout 60s; + } + + ssl_certificate /etc/letsencrypt/live/jqc.app-0001/fullchain.pem; + ssl_certificate_key /etc/letsencrypt/live/jqc.app-0001/privkey.pem; + include /etc/letsencrypt/options-ssl-nginx.conf; + ssl_dhparam /etc/letsencrypt/ssl-dhparams.pem; +} + +server { + listen 80; + server_name *.jqc.app; + + # MT-24: the Certbot-generated `if ($host = *.jqc.app)` test is a literal + # string comparison — it never matches a wildcard server_name, so plain + # HTTP requests to tenant subdomains fell through with no redirect. This + # block matches only *.jqc.app already, so redirect unconditionally. + return 301 https://$host$request_uri; +} diff --git a/tests/test_forwarded_host.py b/tests/test_forwarded_host.py new file mode 100644 index 0000000..ba0f416 --- /dev/null +++ b/tests/test_forwarded_host.py @@ -0,0 +1,104 @@ +import pytest + +from werkzeug.test import EnvironBuilder + + +def _drive(app, headers): + """Send one request through the app's REAL ProxyFix instance. + + The middleware's wrapped application is swapped for a recorder for the + duration of the call, so what is measured is the exact ProxyFix object + create_app() installed — not a reconstruction of it — without needing a + second create_app() (blueprints cannot be registered twice per process) + or a route added after the app has served its first request. + """ + seen = {} + + def recorder(environ, start_response): + seen['host'] = environ.get('HTTP_HOST') + seen['scheme'] = environ.get('wsgi.url_scheme') + start_response('200 OK', [('Content-Type', 'text/plain')]) + return [b'ok'] + + middleware = app.wsgi_app + original = middleware.app + middleware.app = recorder + try: + environ = EnvironBuilder(path='/', headers=headers).get_environ() + middleware(environ, lambda *a, **kw: None) + finally: + middleware.app = original + return seen + + +def _observed_host(app, headers): + """Return the host the application resolves for a request.""" + return _drive(app, headers).get('host') + + +def test_forwarded_host_does_not_override_the_real_host(app): + """The core assertion: a forged header must be ignored.""" + host = _observed_host(app, { + 'Host': 'attacker.jqc.app', + 'X-Forwarded-Host': 'victim.jqc.app', + }) + assert host == 'attacker.jqc.app', ( + 'X-Forwarded-Host overrode the real Host header — tenant selection is ' + 'attacker-controlled. Check ProxyFix x_host in app/__init__.py.' + ) + assert 'victim' not in (host or '') + + +def test_real_host_is_still_used_normally(app): + """Without the forged header, nothing changes.""" + assert _observed_host(app, {'Host': 'lts.jqc.app'}) == 'lts.jqc.app' + + +def test_proxyfix_is_configured_without_x_host(app): + """Pin the setting itself, so a future edit has to be deliberate.""" + from werkzeug.middleware.proxy_fix import ProxyFix + + wsgi = app.wsgi_app + assert isinstance(wsgi, ProxyFix), 'ProxyFix is no longer installed' + assert wsgi.x_host == 0, ( + 'ProxyFix x_host must stay 0 — nginx sets Host from the real request, ' + 'and X-Forwarded-Host is client-controlled.' + ) + # The two we DO want unwrapped, for rate limiting and HTTPS URL building. + assert wsgi.x_for == 1 + assert wsgi.x_proto == 1 + + +def test_forwarded_proto_is_still_honoured(app): + """x_proto must keep working — email links depend on it.""" + seen = _drive(app, {'Host': 'lts.jqc.app', 'X-Forwarded-Proto': 'https'}) + assert seen.get('scheme') == 'https' + + +# ── Config guard ───────────────────────────────────────────────────────────── + +def test_shipped_nginx_configs_pin_forwarded_host(): + """Every proxying block in deploy/nginx must pin X-Forwarded-Host. + + These files are the reference the server is deployed from. A block that + proxies without pinning the header reopens the hole on whichever host uses + it, so the omission should fail here rather than on the box. + """ + import pathlib + + conf_dir = pathlib.Path(__file__).resolve().parent.parent / 'deploy' / 'nginx' + configs = sorted(conf_dir.glob('*.conf')) + assert configs, 'no nginx reference configs found in deploy/nginx/' + + offenders = [] + for path in configs: + text = path.read_text(encoding='utf-8') + if 'proxy_pass' not in text: + continue + if 'proxy_set_header X-Forwarded-Host' not in text.replace(' ', ' '): + offenders.append(path.name) + + assert not offenders, ( + 'nginx config proxies without pinning X-Forwarded-Host: ' + + ', '.join(offenders) + )