Browse Source

Security hardening (maziggy/bambuddy-security #11)

maziggy 1 week ago
parent
commit
83594da2ee

+ 1 - 1
backend/app/api/routes/auth.py

@@ -1230,7 +1230,7 @@ async def forgot_password_confirm(request: ForgotPasswordConfirmRequest, db: Asy
         raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid or expired password reset token")
         raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid or expired password reset token")
 
 
     username, expires_at = row
     username, expires_at = row
-    # SQLite returns naive datetimes; treat them as UTC.
+    # Stored as naive UTC on both databases; treat it as UTC.
     if expires_at.tzinfo is None:
     if expires_at.tzinfo is None:
         expires_at = expires_at.replace(tzinfo=timezone.utc)
         expires_at = expires_at.replace(tzinfo=timezone.utc)
     if now > expires_at:
     if now > expires_at:

+ 3 - 3
backend/app/api/routes/mfa.py

@@ -160,9 +160,9 @@ def _etag_matches(if_none_match: str | None, etag_raw: str | None) -> bool:
 def _as_utc(dt: datetime) -> datetime:
 def _as_utc(dt: datetime) -> datetime:
     """Return *dt* with UTC timezone attached.
     """Return *dt* with UTC timezone attached.
 
 
-    SQLite/aiosqlite strips timezone info when reading DateTime(timezone=True)
-    columns back – the stored value is always UTC, so we just re-attach the
-    info when doing Python-level comparisons.
+    DateTime columns come back naive on both SQLite and PostgreSQL – the
+    stored value is always UTC, so we just re-attach the info when doing
+    Python-level comparisons.
     """
     """
     return dt if dt.tzinfo is not None else dt.replace(tzinfo=timezone.utc)
     return dt if dt.tzinfo is not None else dt.replace(tzinfo=timezone.utc)
 
 

+ 8 - 0
backend/app/api/routes/settings.py

@@ -1594,6 +1594,14 @@ async def restore_backup(
             await reinitialize_database()
             await reinitialize_database()
             await init_db()
             await init_db()
 
 
+            # 9. Sign everyone out. The restored users and auth tokens come from
+            # another point in time, possibly written by older code, so no
+            # session issued before the restore should stay valid.
+            from backend.app.core import database as _db_mod
+
+            async with _db_mod.engine.begin() as conn:
+                await _db_mod.invalidate_all_sessions(conn)
+
             logger.info("Restore complete - restart required")
             logger.info("Restore complete - restart required")
             message = "Backup restored successfully. Please restart Bambuddy for changes to take effect."
             message = "Backup restored successfully. Please restart Bambuddy for changes to take effect."
             if skipped_dirs:
             if skipped_dirs:

+ 74 - 0
backend/app/core/database.py

@@ -5411,6 +5411,80 @@ async def _run_migrations(conn):
     # default -- identical DDL on SQLite and Postgres.
     # default -- identical DDL on SQLite and Postgres.
     await _safe_execute(conn, "ALTER TABLE print_archives ADD COLUMN timelapse_plate_id INTEGER")
     await _safe_execute(conn, "ALTER TABLE print_archives ADD COLUMN timelapse_plate_id INTEGER")
 
 
+    await _migrate_auth_datetimes_to_timestamp(conn)
+
+
+# Columns that used to be declared DateTime(timezone=True). Every bound datetime
+# reaches asyncpg naive (see _strip_tz_from_params), and asyncpg reads a naive
+# value for a timestamptz bind as process-local time, so on a host with TZ set
+# these were stored shifted by its UTC offset. They are plain DateTime now, like
+# every other column; this converts the PostgreSQL columns to match.
+_AUTH_DATETIME_COLUMNS = (
+    ("users", "password_changed_at"),
+    ("auth_ephemeral_tokens", "expires_at"),
+    ("auth_ephemeral_tokens", "created_at"),
+    ("auth_rate_limit_events", "occurred_at"),
+)
+
+
+async def _migrate_auth_datetimes_to_timestamp(conn) -> None:
+    """Convert the auth datetime columns from timestamptz to TIMESTAMP (UTC).
+
+    PostgreSQL only; SQLite has no time zone type. Only a column that is still
+    timestamptz is converted: the same USING on a TIMESTAMP column would shift
+    it by the session time zone.
+
+    The values already stored keep their offset error (the host's offset at
+    write time is not recorded), and that includes password_changed_at on
+    installs where the column is already TIMESTAMP: the bind followed the
+    model's type, not the column's. So when anything was converted, which
+    happens exactly once on an install that ran the old models, every session
+    is invalidated (see invalidate_all_sessions).
+    """
+    from sqlalchemy import text
+
+    if is_sqlite():
+        return
+    converted = False
+    for table, column in _AUTH_DATETIME_COLUMNS:
+        result = await conn.execute(
+            text(
+                "SELECT data_type FROM information_schema.columns "
+                "WHERE table_schema = current_schema() AND table_name = :t AND column_name = :c"
+            ),
+            {"t": table, "c": column},
+        )
+        row = result.fetchone()
+        if row and row[0] == "timestamp with time zone":
+            await _safe_execute(
+                conn,
+                f"ALTER TABLE {table} ALTER COLUMN {column} TYPE TIMESTAMP USING ({column} AT TIME ZONE 'UTC')",
+            )
+            converted = True
+    if converted:
+        await invalidate_all_sessions(conn)
+
+
+async def invalidate_all_sessions(conn) -> None:
+    """Make every user sign in again and drop every short-lived auth record.
+
+    Sets password_changed_at to now for all users, so every JWT issued before
+    is rejected, and deletes the pending auth tokens (reset links, 2FA and OIDC
+    state, media/camera/websocket tokens, slicer downloads, connect codes) and
+    the rate-limit events, so their counters start from zero. The logout
+    blocklist (revoked_jti) is kept. Used once after the auth
+    datetime conversion and after every backup restore, where the restored
+    rows may carry values written by older code.
+    """
+    from sqlalchemy import text
+
+    from backend.app.utils.local_time import utcnow_naive
+
+    async with conn.begin_nested():
+        await conn.execute(text("UPDATE users SET password_changed_at = :now"), {"now": utcnow_naive()})
+        await conn.execute(text("DELETE FROM auth_ephemeral_tokens WHERE token_type <> 'revoked_jti'"))
+        await conn.execute(text("DELETE FROM auth_rate_limit_events"))
+
 
 
 async def _backfill_snapshot_prices(conn) -> None:
 async def _backfill_snapshot_prices(conn) -> None:
     """Give the energy snapshots taken before #1251 the price set at upgrade.
     """Give the energy snapshots taken before #1251 the price set at upgrade.

+ 3 - 3
backend/app/models/auth_ephemeral.py

@@ -91,9 +91,9 @@ class AuthEphemeralToken(Base):
     # (XSS can read JS memory but cannot read HttpOnly cookies).
     # (XSS can read JS memory but cannot read HttpOnly cookies).
     challenge_id: Mapped[str | None] = mapped_column(String(128), nullable=True)
     challenge_id: Mapped[str | None] = mapped_column(String(128), nullable=True)
 
 
-    expires_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False)
+    expires_at: Mapped[datetime] = mapped_column(DateTime, nullable=False)
     created_at: Mapped[datetime] = mapped_column(
     created_at: Mapped[datetime] = mapped_column(
-        DateTime(timezone=True),
+        DateTime,
         nullable=False,
         nullable=False,
         default=lambda: datetime.now(timezone.utc),
         default=lambda: datetime.now(timezone.utc),
     )
     )
@@ -224,7 +224,7 @@ class AuthRateLimitEvent(Base):
     username: Mapped[str] = mapped_column(String(150), nullable=False, index=True)
     username: Mapped[str] = mapped_column(String(150), nullable=False, index=True)
     event_type: Mapped[str] = mapped_column(String(20), nullable=False)  # '2fa_attempt' | 'email_send'
     event_type: Mapped[str] = mapped_column(String(20), nullable=False)  # '2fa_attempt' | 'email_send'
     occurred_at: Mapped[datetime] = mapped_column(
     occurred_at: Mapped[datetime] = mapped_column(
-        DateTime(timezone=True),
+        DateTime,
         nullable=False,
         nullable=False,
         default=lambda: datetime.now(timezone.utc),
         default=lambda: datetime.now(timezone.utc),
     )
     )

+ 1 - 1
backend/app/models/user.py

@@ -37,7 +37,7 @@ class User(Base):
 
 
     # Set whenever the local password is changed/reset — used to invalidate JWTs
     # Set whenever the local password is changed/reset — used to invalidate JWTs
     # issued before the change (M-R7-B).  NULL means no password change recorded yet.
     # issued before the change (M-R7-B).  NULL means no password change recorded yet.
-    password_changed_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True)
+    password_changed_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True)
 
 
     # Per-user Bambu Cloud credentials (when auth is enabled, each user has their own)
     # Per-user Bambu Cloud credentials (when auth is enabled, each user has their own)
     cloud_token: Mapped[str | None] = mapped_column(String(500), nullable=True, default=None)
     cloud_token: Mapped[str | None] = mapped_column(String(500), nullable=True, default=None)

+ 2 - 2
backend/app/utils/local_time.py

@@ -51,8 +51,8 @@ def local_zone() -> tzinfo:
 def utcnow_naive() -> datetime:
 def utcnow_naive() -> datetime:
     """Current UTC time, tzinfo stripped.
     """Current UTC time, tzinfo stripped.
 
 
-    Bambuddy's ``DateTime`` columns are naive and hold UTC; only the few that
-    genuinely need an offset are declared ``DateTime(timezone=True)``. SQLite
+    Bambuddy's ``DateTime`` columns are naive and hold UTC; none is declared
+    ``DateTime(timezone=True)`` (test_code_quality.py enforces it). SQLite
     silently tolerates an aware value written to a naive column (its bind
     silently tolerates an aware value written to a naive column (its bind
     processor reads the fields and drops the offset), which is why aware writes
     processor reads the fields and drops the offset), which is why aware writes
     survived here for so long — but **asyncpg rejects them outright** with
     survived here for so long — but **asyncpg rejects them outright** with

+ 1 - 0
backend/tests/integration/test_backup_manifest.py

@@ -117,6 +117,7 @@ async def test_overlay_logo_survives_backup_and_restore(async_client, monkeypatc
         patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
         patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
         patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
         patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
         patch("backend.app.core.database.init_db", new_callable=AsyncMock),
         patch("backend.app.core.database.init_db", new_callable=AsyncMock),
+        patch("backend.app.core.database.invalidate_all_sessions", new_callable=AsyncMock),
         patch("backend.app.services.print_scheduler.scheduler.stop"),
         patch("backend.app.services.print_scheduler.scheduler.stop"),
         patch("backend.app.services.smart_plug_manager.smart_plug_manager.stop_scheduler"),
         patch("backend.app.services.smart_plug_manager.smart_plug_manager.stop_scheduler"),
         patch("backend.app.services.notification_service.notification_service.stop_digest_scheduler"),
         patch("backend.app.services.notification_service.notification_service.stop_digest_scheduler"),

+ 4 - 0
backend/tests/integration/test_security.py

@@ -2014,6 +2014,7 @@ class TestBackupKeyFiles:
             patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
             patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
             patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
             patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
             patch("backend.app.core.database.init_db", new_callable=AsyncMock),
             patch("backend.app.core.database.init_db", new_callable=AsyncMock),
+            patch("backend.app.core.database.invalidate_all_sessions", new_callable=AsyncMock),
         ):
         ):
             resp = await async_client.post(
             resp = await async_client.post(
                 "/api/v1/settings/restore",
                 "/api/v1/settings/restore",
@@ -2054,6 +2055,7 @@ class TestBackupKeyFiles:
             patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
             patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
             patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
             patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
             patch("backend.app.core.database.init_db", new_callable=AsyncMock),
             patch("backend.app.core.database.init_db", new_callable=AsyncMock),
+            patch("backend.app.core.database.invalidate_all_sessions", new_callable=AsyncMock),
         ):
         ):
             resp = await async_client.post(
             resp = await async_client.post(
                 "/api/v1/settings/restore",
                 "/api/v1/settings/restore",
@@ -2175,6 +2177,7 @@ class TestBackupKeyFiles:
             patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
             patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
             patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
             patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
             patch("backend.app.core.database.init_db", new_callable=AsyncMock),
             patch("backend.app.core.database.init_db", new_callable=AsyncMock),
+            patch("backend.app.core.database.invalidate_all_sessions", new_callable=AsyncMock),
         ):
         ):
             resp = await async_client.post(
             resp = await async_client.post(
                 "/api/v1/settings/restore",
                 "/api/v1/settings/restore",
@@ -2388,6 +2391,7 @@ class TestBackupKeyFiles:
                 patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
                 patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
                 patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
                 patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
                 patch("backend.app.core.database.init_db", new_callable=AsyncMock),
                 patch("backend.app.core.database.init_db", new_callable=AsyncMock),
+                patch("backend.app.core.database.invalidate_all_sessions", new_callable=AsyncMock),
                 open(zip_path, "rb") as f,
                 open(zip_path, "rb") as f,
             ):
             ):
                 resp = await async_client.post(
                 resp = await async_client.post(

+ 32 - 0
backend/tests/unit/test_code_quality.py

@@ -287,3 +287,35 @@ class TestLogErrorPatterns:
             client._process_message(msg)
             client._process_message(msg)
 
 
         assert not capture_logs.has_errors(), f"Errors during MQTT processing:\n{capture_logs.format_errors()}"
         assert not capture_logs.has_errors(), f"Errors during MQTT processing:\n{capture_logs.format_errors()}"
+
+
+class TestDateTimeColumns:
+    """No model column may be DateTime(timezone=True).
+
+    On PostgreSQL every bound datetime reaches asyncpg naive (the engine strips
+    tzinfo), and asyncpg reads a naive value for a timestamptz bind as
+    process-local time. With TZ set on the host such a column is stored shifted
+    by the UTC offset. Every column is plain DateTime holding naive UTC.
+    """
+
+    def test_no_timezone_aware_datetime_columns(self):
+        import importlib
+        import pkgutil
+
+        from sqlalchemy import DateTime
+
+        import backend.app.models
+        from backend.app.core.database import Base
+
+        # The package __init__ does not import every model module, so load them
+        # all here; otherwise a table outside it would never be checked.
+        for module in pkgutil.iter_modules(backend.app.models.__path__):
+            importlib.import_module(f"backend.app.models.{module.name}")
+
+        offenders = [
+            f"{table.name}.{column.name}"
+            for table in Base.metadata.sorted_tables
+            for column in table.columns
+            if isinstance(column.type, DateTime) and column.type.timezone
+        ]
+        assert not offenders, f"DateTime(timezone=True) columns: {offenders}"

+ 82 - 0
backend/tests/unit/test_invalidate_all_sessions.py

@@ -0,0 +1,82 @@
+"""invalidate_all_sessions signs every user out and drops pending auth tokens.
+
+It runs once after the auth datetime columns are converted on PostgreSQL and
+after every backup restore, where the rows may come from older code.
+"""
+
+import sqlite3
+from datetime import datetime, timedelta, timezone
+from unittest.mock import AsyncMock, patch
+
+import pytest
+from sqlalchemy import select
+
+from backend.app.core.auth import _is_token_fresh
+from backend.app.core.config import settings as app_settings
+from backend.app.core.database import invalidate_all_sessions
+from backend.app.models.auth_ephemeral import AuthEphemeralToken, AuthRateLimitEvent, EventType, TokenType
+from backend.app.models.user import User
+
+
+@pytest.mark.asyncio
+async def test_signs_every_user_out_and_drops_short_lived_auth_records(test_engine, db_session):
+    db_session.add_all(
+        [
+            User(username="changed", password_hash="x", password_changed_at=datetime(2026, 1, 1)),
+            User(username="never_changed", password_hash="x", password_changed_at=None),
+        ]
+    )
+    expires = datetime.now(timezone.utc) + timedelta(hours=1)
+    for token_type in ("revoked_jti", "media", "camera_stream", "websocket", *[t.value for t in TokenType]):
+        db_session.add(AuthEphemeralToken(token=f"t-{token_type}", token_type=token_type, expires_at=expires))
+    for event_type in EventType:
+        db_session.add(AuthRateLimitEvent(username="changed", event_type=event_type.value))
+    await db_session.commit()
+
+    before = datetime.now(timezone.utc)
+    async with test_engine.begin() as conn:
+        await invalidate_all_sessions(conn)
+
+    db_session.expire_all()
+    users = (await db_session.execute(select(User))).scalars().all()
+    assert {u.username for u in users} == {"changed", "never_changed"}
+    for user in users:
+        # A token issued before the call is rejected, one issued after it is not.
+        assert _is_token_fresh(int(before.timestamp()) - 1, user) is False
+        assert _is_token_fresh(int(before.timestamp()) + 2, user) is True
+
+    remaining = (await db_session.execute(select(AuthEphemeralToken.token_type))).scalars().all()
+    assert remaining == ["revoked_jti"]
+    assert (await db_session.execute(select(AuthRateLimitEvent))).scalars().all() == []
+
+
+@pytest.mark.asyncio
+async def test_restore_ends_by_invalidating_all_sessions(async_client, monkeypatch, tmp_path):
+    from backend.app.api.routes.settings import create_backup_zip
+
+    # Keep the restored database separate from the test engine.
+    db_path = tmp_path / "bambuddy.db"
+    with sqlite3.connect(db_path) as db:
+        db.execute("CREATE TABLE backup_marker (id INTEGER PRIMARY KEY)")
+    monkeypatch.setenv("DATA_DIR", str(tmp_path))
+    monkeypatch.setattr(app_settings, "base_dir", tmp_path)
+    monkeypatch.setattr(app_settings, "database_url", f"sqlite+aiosqlite:///{db_path}")
+    zip_path, _ = await create_backup_zip(output_path=tmp_path)
+
+    invalidate = AsyncMock()
+    with (
+        patch("backend.app.core.database.close_all_connections", new_callable=AsyncMock),
+        patch("backend.app.core.database.reinitialize_database", new_callable=AsyncMock),
+        patch("backend.app.core.database.init_db", new_callable=AsyncMock),
+        patch("backend.app.core.database.invalidate_all_sessions", invalidate),
+        patch("backend.app.services.print_scheduler.scheduler.stop"),
+        patch("backend.app.services.smart_plug_manager.smart_plug_manager.stop_scheduler"),
+        patch("backend.app.services.notification_service.notification_service.stop_digest_scheduler"),
+    ):
+        response = await async_client.post(
+            "/api/v1/settings/restore",
+            files={"file": ("backup.zip", zip_path.read_bytes(), "application/zip")},
+        )
+
+    assert response.status_code == 200, response.text
+    invalidate.assert_awaited_once()