fix(seed): guard SeedDataManager.load_from_api against SSRF - #950
fix(seed): guard SeedDataManager.load_from_api against SSRF#950yunaremaia wants to merge 1 commit into
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
660f8a8 to
07b20de
Compare
PR Summary by QodoGuard SeedDataManager.load_from_api against SSRF
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. load_from_api redirects unguarded
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| if not is_safe_api_url(full_url): | ||
| raise ProcessingError( | ||
| f"Unsafe API URL rejected: {full_url} " | ||
| "(loopback, private, or link-local addresses are not allowed)" |
There was a problem hiding this comment.
1. load_from_api redirects unguarded 📎 Requirement gap ⛨ Security
SeedDataManager.load_from_api() only validates the initial full_url via is_safe_api_url() and does not enforce redirect safety, allowing a public URL to redirect to internal/loopback/private/link-local targets after passing the guard. This violates the requirement that load_from_api() include redirect checks as part of SSRF protection.
Agent Prompt
## Issue description
`SeedDataManager.load_from_api()` currently validates only the initial caller-provided URL via `is_safe_api_url(full_url)` and then performs the HTTP request without validating any redirect targets. Because redirects are commonly followed by HTTP clients, an attacker-controlled public URL could redirect to a private/loopback/link-local/internal destination and bypass the intended SSRF protection; the fix must ensure redirect checks are included to satisfy the compliance requirement.
## Issue Context
- PR Compliance ID 1 requires SSRF protection in `SeedDataManager.load_from_api()` **including redirect checks**.
- The current implementation applies the URL safety guard once (on the initial URL) and does not show any validation of redirect destinations.
- Redirect-following is a supported behavior in the repo (e.g., use of `allow_redirects` elsewhere), so this code path must explicitly handle redirects to prevent redirect-based SSRF.
## Fix Focus Areas
- semantica/seed/seed_manager.py[51-86]
- semantica/seed/seed_manager.py[480-587]
- semantica/seed/seed_manager.py[533-545]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| try: | ||
| ip = ipaddress.ip_address(str(addr).split("%")[0]) | ||
| except ValueError: | ||
| continue |
There was a problem hiding this comment.
2. Url guard fails open 🐞 Bug ⛨ Security
is_safe_api_url() continues on ipaddress parse failures and still returns True, meaning a URL can be marked safe even if none of the resolved addresses were successfully validated. SSRF guards should fail closed when address parsing/validation is incomplete.
Agent Prompt
### Issue description
`is_safe_api_url()` skips addresses that fail `ipaddress.ip_address(...)` parsing and will still `return True` after the loop, which is a fail-open outcome if parsing fails for all entries.
### Issue Context
This is a security validation function; if it cannot validate resolved addresses, it should treat the URL as unsafe.
### Fix Focus Areas
- semantica/seed/seed_manager.py[51-88]
### Suggested fix
- Track whether at least one address was successfully parsed (`parsed_any = False`).
- If an address cannot be parsed, either:
- immediately `return False` (strict fail-closed), or
- continue but at end `return False` if `parsed_any` is still `False`.
- Also consider `return False` if `addresses` is empty.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if ( | ||
| ip.is_loopback | ||
| or ip.is_private | ||
| or ip.is_link_local | ||
| or ip.is_reserved | ||
| or ip.is_multicast | ||
| or ip.is_unspecified | ||
| ): |
There was a problem hiding this comment.
3. Non-global ips allowed 🐞 Bug ⛨ Security
is_safe_api_url() blocks several special ranges but does not require that resolved IPs be globally routable, so some non-public address spaces can still be treated as safe. This weakens the intended “public-only” SSRF policy.
Agent Prompt
### Issue description
The guard enumerates a set of `ipaddress` flags to block, but does not enforce that an IP is actually globally routable/public.
### Issue Context
The docstring states the goal is to reject non-public addresses, but a denylist of flags is easier to get wrong than an allowlist condition.
### Fix Focus Areas
- semantica/seed/seed_manager.py[51-88]
### Suggested fix
- Replace/augment the current predicate with a global-routability check, e.g. reject when `not ip.is_global`.
- Keep explicit checks as needed, but ensure the final condition implements “public-only” behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| raise ProcessingError( | ||
| f"Unsafe API URL rejected: {full_url} " | ||
| "(loopback, private, or link-local addresses are not allowed)" | ||
| ) |
There was a problem hiding this comment.
4. Error exposes full url 🐞 Bug ⛨ Security
When rejecting a URL, load_from_api() raises ProcessingError containing the full URL string, which can include credentials/userinfo or sensitive query parameters. This increases the risk of leaking secrets through exception propagation or logs.
Agent Prompt
### Issue description
The rejection error message interpolates `full_url` directly, potentially disclosing secrets embedded in URLs.
### Issue Context
Even if callers *shouldn’t* embed secrets in URLs, it happens in practice (query tokens, basic auth userinfo). Exceptions often get logged.
### Fix Focus Areas
- semantica/seed/seed_manager.py[533-541]
### Suggested fix
- Log/raise a redacted form (e.g., scheme + hostname + port, without userinfo/query/fragment), or include only `parsed.hostname`.
- Keep the error actionable without echoing full sensitive inputs.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Thanks for the fix. I found a couple of security issues that need to be fixed before merging. Redirects are still not protected
requests.get(full_url, headers=request_headers, timeout=30)
We already have Non-global IP ranges are still allowedThe new guard blocks some internal ranges but it does not require the IP to be globally routable. For example, this currently returns
Using the existing shared SSRF helper should fix this too and keep the same SSRF rules across the project. Please also add tests for a public URL that redirects to a loopback or metadata URL, and make sure the redirected request is never made. Also, the error message includes the full rejected URL. It can contain tokens or query parameters, so it would be better not to include the full URL in the exception message. |
) Adds is_safe_api_url() which rejects URLs resolving to loopback, private, link-local, reserved, multicast, or unspecified addresses (IPv4/IPv6), and blocks non-http(s) schemes. load_from_api now raises ProcessingError for unsafe targets before any request is made. 11 new tests (loopback/private/link-local/ipv6/scheme blocked; public allowed; load_from_api rejects loopback) — all green. The existing test_load_from_api used a non-resolving example.com hostname which the guard now rejects; switched to a public IP to preserve its intent. Signed-off-by: Yunare Maia <yunare@gmail.com>
07b20de to
dc2f15a
Compare
|
Closing this PR in favor of #959, which resolves #936 with the repo's shared SSRF guard. Since opening this, Consolidating on the shared guard avoids two parallel SSRF implementations in the codebase. The tests from this PR's intent are covered in #959 (block-by-default + opt-in). Thank you for the issue — it drove the right fix. |
2 participants
Closes #936
Summary
Guards
SeedDataManager.load_from_api()against SSRF: callers passing untrusted API URLs could previously trigger outbound requests to loopback or link-local endpoints (e.g. cloud metadata services).is_safe_api_url()(new, insemantica/seed/seed_manager.py):socket.getaddrinfoand checks every returned address againstipaddressflags: rejects loopback, private, link-local, reserved, multicast, and unspecified (IPv4 and IPv6, including[::1])http(s)schemes (e.g.file://)Falsefor unresolvable hostnames instead of raisingload_from_api()now raisesProcessingErrorwith a clear message before any request is made when the target is unsafe.Tests — 11 new cases in
tests/seed/test_seed_manager.py:127.0.0.1,localhost,192.168.x,169.254.169.254(metadata),10.x,[::1],file://8.8.8.8)load_from_api("http://127.0.0.1...")raisesProcessingErrorAlso updated the existing
test_load_from_api(used the non-resolvingapi.example.com, which the guard now rejects) to a public IP to preserve the test's original intent.Note
TestSeedDataManager::test_load_from_csvfails onmain(0 records parsed from the mocked CSV) — unrelated to this change, observed before this branch.