fix(sidecar): block private targets in global fetch#3608
Conversation
|
@lspassos1 is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
|
This PR keeps the fix intentionally narrow for #3549: the sidecar global The compatibility exceptions are explicit and limited to known local sidecar flows: the sidecar's own bound origin, programmatic dev/test Validated with |
![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: 7f5fbb08d9
ℹ️ 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".
| }; | ||
| const pinnedV4 = isAllowedPrivateSidecarFetch(url) ? null : firstIPv4Address(safety.resolvedAddresses); | ||
| if (pinnedV4) { | ||
| requestOptions.lookup = (_hostname, _opts, cb) => cb(null, pinnedV4, 4); |
There was a problem hiding this comment.
The new pinned-IPv4 path in the global fetch override sets requestOptions.lookup to call cb(null, pinnedV4, 4) synchronously, which can race with req.on('error', reject) registration. If the connection fails immediately (for example when the machine is offline or the route is unreachable), the socket emits an unhandled 'error' and the sidecar process exits instead of returning a handler error. This regression is in the new non-private fetch path, so ordinary handler fetch() calls to public URLs can now crash the sidecar under network-failure conditions.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR closes a gap where the process-wide
Confidence Score: 4/5The SSRF fix is fail-closed and correctly guarded; the main compatibility risk is handled through an explicit opt-in flag that cannot be set via env or CLI. The core guard logic is sound — DNS resolution checks all returned addresses, IP-literal fast-paths are handled, and the DNS-pinning pattern is correctly applied after the safety check. The two minor issues found do not affect the correctness of the SSRF protection itself. The SSRF logic lives entirely in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Handler calls fetch] --> B{http or https?}
B -- No --> C[_originalFetch passthrough]
B -- Yes --> D{isAllowedPrivateSidecarFetch?}
D -- Yes --> E[Skip SSRF check, no IP pinning]
D -- No --> F[isSafeUrl: DNS resolve and check all IPs are public]
F -- unsafe --> G[Throw ERR_SSRF_BLOCKED]
F -- safe --> H[Pin first IPv4 via lookup callback]
E --> I[acquireUpstreamSlot]
H --> I
I --> J[http or https.request with family 4]
J --> K[releaseUpstreamSlot]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -17,6 +18,8 @@ const brotliCompressAsync = promisify(brotliCompress); | |||
| // IPv6 endpoints time out, causing ETIMEDOUT. This override ensures ALL | |||
| // fetch() calls in dynamically-loaded handler modules (api/*.js) use IPv4. | |||
There was a problem hiding this comment.
_privilegedFetch bypasses upstream concurrency slot
_privilegedFetch calls _originalFetch directly, skipping both the SSRF guard and the acquireUpstreamSlot / releaseUpstreamSlot rate-limiter that ipv4Fetch enforces. Because fetchWithTimeout uses _privilegedFetch for HTTP calls whenever allowPrivateNetwork: true, concurrent start-up warm-up probes to Ollama and LLM_API_URL can fire simultaneously and unconstrained. In the current code the affected callers are limited and low-volume, but any future caller that sets allowPrivateNetwork: true on an HTTP URL will silently escape the concurrency budget.
| return _yahooQueue; | ||
| } | ||
|
|
||
| function redactUrlForLog(rawUrl) { | ||
| try { | ||
| const redacted = new URL(String(rawUrl)); | ||
| redacted.username = ''; | ||
| redacted.password = ''; | ||
| return redacted.toString(); |
There was a problem hiding this comment.
redactUrlForLog preserves query parameters
redactUrlForLog strips username and password but leaves the full path and query string intact in the ERR_SSRF_BLOCKED error message. Credentials or tokens passed as query parameters (common in OAuth callback URLs and third-party integrations) would be surfaced verbatim in the blocked-URL error, which may end up in logs that capture the thrown Error's message.
lspassos1
force-pushed
the
fix/sidecar-fetch-ssrf
branch
from
7f5fbb0 to
a8e9f90
Compare
|
Reviewed all bot feedback on #3608 and addressed the actionable items in a8e9f90:
Re-ran |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8e9f90ebc
ℹ️ 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".
| const fetchImpl = allowPrivateNetwork ? _privilegedFetch : fetch; | ||
| return await fetchImpl(fetchUrl, { ...fetchOptions, headers: fetchHeaders, signal: controller.signal }); |
There was a problem hiding this comment.
fetchWithTimeout() now routes allowPrivateNetwork calls through _originalFetch (const fetchImpl = allowPrivateNetwork ? _privilegedFetch : fetch), which bypasses the sidecar's globalThis.fetch IPv4 override. That regression affects newly-updated call sites like validateSecretAgainstProvider() and /api/llm-health probes that pass allowPrivateNetwork: true: these requests can again prefer/bind to IPv6 and fail or time out on environments where IPv6 is misconfigured but IPv4 is healthy, causing false negative health/validation results.
Useful? React with 👍 / 👎.
1 participant
Summary
Closes #3549. The Tauri sidecar global
fetchoverride now treats SSRF protection as the default floor for handler fetches: http(s) targets are validated before connection, private/reserved addresses are blocked, and validated IPv4 DNS results are pinned into the outbound request.Root cause
The sidecar had an SSRF guard for
/api/rss-proxy, but the process-wideglobalThis.fetchreplacement did not use it. Any sidecar handler that later fetched a user-influenced URL through globalfetchcould reach loopback, link-local metadata, or private RFC1918 addresses unless that handler remembered to add its own guard.Changes
isSafeUrl()from the global sidecarfetchoverride before outbound http(s) requests.ERR_SSRF_BLOCKEDerror and a redacted URL in the message.remoteBase, and Ollama/LLM health probes.Validation
node --test src-tauri/sidecar/local-api-server.test.mjs- 39/39 passnpm run test:sidecar- 96/96 passnpm run typechecknpm run typecheck:apinpx biome lint src-tauri/sidecar/local-api-server.mjs src-tauri/sidecar/local-api-server.test.mjs- exits 0; existing complexity/HTML-string warnings remain inlocal-api-server.mjsgit diff --check -- src-tauri/sidecar/local-api-server.mjs src-tauri/sidecar/local-api-server.test.mjsRisk
Medium-low. The intended behavior change is fail-closed for sidecar handler
fetch()calls to private networks. The main compatibility risk is code that intentionally used globalfetchfor arbitrary localhost services; known local LLM/Ollama probes are explicitly preserved, and the sidecar's own local origin remains allowed.