Skip to content

fix(utils): prevent SSRF in untrusted URL downloads - #2298

Open
KevinAndrewDong wants to merge 3 commits into
refly-ai:mainfrom
KevinAndrewDong:fix/ssrf-2280
Open

KevinAndrewDong wants to merge 3 commits into
refly-ai:mainfrom
KevinAndrewDong:fix/ssrf-2280

Conversation

@KevinAndrewDong

@KevinAndrewDong KevinAndrewDong commented Oct 1, 2026 •

Copy link
Copy Markdown

Summary

Untrusted URLs passed to /v1/misc/scrape and dumpFileFromURL could reach internal services and return page metadata or upload response bodies. Both paths now use a shared server-only safeFetch.

Fixes #2280.

  • Restrict destinations to public HTTP(S) addresses, including IPv4-mapped IPv6 handling and rejection of private, special and mixed public/private DNS answers.
  • Validate DNS answers in the actual socket lookup and pass only checked addresses to the connection layer. Disable keep-alive connection reuse with pipelining: 0.
  • Follow redirects manually, revalidating each destination, with a limit of 20 redirects.
  • Keep policy rejections as ParamsError and normalize DNS/connection failures to ConnectionError, without exposing URLs, resolved addresses or chained causes.

Compatibility

  • Preserve HTML metadata extraction and binary upload behavior.
  • Add no custom URL-length or userinfo restriction; the underlying fetch credential restriction remains unchanged.
  • Export safeFetch and scrapeWeblink from @refly/utils/server so Node dependencies do not enter the browser-facing root export. Update the existing API consumers.
  • Add ipaddr.js and undici as direct dependencies.

Validation

Latest HTTPS follow-up (2026-10-08; production implementation unchanged):

  • Focused safe-fetch suites: 108 passed on each of Node 24.0.0 and 26.10.0 (101 existing tests plus 7 real HTTPS tests).
  • HTTPS fixtures exercise the original hostname, Host header and SNI with native TLS validation, reject hostname mismatch and an untrusted certificate, and revalidate public redirects. Private-IP, private-DNS and rebinding redirects are blocked before a second server connection.
  • Negative controls disabling hostname validation or replacing the guarded socket lookup made the relevant tests fail; the test file was restored exactly.
  • Changed-test TypeScript, Biome and diff checks passed.

Original implementation validation (2026-10-01; not rerun for this test-only follow-up):

  • API misc SSRF suite: 5 passed, covering both consumers and binary upload behavior.
  • Utils/API TypeScript checks, official API build and Biome passed.
  • Private-IPv4 mutation triggered 6 failures; source restored exactly and all 101 focused tests passed afterward.

Full suites are not green. The original comparison against clean main 71f8b875 found:

  • Utils: patched 255 passed / 43 failed; baseline 154 passed / 43 failed. All 43 failed test identities match.
  • API: both runs have the same 4 failed suites and 9 passed suites before terminating with ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG; neither full run completed.

The HTTPS tests use real Undici/TLS connections with mocked DNS, a test CA and test-only routing to a local server after the production socket lookup approves the public address. Default certificate and hostname checks remain enabled. Public-network HTTPS and production end-to-end behavior remain unverified.

AI assistance

ChatGPT assisted with SSRF threat-model and compatibility review. Codex assisted with reproduction, the shared fetch guard and regression validation, including the local HTTPS follow-up.

Summary by CodeRabbit

  • Bug Fixes
    • Web link previews and URL downloads now reject unsafe destinations, including private network addresses, invalid DNS results, and unsafe redirect targets.
    • Connection and address-validation failures return generic errors without exposing sensitive URL or network details.
    • Redirects are supported up to a limit of 20, with each destination checked before connecting.
    • HTTPS connections validate certificates against the requested hostname, including after redirects.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: afdae39d-177d-48ac-aa81-4dc628ea0e6a
📥 Commits

Reviewing files that changed from the base of the PR and between eceda89 and 721df21.

📒 Files selected for processing (7)
  • packages/utils/src/__fixtures__/safe-fetch-tls/README.md
  • packages/utils/src/__fixtures__/safe-fetch-tls/ca.crt
  • packages/utils/src/__fixtures__/safe-fetch-tls/server.crt
  • packages/utils/src/__fixtures__/safe-fetch-tls/server.key
  • packages/utils/src/__fixtures__/safe-fetch-tls/untrusted.crt
  • packages/utils/src/__fixtures__/safe-fetch-tls/untrusted.key
  • packages/utils/src/safe-fetch.https.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a server-side URL fetch helper that validates destinations and redirect targets. The scraper and API URL-download path use this helper. The patch also adds server-specific utility exports and tests for validation, error handling, and download behavior.

