From 40436c3381c71e644ad97272d8a8df91938d79da Mon Sep 17 00:00:00 2001 From: wind Date: Sat, 28 Feb 2026 19:41:01 +0100 Subject: [PATCH 1/3] fix: severity URL param, API validation, and json.loads guard (#46 #47 #48) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #46 — RisksPage reads initial severity filter from URL search params: - Import useSearchParams from react-router-dom - Initialise sevFilter from ?severity= query param on mount - Validate against SEVERITIES allowlist to prevent invalid state - Navigating to /risks?severity=critical now pre-filters the list #47 — Validate severity query param in /api/risks: - Change severity param type from str|None to Literal[critical|high|medium|low]|None - FastAPI/Pydantic now returns 422 on invalid values instead of silent empty list #48 — Guard json.loads in recommendations endpoint: - Add _safe_load_steps() helper that wraps json.loads in try/except - Empty/falsy steps returns [] gracefully - Malformed JSON logs a warning and returns [] instead of 500 - Added logging import alongside existing json import - Tests: 422 on invalid severity, malformed steps returns [], empty steps returns [] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- backend/app/api/__init__.py | 24 +++++++- backend/tests/test_api.py | 94 ++++++++++++++++++++++++++++++++ frontend/src/pages/RisksPage.tsx | 10 +++- 3 files changed, 123 insertions(+), 5 deletions(-) diff --git a/backend/app/api/__init__.py b/backend/app/api/__init__.py index 5e01006..13752ff 100644 --- a/backend/app/api/__init__.py +++ b/backend/app/api/__init__.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import Annotated +from typing import Annotated, Literal from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException from pydantic import BaseModel @@ -174,10 +174,13 @@ class RiskSummary(BaseModel): total: int +_SeverityParam = Literal["critical", "high", "medium", "low"] + + @router.get("/risks", response_model=list[RiskOut]) def list_risks( db: Annotated[Session, Depends(get_db)], - severity: str | None = None, + severity: _SeverityParam | None = None, device_id: int | None = None, ) -> list[RiskOut]: """Return risk findings, ordered by severity then detected_at. @@ -263,6 +266,9 @@ def _risk_to_out(r) -> RiskOut: # noqa: ANN001 — SQLAlchemy instance # ── /api/recommendations ────────────────────────────────────────────────────── import json as _json # noqa: E402 — after router definitions to keep imports grouped above +import logging as _logging # noqa: E402 — after router definitions to keep imports grouped above + +_api_logger = _logging.getLogger(__name__) class RecommendationOut(BaseModel): @@ -328,6 +334,18 @@ def device_recommendations( return [_rec_to_out(r) for r in recs] +def _safe_load_steps(raw: str | None) -> list[str]: + """Deserialise recommendation steps from JSON string; returns [] on failure.""" + if not raw: + return [] + try: + result = _json.loads(raw) + return result if isinstance(result, list) else [] + except _json.JSONDecodeError: + _api_logger.warning("Malformed steps JSON in recommendation: %r", raw[:120]) + return [] + + def _rec_to_out(r) -> RecommendationOut: # noqa: ANN001 — SQLAlchemy instance return RecommendationOut( id=r.id, @@ -337,7 +355,7 @@ def _rec_to_out(r) -> RecommendationOut: # noqa: ANN001 — SQLAlchemy instance severity=r.severity, title=r.title, description=r.description, - steps=_json.loads(r.steps), + steps=_safe_load_steps(r.steps), effort=r.effort, impact=r.impact, created_at=r.created_at.isoformat() if r.created_at else None, diff --git a/backend/tests/test_api.py b/backend/tests/test_api.py index dfcb84f..691574e 100644 --- a/backend/tests/test_api.py +++ b/backend/tests/test_api.py @@ -24,6 +24,7 @@ @pytest.fixture(scope="module") def db_engine(): import app.models.device # noqa: F401 — side-effect import registers ORM tables + import app.models.recommendation # noqa: F401 — side-effect import registers ORM tables import app.models.risk # noqa: F401 — side-effect import registers ORM tables import app.models.scan # noqa: F401 — side-effect import registers ORM tables from app.db import Base @@ -481,3 +482,96 @@ def test_list_risks_filter_by_severity_and_device_id(client, seeded_db, db_engin db2.execute(__import__("sqlalchemy").delete(Risk).where(Risk.id == risk_id)) db2.commit() db2.close() + + +# ── #47 severity query param validation ─────────────────────────────────────── + +def test_invalid_severity_returns_422(client): + """Invalid severity value must return 422, not silently return empty list.""" + response = client.get("/api/risks?severity=bogus") + assert response.status_code == 422 + + +def test_valid_severities_return_200(client): + """Each valid severity value must be accepted.""" + for sev in ("critical", "high", "medium", "low"): + response = client.get(f"/api/risks?severity={sev}") + assert response.status_code == 200, f"severity={sev} returned {response.status_code}" + + +# ── #48 malformed steps JSON in recommendations ─────────────────────────────── + +def test_malformed_steps_json_returns_empty_list(client, seeded_db, seeded_risk, db_engine): + """Recommendation with malformed steps JSON must return 200 with steps=[].""" + from sqlalchemy.orm import sessionmaker + from app.models.recommendation import Recommendation + + Session = sessionmaker(bind=db_engine) # noqa: N806 — sessionmaker convention; uppercase matches class naming + db = Session() + rec = Recommendation( + device_id=seeded_db["device_id"], + risk_id=seeded_risk["risk_id"], + check_id="test_check", + severity="low", + title="Test rec", + description="desc", + steps="NOT VALID JSON {{{", + effort="low", + impact="low", + ) + db.add(rec) + db.commit() + rec_id = rec.id + db.close() + + try: + response = client.get(f"/api/recommendations?device_id={seeded_db['device_id']}") + assert response.status_code == 200 + data = response.json() + target = next((r for r in data if r["id"] == rec_id), None) + assert target is not None + assert target["steps"] == [] + finally: + Session2 = sessionmaker(bind=db_engine) # noqa: N806 — sessionmaker convention; uppercase matches class naming + db2 = Session2() + db2.execute(__import__("sqlalchemy").delete(Recommendation).where(Recommendation.id == rec_id)) + db2.commit() + db2.close() + + +def test_empty_steps_returns_empty_list(client, seeded_db, seeded_risk, db_engine): + """Recommendation with empty-string steps must return 200 with steps=[].""" + from sqlalchemy.orm import sessionmaker + from app.models.recommendation import Recommendation + + Session = sessionmaker(bind=db_engine) # noqa: N806 — sessionmaker convention; uppercase matches class naming + db = Session() + rec = Recommendation( + device_id=seeded_db["device_id"], + risk_id=seeded_risk["risk_id"], + check_id="test_check_empty", + severity="low", + title="Test rec empty", + description="desc", + steps="", + effort="low", + impact="low", + ) + db.add(rec) + db.commit() + rec_id = rec.id + db.close() + + try: + response = client.get(f"/api/recommendations?device_id={seeded_db['device_id']}") + assert response.status_code == 200 + data = response.json() + target = next((r for r in data if r["id"] == rec_id), None) + assert target is not None + assert target["steps"] == [] + finally: + Session2 = sessionmaker(bind=db_engine) # noqa: N806 — sessionmaker convention; uppercase matches class naming + db2 = Session2() + db2.execute(__import__("sqlalchemy").delete(Recommendation).where(Recommendation.id == rec_id)) + db2.commit() + db2.close() diff --git a/frontend/src/pages/RisksPage.tsx b/frontend/src/pages/RisksPage.tsx index 58c7ddf..c880d64 100644 --- a/frontend/src/pages/RisksPage.tsx +++ b/frontend/src/pages/RisksPage.tsx @@ -3,11 +3,13 @@ * Route: /risks */ import React, { useMemo, useState } from "react"; -import { Link } from "react-router-dom"; +import { Link, useSearchParams } from "react-router-dom"; import { Card, Badge, SkeletonCard, PageHeader } from "../components"; import { useRisks, useRiskSummary, useDevices } from "../hooks"; import type { Risk, Severity } from "../types/api"; +const SEVERITIES: Severity[] = ["critical", "high", "medium", "low"]; + const SEV_LEVELS: Severity[] = ["critical", "high", "medium", "low"]; const SEV_COLOR: Record = { @@ -86,7 +88,11 @@ function RiskModal({ risk, onClose }: { risk: Risk; onClose: () => void }) { } export function RisksPage() { - const [sevFilter, setSevFilter] = useState(""); + const [searchParams] = useSearchParams(); + const initialSev = searchParams.get("severity"); + const [sevFilter, setSevFilter] = useState( + SEVERITIES.includes(initialSev as Severity) ? (initialSev as Severity) : "", + ); const [devFilter, setDevFilter] = useState(""); const [selectedRisk, setSelectedRisk] = useState(null); From 2b6cc0f01a1b1da762d5aba91285933f47fc9e01 Mon Sep 17 00:00:00 2001 From: wind Date: Sat, 28 Feb 2026 19:44:06 +0100 Subject: [PATCH 2/3] fix: ruff E501/I001 in new recommendation tests - Sort deferred imports (app before sqlalchemy) to satisfy I001 - Replace __import__('sqlalchemy') with explicit import to satisfy E501 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- backend/tests/test_api.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/backend/tests/test_api.py b/backend/tests/test_api.py index 691574e..e58df8f 100644 --- a/backend/tests/test_api.py +++ b/backend/tests/test_api.py @@ -503,8 +503,9 @@ def test_valid_severities_return_200(client): def test_malformed_steps_json_returns_empty_list(client, seeded_db, seeded_risk, db_engine): """Recommendation with malformed steps JSON must return 200 with steps=[].""" + import sqlalchemy + from app.models.recommendation import Recommendation # noqa: PLC0415 — deferred import from sqlalchemy.orm import sessionmaker - from app.models.recommendation import Recommendation Session = sessionmaker(bind=db_engine) # noqa: N806 — sessionmaker convention; uppercase matches class naming db = Session() @@ -534,15 +535,16 @@ def test_malformed_steps_json_returns_empty_list(client, seeded_db, seeded_risk, finally: Session2 = sessionmaker(bind=db_engine) # noqa: N806 — sessionmaker convention; uppercase matches class naming db2 = Session2() - db2.execute(__import__("sqlalchemy").delete(Recommendation).where(Recommendation.id == rec_id)) + db2.execute(sqlalchemy.delete(Recommendation).where(Recommendation.id == rec_id)) db2.commit() db2.close() def test_empty_steps_returns_empty_list(client, seeded_db, seeded_risk, db_engine): """Recommendation with empty-string steps must return 200 with steps=[].""" + import sqlalchemy + from app.models.recommendation import Recommendation # noqa: PLC0415 — deferred import from sqlalchemy.orm import sessionmaker - from app.models.recommendation import Recommendation Session = sessionmaker(bind=db_engine) # noqa: N806 — sessionmaker convention; uppercase matches class naming db = Session() @@ -572,6 +574,6 @@ def test_empty_steps_returns_empty_list(client, seeded_db, seeded_risk, db_engin finally: Session2 = sessionmaker(bind=db_engine) # noqa: N806 — sessionmaker convention; uppercase matches class naming db2 = Session2() - db2.execute(__import__("sqlalchemy").delete(Recommendation).where(Recommendation.id == rec_id)) + db2.execute(sqlalchemy.delete(Recommendation).where(Recommendation.id == rec_id)) db2.commit() db2.close() From 33b7cb5d6afdf42341b838415345046dc4893210 Mon Sep 17 00:00:00 2001 From: wind Date: Sat, 28 Feb 2026 19:46:58 +0100 Subject: [PATCH 3/3] style: ruff format test_api.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- backend/tests/test_api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/tests/test_api.py b/backend/tests/test_api.py index e58df8f..2d54156 100644 --- a/backend/tests/test_api.py +++ b/backend/tests/test_api.py @@ -486,6 +486,7 @@ def test_list_risks_filter_by_severity_and_device_id(client, seeded_db, db_engin # ── #47 severity query param validation ─────────────────────────────────────── + def test_invalid_severity_returns_422(client): """Invalid severity value must return 422, not silently return empty list.""" response = client.get("/api/risks?severity=bogus") @@ -501,6 +502,7 @@ def test_valid_severities_return_200(client): # ── #48 malformed steps JSON in recommendations ─────────────────────────────── + def test_malformed_steps_json_returns_empty_list(client, seeded_db, seeded_risk, db_engine): """Recommendation with malformed steps JSON must return 200 with steps=[].""" import sqlalchemy