Skip to content

fix(cli): re-validate SSRF denylist on redirects + harden isPrivateUrl#1212

Merged
vanceingalls merged 3 commits into
mainfrom
06-04-fix_cli_re-validate_ssrf_denylist_on_redirects_harden_isprivateurl
Jun 6, 2026
Merged

fix(cli): re-validate SSRF denylist on redirects + harden isPrivateUrl#1212
vanceingalls merged 3 commits into
mainfrom
06-04-fix_cli_re-validate_ssrf_denylist_on_redirects_harden_isprivateurl

Conversation

@vanceingalls
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls commented Jun 5, 2026

Summary

  • Adds safeFetch, a redirect-aware wrapper around fetch that re-runs the SSRF denylist on every hop before following a redirect.
  • Routes fetchBuffer and the Lottie media fetch through safeFetch so redirect chains can't bounce through a public URL to reach an internal or cloud-metadata host.
  • Hardens isPrivateUrl to also block 0.0.0.0 / 0.0.0.0/8, IPv6 loopback (::1), IPv4-mapped (::ffff:…), unique-local (fc00::/7), and link-local (fe80::/10) ranges.

Security

F-002 MEDfetchBuffer followed redirects without re-checking the denylist on the destination. A 30x redirect from an allowlisted public URL to 169.254.169.254 or an internal host would succeed, leaking the response to the caller (e.g. captured page assets written to local disk).

F-003 MEDisPrivateUrl did not cover 0.0.0.0 (maps to localhost on most OSes), IPv6 loopback, or IPv6 private ranges. An asset URL using those addresses would bypass the denylist. Alternate IPv4 encodings (decimal/octal/hex) are already normalized to dotted-quad by WHATWG URL parsing and remain blocked.

Test plan

  • Unit tests cover redirect-chain blocking (redirect to metadata IP rejected)
  • Unit tests cover new isPrivateUrl address forms (0.0.0.0, ::1, fc00::1, fe80::1, ::ffff:192.168.1.1)
  • Existing fetch and asset-download tests pass

Copy link
Copy Markdown
Collaborator Author

vanceingalls commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: #1212 — SSRF denylist on redirects + harden isPrivateUrl

Solid hardening. Notes:

isPrivateUrl

  • Protocol check moved to top — file://, ftp://, etc. are now blocked before hostname parsing. ✓
  • 0.0.0.0/8 block added (was missing before, maps to localhost on some kernels). ✓
  • 127.0.0.0/8 block is now correct (covers all of loopback, not just 127.0.0.1). ✓
  • WHATWG URL canonicalization means decimal/hex/octal IPv4 like http://2130706433/ and http://0x7f000001/ are already normalized to dotted-quad before isPrivateIpv4 sees them. The tests confirm this assumption. ✓

safeFetch

  • Manual redirect loop with redirect: "manual" re-runs isPrivateUrl on every Location header. This is the correct approach — redirect: "follow" would bypass the check on subsequent hops. ✓
  • MAX_FETCH_REDIRECTS=5 is a reasonable cap.
  • One potential issue: new URL(loc, current) will throw if loc is malformed. Wrapping that in try/catch and returning null would be safer, otherwise a redirect to a malformed Location surfaces as an unhandled exception rather than a null return.

fetchBuffer / saveLottieAnimations

  • Both callsites switched to safeFetch. ✓
  • if (!res || !res.ok) correctly handles the null return. ✓

Minor nit on the redirect resolve: add a try/catch around new URL(loc, current). Otherwise LGTM.

Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One actionable item before merging: in safeFetch, the redirect resolution via new URL(loc, current) can throw if the Location header is malformed. That would surface as an unhandled exception rather than a clean null return. Please wrap it in try/catch and return null on parse failure.

@vanceingalls vanceingalls force-pushed the 06-04-fix_cli_bind_studio_preview_server_to_loopback_by_default branch from 488f5e3 to 74fb9d2 Compare June 5, 2026 21:06
@vanceingalls vanceingalls force-pushed the 06-04-fix_cli_re-validate_ssrf_denylist_on_redirects_harden_isprivateurl branch from 38d4151 to a659df1 Compare June 5, 2026 21:06
@vanceingalls
Copy link
Copy Markdown
Collaborator Author

The try/catch around new URL(loc, current) was added in the same PR (see safeFetch lines 335–339 in assetDownloader.ts). Malformed Location headers now return null cleanly. This should resolve the change request.

miguel-heygen
miguel-heygen previously approved these changes Jun 5, 2026
@vanceingalls vanceingalls changed the base branch from 06-04-fix_cli_bind_studio_preview_server_to_loopback_by_default to graphite-base/1212 June 5, 2026 21:48
vanceingalls and others added 2 commits June 5, 2026 21:49
fetchBuffer followed redirects without re-checking the destination, so a
public asset URL on a captured page could 30x to an internal or cloud-metadata
host (the response then landing on local disk). Add safeFetch, which resolves
redirects manually and re-runs the denylist on every hop, and route fetchBuffer
and the lottie media fetch through it.

Harden isPrivateUrl to also block 0.0.0.0 (and 0.0.0.0/8) and IPv6 loopback,
IPv4-mapped, unique-local (fc00::/7) and link-local (fe80::/10) ranges.
Alternate IPv4 encodings (decimal/octal/hex) are already normalized to
dotted-quad by WHATWG URL parsing, so they were and remain blocked.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
new URL(loc, current) throws a TypeError when the Location header is not a
valid URL.  Wrap in try/catch and return null so a malformed redirect is
treated the same as a blocked/private redirect rather than surfacing as an
unhandled exception.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vanceingalls vanceingalls force-pushed the 06-04-fix_cli_re-validate_ssrf_denylist_on_redirects_harden_isprivateurl branch from a659df1 to fd683a1 Compare June 5, 2026 21:49
@graphite-app graphite-app Bot changed the base branch from graphite-base/1212 to main June 5, 2026 21:50
@graphite-app graphite-app Bot dismissed miguel-heygen’s stale review June 5, 2026 21:50

The base branch was changed.

@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented Jun 5, 2026

Merge activity

  • Jun 5, 9:50 PM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.
  • Jun 6, 12:01 AM UTC: @vanceingalls merged this pull request with Graphite.

@vanceingalls vanceingalls merged commit 1f37920 into main Jun 6, 2026
47 checks passed
@vanceingalls vanceingalls deleted the 06-04-fix_cli_re-validate_ssrf_denylist_on_redirects_harden_isprivateurl branch June 6, 2026 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants