Skip to content

Conversation

@eugeneg-aya
Copy link

@eugeneg-aya eugeneg-aya commented Dec 2, 2025

What does this PR do?

throw exception in case of unexpected response
#36

Summary by CodeRabbit

  • Bug Fixes
    • Improved malware scan reliability with stricter validation of scanner responses.
    • More accurate clean vs. infected detection to reduce false results.
    • Clearer error reporting when scans return unexpected or malformed responses, helping troubleshooting.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Walkthrough

The fileScan method in src/ClamAV/ClamAV.php was changed to use a $response variable instead of $out, add input validation for non-string or empty responses, enforce a stricter response format (expects "/path/to/file: STATUS"), and extract the STATUS token robustly. It returns true for OK, false for FOUND, and throws a RuntimeException for empty/non-string responses, unexpected formats, or unrecognized statuses (including the full response text in the exception message).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify the response format assumption matches ClamAV daemon output variants (e.g., multiple-line responses, additional whitespace, or trailing text).
  • Confirm input validation behavior for non-string/empty values is appropriate for callers.
  • Review exception messages to ensure they provide necessary context without leaking sensitive data.
  • Check for any remaining references to the old $out variable and ensure no scope/regression issues in src/ClamAV/ClamAV.php.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: throwing an exception when an unexpected response is received, which directly aligns with the modification to the fileScan method.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a9f6aa and 7040df5.

📒 Files selected for processing (1)
  • src/ClamAV/ClamAV.php (1 hunks)
🔇 Additional comments (5)
src/ClamAV/ClamAV.php (5)

135-137: LGTM! Robust input validation.

The validation properly handles null and empty responses from sendCommand(), which can return ?string. This defensive check prevents downstream parsing errors.


139-146: Good format validation with helpful error message.

The format check ensures the response follows the expected "/path/to/file: STATUS" pattern. Including the full response in the exception message aids debugging.

Note: File paths containing colons could theoretically cause incorrect parsing, but this is an edge case and the original code had the same limitation.


148-148: Clean status extraction.

Using trim(end($parts)) correctly extracts the status token from the last segment.


158-161: Comprehensive error handling for unexpected responses.

Throwing a RuntimeException for any status other than OK or ending with FOUND ensures that unexpected ClamAV responses (e.g., "ERROR", "UNKNOWN") are surfaced rather than silently mishandled. Including both the status and full response in the error message provides excellent debugging context.


150-156: Excellent fix! FOUND check now handles virus names correctly.

The use of str_ends_with($status, 'FOUND') properly matches ClamAV responses that include virus names (e.g., "Win.Test.EICAR_HDB-1 FOUND"). This addresses the critical issue identified in the previous review.

The project requires PHP >=8.0, which fully supports str_ends_with(), so this implementation is appropriate without requiring backward compatibility fallbacks.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5f0078 and 6a9f6aa.

📒 Files selected for processing (1)
  • src/ClamAV/ClamAV.php (1 hunks)
🔇 Additional comments (3)
src/ClamAV/ClamAV.php (3)

133-137: Good defensive validation for daemon response.

The null/empty check correctly handles edge cases where the ClamAV daemon may not respond or return an invalid response.


139-146: Response parsing looks correct for the expected format.

The validation ensures we have at least path and status components. Note: paths containing colons would cause issues, but this is a pre-existing limitation shared with fileScanInStream and continueScan methods.


158-161: Good exception messaging with debugging context.

Including both the parsed status and full response will help diagnose issues. Once the FOUND check (above) is fixed, this correctly handles genuinely unexpected responses like ERROR or UNKNOWN.

@eugeneg-aya eugeneg-aya changed the title SO-4011 throw exception in case of unexpected response throw exception in case of unexpected response Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant