-
Notifications
You must be signed in to change notification settings - Fork 18
π‘οΈ Sentinel: [HIGH] Enforce safeFetch for downloader API connections to prevent SSRF #889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ vi.mock("../logger.js", () => ({ | |
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π Maintainability & Code Quality | π Major | ποΈ Heavy lift Centralize the repeated SSRF test setup. The same mock is defined in six files. Move it to
As per coding guidelines, shared test setup must use π Affects 6 files
π€ Prompt for AI AgentsSource: 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 pathCover π Affects 6 files
π€ Prompt for AI Agents |
||
| })); | ||
|
|
||
| vi.mock("../downloaders/utils.js", async (importOriginal) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,11 @@ vi.mock("../logger.js", () => ({ | |
|
|
||
| const fetchMock = vi.fn(); | ||
| global.fetch = fetchMock as unknown as typeof fetch; | ||
| 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 }), | ||
| })); | ||
|
Comment on lines
+31
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π― 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 The mock aliases let regressions to native π Affects 3 files
π€ Prompt for AI Agents |
||
|
|
||
| const createDownloader = (overrides: Partial<Downloader> = {}): Downloader => { | ||
| const now = new Date("2024-01-01T00:00:00.000Z"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π 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
Source: Linters/SAST tools