fix(security): validate browser navigation targets to block SSRF - #18
Conversation
The screenshot and report routes handed the caller's URL to Chromium, which resolved and navigated on its own. A hostname that resolved to a public address at the API's check could point at an internal or cloud-metadata address (e.g. 169.254.169.254) by the time the browser connected, and headerless endpoints like AWS IMDSv1 were reachable. Validate the target URL before navigating and re-check every request the page makes (redirects, iframes, subresources), aborting any that resolve to a private or reserved address.
🟢 Tier S · Ready to mergeAdds an SSRF guard (
📂 Walkthrough · 4
🔇 Filtered out · 2Findings Hansi considered but did not post.
Reviewed |
Docker Image Stats
Screenshot benchmark: Average of 3 runs on https://appwrite.io |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Re-check every request the page makes — redirects, iframes and | ||
| // subresources each resolve independently and could target an internal | ||
| // address that the initial check never saw. | ||
| await page.route("**/*", async (route, request) => { |
There was a problem hiding this comment.
Knowledge Base Used: Browser execution lifecycle
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/screenshots.ts
Line: 54
Comment:
**Popup navigation escapes the guard** A public page can open a popup to an internal URL. This route is attached only to the original page, so it does not check the popup’s initial request. Chromium can therefore request an internal address during a screenshot or report. **How this was verified:** The route is attached only to the page created for the validated URL, while that page can open another page in the same context.
**Knowledge Base Used:** [Browser execution lifecycle](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/docker-browser/-/docs/browser-execution.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| let allowed = hostAllowed.get(requestUrl.hostname); | ||
| if (allowed === undefined) { | ||
| allowed = isPublicHost(requestUrl.hostname); | ||
| hostAllowed.set(requestUrl.hostname, allowed); | ||
| } |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/screenshots.ts
Line: 58-62
Comment:
**Cached approval survives DNS changes** If an attacker-controlled hostname changes from a public address to an internal one during a capture or audit, later requests to that hostname reuse its first approval. The handler does not check the new address before allowing Chromium’s independent connection, so an internal request can proceed. The report route uses the same cache. **How this was verified:** Both routes cache the first hostname approval, while Chromium is not pinned to the address checked by Node.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| describe("assertPublicUrl", () => { | ||
| test("rejects non-http schemes", async () => { | ||
| await expect(assertPublicUrl("file:///etc/passwd")).rejects.toThrow( | ||
| "is not allowed", | ||
| ); | ||
| await expect(assertPublicUrl("gopher://1.1.1.1/")).rejects.toThrow( | ||
| "is not allowed", | ||
| ); | ||
| }); | ||
|
|
||
| test("rejects internal and metadata targets", async () => { | ||
| await expect( | ||
| assertPublicUrl("http://169.254.169.254/latest/meta-data/"), | ||
| ).rejects.toThrow("not publicly routable"); | ||
| await expect(assertPublicUrl("http://127.0.0.1/")).rejects.toThrow( | ||
| "not publicly routable", | ||
| ); | ||
| await expect(assertPublicUrl("http://[::1]/")).rejects.toThrow( | ||
| "not publicly routable", | ||
| ); | ||
| }); | ||
|
|
||
| test("rejects malformed URLs", async () => { | ||
| await expect(assertPublicUrl("not a url")).rejects.toThrow("Invalid URL"); | ||
| }); |
There was a problem hiding this comment.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/ssrf.test.ts
Line: 48-72
Comment:
**Endpoint blocking remains untested** These tests check the helper’s results and error text, but not whether either endpoint rejects an internal target or blocks a redirect or page resource aimed at one. Browser-backed endpoint tests would catch a broken route guard that this suite currently misses.
**Knowledge Base Used:**
- [Screenshot endpoint](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/docker-browser/-/docs/screenshot-api.md)
- [Lighthouse report endpoint](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/docker-browser/-/docs/lighthouse-report-api.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
What does this PR do?
Closes an SSRF gap in the screenshot and report routes. Both took the caller's URL and handed it to Chromium, which resolves and navigates on its own. Because the calling API (
appwrite/appwrite) can only check the hostname's DNS answer and cannot pin the browser's connection, a hostname that resolved to a public address at check time could resolve to an internal or cloud-metadata address (e.g.169.254.169.254) by the time the browser connected — and headerless endpoints such as AWS IMDSv1 were reachable and rendered into the returned screenshot.Changes:
src/utils/ssrf.ts— a guard built on Node'snet.BlockList.isPublicIp,isPublicHost(resolves and checks every address), andassertPublicUrl(scheme + host). Blocks private, loopback, link-local (incl. the metadata range), CGNAT, ULA, and IPv4-embedding ranges, and handles IPv4-mapped IPv6.screenshots.ts/reports.ts— validate the target URL beforepage.goto, and inpage.route("**/*")re-check every request the page makes (redirects, iframes, subresources), aborting any that resolve to a non-public host. Header injection still applies only to the target origin.tests/unit/ssrf.test.ts— unit coverage for the guard.Test Plan
bun test tests/unit/ssrf.test.ts— 8 pass.bun run type-check,bun run lint(Biome) andbun run check(ESLint) all clean.Notes
appwrite/browserimage pin indocker-compose.yml.