Skip to content

fix(review): close trailing dot and *.localhost SSRF host bypasses#1382

Open
Dexterity104 wants to merge 1 commit into
JSONbored:mainfrom
Dexterity104:fix/ssrf-trailing-dot-hostname
Open

fix(review): close trailing dot and *.localhost SSRF host bypasses#1382
Dexterity104 wants to merge 1 commit into
JSONbored:mainfrom
Dexterity104:fix/ssrf-trailing-dot-hostname

Conversation

@Dexterity104

Copy link
Copy Markdown

Summary

The SSRF host guard in src/review/content-lane/safe-url.ts returned a public verdict for several named loopback hosts, because its checks only matched the bare forms. Two gaps let an untrusted URL pass isSafeHttpUrl while still resolving to loopback or an internal address:

  • Trailing FQDN root dot. new URL("https://localhost./").hostname keeps the dot as localhost., which never equals localhost and never ends with .local, so the guard read it as public. The parser strips the dot for IP literals, so only named hosts leaked. The same held for foo.local. and bar.internal..
  • The RFC 6761 *.localhost namespace. The guard matched bare localhost but not db.localhost or foo.bar.localhost, which systemd-resolved and browsers (including the visual lane headless Chrome) resolve to loopback.

The fix normalizes the host once at the shared chokepoint hostIsPrivateOrLocal: lowercase, strip the trailing dot run, then reject localhost, the *.localhost namespace, .local, and .internal. Both isSafeHttpUrl and isSafeEndpointUrl inherit the hardening through that single function, and the duplicate .toLowerCase() at both call sites was removed so normalization lives in one place.

Scope note: the trailing dot case was the reported gap. The *.localhost case is the same class of named loopback bypass on the same line, so both are closed together. The behavior was confirmed in Node and in the Workers runtime, and public hosts such as example.com. stay allowed.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • Linked the tracking issue, or this fix is self-contained and the summary explains why a separate issue is not required.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • No checks were skipped. The whole gate ran green through npm run test:ci plus npm audit --audit-level=moderate. npm run test:coverage reports 100% lines and branches on safe-url.ts, with every changed line and branch arm exercised. Two negative path regression tests were added (the *.localhost namespace and the trailing dot forms); each fails before the fix and passes after.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable. This change is backend only and has no visible UI surface, so no screenshots are required.

Notes

  • The guard feeds server side fetches in source-evidence.ts (rechecked per redirect hop) and the screenshot navigation guard in visual/shot.ts, so the hardening covers both the HTTPS fetch path and the secure WebSocket endpoint path.
  • Normalization was consolidated into hostIsPrivateOrLocal, which keeps a single defensive point for the lowercase plus trailing dot strip and avoids the duplicate host normalization that existed at the two call sites.

@Dexterity104 Dexterity104 requested a review from JSONbored as a code owner June 25, 2026 17:43
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jun 25, 2026
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.36%. Comparing base (f160cbb) to head (30ffe86).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1382   +/-   ##
=======================================
  Coverage   95.35%   95.36%           
=======================================
  Files         191      191           
  Lines       20711    20712    +1     
  Branches     7488     7489    +1     
=======================================
+ Hits        19750    19751    +1     
  Misses        380      380           
  Partials      581      581           
Files with missing lines Coverage Δ
src/review/content-lane/safe-url.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant