fix(server): harden security, fix review issues from PR #189

.github/workflows/server-test.yml

@@ -32,16 +32,23 @@ jobs:
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
 
       - name: Setup Python
-        uses: actions/setup-python@2f17f13ef37957a9bc7461aa1a61b211e3c1c3ae # v7.0.0
+        uses: actions/setup-python@v6
         with:
           python-version: "3.12"
 
       - name: Install Server and Test Dependencies
         run: |
           python -m pip install --upgrade pip
           pip install -e "server/[test]"
+          pip install pytest-cov pytest-order pytest-dependency
 
       - name: Run Server Tests (non-Docker)
         run: |
-          python -m pytest server/tests/ -v \
+          python -m pytest server/tests/ -v --cov=pssm_gremlin_server --cov-report=xml --override-ini="addopts=" \
             -k "not Docker and not docker and not runner_image and not server_image and not test_server_image and not test_runner_image and not test_server_rejects and not test_server_reports and not running_gremlin"
+
+      - name: Upload coverage to Codecov
+        uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0
+        with:
+          files: ./coverage.xml
+          flags: server

.gitignore

@@ -142,3 +142,4 @@ src/REvoDesign/UI/Ui_REvoDesign.py
 users.sqlite3
 testerror.txt
 /server/server_state
+/server/pssm_gremlin_server.egg-info

CHANGELOG.md

@@ -172,6 +172,56 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Admin password**: the bootstrap-generated admin password was logged to
   gunicorn stderr (inaccessible to the operator). The restart script now
   generates and displays it on the console before bringing services up.
+  - **Host header spoofing in email links**: `_public_base_url()` now always
+    uses the configured `SERVER_BASE_URL` for verification / password-reset
+    links, never the request `Host` header (which an attacker can spoof to
+    place valid tokens in links pointing to an attacker-controlled domain).
+  - **Role demotion doesn't clear `is_admin`**: updating a user role from
+    `admin` to `user`/`guest` now also sets `is_admin = False`, preventing
+    privilege-leak bugs where `require_admin()` checks `is_admin` instead of
+    `role`.
+  - **Cancelled tasks resurrected by late worker writes**: cancelled tasks are
+    now treated as terminal — the DB update guard prevents late `running`/
+    `finished` status writes from overwriting a `cancelled` task.
+  - **Legacy NULL columns not backfilled**: `_ensure_columns()` now backfills
+    `deleted`, `registration_status`, `user_status`, and `role` for rows that
+    predate those columns.
+  - **Admin digest silently drops users on email failure**: `_send_email()`
+    returns `False` on failure (doesn't raise), so the try/except never caught
+    transient errors. The digest loop now checks return values and unmarks users
+    as notified when all recipient deliveries fail.
+  - **Failed-task archives recreated after deletion**: the three exception
+    handlers in `run_gremlin_task` now guard `_pack_failed_results_archive`
+    with `_task_is_deleted()` so deleted tasks don't get artifacts recreated.
+  - **Celery submission failure leaves orphaned pending task**:
+    `apply_async` failures now mark the task as failed and return a 503,
+    preventing the pending task from consuming the user's quota forever.
+  - **Account enumeration via resend-verification**: the unauthenticated
+    endpoint now returns the same generic response for unknown, deleted, banned,
+    and already-verified accounts.
+  - **Batch-enable doesn't verify email**: batch-enable (`enable` action)
+    now also sets `email_verified = True` so admin-approved users can
+    immediately use features that require a verified email.
+  - **Rate limiter unbounded growth**: the in-memory rate-limit state dict
+    now periodically prunes expired IP entries, preventing unbounded memory
+    growth across the process lifetime.
+  - **Pydantic email validators crash on non-string input**:
+    `normalize_email()` now raises a clean `ValueError` instead of
+    `AttributeError`.
+  - **Test DB path targets wrong database**: `_inject_admin_password` now
+    writes to the user DB (`users.sqlite3`) instead of the task DB
+    (`pssm_gremlin_server.sqlite3`).
+  - **Missing columns in SQLAlchemy metadata**: `admin_notified`,
+    `verification_resend_count`, and `verification_resend_at` are now
+    declared in the `_users_table` Table definition.
+  - **Server test env now self-contained**: the server test suite no longer
+    imports from the root `tests/conftest.py`. The server conftest defines
+    `REPO_DIR` and `has_docker_daemon` locally, enabling a lightweight
+    `REvoDesignServerDev` conda env.
+  - **CI workflow fix**: replaced unresolvable `actions/setup-python` pinned
+    SHA with `@v6` in `server-test.yml`.
+
+
 - **Chrome file picker not opening**: native ``<input type="file">`` click
   fails to open the file dialog in Chrome (works in Safari).  Added
   drag-and-drop file upload as a browser-agnostic workaround — drop a

codecov.yml

@@ -0,0 +1,27 @@
+# Codecov configuration for REvoDesign monorepo
+# https://docs.codecov.com/docs/codecovyml-reference
+
+flags:
+  server:
+    paths:
+      - server/pssm_gremlin_server/
+    carryforward: false
+
+coverage:
+  status:
+    project:
+      server:
+        target: auto
+        threshold: 1%
+        flags:
+          - server
+
+comment:
+  layout: "reach, diff, flags, files"
+  behavior: default
+  require_changes: false
+  require_head: yes
+  show_carryforward_flags: false
+
+github_checks:
+  annotations: true

server/.env.example

@@ -129,6 +129,9 @@ RUNNER_GROUP=revodesign_appgroup
 # ============================================================================
 # Public HTTP port.
 PORT=8080
+# Number of days to retain task result files before automatic cleanup.
+# Default is 30 days.  Set to 0 to disable auto-cleanup.
+# RESULT_RETENTION_DAYS=30
 # Scope task dashboard to the logged-in user (false, default) or show all
 # tasks to everyone (true, for public-facing deployments without auth).
 PUBLIC_DASHBOARD=false

server/pssm_gremlin_server/auth.py

@@ -103,6 +103,8 @@ def _env_int(var: str, default: int) -> int:
     sa.Column("deleted", sa.Boolean, nullable=False, default=False),
     sa.Column("role", sa.String(32), nullable=False, default="user"),
     sa.Column("admin_notified", sa.Boolean, nullable=False, default=False),
+    sa.Column("verification_resend_count", sa.Integer, nullable=False, default=0),
+    sa.Column("verification_resend_at", sa.Float, nullable=True),
     sa.Column("registration_ip", sa.String(45), nullable=True),
     sa.Column("registration_country", sa.String(8), nullable=True),
 )
