fix(security): close SSRF in ArxivPaperTool.download_pdf, plus a DNS-rebinding gap in safe_get - #6795
fix(security): close SSRF in ArxivPaperTool.download_pdf, plus a DNS-rebinding gap in safe_get#6795Eman-Yousaf wants to merge 5 commits into
Conversation
…ebinding gap in safe_get Fixes crewAIInc#6694. ## What's actually vulnerable The issue report names both urllib calls in ArxivPaperTool, but only one is a real SSRF vector: - fetch_arxiv_data()'s urlopen call targets a hardcoded BASE_API_URL (export.arxiv.org); search_query only ever lands in the query string, percent-encoded. Not an SSRF vector for destination redirection. It was plain http://, though -- a real but separate weakness (a network MITM can tamper with the API response). Switched to https:// and migrated to safe_get() for consistency and redirect-safety while touching this. - download_pdf()'s urlretrieve call is the real vector: pdf_url comes from parsing the arxiv API's XML *response* (an href attribute), not directly from search_query. Whatever URL shows up there was fetched and written to disk with zero validation -- reachable via the same plain-HTTP MITM angle, or a malicious link ever indexed upstream. Migrated to safe_download() (new helper, see below). ## Additional finding: DNS-rebinding TOCTOU in the existing safe_requests infra crewai_tools.security already has real SSRF protection (validate_url, safe_get) that both of the above should have been using from the start. While wiring the arxiv tool into it, found a real gap in the shared infrastructure itself: validate_url() resolves DNS, checks the IP, and returns the *original URL string* -- it doesn't pin the actual connection to the address it just validated. requests.get() then re-resolves DNS itself at connection time. An attacker controlling DNS (or using a short-TTL record) can present a safe IP for validation and a private one for the real connection moments later. Fixed by: - safe_path.validate_and_resolve(url) -> (url, ip): shares validate_url's exact logic but also returns the checked IP. validate_url/ resolve_validated_ip are now thin wrappers around it (validate_url's public signature/behavior is unchanged). - safe_requests._pin_dns(hostname, ip): a context manager that pins socket.getaddrinfo(hostname, ...) to the validated IP for the duration of one request. safe_get() now validates+pins on the initial URL and on every redirect hop, closing the window for both direct requests and redirect-based rebinding. - safe_requests.safe_download(url, dest_path): new helper for streaming a validated, pinned download to disk (safe_get didn't have an equivalent for arxiv_paper_tool's use case). ## Testing - New regression tests: - test_safe_get_pins_dns_against_rebinding: proves the fix directly -- mocks getaddrinfo to return a safe IP on the first lookup and a private one on any subsequent lookup (simulating rebinding), and asserts the connection never sees the private IP. Fails against the pre-fix code. - test_safe_get_restores_real_resolver_after_pinning: pinning doesn't leak past the request it was applied to. - safe_download: writes-to-disk, blocks private IP, blocks redirect to private IP. - ArxivPaperTool.download_pdf: blocks 127.0.0.1 and the AWS/GCP/Azure metadata endpoint end-to-end (not mocked at the safe_download level), proving the tool itself is protected, not just the helper in isolation. - Existing arxiv_paper_tool_test.py tests updated to mock safe_get/ safe_download instead of urllib (the code path changed; test intent didn't). - Full suite: `uv run --package crewai-tools pytest lib/crewai-tools/tests/utilities/test_safe_path.py lib/crewai-tools/tests/utilities/test_safe_requests.py lib/crewai-tools/tests/tools/arxiv_paper_tool_test.py` -- 55 passed. - `ruff check` clean on all changed files.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughURL validation now returns checked IP addresses. Safe requests pin DNS resolution across redirects and add streamed downloads. Arxiv API and PDF retrieval now use these helpers, with updated security and error-handling tests. ChangesSecure networking and Arxiv integration
Sequence Diagram(s)sequenceDiagram
participant ArxivPaperTool
participant safe_get
participant validate_and_resolve
participant Requests
participant safe_download
ArxivPaperTool->>safe_get: fetch HTTPS API URL
safe_get->>validate_and_resolve: validate and resolve URL
validate_and_resolve-->>safe_get: validated URL and IP
safe_get->>Requests: perform DNS-pinned request
Requests-->>ArxivPaperTool: API response text
ArxivPaperTool->>safe_download: download PDF with timeout
safe_download->>Requests: stream validated PDF response
safe_download-->>ArxivPaperTool: completed file
Suggested reviewers: 🚥 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 (3)
lib/crewai-tools/tests/tools/arxiv_paper_tool_test.py (2)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the arguments passed to
safe_download.
assert_called_once()passes even ifdownload_pdfsends the wrong URL, destination, or no timeout.download_pdfconvertssave_pathtostrand forwardsREQUEST_TIMEOUT. Assert that contract.♻️ Proposed test tightening
def test_download_pdf_success(mock_safe_download): tool = ArxivPaperTool() tool.download_pdf("http://arxiv.org/pdf/1234.5678.pdf", Path("test.pdf")) - mock_safe_download.assert_called_once() + mock_safe_download.assert_called_once_with( + "http://arxiv.org/pdf/1234.5678.pdf", + "test.pdf", + timeout=ArxivPaperTool.REQUEST_TIMEOUT, + )🤖 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/tools/arxiv_paper_tool_test.py` around lines 49 - 53, Update test_download_pdf_success to assert the exact arguments passed to safe_download, verifying the original PDF URL, the destination converted from Path("test.pdf") to its string form, and the configured REQUEST_TIMEOUT value; retain the single-call assertion.
68-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear the unsafe-paths escape hatch so these end-to-end tests stay hermetic.
Both tests exercise the real
safe_downloadchain.validate_and_resolvereturns early and skips validation whenCREWAI_TOOLS_ALLOW_UNSAFE_PATHSis set. If that variable is set in a developer or CI environment, these tests stop asserting the SSRF protection and instead attempt real requests to127.0.0.1and169.254.169.254, which can hang until the timeout. Remove the variable in the tests withmonkeypatch.delenv(..., raising=False).💚 Proposed fix
-def test_download_pdf_blocks_private_ip(): +def test_download_pdf_blocks_private_ip(monkeypatch): """Regression test for the SSRF fix (`#6694`): download_pdf must reject a URL that resolves to a private/reserved IP before making any request, end to end through the real safe_download/safe_get/validate_and_resolve chain -- not mocked out, so this exercises the actual protection. """ + monkeypatch.delenv("CREWAI_TOOLS_ALLOW_UNSAFE_PATHS", raising=False) tool = ArxivPaperTool() with pytest.raises(ValueError, match="private/reserved IP"): tool.download_pdf("http://127.0.0.1/malicious.pdf", Path("test.pdf")) -def test_download_pdf_blocks_cloud_metadata_endpoint(): +def test_download_pdf_blocks_cloud_metadata_endpoint(monkeypatch): """Same as above, for the AWS/GCP/Azure metadata endpoint specifically -- the concrete credential-theft scenario named in `#6694`.""" + monkeypatch.delenv("CREWAI_TOOLS_ALLOW_UNSAFE_PATHS", raising=False) tool = ArxivPaperTool() with pytest.raises(ValueError, match="private/reserved IP"): tool.download_pdf( "http://169.254.169.254/latest/meta-data/", Path("test.pdf") )🤖 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/tools/arxiv_paper_tool_test.py` around lines 68 - 86, Update test_download_pdf_blocks_private_ip and test_download_pdf_blocks_cloud_metadata_endpoint to accept pytest's monkeypatch fixture and remove CREWAI_TOOLS_ALLOW_UNSAFE_PATHS with monkeypatch.delenv(..., raising=False) before invoking ArxivPaperTool.download_pdf, ensuring both end-to-end SSRF checks cannot use the unsafe-paths escape hatch.lib/crewai-tools/src/crewai_tools/security/safe_requests.py (1)
155-163: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrite the download to a temporary file, then rename it.
If the stream fails mid-transfer,
safe_downloadleaves a truncated file atdest_path. A caller such asArxivPaperTool.download_pdfcannot distinguish a truncated PDF from a complete one.♻️ Proposed atomic write
response = safe_get(url, max_redirects=max_redirects, stream=True, **kwargs) + dest = Path(dest_path) + tmp_path = dest.with_name(f"{dest.name}.part") try: response.raise_for_status() - with open(dest_path, "wb") as fh: + with open(tmp_path, "wb") as fh: for chunk in response.iter_content(chunk_size=chunk_size): if chunk: fh.write(chunk) + tmp_path.replace(dest) + except BaseException: + tmp_path.unlink(missing_ok=True) + raise finally: response.close()🤖 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/src/crewai_tools/security/safe_requests.py` around lines 155 - 163, Update safe_download to write the streamed response to a temporary file in the destination directory, then atomically rename it to dest_path only after the transfer completes successfully. Ensure failed or interrupted downloads clean up the temporary file and never leave a truncated dest_path, while preserving response closure and existing status validation.
🤖 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 `@lib/crewai-tools/src/crewai_tools/security/safe_requests.py`:
- Around line 55-85: Update _pin_dns so patching, using, and restoring
socket.getaddrinfo cannot overlap across concurrent requests: guard the entire
context-manager window with a shared lock, acquiring it before replacing the
resolver and releasing it only after restoration in finally. Preserve delegation
for non-matching hosts and the existing pinned-address behavior.
---
Nitpick comments:
In `@lib/crewai-tools/src/crewai_tools/security/safe_requests.py`:
- Around line 155-163: Update safe_download to write the streamed response to a
temporary file in the destination directory, then atomically rename it to
dest_path only after the transfer completes successfully. Ensure failed or
interrupted downloads clean up the temporary file and never leave a truncated
dest_path, while preserving response closure and existing status validation.
In `@lib/crewai-tools/tests/tools/arxiv_paper_tool_test.py`:
- Around line 49-53: Update test_download_pdf_success to assert the exact
arguments passed to safe_download, verifying the original PDF URL, the
destination converted from Path("test.pdf") to its string form, and the
configured REQUEST_TIMEOUT value; retain the single-call assertion.
- Around line 68-86: Update test_download_pdf_blocks_private_ip and
test_download_pdf_blocks_cloud_metadata_endpoint to accept pytest's monkeypatch
fixture and remove CREWAI_TOOLS_ALLOW_UNSAFE_PATHS with monkeypatch.delenv(...,
raising=False) before invoking ArxivPaperTool.download_pdf, ensuring both
end-to-end SSRF checks cannot use the unsafe-paths escape hatch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 60c95f76-8cce-4aa2-9c00-7464e832ba90
📒 Files selected for processing (5)
lib/crewai-tools/src/crewai_tools/security/safe_path.pylib/crewai-tools/src/crewai_tools/security/safe_requests.pylib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.pylib/crewai-tools/tests/tools/arxiv_paper_tool_test.pylib/crewai-tools/tests/utilities/test_safe_requests.py
There was a problem hiding this comment.
Pull request overview
This PR hardens crewai-tools against SSRF in ArxivPaperTool and closes a DNS-rebinding TOCTOU gap in the shared safe-request infrastructure by validating URLs and pinning DNS resolution across request and redirect hops.
Changes:
- Switched Arxiv API fetching to
safe_get()(HTTPS) and PDF downloads to newsafe_download()to ensure URL validation + redirect safety. - Added DNS pinning to
safe_get()usingvalidate_and_resolve()so the connection is bound to the validated IP (including across redirects). - Added/updated regression tests covering DNS rebinding pinning behavior and validated downloads.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/crewai-tools/src/crewai_tools/security/safe_path.py | Adds validate_and_resolve() (returns validated URL + validated IP) and keeps validate_url() behavior intact via wrapper. |
| lib/crewai-tools/src/crewai_tools/security/safe_requests.py | Implements DNS pinning in safe_get() and adds streaming safe_download() helper. |
| lib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.py | Migrates Arxiv fetch/download flows from urllib to the validated safe request helpers and upgrades API URL to HTTPS. |
| lib/crewai-tools/tests/utilities/test_safe_requests.py | Adds tests for DNS pinning, pin restoration, and safe_download() behavior. |
| lib/crewai-tools/tests/tools/arxiv_paper_tool_test.py | Updates tests to mock safe_get/safe_download and adds end-to-end SSRF regression tests for download_pdf. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| original_getaddrinfo = socket.getaddrinfo | ||
| family = socket.AF_INET6 if ":" in ip else socket.AF_INET | ||
|
|
||
| def pinned_getaddrinfo( | ||
| host: str, port: int, *args: Any, **kwargs: Any | ||
| ) -> list[tuple[Any, ...]]: | ||
| if host == hostname: | ||
| sockaddr = (ip, port, 0, 0) if family == socket.AF_INET6 else (ip, port) | ||
| return [(family, socket.SOCK_STREAM, socket.IPPROTO_TCP, "", sockaddr)] | ||
| return original_getaddrinfo(host, port, *args, **kwargs) | ||
|
|
||
| socket.getaddrinfo = pinned_getaddrinfo | ||
| try: | ||
| yield | ||
| finally: | ||
| socket.getaddrinfo = original_getaddrinfo |
| ValueError: If the URL (or any redirect target) fails validation. | ||
| requests.HTTPError: If the final response has an error status code. | ||
| """ | ||
| response = safe_get(url, max_redirects=max_redirects, stream=True, **kwargs) |
| def download_pdf(self, pdf_url: str, save_path: str) -> None: | ||
| try: | ||
| logger.info(f"Downloading PDF from {pdf_url} to {save_path}") |
…tream kwarg Both CodeRabbit and Copilot independently flagged the same real bug: _pin_dns patches socket.getaddrinfo process-wide with no synchronization, so two concurrent safe_get/safe_download calls could overwrite each other's pin and have one thread's `finally` restore over another's still-active pin -- silently dropping DNS-rebinding protection for whichever request loses the race. My original docstring's "unaffected" claim for concurrent requests was wrong. Fixed with a threading.Lock serializing the whole patch/use/restore window; added test_safe_get_dns_pin_is_thread_safe (two real threads, widened race window via a small sleep) to guard it. Also addressed: - safe_download now writes to a `.part` temp file and renames into place only on success, so a failed/interrupted download never leaves a truncated file at dest_path (CodeRabbit). New test: test_safe_download_leaves_no_partial_file_on_failure. - safe_download hard-coded stream=True but also forwarded **kwargs, so a caller-supplied stream= would raise "multiple values for keyword argument" (Copilot). Now explicitly overrides kwargs["stream"] instead of passing it positionally-and-by-kwarg. New test: test_safe_download_ignores_conflicting_stream_kwarg. - download_pdf's save_path was typed str but always called with a Path (previously needing a type: ignore); widened to str | Path and dropped the ignore (Copilot). - test_download_pdf_success now asserts the exact args passed to safe_download instead of just assert_called_once() (CodeRabbit). - The two end-to-end SSRF regression tests now clear CREWAI_TOOLS_ALLOW_UNSAFE_PATHS via monkeypatch so they stay hermetic regardless of the environment (CodeRabbit). 58 passed (was 55; +2 lock/atomic-write regression tests, +1 stream-kwarg test), ruff clean.
|
Thanks both — addressed everything in 83d1f53: The real one (flagged independently by both @coderabbitai and @copilot): Also fixed:
58 passed (was 55), |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- commit ---'
git show --stat --oneline 83d1f53
printf '%s\n' '--- safe_requests outline ---'
ast-grep outline lib/crewai-tools/src/crewai_tools/security/safe_requests.py --items all
printf '%s\n' '--- relevant implementation ---'
rg -n -A80 -B15 'def _pin_dns|def safe_download|threading\.Lock|os\.replace|\.part|kwargs\["stream"\]' \
lib/crewai-tools/src/crewai_tools/security/safe_requests.py
printf '%s\n' '--- Arxiv signature and call ---'
rg -n -A20 -B10 'def download_pdf|safe_download\s*\(' \
lib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.py
printf '%s\n' '--- regression tests ---'
rg -n -A45 -B10 'test_safe_get_dns_pin_is_thread_safe|\.part|stream=|test_download_pdf_success|ALLOW_UNSAFE_PATHS' \
lib/crewai-tools/testsLength of output: 71590
I also confirmed the atomic ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/crewai-tools/src/crewai_tools/security/safe_requests.py:145
- When
stream=True(as forced bysafe_download), redirect responses are left open while following the next hop. That can keep the underlying connection/resources for each 3xx response alive until GC, which is avoidable and can accumulate across redirects.
Consider explicitly closing each redirect response after adding it to history (the metadata remains readable), before issuing the next request.
if not _same_origin(current_url, redirect_url):
request_kwargs = _strip_cross_origin_credentials(request_kwargs)
history.append(response)
current_url, pinned_ip = redirect_url, redirect_ip
lib/crewai-tools/src/crewai_tools/security/safe_requests.py:175
safe_downloaduses a deterministic temp path (<dest>.part). If two threads/processes download to the same destination concurrently, they can clobber each other’s temp file and the cleanup/rename logic can produce surprising results.
Consider incorporating a per-thread (or otherwise unique) suffix into the temp filename to avoid collisions.
tmp_path = dest.with_name(f"{dest.name}.part")
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/crewai-tools/src/crewai_tools/security/safe_requests.py (1)
174-176: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse a per-call unique temp file name to avoid collisions.
tmp_pathis derived only fromdest.name. If twosafe_downloadcalls target the samedest_pathat the same time, both write to the same.partfile. Concurrent writes to the same temp file interleave and corrupt the output, which defeats the atomic-rename guarantee this function is designed to provide.Add a per-call unique component (thread id, pid, or a UUID) to
tmp_path.♻️ Proposed fix
dest = Path(dest_path) - tmp_path = dest.with_name(f"{dest.name}.part") + tmp_path = dest.with_name(f"{dest.name}.{threading.get_ident()}.part") kwargs["stream"] = True🤖 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/src/crewai_tools/security/safe_requests.py` around lines 174 - 176, Update the temporary path construction in safe_download to include a per-call unique component, such as a UUID, process/thread identifier, or equivalent, alongside dest.name. Preserve the existing temporary-file and atomic-rename flow while ensuring concurrent calls targeting the same destination never share tmp_path.lib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.py (1)
79-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd docstrings to
fetch_arxiv_dataanddownload_pdf.Both are public methods whose contracts changed in this diff:
fetch_arxiv_datanow raises throughsafe_get/ValueErrorinstead ofurlliberrors, anddownload_pdfnow acceptsstr | Pathand raisesrequests.RequestException,ValueError, orOSErrorinstead of usingurlretrieve. Document the parameters, return value, and raised exceptions for both.Based on coding guidelines: "Document public APIs and complex logic in Python code."
Also applies to: 159-176
🤖 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/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.py` around lines 79 - 121, ||||Add docstrings to the public methods fetch_arxiv_data and download_pdf, documenting their parameters, return values, and the exceptions they may raise. Reflect fetch_arxiv_data’s safe_get/ValueError behavior and download_pdf’s str | Path input plus requests.RequestException, ValueError, and OSError behavior; leave implementation logic unchanged.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/src/crewai_tools/security/safe_requests.py`:
- Around line 174-176: Update the temporary path construction in safe_download
to include a per-call unique component, such as a UUID, process/thread
identifier, or equivalent, alongside dest.name. Preserve the existing
temporary-file and atomic-rename flow while ensuring concurrent calls targeting
the same destination never share tmp_path.
In
`@lib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.py`:
- Around line 79-121: ||||Add docstrings to the public methods fetch_arxiv_data
and download_pdf, documenting their parameters, return values, and the
exceptions they may raise. Reflect fetch_arxiv_data’s safe_get/ValueError
behavior and download_pdf’s str | Path input plus requests.RequestException,
ValueError, and OSError behavior; leave implementation logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f80f2afd-8ea8-4323-872f-8527519bc3c2
📒 Files selected for processing (4)
lib/crewai-tools/src/crewai_tools/security/safe_requests.pylib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.pylib/crewai-tools/tests/tools/arxiv_paper_tool_test.pylib/crewai-tools/tests/utilities/test_safe_requests.py
… eagerly CodeRabbit caught a real bug in my own previous fix: safe_download's temp file name was derived only from dest_path (dest.name + ".part"), so two concurrent safe_download calls targeting the same destination would race on writing to the *same* temp file, interleaving/corrupting both -- exactly the kind of bug the atomic-rename change was supposed to prevent, just moved to a different spot. Fixed by including a uuid4 in the temp filename, so every call gets its own temp file regardless of thread, process, or how many callers target the same dest_path concurrently. New test: test_safe_download_concurrent_calls_to_same_dest_dont_corrupt (two real threads, asserts the final file is always one complete, uncorrupted payload, never a mix of both -- last-writer-wins on the final rename is fine, byte corruption is not). Also (Copilot, resource hygiene): safe_get now closes each redirect response right after appending it to `history`, instead of leaving it open until GC. Redirect bodies are never read, so this is safe -- status_code/headers/url stay accessible on history entries, only body access would be affected and nothing reads redirect bodies. Also (CodeRabbit, docs nitpick): added docstrings to fetch_arxiv_data and download_pdf documenting the parameters/returns/raises that changed when they moved off urllib onto safe_get/safe_download. 59 passed (was 58; +1 new regression test), ruff clean.
|
Thanks for the follow-up round — addressed in c7dc446: Real bug, caught by CodeRabbit: my own previous fix for the truncated-download issue introduced a new one — Copilot's resource-hygiene suggestion: Docs nitpick: added docstrings to 59 passed (was 58), |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/crewai-tools/src/crewai_tools/security/safe_path.py:248
validate_and_resolve()returns the first address fromsocket.getaddrinfo(). On dual-stack hosts, that list may start with an IPv6 address; pinningsafe_get()to that IP can make requests fail in IPv4-only environments (previouslyrequestscould fall back to IPv4). Consider preferring IPv4 addresses when choosing the pinned IP (while still validating all resolved addresses).
first_ip: str | None = None
for _family, _, _, _, sockaddr in addrinfos:
ip_str = str(sockaddr[0])
lib/crewai-tools/src/crewai_tools/security/safe_requests.py:90
_pin_dns()replacessocket.getaddrinfowithpinned_getaddrinfo, butpinned_getaddrinfoassumesportis always anintand buildssockaddrusing it directly.socket.getaddrinfoalso acceptsstr | Noneports (service names / unspecified), so any such lookup for the pinned hostname during the pin window could break (and this narrower signature can also trip mypy under strict typing).
def pinned_getaddrinfo(
host: str, port: int, *args: Any, **kwargs: Any
) -> list[tuple[Any, ...]]:
if host == hostname:
sockaddr = (ip, port, 0, 0) if family == socket.AF_INET6 else (ip, port)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai-tools/src/crewai_tools/security/safe_requests.py (1)
110-119: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDefine the proxy policy for SSRF protection.
safe_getusesrequests.get(...)without settingtrust_env=Falseor an explicitproxiesvalue. Requests 2.33.0 creates a temporary session withtrust_env=True, soHTTP_PROXY,HTTPS_PROXY,ALL_PROXY, andNO_PROXYcan still affect the hop chosen by the HTTP client. Set a documented proxy policy before DNS-pinned egress: disable environment proxies for this helper, or validate configured proxies as trusted egress.🤖 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/src/crewai_tools/security/safe_requests.py` around lines 110 - 119, Update safe_get’s request setup to enforce a documented proxy policy before DNS-pinned egress: disable environment-derived proxies by using a request configuration that sets trust_env=False, or explicitly validate and apply only trusted proxies. Ensure every requests.get call in the redirect loop uses that policy so HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, and NO_PROXY cannot alter the selected hop.
🤖 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 `@lib/crewai-tools/tests/utilities/test_safe_requests.py`:
- Around line 417-424: Update the worker-thread assertions in the concurrent
safe-request test to verify every thread has completed after the join calls.
Assert that no thread in threads remains alive before validating errors, dest
contents, or temporary files, while preserving the existing timeout and
subsequent assertions.
---
Outside diff comments:
In `@lib/crewai-tools/src/crewai_tools/security/safe_requests.py`:
- Around line 110-119: Update safe_get’s request setup to enforce a documented
proxy policy before DNS-pinned egress: disable environment-derived proxies by
using a request configuration that sets trust_env=False, or explicitly validate
and apply only trusted proxies. Ensure every requests.get call in the redirect
loop uses that policy so HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, and NO_PROXY cannot
alter the selected hop.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 536113ec-f0d1-4fd2-a00e-212a49b0039e
📒 Files selected for processing (3)
lib/crewai-tools/src/crewai_tools/security/safe_requests.pylib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.pylib/crewai-tools/tests/utilities/test_safe_requests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.py
| for thread in threads: | ||
| thread.start() | ||
| for thread in threads: | ||
| thread.join(timeout=10) | ||
|
|
||
| assert not errors | ||
| assert dest.read_bytes() in payloads.values() | ||
| assert list(tmp_path.glob("*.part")) == [] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assert that both worker threads complete.
thread.join(timeout=10) returns when the timeout expires. The test can pass if one call creates dest and the other call is still blocked. Assert that no thread is alive before checking the output.
Proposed fix
for thread in threads:
thread.join(timeout=10)
+ assert all(not thread.is_alive() for thread in threads)
assert not errors📝 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.
| for thread in threads: | |
| thread.start() | |
| for thread in threads: | |
| thread.join(timeout=10) | |
| assert not errors | |
| assert dest.read_bytes() in payloads.values() | |
| assert list(tmp_path.glob("*.part")) == [] | |
| for thread in threads: | |
| thread.start() | |
| for thread in threads: | |
| thread.join(timeout=10) | |
| assert all(not thread.is_alive() for thread in threads) | |
| assert not errors | |
| assert dest.read_bytes() in payloads.values() | |
| assert list(tmp_path.glob("*.part")) == [] |
🤖 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_requests.py` around lines 417 -
424, Update the worker-thread assertions in the concurrent safe-request test to
verify every thread has completed after the join calls. Assert that no thread in
threads remains alive before validating errors, dest contents, or temporary
files, while preserving the existing timeout and subsequent assertions.
Source: Coding guidelines
…lver Two more real (if narrower) findings from Copilot's re-review: 1. validate_and_resolve picked whichever address getaddrinfo returned first as the pin target. On a dual-stack host that's frequently IPv6, and pinning to a single address forgoes the normal multi-address connection fallback an unpinned request would get -- many real environments (CI runners, containers, some networks) have working IPv4 but broken or absent IPv6 routes, so a bare "first address" pin could make otherwise-working requests fail. Now prefers an IPv4 address among the validated results, falling back to IPv6 only if that's all that's offered. Every returned address is still validated regardless of which one ends up pinned -- a private IPv6 address in the results still blocks the request even if an IPv4 address would have been preferred. 2. _pin_dns's pinned_getaddrinfo typed `port: int` and used it directly to build a sockaddr, but socket.getaddrinfo's real signature allows a service name string or None for port too. A lookup for the pinned hostname with a non-int port during the (now-locked, so brief) pin window would have built a malformed sockaddr. Widened the type to `int | str | None` and added an isinstance check: matching-host lookups with a non-int port fall through to the real resolver instead. New tests: TestValidateAndResolve (dual-stack prefers IPv4, IPv6-only still works, a private address anywhere in the results still blocks regardless of preference) and test_pinned_resolver_falls_through_for_non_int_port. 63 passed (was 59), ruff clean.
|
Addressed both in 895773d — thanks for surfacing these even though they were marked suppressed: IPv4 preference: Port typing in the pinned resolver: New tests: 63 passed (was 59), |
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 `@lib/crewai-tools/tests/utilities/test_safe_requests.py`:
- Around line 257-271: Update the test around safe_get to wrap the
fixture-provided socket.getaddrinfo resolver with a call-tracking function
before invoking safe_get. Have the wrapper record its host and service
arguments, delegate to the original resolver, and assert the recorded call
contains ("public.example", "http"); remove the fake_get-local tracking that
cannot verify delegation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 26d96823-632d-4170-82ce-8ef7c5cea71b
📒 Files selected for processing (4)
lib/crewai-tools/src/crewai_tools/security/safe_path.pylib/crewai-tools/src/crewai_tools/security/safe_requests.pylib/crewai-tools/tests/utilities/test_safe_path.pylib/crewai-tools/tests/utilities/test_safe_requests.py
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/crewai-tools/src/crewai_tools/security/safe_path.py
- lib/crewai-tools/src/crewai_tools/security/safe_requests.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
lib/crewai-tools/src/crewai_tools/security/safe_requests.py:97
_pin_dnsreplacessocket.getaddrinfowithpinned_getaddrinfo, but the replacement callable currently only acceptshost: str. The realsocket.getaddrinfoAPI allowshostto bestr | bytes | None, so any concurrent lookup usingbytes/Noneduring the pin window would raise aTypeErrorand break unrelated networking code in the process. Make the shim match the real signature and fall through to the original resolver for non-strhosts.
def pinned_getaddrinfo(
host: str, port: int | str | None, *args: Any, **kwargs: Any
) -> list[tuple[Any, ...]]:
lib/crewai-tools/src/crewai_tools/tools/arxiv_paper_tool/arxiv_paper_tool.py:30
ArxivPaperToolnow depends onrequests(directly and viasafe_get/safe_download), but itspackage_dependencieslist still only includespydantic. This list is used when generating tool specs (and potentially for dependency installation), so it should includerequeststo avoid runtime import failures in minimal tool environments.
class ArxivPaperTool(BaseTool):
BASE_API_URL: ClassVar[str] = "https://export.arxiv.org/api/query"
SLEEP_DURATION: ClassVar[int] = 1
lib/crewai-tools/src/crewai_tools/security/safe_requests.py:187
safe_downloadcan also raiserequests.RequestExceptionfrom the underlyingsafe_get/requests.getcall (connection errors, timeouts, etc.) andOSErrorfrom filesystem writes/replaces, but the docstring currently only mentionsValueErrorandrequests.HTTPError. Updating the docstring makes the API contract clearer for callers.
Raises:
ValueError: If the URL (or any redirect target) fails validation.
requests.HTTPError: If the final response has an error status code.
"""
…gation CodeRabbit caught a real test-quality gap: the test asserted on the *returned IP* from the non-int-port lookup, but the public_dns fixture resolves "public.example" to the same IP for both the pinned request and the fallback delegation path -- so a broken pinned_getaddrinfo that incorrectly intercepted a non-int port (instead of falling through) would have returned that same IP and passed the test anyway. Fixed by wrapping the fixture-installed resolver with a call-tracking function *before* invoking safe_get, and asserting the non-int-port lookup actually reached it -- proving delegation happened, not just that the return value happened to match. 63 passed, ruff clean (multi-path invocation confirmed; a single-file `ruff check` run hit a "no files found under the given path(s)" warning that turned out to be specific to that invocation shape under `--package` scoping, not a real issue -- the multi-path check that's worked reliably all session found and checked the file correctly).
|
Good catch, fixed in a7fdf3c: the test asserted on the returned IP from the non-int-port lookup, but the `public_dns` fixture resolves `public.example` to the same address for both the pinned request and the fallback-delegation path — so a broken `pinned_getaddrinfo` that incorrectly intercepted a non-int port would have returned that same IP and passed anyway. It never actually proved delegation happened. Fixed by wrapping the fixture-installed resolver with a call-tracking function before invoking `safe_get`, and asserting the non-int-port lookup actually reached it directly — exactly your suggested fix. 63 passed, `ruff check` clean. |
Fixes #6694.
What's actually vulnerable
The issue names both
urllibcalls inArxivPaperTool, but only one is a real SSRF vector:fetch_arxiv_data()'surlopencall targets a hardcodedBASE_API_URL(export.arxiv.org);search_queryonly ever lands in the query string, percent-encoded. Not an SSRF vector for destination redirection. It was plainhttp://, though — a real but separate weakness (a network MITM can tamper with the API response). Switched tohttps://and migrated tosafe_get()for consistency and redirect-safety while touching this.download_pdf()'surlretrievecall is the real vector:pdf_urlcomes from parsing the arxiv API's XML response (anhrefattribute), not directly fromsearch_query. Whatever URL shows up there was fetched and written to disk with zero validation — reachable via the same plain-HTTP MITM angle, or a malicious link ever indexed upstream. Migrated tosafe_download()(new helper, see below).Additional finding: DNS-rebinding TOCTOU in the existing
safe_requestsinfracrewai_tools.securityalready has real SSRF protection (validate_url,safe_get) that both call sites above should have been using from the start. While wiring the arxiv tool into it, I found a real gap in the shared infrastructure itself:validate_url()resolves DNS, checks the IP, and returns the original URL string — it doesn't pin the actual connection to the address it just validated.requests.get()then re-resolves DNS itself at connection time. An attacker controlling DNS (or using a short-TTL record) can present a safe IP for validation and a private one for the real connection moments later.Fixed by:
safe_path.validate_and_resolve(url) -> (url, ip): sharesvalidate_url's exact logic but also returns the checked IP.validate_url/newresolve_validated_ipare now thin wrappers around it (validate_url's public signature/behavior is unchanged).safe_requests._pin_dns(hostname, ip): a context manager that pinssocket.getaddrinfo(hostname, ...)to the validated IP for the duration of one request.safe_get()now validates+pins on the initial URL and on every redirect hop, closing the window for both direct requests and redirect-based rebinding.safe_requests.safe_download(url, dest_path): new helper for streaming a validated, pinned download to disk —safe_getdidn't have an equivalent forarxiv_paper_tool's use case.Testing
test_safe_get_pins_dns_against_rebinding: proves the fix directly — mocksgetaddrinfoto return a safe IP on the first lookup and a private one on any subsequent lookup (simulating rebinding), and asserts the connection never sees the private IP. Fails against the pre-fix code.test_safe_get_restores_real_resolver_after_pinning: pinning doesn't leak past the request it was applied to.safe_download: writes-to-disk, blocks private IP, blocks redirect to private IP.ArxivPaperTool.download_pdf: blocks127.0.0.1and the AWS/GCP/Azure metadata endpoint end-to-end (not mocked at thesafe_downloadlevel), proving the tool itself is protected, not just the helper in isolation.arxiv_paper_tool_test.pytests updated to mocksafe_get/safe_downloadinstead ofurllib(the code path changed; test intent didn't).uv run --package crewai-tools pytest lib/crewai-tools/tests/utilities/test_safe_path.py lib/crewai-tools/tests/utilities/test_safe_requests.py lib/crewai-tools/tests/tools/arxiv_paper_tool_test.py— 55 passed.ruff checkclean on all changed files.