diff --git a/apis/devices.py b/apis/devices.py index 51906fb..bc09b86 100644 --- a/apis/devices.py +++ b/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(), ): diff --git a/auth.py b/auth.py index abdfdd5..4c09962 100644 --- a/auth.py +++ b/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, diff --git a/main.py b/main.py index f031498..0465853 100644 --- a/main.py +++ b/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 '" ) + 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) diff --git a/tests/conftest.py b/tests/conftest.py index f23e5bb..26db62b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -49,9 +49,20 @@ 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(): @@ -59,8 +70,22 @@ def override_db_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() diff --git a/tests/integration/test_server.py b/tests/integration/test_server.py index 5ad8ade..76fc452 100644 --- a/tests/integration/test_server.py +++ b/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 diff --git a/tests/test_auth.py b/tests/test_auth.py index 821201a..77f8a9d 100644 --- a/tests/test_auth.py +++ b/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) diff --git a/tests/test_cors.py b/tests/test_cors.py index 7cd444e..a90a6a1 100644 --- a/tests/test_cors.py +++ b/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()