fix(rules): detect current ngrok tunnel domains and add bore.pub/serveo.net/localtunnel.me - #132
Conversation
…et/localtunnel.me ngrok migrated off ngrok.io to ngrok-free.dev / ngrok.app, so exfil to a current ngrok endpoint slipped past the suspicious-domain checks. Add the modern ngrok domains plus bore.pub, serveo.net and localtunnel.me to: - ContextExtractor.SUSPICIOUS_DOMAINS (Python string-literal URL classification) - tool_chaining_abuse_generic.yara / command_injection_generic.yara exfil dests - cross_skill_scanner exfil pattern list Add regression tests for the new domains (YARA true-positives + ContextExtractor). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR expands tunnel and exfiltration detection. It adds normalized URL hostname matching, new suspicious tunnel domains, broader CrossSkillScanner and YARA patterns, and regression tests for tunnel and curl-based exfiltration cases. ChangesTunnel domain detection expansion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_yara_true_positives.py (1)
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate tunnel-host lists across two parametrized tests.
Both lists overlap almost entirely and could drift if new hosts are added to one but not the other. Consider extracting a shared
TUNNEL_HOSTSconstant.Also applies to: 235-235
🤖 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_yara_true_positives.py` at line 182, The parametrized tunnel-host values are duplicated across the true-positive tests and can drift over time. Extract the shared host list into a single constant such as TUNNEL_HOSTS, then reuse it in both parametrized tests that reference the overlap so there is one source of truth.
🤖 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/static_analysis/context_extractor.py`:
- Around line 188-196: The tunnel-domain detection in context_extractor.py is
shadowed by the broad legitimate-domain substring check, so localhost.run can
never reach the suspicious-domain branch. Update the logic around the domain
matching in the extractor method to avoid raw substring containment for
LEGITIMATE_DOMAINS and suspicious domains, using exact or suffix-based host
matching instead. Make sure the check order and matching rules in the relevant
domain-scanning function still allow localhost.run and similar tunnel hosts to
be classified as suspicious.
---
Nitpick comments:
In `@tests/test_yara_true_positives.py`:
- Line 182: The parametrized tunnel-host values are duplicated across the
true-positive tests and can drift over time. Extract the shared host list into a
single constant such as TUNNEL_HOSTS, then reuse it in both parametrized tests
that reference the overlap so there is one source of truth.
🪄 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: 31fbda83-1376-4440-b03b-ff1a64dffa86
📒 Files selected for processing (6)
skill_scanner/core/analyzers/cross_skill_scanner.pyskill_scanner/core/static_analysis/context_extractor.pyskill_scanner/data/packs/core/yara/command_injection_generic.yaraskill_scanner/data/packs/core/yara/tool_chaining_abuse_generic.yaratests/static_analysis/test_suspicious_url_domains.pytests/test_yara_true_positives.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.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- _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.
Actionable comments posted: 1
🤖 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/static_analysis/context_extractor.py`:
- Around line 376-385: Update the URL filtering logic in the context extraction
flow around hostname parsing to parse each URL first and continue unless
parsed.scheme.lower() is exactly "http" or "https"; this must accept uppercase
HTTPS and reject schemes such as httpx before suspicious-domain matching. Add
regression coverage for https://abc.ngrok.app/... and httpx://abc.ngrok.app/...
preserving the expected suspicious-domain 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: fa261ef0-7eb7-4de1-bf18-ac120353a89f
📒 Files selected for processing (2)
skill_scanner/core/static_analysis/context_extractor.pytests/static_analysis/test_suspicious_url_domains.py
Summary
The suspicious-domain lists and tunnel-service YARA rules only recognized the legacy
ngrok.iohost and a couple of others. Modern ngrok deployments usengrok-free.dev,ngrok-free.app, andngrok.app, and several other tunnel/reverse-proxy services (bore.pub,serveo.net,localtunnel.me) were not covered at all — so a skill exfiltrating data through those endpoints would go undetected.This PR expands the tunnel/proxy coverage consistently across all the places these hosts are matched:
core/static_analysis/context_extractor.py—SUSPICIOUS_DOMAINScore/analyzers/cross_skill_scanner.py—EXFIL_PATTERNSdata/packs/core/yara/tool_chaining_abuse_generic.yara—$exfil_requestbindata/packs/core/yara/command_injection_generic.yara—$data_exfiltration_known_destTest plan
tests/static_analysis/test_suspicious_url_domains.pyassert the new tunnel hosts are flagged and legitimate API hosts are not.tests/test_yara_true_positives.py.uv run pytest tests/passes.uv run pre-commit run --all-filespasses.uv run python evals/runners/benchmark_runner.py— 100% precision/recall, no regression.Made with Cursor
Summary by CodeRabbit