Feat(#10): Google Safe Browsing 및 VirusTotal 연동 하이브리드 보안 검사 파이프라인 구현 - #11
Conversation
📝 WalkthroughWalkthroughAdds asynchronous Google Safe Browsing and VirusTotal URL checks, configurable mock security data, a hybrid threat-analysis pipeline with SAFE/SUSPICIOUS/DANGEROUS grading, and tests covering grading and mock-mode behavior. ChangesSecurity pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/security/test_security_engine.py (1)
64-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the non-mock orchestration branch.
This test only exercises mock mode, so it misses the production-path variable mismatch and provider fallback handling. Add a test with
is_mock_enabled=Falseand mocked provider responses for a clean result, an error payload, and a scanning payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/test_security_engine.py` around lines 64 - 79, Add a separate async test for analyze_threat_pipeline with is_mock_enabled patched to return False, mocking provider responses for clean, error-payload, and scanning outcomes. Assert each provider result is routed through the non-mock orchestration branch correctly, including fallback handling and the expected output fields, so the production-path variable usage is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/service/security/google_safe_browsing.py`:
- Around line 17-19: Update the Google Safe Browsing flow around its credential
check and request error handling to return an explicit unavailable/error state
rather than False, distinguishing provider failures from “not detected.”
Propagate this state through the VirusTotal fallback and final grading logic so
any URL that was not successfully scanned cannot be upgraded to SAFE.
In `@app/service/security/parser.py`:
- Around line 21-27: In the parser flow, update the Google Safe Browsing result
check near determine_final_threat_grade to use the consistently assigned
gab_malicious variable instead of gsb_malicious. Preserve the existing warning,
stats initialization, and return behavior when the provider reports a malicious
URL.
- Around line 65-70: Update the total_analyzed_engines calculation in the
virustotal result construction to sum only the four numeric engine-count fields:
malicious_count, suspicious_count, harmless_engines, and the remaining numeric
category. Do not sum all vt_stats.values() or account for error/status entries
through subtraction; preserve the fallback metadata without allowing it into the
total.
In `@app/service/security/virustotal.py`:
- Around line 58-60: Update the newly submitted scan path in the VirusTotal
request flow so it does not return zero counts that downstream grading can
interpret as SAFE. Poll the submitted analysis until VirusTotal reports
completion and return its actual results, or return an explicit pending/unknown
status that the parser and grader preserve as non-safe.
- Line 8: Invoke the load_dotenv function during module initialization before
the os.getenv() lookup for VIRUSTOTAL_API_KEY, rather than merely referencing
the function, so environment values from .env are loaded before the key is read.
- Around line 71-78: In the VirusTotal report handling flow, replace the
undefined stats source in the return block with values read from the report’s
attributes.last_analysis_stats field. Keep last_analysis_status for analysis
state handling, and preserve the existing default of 0 for missing malicious,
suspicious, harmless, or undetected counts.
---
Nitpick comments:
In `@tests/security/test_security_engine.py`:
- Around line 64-79: Add a separate async test for analyze_threat_pipeline with
is_mock_enabled patched to return False, mocking provider responses for clean,
error-payload, and scanning outcomes. Assert each provider result is routed
through the non-mock orchestration branch correctly, including fallback handling
and the expected output fields, so the production-path variable usage is
exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6681c3b-28f6-438f-8411-e8e429cb1ebd
📒 Files selected for processing (5)
app/service/security/google_safe_browsing.pyapp/service/security/mock_provider.pyapp/service/security/parser.pyapp/service/security/virustotal.pytests/security/test_security_engine.py
| if not GOOGLE_API_KEY: | ||
| logger.warning("Google Safe Browsing API Key가 누락되었습니다.") | ||
| return False |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not treat an unavailable provider as a clean result.
Missing credentials, HTTP failures, and timeouts all return False, which is indistinguishable from “not detected.” Combined with VirusTotal’s zero-count fallbacks, the pipeline can label an unscanned URL SAFE. Return an explicit unavailable/error state and ensure the final grader never upgrades it to SAFE.
Also applies to: 58-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/service/security/google_safe_browsing.py` around lines 17 - 19, Update
the Google Safe Browsing flow around its credential check and request error
handling to return an explicit unavailable/error state rather than False,
distinguishing provider failures from “not detected.” Propagate this state
through the VirusTotal fallback and final grading logic so any URL that was not
successfully scanned cannot be upgraded to SAFE.
| gab_malicious = await check_google_safe_browsing(url) | ||
|
|
||
| # 구글 블랙리스트에 등록되어 있으면 차단 처리 | ||
| if gsb_malicious: | ||
| logger.warning(f"[Pipeline] 1단계 구글 필터 감지 차단 -> 즉시 DANGEROUS 반환: {url}") | ||
| vt_stats = {"malicious": 0, "suspicious": 0, "harmless": 0, "undetected": 0} | ||
| return determine_final_threat_grade(url, gsb_malicious, vt_stats) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Use the Google result variable consistently.
The non-mock branch assigns gab_malicious, then reads gsb_malicious. Because gsb_malicious is only assigned in the mock branch, every real-provider request raises UnboundLocalError at Line 24.
Proposed fix
- gab_malicious = await check_google_safe_browsing(url)
+ gsb_malicious = await check_google_safe_browsing(url)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gab_malicious = await check_google_safe_browsing(url) | |
| # 구글 블랙리스트에 등록되어 있으면 차단 처리 | |
| if gsb_malicious: | |
| logger.warning(f"[Pipeline] 1단계 구글 필터 감지 차단 -> 즉시 DANGEROUS 반환: {url}") | |
| vt_stats = {"malicious": 0, "suspicious": 0, "harmless": 0, "undetected": 0} | |
| return determine_final_threat_grade(url, gsb_malicious, vt_stats) | |
| gsb_malicious = await check_google_safe_browsing(url) | |
| # 구글 블랙리스트에 등록되어 있으면 차단 처리 | |
| if gsb_malicious: | |
| logger.warning(f"[Pipeline] 1단계 구글 필터 감지 차단 -> 즉시 DANGEROUS 반환: {url}") | |
| vt_stats = {"malicious": 0, "suspicious": 0, "harmless": 0, "undetected": 0} | |
| return determine_final_threat_grade(url, gsb_malicious, vt_stats) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/service/security/parser.py` around lines 21 - 27, In the parser flow,
update the Google Safe Browsing result check near determine_final_threat_grade
to use the consistently assigned gab_malicious variable instead of
gsb_malicious. Preserve the existing warning, stats initialization, and return
behavior when the provider reports a malicious URL.
| "virustotal": { | ||
| "malicious_engines": malicious_count, | ||
| "suspicious_engines": suspicious_count, | ||
| "harmless_engines": vt_stats.get("harmless", 0), | ||
| "total_analyzed_engines": sum(vt_stats.values()) - (1 if "error" in vt_stats or "status" in vt_stats else 0) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Sum only numeric engine-count fields.
vt_stats.values() includes "error": "..." or "status": "scanning" for the provider’s fallback paths, so sum(...) raises TypeError. Subtracting one afterward cannot prevent that. Calculate the total from the four numeric categories explicitly.
Proposed fix
- "total_analyzed_engines": sum(vt_stats.values()) - (1 if "error" in vt_stats or "status" in vt_stats else 0)
+ "total_analyzed_engines": sum(
+ vt_stats.get(category, 0)
+ for category in ("malicious", "suspicious", "harmless", "undetected")
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "virustotal": { | |
| "malicious_engines": malicious_count, | |
| "suspicious_engines": suspicious_count, | |
| "harmless_engines": vt_stats.get("harmless", 0), | |
| "total_analyzed_engines": sum(vt_stats.values()) - (1 if "error" in vt_stats or "status" in vt_stats else 0) | |
| } | |
| "virustotal": { | |
| "malicious_engines": malicious_count, | |
| "suspicious_engines": suspicious_count, | |
| "harmless_engines": vt_stats.get("harmless", 0), | |
| "total_analyzed_engines": sum( | |
| vt_stats.get(category, 0) | |
| for category in ("malicious", "suspicious", "harmless", "undetected") | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/service/security/parser.py` around lines 65 - 70, Update the
total_analyzed_engines calculation in the virustotal result construction to sum
only the four numeric engine-count fields: malicious_count, suspicious_count,
harmless_engines, and the remaining numeric category. Do not sum all
vt_stats.values() or account for error/status entries through subtraction;
preserve the fallback metadata without allowing it into the total.
| from dotenv import load_dotenv | ||
|
|
||
| # 환경 변수 로드 | ||
| load_dotenv |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*load_dotenv\s*$|load_dotenv\(\)' app/service/securityRepository: SafeFam/SafeFam_AI
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- app/service/security/virustotal.py (top section) ---'
sed -n '1,120p' app/service/security/virustotal.py | cat -n
echo
echo '--- sibling usage patterns ---'
sed -n '1,40p' app/service/security/google_safe_browsing.py | cat -n
echo
sed -n '1,40p' app/service/security/mock_provider.py | cat -nRepository: SafeFam/SafeFam_AI
Length of output: 7087
Invoke load_dotenv() here. load_dotenv by itself is a no-op, so VIRUSTOTAL_API_KEY from .env is never loaded before os.getenv() runs and the module falls back to the empty-result path.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 8-8: Found useless expression. Either assign it to a variable or remove it.
(B018)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/service/security/virustotal.py` at line 8, Invoke the load_dotenv
function during module initialization before the os.getenv() lookup for
VIRUSTOTAL_API_KEY, rather than merely referencing the function, so environment
values from .env are loaded before the key is read.
Source: Linters/SAST tools
| # 최초 요청 직후에는 기본 구조 데이터만 반환 | ||
| logger.info(f"[VirusTotal] 신규 스캔 요청 완료: {url}") | ||
| return {"malicious": 0, "suspicious": 0, "harmless": 0, "undetected": 0, "status": "scanning"} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not grade a newly submitted scan from zero counts.
A 404 URL is submitted for analysis, but the pipeline receives zeros and will interpret the result as SAFE once the parser error is fixed. Poll the submitted analysis until completion, or propagate an explicit pending/unknown outcome that cannot be graded as safe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/service/security/virustotal.py` around lines 58 - 60, Update the newly
submitted scan path in the VirusTotal request flow so it does not return zero
counts that downstream grading can interpret as SAFE. Poll the submitted
analysis until VirusTotal reports completion and return its actual results, or
return an explicit pending/unknown status that the parser and grader preserve as
non-safe.
| status = report_data.get("data", {}).get("attributes", {}).get("last_analysis_status", {}) | ||
|
|
||
| logger.info(f"[VirusTotal] 분석 완료 - 악성: {status.get('malicious', 0)}, 의심: {status.get('suspicious', 0)}") | ||
| return { | ||
| "malicious": stats.get("malicious", 0), | ||
| "suspicious": stats.get("suspicious", 0), | ||
| "harmless": stats.get("harmless", 0), | ||
| "undetected": stats.get("undetected", 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the official VirusTotal API v3 documentation for URL objects, which attributes contain the analysis completion status and the per-category engine counts?
💡 Result:
According to the official VirusTotal API v3 documentation for URL objects, the attributes for tracking analysis status and per-category engine counts are as follows: 1. Analysis Completion Status: The URL object itself does not contain an explicit "status" attribute [1][2]. Instead, the analysis status is managed via the Analyses object [3][4]. When you submit a URL for analysis, you receive an ID for an Analysis object [5], which includes a status attribute with possible values such as "queued," "in-progress," and "completed" [4]. 2. Per-category Engine Counts: The URL object contains the last_analysis_stats attribute [1][2]. This is a dictionary that provides a summary of the scan results, including the count of engines that categorized the URL as "harmless," "malicious," "suspicious," "timeout," or "undetected" [1][2]. For detailed results per individual engine, the last_analysis_results attribute within the URL object provides a dictionary where each key is the scanner name, containing sub-fields such as result, category, engine_name, and method [1][2].
Citations:
- 1: https://docs.virustotal.com/reference/url-object
- 2: https://virustotal.readme.io/reference/url-object
- 3: https://docs.virustotal.com/reference/analyses-object
- 4: https://virustotal.readme.io/reference/analyses-object
- 5: https://virustotal.github.io/vt-py/quickstart.html
🏁 Script executed:
sed -n '1,140p' app/service/security/virustotal.pyRepository: SafeFam/SafeFam_AI
Length of output: 3745
🏁 Script executed:
rg -n "\bstats\b" app/service/security/virustotal.pyRepository: SafeFam/SafeFam_AI
Length of output: 393
Read last_analysis_stats before returning. stats is undefined here, so successful VirusTotal reports fall into the exception path. last_analysis_status is the analysis state; use last_analysis_stats for the category counts.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 75-75: Undefined name stats
(F821)
[error] 76-76: Undefined name stats
(F821)
[error] 77-77: Undefined name stats
(F821)
[error] 78-78: Undefined name stats
(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/service/security/virustotal.py` around lines 71 - 78, In the VirusTotal
report handling flow, replace the undefined stats source in the return block
with values read from the report’s attributes.last_analysis_stats field. Keep
last_analysis_status for analysis state handling, and preserve the existing
default of 0 for missing malicious, suspicious, harmless, or undetected counts.
Source: Linters/SAST tools
1 participant
📝 개요
VirusTotal의 무료 API 호출 한도(분당 4회) 문제와 시연 시 병목 현상을 해결하기 위해, Google Safe Bwosing API(1차 차단 필터) 와 VirusTotal API(2차 정밀 분석) 를 연동한 하이브리드 다단계 검사 파이프라인을 구축합니다.
🔗 관련 이슈
🎯 주요 변경 사항
Google Safe Browsing API 비동기 연동 및 1차 필터링
VirusTotal API 비동기 정밀 조회 및 예외 처리
통합 결과 분석 파서 및 위협 등급 스코어링
SAFE,SUSPICIOUS,DANGEROUS3단계 등급으로 스코어링로컬 테스트용 Sanbox Mock 모드 구현
자동화된 통합 및 단위 테스트 코드 구축
📸 사진
[하이브리드 보안 검사 엔진의 정상 작동을 확인하는 5개 시나리오 테스트(pytest) 통과 완료]
✅ PR 체크리스트
uvicorn구동 또는 테스트 코드)를 통과했습니다.Summary by CodeRabbit
New Features
Tests