fix: harden input handling, mock wiring, and config payload (v0.0.36)

CHANGELOG.md

@@ -2,6 +2,30 @@
 
 All notable changes to this project will be documented in this file.
 
+## v0.0.36
+
+### Security / Hardening
+- **fix: user input in `get_calibration_history()` was not URL-encoded** — the `sensor` argument was concatenated raw into the request URL. A value containing spaces, `#`, `&`, or other reserved characters produced a malformed URL or injected an extra query parameter (`sensor="pH&x=1"` → `?pH&x=1`). The value is now percent-encoded with `urllib.parse.quote(safe="")`, consistent with the encoding already applied in `set_switch_state()`.
+- **fix: `get_log()` no longer accepts arbitrary `log_type` values** — the `log_type` argument is now validated against the documented set (`actions`, `switching`, `onewire`) and raises `VioletPoolAPIError` for anything else, preventing untrusted values from reaching the request URL.
+- **fix: `restore_calibration()` now sanitizes `sensor` and `timestamp`** — previously these caller-provided strings were placed into the POST form data without length bounds or character filtering, unlike every key in `set_config()`. Both are now run through `InputSanitizer.sanitize_string()`.
+
+### Fixes
+- **fix: integer config values were coerced to float on the wire** — `_sanitize_config_payload()` routed every `int`/`float` through `InputSanitizer.sanitize_numeric()`, which always returns a `float`. An integer flag like `1` was therefore form-encoded as `1.0`, and `{"DOS_*_use": 1}` (used by `set_dosage_enabled()`) sent `1.0`/`0.0`. Integer and boolean values are now preserved as integers (`bool` → `0`/`1`).
+- **fix: `set_config()` was the only state-changing POST that retried on 5xx** — all other POSTs (`triggerManualDosing`, `setDosingParameters`, …) are non-retryable by default, and `test_manual_dosing_post_is_not_retried` codifies that contract. `set_config()` silently opted into `retryable=True`, so a transient 5xx after the controller had already applied the payload would re-send it. The override is removed; `set_config` is now non-retryable like the rest.
+- **fix: mock server `set_omni_position` handler was never wired** — `handle_set_omni_position` was defined but `create_app()` never routed `/setFunctionManually` OMNI queries to it, so every `OMNI,OMNI_DC<N>` request fell through to the generic handler. This returned `OK\nOMNI\n...` (instead of the documented `OK\nOMNITRONIC\n...`) and never updated `omni_position` in `/mock/state`. The OMNI case is now dispatched from `handle_set_function_manually`, and the smoke test covers it end-to-end.
+
+### Cleanup
+- **refactor: remove duplicate `InputSanitizer.validate_duration` / `validate_speed`** — these static methods collided by name with the raising validators in `_api_model` but silently clamped values instead, and were unused. Removed to avoid the foot-gun; the canonical `validate_duration` is the one re-exported from the package root.
+- **refactor: correct `duration` / `last_value` type hints** — annotated as `float | None` even though `validate_duration()` rejects non-whole numbers and `_build_manual_command()` truncates `last_value` via `int()`. Now typed as `int | None` to match actual behaviour.
+- **style: fix mojibake** (`ÔåÆ` → `→`) in docstrings/comments and drop redundant `except (Exception):` parentheses.
+
+### Installation
+```bash
+pip install violet-poolController-api==0.0.36
+```
+
+---
+
 ## v0.0.35
 
 ### Fixes

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "violet-poolController-api"
-version = "0.0.35"
+version = "0.0.36"
 authors = [
   { name="Basti (Xerolux)", email="git@xerolux.de" },
 ]

tests/mock_server.py

@@ -462,6 +462,9 @@ async def handle_set_function_manually(request: web.Request) -> web.Response:
     if not query:
         return web.Response(text="ERROR\nMissing query parameter")
 
+    if query.startswith("OMNI,"):
+        return await handle_set_omni_position(request)
+
     parts = query.split(",")
     key = parts[0]
     action = parts[1] if len(parts) > 1 else "ON"
