Harden URL reads and remote device call targeting - #619
Open
AMeloDragon wants to merge 3 commits into
Open
Conversation
Block SSRF-prone URL targets in read_file URL mode and require explicit device_id matching before processing remote calls. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change filters remote tool calls by device ID and adds SSRF protections to URL fetching. URL validation checks HTTPS targets and resolved addresses, validates redirects, limits redirect hops, and parses PDFs from buffered responses. ChangesRemote tool-call routing
Secure URL fetching
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/tools/filesystem.ts (1)
48-87: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the blocked IP ranges.
isPrivateIpAddressmisses several ranges that are commonly used for SSRF against cloud and local services:
- IPv4:
192.0.0.0/24,198.18.0.0/15, multicast224.0.0.0/4, reserved240.0.0.0/4, and broadcast255.255.255.255.- IPv6: the unspecified address
::, multicastff00::/8, NAT6464:ff9b::/96, and IPv4-compatible::a.b.c.dforms.Note that
169.254.169.254is already blocked by thea === 169 && b === 254branch, so cloud metadata over IPv4 is covered.🔒 Proposed additional checks
const [a, b] = octets; return ( a === 0 || // "this network" a === 10 || a === 127 || (a === 100 && b >= 64 && b <= 127) || // carrier-grade NAT (a === 169 && b === 254) || (a === 172 && b >= 16 && b <= 31) || - (a === 192 && b === 168) + (a === 192 && b === 168) || + (a === 192 && b === 0) || // 192.0.0.0/24 IETF protocol assignments + (a === 198 && (b === 18 || b === 19)) || // benchmarking + a >= 224 || // multicast, reserved, broadcast + ip === '255.255.255.255' ); } // IPv6 local/loopback/IPv4-mapped ranges if (ipVersion === 6) { - if (ip === '::1') { + if (ip === '::1' || ip === '::') { return true; } + if (ip.startsWith('ff')) { + return true; // multicast (ff00::/8) + } + if (ip.startsWith('64:ff9b:')) { + return true; // NAT64 + } if (ip.startsWith('fc') || ip.startsWith('fd')) { return true; // unique local address space (fc00::/7) }🤖 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 `@src/tools/filesystem.ts` around lines 48 - 87, Extend isPrivateIpAddress to classify the requested IPv4 ranges as private, including 192.0.0.0/24, 198.18.0.0/15, multicast 224.0.0.0/4, reserved 240.0.0.0/4, and 255.255.255.255. In its IPv6 handling, also block ::, ff00::/8, NAT64 addresses under 64:ff9b::/96, and IPv4-compatible ::a.b.c.d forms by delegating the embedded IPv4 value to isPrivateIpAddress; preserve the existing IPv4-mapped handling and metadata-range coverage.
🤖 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 `@src/remote-device/remote-channel.ts`:
- Around line 217-222: Update the Realtime subscription and its underlying
RLS/routing configuration to enforce a device-level constraint equivalent to
new.device_id = auth.uid() before remote call payloads reach the handler. Keep
the payloadDeviceId check in the remote-channel handler as defense in depth, but
do not rely on handleNewToolCall or local filtering as the primary authorization
boundary.
In `@src/tools/filesystem.ts`:
- Around line 508-513: Update the PDF handling around isPdf and
parsePdfToMarkdown to parse the already fetched, validated response body via a
buffer-accepting entry point in the PDF markdown parser, avoiding any second URL
request and preserving the existing redirect validation, abort signal, and
timeout protections. Change PDF extension detection to use currentUrl.pathname
so query strings and fragments do not prevent recognizing .pdf resources.
- Around line 471-499: Update the redirect-fetch loop around
validateRemoteReadUrl so each fetch connection is constrained to the already
validated address for its hostname, preventing a second unconstrained DNS
resolution; preserve the original hostname for Host/TLS SNI when connecting by
IP. Before following each redirect, consume or cancel the prior redirect
response body to release resources.
---
Nitpick comments:
In `@src/tools/filesystem.ts`:
- Around line 48-87: Extend isPrivateIpAddress to classify the requested IPv4
ranges as private, including 192.0.0.0/24, 198.18.0.0/15, multicast 224.0.0.0/4,
reserved 240.0.0.0/4, and 255.255.255.255. In its IPv6 handling, also block ::,
ff00::/8, NAT64 addresses under 64:ff9b::/96, and IPv4-compatible ::a.b.c.d
forms by delegating the embedded IPv4 value to isPrivateIpAddress; preserve the
existing IPv4-mapped handling and metadata-range coverage.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 979e45f1-8645-4fe5-9d4b-1797c8f3f33c
📒 Files selected for processing (3)
src/remote-device/device.tssrc/remote-device/remote-channel.tssrc/tools/filesystem.ts
Pin URL fetch DNS to validated addresses, cancel redirect bodies, and parse PDF bytes from the validated fetch response to avoid a second unvalidated request. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens two security-sensitive areas of the codebase: URL-based remote reads (to reduce SSRF exposure) and remote tool-call processing (to ensure calls are only executed by the explicitly targeted device). It also refactors PDF parsing so URL reads can reuse already-fetched PDF bytes instead of re-downloading.
Changes:
- Added HTTPS-only URL validation with DNS/IP allowlisting and manual redirect re-validation for
readFileFromUrl. - Enforced strict
device_idmatching for remote tool calls both at realtime subscription handling and at execution time. - Introduced
parsePdfBufferToMarkdownto parse already-fetched PDF bytes (and updated exports/imports accordingly).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/pdf/markdown.ts | Adds a buffer-based PDF-to-Markdown entry point and routes string-source parsing through it. |
| src/tools/pdf/index.ts | Re-exports the new parsePdfBufferToMarkdown API. |
| src/tools/filesystem.ts | Implements URL SSRF hardening, redirect handling, and switches PDF URL handling to parse from fetched bytes. |
| src/remote-device/remote-channel.ts | Drops realtime tool-call events that are not explicitly targeted to the current device. |
| src/remote-device/device.ts | Requires device_id to be present and match the current device before executing a tool call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tools/pdf/markdown.ts (1)
262-283: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not leave
parsePdfToMarkdownaccepting URL strings.
parsePdfToMarkdown()still callsfetch(source)forsourcevalues that start withhttp://orhttps://, without HTTPS pinning, redirect/timeout limits, or private/loopback blocking. Accept only local PDF paths here, or route URL fetches through the same validated URL read path used byreadFileFromUrl()before callingparsePdfBufferToMarkdown().🤖 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 `@src/tools/pdf/markdown.ts` around lines 262 - 283, The parsePdfToMarkdown flow must not directly fetch URL sources through loadPdfToBuffer. Restrict loadPdfToBuffer and parsePdfToMarkdown to local PDF paths, or reuse the existing validated readFileFromUrl path for URLs—including its HTTPS, redirect, timeout, and private/loopback protections—before passing data to parsePdfBufferToMarkdown.src/tools/filesystem.ts (1)
607-618: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDrop the
instanceof DOMExceptioncheck for fetch timeouts.
cross-fetch4.1.0 usesnode-fetch2.7.0 in Node environments, and thatAbortErroris anErrorsubclass, not aDOMException. Useerror.name === 'AbortError'so these fetch timeouts get the specific timeout message instead ofFailed to fetch URL.💡 Proposed fix
- const errorMessage = error instanceof DOMException && error.name === 'AbortError' + const errorMessage = error instanceof Error && error.name === 'AbortError' ? `URL fetch timed out after ${FILE_OPERATION_TIMEOUTS.URL_FETCH}ms: ${url}` : `Failed to fetch URL: ${error instanceof Error ? error.message : String(error)}`;🤖 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 `@src/tools/filesystem.ts` around lines 607 - 618, Update the error classification in the URL fetch catch block to identify abort timeouts using error.name === 'AbortError' without requiring error to be a DOMException. Preserve the existing timeout message for abort errors and the generic failure message for all other errors in the fetch flow.
🤖 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.
Outside diff comments:
In `@src/tools/filesystem.ts`:
- Around line 607-618: Update the error classification in the URL fetch catch
block to identify abort timeouts using error.name === 'AbortError' without
requiring error to be a DOMException. Preserve the existing timeout message for
abort errors and the generic failure message for all other errors in the fetch
flow.
In `@src/tools/pdf/markdown.ts`:
- Around line 262-283: The parsePdfToMarkdown flow must not directly fetch URL
sources through loadPdfToBuffer. Restrict loadPdfToBuffer and parsePdfToMarkdown
to local PDF paths, or reuse the existing validated readFileFromUrl path for
URLs—including its HTTPS, redirect, timeout, and private/loopback
protections—before passing data to parsePdfBufferToMarkdown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79f8416d-d973-477d-84ba-2933af4c2555
📒 Files selected for processing (3)
src/tools/filesystem.tssrc/tools/pdf/index.tssrc/tools/pdf/markdown.ts
Handle expanded IPv6 loopback forms in SSRF checks and avoid an extra PDF buffer copy during validated URL reads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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.
Why
This change closes two high-risk security gaps found during audit: server-side request forgery via URL-based file reads, and cross-device remote tool execution when calls are not explicitly bound to a target device.
What changed
readFileFromUrlto reduce SSRF risk:device_idis present and exactly matches the current device:remote-channel.ts)device.ts)Notes for reviewers
Summary by CodeRabbit
Bug Fixes
New Features