Skip to content
PR Quorum
Sign inStart free
← Review gallery
wonderwhy-er/DesktopCommanderMCP #597openreviewed 5h ago

Add SSRF protection to read_file URL fetches (#587, #560)

by rahul188+174 64 filesreviewed in 1m 58sView PR on GitHub
minor notesThe panel surfaced minor issues worth a look before merge.3 findings

Reviewer panel

Correctness
deepseek/deepseek-v4-pro
errored
Security
deepseek/deepseek-v4-pro
0 found
Architecture
deepseek/deepseek-v4-pro
3 found

Findings

01Mmedconf 85%

Redirect loop may leak internal IP via DNS resolution

src/tools/filesystem.ts:457

When following redirects, the code calls assertUrlIsFetchable(nextUrl) which performs a DNS lookup on the redirect target's hostname. If the redirect target is a hostname that resolves to a mix of public and private IPs, the guard correctly blocks it. However, the DNS resolution itself has already occurred, potentially leaking the internal hostname to the DNS resolver configured on the host. This is a minor information disclosure vector, but since the DNS query is made before the IP check, an attacker could use a redirect to trigger DNS lookups for internal hostnames. The severity is medium because it requires a malicious or compromised redirect target and a DNS resolver that logs or monitors queries.

02Llowconf 90%

Redirect URL construction may bypass hostname validation

src/tools/filesystem.ts:457

The redirect URL is constructed using new URL(location, currentUrl).toString(). If the location header contains a protocol-relative URL (e.g., //internal.example.com/admin), the resulting URL will inherit the scheme from currentUrl but use the new hostname. This is standard URL resolution behavior, but it means the hostname in the location header is not directly validated as a hostname—only the resolved IP addresses are checked. If internal.example.com resolves to a public IP due to split-horizon DNS or a misconfiguration, the guard would pass, potentially allowing access to an internal service. This is a low-severity edge case because it requires a specific DNS setup and a redirect to a protocol-relative URL.

03Llowconf 80%

PDF download uses validated final URL but may bypass content-type checks

src/tools/filesystem.ts:481

The PDF handling branch now uses currentUrl (the final validated URL after redirects) instead of the original url. While this is correct for SSRF protection, the isPdf check still uses currentUrl.toLowerCase().endsWith('.pdf') as a fallback. If the final URL after redirects does not end in .pdf but the content is actually a PDF, the isPdfFile(contentType) check should catch it. However, if the Content-Type header is missing or incorrect, and the URL does not end in .pdf, the file will be treated as non-PDF, potentially bypassing the PDF parsing logic. This is a low-severity edge case because it requires a misconfigured server and a redirect chain that changes the URL extension.

Want this on your own pull requests?
The same reviewer panel runs on every PR — one focused review, no noise.
Start freeLive demo