From 0a70267692b0d888f0fcf2936d4dbf68a631c508 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 10:46:45 -0400 Subject: [PATCH] fix(api-v3): reject MQTT bridge bodies the endpoint cannot apply Two CodeRabbit findings on the bridge settings endpoint, both of which returned 200 while doing something other than what the caller asked. `request.get_json(silent=True) or {}` turned a missing or unparseable body -- and the JSON literals null, [] and false -- into an empty dict, which then satisfied the isinstance(data, dict) guard on the very next line. The guard was there to reject exactly those bodies. Dropping the `or {}` lets None fail it. The same `or {}` on /errors/clear is left alone: its docstring documents the body as optional, so an absent body legitimately means "use the defaults". The difference is that saving settings has nothing sensible to do with no body. `if data.get('clear_password'):` accepted any truthy value, and the string "false" is truthy in Python -- so a client echoing the field back as a string wiped a password it meant to keep. Now coerced through the package's existing _coerce_to_bool, which already maps 'true'/'on'/'1'/'yes' and nothing else. test_mqtt_bridge_config_endpoint.py covers both: five unusable body shapes plus a missing body, and clear_password across truthy and falsy spellings. Verified against the unfixed code -- reverting the body guard fails 5, reverting the coercion fails 3. Not changed here: CodeRabbit also asks this endpoint to reject MQTT credentials when TLS is off (CWE-319). That is a policy decision about the feature rather than a defect -- unencrypted MQTT on a trusted LAN is common and often deliberate -- so it is raised on the PR for a maintainer call instead. Co-Authored-By: Claude Opus 5 --- test/test_mqtt_bridge_config_endpoint.py | 97 ++++++++++++++++++++++++ web_interface/blueprints/api_v3/misc.py | 12 ++- 2 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 test/test_mqtt_bridge_config_endpoint.py diff --git a/test/test_mqtt_bridge_config_endpoint.py b/test/test_mqtt_bridge_config_endpoint.py new file mode 100644 index 00000000..1d2ae97e --- /dev/null +++ b/test/test_mqtt_bridge_config_endpoint.py @@ -0,0 +1,97 @@ +"""The MQTT bridge settings endpoint must reject bodies it cannot apply. + +Two defects CodeRabbit raised on #554, both of which reported success while +doing something other than what the caller asked: + +* `request.get_json(silent=True) or {}` turned a missing or unparseable body -- + and the JSON literals `null`, `[]` and `false` -- into an empty dict, which + then satisfied the `isinstance(data, dict)` guard directly below it. Malformed + JSON therefore returned 200 having applied nothing. + +* `if data.get('clear_password'):` accepted any truthy value. The string + "false" is truthy in Python, so a client echoing the field back as a string + wiped a stored password it meant to keep. +""" + +import json +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +from flask import Flask + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from web_interface.blueprints.api_v3 import api_v3 # noqa: E402 +import web_interface.blueprints.api_v3 as pkg # noqa: E402 +import web_interface.blueprints.api_v3.misc as misc # noqa: E402 + +URL = "/api/v3/integrations/mqtt-bridge/config" + + +@pytest.fixture +def client(tmp_path, monkeypatch): + cfg = tmp_path / "bridge_config.json" + # Both modules: misc.py writes through its own imported copies of these + # names, while _read_mqtt_bridge_config() lives in the package __init__ and + # reads the one bound there. Patching only one leaves the read and the + # write pointing at different files. + for mod in (misc, pkg): + monkeypatch.setattr(mod, "_MQTT_BRIDGE_CONFIG", cfg, raising=False) + monkeypatch.setattr(mod, "_MQTT_BRIDGE_DIR", tmp_path, raising=False) + monkeypatch.setattr(api_v3, "config_manager", MagicMock(), raising=False) + + app = Flask(__name__) + app.config["TESTING"] = True + app.register_blueprint(api_v3, url_prefix="/api/v3") + app.cfg_path = cfg + return app.test_client(), cfg + + +class TestABodyItCannotApplyIsRejected: + @pytest.mark.parametrize("raw", ["{ not json", "null", "[]", "false", '"a string"']) + def test_unusable_bodies_are_rejected(self, client, raw): + c, _ = client + r = c.put(URL, data=raw, content_type="application/json") + # The regression: every one of these returned 200 having applied nothing. + assert r.status_code == 400, f"{raw!r} was accepted" + assert "JSON object" in r.get_json()["message"] + + def test_a_missing_body_is_rejected(self, client): + c, _ = client + assert c.put(URL).status_code == 400 + + def test_a_real_object_is_still_accepted(self, client): + c, _ = client + r = c.put(URL, json={"mqtt_host": "192.168.1.20"}) + assert r.status_code == 200, r.get_json() + + +class TestClearPasswordNeedsARealBoolean: + def _seed(self, cfg, password="hunter2"): + cfg.write_text(json.dumps({"mqtt_password": password}), encoding="utf-8") + + def _stored(self, cfg): + return json.loads(cfg.read_text(encoding="utf-8")).get("mqtt_password") + + def test_the_string_false_does_not_clear_it(self, client): + c, cfg = client + self._seed(cfg) + assert c.put(URL, json={"clear_password": "false"}).status_code == 200 + # The regression: "false" is truthy, so this wiped the password. + assert self._stored(cfg) == "hunter2" + + @pytest.mark.parametrize("falsy", [False, "no", "0", "", None]) + def test_other_falsy_spellings_do_not_clear_it(self, client, falsy): + c, cfg = client + self._seed(cfg) + assert c.put(URL, json={"clear_password": falsy}).status_code == 200 + assert self._stored(cfg) == "hunter2" + + @pytest.mark.parametrize("truthy", [True, "true", "1", "yes", "on"]) + def test_real_truthy_values_still_clear_it(self, client, truthy): + c, cfg = client + self._seed(cfg) + assert c.put(URL, json={"clear_password": truthy}).status_code == 200 + assert self._stored(cfg) is None diff --git a/web_interface/blueprints/api_v3/misc.py b/web_interface/blueprints/api_v3/misc.py index 5989ffd8..ad88326c 100644 --- a/web_interface/blueprints/api_v3/misc.py +++ b/web_interface/blueprints/api_v3/misc.py @@ -5,6 +5,7 @@ Routes decorate the shared `api_v3` Blueprint from ._common, so their endpoint names are unchanged by living here. """ from web_interface.blueprints.api_v3 import ( + _coerce_to_bool, ErrorCode, Path, _JOURNALCTL, _MQTT_BRIDGE_CONFIG, _MQTT_BRIDGE_DEFAULTS, _MQTT_BRIDGE_DIR, _SUDO, _coerce_mqtt_bridge_value, _get_display_service_status, _mqtt_bridge_service_state, @@ -436,7 +437,11 @@ def update_mqtt_bridge_config(): have to blank it on every save. """ try: - data = request.get_json(silent=True) or {} + # No `or {}` here: get_json(silent=True) returns None for a missing or + # unparseable body, and `None or {}` produced an empty dict that then + # satisfied the isinstance check below -- so malformed JSON, `null`, + # `[]` and `false` all reported success while applying nothing. + data = request.get_json(silent=True) if not isinstance(data, dict): return jsonify({'status': 'error', 'message': 'Body must be a JSON object'}), 400 @@ -454,7 +459,10 @@ def update_mqtt_bridge_config(): config.update(updates) - if data.get('clear_password'): + # Coerced, not merely truthy: the string "false" is truthy in Python, + # so a client echoing the field back as a string would have wiped a + # stored password it meant to keep. + if _coerce_to_bool(data.get('clear_password')): config['mqtt_password'] = None elif 'mqtt_password' in data and str(data['mqtt_password']) != '': new_password = str(data['mqtt_password'])