Aug 21 - Fixed Forwarded host

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)

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;
+}

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;
+}

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)
+    )