🛡️ Sentinel: [MEDIUM] Fix DoS vulnerability in AudioFeedback

[Whamp]

User description

🚨 Severity: MEDIUM
💡 Vulnerability: AudioFeedback loaded audio files into memory without checking size, allowing potential Denial of Service (DoS) if a large file was configured.
🎯 Impact: Memory exhaustion leading to application crash.
🔧 Fix: Enforced a 5MB limit on audio files in AudioFeedback._load_and_cache.
✅ Verification: Added tests/test_audio_security.py and updated tests/test_audio_feedback.py to verify the size check works.

---

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

---

PR Type

Bug fix, Tests

---

Description

• Added 5MB size limit to prevent DoS via large audio files

• Implemented file size validation in _load_and_cache method

• Updated existing tests to mock file size checks

• Added comprehensive security test suite for audio file handling

---

Diagram Walkthrough

flowchart LR
  A["AudioFeedback._load_and_cache"] --> B["Check file size"]
  B --> C{Size > 5MB?}
  C -->|Yes| D["Raise ValueError"]
  C -->|No| E["Load audio file"]
  F["MAX_AUDIO_FILE_SIZE_BYTES constant"] --> B

Loading

File Walkthrough

	Relevant files
Bug fix	audio_feedback.py Add file size limit to audio loading                                          src/chirp/audio_feedback.py Added MAX_AUDIO_FILE_SIZE_BYTES constant set to 5MB Implemented file size validation in _load_and_cache method before loading Raises ValueError if audio file exceeds size limit Prevents memory exhaustion from large audio files +10/-0
Tests	test_audio_feedback.py Update tests to mock file size checks                                        tests/test_audio_feedback.py Updated test_load_and_cache_sounddevice to mock Path.stat for size check Updated test_load_and_cache_with_volume_scaling to mock file size validation Added proper mocking to pass the new size limit check Wrapped test logic with patch context manager for file stat mocking +45/-37  test_audio_security.py Add audio security test suite                                                        tests/test_audio_security.py Created new security test file for audio file handling Added test_audio_file_size_limit to verify 6MB files are rejected Tests that ValueError is raised with appropriate error message Validates the 5MB size limit enforcement +48/-0
Documentation	sentinel.md Document audio file DoS vulnerability                                        .jules/sentinel.md Documented the unbounded audio file loading vulnerability Recorded learning about dangers of unchecked bulk file loaders Added prevention strategy for file input size limits +5/-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
⚪	Information disclosure Description: The raised ValueError includes the full filesystem path ({path}) and exact file size, which could expose sensitive local path information to logs/clients if the exception message is surfaced (e.g., via self._logger.exception in the caller or higher-level error handling). audio_feedback.py [136-141] Referred Code # Security: Prevent DoS by rejecting large files size = path.stat().st_size if size > MAX_AUDIO_FILE_SIZE_BYTES: raise ValueError( f"Audio file too large: {path} ({size} bytes > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" )
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: 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 Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Path leaked in error: The new ValueError includes the full filesystem path and exact file size in the exception string, which may be user-visible depending on higher-level handling and could leak internal path information. Referred Code raise ValueError( f"Audio file too large: {path} ({size} bytes > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" ) 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: Exception logs include path: _play_sound logs exceptions including exc, which will embed the ValueError message containing the full audio file path and size, potentially exposing sensitive filesystem details if logs are broadly accessible. Referred Code except Exception as exc: # pragma: no cover - defensive self._logger.exception("Failed to play sound %s: %s", asset_name, 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 TOCTOU vulnerability during file validation Fix a Time-of-Check to Time-of-Use (TOCTOU) vulnerability by opening the audio file before checking its size. This ensures the size check and file read operations are performed on the exact same file, preventing a potential bypass of the size limit. src/chirp/audio_feedback.py [135-159] def _load_and_cache(self, path: Path, key: str) -> Any: - # Security: Prevent DoS by rejecting large files - size = path.stat().st_size - if size > MAX_AUDIO_FILE_SIZE_BYTES: - raise ValueError( - f"Audio file too large: {path} ({size} bytes > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" - ) + try: + with path.open("rb") as f: + # Security: Prevent DoS by checking size on the open file handle to avoid TOCTOU. + f.seek(0, 2) # Move to the end of the file + size = f.tell() + if size > MAX_AUDIO_FILE_SIZE_BYTES: + raise ValueError( + f"Audio file too large: {path} ({size} bytes > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" + ) + f.seek(0) # Rewind to the beginning - 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() - n_channels = wf.getnchannels() - n_frames = wf.getnframes() - audio_bytes = wf.readframes(n_frames) - data = self._load_sounddevice_data( - audio_bytes, samplerate, n_channels, n_frames - ) - self._cache[key] = data - return data - elif self._use_winsound: - # For winsound, we just cache the path and play it directly. - # The size check above provides a layer of protection. - self._cache[key] = path - return path - else: # pragma: no cover + if self._use_sounddevice: + # Load as numpy array for volume-controlled playback via sounddevice + with wave.open(f, "rb") as wf: + samplerate = wf.getframerate() + n_channels = wf.getnchannels() + n_frames = wf.getnframes() + audio_bytes = wf.readframes(n_frames) + data = self._load_sounddevice_data( + audio_bytes, samplerate, n_channels, n_frames + ) + self._cache[key] = data + return data + elif self._use_winsound: + # For winsound, read into memory now to avoid TOCTOU. + audio_bytes = f.read() + self._cache[key] = audio_bytes + return audio_bytes + else: # pragma: no cover + self._cache[key] = None + return None + except FileNotFoundError: + self._logger.warning("Audio file not found: %s", path) self._cache[key] = None return None [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies and fixes a Time-of-Check to Time-of-Use (TOCTOU) security vulnerability in the PR's implementation, which is a critical improvement for the introduced security feature.	High
General	Validate decoded audio size Add a validation step to check the size of the decoded PCM audio data before reading it into memory. This prevents memory exhaustion from compressed audio files (e.g., "zip bombs") that are small on disk but large when uncompressed. src/chirp/audio_feedback.py [143-146] 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() - ... + nframes = wf.getnframes() + nchannels = wf.getnchannels() + sampwidth = wf.getsampwidth() + pcm_size = nframes * nchannels * sampwidth + if pcm_size > MAX_AUDIO_FILE_SIZE_BYTES: + raise ValueError( + f"Decoded audio too large: {pcm_size} bytes > {MAX_AUDIO_FILE_SIZE_BYTES}" + ) frames = wf.readframes(nframes) ... [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: This suggestion adds a second layer of defense by checking the size of the decoded audio data, which protects against "zip bomb" style attacks where a small file decompresses to a very large size in memory.	Medium
	Handle stat errors gracefully Improve error handling by wrapping the path.stat() call in a try...except OSError block. Additionally, use path.name in error messages to avoid leaking the full file path. src/chirp/audio_feedback.py [137-141] -size = path.stat().st_size +try: + size = path.stat().st_size +except OSError as e: + raise ValueError(f"Unable to access audio file: {path.name}") from e + if size > MAX_AUDIO_FILE_SIZE_BYTES: raise ValueError( - f"Audio file too large: {path} ({size} bytes > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" + f"Audio file too large: {path.name} ({size} bytes > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" ) Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion improves robustness by handling potential OSError during the file size check and enhances security by preventing potential file path leakage in error messages, which are valuable but not critical changes.	Low
	Make file size limit configurable Make the maximum audio file size limit configurable by passing it as a parameter to the AudioFeedback constructor. This improves flexibility and simplifies testing by allowing the limit to be adjusted per instance. src/chirp/audio_feedback.py [28-139] MAX_AUDIO_FILE_SIZE_BYTES = 5 * 1024 * 1024 # 5MB -... -def __init__( - self, - *, - logger: logging.Logger, - enabled: bool = True, - volume: float = 1.0, -) -> None: + +class AudioFeedback: + def __init__( + self, + *, + logger: logging.Logger, + enabled: bool = True, + volume: float = 1.0, + max_file_size_bytes: int = MAX_AUDIO_FILE_SIZE_BYTES, + ) -> None: + ... + self._max_file_size_bytes = max_file_size_bytes ... -... -size = path.stat().st_size -if size > MAX_AUDIO_FILE_SIZE_BYTES: - ... + def _load_and_cache(self, path: Path, key: str) -> Any: + size = path.stat().st_size + if size > self._max_file_size_bytes: + ... [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 4 __ Why: This suggestion improves configurability and testability by making the file size limit an injectable parameter. While this is a good design practice, it is a moderate-impact refactoring rather than a fix for a defect.	Low
More