From e87797e997225d723b89097055d31396f4262f8f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 13:01:29 +0000 Subject: [PATCH] fix: bound the logo download, and stop malformed input reading as a fault Review findings on the coverage branch. The download size cap I added checked len(response.content), which has already buffered the whole body -- it stopped the bytes reaching disk but not memory, which was the point. A server that omits Content-Length and never stops sending would still exhaust the process. Stream it instead, counting as it arrives, into a sibling .part file that is replaced over the target only once it decodes. A transfer that dies midway now leaves nothing behind rather than a truncated logo for load_logo() to cache. The follower's control-message handler caught three exception types, but two reachable UDP payloads raise others: a bare JSON scalar makes msg.get() raise AttributeError, and an "sx" carrying a non-numeric x raises ValueError or TypeError from float(). Those escaped to the outer handler, skipping the legacy-PNG fallback and -- since this branch added a backoff there -- charging one malformed packet a 0.1s stall on the receive path. The legacy-PNG path also decoded without the dimension cap its TCP counterpart applies, so a crafted 65KB frame could force a large allocation on the render thread; both paths now share one constant. Three repo_url handlers called .strip() on client input without checking it was a string, so {"repo_url": 12345} answered 500. The credentials upload parsed the same file twice, the second time inside a bare except that a preceding parse had already made unreachable. And both raw-config handlers kept a json.JSONDecodeError arm that get_json(silent=True) had turned into dead code, collapsing "sent something unparseable" into "sent nothing" -- they now say which. Two of the new tests were not testing what they claimed. The pruning round-trip wrote ten backups inside one second, so all ten landed on the same int(time.time()) filename and overwrote each other; it never reached the limit it asserted. And the sync clock helper patched attributes on the stdlib time module, freezing time process-wide for every daemon thread earlier tests had left running. Full suite: 3352 passed, coverage 54%. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh --- src/common/logo_helper.py | 52 ++++--- src/common/sync_manager.py | 30 +++- test/test_api_v3_calendar_credentials.py | 24 +++- test/test_api_v3_registry_endpoints.py | 15 +- test/test_logo_helper.py | 87 +++++++++++- test/test_sync_manager.py | 140 ++++++++++++++++--- test/web_interface/test_api_v3_config_raw.py | 5 + web_interface/blueprints/api_v3.py | 55 +++++--- 8 files changed, 326 insertions(+), 82 deletions(-) diff --git a/src/common/logo_helper.py b/src/common/logo_helper.py index ea0134f2..c801f9cb 100644 --- a/src/common/logo_helper.py +++ b/src/common/logo_helper.py @@ -6,6 +6,7 @@ Extracted from LEDMatrix core to provide reusable functionality for plugins. """ import logging +import os from pathlib import Path from typing import Dict, List, Optional, Union @@ -271,32 +272,43 @@ class LogoHelper: decodable image before it is left on disk: a logo URL is remote input, and without this an oversized or malformed response would be cached for every later load_logo() call to trip over. + + The body is streamed and counted as it arrives rather than read + through response.content, which buffers the whole thing first — + a server that omits Content-Length and never stops sending would + exhaust memory before any size check could run. Nothing lands at + file_path until the download completes and decodes, so a failed + download cannot leave a truncated logo behind either. """ # Ensure directory exists with proper permissions ensure_directory_permissions(file_path.parent, get_assets_dir_mode()) - # Download with timeout - response = self.session.get(url, timeout=30) - response.raise_for_status() - - content = response.content - if len(content) > MAX_LOGO_BYTES: - raise ValueError( - f"Logo at {url} is {len(content)} bytes, over the " - f"{MAX_LOGO_BYTES}-byte limit; not saved") - - # Save to file - with open(file_path, 'wb') as f: - f.write(content) - - # Verify it decodes before leaving it on disk. PIL raises - # DecompressionBombError past its own pixel limit; a partial or - # non-image response raises UnidentifiedImageError/OSError. + tmp_path = file_path.with_name(file_path.name + '.part') try: - with Image.open(file_path) as probe: + with self.session.get(url, timeout=30, stream=True) as response: + response.raise_for_status() + downloaded = 0 + with open(tmp_path, 'wb') as f: + for chunk in response.iter_content(chunk_size=64 * 1024): + if not chunk: + continue + downloaded += len(chunk) + if downloaded > MAX_LOGO_BYTES: + raise ValueError( + f"Logo at {url} exceeds the " + f"{MAX_LOGO_BYTES}-byte limit; not saved") + f.write(chunk) + + # Verify it decodes before it becomes the cached logo. PIL + # raises DecompressionBombError past its own pixel limit; a + # partial or non-image response raises UnidentifiedImageError + # (an OSError subclass). + with Image.open(tmp_path) as probe: probe.load() - except Exception: - file_path.unlink(missing_ok=True) + + os.replace(tmp_path, file_path) + except BaseException: + tmp_path.unlink(missing_ok=True) raise # Set proper file permissions after saving diff --git a/src/common/sync_manager.py b/src/common/sync_manager.py index 41cef988..38239415 100644 --- a/src/common/sync_manager.py +++ b/src/common/sync_manager.py @@ -37,6 +37,13 @@ _RAW_MAGIC = b'SYNC_RAW' _RAW_HEADER = struct.Struct(' _MAX_W or img.height > _MAX_H: + if img.width > _MAX_FRAME_W or img.height > _MAX_FRAME_H: self.logger.warning( "Sync: rejected oversized scroll image %dx%d (max %dx%d) from %s", - img.width, img.height, _MAX_W, _MAX_H, addr, + img.width, img.height, _MAX_FRAME_W, _MAX_FRAME_H, addr, ) continue try: @@ -525,10 +531,26 @@ class DisplaySyncManager: # Leader started a new scroll cycle — rebuild local image if self._on_new_cycle: self._on_new_cycle() - except (json.JSONDecodeError, UnicodeDecodeError, KeyError): + except (json.JSONDecodeError, UnicodeDecodeError, KeyError, + AttributeError, TypeError, ValueError): # Not a control message — try legacy PNG frame. + # The tuple is wide because a UDP payload is + # attacker-shaped: valid-but-non-object JSON makes + # msg.get() raise AttributeError, and an "sx" with a + # non-numeric x raises ValueError/TypeError from + # float(). Those must land here, not in the outer + # handler, which would skip this fallback and pay + # the error backoff for one malformed packet. try: img = Image.open(io.BytesIO(data)) + if img.width > _MAX_FRAME_W or img.height > _MAX_FRAME_H: + # Same cap the TCP image path applies: decode + # is deferred until load(), so check first. + self.logger.debug( + "Sync: rejected oversized legacy frame %dx%d from %s", + img.width, img.height, sender_ip, + ) + continue img.load() self._handle_received_frame(img, sender_ip) except Exception as exc: diff --git a/test/test_api_v3_calendar_credentials.py b/test/test_api_v3_calendar_credentials.py index 6c24fa93..48274e01 100644 --- a/test/test_api_v3_calendar_credentials.py +++ b/test/test_api_v3_calendar_credentials.py @@ -22,6 +22,7 @@ import stat import sys import time from pathlib import Path +from types import SimpleNamespace import pytest @@ -192,14 +193,25 @@ class TestBackupPruning: upload(api_v3_client, VALID_CREDENTIALS) assert len(backups(plugin_dir)) == 3 # 2 seeded + 1 new - def test_repeated_uploads_stay_bounded(self, api_v3_client, plugin_dir): + def test_repeated_uploads_stay_bounded( + self, api_v3_client, plugin_dir, api_v3_module, monkeypatch): + # The backup filename carries int(time.time()), so uploads inside + # the same second all write the same name and overwrite each other. + # Advance a fake clock a second per round — otherwise this never + # reaches six backups and the bound holds for the wrong reason. + clock = {"now": int(time.time())} + monkeypatch.setattr( + api_v3_module, "time", SimpleNamespace(time=lambda: clock["now"])) for i in range(10): + clock["now"] += 1 upload(api_v3_client, {"installed": {"round": i}}) - # Distinct mtimes so ordering is well-defined between rounds. - for path in backups(plugin_dir): - os.utime(path, (path.stat().st_mtime, path.stat().st_mtime)) - time.sleep(0.01) - assert len(backups(plugin_dir)) <= 5 + os.utime(plugin_dir / "credentials.json", + (clock["now"], clock["now"])) + remaining = backups(plugin_dir) + assert len(remaining) == 5 + # And they are the five most recent rounds, not an arbitrary five. + kept = sorted(int(p.name.rsplit(".", 1)[1]) for p in remaining) + assert kept == [clock["now"] - 4 + i for i in range(5)] def test_unremovable_backup_does_not_fail_the_upload( self, api_v3_client, plugin_dir, monkeypatch): diff --git a/test/test_api_v3_registry_endpoints.py b/test/test_api_v3_registry_endpoints.py index e1968c39..85f4b377 100644 --- a/test/test_api_v3_registry_endpoints.py +++ b/test/test_api_v3_registry_endpoints.py @@ -166,9 +166,14 @@ class TestRegistryFromUrl: assert body["message"] == "An error occurred; see logs for details" assert "Traceback" not in str(body) - def test_non_string_repo_url_is_a_500_not_a_crash( - self, api_v3_client, api_v3_module): - # .strip() on a non-string raises; the handler's catch-all turns - # that into a 500 rather than propagating. + def test_non_string_repo_url_is_rejected(self, api_v3_client, api_v3_module): + # Regression: .strip() on a non-string raised, and the catch-all + # reported the caller's own mistake as a server fault. response = api_v3_client.post(self.URL, json={"repo_url": 12345}) - assert response.status_code == 500 + assert response.status_code == 400 + api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.assert_not_called() + + def test_blank_repo_url_is_rejected(self, api_v3_client, api_v3_module): + response = api_v3_client.post(self.URL, json={"repo_url": " "}) + assert response.status_code == 400 + api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.assert_not_called() diff --git a/test/test_logo_helper.py b/test/test_logo_helper.py index 44af75cf..9589f28f 100644 --- a/test/test_logo_helper.py +++ b/test/test_logo_helper.py @@ -21,7 +21,7 @@ from unittest.mock import MagicMock, patch import pytest import requests -from PIL import Image +from PIL import Image, UnidentifiedImageError from src.common.logo_helper import MAX_LOGO_BYTES, LogoHelper @@ -46,10 +46,44 @@ def write_logo(path: Path, size=(20, 20), color=(255, 0, 0), fmt="PNG") -> Path: return path -def fake_response(content: bytes): +def fake_response(content: bytes, chunk_size: int = 64 * 1024): + """Stand-in for a streamed requests.Response. + + _download_logo opens `with session.get(..., stream=True)` and reads + through iter_content(), so the fake has to be a context manager that + yields the body in pieces rather than exposing it as .content. + Chunking is the fake's own, not the caller's, so a test can dribble a + body out in small pieces. + """ response = MagicMock() - response.content = content + response.__enter__.return_value = response + response.__exit__.return_value = False response.raise_for_status = MagicMock() + + def _iter_content(*_args, **_kwargs): + for i in range(0, len(content), chunk_size): + yield content[i:i + chunk_size] + + response.iter_content = _iter_content + return response + + +def endless_response(chunk: bytes = b"\x00" * 65536): + """A server that declares no length and never stops sending. + + This is the case response.content could not survive: it buffers to + completion, so the size check never got a chance to run. + """ + response = MagicMock() + response.__enter__.return_value = response + response.__exit__.return_value = False + response.raise_for_status = MagicMock() + + def _iter_content(*_args, **_kwargs): + while True: + yield chunk + + response.iter_content = _iter_content return response @@ -169,7 +203,10 @@ class TestLoadLogoWithDownload: logo = helper.load_logo_with_download("PHI", path, "http://x/logo.png") assert logo is not None assert path.exists() - helper.session.get.assert_called_once_with("http://x/logo.png", timeout=30) + # stream=True is load-bearing: it is what lets the size cap apply + # before the body is buffered. + helper.session.get.assert_called_once_with( + "http://x/logo.png", timeout=30, stream=True) def test_download_failure_falls_back_to_placeholder(self, helper, tmp_path): helper.session.get = MagicMock( @@ -215,18 +252,56 @@ class TestDownloadLogo: path = tmp_path / "huge.png" helper.session.get = MagicMock( return_value=fake_response(b"\x00" * (MAX_LOGO_BYTES + 1))) - with pytest.raises(ValueError, match="over the"): + with pytest.raises(ValueError, match="exceeds the"): helper._download_logo("http://x/huge.png", path) assert not path.exists() + def test_unbounded_response_is_aborted_at_the_cap(self, helper, tmp_path): + # Regression: the cap used to be checked against response.content, + # which buffers the whole body first — so a server that omits + # Content-Length and never stops sending exhausted memory before + # the check could run. Streaming counts bytes as they arrive, so + # this terminates instead of hanging. + path = tmp_path / "endless.png" + helper.session.get = MagicMock(return_value=endless_response()) + with pytest.raises(ValueError, match="exceeds the"): + helper._download_logo("http://x/endless.png", path) + assert not path.exists() + + def test_no_partial_file_is_left_when_the_stream_dies(self, helper, tmp_path): + # A transfer that fails midway must not leave a truncated logo + # where the real one belongs — load_logo() would cache it. + path = tmp_path / "cut.png" + real = png_bytes() + + def _dies_midway(*_args, **_kwargs): + yield real[:20] + raise OSError("connection reset") + + response = MagicMock() + response.__enter__.return_value = response + response.__exit__.return_value = False + response.raise_for_status = MagicMock() + response.iter_content = _dies_midway + helper.session.get = MagicMock(return_value=response) + + with pytest.raises(OSError): + helper._download_logo("http://x/cut.png", path) + assert not path.exists() + assert list(tmp_path.glob("*.part")) == [] + def test_non_image_response_is_deleted_and_raises(self, helper, tmp_path): # Regression: undecodable bytes stayed on disk, so every later # load_logo() call hit the corrupt file instead of re-downloading. path = tmp_path / "bad.png" helper.session.get = MagicMock(return_value=fake_response(b"404")) - with pytest.raises(Exception): + # Specifically Pillow's identify failure, not any OSError: the + # point is that the bytes did not decode, and OSError alone would + # also admit unrelated filesystem faults. + with pytest.raises(UnidentifiedImageError): helper._download_logo("http://x/bad.png", path) assert not path.exists() + assert list(tmp_path.glob("*.part")) == [] def test_decompression_bomb_is_deleted_and_raises(self, helper, tmp_path, monkeypatch): path = tmp_path / "bomb.png" diff --git a/test/test_sync_manager.py b/test/test_sync_manager.py index daefaff6..2b816761 100644 --- a/test/test_sync_manager.py +++ b/test/test_sync_manager.py @@ -31,6 +31,7 @@ import socket import threading import time from pathlib import Path +from types import SimpleNamespace from unittest.mock import MagicMock, patch import numpy as np @@ -118,11 +119,27 @@ def raise_n_then_stop(mgr, exc, count): return _side_effect +def fake_clock(monkeypatch, *, time_fn=None, sleep_fn=None): + """Swap sync_manager's own `time` reference for a private stand-in. + + sync_manager.time IS the stdlib module, so patching attributes on it + would freeze the clock and no-op sleep for the whole process — + including the daemon threads earlier tests left running, which is a + hard-to-trace source of cross-test flakiness. Rebinding the module's + reference keeps the patch scoped to the code under test. Anything not + overridden falls through to the real functions. + """ + monkeypatch.setattr(sync_manager, "time", SimpleNamespace( + time=time_fn or time.time, + sleep=sleep_fn or time.sleep, + )) + + def run_watchdog_once(monkeypatch, mgr, watchdog, now): """Run exactly one watchdog iteration at a frozen wall-clock time.""" - monkeypatch.setattr(sync_manager.time, "time", lambda: now) - monkeypatch.setattr( - sync_manager.time, "sleep", lambda _: setattr(mgr, "_running", False)) + fake_clock(monkeypatch, + time_fn=lambda: now, + sleep_fn=lambda _: setattr(mgr, "_running", False)) mgr._running = True watchdog() @@ -334,11 +351,11 @@ class TestLeaderRecvLoop: assert mgr._leader_state is LeaderState.CONNECTED assert mgr._peer_ip == "10.0.0.8" - def test_heartbeat_from_known_peer_refreshes_timer(self): + def test_heartbeat_from_known_peer_refreshes_timer(self, monkeypatch): mgr = make_manager(role=SyncRole.LEADER) mgr._peer_ip = "10.0.0.8" - with patch.object(sync_manager.time, "time", return_value=12345.0): - self._drive(mgr, json.dumps({"t": "hb"}).encode()) + fake_clock(monkeypatch, time_fn=lambda: 12345.0) + self._drive(mgr, json.dumps({"t": "hb"}).encode()) assert mgr._last_heartbeat_time == 12345.0 def test_heartbeat_from_stranger_is_ignored(self): @@ -370,7 +387,7 @@ class TestLeaderRecvLoop: mgr._recv_sock = MagicMock() mgr._recv_sock.recvfrom.side_effect = raise_n_then_stop(mgr, OSError("boom"), 3) sleeps = MagicMock() - monkeypatch.setattr(sync_manager.time, "sleep", sleeps) + fake_clock(monkeypatch, sleep_fn=sleeps) mgr._running = True mgr._leader_recv_loop() assert sleeps.call_count == 3 @@ -470,6 +487,45 @@ class TestFollowerRecvLoop: self._drive(mgr, json.dumps({"t": "nc"}).encode()) assert calls == [1] + def test_non_object_json_does_not_reach_the_outer_handler(self): + # A bare JSON scalar parses, then msg.get() raises AttributeError. + # That has to be caught here so the payload still gets its shot at + # the legacy-PNG fallback; escaping to the outer handler would also + # charge one malformed packet the 0.1s error backoff. + mgr = make_manager(role=SyncRole.FOLLOWER) + sleeps = MagicMock() + with patch.object(sync_manager, "time", + SimpleNamespace(time=time.time, sleep=sleeps)): + self._drive(mgr, b"12345") + assert mgr.get_latest_frame() is None + sleeps.assert_not_called() + + def test_non_numeric_scroll_x_does_not_reach_the_outer_handler(self): + # float("a") raises ValueError; {"x": null} raises TypeError. + for payload in ({"t": "sx", "x": "a"}, {"t": "sx", "x": None}): + mgr = make_manager(role=SyncRole.FOLLOWER) + sleeps = MagicMock() + with patch.object(sync_manager, "time", + SimpleNamespace(time=time.time, sleep=sleeps)): + self._drive(mgr, json.dumps(payload).encode()) + assert mgr.get_latest_scroll_x() is None + sleeps.assert_not_called() + + def test_oversized_legacy_frame_is_rejected_before_decode(self, monkeypatch): + # The UDP path is reachable by any host on the LAN, so it caps + # dimensions before load() just as the TCP image server does. + mgr = make_manager(role=SyncRole.FOLLOWER) + + class Huge: + width, height = 10, sync_manager._MAX_FRAME_H + 1 + + def load(self): + raise AssertionError("load() must not run past the cap") + + monkeypatch.setattr(sync_manager.Image, "open", lambda *a, **kw: Huge()) + self._drive(mgr, b"\x89PNG not really but not JSON either") + assert mgr.get_latest_frame() is None + def test_scroll_x_missing_key_is_swallowed(self): mgr = make_manager(role=SyncRole.FOLLOWER) self._drive(mgr, json.dumps({"t": "sx"}).encode()) # no "x" @@ -480,7 +536,7 @@ class TestFollowerRecvLoop: mgr._recv_sock = MagicMock() mgr._recv_sock.recvfrom.side_effect = raise_n_then_stop(mgr, OSError("boom"), 3) sleeps = MagicMock() - monkeypatch.setattr(sync_manager.time, "sleep", sleeps) + fake_clock(monkeypatch, sleep_fn=sleeps) mgr._running = True mgr._follower_recv_loop() assert sleeps.call_count == 3 @@ -797,23 +853,71 @@ class TestStop: make_manager(role=SyncRole.STANDALONE).stop() # all sockets None -class TestLoopbackHandshake: +def _broadcast_works(port): + """True when this environment can actually deliver a UDP broadcast. + + The handshake below depends on it: the follower announces itself to + ("", port). Sandboxes and some CI networks drop or refuse + broadcast, and sync_manager swallows the sendto error, so without + this probe the test would just wait out its deadline and fail for a + reason that has nothing to do with the code. + """ + recv = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + send = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + try: + recv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + recv.bind(("", port)) + recv.settimeout(0.5) + send.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1) + send.sendto(b"probe", ("", port)) + return recv.recvfrom(64)[0] == b"probe" + except OSError: + return False + finally: + recv.close() + send.close() + + +class TestRealSocketHandshake: def test_leader_and_follower_negotiate_over_real_sockets(self, monkeypatch): # One end-to-end check that the wire format actually round-trips: # every other test drives the loops with mocked sockets. + # + # Not loopback-only, despite the free-port probe below: the manager + # binds UDP and TCP on all interfaces and the follower announces by + # broadcast. That is the behaviour under test, so the environment + # has to support it. monkeypatch.setattr(sync_manager, "HELLO_INTERVAL", 0.02) monkeypatch.setattr(sync_manager, "HEARTBEAT_INTERVAL", 0.02) - # Pick a free port by binding one on loopback and releasing it. - # Loopback, not "", so this test never opens a port to the network. - probe = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) - probe.bind(("127.0.0.1", 0)) - port = probe.getsockname()[1] - probe.close() - hw = {"rows": 32, "cols": 64, "chain_length": 1} - leader = DisplaySyncManager("leader", {"port": port}, hw, MagicMock()) - follower = DisplaySyncManager("follower", {"port": port}, hw, MagicMock()) + leader = follower = None + # The free-port probe is inherently racy — the port can be taken + # between release and rebind — so retry rather than fail on it. + for attempt in range(5): + probe = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + probe.bind(("", 0)) + port = probe.getsockname()[1] + probe.close() + + if not _broadcast_works(port): + pytest.skip("environment cannot deliver UDP broadcast") + + try: + leader = DisplaySyncManager("leader", {"port": port}, hw, MagicMock()) + follower = DisplaySyncManager("follower", {"port": port}, hw, MagicMock()) + break + except OSError: + # Port taken between probe and bind, or the TCP image + # server could not bind port+1. Tear down whichever end + # came up before retrying with a fresh port. + for mgr in (leader, follower): + if mgr is not None: + mgr.stop() + leader = follower = None + else: + pytest.skip("could not obtain a free port pair for the handshake") + try: deadline = time.time() + 5.0 while time.time() < deadline: diff --git a/test/web_interface/test_api_v3_config_raw.py b/test/web_interface/test_api_v3_config_raw.py index b01d6b2c..bceebd13 100644 --- a/test/web_interface/test_api_v3_config_raw.py +++ b/test/web_interface/test_api_v3_config_raw.py @@ -112,6 +112,11 @@ class TestSaveRawMain: assert response.status_code == 400 body = response.get_json() assert body["status"] == "error" + # A body that was sent but does not parse is a distinct mistake + # from sending none, and says so. Previously the handler's own + # json.JSONDecodeError arm was unreachable — Werkzeug raised + # first — so this collapsed into "No data provided". + assert "Invalid JSON in request body" in body["message"] def test_config_error_is_a_500_with_context(self, env, monkeypatch): def refuse(kind, data): diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index e5f7d44c..286fa0db 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -1345,18 +1345,20 @@ def save_raw_main_config(): if not api_v3.config_manager: return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500 + # silent=True so a malformed body returns None instead of raising + # Werkzeug's own BadRequest, which would answer in a different + # shape than this API's. Distinguish the two causes: a body that + # was sent but does not parse is a different mistake from no body. data = request.get_json(silent=True) + if data is None and request.get_data(): + return jsonify({'status': 'error', 'message': 'Invalid JSON in request body'}), 400 if not data: return jsonify({'status': 'error', 'message': 'No data provided'}), 400 - # Validate that it's valid JSON (already parsed by request.get_json()) # Save the raw config file api_v3.config_manager.save_raw_file_content('main', data) return jsonify({'status': 'success', 'message': 'Main configuration saved successfully'}) - except json.JSONDecodeError as e: - logger.error('Invalid JSON', exc_info=True) - return jsonify({'status': 'error', 'message': 'Invalid JSON in request body'}), 400 except Exception as e: from src.exceptions import ConfigError logger.error("Error saving raw main config", exc_info=True) @@ -1391,7 +1393,11 @@ def save_raw_secrets_config(): if not api_v3.config_manager: return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500 + # See save_raw_main_config: silent parsing, with a sent-but-broken + # body reported separately from a missing one. data = request.get_json(silent=True) + if data is None and request.get_data(): + return jsonify({'status': 'error', 'message': 'Invalid JSON in request body'}), 400 if not data: return jsonify({'status': 'error', 'message': 'No data provided'}), 400 @@ -1403,9 +1409,6 @@ def save_raw_secrets_config(): api_v3.plugin_store_manager.github_token = api_v3.plugin_store_manager._load_github_token() return jsonify({'status': 'success', 'message': 'Secrets configuration saved successfully'}) - except json.JSONDecodeError as e: - logger.error('Invalid JSON', exc_info=True) - return jsonify({'status': 'error', 'message': 'Invalid JSON in request body'}), 400 except Exception as e: from src.exceptions import ConfigError logger.error("Error saving raw secrets config", exc_info=True) @@ -3975,6 +3978,11 @@ def install_plugin_from_url(): if not data or 'repo_url' not in data: return jsonify({'status': 'error', 'message': 'repo_url required'}), 400 + # A non-string repo_url is a client mistake, not a server fault: + # .strip() would raise and the catch-all would report it as a 500. + if not isinstance(data['repo_url'], str) or not data['repo_url'].strip(): + return jsonify({'status': 'error', 'message': 'repo_url must be a non-empty string'}), 400 + repo_url = data['repo_url'].strip() plugin_id = data.get('plugin_id') # Optional, for monorepo installations plugin_path = data.get('plugin_path') # Optional, for monorepo subdirectory @@ -4030,6 +4038,11 @@ def get_registry_from_url(): if not data or 'repo_url' not in data: return jsonify({'status': 'error', 'message': 'repo_url required'}), 400 + # A non-string repo_url is a client mistake, not a server fault: + # .strip() would raise and the catch-all would report it as a 500. + if not isinstance(data['repo_url'], str) or not data['repo_url'].strip(): + return jsonify({'status': 'error', 'message': 'repo_url must be a non-empty string'}), 400 + repo_url = data['repo_url'].strip() # Get registry from the URL @@ -4075,6 +4088,11 @@ def add_saved_repository(): if not data or 'repo_url' not in data: return jsonify({'status': 'error', 'message': 'repo_url required'}), 400 + # A non-string repo_url is a client mistake, not a server fault: + # .strip() would raise and the catch-all would report it as a 500. + if not isinstance(data['repo_url'], str) or not data['repo_url'].strip(): + return jsonify({'status': 'error', 'message': 'repo_url must be a non-empty string'}), 400 + repo_url = data['repo_url'].strip() name = data.get('name') @@ -7289,25 +7307,16 @@ def upload_calendar_credentials(): try: file_content = file.read() file.seek(0) - json.loads(file_content) + creds_data = json.loads(file_content) except json.JSONDecodeError: return jsonify({'status': 'error', 'message': 'File is not valid JSON'}), 400 - # Validate it looks like Google OAuth credentials. The content - # already parsed as JSON above, so anything raising here means it is - # not credentials-shaped — a bare scalar, for instance, where the - # membership test raises TypeError. Reject rather than swallow: a - # file saved as credentials.json but not usable as credentials only - # fails later, somewhere less obvious. - try: - file.seek(0) - creds_data = json.loads(file.read()) - file.seek(0) - is_oauth_shaped = 'installed' in creds_data or 'web' in creds_data - except Exception: - is_oauth_shaped = False - - if not is_oauth_shaped: + # Validate it looks like Google OAuth credentials. A bare scalar, a + # list, true/null — all valid JSON, none of them credentials. Reject + # rather than save: a file written as credentials.json but unusable + # as credentials only fails later, somewhere less obvious. + if not isinstance(creds_data, dict) or not ( + 'installed' in creds_data or 'web' in creds_data): return jsonify({ 'status': 'error', 'message': 'File does not appear to be a valid Google OAuth credentials file'