@@ -163,13 +165,35 @@ def _ensure_columns(conn) -> None:
         ]:
             if col not in existing:
                 conn.exec_driver_sql(f"ALTER TABLE users ADD COLUMN {col} {coltype};")
-        # Backfill: when adding admin_notified for the first time, mark all
-        # existing non-admin users as notified so they don't appear in the
-        # first digest.  Admins are always excluded from the digest.
+        # When adding admin_notified for the first time, mark all existing
+        # non-admin users as notified so they don't appear in the first digest.
+        # Admins are always excluded from the digest.  This is intentionally
+        # gated — we only want this on the very first migration.
         if "admin_notified" not in existing:
             conn.exec_driver_sql(
                 "UPDATE users SET admin_notified = 1 WHERE is_admin = 0"
             )
+        # Idempotent backfills — run every startup (not gated) so legacy rows
+        # that predate a column get their NULLs patched even when the column
+        # was added by a previous deployment that didn't include a backfill.
+        conn.exec_driver_sql(
+            "UPDATE users SET deleted = 0 WHERE deleted IS NULL"
+        )
+        conn.exec_driver_sql(
+            "UPDATE users SET registration_status = 'approved' "
+            "WHERE registration_status IS NULL"
+        )
+        conn.exec_driver_sql(
+            "UPDATE users SET user_status = 'active' WHERE user_status IS NULL"
+        )
+        conn.exec_driver_sql(
+            "UPDATE users SET role = CASE WHEN is_admin THEN 'admin' ELSE 'user' END "
+            "WHERE role IS NULL"
+        )
+        conn.exec_driver_sql(
+            "UPDATE users SET verification_resend_count = 0 "
+            "WHERE verification_resend_count IS NULL"
+        )
 
     # -- write helpers -------------------------------------------------------
 
