fix(tools): block CGNAT and non-global IPs in validate_url - #6872
fix(tools): block CGNAT and non-global IPs in validate_url#6872SashaMIT wants to merge 2 commits into
Conversation
The hand-maintained SSRF blocklist missed CGNAT (100.64/10) and other non-global ranges that Python marks is_global=False. Host scrapers using safe_get / validate_url (and Selenium after crewAIInc#6870) could still reach them. Classify with is_global, expand 0.0.0.0/8, add 100.64/10, and cover with tests. Signed-off-by: Sasha Mitchell <sash.t.mitchell@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesSafe URL IP validation
🚥 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.
🧹 Nitpick comments (1)
lib/crewai-tools/tests/utilities/test_safe_path.py (1)
157-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for Class E addresses.
The new tests cover CGNAT, benchmarking, documentation, and
0.0.0.0/8, but they do not cover the Class E range that_is_private_or_reservednow claims to reject. Add a behavior test for240.0.0.1with the sameValueErrorassertion.Suggested regression test
+ def test_blocks_class_e(self): + with pytest.raises(ValueError, match="private/reserved IP"): + validate_url("http://240.0.0.1/")As per coding guidelines,
**/*test*.py: Write unit tests for new functionality, focusing on behavior rather than implementation details. The PR objective includes Class E-style ranges in the addresses thatvalidate_urlmust reject.🤖 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 `@lib/crewai-tools/tests/utilities/test_safe_path.py` around lines 157 - 173, Add a regression test alongside the existing reserved-address tests that calls validate_url with http://240.0.0.1/ and asserts ValueError matching "private/reserved IP", covering rejection of Class E addresses.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@lib/crewai-tools/tests/utilities/test_safe_path.py`:
- Around line 157-173: Add a regression test alongside the existing
reserved-address tests that calls validate_url with http://240.0.0.1/ and
asserts ValueError matching "private/reserved IP", covering rejection of Class E
addresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a83de09-c7a4-423a-a01e-1163937b6abd
📒 Files selected for processing (2)
lib/crewai-tools/src/crewai_tools/security/safe_path.pylib/crewai-tools/tests/utilities/test_safe_path.py
|
the also
same unwrap trick you're doing for |
is_global is True for multicast and for many 64:ff9b::/96 embeds, so reject multicast and unwrap the well-known NAT64 prefix like ::ffff:.
1ddb026 to
05c67d4
Compare
|
Good catch @manjunathbhaskar — folded in on
Added regression tests for both. Ready for another look. |
Summary
Sibling of #6870. Shared
validate_url(used by host scrapers viasafe_get, and by Selenium once #6870 lands) only checked a hand-maintained IPv4 list. That list missed:100.64.0.0/10(is_privateis False there)0.0.0.0/8beyond0.0.0.0/32So a prompt-injected scrape URL to e.g.
http://100.64.0.1/passed validation at tip92012aec.Fix
not ipaddress…is_globalas blocked (covers CGNAT)0.0.0.0/8and add100.64.0.0/10Testing
uv run pytest tests/utilities/test_safe_path.py→ 36 passed (new CGNAT / 198.18 / 192.0.2 / 0.0.0.1 cases).Made with Cursor