Repository navigation
[Bugs] mcp, editor: harden image URL fetching and login redirect - #1122
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 9425501d56
ℹ️ 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 location = res.headers.get('location') | ||
| const status = res.statusCode ?? 0 | ||
| if (status >= 300 && status < 400) { | ||
| res.resume() |
There was a problem hiding this comment.
When an attacker-controlled endpoint returns a redirect or non-2xx response with an unbounded body, res.resume() drains that body without applying maxBytes; after the next hop succeeds or the error is thrown, the finally block clears the timeout while the prior response can continue streaming indefinitely. Repeated image-tool calls can therefore leave persistent sockets consuming bandwidth and resources despite the advertised size and timeout limits. Destroy these responses, or drain them under the same cap and abort lifecycle, rather than resuming them unchecked.
Useful? React with 👍 / 👎.
| details: Record<string, unknown>, | ||
| log: Record<string, unknown> = {}, | ||
| ): never { | ||
| console.error(`[pascal-mcp] safe_fetch_blocked ${JSON.stringify({ reason, ...details, ...log })}`) |
There was a problem hiding this comment.
Blocked requests currently serialize details.url verbatim, including URL userinfo and query parameters. Image inputs commonly use presigned URLs whose query contains credentials or access tokens, so an allowlist rejection, redirect to a blocked host, or newly-private DNS answer can persist those secrets in server logs visible beyond the MCP caller. Log only a sanitized origin/hostname and path, or explicitly strip credentials and the query string.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9425501. Configure here.
| if (a === 0) return true // current-network | ||
| if (a >= 224) return true // multicast / reserved | ||
| if (a === 198 && (b === 18 || b === 19)) return true // 198.18.0.0/15 benchmarking | ||
| if (a! >= 224) return true // multicast, reserved, broadcast |
There was a problem hiding this comment.
IPv4 /24 check blocks a /16
Medium Severity
isBlockedV4 treats any 192.0.*.* address as blocked. The comment describes IETF 192.0.0.0/24, but the check only compares the first two octets, so it matches 192.0.0.0/16. Public hosts in that /16 (including ICANN/IANA space such as 192.0.43.8) are refused by safeFetch, so vision and photo_to_scene image URLs on those addresses fail.
Reviewed by Cursor Bugbot for commit 9425501. Configure here.
2 participants
What does this PR do?
Two security hardening fixes.
mcp:safeFetch(used byanalyze_floorplan_image,analyze_room_photo,photo_to_scene)dns.lookup({ all: true })and requires every resolved address to be public.100.64.0.0/10, NAT6464:ff9b::/96, and a few other special-use ranges.lookup. Requests go throughnode:http/node:httpsinstead of globalfetch, which works the same under Node and Bun.[pascal-mcp] safe_fetch_blocked {...}line to stderr whenever a fetch is refused by policy (scheme, host/address, or origin allowlist).resolve,request) is an optional third argument so tests can cover redirects and DNS changes without real network access. Callers are unchanged.apps/editor: post-loginnextredirectsafeNextPathmoved tolib/safe-next-path.ts. It now parsesnextagainst the app's public origin, requires the same origin, and returns the normalised path, falling back to/otherwise.How to test
bun test packages/mcp/src/lib/safe-fetch.test.ts: covers the address classifier for every range and encoding, DNS answers containing any private address, rechecks on redirects, connection pinning (including one real socket test), size caps, and the block log.cd apps/editor && bun test lib/safe-next-path.test.ts: same-origin paths are kept and anything else falls back to/.bun run cipasses locally.Screenshots / screen recording
N/A, non-visual change.
Checklist
bun devbun checkto verify)mainbranchNote
High Risk
Rewrites server-side URL fetching (SSRF/DNS-rebind) and post-login redirect validation—both are classic high-impact attack surfaces if wrong.
Reviewed by Cursor Bugbot for commit 9425501. Bugbot is set up for automated code reviews on this repo. Configure here.