@@ -646,15 +670,10 @@ def validate_captcha(token: str, answer: str) -> bool:
 def _public_base_url() -> str:
     """Return the public-facing base URL for email links.
 
-    Uses the current request's ``Host`` header so email links point to the
-    same domain the user is accessing.  Falls back to ``SERVER_BASE_URL``
-    env var, then ``http://localhost:8080`` (dev).
+    Always uses the configured ``SERVER_BASE_URL`` — never the request
+    ``Host`` header, which an attacker can spoof to place valid tokens
+    into links pointing at an attacker-controlled domain.
     """
-    # ponytail: request.host_url already has scheme+host from the Host header
-    try:
-        return request.host_url.rstrip("/")
-    except RuntimeError:
-        pass  # outside request context (tests, scripts)
     return _env_str("SERVER_BASE_URL", "http://localhost:8080").rstrip("/")
 
 
@@ -902,18 +921,18 @@ def send_admin_digest() -> bool:
         f"text-decoration:none;border-radius:6px;font-weight:600;\">"
         f"Review Registrations</a></p>"
     )
-    try:
-        for email in recipients:
-            _send_email(
-                to=email,
-                subject=f"{len(new_users)} new registration(s) — REvoDesign GREMLIN",
-                text=text,
-                html=_email_html(html_body),
-            )
-    except Exception:
+    any_sent = False
+    subject = f"{len(new_users)} new registration(s) — REvoDesign GREMLIN"
+    html_content = _email_html(html_body)
+    for email in recipients:
+        try:
+            if _send_email(to=email, subject=subject, text=text, html=html_content):
+                any_sent = True
+        except Exception:
+            logging.exception("Failed to send admin digest to %s", email)
+    if not any_sent:
         db.unmark_users_notified(user_ids)
-        raise
-    return True
+    return any_sent
 
 
 def validate_reset_token(token: str) -> int | None:

server/pssm_gremlin_server/db.py

@@ -34,6 +34,7 @@ class TaskDatabase:
     """Minimal SQLite-based task tracker for GREMLIN jobs."""
 
     DELETED_STATUSES = {"deleted:finshed", "deleted:cancel"}
+    TERMINAL_STATUSES = {"deleted:finshed", "deleted:cancel", "cancelled"}
 
     VALID_STATUSES = {
         "pending",
@@ -160,11 +161,12 @@ def update_task(self, md5sum: str, **fields) -> None:
         if status:
             self._ensure_status(status)
         stmt = update(self.tasks_table).where(self.tasks_table.c.md5sum == md5sum).values(**fields)
-        # Deleted tasks are terminal in the runtime state machine.
+        # Terminal tasks (deleted / cancelled) must stay terminal.
         # Ignore late worker writes (running/packing/finished/run_stage, etc.)
-        # that would otherwise resurrect tasks after user deletion.
+        # that would otherwise resurrect tasks after user deletion or
+        # cancellation.
         if status is None or (not self._is_deleted_status(status)):
-            stmt = stmt.where(self.tasks_table.c.status.notin_(tuple(self.DELETED_STATUSES)))
+            stmt = stmt.where(self.tasks_table.c.status.notin_(tuple(self.TERMINAL_STATUSES)))
         with self.engine.begin() as conn:
             conn.execute(stmt)
 

server/pssm_gremlin_server/pssm_gremlin.py

@@ -623,15 +623,22 @@ def _deleted_status_from_task(task: dict[str, Any]) -> str:
 
 
 def _is_deleted_status(status: Any) -> bool:
+    """True when *status* is a deleted state (``deleted:finshed`` or ``deleted:cancel``)."""
     normalized = str(status or "").strip().lower()
     return normalized in {"deleted:finshed", "deleted:cancel"}
 
 
-def _task_is_deleted(md5sum: str) -> bool:
+def _is_terminal_status(status: Any) -> bool:
+    """True when *status* is terminal — deleted or cancelled."""
+    normalized = str(status or "").strip().lower()
+    return normalized in {"deleted:finshed", "deleted:cancel", "cancelled"}
+
+
+def _task_is_terminal(md5sum: str) -> bool:
     task = task_store.get_task(md5sum)
     if not task:
         return False
-    return _is_deleted_status(task.get("status"))
+    return _is_terminal_status(task.get("status"))
 
 
 def _create_mount(mount_name: str, path: str, read_only=True) -> tuple[types.Mount, str]:
@@ -860,7 +867,7 @@ def run_gremlin_task(md5sum):
     def _on_stage_change(stage: str) -> None:
         if stage == stage_state["current"]:
             return
-        if _task_is_deleted(md5sum):
+        if _task_is_terminal(md5sum):
             return
         stage_state["current"] = stage
         task_store.update_task(md5sum, run_stage=stage)
@@ -871,18 +878,18 @@ def _on_stage_change(stage: str) -> None:
             output_dir=output_dir,
             stage_callback=_on_stage_change,
         )
