diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000..32864d3 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,51 @@ +# Pull Request + +## Summary + + + +## Type of Change + +- [ ] Bug fix +- [ ] New feature +- [ ] Refactor / code quality +- [ ] CI / tooling +- [ ] Documentation + +## Checklist + +### Tests Added + +- [ ] New/changed behaviour is covered by unit tests +- [ ] Full backend test suite passes locally (`pytest backend/tests/`) +- [ ] Legacy migration tests still pass where applicable + (`pytest backend/migration/tests/`) + +### Verified Workflow Paths + +- [ ] I ran the workflow path verifier and it passed: + `python3 tools/verify_workflow_paths.py` + (verifies every file/script referenced by CI workflows exists) +- [ ] Any moved/renamed files are updated in workflows, scripts + (`scripts/`, `preflight.*`), and docs + +### Safety & Security + +- [ ] No new `# nosec` / `# noqa` suppressions — or each new suppression is + documented in `docs/security/Rationale.md` +- [ ] No secrets, credentials, or internal paths added to code or logs +- [ ] Shell scripts validate their inputs (env names, table names, paths) + +### API & Data + +- [ ] New/changed endpoints declare a Pydantic `response_model` +- [ ] Database state transitions follow the `RunStatus` lifecycle + (`ALLOWED_TRANSITIONS` in `backend/database/models.py`) + +## Screenshots / Output + + + +## Notes for Reviewers + + diff --git a/backend/api/routes/dashboard.py b/backend/api/routes/dashboard.py index 2d858d3..a1efb3c 100644 --- a/backend/api/routes/dashboard.py +++ b/backend/api/routes/dashboard.py @@ -5,13 +5,14 @@ from fastapi import APIRouter, Depends from sqlalchemy.orm import Session +from api.schemas import DashboardStats from database.connection import get_db from services import dashboard_service router = APIRouter(tags=["dashboard"]) -@router.get("/dashboard") +@router.get("/dashboard", response_model=DashboardStats) def get_dashboard(db: Session = Depends(get_db)): """Return aggregate dashboard statistics.""" return dashboard_service.get_dashboard_stats(db) diff --git a/backend/api/routes/execution.py b/backend/api/routes/execution.py index c5caf45..c636235 100644 --- a/backend/api/routes/execution.py +++ b/backend/api/routes/execution.py @@ -6,13 +6,14 @@ from fastapi import APIRouter, Depends, HTTPException from sqlalchemy.orm import Session +from api.schemas import EvaluationResponse, MigrationExecuteResponse from database.connection import get_db from services import execution_service, evaluation_service router = APIRouter(prefix="/migrations", tags=["execution"]) -@router.post("/{run_id}/execute") +@router.post("/{run_id}/execute", response_model=MigrationExecuteResponse) def execute_migration(run_id: int, db: Session = Depends(get_db)): """Execute migration — create staging tables and load CSV data.""" result = execution_service.execute_migration(db, run_id) @@ -21,7 +22,7 @@ def execute_migration(run_id: int, db: Session = Depends(get_db)): return result -@router.post("/{run_id}/evaluate") +@router.post("/{run_id}/evaluate", response_model=EvaluationResponse) def evaluate_migration(run_id: int, db: Session = Depends(get_db)): """Evaluate data quality of migrated data vs source CSVs.""" result = evaluation_service.evaluate_run(db, run_id) diff --git a/backend/api/routes/health.py b/backend/api/routes/health.py index d2165ce..c868f53 100644 --- a/backend/api/routes/health.py +++ b/backend/api/routes/health.py @@ -1,6 +1,7 @@ """Health-check route for the Migration Evaluation Platform (MEP).""" from fastapi import APIRouter +from api.schemas import HealthResponse from config import settings from database.connection import check_db_connection @@ -10,17 +11,17 @@ VERSION = "0.1.0" -@router.get("/health") -def health() -> dict: +@router.get("/health", response_model=HealthResponse) +def health() -> HealthResponse: """Report service health. Always returns HTTP 200. If the database is unreachable, the response still succeeds but reports ``"database": "disconnected"``. """ db_connected = check_db_connection() - return { - "status": "healthy", - "version": VERSION, - "environment": settings.APP_ENV, - "database": "connected" if db_connected else "disconnected", - } + return HealthResponse( + status="healthy", + version=VERSION, + environment=settings.APP_ENV, + database="connected" if db_connected else "disconnected", + ) diff --git a/backend/api/routes/reports.py b/backend/api/routes/reports.py index 176eec8..c52e363 100644 --- a/backend/api/routes/reports.py +++ b/backend/api/routes/reports.py @@ -7,13 +7,17 @@ from fastapi.responses import FileResponse from sqlalchemy.orm import Session +from api.schemas import ReportResponse from database.connection import get_db -from services import report_service, schema_service, execution_service, evaluation_service +from services import report_service, schema_service, evaluation_service router = APIRouter(prefix="/reports", tags=["reports"]) -@router.post("/{run_id}/generate") +# ``response_model=ReportResponse`` intentionally filters out the internal +# ``path`` key returned by report_service so server filesystem paths are not +# leaked to API clients (the frontend only consumes ``download_url``). +@router.post("/{run_id}/generate", response_model=ReportResponse) def generate_report( run_id: int, format: str = Query("json", pattern="^(json|html)$"), diff --git a/backend/api/schemas.py b/backend/api/schemas.py index 36406ec..2966ea2 100644 --- a/backend/api/schemas.py +++ b/backend/api/schemas.py @@ -10,6 +10,24 @@ from pydantic import BaseModel, Field +# --------------------------------------------------------------------------- +# Service Meta (root & health) +# --------------------------------------------------------------------------- + +class RootResponse(BaseModel): + """Root route payload confirming the API is running.""" + message: str + version: str + + +class HealthResponse(BaseModel): + """Health-check payload.""" + status: str = Field(..., examples=["healthy"]) + version: str + environment: str + database: str = Field(..., examples=["connected", "disconnected"]) + + # --------------------------------------------------------------------------- # Migration Run # --------------------------------------------------------------------------- diff --git a/backend/config.py b/backend/config.py index 799b2c3..31ee8c7 100644 --- a/backend/config.py +++ b/backend/config.py @@ -3,6 +3,7 @@ Settings are loaded from environment variables and/or a local ``.env`` file using pydantic-settings. """ +from pydantic import model_validator from pydantic_settings import BaseSettings, SettingsConfigDict @@ -19,6 +20,31 @@ class Settings(BaseSettings): # Enable debug behaviour (verbose logging, etc.) DEBUG: bool = True + # Allow the application to auto-create database tables on startup via + # ``Base.metadata.create_all``. This is a development convenience only — + # production schema changes MUST go through explicit, reviewed migrations + # (e.g. Alembic). Startup fails fast if this is enabled in production. + ALLOW_SCHEMA_AUTO_CREATE: bool = True + + @model_validator(mode="after") + def _forbid_auto_create_in_production(self) -> "Settings": + """Fail fast when schema auto-creation is enabled in production. + + Auto-creating tables in production can silently mutate the schema, + mask missing migrations, and cause data loss on model drift. We treat + this combination as a fatal misconfiguration so the service refuses to + start rather than running unsafely. + """ + if self.APP_ENV.strip().lower() == "production" and self.ALLOW_SCHEMA_AUTO_CREATE: + raise ValueError( + "Unsafe configuration: ALLOW_SCHEMA_AUTO_CREATE must be false when " + "APP_ENV=production. Schema changes in production must be applied " + "through explicit migrations, not automatic table creation. " + "Set ALLOW_SCHEMA_AUTO_CREATE=false (and use a migration tool) " + "to start the service in production." + ) + return self + model_config = SettingsConfigDict( env_file=".env", env_file_encoding="utf-8", diff --git a/backend/main.py b/backend/main.py index d8db875..4f45397 100644 --- a/backend/main.py +++ b/backend/main.py @@ -11,6 +11,7 @@ from fastapi.middleware.cors import CORSMiddleware from api.routes import health, migrations, validation, execution, reports, dashboard +from api.schemas import RootResponse from config import settings from database.connection import check_db_connection, engine from database.models import Base @@ -47,14 +48,19 @@ def on_startup() -> None: logger.info("Environment: %s | Debug: %s", settings.APP_ENV, settings.DEBUG) if check_db_connection(): logger.info("Database connection: OK") - # Auto-create tables in dev (use Alembic migrations in production) - Base.metadata.create_all(bind=engine) - logger.info("Database tables ensured") + if settings.ALLOW_SCHEMA_AUTO_CREATE: + # Auto-create tables in dev only. Production must use explicit + # migrations; config.Settings refuses to start with this flag + # enabled when APP_ENV=production. + Base.metadata.create_all(bind=engine) + logger.info("Database tables ensured (schema auto-create enabled)") + else: + logger.info("Schema auto-create disabled; expecting managed migrations") else: logger.warning("Database connection: UNAVAILABLE") -@app.get("/") -def root() -> dict: +@app.get("/", response_model=RootResponse) +def root() -> RootResponse: """Root route confirming the API is running.""" - return {"message": "MEP API is running", "version": VERSION} + return RootResponse(message="MEP API is running", version=VERSION) diff --git a/backend/tests/test_config_safety.py b/backend/tests/test_config_safety.py new file mode 100644 index 0000000..11ff341 --- /dev/null +++ b/backend/tests/test_config_safety.py @@ -0,0 +1,62 @@ +"""Tests for production configuration safety gates in ``config.Settings``. + +The key invariant: the application must REFUSE to start when +``APP_ENV=production`` and ``ALLOW_SCHEMA_AUTO_CREATE=true``, because schema +auto-creation in production can silently mutate the database schema. +""" +import pytest +from pydantic import ValidationError + +from config import Settings + + +def make_settings(**overrides): + """Build Settings isolated from any local .env file.""" + return Settings(_env_file=None, **overrides) + + +class TestProductionSafetyGate: + def test_production_with_auto_create_fails(self): + """production + auto-create enabled must raise at construction time.""" + with pytest.raises(ValidationError) as exc_info: + make_settings(APP_ENV="production", ALLOW_SCHEMA_AUTO_CREATE=True) + assert "ALLOW_SCHEMA_AUTO_CREATE" in str(exc_info.value) + + def test_production_case_insensitive(self): + """The gate must not be bypassable via casing or whitespace.""" + for env in ("PRODUCTION", "Production", " production "): + with pytest.raises(ValidationError): + make_settings(APP_ENV=env, ALLOW_SCHEMA_AUTO_CREATE=True) + + def test_production_with_auto_create_disabled_ok(self): + """production + auto-create disabled is a valid configuration.""" + s = make_settings(APP_ENV="production", ALLOW_SCHEMA_AUTO_CREATE=False) + assert s.APP_ENV == "production" + assert s.ALLOW_SCHEMA_AUTO_CREATE is False + + def test_development_with_auto_create_ok(self): + """development keeps the convenient auto-create default.""" + s = make_settings(APP_ENV="development", ALLOW_SCHEMA_AUTO_CREATE=True) + assert s.ALLOW_SCHEMA_AUTO_CREATE is True + + def test_staging_with_auto_create_ok(self): + """staging is not gated (only production is fatal).""" + s = make_settings(APP_ENV="staging", ALLOW_SCHEMA_AUTO_CREATE=True) + assert s.ALLOW_SCHEMA_AUTO_CREATE is True + + def test_default_is_development_with_auto_create(self): + """Defaults stay developer-friendly.""" + s = make_settings() + assert s.APP_ENV == "development" + assert s.ALLOW_SCHEMA_AUTO_CREATE is True + + def test_env_variable_string_coercion(self, monkeypatch): + """Values coming from environment variables are honoured too.""" + monkeypatch.setenv("APP_ENV", "production") + monkeypatch.setenv("ALLOW_SCHEMA_AUTO_CREATE", "true") + with pytest.raises(ValidationError): + Settings(_env_file=None) + + monkeypatch.setenv("ALLOW_SCHEMA_AUTO_CREATE", "false") + s = Settings(_env_file=None) + assert s.ALLOW_SCHEMA_AUTO_CREATE is False diff --git a/docs/security/Rationale.md b/docs/security/Rationale.md new file mode 100644 index 0000000..b7cb1e8 --- /dev/null +++ b/docs/security/Rationale.md @@ -0,0 +1,84 @@ +# Security Suppression Rationale + +This document records every linter/security-scanner suppression (`# nosec`, +`# noqa`, `shellcheck disable`, etc.) in the codebase, with a technical +justification for why the finding is a false positive or an accepted risk. + +**Policy:** a suppression comment is only acceptable when it is accompanied by +an entry in this file. PRs that add a new suppression must update this document +(see the PR template checklist). + +--- + +## Active Suppressions + +### 1. `backend/database/connection.py` — `# noqa: BLE001` (broad exception in health check) + +```python +except Exception as exc: # noqa: BLE001 - health check must never raise +``` + +- **Rule suppressed:** BLE001 (blind `except Exception`). +- **Justification:** `check_db_connection()` is used by the `/api/health` + endpoint and the startup hook. Its contract is *"return False on any + failure, never raise"*. The set of exceptions SQLAlchemy/psycopg2 can raise + during a connection attempt is broad and driver-dependent (OperationalError, + DBAPIError, socket/DNS errors, ...). Catching a narrower set risks the + health endpoint returning HTTP 500 instead of a controlled + `"database": "disconnected"` payload. +- **Risk accepted:** silently swallowing an unexpected programming error in + this small, side-effect-free function. Mitigated by the failure being + surfaced in the health payload and startup logs. + +### 2. `backend/migration/evals/runner.py` — `# nosec B608` (SQL string composition) + +```python +# runner.py:309 (inside _count_dev_rows) +query = 'SELECT count(*) FROM te_dev."' + tbl + '";' # nosec B608 +``` + +- **Rule suppressed:** Bandit B608 (possible SQL injection via string + concatenation). +- **Justification:** `tbl` is not user input. It iterates over the hardcoded + `_DEV_SEED_TABLES` constant defined in the same module; no external or + request-derived value can reach this string. Table names cannot be bound as + query parameters in PostgreSQL, so string composition is the only option + here, and the identifier is double-quoted against a fixed allowlist. +- **Risk accepted:** none in the current call graph. If the eval runner is + ever refactored to accept table names from configuration or user input, + this suppression MUST be revisited and the identifier quoted/validated + (e.g. `psycopg2.sql.Identifier`). + +### 3. `backend/migration/evals/runner.py` — `# noqa: BLE001` (broad exception in eval loop) + +- **Rule suppressed:** BLE001 (blind `except Exception`). +- **Justification:** the eval runner executes a batch of independent eval + cases; one case failing for any reason (bad fixture, DB hiccup, assertion + bug) must be recorded as a failed case rather than aborting the whole run. + The exception is captured into the eval report, not discarded. +- **Risk accepted:** an unexpected error is downgraded to a failed eval case. + Acceptable for a developer-facing tool; the full traceback is preserved in + the report output. + +--- + +## Adding a New Suppression (template) + +Copy this template into the "Active Suppressions" section: + +```markdown +### N. `:` — `` () + +- **Rule suppressed:** . +- **Justification:** . +- **Risk accepted:** . +``` + +Guidelines: + +1. Prefer fixing the finding over suppressing it. +2. Scope suppressions to a single line — never file-wide or rule-wide + disables. +3. Include the rule ID in the comment (`# noqa: BLE001`, not bare `# noqa`). +4. Re-review entries here whenever the suppressed code changes.