From 2a92f6c9c041576b13908e637f21bec888440696 Mon Sep 17 00:00:00 2001 From: aswin Date: Sun, 28 Jun 2026 21:12:27 +0530 Subject: [PATCH] Harden config/security + wire Alembic (audit follow-up) Audit found no architectural rework needed; these close the real findings: - Refuse prod boot (APP_ENV=prod) when JWT_SECRET/ADMIN_PASSWORD are defaults; warn in dev. Log a change-password reminder after seeding the first admin. - Make CORS origins configurable (CORS_ORIGINS); default unchanged. - Move the audit-log DB write off the event loop via run_in_threadpool. - Add validation floors: user password min_length, non-empty names, session rate>0. - Wire Alembic for schema changes (baseline builds from metadata, Postgres+SQLite portable); create_all stays the zero-config boot path. Smoke test extended for the new validation; passwords bumped to satisfy it. Co-Authored-By: Claude Opus 4.8 --- .env.example | 5 ++ README.md | 22 ++++++- backend/alembic.ini | 39 +++++++++++ backend/alembic/env.py | 42 ++++++++++++ backend/alembic/script.py.mako | 24 +++++++ backend/alembic/versions/0001_baseline.py | 29 ++++++++ backend/app/config.py | 19 ++++++ backend/app/main.py | 80 +++++++++++++++-------- backend/app/schemas.py | 10 +-- backend/app/seed.py | 5 ++ backend/requirements.txt | 1 + backend/test_smoke.py | 10 ++- 12 files changed, 250 insertions(+), 36 deletions(-) create mode 100644 backend/alembic.ini create mode 100644 backend/alembic/env.py create mode 100644 backend/alembic/script.py.mako create mode 100644 backend/alembic/versions/0001_baseline.py diff --git a/.env.example b/.env.example index e891c56..36ef6e3 100644 --- a/.env.example +++ b/.env.example @@ -6,10 +6,15 @@ POSTGRES_PASSWORD=change-me POSTGRES_DB=studio # --- Backend / auth --- +# Set APP_ENV=prod in real deployments: the app refuses to boot if JWT_SECRET or +# ADMIN_PASSWORD are left at their insecure defaults. +APP_ENV=prod JWT_SECRET=generate-a-long-random-string # First admin, created on first boot only (no users yet): ADMIN_EMAIL=admin@example.com ADMIN_PASSWORD=change-me +# CORS: only needed for split UI/API origins. The single-image deploy is same-origin. +# CORS_ORIGINS=https://studio.example.com # --- Email (SMTP) — leave SMTP_HOST blank to disable email notifications --- SMTP_HOST= diff --git a/README.md b/README.md index 23d6930..67df9db 100644 --- a/README.md +++ b/README.md @@ -94,5 +94,23 @@ Pluggable providers, all optional: ## Schema migrations -Tables are created on startup (`Base.metadata.create_all`) — no migrations yet. -If the schema starts churning, add Alembic. +Fresh installs build the schema on startup via `Base.metadata.create_all` — zero +config, nothing to run. **Alembic** is wired in `backend/` for *schema changes*: + +```bash +cd backend +alembic revision --autogenerate -m "describe change" # after editing models +alembic upgrade head # apply (uses DATABASE_URL) +``` + +The `0001_baseline` revision recreates the current schema from the models, so it's +identical to `create_all` and works on both Postgres and SQLite. On an existing DB +created by `create_all` (no `alembic_version` table), run `alembic stamp head` once +before applying future migrations. + +## Production secrets + +Set `APP_ENV=prod` in `.env` for real deployments: the app then **refuses to boot** +if `JWT_SECRET` or `ADMIN_PASSWORD` are still the insecure defaults. In split +deployments (UI and API on different origins) set `CORS_ORIGINS` to the UI origin; +the default single-image deploy serves both same-origin and needs nothing. diff --git a/backend/alembic.ini b/backend/alembic.ini new file mode 100644 index 0000000..a736863 --- /dev/null +++ b/backend/alembic.ini @@ -0,0 +1,39 @@ +# Alembic config. The DB URL is NOT set here — env.py reads it from app settings +# (DATABASE_URL), so there's one source of truth and no secrets in this file. +[alembic] +script_location = alembic +prepend_sys_path = . + +[loggers] +keys = root,sqlalchemy,alembic + +[handlers] +keys = console + +[formatters] +keys = generic + +[logger_root] +level = WARN +handlers = console +qualname = + +[logger_sqlalchemy] +level = WARN +handlers = +qualname = sqlalchemy.engine + +[logger_alembic] +level = INFO +handlers = +qualname = alembic + +[handler_console] +class = StreamHandler +args = (sys.stderr,) +level = NOTSET +formatter = generic + +[formatter_generic] +format = %(levelname)-5.5s [%(name)s] %(message)s +datefmt = %H:%M:%S diff --git a/backend/alembic/env.py b/backend/alembic/env.py new file mode 100644 index 0000000..0d3c490 --- /dev/null +++ b/backend/alembic/env.py @@ -0,0 +1,42 @@ +"""Alembic environment. Self-contained: pulls the DB URL and metadata straight +from the app so there is one source of truth (no URL duplicated in alembic.ini). +""" +from logging.config import fileConfig + +from alembic import context +from sqlalchemy import create_engine + +from app.config import settings +from app.database import Base +import app.models # noqa: F401 — import for side effect: populates Base.metadata + +config = context.config +if config.config_file_name is not None: + fileConfig(config.config_file_name) + +target_metadata = Base.metadata + + +def run_migrations_offline() -> None: + context.configure( + url=settings.database_url, + target_metadata=target_metadata, + literal_binds=True, + dialect_opts={"paramstyle": "named"}, + ) + with context.begin_transaction(): + context.run_migrations() + + +def run_migrations_online() -> None: + connectable = create_engine(settings.database_url) + with connectable.connect() as connection: + context.configure(connection=connection, target_metadata=target_metadata) + with context.begin_transaction(): + context.run_migrations() + + +if context.is_offline_mode(): + run_migrations_offline() +else: + run_migrations_online() diff --git a/backend/alembic/script.py.mako b/backend/alembic/script.py.mako new file mode 100644 index 0000000..590f5b3 --- /dev/null +++ b/backend/alembic/script.py.mako @@ -0,0 +1,24 @@ +"""${message} + +Revision ID: ${up_revision} +Revises: ${down_revision | comma,n} +Create Date: ${create_date} +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +${imports if imports else ""} + +revision: str = ${repr(up_revision)} +down_revision: Union[str, None] = ${repr(down_revision)} +branch_labels: Union[str, Sequence[str], None] = ${repr(branch_labels)} +depends_on: Union[str, Sequence[str], None] = ${repr(depends_on)} + + +def upgrade() -> None: + ${upgrades if upgrades else "pass"} + + +def downgrade() -> None: + ${downgrades if downgrades else "pass"} diff --git a/backend/alembic/versions/0001_baseline.py b/backend/alembic/versions/0001_baseline.py new file mode 100644 index 0000000..d7ab4f6 --- /dev/null +++ b/backend/alembic/versions/0001_baseline.py @@ -0,0 +1,29 @@ +"""baseline: full current schema + +Builds every table from the live SQLAlchemy metadata rather than hand-written +DDL, so it is identical to the app's create_all and works on both Postgres +(prod) and SQLite (tests). Future schema changes get their own autogenerated +revisions on top of this. + +Revision ID: 0001_baseline +Revises: +Create Date: 2026-06-28 +""" +from typing import Sequence, Union + +from app.database import Base +import app.models # noqa: F401 — populates Base.metadata +from alembic import op + +revision: str = "0001_baseline" +down_revision: Union[str, None] = None +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + Base.metadata.create_all(bind=op.get_bind()) + + +def downgrade() -> None: + Base.metadata.drop_all(bind=op.get_bind()) diff --git a/backend/app/config.py b/backend/app/config.py index ea0e790..8f967d7 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -3,10 +3,14 @@ class Settings(BaseSettings): # ponytail: one settings object; defaults make local dev work with zero .env + app_env: str = "dev" # set APP_ENV=prod to refuse booting with default secrets database_url: str = "postgresql+psycopg2://studio:studio@db:5432/studio" jwt_secret: str = "change-me-in-prod" jwt_algorithm: str = "HS256" access_token_expire_minutes: int = 60 * 12 + # CORS: comma-separated origins. Default "*" suits split dev; the single-image + # deploy serves the SPA same-origin so this is moot there. Set in prod if split. + cors_origins: str = "*" # First-admin seed (created on boot if no users exist) admin_email: str = "admin@example.com" @@ -33,3 +37,18 @@ class Config: settings = Settings() + +# Default placeholder secrets shipped for zero-config local dev. If any of these +# survive into a prod boot (APP_ENV=prod), startup refuses (see main.lifespan). +DEFAULT_JWT_SECRET = "change-me-in-prod" +DEFAULT_ADMIN_PASSWORD = "admin123" + + +def insecure_defaults() -> list[str]: + """Names of secrets still set to their dev placeholder. Empty == safe.""" + bad = [] + if settings.jwt_secret == DEFAULT_JWT_SECRET: + bad.append("JWT_SECRET") + if settings.admin_password == DEFAULT_ADMIN_PASSWORD: + bad.append("ADMIN_PASSWORD") + return bad diff --git a/backend/app/main.py b/backend/app/main.py index c53c501..cdc042a 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1,3 +1,4 @@ +import logging from contextlib import asynccontextmanager from pathlib import Path @@ -5,8 +6,11 @@ from fastapi.middleware.cors import CORSMiddleware from fastapi.middleware.gzip import GZipMiddleware from fastapi.responses import FileResponse +from starlette.concurrency import run_in_threadpool from .auth import decode_token +from .config import insecure_defaults +from .config import settings as cfg # 'settings' name is taken by the router module below from .database import Base, SessionLocal, engine from .models import AuditLog, StudioSettings, User from .routers import ( @@ -27,11 +31,25 @@ from .seed import seed_admin AUDIT_METHODS = {"POST", "PUT", "DELETE", "PATCH"} +log = logging.getLogger("studio") @asynccontextmanager async def lifespan(app: FastAPI): - # No migrations yet — create_all on startup (consider Alembic if schema churns) + # Refuse to boot a prod deploy still using the dev placeholder secrets; in dev + # just warn so zero-.env local runs (and tests) keep working untouched. + bad = insecure_defaults() + if bad: + if cfg.app_env == "prod": + raise RuntimeError( + f"Refusing to start: {', '.join(bad)} still set to the insecure " + f"default. Set real values in .env (APP_ENV=prod)." + ) + log.warning("Using insecure default %s — fine for dev, NOT for prod.", ", ".join(bad)) + # Schema is created here via create_all for zero-config single-container boot. + # ponytail: Alembic is wired (backend/alembic) for schema *changes*; run + # `alembic upgrade head` on deploy when migrations exist. create_all stays the + # fresh-install path. Base.metadata.create_all(bind=engine) db = SessionLocal() try: @@ -46,41 +64,51 @@ async def lifespan(app: FastAPI): app.add_middleware(GZipMiddleware, minimum_size=1024) app.add_middleware( CORSMiddleware, - allow_origins=["*"], + allow_origins=[o.strip() for o in cfg.cors_origins.split(",") if o.strip()], allow_methods=["*"], allow_headers=["*"], ) +def _write_audit(method: str, path: str, authz: str, status_code: int) -> None: + """Blocking audit write. Runs in a threadpool so it never blocks the loop.""" + user_id, user_email = None, "anonymous" + if authz.lower().startswith("bearer "): + try: + user_id = int(decode_token(authz.split(" ", 1)[1]).get("sub")) + except Exception: + user_id = None + db = SessionLocal() + try: + s = db.get(StudioSettings, 1) + if s is not None and not s.audit_enabled: + return # auditing turned off + if user_id is not None: + u = db.get(User, user_id) + user_email = u.email if u else f"#{user_id}" + db.add(AuditLog( + user_id=user_id, user_email=user_email, + method=method, path=path, status_code=status_code, + )) + db.commit() + finally: + db.close() + + @app.middleware("http") async def audit_middleware(request: Request, call_next): response = await call_next(request) - # Record mutations only; never let auditing break the request. + # Record mutations only; never let auditing break the request. The DB write is + # synchronous, so defer it to a threadpool to keep it off the event loop. try: if request.method in AUDIT_METHODS and not request.url.path.startswith("/api/auth"): - user_id, user_email = None, "anonymous" - authz = request.headers.get("authorization", "") - if authz.lower().startswith("bearer "): - try: - user_id = int(decode_token(authz.split(" ", 1)[1]).get("sub")) - except Exception: - user_id = None - db = SessionLocal() - try: - s = db.get(StudioSettings, 1) - if s is not None and not s.audit_enabled: - return response # auditing turned off - if user_id is not None: - u = db.get(User, user_id) - user_email = u.email if u else f"#{user_id}" - db.add(AuditLog( - user_id=user_id, user_email=user_email, - method=request.method, path=request.url.path, - status_code=response.status_code, - )) - db.commit() - finally: - db.close() + await run_in_threadpool( + _write_audit, + request.method, + request.url.path, + request.headers.get("authorization", ""), + response.status_code, + ) except Exception: pass return response diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 60a05a4..30286fc 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -29,7 +29,7 @@ class UserOut(ORM): class UserCreate(BaseModel): email: EmailStr - password: str + password: str = Field(min_length=6) # reject empty/trivial passwords at create role: str = "staff" # admin|staff|parent|tutor student_ids: list[int] = [] # only used for parent accounts tutor_id: int | None = None # only used for tutor accounts @@ -46,7 +46,7 @@ class PasswordChange(BaseModel): # ---- Tutors ---- class TutorBase(BaseModel): - name: str + name: str = Field(min_length=1) phone: str | None = None email: str | None = None is_guest: bool = False @@ -65,7 +65,7 @@ class TutorOut(ORM, TutorBase): # ---- Batches ---- class BatchBase(BaseModel): - name: str + name: str = Field(min_length=1) classes_per_week: int = Field(default=1, ge=1) @@ -82,7 +82,7 @@ class BatchOut(ORM, BatchBase): # ---- Students ---- class StudentBase(BaseModel): - name: str + name: str = Field(min_length=1) guardian_name: str | None = None guardian_phone: str | None = None guardian_email: str | None = None @@ -116,7 +116,7 @@ class SessionBase(BaseModel): date: date start_time: time | None = None end_time: time | None = None - rate: float | None = None + rate: float | None = Field(default=None, gt=0) # private/dropin fee; never ≤0 tutor_id: int | None = None batch_id: int | None = None notes: str | None = None diff --git a/backend/app/seed.py b/backend/app/seed.py index 68927a2..7e74c7a 100644 --- a/backend/app/seed.py +++ b/backend/app/seed.py @@ -1,9 +1,13 @@ +import logging + from sqlalchemy.orm import Session from .auth import hash_password from .config import settings from .models import User +log = logging.getLogger("studio") + def seed_admin(db: Session) -> None: """Create the first admin on boot if there are no users yet.""" @@ -17,3 +21,4 @@ def seed_admin(db: Session) -> None: ) ) db.commit() + log.warning("Seeded first admin %s — log in and change this password now.", settings.admin_email) diff --git a/backend/requirements.txt b/backend/requirements.txt index 9eff21b..396fd99 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -1,6 +1,7 @@ fastapi==0.111.0 uvicorn[standard]==0.30.1 sqlalchemy==2.0.30 +alembic==1.13.1 psycopg2-binary==2.9.9 pydantic==2.7.1 pydantic-settings==2.3.1 diff --git a/backend/test_smoke.py b/backend/test_smoke.py index 9e27f5c..7635789 100644 --- a/backend/test_smoke.py +++ b/backend/test_smoke.py @@ -48,8 +48,8 @@ def run(): # Parent isolation other = c.post("/api/students", json={"name": "Hidden Kid"}, headers=h).json() - c.post("/api/users", json={"email": "parent@example.com", "password": "pw", "role": "parent", "student_ids": [sid]}, headers=h) - ptok = c.post("/api/auth/login", data={"username": "parent@example.com", "password": "pw"}).json() + c.post("/api/users", json={"email": "parent@example.com", "password": "parentpw", "role": "parent", "student_ids": [sid]}, headers=h) + ptok = c.post("/api/auth/login", data={"username": "parent@example.com", "password": "parentpw"}).json() ph = {"Authorization": f"Bearer {ptok['access_token']}"} assert [s["name"] for s in c.get("/api/students", headers=ph).json()] == ["Asha"] assert c.get(f"/api/students/{other['id']}", headers=ph).status_code == 404 @@ -68,6 +68,10 @@ def run(): # Money validation: amounts must be > 0 assert c.post("/api/payments", json={"amount": -50, "method": "cash"}, headers=h).status_code == 422 + # Input floors: empty user password, empty name, non-positive session rate all rejected + assert c.post("/api/users", json={"email": "x@example.com", "password": "", "role": "staff"}, headers=h).status_code == 422 + assert c.post("/api/students", json={"name": ""}, headers=h).status_code == 422 + assert c.post("/api/sessions", json={"session_type": "private", "date": "2030-04-01", "rate": -5}, headers=h).status_code == 422 # Fees stay removed; studio settings are back for the invoice header assert c.get("/api/fees/structures", headers=h).status_code == 404 @@ -117,7 +121,7 @@ def run(): assert c.get("/api/students", headers=th).json() == [] assert c.get("/api/reports/my-earnings", headers=th).status_code == 200 # a tutor can't be linked to a second login - assert c.post("/api/users", json={"email": "dup@example.com", "password": "pw123", "role": "tutor", "tutor_id": tut["id"]}, headers=h).status_code == 400 + assert c.post("/api/users", json={"email": "dup@example.com", "password": "dup1234", "role": "tutor", "tutor_id": tut["id"]}, headers=h).status_code == 400 # Global search: staff finds students/batches/tutors; non-staff blocked res = c.get("/api/search?q=Asha", headers=h).json()