From 1d4896d643e1be7f9297702c43c066fb39342465 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sat, 31 Jan 2026 02:39:47 +0000 Subject: [PATCH] feat(security): enforce 5MB limit on audio feedback files Prevents potential Memory Exhaustion DoS by rejecting audio files larger than 5MB in AudioFeedback._load_and_cache. This applies to both sounddevice and winsound paths, though primarily critical for sounddevice which loads the entire file into memory. Includes: - New test suite tests/test_audio_security.py - Updates to tests/test_audio_feedback.py to mock file sizes - Sentinel journal entry in .jules/sentinel.md Co-authored-by: Whamp <1115485+Whamp@users.noreply.github.com> --- .jules/sentinel.md | 5 +++++ src/chirp/audio_feedback.py | 10 +++++++++ tests/test_audio_feedback.py | 8 ++++++-- tests/test_audio_security.py | 39 ++++++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 tests/test_audio_security.py diff --git a/.jules/sentinel.md b/.jules/sentinel.md index de6782e..aabcbdb 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -7,3 +7,8 @@ **Vulnerability:** Control characters could be injected via `word_overrides` configuration, bypassing initial input sanitization. **Learning:** Initial input sanitization is insufficient when configuration data (overrides) can re-introduce unsafe characters during processing. **Prevention:** Implement "Output Sanitization" as a final step in data processing pipelines. Ensure sanitization logic is reusable and safe (e.g., does not unintentionally destroy formatting like trailing whitespace unless intended). + +## 2025-05-21 - Unrestricted Audio File Loading (DoS) +**Vulnerability:** `AudioFeedback` loaded entire audio files into memory without size checks, allowing a potential Memory Exhaustion DoS via large configured file paths. +**Learning:** Implicit trust in local configuration files can still lead to resource exhaustion if user input (file paths) points to massive resources. +**Prevention:** Enforce strict resource limits (e.g., file size caps) on all loaded assets, even those specified in local configuration. diff --git a/src/chirp/audio_feedback.py b/src/chirp/audio_feedback.py index 45ca0a4..e0c78bf 100644 --- a/src/chirp/audio_feedback.py +++ b/src/chirp/audio_feedback.py @@ -25,6 +25,9 @@ winsound = None # type: ignore[assignment] +MAX_AUDIO_FILE_SIZE_BYTES = 5 * 1024 * 1024 # 5 MB + + class AudioFeedback: def __init__( self, @@ -130,6 +133,13 @@ def _play_sound(self, asset_name: str, override_path: Optional[str]) -> None: self._logger.exception("Failed to play sound %s: %s", asset_name, exc) def _load_and_cache(self, path: Path, key: str) -> Any: + # Security check: prevent loading massive files (DoS) + file_size = path.stat().st_size + if file_size > MAX_AUDIO_FILE_SIZE_BYTES: + raise ValueError( + f"Audio file '{path.name}' too large ({file_size} > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" + ) + if self._use_sounddevice: # Load as numpy array for volume-controlled playback via sounddevice with wave.open(str(path), "rb") as wf: diff --git a/tests/test_audio_feedback.py b/tests/test_audio_feedback.py index c656333..5456071 100644 --- a/tests/test_audio_feedback.py +++ b/tests/test_audio_feedback.py @@ -62,7 +62,9 @@ def test_load_and_cache_sounddevice(self, mock_wave, mock_np, mock_sd): mock_np.frombuffer.return_value = mock_audio_data key = "test_key" - data = af._load_and_cache(Path("/fake/sound.wav"), key) + with patch.object(Path, "stat") as mock_stat: + mock_stat.return_value.st_size = 1024 + data = af._load_and_cache(Path("/fake/sound.wav"), key) mock_wave.open.assert_called_with("/fake/sound.wav", "rb") mock_np.frombuffer.assert_called() @@ -110,7 +112,9 @@ def test_load_and_cache_with_volume_scaling(self, mock_wave, mock_np, mock_sd): af = AudioFeedback(logger=self.mock_logger, enabled=True, volume=0.5) key = "test_key" - data = af._load_and_cache(Path("/fake/sound.wav"), key) + with patch.object(Path, "stat") as mock_stat: + mock_stat.return_value.st_size = 1024 + data = af._load_and_cache(Path("/fake/sound.wav"), key) audio_data, samplerate = data diff --git a/tests/test_audio_security.py b/tests/test_audio_security.py new file mode 100644 index 0000000..19277c9 --- /dev/null +++ b/tests/test_audio_security.py @@ -0,0 +1,39 @@ +import logging +import unittest +from pathlib import Path +from unittest.mock import MagicMock, patch + +from chirp.audio_feedback import AudioFeedback + +class TestAudioSecurity(unittest.TestCase): + def setUp(self): + self.mock_logger = MagicMock(spec=logging.Logger) + + @patch("chirp.audio_feedback.sd", new=MagicMock()) + @patch("chirp.audio_feedback.winsound", None) + def test_load_and_cache_rejects_large_files(self): + """_load_and_cache should raise ValueError if file exceeds size limit.""" + af = AudioFeedback(logger=self.mock_logger, enabled=True) + + # Mock Path.stat to return a large size + large_size = 6 * 1024 * 1024 # 6MB + + # We patch Path.stat so that any Path object's stat() method returns our mock + with patch.object(Path, "stat") as mock_stat: + mock_stat.return_value.st_size = large_size + + # We don't need to mock wave.open because it should fail BEFORE opening the file + # If it tries to open, it means the security check failed (or doesn't exist) + # and potentially would raise FileNotFoundError since the file doesn't exist, + # but we want to assert it raises ValueError specifically due to size. + + # To distinguish from FileNotFoundError (if it proceeds to open), + # we rely on the specific ValueError expectation. + + with self.assertRaises(ValueError) as cm: + af._load_and_cache(Path("fake_large_file.wav"), "key") + + self.assertIn("too large", str(cm.exception)) + +if __name__ == "__main__": + unittest.main()