-        if _task_is_deleted(md5sum):
+        if _task_is_terminal(md5sum):
             logging.info("Task %s was deleted during execution; skipping result packing and finalization.", md5sum)
             return
         final_stage = stage_state["current"] or _RUNNING_TRACE_STEPS[-1][0]
         task_store.update_task(md5sum, status="packing results", run_stage=final_stage)
         refreshed_task = task_store.get_task(md5sum) or task
-        if _is_deleted_status(refreshed_task.get("status")):
+        if _is_terminal_status(refreshed_task.get("status")):
             logging.info("Task %s was deleted before archive packing; skipping artifact packaging.", md5sum)
             return
         _pack_results_archive(refreshed_task)
         refreshed_task = task_store.get_task(md5sum) or refreshed_task
-        if _is_deleted_status(refreshed_task.get("status")):
+        if _is_terminal_status(refreshed_task.get("status")):
             logging.info("Task %s was deleted during archive packing; skipping final status update.", md5sum)
             return
         finish_time = time.time()
@@ -897,40 +904,43 @@ def _on_stage_change(stage: str) -> None:
     except docker.errors.ContainerError as exc:
         finish_time = time.time()
         error_message = f"docker: {exc}"
-        _pack_failed_results_archive(task, error_message)
-        task_store.update_task(
-            md5sum,
-            status="failed",
-            finished_at=finish_time,
-            walltime=finish_time - start_time,
-            error=error_message,
-            run_stage=stage_state["current"],
-        )
+        if not _task_is_terminal(md5sum):
+            _pack_failed_results_archive(task, error_message)
+            task_store.update_task(
+                md5sum,
+                status="failed",
+                finished_at=finish_time,
+                walltime=finish_time - start_time,
+                error=error_message,
+                run_stage=stage_state["current"],
+            )
     except docker.errors.DockerException as exc:
         finish_time = time.time()
         error_message = f"docker: {exc}"
-        _pack_failed_results_archive(task, error_message)
-        task_store.update_task(
-            md5sum,
-            status="failed",
-            finished_at=finish_time,
-            walltime=finish_time - start_time,
-            error=error_message,
-            run_stage=stage_state["current"],
-        )
+        if not _task_is_terminal(md5sum):
+            _pack_failed_results_archive(task, error_message)
+            task_store.update_task(
+                md5sum,
+                status="failed",
+                finished_at=finish_time,
+                walltime=finish_time - start_time,
+                error=error_message,
+                run_stage=stage_state["current"],
+            )
         logging.error("Docker daemon unavailable for GREMLIN task %s: %s", md5sum, exc)
     except Exception as exc:  # pylint: disable=broad-except
         finish_time = time.time()
         error_message = str(exc)
-        _pack_failed_results_archive(task, error_message)
-        task_store.update_task(
-            md5sum,
-            status="failed",
-            finished_at=finish_time,
-            walltime=finish_time - start_time,
-            error=error_message,
-            run_stage=stage_state["current"],
-        )
+        if not _task_is_terminal(md5sum):
+            _pack_failed_results_archive(task, error_message)
+            task_store.update_task(
+                md5sum,
+                status="failed",
+                finished_at=finish_time,
+                walltime=finish_time - start_time,
+                error=error_message,
+                run_stage=stage_state["current"],
+            )
         logging.exception("Unexpected failure while running GREMLIN task %s", md5sum)