🛡️ Sentinel: [HIGH] Enforce safeFetch for downloader API connections to prevent SSRF - #889
Open
Doezer wants to merge 1 commit into
Open
🛡️ Sentinel: [HIGH] Enforce safeFetch for downloader API connections to prevent SSRF#889Doezer wants to merge 1 commit into
Doezer wants to merge 1 commit into
Conversation
Co-authored-by: Doezer <11655673+Doezer@users.noreply.github.com>
Contributor
|
👋 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughDownloader API requests now use ChangesSSRF-safe downloader routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.jules/sentinel.md:
- Line 31: Insert one blank line immediately after the “2025-05-24 - SSRF
Prevention in Internal Downstream Requests” heading in the changelog section,
before its following paragraph.
In `@server/__tests__/downloaders_deluge.test.ts`:
- Line 22: Centralize the shared SSRF test mocks in tests/setup.ts: move the
isSafeUrl and safeFetch mock definitions there, then remove them from
server/__tests__/downloaders_deluge.test.ts#L22-L22,
server/__tests__/downloaders_deluge_coverage.test.ts#L22-L22,
server/__tests__/downloaders_deluge_remaining.test.ts#L21-L24,
server/__tests__/downloaders_rtorrent_remaining.test.ts#L25-L25,
server/__tests__/downloaders_transmission_remaining.test.ts#L24-L24, and
server/__tests__/routes.test.ts#L4-L7. Retain only test-specific local overrides
in each file.
- Line 22: Pass-through safeFetch mocks bypass wrapper protections, leaving
validation, redirect revalidation, DNS-rebinding, timeout, and
private/unsafe-redirect behavior untested. In
server/__tests__/downloaders_deluge.test.ts:22,
server/__tests__/downloaders_deluge_coverage.test.ts:22,
server/__tests__/downloaders_deluge_remaining.test.ts:21-24,
server/__tests__/downloaders_rtorrent_remaining.test.ts:25, and
server/__tests__/downloaders_transmission_remaining.test.ts:24, add coverage
using the real safeFetch wrapper for the respective downloader paths. In
server/__tests__/routes.test.ts:4-7, add route-level coverage for private and
unsafe-redirect rejection; retain pass-through mocks only where wrapper controls
are not under test.
In `@server/__tests__/downloaders_helpers_regression.test.ts`:
- Around line 31-35: Update the SSRF mocks and downloader assertions in
server/__tests__/downloaders_helpers_regression.test.ts (lines 31-35),
server/__tests__/downloaders_nzbget_remaining.test.ts (line 17), and
server/__tests__/downloaders_qbittorrent_remaining.test.ts (line 24): import and
spy on safeFetch, assert each downloader invokes it rather than only
global.fetch or fetchMock, and add separate coverage for private-URL and
redirect validation paths.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: d60fd4f4-b45f-4b80-84a7-d650b7d161c3
📒 Files selected for processing (15)
.jules/sentinel.mdserver/__tests__/downloaders_deluge.test.tsserver/__tests__/downloaders_deluge_coverage.test.tsserver/__tests__/downloaders_deluge_remaining.test.tsserver/__tests__/downloaders_helpers_regression.test.tsserver/__tests__/downloaders_nzbget_remaining.test.tsserver/__tests__/downloaders_qbittorrent_remaining.test.tsserver/__tests__/downloaders_rtorrent_remaining.test.tsserver/__tests__/downloaders_transmission_remaining.test.tsserver/__tests__/routes.test.tsserver/downloaders/deluge.tsserver/downloaders/nzbget.tsserver/downloaders/qbittorrent.tsserver/downloaders/rtorrent.tsserver/downloaders/transmission.ts
| **Learning:** Checking string prefixes for URL validation is fundamentally insecure because parts of the prefix might be interpreted as the username/password in a URL with a different domain. Attackers can leverage this to bypass domain allowlists. | ||
| **Prevention:** Always use the `URL` object (e.g., `new URL()`) to parse URLs and explicitly validate the `hostname` and `pathname` properties instead of checking raw string prefixes. | ||
|
|
||
| ## 2025-05-24 - SSRF Prevention in Internal Downstream Requests |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line after the heading.
Markdownlint reports MD022 because Line [31] is immediately followed by the paragraph on Line [32]. Insert one blank line.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 @.jules/sentinel.md at line 31, Insert one blank line immediately after the
“2025-05-24 - SSRF Prevention in Internal Downstream Requests” heading in the
changelog section, before its following paragraph.
Source: Linters/SAST tools
|
|
||
| vi.mock("../ssrf.js", () => ({ | ||
| isSafeUrl: vi.fn().mockResolvedValue(true), | ||
| safeFetch: vi.fn((url: string, options: RequestInit) => fetch(url, options)), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Centralize the repeated SSRF test setup. The same mock is defined in six files. Move it to tests/setup.ts, and retain only test-specific overrides locally.
server/__tests__/downloaders_deluge.test.ts#L22-L22: move thesafeFetchmock to shared setup.server/__tests__/downloaders_deluge_coverage.test.ts#L22-L22: move thesafeFetchmock to shared setup.server/__tests__/downloaders_deluge_remaining.test.ts#L21-L24: move theisSafeUrlandsafeFetchmocks to shared setup.server/__tests__/downloaders_rtorrent_remaining.test.ts#L25-L25: move thesafeFetchmock to shared setup.server/__tests__/downloaders_transmission_remaining.test.ts#L24-L24: move thesafeFetchmock to shared setup.server/__tests__/routes.test.ts#L4-L7: move theisSafeUrlandsafeFetchmocks to shared setup.
As per coding guidelines, shared test setup must use tests/setup.ts.
📍 Affects 6 files
server/__tests__/downloaders_deluge.test.ts#L22-L22(this comment)server/__tests__/downloaders_deluge_coverage.test.ts#L22-L22server/__tests__/downloaders_deluge_remaining.test.ts#L21-L24server/__tests__/downloaders_rtorrent_remaining.test.ts#L25-L25server/__tests__/downloaders_transmission_remaining.test.ts#L24-L24server/__tests__/routes.test.ts#L4-L7
🤖 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 `@server/__tests__/downloaders_deluge.test.ts` at line 22, Centralize the
shared SSRF test mocks in tests/setup.ts: move the isSafeUrl and safeFetch mock
definitions there, then remove them from
server/__tests__/downloaders_deluge.test.ts#L22-L22,
server/__tests__/downloaders_deluge_coverage.test.ts#L22-L22,
server/__tests__/downloaders_deluge_remaining.test.ts#L21-L24,
server/__tests__/downloaders_rtorrent_remaining.test.ts#L25-L25,
server/__tests__/downloaders_transmission_remaining.test.ts#L24-L24, and
server/__tests__/routes.test.ts#L4-L7. Retain only test-specific local overrides
in each file.
Source: Coding guidelines
🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rtk rg -n 'safeFetch|isSafeUrl|DNS rebinding|private|redirect|timeout' server --glob '*.test.ts'Repository: Doezer/Questarr
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -u
echo "== repository files around tests/implementation =="
git ls-files | rg '(^|/)server/(ssrf\.ts|downloaders/.*\.ts|__tests__/.*\.test\.ts|routes\.test\.ts)$' | sed -n '1,120p'
echo ""
echo "== search SSRF terms in tracked server tests without rtk =="
rg -n 'safeFetch|isSafeUrl|DNS rebinding|private|redirect|timeout' server --glob '*.test.ts' || true
echo ""
echo "== relevant test imports/mock snippets =="
for f in \
server/__tests__/downloaders_deluge.test.ts \
server/__tests__/downloaders_deluge_coverage.test.ts \
server/__tests__/downloaders_deluge_remaining.test.ts \
server/__tests__/downloaders_rtorrent_remaining.test.ts \
server/__tests__/downloaders_transmission_remaining.test.ts \
server/__tests__/routes.test.ts
do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,80p' "$f"
fi
done
echo ""
echo "== ssrf implementation outline and content =="
wc -l server/ssrf.ts
sed -n '1,260p' server/ssrf.tsRepository: Doezer/Questarr
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "== targeted test snippets =="
for f in \
server/__tests__/downloaders_deluge.test.ts \
server/__tests__/downloaders_deluge_coverage.test.ts \
server/__tests__/downloaders_deluge_remaining.test.ts \
server/__tests__/downloaders_rtorrent_remaining.test.ts \
server/__tests__/downloaders_transmission_remaining.test.ts \
server/__tests__/routes.test.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,120p' "$f"
fi
done
echo ""
echo "== targeted ssrf implementation =="
sed -n '1,240p' server/ssrf.ts
echo ""
echo "== targeted existing SSRF tests snippets =="
for f in server/__tests__/ssrf.test.ts server/__tests__/downloaders_ssrf.test.ts server/__tests__/rss-ssrf.test.ts server/__tests__/ssrf_routes.test.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
done
echo ""
echo "== safeFetch call sites in listed targeted files =="
python3 - <<'PY'
from pathlib import Path
paths = [
"server/__tests__/downloaders_deluge.test.ts",
"server/__tests__/downloaders_deluge_coverage.test.ts",
"server/__tests__/downloaders_deluge_remaining.test.ts",
"server/__tests__/downloaders_rtorrent_remaining.test.ts",
"server/__tests__/downloaders_transmission_remaining.test.ts",
"server/__tests__/routes.test.ts",
"server/ssrf.ts",
]
for p in paths:
text = Path(p).read_text()
print(f"--- {p} contains safeFetch/isSafeUrl/redirect/timeout/private/DNS occurrences ---")
for i, line in enumerate(text.splitlines(), 1):
lower = line.lower()
if any(x in lower for x in ["safefetch", "issafeurl", "redirect", "timeout", "private", "dns", "metadata", "rebind"]):
print(f"{i}: {line}")
PYRepository: Doezer/Questarr
Length of output: 50371
Other (CWE-693)
Reachability path
● Entry
server/__tests__/downloaders_deluge.test.ts
│
▼
● Sink
server/downloaders/deluge.ts
Cover safeFetch controls outside pass-through mock tests. The listed tests replace safeFetch with native fetch, so they no longer test URL validation, redirect revalidation, DNS-rebinding checks, or timeouts for those paths. Add coverage in tests that use the real wrapper for Deluge, rTorrent, Transmission, and route-level private/unsafe-redirect rejection.
📍 Affects 6 files
server/__tests__/downloaders_deluge.test.ts#L22-L22(this comment)server/__tests__/downloaders_deluge_coverage.test.ts#L22-L22server/__tests__/downloaders_deluge_remaining.test.ts#L21-L24server/__tests__/downloaders_rtorrent_remaining.test.ts#L25-L25server/__tests__/downloaders_transmission_remaining.test.ts#L24-L24server/__tests__/routes.test.ts#L4-L7
🤖 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 `@server/__tests__/downloaders_deluge.test.ts` at line 22, Pass-through
safeFetch mocks bypass wrapper protections, leaving validation, redirect
revalidation, DNS-rebinding, timeout, and private/unsafe-redirect behavior
untested. In server/__tests__/downloaders_deluge.test.ts:22,
server/__tests__/downloaders_deluge_coverage.test.ts:22,
server/__tests__/downloaders_deluge_remaining.test.ts:21-24,
server/__tests__/downloaders_rtorrent_remaining.test.ts:25, and
server/__tests__/downloaders_transmission_remaining.test.ts:24, add coverage
using the real safeFetch wrapper for the respective downloader paths. In
server/__tests__/routes.test.ts:4-7, add route-level coverage for private and
unsafe-redirect rejection; retain pass-through mocks only where wrapper controls
are not under test.
Comment on lines
+31
to
+35
| vi.mock("../ssrf.js", () => ({ | ||
| isSafeUrl: vi.fn().mockResolvedValue(true), | ||
| safeFetch: vi.fn((url: string, options?: RequestInit) => global.fetch(url, options)), | ||
| resolveSafeAddress: vi.fn().mockResolvedValue({ address: "127.0.0.1", family: 4 }), | ||
| })); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rtk rg -n -C 6 'safeFetch|global\.fetch|fetchMock|isSafeUrl' server/__tests__Repository: Doezer/Questarr
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo files matching test target:"
git ls-files 'server/__tests__/*' | sed -n '1,200p'
echo
echo "security/ssrf.ts:"
if [ -f server/ssrf.ts ]; then wc -l server/ssrf.ts; sed -n '1,240p' server/ssrf.ts; fi
echo
echo "mocks and safeFetch references in tests (without rtk):"
rg -n -C 4 'safeFetch|global\.fetch|fetchMock|isSafeUrl|resolveSafeAddress' server/__tests__ || trueRepository: Doezer/Questarr
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ssrf.ts:"
if [ -f server/ssrf.ts ]; then
wc -l server/ssrf.ts
sed -n '1,260p' server/ssrf.ts
fi
echo
echo "focused test files:"
for f in server/__tests__/downloaders_helpers_regression.test.ts server/__tests__/downloaders_nzbget_remaining.test.ts server/__tests__/downloaders_qbittorrent_remaining.test.ts; do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
echo
echo "focused safeFetch/imported test assertions:"
python3 - <<'PY'
from pathlib import Path
files = [
Path("server/__tests__/downloaders_helpers_regression.test.ts"),
Path("server/__tests__/downloaders_nzbget_remaining.test.ts"),
Path("server/__tests__/downloaders_qbittorrent_remaining.test.ts"),
]
for p in files:
if not p.exists():
continue
text = p.read_text()
print(f"{p}: safeFetch_imported={'.safeFetch' in text}; safeFetch_expected={text.count('safeFetch')}")
PYRepository: Doezer/Questarr
Length of output: 29698
Assert safeFetch usage in these test mocks.
The mock aliases let regressions to native fetch still pass. In these files, import safeFetch where used and verify the downloader path runs through it instead of relying only on global.fetch/fetchMock. Also cover private URL and redirect checks separately.
📍 Affects 3 files
server/__tests__/downloaders_helpers_regression.test.ts#L31-L35(this comment)server/__tests__/downloaders_nzbget_remaining.test.ts#L17-L17server/__tests__/downloaders_qbittorrent_remaining.test.ts#L24-L24
🤖 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 `@server/__tests__/downloaders_helpers_regression.test.ts` around lines 31 -
35, Update the SSRF mocks and downloader assertions in
server/__tests__/downloaders_helpers_regression.test.ts (lines 31-35),
server/__tests__/downloaders_nzbget_remaining.test.ts (line 17), and
server/__tests__/downloaders_qbittorrent_remaining.test.ts (line 24): import and
spy on safeFetch, assert each downloader invokes it rather than only
global.fetch or fetchMock, and add separate coverage for private-URL and
redirect validation paths.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚨 Severity: HIGH
💡 Vulnerability: Downloader API communication (e.g. qBittorrent, Transmission, NZBGet, Deluge, rTorrent) was using the native
fetch()to call external URLs (e.g. downloaders running locally or remotely), making the application vulnerable to Server-Side Request Forgery (SSRF) and DNS Rebinding via metadata or local networks.🎯 Impact: Attackers could potentially exploit this to access sensitive hostnames or local/cloud metadata networks (169.254.169.254, etc.) via DNS rebinding if they could control the configured downloader URLs.
🔧 Fix: Replaced the native
fetch()calls in the downloader client implementations with the securesafeFetchwrapper fromserver/ssrf.tsto ensure strict host-level validation of all connections. Additionally, updated the test suites to properly mocksafeFetch.✅ Verification: Ran the full test suite (
npm run test) and verified that all tests passed successfully, confirming the intended behavior without regressions.PR created automatically by Jules for task 1321208409107426218 started by @Doezer
Summary by CodeRabbit
Security
Tests
Documentation