🛡️ Sentinel: Enforce file size limit on audio feedback assets

[Whamp]

User description

Implemented a security enhancement to limit the size of audio feedback files to 5MB. This prevents a potential Denial of Service (DoS) vulnerability where loading a maliciously large audio file could cause memory exhaustion and crash the application.

Changes:

• Modified src/chirp/audio_feedback.py to include MAX_AUDIO_FILE_SIZE_BYTES and a file size check.
• Created tests/test_audio_security.py to verify the new security constraint.
• Updated tests/test_audio_feedback.py to properly mock file system statistics in existing tests.

---

PR created automatically by Jules for task 8979471785784312993 started by @Whamp

---

PR Type

Bug fix, Enhancement, Tests

---

Description

• Enforce 5MB file size limit on audio feedback assets

• Prevents DoS vulnerability from memory exhaustion attacks

• Added file size validation in _load_and_cache method

• Created comprehensive security test suite

• Updated existing tests to mock file system operations

---

Diagram Walkthrough

flowchart LR
  A["Audio File Loading"] --> B["Check File Size"]
  B --> C{Size <= 5MB?}
  C -->|Yes| D["Load and Cache"]
  C -->|No| E["Return None + Log Warning"]
  D --> F["Play Audio"]
  E --> G["Skip Playback"]

Loading

File Walkthrough

	Relevant files
Security enhancement	audio_feedback.py Add 5MB file size limit validation                                              src/chirp/audio_feedback.py Added MAX_AUDIO_FILE_SIZE_BYTES constant set to 5MB Implemented file size validation in _load_and_cache method Returns None and logs warning if file exceeds size limit Handles OSError exceptions during file stat operations +17/-0
Tests	test_audio_feedback.py Mock file system operations in tests                                          tests/test_audio_feedback.py Added @patch("pathlib.Path.stat") decorator to two test methods Mocked path.stat().st_size to return valid file size (1024 bytes) Updated test_load_and_cache_sounddevice and test_load_and_cache_with_volume_scaling +10/-2    test_audio_security.py Add audio file size security tests                                              tests/test_audio_security.py Created new security test file with TestAudioFeedbackSecurity class Implemented test_load_large_file_fails to verify 5MB limit enforcement Validates that oversized files return None and trigger warning logs Mocks all external dependencies including wave, numpy, and sounddevice +52/-0

