mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-17 20:39:13 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
@@ -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'])
|
||||
|
||||
Reference in New Issue
Block a user