backport: harden outbound fetches on release/1.4.0#642
Conversation
Backport the useful security changes from PR #640 to release/1.4.0. Address the review follow-ups by preserving bracketed IPv6 literal support in safeFetch and pinning SABnzbd insecure SSL fallback requests to the resolved IP to avoid DNS rebinding. PR #639 was reviewed but not backported because it is a low-value performance refactor for a stable release branch and still had open review concerns around IGDB ID handling and incomplete optimization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the SSRF protection mechanism by introducing a centralized resolveSafeAddress helper and a normalizeHostname utility in server/ssrf.ts. It updates several instances of standard fetch to use safeFetch across server/downloaders.ts, server/igdb.ts, and server/routes.ts, and adds corresponding tests for bracketed IPv6 literals and SSL fallback. The review feedback highlights several improvement opportunities: handling options.signal and calculating Content-Length in the custom fetchInsecure method, preserving the original error cause in resolveSafeAddress, and explicitly setting allowPrivate: false for external API requests (IGDB and Discord webhooks) as a defense-in-depth measure against SSRF.
| private async fetchInsecure(url: string, options: RequestInit): Promise<Response> { | ||
| const parsedUrl = new URL(url); | ||
| const { address, family } = await resolveSafeAddress(parsedUrl.hostname, true); | ||
| const safeUrl = new URL(url); | ||
| safeUrl.hostname = family === 6 ? `[${address}]` : address; | ||
|
|
||
| const headers = new Headers(options.headers || {}); | ||
| headers.set("Host", parsedUrl.hostname); | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const req = https.request( | ||
| url, | ||
| safeUrl.toString(), | ||
| { | ||
| method: options.method || "GET", | ||
| headers: options.headers as unknown as import("http").OutgoingHttpHeaders, | ||
| headers: Object.fromEntries(headers.entries()) as import("http").OutgoingHttpHeaders, | ||
| rejectUnauthorized: false, | ||
| timeout: 30000, | ||
| }, |
There was a problem hiding this comment.
In fetchInsecure, the custom https.request implementation has two issues:
- Ignoring
options.signal: Timeout or abort signals set by the caller (e.g.,AbortSignal.timeout) are ignored, which can lead to hung requests. We should passsignal: options.signaltohttps.request. - Missing
Content-Length: Unlike standardfetch,https.requestdoes not automatically calculate theContent-Lengthheader forBufferorstringbodies. This can cause some self-hosted downloaders to reject the request with411 Length Requiredor hang. We should calculate and setContent-Lengthifoptions.bodyis present and not already set.
private async fetchInsecure(url: string, options: RequestInit): Promise<Response> {
const parsedUrl = new URL(url);
const { address, family } = await resolveSafeAddress(parsedUrl.hostname, true);
const safeUrl = new URL(url);
safeUrl.hostname = family === 6 ? `[${address}]` : address;
const headers = new Headers(options.headers || {});
headers.set("Host", parsedUrl.hostname);
if (options.body && !headers.has("content-length")) {
if (typeof options.body === "string") {
headers.set("content-length", String(Buffer.byteLength(options.body)));
} else if (Buffer.isBuffer(options.body) || options.body instanceof Uint8Array) {
headers.set("content-length", String(options.body.byteLength));
}
}
return new Promise((resolve, reject) => {
const req = https.request(
safeUrl.toString(),
{
method: options.method || "GET",
headers: Object.fromEntries(headers.entries()) as import("http").OutgoingHttpHeaders,
rejectUnauthorized: false,
timeout: 30000,
signal: options.signal,
},| } catch (error) { | ||
| if (error instanceof Error && error.message === "Invalid or unsafe URL") { | ||
| throw error; | ||
| } | ||
|
|
||
| throw new Error(`Failed to resolve hostname: ${normalizedHostname}`); | ||
| } |
There was a problem hiding this comment.
In resolveSafeAddress, when dns.lookup fails, the catch block throws a generic error: throw new Error("Failed to resolve hostname: ...");. This discards the original error's details (such as ENOTFOUND or EAI_AGAIN), making troubleshooting and logging difficult.
We should preserve the original error by passing it as the cause option in the Error constructor.
| } catch (error) { | |
| if (error instanceof Error && error.message === "Invalid or unsafe URL") { | |
| throw error; | |
| } | |
| throw new Error(`Failed to resolve hostname: ${normalizedHostname}`); | |
| } | |
| } catch (error) { | |
| if (error instanceof Error && error.message === "Invalid or unsafe URL") { | |
| throw error; | |
| } | |
| throw new Error(`Failed to resolve hostname: ${normalizedHostname}`, { cause: error }); | |
| } |
| const response = await safeFetch( | ||
| `https://id.twitch.tv/oauth2/token?client_id=${clientId}&client_secret=${clientSecret}&grant_type=client_credentials`, | ||
| { | ||
| method: "POST", |
There was a problem hiding this comment.
Since IGDB is a public external API, we should explicitly set allowPrivate: false in the safeFetch options as a defense-in-depth measure. This prevents potential SSRF or DNS rebinding attacks targeting internal/private networks if the external domain's DNS is manipulated or hijacked.
const response = await safeFetch(
`https://id.twitch.tv/oauth2/token?client_id=${clientId}&client_secret=${clientSecret}&grant_type=client_credentials`,
{
method: "POST",
allowPrivate: false,References
- For defense-in-depth against Server-Side Request Forgery (SSRF), re-validate external URLs immediately before they are used, even if they were validated when saved.
| const response = await safeFetch(`https://api.igdb.com/v4/${endpoint}`, { | ||
| method: "POST", | ||
| headers: { | ||
| Accept: "application/json", |
There was a problem hiding this comment.
Similar to the authentication request, we should explicitly set allowPrivate: false in the safeFetch options for the IGDB API endpoint to prevent SSRF targeting internal/private networks.
| const response = await safeFetch(`https://api.igdb.com/v4/${endpoint}`, { | |
| method: "POST", | |
| headers: { | |
| Accept: "application/json", | |
| const response = await safeFetch(`https://api.igdb.com/v4/${endpoint}`, { | |
| method: "POST", | |
| allowPrivate: false, | |
| headers: { | |
| Accept: "application/json", |
References
- For defense-in-depth against Server-Side Request Forgery (SSRF), re-validate external URLs immediately before they are used, even if they were validated when saved.
| formData.append("file", new Blob([imageBuffer], { type: "image/png" }), "questarr-stats.png"); | ||
|
|
||
| const discordRes = await fetch(webhookUrl, { method: "POST", body: formData }); | ||
| const discordRes = await safeFetch(webhookUrl, { method: "POST", body: formData }); |
There was a problem hiding this comment.
Since the Discord webhook is a public external service, we should explicitly set allowPrivate: false in the safeFetch options as a defense-in-depth measure. This prevents potential SSRF or DNS rebinding attacks targeting internal/private networks if the webhook URL's DNS is manipulated or hijacked.
| const discordRes = await safeFetch(webhookUrl, { method: "POST", body: formData }); | |
| const discordRes = await safeFetch(webhookUrl, { method: "POST", body: formData, allowPrivate: false }); |
References
- For defense-in-depth against Server-Side Request Forgery (SSRF), re-validate external URLs immediately before they are used, even if they were validated when saved.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 132a0f7cb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return new Promise((resolve, reject) => { | ||
| const req = https.request( | ||
| url, | ||
| safeUrl.toString(), |
There was a problem hiding this comment.
For HTTPS SABnzbd instances with self-signed certs behind a TLS virtual host or reverse proxy, this retry now connects to the resolved IP address, but the request options never set servername back to parsedUrl.hostname. Node does not use a hostname SNI value when the connection target is an IP, so those servers can present the default certificate or reject the handshake before the Host header is used; the previous retry connected to the original hostname and preserved SNI. Please pass the original hostname as the TLS servername when using safeUrl.toString().
Useful? React with 👍 / 👎.
|
1 participant
Summary
elease/1.4.0
Review notes