diff --git a/src/backend/api/routers/settings.py b/src/backend/api/routers/settings.py index e63ede9..c755033 100644 --- a/src/backend/api/routers/settings.py +++ b/src/backend/api/routers/settings.py @@ -13,6 +13,42 @@ from src.backend.models.orm.user import User router = APIRouter(prefix="/api/settings", tags=["settings"]) +# Domains for the settings keys the backend actually reads. Values outside these +# would only fail later (csv.writer TypeError on close_run, etc) — reject up front. +_VALID_CSV_DELIMITERS = {",", ";", "\t", "|"} +_VALID_CSV_DECIMAL_SEPARATORS = {",", "."} + + +def _validate_setting_value(key: str, value: str) -> None: + """Raise 422 for a value that would break the code reading this key. + + Unrecognised keys pass through unvalidated: the settings store is generic. + """ + if key == "csv_delimiter": + if value not in _VALID_CSV_DELIMITERS: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="csv_delimiter deve essere un carattere singolo fra ',' ';' TAB '|'", + ) + elif key == "csv_decimal_separator": + if value not in _VALID_CSV_DECIMAL_SEPARATORS: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="csv_decimal_separator deve essere ',' o '.'", + ) + elif key == "auto_logout_minutes": + if value != "" and (not value.isdigit() or not (1 <= int(value) <= 480)): + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="auto_logout_minutes deve essere vuoto (disabilitato) o un intero fra 1 e 480", + ) + elif key in ("company_name", "company_logo_path"): + if not value.strip() or len(value) > 200: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"{key} deve essere una stringa non vuota di massimo 200 caratteri", + ) + @router.get("/") async def get_settings( @@ -49,6 +85,8 @@ async def update_settings( updated_keys = [] for key, value in settings_data.items(): + _validate_setting_value(key, value) + # Check if setting exists result = await db.execute( select(SystemSetting).where(SystemSetting.setting_key == key) diff --git a/src/backend/services/production_export_service.py b/src/backend/services/production_export_service.py index 0ea2b96..1c92160 100644 --- a/src/backend/services/production_export_service.py +++ b/src/backend/services/production_export_service.py @@ -11,6 +11,7 @@ files that open correctly in their Excel. """ import csv import io +import logging from datetime import datetime from pathlib import Path @@ -25,10 +26,18 @@ from src.backend.models.orm.setting import SystemSetting from src.backend.models.orm.station import Station from src.backend.models.orm.task import RecipeSubtask +logger = logging.getLogger(__name__) + # Written under the upload directory: it is the volume that is already mounted and # backed up, so the files survive a container being replaced. EXPORT_SUBDIR = "statistics" +# `csv.writer` requires a 1-character delimiter. PUT /api/settings/ validates this +# going forward (gitea#23), but a value written before that guard existed must not +# be able to make close_run fail: fall back and log instead of raising. +_VALID_CSV_DELIMITERS = {",", ";", "\t", "|"} +_FALLBACK_CSV_DELIMITER = ";" + HEADER = [ "production_run_id", "station_code", @@ -75,8 +84,17 @@ async def _csv_format(db: AsyncSession) -> tuple[str, str]: ) ) decimal = decimal_row.scalar_one_or_none() + + delimiter_value = delimiter.setting_value if delimiter else "," + if delimiter_value not in _VALID_CSV_DELIMITERS: + logger.warning( + "csv_delimiter non valido nel DB (%r): uso %r", + delimiter_value, _FALLBACK_CSV_DELIMITER, + ) + delimiter_value = _FALLBACK_CSV_DELIMITER + return ( - delimiter.setting_value if delimiter else ",", + delimiter_value, decimal.setting_value if decimal else ".", ) diff --git a/src/backend/tests/test_production_export_service.py b/src/backend/tests/test_production_export_service.py new file mode 100644 index 0000000..5e2ddfe --- /dev/null +++ b/src/backend/tests/test_production_export_service.py @@ -0,0 +1,57 @@ +"""Unit tests for production_export_service.py. + +- CSV format defenses (gitea#23): a dirty csv_delimiter read from the DB must + never make close_run fail with a csv.writer TypeError. +""" +from sqlalchemy.ext.asyncio import AsyncSession + +from src.backend.models.orm.setting import SystemSetting +from src.backend.services.production_export_service import _csv_format + + +class TestCsvFormatDefense: + """`_csv_format` must never hand csv.writer a delimiter it will reject.""" + + async def test_invalid_delimiter_in_db_falls_back_to_semicolon( + self, db_session: AsyncSession + ): + db_session.add( + SystemSetting( + setting_key="csv_delimiter", setting_value=";;", setting_type="string" + ) + ) + await db_session.flush() + + delimiter, _ = await _csv_format(db_session) + assert delimiter == ";" + + async def test_empty_delimiter_in_db_falls_back(self, db_session: AsyncSession): + db_session.add( + SystemSetting( + setting_key="csv_delimiter", setting_value="", setting_type="string" + ) + ) + await db_session.flush() + + delimiter, _ = await _csv_format(db_session) + assert delimiter == ";" + + async def test_valid_delimiter_passes_through_unchanged( + self, db_session: AsyncSession + ): + db_session.add( + SystemSetting( + setting_key="csv_delimiter", setting_value="|", setting_type="string" + ) + ) + await db_session.flush() + + delimiter, _ = await _csv_format(db_session) + assert delimiter == "|" + + async def test_no_delimiter_setting_defaults_to_comma( + self, db_session: AsyncSession + ): + delimiter, decimal = await _csv_format(db_session) + assert delimiter == "," + assert decimal == "." diff --git a/src/backend/tests/test_settings.py b/src/backend/tests/test_settings.py index bffd934..1ba48a1 100644 --- a/src/backend/tests/test_settings.py +++ b/src/backend/tests/test_settings.py @@ -105,3 +105,80 @@ class TestUpdateSettings: json={"some_key": "some_value"}, ) assert resp.status_code == 403 + + +class TestUpdateSettingsValidation: + """gitea#23: an invalid csv_delimiter must be rejected, not accepted and + left to blow up close_run later with a csv.writer TypeError.""" + + async def test_csv_delimiter_two_chars_rejected( + self, client: AsyncClient, admin_user: User + ): + resp = await client.put( + "/api/settings/", + headers=auth_headers(admin_user), + json={"csv_delimiter": ";;"}, + ) + assert resp.status_code == 422 + + async def test_csv_delimiter_empty_rejected( + self, client: AsyncClient, admin_user: User + ): + resp = await client.put( + "/api/settings/", + headers=auth_headers(admin_user), + json={"csv_delimiter": ""}, + ) + assert resp.status_code == 422 + + async def test_csv_delimiter_tab_accepted( + self, client: AsyncClient, admin_user: User + ): + resp = await client.put( + "/api/settings/", + headers=auth_headers(admin_user), + json={"csv_delimiter": "\t"}, + ) + assert resp.status_code == 200 + + async def test_csv_decimal_separator_invalid_rejected( + self, client: AsyncClient, admin_user: User + ): + resp = await client.put( + "/api/settings/", + headers=auth_headers(admin_user), + json={"csv_decimal_separator": "x"}, + ) + assert resp.status_code == 422 + + async def test_auto_logout_minutes_out_of_range_rejected( + self, client: AsyncClient, admin_user: User + ): + resp = await client.put( + "/api/settings/", + headers=auth_headers(admin_user), + json={"auto_logout_minutes": "0"}, + ) + assert resp.status_code == 422 + + async def test_auto_logout_minutes_empty_disables( + self, client: AsyncClient, admin_user: User + ): + """Empty string is what the settings page sends to disable auto-logout.""" + resp = await client.put( + "/api/settings/", + headers=auth_headers(admin_user), + json={"auto_logout_minutes": ""}, + ) + assert resp.status_code == 200 + + async def test_unknown_key_not_validated( + self, client: AsyncClient, admin_user: User + ): + """Keys with no known validator pass through (generic settings store).""" + resp = await client.put( + "/api/settings/", + headers=auth_headers(admin_user), + json={"some_future_key": "anything"}, + ) + assert resp.status_code == 200