From 8f5e36a0793e733ddaa14015a4844464251dcc62 Mon Sep 17 00:00:00 2001
From: Ahmed Dlshad <ahmed.dlshad.m@gmail.com>
Date: Tue, 23 Jun 2026 17:12:32 +0300
Subject: [PATCH] fix(routes): log and cleanly 500 on unreadable HTML page
 (#4637)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(routes): serve 404 instead of 500 when an HTML page file is missing

_serve_html_with_nonce opened the HTML file with no error handling, and
callers such as /backgrounds and /login pass their paths in with no
existence check, so a missing or unreadable file raised an unhandled
OSError that surfaced as a 500. Wrap the read and raise HTTPException(404)
instead; the normal render path (CSP-nonce substitution) is unchanged.

Fixes #4594

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(routes): distinguish missing page (404) from read failure (500)

The previous fix caught a broad OSError and returned 404 for every
failure, which masks real server-side problems (permission errors, I/O
failures) as "not found" and lets them slip past error alerting. Split
FileNotFoundError (genuine 404) from other OSError, which now logs the
exception and returns a generic 500 — without leaking the OS error
string or file path into the response body.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(routes): treat unreadable bundled HTML page as logged 500, not 404

Per PR #4637 review: every caller of the page-render helper serves a fixed,
server-owned template (index/login/backgrounds), never a client-supplied
path. So a missing or unreadable file is a server fault (broken deployment),
not a client "not found" — a 404 there mislabels a server error and hides a
missing core template from 5xx alerting, contradicting the OSError->500
rationale this PR is built on. Collapse both branches into a single logged,
leak-free 500.

Move the helper to src.app_helpers.serve_html_with_nonce so the behavior can
be unit-tested without importing the whole app (app.py is the slim
orchestrator; the test harness stubs src.database, so importing app in tests
is not viable). Add tests pinning missing/unreadable -> 500 (not 404) and
nonce injection on the happy path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
---
 app.py                              | 20 ++++-------
 src/app_helpers.py                  | 31 ++++++++++++++++-
 tests/test_serve_html_with_nonce.py | 52 +++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 15 deletions(-)
 create mode 100644 tests/test_serve_html_with_nonce.py

diff --git a/app.py b/app.py
index 57d091efd..07228c482 100644
--- a/app.py
+++ b/app.py
@@ -44,7 +44,7 @@ from typing import Dict
 
 from contextlib import asynccontextmanager
 from fastapi import FastAPI, Request, HTTPException
-from fastapi.responses import JSONResponse, FileResponse, HTMLResponse
+from fastapi.responses import JSONResponse, FileResponse
 from fastapi.middleware.cors import CORSMiddleware
 from fastapi.staticfiles import StaticFiles
 from starlette.middleware.base import BaseHTTPMiddleware
@@ -65,7 +65,7 @@ from core.exceptions import (
 
 import bcrypt as _bcrypt
 
-from src.app_helpers import abs_join
+from src.app_helpers import abs_join, serve_html_with_nonce
 from src.generated_images import GENERATED_IMAGE_HEADERS, resolve_generated_image_path
 from starlette.responses import RedirectResponse
 
@@ -791,22 +791,14 @@ app.include_router(setup_companion_routes())
 
 # ========= ROUTES (kept in app.py) =========
 
-def _serve_html_with_nonce(request: Request, file_path: str) -> HTMLResponse:
-    """Read an HTML file and inject the CSP nonce into inline <script> tags."""
-    with open(file_path, "r", encoding="utf-8") as f:
-        html = f.read()
-    nonce = getattr(request.state, "csp_nonce", "")
-    html = html.replace("{{CSP_NONCE}}", nonce)
-    return HTMLResponse(html)
-
 @app.get("/")
 async def serve_index(request: Request):
     static_path = abs_join(BASE_DIR, "static/index.html")
     if os.path.exists(static_path):
-        return _serve_html_with_nonce(request, static_path)
+        return serve_html_with_nonce(request, static_path)
     root_path = abs_join(BASE_DIR, "index.html")
     if os.path.exists(root_path):
-        return _serve_html_with_nonce(request, root_path)
+        return serve_html_with_nonce(request, root_path)
     raise HTTPException(404, "index.html not found")
 
 @app.get("/notes")
@@ -848,13 +840,13 @@ async def serve_library(request: Request):
 @app.get("/backgrounds")
 async def serve_backgrounds(request: Request):
     """Sandbox page for prototyping background effects. No auth required."""
-    return _serve_html_with_nonce(request, abs_join(BASE_DIR, "static/backgrounds.html"))
+    return serve_html_with_nonce(request, abs_join(BASE_DIR, "static/backgrounds.html"))
 
 @app.get("/login")
 async def serve_login(request: Request):
     if not AUTH_ENABLED:
         return RedirectResponse(url="/", status_code=302)
-    return _serve_html_with_nonce(request, abs_join(BASE_DIR, "static/login.html"))
+    return serve_html_with_nonce(request, abs_join(BASE_DIR, "static/login.html"))
 
 @app.get("/api/version")
 async def get_version():
diff --git a/src/app_helpers.py b/src/app_helpers.py
index 8570820d8..1b915a5a2 100644
--- a/src/app_helpers.py
+++ b/src/app_helpers.py
@@ -1,6 +1,13 @@
 # src/app_helpers.py
-import os
 import base64
+import logging
+import os
+
+from fastapi import HTTPException
+from fastapi.responses import HTMLResponse
+from starlette.requests import Request
+
+logger = logging.getLogger(__name__)
 
 def read_if_exists(path: str) -> str:
     """Read file if it exists, return empty string otherwise."""
@@ -20,6 +27,28 @@ def abs_join(base_dir: str, rel: str) -> str:
     """Join paths and return absolute path."""
     return os.path.abspath(os.path.join(base_dir, rel))
 
+def serve_html_with_nonce(request: Request, file_path: str) -> HTMLResponse:
+    """Read an app-bundled HTML page and inject the CSP nonce into inline <script> tags.
+
+    Callers pass fixed, server-owned template paths (index/login/backgrounds),
+    never a client-supplied path. So any read failure here — a missing file
+    (broken deployment) or a permission/IO error — is a server fault, not a
+    client "not found": map all of them to a logged 500 so a missing core
+    template surfaces in 5xx alerting instead of hiding behind a 404. If a
+    future caller serves a client-influenced path where 404 is correct, branch
+    that at the call site rather than defaulting this shared helper to 404.
+    """
+    try:
+        with open(file_path, "r", encoding="utf-8") as f:
+            html = f.read()
+    except OSError:
+        logger.exception("Failed to read page %s", file_path)
+        raise HTTPException(500, "Internal server error")
+    nonce = getattr(request.state, "csp_nonce", "")
+    html = html.replace("{{CSP_NONCE}}", nonce)
+    return HTMLResponse(html)
+
+
 def inside_base_dir(base_dir: str, path: str) -> bool:
     """Check if path is inside base directory."""
     if not isinstance(base_dir, str) or not isinstance(path, str):
diff --git a/tests/test_serve_html_with_nonce.py b/tests/test_serve_html_with_nonce.py
new file mode 100644
index 000000000..5ec42acdd
--- /dev/null
+++ b/tests/test_serve_html_with_nonce.py
@@ -0,0 +1,52 @@
+"""Behavior tests for src.app_helpers.serve_html_with_nonce.
+
+Every caller of this helper serves a fixed, app-bundled template
+(index/login/backgrounds), never a client-supplied path. So a read failure —
+a missing file (broken deployment) or a permission/IO error — is a server
+fault, not a client "not found", and must surface as a logged 500 rather than
+hiding behind a 404 where 5xx alerting can't see it. These tests lock that
+intent (raised in the PR #4637 review).
+"""
+import types
+
+import pytest
+
+pytest.importorskip("fastapi")
+pytest.importorskip("starlette.responses")
+from fastapi import HTTPException
+
+from src.app_helpers import serve_html_with_nonce
+
+
+def _request_with_nonce(nonce: str = ""):
+    """Minimal stand-in for a Starlette Request: only request.state.csp_nonce is read."""
+    return types.SimpleNamespace(state=types.SimpleNamespace(csp_nonce=nonce))
+
+
+def test_missing_fixed_template_returns_500_not_404(tmp_path):
+    missing = tmp_path / "does_not_exist.html"
+    with pytest.raises(HTTPException) as exc_info:
+        serve_html_with_nonce(_request_with_nonce(), str(missing))
+    assert exc_info.value.status_code == 500
+    # Generic detail — no OS error string or absolute path leaked to the client.
+    assert exc_info.value.detail == "Internal server error"
+
+
+def test_unreadable_template_returns_500(tmp_path):
+    # A directory at the path makes open() raise an OSError subtype
+    # (IsADirectoryError on POSIX, PermissionError on Windows) — same branch.
+    a_dir = tmp_path / "a_dir.html"
+    a_dir.mkdir()
+    with pytest.raises(HTTPException) as exc_info:
+        serve_html_with_nonce(_request_with_nonce(), str(a_dir))
+    assert exc_info.value.status_code == 500
+
+
+def test_readable_template_injects_nonce(tmp_path):
+    page = tmp_path / "page.html"
+    page.write_text('<script nonce="{{CSP_NONCE}}">x</script>', encoding="utf-8")
+    resp = serve_html_with_nonce(_request_with_nonce("nonce-abc"), str(page))
+    assert resp.status_code == 200
+    body = resp.body.decode("utf-8")
+    assert "nonce-abc" in body
+    assert "{{CSP_NONCE}}" not in body