Apply independent review findings and harden auth/CORS edges

apis/devices.py

@@ -4,12 +4,22 @@
 from models import Device, DeviceRegistration
 from services import DeviceService
 
+# Registration is called by the iOS app itself and stays unauthenticated;
+# everything that exposes or destroys device data goes on protected_router,
+# so new routes are authenticated unless deliberately placed here.
 router = APIRouter(
     prefix="/devices",
     tags=["devices"],
     responses={404: {"description": "Not found"}},
 )
 
+protected_router = APIRouter(
+    prefix="/devices",
+    tags=["devices"],
+    dependencies=[Depends(require_api_key)],
+    responses={404: {"description": "Not found"}},
+)
+
 
 @router.post("/register", response_model=Device)
 def register_device(registration: DeviceRegistration, device_service: DeviceService = Depends()):
@@ -26,7 +36,7 @@ def register_device(registration: DeviceRegistration, device_service: DeviceServ
     return device_service.register_device(registration)
 
 
-@router.get("/all", response_model=list[Device], dependencies=[Depends(require_api_key)])
+@protected_router.get("/all", response_model=list[Device])
 def get_registered_devices(
     device_service: DeviceService = Depends(),
 ):
@@ -42,7 +52,7 @@ def get_registered_devices(
     return device_service.get_registered_devices()
 
 
-@router.delete("", response_model=None, dependencies=[Depends(require_api_key)])
+@protected_router.delete("", response_model=None)
 def clear_registered_devices(
     device_service: DeviceService = Depends(),
 ):

auth.py

@@ -24,7 +24,7 @@ def require_api_key(
         HTTPException: 401 if the header is missing or the key does not match.
     """
     if credentials is None or not secrets.compare_digest(
-        credentials.credentials.encode(), getenv("API_KEY").encode()
+        credentials.credentials.encode(), getenv("API_KEY", "").encode()
     ):
         raise HTTPException(
             status_code=status.HTTP_401_UNAUTHORIZED,

main.py

@@ -3,7 +3,7 @@
 It configures middleware, adds sub-routers, and defines application-level health checks.
 """
 
-import os
+import logging
 from contextlib import asynccontextmanager
 
 import uvicorn
@@ -16,14 +16,22 @@
 from push import shutdown_push_handler
 from utils import getenv
 
+logger = logging.getLogger(__name__)
+
 
 @asynccontextmanager
 async def lifespan(app: FastAPI):
-    if not os.getenv("API_KEY"):
+    api_key = getenv("API_KEY", "")
+    if not api_key:
         raise RuntimeError(
             "API_KEY environment variable must be set; protected endpoints "
             "require clients to send it as 'Authorization: Bearer <API_KEY>'"
         )
+    if api_key == "CHANGE_ME":
+        logger.warning(
+            "API_KEY is still the CHANGE_ME placeholder from .env.template; "
+            "set a real secret before exposing this server"
+        )
     EntityBase.metadata.create_all(database.engine)
     yield
     shutdown_push_handler()
@@ -42,6 +50,11 @@ def create_app() -> FastAPI:
     cors_origins = [
         origin.strip() for origin in getenv("CORS_ORIGINS", "").split(",") if origin.strip()
     ]
+    if "*" in cors_origins:
+        raise ValueError(
+            "CORS_ORIGINS must list explicit origins; a wildcard combined "
+            "with credentials would let any website make authenticated requests"
+        )
     if cors_origins:
         app.add_middleware(
             CORSMiddleware,
@@ -51,7 +64,7 @@ def create_app() -> FastAPI:
             allow_headers=["*"],
         )
 
-    routers: list[APIRouter] = [devices.router, push.router]
+    routers: list[APIRouter] = [devices.router, devices.protected_router, push.router]
     for router in routers:
         app.include_router(router)
 

tests/conftest.py

@@ -49,18 +49,43 @@ def test_engine():
     engine.dispose()
 
 
-@pytest.fixture
-def client(test_engine, monkeypatch):
-    """Authenticated TestClient wired to the in-memory database."""
+PROTECTED_ROUTES = [
+    ("POST", "/push/send"),
+    ("GET", "/devices/all"),
+    ("DELETE", "/devices"),
+]
+
+
+@pytest.fixture(params=PROTECTED_ROUTES, ids=lambda route: f"{route[0]} {route[1]}")
+def protected_route(request):
+    """Each (method, path) pair that must require the API key."""
+    return request.param
+
+
+def _build_client(test_engine, monkeypatch, headers=None):
     monkeypatch.setattr(database, "engine", test_engine)
 
     def override_db_session():
         with Session(test_engine) as session:
             yield session
 
     app.dependency_overrides[db_session] = override_db_session
+    return TestClient(app, headers=headers)
+
+
+@pytest.fixture
+def client(test_engine, monkeypatch):
+    """Authenticated TestClient wired to the in-memory database."""
     headers = {"Authorization": f"Bearer {os.environ['API_KEY']}"}
-    with TestClient(app, headers=headers) as test_client:
+    with _build_client(test_engine, monkeypatch, headers) as test_client:
+        yield test_client
+    app.dependency_overrides.clear()
+
+
+@pytest.fixture
+def anon_client(test_engine, monkeypatch):
+    """TestClient that sends no Authorization header."""
+    with _build_client(test_engine, monkeypatch) as test_client:
         yield test_client
     app.dependency_overrides.clear()
 

tests/integration/test_server.py

@@ -70,46 +70,50 @@ def _free_port() -> int:
 
 
 @pytest.fixture(scope="module")
-def server():
+def server(tmp_path_factory):
     """The real application booted as a subprocess against real Postgres."""
     env = _integration_env()
     _drop_devices_table(env)
     port = _free_port()
     env["PORT"] = str(port)
 
-    # Launched exactly as the README documents, so the entrypoint itself
-    # (including HOST/PORT handling) is part of what these tests cover.
-    process = subprocess.Popen(
-        [sys.executable, "main.py"],
-        cwd=REPO_ROOT,
-        env=env,
-        stdout=subprocess.PIPE,
-        stderr=subprocess.STDOUT,
-        text=True,
-    )
-    base_url = f"http://127.0.0.1:{port}"
-    try:
-        deadline = time.monotonic() + 20
-        while time.monotonic() < deadline:
-            if process.poll() is not None:
-                pytest.fail(f"server exited during startup:\n{process.stdout.read()}")
-            try:
-                if httpx.get(f"{base_url}/health", timeout=1).status_code == 200:
-                    break
-            except httpx.TransportError:
+    # Server output goes to a file, not a pipe: an undrained pipe blocks the
+    # server once its buffer fills, which would hang the suite as it grows.
+    log_path = tmp_path_factory.mktemp("server") / "server.log"
+    with open(log_path, "w") as log_file:
+        # Launched exactly as the README documents, so the entrypoint itself
+        # (including HOST/PORT handling) is part of what these tests cover.
+        process = subprocess.Popen(
+            [sys.executable, "main.py"],
+            cwd=REPO_ROOT,
+            env=env,
+            stdout=log_file,
+            stderr=subprocess.STDOUT,
+        )
+        base_url = f"http://127.0.0.1:{port}"
+        try:
+            deadline = time.monotonic() + 20
+            while time.monotonic() < deadline:
+                if process.poll() is not None:
+                    pytest.fail(f"server exited during startup:\n{log_path.read_text()}")
+                try:
+                    if httpx.get(f"{base_url}/health", timeout=1).status_code == 200:
+                        break
+                except httpx.TransportError:
+                    pass
                 time.sleep(0.2)
-        else:
-            pytest.fail("server did not become healthy within 20s")
+            else:
+                pytest.fail("server did not become healthy within 20s")
 
-        yield base_url
-    finally:
-        process.send_signal(signal.SIGINT)
-        try:
-            process.wait(timeout=15)
-        except subprocess.TimeoutExpired:
-            process.kill()
-            pytest.fail("server did not shut down gracefully within 15s")
-    assert process.returncode == 0, f"unclean shutdown:\n{process.stdout.read()}"
+            yield base_url
+        finally:
+            process.send_signal(signal.SIGINT)
+            try:
+                process.wait(timeout=15)
+            except subprocess.TimeoutExpired:
+                process.kill()
+                pytest.fail("server did not shut down gracefully within 15s")
+    assert process.returncode == 0, f"unclean shutdown:\n{log_path.read_text()}"
 
 
 @pytest.fixture
@@ -160,15 +164,9 @@ def test_registration_rejects_missing_token(anon_api):
     assert anon_api.post("/devices/register", json={"name": "no token"}).status_code == 422
 
 
-@pytest.mark.parametrize(
-    ("method", "path"),
-    [
-        ("GET", "/devices/all"),
-        ("DELETE", "/devices"),
-        ("POST", "/push/send"),
-    ],
-)
-def test_protected_routes_require_credentials(anon_api, method, path):
+def test_protected_routes_require_credentials(anon_api, protected_route):
+    method, path = protected_route
+
     response = anon_api.request(method, path, json={"recipients": [], "body": "x"})
 
     assert response.status_code == 401

tests/test_auth.py

@@ -2,54 +2,25 @@
 
 import pytest
 from fastapi.testclient import TestClient
-from sqlalchemy.orm import Session
 
 import database
-from database import db_session
 from main import app
 
 WRONG_KEY_HEADERS = {"Authorization": "Bearer wrong-key"}
 
 
-@pytest.fixture
-def anon_client(test_engine, monkeypatch):
-    """TestClient that sends no Authorization header."""
-    monkeypatch.setattr(database, "engine", test_engine)
+def test_protected_route_rejects_missing_credentials(anon_client, protected_route):
+    method, path = protected_route
 
-    def override_db_session():
-        with Session(test_engine) as session:
-            yield session
-
-    app.dependency_overrides[db_session] = override_db_session
-    with TestClient(app) as test_client:
-        yield test_client
-    app.dependency_overrides.clear()
-
-
-@pytest.mark.parametrize(
-    ("method", "path"),
-    [
-        ("POST", "/push/send"),
-        ("GET", "/devices/all"),
-        ("DELETE", "/devices"),
-    ],
-)
-def test_protected_route_rejects_missing_credentials(anon_client, method, path):
     response = anon_client.request(method, path, json={"recipients": [], "body": "x"})
 
     assert response.status_code == 401
     assert response.headers["WWW-Authenticate"] == "Bearer"
 
 
-@pytest.mark.parametrize(
-    ("method", "path"),
-    [
-        ("POST", "/push/send"),
-        ("GET", "/devices/all"),
-        ("DELETE", "/devices"),
-    ],
-)
-def test_protected_route_rejects_wrong_key(anon_client, method, path):
+def test_protected_route_rejects_wrong_key(anon_client, protected_route):
+    method, path = protected_route
+
     response = anon_client.request(
         method, path, headers=WRONG_KEY_HEADERS, json={"recipients": [], "body": "x"}
     )
@@ -77,3 +48,21 @@ def test_startup_fails_without_api_key(test_engine, monkeypatch):
 
     with pytest.raises(RuntimeError, match="API_KEY"), TestClient(app):
         pass
+
+
+def test_missing_api_key_at_request_time_yields_401_not_500(anon_client, monkeypatch):
+    monkeypatch.delenv("API_KEY")
+
+    response = anon_client.get("/devices/all", headers={"Authorization": "Bearer anything"})
+
+    assert response.status_code == 401
+
+
+def test_placeholder_api_key_logs_a_warning(test_engine, monkeypatch, caplog):
+    monkeypatch.setattr(database, "engine", test_engine)
+    monkeypatch.setenv("API_KEY", "CHANGE_ME")
+
+    with TestClient(app):
+        pass
+
+    assert any("CHANGE_ME" in record.message for record in caplog.records)

tests/test_cors.py

@@ -1,5 +1,6 @@
 """Tests for CORS configuration."""
 
+import pytest
 from fastapi.testclient import TestClient
 
 from main import create_app
@@ -38,3 +39,10 @@ def test_unlisted_origin_is_rejected(monkeypatch):
     )
 
     assert "access-control-allow-origin" not in response.headers
+
+
+def test_wildcard_origin_is_rejected(monkeypatch):
+    monkeypatch.setenv("CORS_ORIGINS", "*")
+
+    with pytest.raises(ValueError, match="CORS_ORIGINS"):
+        create_app()