feat: 나이브 베이즈+Gemini 하이브리드 & 규칙 기반 트랙 스코어링 개편 (#19)#20
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR restructures SMS analysis around Naive Bayes, conditional Gemini escalation, deterministic rules, safer URL tracking, revised VirusTotal scoring, malicious overrides, and URL-aware weighted scoring. It expands response details and adds unit, service, security, URL, and end-to-end coverage. ChangesSMS scoring pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
…ack (#19) Replace the single-track Gemini-only text analysis with a two-stage naive bayes (local, free) -> Gemini (2nd-pass) pipeline, add a deterministic rule-based track (financial institution DB, keyword categories, account/card number patterns), and rework the 3-way scoring engine to reweight tracks when no URL is present and to hard-override to HIGH on confirmed-malicious signals (GSB blacklist, VT multi-engine consensus, local domain rules). Also fixes an SSRF gap and a relative-redirect bug in the URL tracer, and switches VirusTotal scoring to an actual detection-ratio formula instead of a raw count heuristic.
c62548f to
be3fb98
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/utils/scoring_engine.py (1)
105-128: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
contribution_breakdowndoesn't reconcile withfinal_scoreafter the confirmed-malicious override.When
is_confirmed_malicious=Trueraisesfinal_scoretomax(final_score, 70), the individualllm_contrib/url_contrib/rules_contribused to buildbreakdown(returned inSmishingAnalysisResponse.contribution_breakdown) are never adjusted. E.g. withllm_score=5, url_risk_score=0.95, rule_score=0, is_confirmed_malicious=True(mirroring the existing test at tests/security/test_scoring_engine.py lines 57-72), the raw weighted sum is well under 70, yetfinal_scoreis forced to 70+ whilebreakdownstill reflects the pre-override (much lower) contributions — sobreakdown.llm + breakdown.hybrid_url + breakdown.rules != final_score. Since the schema (app/dto/schemas.pyContributionBreakdown) documents this field as a "3개 레이어별 점수 기여도 명세" (per-layer contribution spec), API consumers relying on the breakdown to explain/reconstruct the score will see an inconsistent contract in override cases.♻️ Proposed fix: reflect the override in the breakdown (or document the discrepancy explicitly)
if is_confirmed_malicious: if risk_grade != RiskGrade.HIGH: logger.warning( f"[Scoring Engine] 확정 악성 URL 감지 -> HIGH 등급 강제 오버라이드 (원래 점수: {final_score})" ) + # 오버라이드로 final_score가 상승한 만큼 breakdown에도 반영 (예: rules_contrib 등 대표 트랙에 반영, + # 혹은 별도의 override 근거 필드를 응답에 추가해 소비자가 불일치를 인지할 수 있게 함) final_score = max(final_score, 70) risk_grade = RiskGrade.HIGHAt minimum, consider adding an explicit flag (e.g.
override_applied: bool) to the response so consumers know the breakdown doesn't fully explainfinal_scorein this case.🤖 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/utils/scoring_engine.py` around lines 105 - 128, Update the confirmed-malicious override flow around is_confirmed_malicious and ContributionBreakdown so contribution_breakdown remains consistent with final_score after forcing the HIGH threshold. Adjust or otherwise explicitly represent the override in the per-layer contribution data, and update the related schema/response contract as needed so consumers can reconstruct or identify the overridden score.
🧹 Nitpick comments (4)
tests/security/test_hybrid_url_engine.py (1)
40-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing boundary test at the VT consensus threshold (detected_count == 5).
Tests cover 2 (below) and 7 (above) but not exactly 5, per the doc's "5개 이상" (≥5) threshold. Without it, an off-by-one (
>vs>=) in the actual comparison wouldn't be caught.🤖 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_hybrid_url_engine.py` around lines 40 - 66, Extend the VT consensus tests around HybridUrlEngine.scan_url with a case where detected_count is exactly 5, keeping the other scan mocks consistent with the existing threshold tests, and assert that the result sets is_vt_confirmed to True.app/service/security/hybrid_url_engine.py (1)
72-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefensive
base_score > 1.0guard is now unreachable.
VirusTotalEngine.scan_urlalways returns a ratio capped at1.0(min(malicious_ratio + suspicious_ratio*0.5, 1.0)), so this normalization branch can no longer trigger. Harmless, but worth removing/documenting as legacy if a future refactor touches this area.🤖 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/hybrid_url_engine.py` around lines 72 - 74, Remove the unreachable base_score > 1.0 normalization branch from the risk-score calculation in VirusTotalEngine.scan_url, leaving risk_score to use the capped base_score directly with the existing max/min logic.app/service/security/naive_bayes_text_analyzer.py (2)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCross-module coupling for a generic helper.
determine_text_risk_gradeis imported fromgemini_text_analyzereven though it's now shared by both the naive-bayes and Gemini tracks. Consider moving it to a shared module (e.g.text_risk_grading.py) to avoid the naive-bayes analyzer depending on a module named after an unrelated engine.🤖 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/naive_bayes_text_analyzer.py` at line 6, Move the shared determine_text_risk_grade helper out of gemini_text_analyzer into a neutral shared module such as text_risk_grading.py, then update both the naive-bayes analyzer and Gemini analyzer imports to use that module while preserving existing behavior.
73-77: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDeserializing pickled model artifacts with
joblib.load.
joblib.loadexecutes arbitrary code embedded in the pickle stream.MODEL_PATH/VECTORIZER_PATHare overridable viaNAIVE_BAYES_MODEL_PATH/NAIVE_BAYES_VECTORIZER_PATHenv vars, so if either the deployment env or the artifact directory is ever writable by a less-trusted actor, loading becomes an RCE vector (CWE-502). Given these are trusted, locally-produced training artifacts today, consider at least validating a checksum/signature before load if the artifact pipeline ever becomes externally sourced (e.g., CI artifact download, shared storage).🤖 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/naive_bayes_text_analyzer.py` around lines 73 - 77, Harden artifact loading in the analyzer initialization around MODEL_PATH and VECTORIZER_PATH by validating each artifact’s trusted checksum or signature before calling joblib.load. Reject mismatched or untrusted files and preserve the existing model, threshold, classes, and vectorizer initialization only after both validations succeed.Source: Linters/SAST tools
🤖 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/utils/scoring_engine.py`:
- Around line 33-53: Update ScoringEngine._combine_text_track_score to check
llm_available when naive_bayes_score is None; preserve the existing llm_score
return only when Gemini is available, and explicitly surface the simultaneous
NB/Gemini failure through the established logging or failure-reporting path
instead of silently returning the default 0.
In `@app/utils/url_tracker.py`:
- Around line 28-58: Update _is_public_host and trace_url so DNS validation
returns and preserves the validated address, then configure the httpx
AsyncClient HEAD/GET requests to connect to that pinned IP while retaining the
original hostname for SNI/Host handling across every redirect hop. Add a timeout
around loop.getaddrinfo, and ensure failed or timed-out resolution remains
blocked rather than falling back to resolving the hostname again.
In `@tests/e2e/test_analyze_endpoint.py`:
- Around line 55-70: Update
test_message_with_url_populates_url_analysis_via_hybrid_engine to mock
app.service.scan_service.trace_url before posting the request, while preserving
the existing URL-analysis and hybrid contribution assertions. Do not rely on
live DNS or HTTP behavior; leave redirect coverage to the dedicated URL-tracer
tests.
- Around line 79-91: Strengthen the assertions in the analyze endpoint test:
require hybrid_url to equal 0 for requests without a URL, require rule_analysis
to be present and successful, and validate that the OpenAPI POST operation
defines both a request-body schema and a 200-response schema. Update the
existing contribution_breakdown and schema assertions in the affected test
sections without loosening other scoring checks.
- Around line 38-52: Update
test_phishing_text_without_url_escalates_to_gemini_and_is_high_risk so the
endpoint-level assertion requires body["risk_grade"] to equal "HIGH" rather than
accepting "MEDIUM". Remove the exact stage1_naive_bayes risk_score == 97
assertion from this E2E test, leaving that classifier-internal value to unit
tests while preserving the existing successful response, analysis presence, and
null URL assertions.
---
Outside diff comments:
In `@app/utils/scoring_engine.py`:
- Around line 105-128: Update the confirmed-malicious override flow around
is_confirmed_malicious and ContributionBreakdown so contribution_breakdown
remains consistent with final_score after forcing the HIGH threshold. Adjust or
otherwise explicitly represent the override in the per-layer contribution data,
and update the related schema/response contract as needed so consumers can
reconstruct or identify the overridden score.
---
Nitpick comments:
In `@app/service/security/hybrid_url_engine.py`:
- Around line 72-74: Remove the unreachable base_score > 1.0 normalization
branch from the risk-score calculation in VirusTotalEngine.scan_url, leaving
risk_score to use the capped base_score directly with the existing max/min
logic.
In `@app/service/security/naive_bayes_text_analyzer.py`:
- Line 6: Move the shared determine_text_risk_grade helper out of
gemini_text_analyzer into a neutral shared module such as text_risk_grading.py,
then update both the naive-bayes analyzer and Gemini analyzer imports to use
that module while preserving existing behavior.
- Around line 73-77: Harden artifact loading in the analyzer initialization
around MODEL_PATH and VECTORIZER_PATH by validating each artifact’s trusted
checksum or signature before calling joblib.load. Reject mismatched or untrusted
files and preserve the existing model, threshold, classes, and vectorizer
initialization only after both validations succeed.
In `@tests/security/test_hybrid_url_engine.py`:
- Around line 40-66: Extend the VT consensus tests around
HybridUrlEngine.scan_url with a case where detected_count is exactly 5, keeping
the other scan mocks consistent with the existing threshold tests, and assert
that the result sets is_vt_confirmed to True.
🪄 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: 86b29831-cf7b-4965-8cef-c9fb0cfbf1cb
📒 Files selected for processing (18)
SCORING_PIPELINE_CHANGES.mdapp/dto/schemas.pyapp/service/scan_service.pyapp/service/security/hybrid_url_engine.pyapp/service/security/naive_bayes_text_analyzer.pyapp/service/security/rule_based_analyzer.pyapp/service/security/virustotal.pyapp/utils/scoring_engine.pyapp/utils/url_tracker.pyrequirements.txttests/e2e/test_analyze_endpoint.pytests/security/test_hybrid_url_engine.pytests/security/test_naive_bayes_text_analyzer.pytests/security/test_rule_based_analyzer.pytests/security/test_scoring_engine.pytests/security/test_virustotal.pytests/service/test_scan_service.pytests/url/test_url_tracer.py
| def test_phishing_text_without_url_escalates_to_gemini_and_is_high_risk(): | ||
| """ | ||
| 시나리오: URL 없이 기관 사칭 + 긴급성 유도 문구만 있는 전형적 스미싱 문자 -> | ||
| 나이브 베이즈가 의심 판정해 Gemini 2차 검증까지 실행되고, 최종 위험 등급도 높게 나와야 한다. | ||
| """ | ||
| text = "[검찰청] 귀하 명의로 대포통장이 개설되어 수사가 진행 중입니다. 즉시 아래 링크로 접속하여 신원을 확인하세요." | ||
| response = client.post("/api/analyze", json={"text": text}) | ||
|
|
||
| assert response.status_code == 200 | ||
| body = response.json() | ||
| assert body["status"] == "SUCCESS" | ||
| assert "stage1_naive_bayes" in body["text_analysis"] | ||
| assert body["text_analysis"]["stage1_naive_bayes"]["risk_score"] == 97 | ||
| assert body["risk_grade"] in ("MEDIUM", "HIGH") | ||
| assert body["url_analysis"] is None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the assertion with the test’s stated HIGH-risk contract.
This test is named is_high_risk, but it accepts MEDIUM, so a regression that downgrades this URL-less phishing case still passes. The exact 97 score also couples this E2E test to classifier internals; keep that exact value in unit tests and assert the endpoint-level behavior here.
Proposed assertion adjustment
- assert body["text_analysis"]["stage1_naive_bayes"]["risk_score"] == 97
- assert body["risk_grade"] in ("MEDIUM", "HIGH")
+ assert 0 <= body["text_analysis"]["stage1_naive_bayes"]["risk_score"] <= 100
+ assert body["risk_grade"] == "HIGH"📝 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.
| def test_phishing_text_without_url_escalates_to_gemini_and_is_high_risk(): | |
| """ | |
| 시나리오: URL 없이 기관 사칭 + 긴급성 유도 문구만 있는 전형적 스미싱 문자 -> | |
| 나이브 베이즈가 의심 판정해 Gemini 2차 검증까지 실행되고, 최종 위험 등급도 높게 나와야 한다. | |
| """ | |
| text = "[검찰청] 귀하 명의로 대포통장이 개설되어 수사가 진행 중입니다. 즉시 아래 링크로 접속하여 신원을 확인하세요." | |
| response = client.post("/api/analyze", json={"text": text}) | |
| assert response.status_code == 200 | |
| body = response.json() | |
| assert body["status"] == "SUCCESS" | |
| assert "stage1_naive_bayes" in body["text_analysis"] | |
| assert body["text_analysis"]["stage1_naive_bayes"]["risk_score"] == 97 | |
| assert body["risk_grade"] in ("MEDIUM", "HIGH") | |
| assert body["url_analysis"] is None | |
| def test_phishing_text_without_url_escalates_to_gemini_and_is_high_risk(): | |
| """ | |
| 시나리오: URL 없이 기관 사칭 + 긴급성 유도 문구만 있는 전형적 스미싱 문자 -> | |
| 나이브 베이즈가 의심 판정해 Gemini 2차 검증까지 실행되고, 최종 위험 등급도 높게 나와야 한다. | |
| """ | |
| text = "[검찰청] 귀하 명의로 대포통장이 개설되어 수사가 진행 중입니다. 즉시 아래 링크로 접속하여 신원을 확인하세요." | |
| response = client.post("/api/analyze", json={"text": text}) | |
| assert response.status_code == 200 | |
| body = response.json() | |
| assert body["status"] == "SUCCESS" | |
| assert "stage1_naive_bayes" in body["text_analysis"] | |
| assert 0 <= body["text_analysis"]["stage1_naive_bayes"]["risk_score"] <= 100 | |
| assert body["risk_grade"] == "HIGH" | |
| assert body["url_analysis"] is None |
🤖 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/e2e/test_analyze_endpoint.py` around lines 38 - 52, Update
test_phishing_text_without_url_escalates_to_gemini_and_is_high_risk so the
endpoint-level assertion requires body["risk_grade"] to equal "HIGH" rather than
accepting "MEDIUM". Remove the exact stage1_naive_bayes risk_score == 97
assertion from this E2E test, leaving that classifier-internal value to unit
tests while preserving the existing successful response, analysis presence, and
null URL assertions.
| assert { | ||
| "status", "message", "final_score", "risk_grade", | ||
| "contribution_breakdown", "text_analysis", "url_analysis" | ||
| }.issubset(body.keys()) | ||
|
|
||
| assert 0 <= body["final_score"] <= 100 | ||
| assert body["risk_grade"] in ("HIGH", "MEDIUM", "LOW") | ||
|
|
||
| # URL 없으면 그 30%가 LLM/규칙 트랙으로 재배분되어 상한이 65/35로 늘어남 | ||
| breakdown = body["contribution_breakdown"] | ||
| assert 0 <= breakdown["llm"] <= 65 | ||
| assert 0 <= breakdown["hybrid_url"] <= 30 | ||
| assert 0 <= breakdown["rules"] <= 35 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Strengthen the schema and scoring contract assertions.
The current checks are too permissive: a no-URL request may still contribute to hybrid_url, rule_analysis can disappear without failing the test, and an OpenAPI operation with no request or response schema would still pass. Assert hybrid_url == 0, require successful rule_analysis, and validate the POST request body plus 200-response schemas.
Also applies to: 111-118
🤖 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/e2e/test_analyze_endpoint.py` around lines 79 - 91, Strengthen the
assertions in the analyze endpoint test: require hybrid_url to equal 0 for
requests without a URL, require rule_analysis to be present and successful, and
validate that the OpenAPI POST operation defines both a request-body schema and
a 200-response schema. Update the existing contribution_breakdown and schema
assertions in the affected test sections without loosening other scoring checks.
…eously _combine_text_track_score returned llm_score whenever naive_bayes_score was None, without checking llm_available in that branch. Since NB being unavailable always routes through the Gemini fallback path in _analyze_text_hybrid, a simultaneous Gemini failure meant llm_score defaulted to 0 and the text track silently scored as safe - with the outage only visible if a consumer inspected text_analysis.result.grade. Add a fallback score (80) used only when both engines are down, sized so it alone clears the MEDIUM threshold even under the lowest track weight (URL-present, 50%), so total classifier outage surfaces as MEDIUM+ instead of being indistinguishable from a genuinely safe message.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/utils/url_tracker.py (1)
16-58: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDNS-rebinding TOCTOU still present: resolved IP is validated but discarded before the real connection.
_is_public_hostresolves and validates the hostname's IPs, buttrace_url(lines 86-98) then passes the original hostname string toclient.head/client.get, letting httpx re-resolve DNS independently at connection time. An attacker can return a safe IP for the validation lookup and a private/metadata IP for the connection lookup (or simply have DNS flip between the two lookups), bypassing the SSRF guard. This was already raised in a prior review round on this same function; it doesn't look to have been addressed. A durable fix needs a custom httpx transport that pins the connection to the already-validated IP (preservingHost/SNI for the original hostname) rather than re-resolving DNS per hop. Also applies to lines 86-98 where the validated hostname is handed to the client.🤖 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/utils/url_tracker.py` around lines 16 - 58, The SSRF validation in _is_public_host is vulnerable to DNS rebinding because trace_url passes the hostname to httpx, which resolves it again. Update _is_public_host and the trace_url request flow to return or retain the validated address, then use a custom httpx transport that connects to that pinned IP while preserving the original hostname for the Host header and TLS SNI on every redirect hop.
🧹 Nitpick comments (1)
app/service/security/naive_bayes_text_analyzer.py (1)
90-132: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSynchronous model load/inference blocks the asyncio event loop.
joblib.load(via_load_artifacts) and_model.predict_proba/_vectorizer.transformare CPU/IO-bound synchronous calls executed directly insideasync def analyze_text_with_naive_bayes, with no yielding to the loop. Since this coroutine runs concurrently with the Gemini/URL tracks viaasyncio.gatherinscan_service.py, every inference (and the one-time artifact load) stalls all other concurrently-scheduled requests on this worker for its duration.Consider offloading to a thread:
♻️ Proposed refactor
try: - import numpy as np - from scipy.sparse import csr_matrix, hstack - - text_norm = _normalize_text(text) - struct = np.array([_extract_struct_features(text)]) - - X_text = _vectorizer.transform([text_norm]) - X = hstack([X_text, csr_matrix(struct)]) - - phishing_idx = _classes.index("phishing") - prob_phishing = _model.predict_proba(X)[0][phishing_idx] - risk_score = int(prob_phishing * 100) + import asyncio + + def _infer(): + import numpy as np + from scipy.sparse import csr_matrix, hstack + text_norm = _normalize_text(text) + struct = np.array([_extract_struct_features(text)]) + X_text = _vectorizer.transform([text_norm]) + X = hstack([X_text, csr_matrix(struct)]) + phishing_idx = _classes.index("phishing") + prob = _model.predict_proba(X)[0][phishing_idx] + return prob, int(prob * 100) + + prob_phishing, risk_score = await asyncio.to_thread(_infer)Also worth wrapping
_load_artifacts()(or at least thejoblib.loadcalls) the same way for the cold-start case.🤖 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/naive_bayes_text_analyzer.py` around lines 90 - 132, Offload the synchronous artifact loading and inference work in analyze_text_with_naive_bayes to a worker thread so it does not block the asyncio event loop. Wrap _load_artifacts for cold-start loading and move _vectorizer.transform, feature construction, and _model.predict_proba into a synchronous helper invoked via asyncio.to_thread (or the project’s equivalent), while preserving the existing availability, result, and exception behavior.
🤖 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/scan_service.py`:
- Around line 118-129: Update the pipeline exception handler around the returned
SmishingAnalysisResponse to use RiskGrade.MEDIUM instead of RiskGrade.LOW for
hard failures, while preserving final_score=0 and the existing ERROR status and
message.
In `@tests/url/test_url_tracer.py`:
- Around line 7-30: Update the tests test_normal_url_no_redirect,
test_tinyurl_resolution, and test_broken_url_graceful_handling to mock
AsyncClient responses instead of contacting Google, TinyURL, or DNS. Configure
the mocks to represent no redirect, a resolved TinyURL destination, and the
timeout/error fallback respectively, while preserving each test’s existing
assertions; leave live checks for a separately marked integration suite.
- Around line 87-105: The direct-target SSRF tests must verify blocking occurs
before network I/O, not only by comparing the returned URL. In
test_ssrf_guard_blocks_loopback_ip_literal,
test_ssrf_guard_blocks_cloud_metadata_ip, and
test_ssrf_guard_blocks_private_lan_ip, patch httpx.AsyncClient.head and assert
the mock was never awaited after calling trace_url.
---
Duplicate comments:
In `@app/utils/url_tracker.py`:
- Around line 16-58: The SSRF validation in _is_public_host is vulnerable to DNS
rebinding because trace_url passes the hostname to httpx, which resolves it
again. Update _is_public_host and the trace_url request flow to return or retain
the validated address, then use a custom httpx transport that connects to that
pinned IP while preserving the original hostname for the Host header and TLS SNI
on every redirect hop.
---
Nitpick comments:
In `@app/service/security/naive_bayes_text_analyzer.py`:
- Around line 90-132: Offload the synchronous artifact loading and inference
work in analyze_text_with_naive_bayes to a worker thread so it does not block
the asyncio event loop. Wrap _load_artifacts for cold-start loading and move
_vectorizer.transform, feature construction, and _model.predict_proba into a
synchronous helper invoked via asyncio.to_thread (or the project’s equivalent),
while preserving the existing availability, result, and exception behavior.
🪄 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: 45aad74d-2369-4b33-b72c-e82691ba1e72
📒 Files selected for processing (18)
SCORING_PIPELINE_CHANGES.mdapp/dto/schemas.pyapp/service/scan_service.pyapp/service/security/hybrid_url_engine.pyapp/service/security/naive_bayes_text_analyzer.pyapp/service/security/rule_based_analyzer.pyapp/service/security/virustotal.pyapp/utils/scoring_engine.pyapp/utils/url_tracker.pyrequirements.txttests/e2e/test_analyze_endpoint.pytests/security/test_hybrid_url_engine.pytests/security/test_naive_bayes_text_analyzer.pytests/security/test_rule_based_analyzer.pytests/security/test_scoring_engine.pytests/security/test_virustotal.pytests/service/test_scan_service.pytests/url/test_url_tracer.py
🚧 Files skipped from review as they are similar to previous changes (9)
- app/service/security/virustotal.py
- tests/security/test_rule_based_analyzer.py
- tests/security/test_virustotal.py
- app/service/security/rule_based_analyzer.py
- tests/e2e/test_analyze_endpoint.py
- app/dto/schemas.py
- app/service/security/hybrid_url_engine.py
- tests/security/test_hybrid_url_engine.py
- app/utils/scoring_engine.py
…SSRF _is_public_host validated a hostname's DNS resolution but discarded the resolved IP, letting httpx re-resolve at connect time — an attacker with a short-TTL record could pass validation then rebind to a private/metadata IP before the actual request. Now the validated IP is pinned via a custom httpx transport/network backend (SNI and Host header stay on the original hostname, so TLS validation is unaffected), and the getaddrinfo call is wrapped in a timeout to avoid hanging on unresponsive resolvers.
5bcacef to
c6d2149
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/utils/url_tracker.py (2)
164-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
limitsis ignored when a custom transport is passed.
AsyncClientonly applieslimitsto the transport it builds itself; here the pool limits come from_PinnedIPTransport. Drop it to avoid implying otherwise.🤖 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/utils/url_tracker.py` around lines 164 - 167, Remove the unused limits configuration and the limits argument from the httpx.AsyncClient construction in the URL tracking flow, since the custom _PinnedIPTransport controls connection pooling. Keep the existing transport and redirect behavior unchanged.
113-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid relying on httpx’s private SSL config helper.
_PinnedIPTransport.__init__callshttpx._config.create_ssl_contextand skipssuper().__init__(), so a minor httpx upgrade can break this transport by changing a private API. Build the SSL context explicitly instead (or forward through the public transport constructor) and consider a narrower httpx/httpcore range for the pinned connection pool.🤖 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/utils/url_tracker.py` around lines 113 - 129, Update _PinnedIPTransport.__init__ to stop importing httpx._config.create_ssl_context and avoid depending on that private API; construct the required SSL context through supported public APIs or reuse the public transport initialization path while preserving the custom _PinnedIPBackend connection pool. Ensure the pinned pool remains compatible with the supported httpx/httpcore versions.Source: Linters/SAST tools
🤖 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/utils/url_tracker.py`:
- Around line 186-189: After normalizing Location with urljoin in the
redirect-tracking flow, validate the resulting current_url scheme against an
HTTP/HTTPS allowlist before assigning or returning it. Reject non-HTTP(S)
schemes such as ftp, javascript, and data so they cannot become the final URL or
trigger the next request; use the existing redirect/error handling path where
appropriate.
---
Nitpick comments:
In `@app/utils/url_tracker.py`:
- Around line 164-167: Remove the unused limits configuration and the limits
argument from the httpx.AsyncClient construction in the URL tracking flow, since
the custom _PinnedIPTransport controls connection pooling. Keep the existing
transport and redirect behavior unchanged.
- Around line 113-129: Update _PinnedIPTransport.__init__ to stop importing
httpx._config.create_ssl_context and avoid depending on that private API;
construct the required SSL context through supported public APIs or reuse the
public transport initialization path while preserving the custom
_PinnedIPBackend connection pool. Ensure the pinned pool remains compatible with
the supported httpx/httpcore versions.
🪄 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: 285538a5-3dcb-43ef-ad2b-de5cec8261b4
📒 Files selected for processing (1)
app/utils/url_tracker.py
| if not location.startswith(("http://", "https://")): | ||
| location = urljoin(current_url, location) | ||
|
|
||
| current_url = location |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add a scheme allowlist after normalization.
urljoin returns any absolute non-http(s) Location verbatim (ftp://…, javascript:…, data:…). That value becomes current_url, the next hop raises inside httpx, the generic handler breaks the loop, and the non-http URL is returned to callers as the "final" URL.
🛡️ Proposed fix
# 절대 URL이 아닌 모든 경우(경로만/프로토콜 상대경로 등)를 urljoin으로 정규화
if not location.startswith(("http://", "https://")):
location = urljoin(current_url, location)
+ # http(s) 이외의 스킴으로의 리다이렉트는 추적 대상이 아니므로 중단
+ if urlparse(location).scheme not in ("http", "https"):
+ logger.warning(f"지원하지 않는 스킴으로의 리다이렉트 차단: {location}")
+ break
+
current_url = location📝 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.
| if not location.startswith(("http://", "https://")): | |
| location = urljoin(current_url, location) | |
| current_url = location | |
| if not location.startswith(("http://", "https://")): | |
| location = urljoin(current_url, location) | |
| # http(s) 이외의 스킴으로의 리다이렉트는 추적 대상이 아니므로 중단 | |
| if urlparse(location).scheme not in ("http", "https"): | |
| logger.warning(f"지원하지 않는 스킴으로의 리다이렉트 차단: {location}") | |
| break | |
| current_url = location |
🤖 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/utils/url_tracker.py` around lines 186 - 189, After normalizing Location
with urljoin in the redirect-tracking flow, validate the resulting current_url
scheme against an HTTP/HTTPS allowlist before assigning or returning it. Reject
non-HTTP(S) schemes such as ftp, javascript, and data so they cannot become the
final URL or trigger the next request; use the existing redirect/error handling
path where appropriate.
…nt test, fix stale test drift - SSRF guard tests for direct blocked-IP targets now patch httpx.AsyncClient.head and assert it was never awaited, so a passing result actually proves the guard ran before any connection instead of just matching by coincidence. - e2e analyze endpoint test mocks app.service.scan_service.trace_url instead of hitting google.com live, removing outbound DNS/HTTP dependency from CI; redirect behavior stays covered by the dedicated URL-tracer tests. - Fixed 3 test files broken by prior refactors: gemini_text_analyzer test now patches the actual MOCK_ENABLED constant instead of a removed is_mock_enabled function, url_extractor test points at the current app.utils.url_tracker location, and removed parser.py + its test (dead code superseded by hybrid_url_engine.py, never imported anywhere, and buggy on its live path).
… into integration suite Tests were hitting google.com, tinyurl.com, and a nonexistent domain over real DNS/HTTP, making them flaky or impossible offline/in CI. Unit tests now mock httpx.AsyncClient.head and _is_public_host so the redirect/DNS-failure paths are deterministic. The same live checks move to tests/url/test_url_tracer_integration.py under a new `integration` pytest marker, excluded by default via pytest.ini and run explicitly with `pytest -m integration`.
analyze_pipeline's outer exception handler returned final_score=0, risk_grade=LOW on any unhandled failure (network error, bug, etc.) - the same "confirmed safe" outcome the BOTH_ENGINES_UNAVAILABLE_FALLBACK_SCORE guard was added to prevent for the narrower case of both text classifiers being down. A consumer reading risk_grade/final_score without checking status=="ERROR" would see an unanalyzed message as low risk. Now the fallback forces both the grade (MEDIUM) and the score (ScoringEngine.PIPELINE_FAILURE_FALLBACK_SCORE) together so the response stays internally consistent instead of just swapping the enum value.
…alicious override The GSB/VT/rule-domain confirmed-malicious override forced final_score up to 70+ but left llm_contrib/url_contrib/rules_contrib (the values fed into contribution_breakdown) at their pre-override sum, so the breakdown could sit tens of points below the score it's supposed to explain. Now the score gap introduced by the override is distributed into the breakdown too - url first (the override's usual source), spilling into rules then llm if url_contrib is already at its schema cap - so llm+hybrid_url+rules always equals final_score.
1 participant
Summary
POST /api/analyze) 기준 E2E 통합 테스트 추가세부 내용은
SCORING_PIPELINE_CHANGES.md참고.Test plan
pytest tests/신규/수정 대상 59개 전부 통과 (naive bayes, rule engine, hybrid url engine, virustotal, scoring engine, scan service, url tracer, e2e)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
rule_analysisin results.Documentation
Tests