---

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	None handling DoS Description: _load_and_cache() now returns None for oversized/un-stat-able files, but _play_sound() still unconditionally passes the returned value into _play_cached(data) (line 129), which may raise and crash audio playback (potential DoS) if _play_cached does not explicitly handle None. audio_feedback.py [128-149] Referred Code data = self._load_and_cache(path, cache_key) self._play_cached(data) except FileNotFoundError: self._logger.warning("Sound file missing: %s", override_path or asset_name) except Exception as exc: # pragma: no cover - defensive self._logger.exception("Failed to play sound %s: %s", asset_name, exc) def _load_and_cache(self, path: Path, key: str) -> Any: try: file_size = path.stat().st_size if file_size > MAX_AUDIO_FILE_SIZE_BYTES: self._logger.warning( "Audio file %s is too large (%d bytes). Max allowed: %d bytes.", path, file_size, MAX_AUDIO_FILE_SIZE_BYTES, ) return None except OSError as exc: self._logger.warning("Could not stat audio file %s: %s", path, exc) return None ... (clipped 1 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unstructured warning logs: Newly added warnings log interpolated strings (including path) rather than explicitly structured fields (e.g., JSON/key-value), so it is unclear whether production logs meet the structured-logging requirement and avoid leaking potentially sensitive local paths. Referred Code self._logger.warning( "Audio file %s is too large (%d bytes). Max allowed: %d bytes.", path, file_size, MAX_AUDIO_FILE_SIZE_BYTES, ) return None except OSError as exc: self._logger.warning("Could not stat audio file %s: %s", path, exc) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Fix a TOCTOU race condition Refactor _load_and_cache to prevent a TOCTOU race condition by opening the file first and then checking its size from the file handle before processing its contents. src/chirp/audio_feedback.py [135-154] def _load_and_cache(self, path: Path, key: str) -> Any: try: - file_size = path.stat().st_size - if file_size > MAX_AUDIO_FILE_SIZE_BYTES: - self._logger.warning( - "Audio file %s is too large (%d bytes). Max allowed: %d bytes.", - path, - file_size, - MAX_AUDIO_FILE_SIZE_BYTES, - ) - return None + with path.open("rb") as f: + # Get file size from the file object to avoid TOCTOU race condition + f.seek(0, 2) # Seek to the end of the file + file_size = f.tell() + f.seek(0) # Rewind to the beginning + + if file_size > MAX_AUDIO_FILE_SIZE_BYTES: + self._logger.warning( + "Audio file %s is too large (%d bytes). Max allowed: %d bytes.", + path, + file_size, + MAX_AUDIO_FILE_SIZE_BYTES, + ) + return None + + if self._use_sounddevice: + # Load as numpy array for volume-controlled playback via sounddevice + with wave.open(f, "rb") as wf: + samplerate = wf.getframerate() + nchannels = wf.getnchannels() + ... except OSError as exc: - self._logger.warning("Could not stat audio file %s: %s", path, exc) + self._logger.warning("Could not open or read audio file %s: %s", path, exc) return None - if self._use_sounddevice: - # Load as numpy array for volume-controlled playback via sounddevice - with wave.open(str(path), "rb") as wf: - samplerate = wf.getframerate() - nchannels = wf.getnchannels() - ... - [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical TOCTOU race condition vulnerability in the newly added file size check and provides a robust solution, significantly improving the security of the feature.	High
Possible issue	Test stat‐exception fallback Add a new unit test to TestAudioFeedbackSecurity to verify that _load_and_cache correctly handles an OSError during the file stat operation by returning None and logging a warning. tests/test_audio_security.py [11-13] class TestAudioFeedbackSecurity(unittest.TestCase): def setUp(self): self.mock_logger = MagicMock(spec=logging.Logger) + @patch("pathlib.Path.stat", side_effect=OSError("stat error")) + @patch("chirp.audio_feedback.sd", new=MagicMock()) + @patch("chirp.audio_feedback.winsound", None) + def test_stat_failure_returns_none(self, mock_sd, mock_stat): + """_load_and_cache should return None and log warning on stat failure.""" + af = AudioFeedback(logger=self.mock_logger, enabled=True) + result = af._load_and_cache(Path("/bad.wav"), "key") + self.assertIsNone(result) + self.mock_logger.warning.assert_called_with("Could not stat audio file %s", Path("/bad.wav"), exc_info=True) + Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out a missing test case for the OSError handling path added in the PR and provides a complete, valid implementation for it.	Low
General	Avoid hardcoding constants in tests In test_load_large_file_fails, import the MAX_AUDIO_FILE_SIZE_BYTES constant and use it to set the mock file size instead of using a hardcoded value. tests/test_audio_security.py [18-43] +from chirp.audio_feedback import AudioFeedback, MAX_AUDIO_FILE_SIZE_BYTES + +... + @patch("pathlib.Path.stat") def test_load_large_file_fails(self, mock_stat, mock_np): - """_load_and_cache should return None if file is too large (>5MB).""" + """_load_and_cache should return None if file is too large.""" af = AudioFeedback(logger=self.mock_logger, enabled=True) - # Mock file size to be slightly larger than 5MB (5 * 1024 * 1024 + 1) - mock_stat.return_value.st_size = 5 * 1024 * 1024 + 1 + # Mock file size to be slightly larger than the max allowed size + mock_stat.return_value.st_size = MAX_AUDIO_FILE_SIZE_BYTES + 1 # We need to mock wave.open so it doesn't actually try to open a file # if the check fails (or if it doesn't fail yet) with patch("chirp.audio_feedback.wave") as mock_wave: # Setup mock to behave "normally" if called mock_wf = MagicMock() mock_wave.open.return_value.__enter__.return_value = mock_wf ... result = af._load_and_cache(Path("/fake/large_file.wav"), "key") # This assertion should fail until the fix is implemented - self.assertIsNone(result, "Should return None for files larger than 5MB") + self.assertIsNone(result, f"Should return None for files larger than {MAX_AUDIO_FILE_SIZE_BYTES} bytes") [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: The suggestion improves test maintainability by proposing to replace a hardcoded value with the MAX_AUDIO_FILE_SIZE_BYTES constant from the application code, ensuring the test stays synchronized with the implementation.	Low
	Log stat errors with traceback When logging an OSError in _load_and_cache, add exc_info=True to include the full traceback for easier debugging. src/chirp/audio_feedback.py [146-148] except OSError as exc: - self._logger.warning("Could not stat audio file %s: %s", path, exc) + self._logger.warning("Could not stat audio file %s", path, exc_info=True) return None Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly proposes adding exc_info=True to the log to include a traceback, which improves debuggability for OSError exceptions during file stat operations.	Low
More