fix(security): remove SSL bypass, log silent errors, add checksum validation#1682
Open
laurigates wants to merge 2 commits intohacksider:mainfrom
Open
fix(security): remove SSL bypass, log silent errors, add checksum validation#1682laurigates wants to merge 2 commits intohacksider:mainfrom
laurigates wants to merge 2 commits intohacksider:mainfrom
Conversation
…idation - Remove macOS ssl._create_unverified_context monkey-patch; SSL certificate verification is now enabled on all platforms - run_ffmpeg and detect_fps now print the error instead of swallowing it - face_masking blending failure is logged before returning the unmodified frame - Add _compute_sha256 helper and expected_checksums parameter to conditional_download; mismatched files are deleted and ValueError raised Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideEnables SSL certificate verification again, improves error visibility in ffmpeg and face masking utilities, and adds optional SHA-256 checksum verification to model downloads to detect and delete corrupted files. Class diagram for utilities and face_masking changesclassDiagram
class Utilities {
<<module>>
+str TEMP_FILE
+str TEMP_DIRECTORY
+str _compute_sha256(file_path)
+bool run_ffmpeg(args)
+float detect_fps(target_path)
+void conditional_download(download_directory_path, urls, expected_checksums)
}
class FaceMasking {
<<module>>
+apply_mask_area(frame, mask, min_x, min_y, max_x, max_y, erosion, blur_amount, feather_amount, sigma)
}
Flow diagram for conditional_download with checksum validationflowchart TD
A[Start conditional_download] --> B{download_directory_path exists?}
B -- Yes --> C[Iterate over urls]
B -- No --> B1[Create download_directory_path] --> C
C --> D[Take next url]
D --> E{url is a Google Drive link?}
E -- Yes --> F[Handle Google Drive download]
E -- No --> G[Handle standard download]
F --> H[Set download_file_path]
G --> H
H --> I{download_file_path exists?}
I -- Yes --> C
I -- No --> J[Download file with urlretrieve and progress]
J --> K{expected_checksums provided and filename in expected_checksums?}
K -- No --> C
K -- Yes --> L["_compute_sha256(download_file_path)"]
L --> M{actual checksum == expected checksum?}
M -- Yes --> N[Log checksum verified] --> C
M -- No --> O[Delete download_file_path]
O --> P[Raise ValueError for checksum mismatch]
C --> Q[All urls processed]
Q --> R[End conditional_download]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new error reporting in
run_ffmpeg,detect_fps, andapply_mask_areauses bareprintcalls, which may be noisy or hard to route in non-CLI environments; consider routing these through the existing logging mechanism orstderrso callers can better control verbosity. - The
expected_checksumsmapping inconditional_downloadis keyed byos.path.basename(url), which can be ambiguous if different URLs share the same filename; consider keying by full URL or allowing callers to specify an explicit filename key to avoid silent collisions. - Changing the
conditional_downloadsignature to addexpected_checksumsmay break existing callers; if this is a public helper, consider using a keyword-only argument, defaulting via**kwargs, or adding a wrapper to maintain backward compatibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new error reporting in `run_ffmpeg`, `detect_fps`, and `apply_mask_area` uses bare `print` calls, which may be noisy or hard to route in non-CLI environments; consider routing these through the existing logging mechanism or `stderr` so callers can better control verbosity.
- The `expected_checksums` mapping in `conditional_download` is keyed by `os.path.basename(url)`, which can be ambiguous if different URLs share the same filename; consider keying by full URL or allowing callers to specify an explicit filename key to avoid silent collisions.
- Changing the `conditional_download` signature to add `expected_checksums` may break existing callers; if this is a public helper, consider using a keyword-only argument, defaulting via `**kwargs`, or adding a wrapper to maintain backward compatibility.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
laurigates
added a commit
to laurigates/Deep-Live-Cam
that referenced
this pull request
Feb 24, 2026
- Replace bare print() calls with logger.error/warning/info in utilities.py and face_masking.py; routes output through the standard logging framework so callers can control verbosity - Key expected_checksums by full URL instead of os.path.basename(url) to prevent silent collisions when different URLs share the same filename - Make expected_checksums keyword-only (add * sentinel) to prevent accidental positional passing and clarify the API boundary Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace bare print() calls with logger.error/warning/info in utilities.py and face_masking.py; routes output through the standard logging framework so callers can control verbosity - Key expected_checksums by full URL instead of os.path.basename(url) to prevent silent collisions when different URLs share the same filename - Make expected_checksums keyword-only (add * sentinel) to prevent accidental positional passing and clarify the API boundary Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ssl._create_unverified_contextmonkey-patch — SSL certificate verification is now enabled on all platformsrun_ffmpeg()anddetect_fps()now print errors instead of silently swallowing themface_masking.apply_mask_area()logs blending failures before returning the unmodified frame_compute_sha256()helper andexpected_checksumsparameter toconditional_download()— mismatched files are deleted andValueErrorraisedReplaces #1658 (closed — wrong branch content).
Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Tighten download and processing security by enabling SSL verification, adding checksum validation for downloaded files, and surfacing previously silent processing errors.
New Features:
Bug Fixes: