Repository navigation
feat: #140 add trusted_reference_domains to LLM analysis policy - #141
Conversation
Add a policy-driven mechanism to demote LLM findings (transitive trust, supply chain) to LOW when all referenced domains are declared as trusted in the scan policy. This addresses systematic false positives for organizations hosting skills and documentation on self-managed GitLab/GitHub instances, where the LLM analyzer flags references to internal repos as external threats. Changes: - LLMAnalysisPolicy: new trusted_reference_domains field (set[str]) - llm_analyzer._convert_to_findings: demotion logic after existing is_internal_file_reading filter - Two helpers: _references_only_trusted_domains (URL extraction) and _mentions_only_trusted_domains (plain-text domain detection) - default_policy.yaml: empty default list with documentation The mechanism mirrors known_installer_domains: findings are demoted to LOW (maintaining visibility) rather than suppressed entirely.
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds ChangesTrusted Reference Domains Feature
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant LLMAnalyzer
participant ScanPolicy
participant Finding
LLMAnalyzer->>ScanPolicy: read trusted_reference_domains
LLMAnalyzer->>LLMAnalyzer: check URLs and domain mentions
LLMAnalyzer->>Finding: demote matching finding severity to LOW
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skill_scanner/core/analyzers/llm_analyzer.py (1)
750-779: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTrailing punctuation after a bare domain URL breaks trust matching.
_URL_PATTERNdoesn't exclude trailing punctuation (.,,, etc.), so a domain immediately followed by punctuation (no path) won't match a trusted entry, since the captured string retains the punctuation. This fails safe (no demotion) rather than being exploitable, but it's an untested edge case worth hardening.♻️ Optional hardening
- for domain in urls: - # Strip port if present (e.g. "gitlab.example.com:8443") - domain_no_port = domain.split(":")[0].lower() + for domain in urls: + # Strip port and trailing punctuation (e.g. "gitlab.example.com:8443," or "gitlab.example.com.") + domain_no_port = domain.split(":")[0].rstrip(".,;)\"'").lower()🤖 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 `@skill_scanner/core/analyzers/llm_analyzer.py` around lines 750 - 779, The trust-check in _references_only_trusted_domains is overly strict for bare URLs because _URL_PATTERN can capture trailing punctuation, causing trusted domains to miss exact matching. Update the URL handling in _references_only_trusted_domains so each extracted domain is normalized to remove trailing punctuation before the existing port-stripping and trusted_reference_domains comparison, keeping the current behavior for untrusted or missing URLs.
🤖 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 `@skill_scanner/core/analyzers/llm_analyzer.py`:
- Around line 606-621: Severity demotion is currently driven by free-text
matches in the LLM `description`, which is too loose and brittle. Update the
demotion logic in `llm_analyzer.py` to key off the parsed
`aitech_code`/`category` for the intended AITech-1.2 and supply-chain codes,
matching the same scoped pattern used by `is_internal_file_reading`, and remove
the dead `not is_internal_file_reading` check since that branch already
continues earlier. Keep the trusted-domain checks in place, but only apply the
LOW severity demotion when the finding’s classified code indicates the relevant
AITech category.
---
Nitpick comments:
In `@skill_scanner/core/analyzers/llm_analyzer.py`:
- Around line 750-779: The trust-check in _references_only_trusted_domains is
overly strict for bare URLs because _URL_PATTERN can capture trailing
punctuation, causing trusted domains to miss exact matching. Update the URL
handling in _references_only_trusted_domains so each extracted domain is
normalized to remove trailing punctuation before the existing port-stripping and
trusted_reference_domains comparison, keeping the current behavior for untrusted
or missing URLs.
🪄 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
Run ID: 028ebb9f-fb5c-4ba4-8af2-8afccbaae096
📒 Files selected for processing (6)
.gitignoredocs/user-guide/custom-policy-configuration.mdskill_scanner/core/analyzers/llm_analyzer.pyskill_scanner/core/scan_policy.pyskill_scanner/data/default_policy.yamltests/test_llm_analyzer.py
ProviderConfig.validate() required a truthy credential for every provider except Bedrock and Ollama, and the only credential source it checked for Vertex was GOOGLE_APPLICATION_CREDENTIALS. This blocked ambient auth via a GCE/Cloud Run attached service account or Workload Identity, even though LiteLLM/google-auth already fall back to it automatically when no explicit credential is passed -- the same pattern already supported for Bedrock's IAM role. Excludes is_vertex from the check, mirroring the Bedrock/Ollama precedent, and documents the fallback.
- _resolve_api_key() now returns None for Vertex instead of the GOOGLE_APPLICATION_CREDENTIALS path, since vertex_ai/gemini-* models set both is_vertex and is_gemini, which was causing the file path to be written into GEMINI_API_KEY. - Regenerated configuration-reference.md via generate_reference_docs.py instead of hand-editing, and updated the underlying descriptions so the doc doesn't drift on next regeneration. Addresses CodeRabbit review feedbak on cisco-ai-defense#144.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_llm_analyzer.py (1)
921-928: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire all referenced domains to pass the trust check.
The mixed URL test checks
_references_only_trusted_domainsin isolation. It does not test_convert_to_findings. The analyzer combines that result with_mentions_only_trusted_domains.
_mentions_only_trusted_domainsreturns true when any trusted-domain substring appears. A finding that contains bothgitlab.example.comandevil.comcan therefore be demoted toLOW. The same substring check can trust a look-alike such asevilgitlab.example.com.Add end-to-end mixed URL, mixed prose, and look-alike-domain cases for both
AITech-1.2andAITech-9.3. Parse domain boundaries and require every referenced domain to be trusted before demotion.This conflicts with the PR objective that demotion applies only when all referenced domains are trusted.
Suggested regression cases
+ def test_mentions_rejects_mixed_and_lookalike_domains(self): + analyzer = self._make_analyzer_with_trusted_domains(["gitlab.example.com"]) + assert not analyzer._mentions_only_trusted_domains( + "gitlab.example.com/repo and evil.com/repo" + ) + assert not analyzer._mentions_only_trusted_domains( + "evilgitlab.example.com/repo" + )Also applies to: 957-973, 995-1013, 1111-1140
🤖 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/test_llm_analyzer.py` around lines 921 - 928, Update the analyzer’s end-to-end demotion logic in _convert_to_findings and the trust helpers _references_only_trusted_domains/_mentions_only_trusted_domains so every referenced domain must be explicitly trusted, using domain-boundary parsing rather than substring matching. Add coverage for mixed URLs, mixed prose, and look-alike domains for both AITech-1.2 and AITech-9.3, while preserving demotion only when all references pass the trust check.
🤖 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.
Outside diff comments:
In `@tests/test_llm_analyzer.py`:
- Around line 921-928: Update the analyzer’s end-to-end demotion logic in
_convert_to_findings and the trust helpers
_references_only_trusted_domains/_mentions_only_trusted_domains so every
referenced domain must be explicitly trusted, using domain-boundary parsing
rather than substring matching. Add coverage for mixed URLs, mixed prose, and
look-alike domains for both AITech-1.2 and AITech-9.3, while preserving demotion
only when all references pass the trust check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 443a9e02-ebb1-404e-9f66-7bbfa6fbb107
📒 Files selected for processing (2)
skill_scanner/core/analyzers/llm_analyzer.pytests/test_llm_analyzer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- skill_scanner/core/analyzers/llm_analyzer.py
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Pull Request
Description
Add a policy-driven mechanism to demote LLM findings (transitive trust, supply chain) to LOW when all referenced domains are declared as trusted in the scan policy.
This addresses systematic false positives for organizations hosting skills and documentation on self-managed GitLab/GitHub instances, where the LLM analyzer flags references to internal repos as external threats.
The mechanism mirrors known_installer_domains: findings are demoted to LOW (maintaining visibility) rather than suppressed entirely.
Type of Change
Related Issues
Fixes #140
Changes Made
Testing
Test Coverage
Manual Testing
Describe manual testing performed:
# Commands run for testing skill-scanner scan examples/test-skillResults:
Checklist
Code Quality
Documentation
Security
Testing
uv run pre-commit run --all-filesuv run python evals/runners/benchmark_runner.pyPerformance Impact
Screenshots (if applicable)
Add screenshots or output examples if relevant.
Additional Notes
Any additional information reviewers should know.
Reviewer Checklist
For reviewers:
Summary by CodeRabbit
New Features
Documentation
Chores