Changes

SSRF-safe URL retrieval

Layer / File(s) Summary
Validate destinations and redirects
packages/utils/src/safe-fetch.ts, packages/utils/src/safe-fetch.test.ts, packages/utils/src/safe-fetch.https.test.ts, packages/utils/src/__fixtures__/safe-fetch-tls/*, packages/utils/package.json, packages/utils/tsconfig.json
Adds safeFetch, which restricts URL schemes, checks literal IPs and DNS answers, validates redirects, and normalizes errors. Tests cover destination policy, redirects, socket lookup, TLS validation, and response handling.
Expose guarded server utilities and update scraping
packages/utils/src/server.ts, packages/utils/src/index.ts, packages/utils/src/scrape-weblink.ts, packages/utils/package.json
Adds a server-specific export for safeFetch and scrapeWeblink. Removes scrapeWeblink from the general utility barrel and changes it to fetch through safeFetch.
Use guarded fetching in API downloads
apps/api/src/modules/misc/misc.service.ts, apps/api/src/modules/misc/misc.service.ssrf.spec.ts
Changes MiscService scraping and URL downloads to use the server utilities. Tests check result handling, propagated errors, and the arguments passed to uploadFile.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: High

Sequence Diagram(s)

sequenceDiagram
  participant MiscController
  participant MiscService
  participant scrapeWeblink
  participant safeFetch
  participant socketLookup
  participant remoteHost
  MiscController->>MiscService: submit URL for scraping
  MiscService->>scrapeWeblink: scrape URL
  scrapeWeblink->>safeFetch: fetch URL
  safeFetch->>socketLookup: validate DNS answers
  socketLookup-->>safeFetch: return validated address
  safeFetch->>remoteHost: request URL
  remoteHost-->>safeFetch: return response or redirect
  safeFetch->>socketLookup: validate redirect destination
  safeFetch-->>scrapeWeblink: return response
  scrapeWeblink-->>MiscService: return metadata
  MiscService-->>MiscController: return scrape result
Loading

Merge Risk: ⚪ Minimal · up to 721df

This change adds SSRF protection for URL downloads, with test certificates and HTTPS tests for the new guard. No merge-blocking issue was identified in the reviewed files.

Security Architecture Review

Security architecture risk: 🔵 Low · up to eceda

The change narrows outbound network access without an established expansion of authentication or storage authority. No introduced security issue was identified in the reviewed paths. Complete caller coverage and production HTTPS behavior remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The security-relevant authority is the API server's outbound network reachability. Caller-selected destinations can produce returned metadata or stored response bytes, so enforcing the shared policy protects both outcomes without adding storage privileges.

Trust Boundaries and Controls

  • observed — The helper checks literal destinations and normalizes IPv4-mapped IPv6 addresses. Its actual socket lookup rejects empty or mixed public/non-public DNS answers and passes only checked addresses onward. Manual redirects repeat destination validation, and connection reuse is disabled.

Resilience and Maintainability Implications

  • observed — Redirect state is local to each invocation, intermediate bodies are cancelled before advancing, and cancellation failures terminate traversal. Storage begins only after final-body buffering. Existing random-key upload behavior and database-before-object-storage ordering predate this PR and are not new concerns.

Hardening Proposals

  • proposed — Add an HTTPS integration check covering TLS hostname verification, checked socket resolution, and redirects. This would strengthen deployment assurance beyond the reviewed mocks and local HTTP fixtures; it is not evidence of a current vulnerability.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 7 files. (6 skipped: 6… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed PR #2298 meets the coding requirements in directly linked issue #2280. safeFetch is shared by scrapeWeblink and dumpFileFromURL. It permits only HTTP(S), rejects unsafe literal and DNS-resolved …
Out of Scope Changes check ✅ Passed The changes remain within issue #2280 scope. The server-only export, API consumer updates, direct dependencies, error normalization, and focused HTTP/HTTPS tests support the shared SSRF protection or …
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing SSRF in untrusted URL downloads through the shared safe-fetch implementation.
Full details: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 7 files. (6 skipped: 6 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
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.

Server-Side Request Forgery in misc/scrape weblink endpoint

1 participant