@@ -737,11 +740,12 @@ async def handle_get_service_states(request: web.Request) -> web.Response:
 
 # OmniTronic multi-port valve state — driven by /setFunctionManually?OMNI,OMNI_DC<N>.
 async def handle_set_omni_position(request: web.Request) -> web.Response:
-    """GET /setFunctionManually?OMNI,OMNI_DC<N> — drive the multi-port valve."""
-    _log_request(request)
-    await _maybe_delay()
-    query = request.url.query.get("query", "") or request.url.path_qs.split("?", 1)[-1]
-    # Parse "OMNI,OMNI_DC<N>,0,0"
+    """GET /setFunctionManually?OMNI,OMNI_DC<N> — drive the multi-port valve.
+
+    Called from ``handle_set_function_manually`` once the OMNI prefix is
+    detected, so request logging and delay have already been applied.
+    """
+    query = request.query_string
     parts = query.split(",")
     if len(parts) < 2 or parts[0] != "OMNI":
         return web.Response(text="ERROR\nINVALID_QUERY", status=400)
@@ -932,7 +936,7 @@ async def auth_middleware(
         try:
             decoded = base64.b64decode(auth_header[6:]).decode("utf-8")
             username, password = decoded.split(":", 1)
-        except (Exception):
+        except Exception:
             _LOGGER.warning("AUTH REJECT: malformed Basic auth from %s", request.remote)
             return web.Response(status=401, text="Unauthorized")
 

tests/test_api.py

@@ -527,7 +527,7 @@ async def test_ext1_readings_filtered_when_not_detected(mock_aioresponse, api_cl
         payload={
             "getReadings": {
                 "PUMPSTATE": "2",
-                # No SYSTEM_ext1module_alive_count  module not connected
+                # No SYSTEM_ext1module_alive_count → module not connected
                 "EXT1_1": 0,
                 "EXT1_2": 0,
             }
@@ -1900,3 +1900,152 @@ def test_state_translation_language_switch() -> None:
 
     with pytest.raises(ValueError, match="Unsupported language"):
         set_state_translation_language("fr")
+
+
+@pytest.mark.asyncio
+async def test_set_config_preserves_int_values(
+    api_client: VioletPoolAPI,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Integer config values must be sent as integers, not coerced to float.
+
+    Regression for v0.0.36: ``InputSanitizer.sanitize_numeric`` returns a
+    float, so an integer flag like ``1`` became ``1.0`` on the wire. The
+    controller firmware expects integer flags (e.g. ``DOS_*_use``).
+    """
+    captured: dict[str, object] = {}
+
+    async def fake_request(endpoint: str, **kwargs: Any) -> str:  # noqa: ANN401
+        captured["data"] = kwargs.get("data")
+        return "OK"
+
+    monkeypatch.setattr(api_client, "_request", fake_request)
+
+    await api_client.set_config({"FLAG_enabled": 1, "FLAG_disabled": 0, "ph_set": 7.4})
+
+    data = captured["data"]
+    assert data == {"FLAG_enabled": 1, "FLAG_disabled": 0, "ph_set": 7.4}
+    assert isinstance(data["FLAG_enabled"], int)
+    assert isinstance(data["ph_set"], float)
+
+
+@pytest.mark.asyncio
+async def test_set_config_preserves_bool_as_int_flag(
+    api_client: VioletPoolAPI,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Booleans are sent as 0/1 integer flags (bool is an int subclass)."""
+    captured: dict[str, object] = {}
+
+    async def fake_request(endpoint: str, **kwargs: Any) -> str:  # noqa: ANN401
+        captured["data"] = kwargs.get("data")
+        return "OK"
+
+    monkeypatch.setattr(api_client, "_request", fake_request)
+
+    await api_client.set_config({"DOS_1_use": True, "DOS_2_use": False})
+
+    data = captured["data"]
+    assert data == {"DOS_1_use": 1, "DOS_2_use": 0}
+
+
+@pytest.mark.asyncio
+async def test_set_config_not_retried_on_server_error(
+    mock_aioresponse: aioresponses,
+    api_client: VioletPoolAPI,
+) -> None:
+    """set_config must not retry on 5xx — consistent with other POSTs.
+
+    A duplicated POST could re-apply a configuration change. POST methods
+    default to non-retryable; set_config previously opted into retries.
+    """
+    url = "http://192.168.1.100/setConfig"
+    mock_aioresponse.post(url, status=500, body="server error")
+
+    with pytest.raises(VioletPoolAPIError):
+        await api_client.set_config({"some_key": 1})
+
+    assert len(mock_aioresponse.requests[("POST", URL(url))]) == 1
+
+
+@pytest.mark.asyncio
+async def test_get_calibration_history_url_encodes_sensor(
+    api_client: VioletPoolAPI,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Sensor names with reserved characters must be percent-encoded.
+
+    Regression for v0.0.36: ``sensor`` was concatenated raw into the URL,
+    so a value containing ``&`` or space could inject extra query
+    parameters. The value passed to ``_request`` must be percent-encoded.
+    """
+    captured: dict[str, object] = {}
+
+    async def fake_request(endpoint: str, **kwargs: Any) -> str:  # noqa: ANN401
+        captured["query"] = kwargs.get("query")
+        return "2024-01-01 | 7.2 | pH_CALIB"
+
+    monkeypatch.setattr(api_client, "_request", fake_request)
+
+    await api_client.get_calibration_history("pH value & test")
+
+    assert captured["query"] == "pH%20value%20%26%20test"
+
+
+@pytest.mark.asyncio
+async def test_get_log_rejects_unknown_log_type(api_client: VioletPoolAPI) -> None:
+    """Unknown log_type values must be rejected before the request is sent."""
+    with pytest.raises(VioletPoolAPIError, match="Unsupported log_type"):
+        await api_client.get_log("drop_table")
+
+
+@pytest.mark.asyncio
+async def test_get_log_valid_log_type_reaches_request(
+    api_client: VioletPoolAPI,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Valid log_type is passed through to the request layer."""
+    captured: dict[str, object] = {}
+
+    async def fake_request(endpoint: str, **kwargs: Any) -> str:  # noqa: ANN401
+        captured["query"] = kwargs.get("query")
+        return "line one\nLOAD_MORE"
+
+    monkeypatch.setattr(api_client, "_request", fake_request)
+
+    result = await api_client.get_log("actions", page=0)
+
+    assert captured["query"] == "actions&0"
+    assert result["has_more"] is True
+    assert result["lines"] == ["line one"]
+
+
+@pytest.mark.asyncio
+async def test_restore_calibration_sanitizes_payload(
+    api_client: VioletPoolAPI,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """restore_calibration must sanitize sensor/timestamp before posting."""
+    captured: dict[str, object] = {}
+
+    async def fake_request(endpoint: str, **kwargs: Any) -> str:  # noqa: ANN401
+        captured["data"] = kwargs.get("data")
+        return "OK"
+
+    monkeypatch.setattr(api_client, "_request", fake_request)
+
+    await api_client.restore_calibration("pH", "2024-01-01 12:00")
+
+    assert captured["data"] == {"sensor": "pH", "timestamp": "2024-01-01 12:00"}
+
+
+def test_input_sanitizer_no_duplicate_validate_duration() -> None:
+    """InputSanitizer must not expose validate_duration/validate_speed.
+
+    These names collided with the raising validators in ``_api_model`` but
+    silently clamped instead. They were removed in v0.0.36.
+    """
+    from violet_poolcontroller_api.utils_sanitizer import InputSanitizer
+
+    assert not hasattr(InputSanitizer, "validate_duration")
+    assert not hasattr(InputSanitizer, "validate_speed")

tests/test_api_smoke.py

@@ -379,6 +379,21 @@ async def run_all_tests(api: VioletPoolAPI) -> None:
         check=lambda r: "not success" if not r.get("success") else None,
     )
 
+    # ── OmniTronic multi-port valve ─────────────────────────────────────
+    print("[OmniTronic]")
+
+    await _run(
+        "set_omni_position(2)",
+        api.set_omni_position(2),
+        check=lambda r: "missing OMNITRONIC ack" if "OMNITRONIC" not in r.get("response", "") else None,
+    )
+
+    await _run(
+        "set_omni_position(0) - filtration",
+        api.set_omni_position(0),
+        check=lambda r: "not success" if not r.get("success") else None,
+    )
+
     # ── PV Surplus ──────────────────────────────────────────────────────
     print("[PV Surplus]")
 

violet_poolcontroller_api/_api_dosing.py

@@ -37,7 +37,7 @@ async def _trigger_dosing(
         key: str,
         action: str,
         *,
-        duration: float | None = None,
+        duration: int | None = None,
     ) -> dict[str, Any]:
         """Trigger or stop a manual dosing run via /triggerManualDosing.
 
@@ -48,9 +48,9 @@ async def _trigger_dosing(
 
         Args:
             key: The dosing pump key (e.g. DOS_6_FLOC).
-            action: ON/START  DOSSTART; OFF/STOP/AUTO  DOSSTOP
+            action: ON/START → DOSSTART; OFF/STOP/AUTO → DOSSTOP
                 (stopping a run returns the channel to automatic mode).
-            duration: Duration in seconds.
+            duration: Duration in seconds (whole number).
 
         Returns:
             A dictionary with the command result.

violet_poolcontroller_api/_api_mixin.py

@@ -49,8 +49,8 @@ async def set_switch_state(
         key: str,
         action: str,
         *,
-        duration: float | None = None,
-        last_value: float | None = None,
+        duration: int | None = None,
+        last_value: int | None = None,
     ) -> dict[str, Any]:
         raise NotImplementedError
 
@@ -59,7 +59,7 @@ async def _trigger_dosing(
         key: str,
         action: str,
         *,
-        duration: float | None = None,
+        duration: int | None = None,
     ) -> dict[str, Any]:
         raise NotImplementedError
 
@@ -68,8 +68,8 @@ def _build_manual_command(
         key: str,
         action: str,
         *,
-        duration: float | None = None,
-        last_value: float | None = None,
+        duration: int | None = None,
+        last_value: int | None = None,
     ) -> str:
         raise NotImplementedError
 

violet_poolcontroller_api/_api_outputs.py

@@ -75,8 +75,8 @@ async def set_switch_state(
         key: str,
         action: str,
         *,
-        duration: float | None = None,
-        last_value: float | None = None,
+        duration: int | None = None,
+        last_value: int | None = None,
     ) -> dict[str, Any]:
         """Control a function output.
 
@@ -86,7 +86,7 @@ async def set_switch_state(
         Args:
             key: The device key.
             action: The action to perform (e.g., ON, OFF, AUTO).
-            duration: An optional duration for the action.
+            duration: An optional whole-number duration in seconds.
             last_value: An optional last value (e.g., speed).
 
         Returns:

violet_poolcontroller_api/_api_system.py

@@ -24,6 +24,8 @@
     ERROR_SEVERITY_INFO,
     ERROR_SEVERITY_REMINDER,
     ERROR_SEVERITY_WARNING,
+    LOG_TYPE_ACTIONS,
+    LOG_TYPES,
     SYSTEM_SERVICES,
 )
 
@@ -127,11 +129,23 @@ async def get_log(
             - ``has_more``: True when ``LOAD_MORE`` sentinel was present
             - ``raw``: the raw text response
 
+        Raises:
+            VioletPoolAPIError: If ``log_type`` is not one of the supported
+                values or the request fails.
+
         """
-        if page < 0 and log_type == "actions":
+        normalized = (log_type or "").strip()
+        if normalized not in LOG_TYPES:
+            msg = (
+                f"Unsupported log_type {log_type!r}. "
+                f"Expected one of: {sorted(LOG_TYPES)}"
+            )
+            raise VioletPoolAPIError(msg)
+
+        if page < 0 and normalized == LOG_TYPE_ACTIONS:
             query = "downloadActionsLog"
         else:
-            query = f"{log_type}&{page}"
+            query = f"{normalized}&{int(page)}"
 
         resp = await self._request(
             API_GET_LOG,