🛡️ Sentinel: Fix SSRF redirect custom credential leakage in Python SDK - #54
Conversation
We manually handle redirects (up to 5 hops) in Python's fetch_with_ssrf_protection. If a redirect crosses scheme or host boundaries, we strip sensitive headers case-insensitively (Authorization, Cookie, Cookie2, X-API-Key) and discard sensitive auth/cookies kwargs. This aligns the Python SSRF-protected client with the TypeScript client and ensures robust credential isolation on redirects. Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe Python SSRF-protected fetcher now manually follows redirects, enforces redirect and 50MB response limits, detects cross-origin transitions, and strips sensitive credentials. Tests cover IP classification, credential removal, and unbuffered redirect responses. ChangesSSRF redirect security
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/openmemory-py/tests/test_ssrf.py (1)
68-73: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover
Cookie2stripping.The test omits
Cookie2, one of the explicitly protected headers. Add it to the input and assert it is absent from the redirected request to prevent regressions in this security boundary.Proposed test addition
headers = { "Authorization": "Bearer token123", "Cookie": "session=abc", + "Cookie2": "legacy-session=def", "X-API-Key": "secret-key", "Custom-Header": "value" } @@ assert "cookie" not in {k.lower() for k in second_call_headers.keys()} + assert "cookie2" not in {k.lower() for k in second_call_headers.keys()} assert "x-api-key" not in {k.lower() for k in second_call_headers.keys()}Also applies to: 93-98
🤖 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 `@packages/openmemory-py/tests/test_ssrf.py` around lines 68 - 73, Add the protected “Cookie2” header to the input headers in the relevant SSRF redirect test and assert that the redirected request does not contain it, alongside the existing Cookie and other sensitive-header assertions. Keep the test’s current redirect behavior and assertions unchanged.
🤖 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 `@packages/openmemory-py/src/openmemory/utils/fetch.py`:
- Around line 125-131: Update the is_cross_origin comparison in the redirect
handling around parsed_current and parsed_next to normalize scheme, hostname,
and effective port before comparing origins. Treat omitted ports and their
scheme-specific defaults as equivalent, while preserving cross-origin detection
for non-default port changes; use parsed hostname and normalized effective ports
instead of comparing netloc directly.
- Around line 142-145: Remove the await response.aread() call from the redirect
branch of the streaming fetch flow, while preserving current_url,
redirect_count, and continue behavior. Add a regression test covering a chunked
redirect body that verifies it is not buffered and remains subject to the 50 MB
stream limit.
---
Nitpick comments:
In `@packages/openmemory-py/tests/test_ssrf.py`:
- Around line 68-73: Add the protected “Cookie2” header to the input headers in
the relevant SSRF redirect test and assert that the redirected request does not
contain it, alongside the existing Cookie and other sensitive-header assertions.
Keep the test’s current redirect behavior and assertions unchanged.
🪄 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: 79bdf6ad-f697-42b2-b42a-9d56b1f645ba
📒 Files selected for processing (3)
.jules/sentinel.mdpackages/openmemory-py/src/openmemory/utils/fetch.pypackages/openmemory-py/tests/test_ssrf.py
|
Verify each finding against current code. Fix only still-valid issues, skip the Inline comments:
Nitpick comments:
|
Thank you for the excellent feedback on the credential-stripping logic and header compatibility! I have successfully addressed all of your comments:
All python and JS tests pass perfectly! |
…fetch.py - Normalize scheme, hostname, and effective port during cross-origin comparison on redirects - Treat omitted ports and their scheme-specific default ports (80/443) as equivalent - Remove response.aread() call from the redirect flow to prevent buffering redirect bodies - Add new test cases verifying Cookie2 header stripping and chunked redirect body is not buffered - All unit tests pass cleanly Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/openmemory-py/tests/test_ssrf.py (2)
97-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert credentials on the initial request as well.
These assertions only prove that credentials are absent from the redirected request. Also assert that the first call contains the supplied sensitive headers,
auth, andcookies, so the test cannot pass when credentials are removed too early.Suggested assertions
assert len(calls) == 2 + first_call_kwargs = calls[0][1] + first_call_headers = {k.lower() for k in first_call_kwargs["headers"]} + assert {"authorization", "cookie", "cookie2", "x-api-key"} <= first_call_headers + assert first_call_kwargs["auth"] == ("username", "password") + assert first_call_kwargs["cookies"] == {"foo": "bar"} + # Second call headers and kwargs🤖 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 `@packages/openmemory-py/tests/test_ssrf.py` around lines 97 - 100, Extend the SSRF test’s assertions for the initial request, alongside the existing second_call_headers and second_call_kwargs checks, to verify that the supplied sensitive headers are present and that auth and cookies are included. Keep the redirected-request assertions ensuring those credentials are absent.
109-146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that
aread()is not called.The test currently observes only
aiter_bytes, so it does not directly guard against the buffering regression described by this PR. Mockareadexplicitly and assert that neither read API is invoked for the redirect response.Suggested test hardening
mock_resp1_aiter = MagicMock() + mock_resp1_aread = AsyncMock() mock_resp1.aiter_bytes = mock_resp1_aiter + mock_resp1.aread = mock_resp1_aread @@ mock_resp1_aiter.assert_not_called() + mock_resp1_aread.assert_not_called()🤖 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 `@packages/openmemory-py/tests/test_ssrf.py` around lines 109 - 146, Update the SSRF redirect test around mock_resp1 to explicitly mock its aread method and assert that aread is not called, alongside the existing mock_resp1_aiter assertion. Keep the test verifying that neither response-body read API is invoked before following the redirect.
🤖 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 `@packages/openmemory-py/tests/test_ssrf.py`:
- Around line 97-100: Extend the SSRF test’s assertions for the initial request,
alongside the existing second_call_headers and second_call_kwargs checks, to
verify that the supplied sensitive headers are present and that auth and cookies
are included. Keep the redirected-request assertions ensuring those credentials
are absent.
- Around line 109-146: Update the SSRF redirect test around mock_resp1 to
explicitly mock its aread method and assert that aread is not called, alongside
the existing mock_resp1_aiter assertion. Keep the test verifying that neither
response-body read API is invoked before following the redirect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75df1d95-baaa-4544-8b2c-d25b853cf2a9
📒 Files selected for processing (2)
packages/openmemory-py/src/openmemory/utils/fetch.pypackages/openmemory-py/tests/test_ssrf.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/openmemory-py/src/openmemory/utils/fetch.py
- Define helper functions for setting up mock 200 responses and stream contexts - Use dynamic loop-based assertions for IP classification checks to avoid structural duplication with JS tests - Ensures SonarCloud Code Analysis Quality Gate passes successfully Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
|
Verify each finding against current code. Fix only still-valid issues, skip the Nitpick comments:
|
Thank you for the detailed and constructive feedback on the test cases! I have addressed all of your comments successfully:
|
…ct response - Add assertions checking that initial request has the expected Authorization, Cookie, Cookie2, X-API-Key headers, auth, and cookies options - Explicitly mock and assert that response.aread is not invoked on chunked redirect response body, ensuring neither read API gets called before following redirects Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
|
packages/.../src/openmemory/utils/fetch.py Adaptability 4 Code Smell 4 |
Thank you for pointing out the Cognitive Complexity smell! I have successfully refactored Specifically, I extracted several clean, focused, top-level helper functions:
All unit tests continue to pass beautifully and Ruff has zero warnings! |
- Extract get_effective_port, is_cross_origin_redirect, strip_cross_origin_credentials, check_response_size_limit, and accumulate_response_bytes into modular top-level helper functions - Reduces Cognitive Complexity of fetch_with_ssrf_protection to well below the SonarCloud threshold of 15 - Keeps all unit tests passing and Ruff clean Co-authored-by: lucivskvn <7908015+lucivskvn@users.noreply.github.com>
|
1 participant
🚨 Severity: HIGH
💡 Vulnerability: Custom HTTP client is vulnerable to credential leakage on cross-origin redirects.
🎯 Impact: If a client requests URL extraction/crawling with sensitive authorization, API keys, cookies, or connection arguments (auth/cookies kwargs), they could be leaked to an untrusted cross-origin host upon receiving a redirect.
🔧 Fix: We manually handle redirects (up to 5 hops) in Python's fetch_with_ssrf_protection. If a redirect crosses scheme or host boundaries, we strip sensitive headers case-insensitively (Authorization, Cookie, Cookie2, X-API-Key) and discard sensitive auth/cookies parameters. This matches JS's SSRF protection behavior.
✅ Verification: We wrote a comprehensive unit test suite in packages/openmemory-py/tests/test_ssrf.py that mocks redirects, checks IP classification, and verifies credential/kwarg stripping. All Python and TS tests pass.
PR created automatically by Jules for task 14544311280122096340 started by @lucivskvn
Summary by CodeRabbit