feat: implement fetchRemoteAsset function for secure asset retrieval with DNS checks and size limits #183
Conversation
…with DNS checks and size limits
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code graph analysis (3)server/services/favicon.test.ts (2)
server/services/seo.ts (5)
server/services/favicon.ts (3)
🔇 Additional comments (15)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
==========================================
- Coverage 69.09% 68.25% -0.85%
==========================================
Files 107 108 +1
Lines 3042 3122 +80
Branches 922 941 +19
==========================================
+ Hits 2102 2131 +29
- Misses 580 626 +46
- Partials 360 365 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/fetch.ts (2)
1-33: Consider combining external and internal AbortSignals for caller cancellationThe function currently creates its own
AbortControllerand ignores anyinit.signal, preventing callers from cancelling in-flight retries. The modern WHATWG pattern is to combine an external signal with an internal timeout signal usingAbortSignal.any([externalSignal, AbortSignal.timeout(timeoutMs)]). This allows both the internal timeout and external cancellation to work correctly while preserving distinct abort reasons. Available in Node 20+ and modern browsers; for older runtimes, a small polyfill combining signals via listeners is widely used. This is an optional improvement—the current behavior is acceptable if external cancellation isn't needed.
35-68: Update comment to clarify strict hostname-equality behavior, or implement true registrable-domain logicThe mismatch is confirmed. Tests verify the function blocks different domains but do not cover subdomain scenarios (e.g.,
example.com→blog.example.com), which your code would reject despite the comment's "registrable domain" wording.The code enforces strict hostname equality post-www-normalization (line 58:
if (fromHost !== toHost) return false;), not the registrable-domain (eTLD+1) semantics referenced in the comment. Web security standards treat the registrable domain (eTLD+1) as the relevant unit for same-site decisions—meaningblog.example.comandexample.comare the same site, but your implementation would block such redirects.Either:
- Change line 57 comment from "registrable domain" to "hostname" to reflect the stricter actual behavior, or
- Compute true registrable domains (e.g., using a Public Suffix List library) to match the comment's promise.
server/services/favicon.ts (1)
60-133: Prevent 200 responses from settingnotFoundpermanentlyAs soon as
fetchRemoteAssetsucceeds we should consider this source “found”, even if conversion/storage fails later. TodayallNotFoundonly flips tofalseon caught errors; when conversion returnsnullwecontinuewithallNotFoundstilltrue, and after exhausting sources we persistnotFound: true. That permanently short-circuits future favicon fetches for domains that served an image but we couldn’t process (e.g., unsupported format). Please setallNotFound = falseimmediately after the successful fetch so we only marknotFoundwhen every source really returned a 404.const asset = await fetchRemoteAsset({ url: src, headers: { Accept: "image/avif,image/webp,image/png,image/*;q=0.9,*/*;q=0.8", "User-Agent": USER_AGENT, }, maxBytes: MAX_FAVICON_BYTES, timeoutMs: REQUEST_TIMEOUT_MS, maxRedirects: 2, allowHttp: src.startsWith("http://"), }); + allNotFound = false; const buf = asset.buffer;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
lib/fetch-remote-asset.test.ts(1 hunks)lib/fetch-remote-asset.ts(1 hunks)lib/fetch.test.ts(4 hunks)lib/fetch.ts(3 hunks)server/services/dns.ts(2 hunks)server/services/favicon.test.ts(4 hunks)server/services/favicon.ts(4 hunks)server/services/seo.test.ts(6 hunks)server/services/seo.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
lib/fetch-remote-asset.test.ts (1)
lib/fetch-remote-asset.ts (2)
fetchRemoteAsset(64-147)RemoteAssetError(25-34)
lib/fetch.test.ts (1)
lib/fetch.ts (1)
fetchWithTimeoutAndRetry(5-33)
server/services/dns.ts (1)
lib/fetch.ts (1)
fetchWithTimeoutAndRetry(5-33)
server/services/seo.test.ts (1)
server/services/seo.ts (1)
getSeo(31-288)
server/services/seo.ts (5)
lib/fetch.ts (1)
fetchWithTimeoutAndRetry(5-33)lib/fetch-remote-asset.ts (1)
fetchRemoteAsset(64-147)lib/constants/app.ts (1)
USER_AGENT(5-7)lib/image.ts (1)
optimizeImageCover(70-79)lib/storage.ts (1)
storeImage(160-208)
server/services/favicon.test.ts (2)
lib/fetch-remote-asset.ts (1)
RemoteAssetError(25-34)server/services/favicon.ts (1)
getOrCreateFaviconBlobUrl(172-182)
server/services/favicon.ts (2)
lib/fetch-remote-asset.ts (2)
fetchRemoteAsset(64-147)RemoteAssetError(25-34)lib/image.ts (1)
convertBufferToImageCover(15-68)
🔇 Additional comments (4)
lib/fetch-remote-asset.test.ts (1)
8-24: Deterministic test harness for fetch + DNS looks solidUse of
vi.hoistedplusbeforeEachto resetfetchMockanddnsLookupMockmakes these tests deterministic and isolates DNS behavior from the network. The private IP test correctly asserts that no fetch occurs when DNS resolves to a blocked address.Also applies to: 64-72
lib/fetch.test.ts (1)
44-57: Tests accurately exercise timeout and retry behaviorThe updated tests for
fetchWithTimeoutAndRetrycover success-before-timeout, abort-on-timeout, and a retry-with-backoff success path, matching the helper’s semantics and guarding against future regressions in the retry loop.Also applies to: 65-82, 85-107
server/services/favicon.test.ts (1)
11-12: fetchRemoteAsset mocking pattern is clean and preserves real exportsUsing a hoisted
fetchRemoteAssetMockand a partial module mock that spreadsimportActualkeeps types likeRemoteAssetErrorreal while swapping out just the network call, which is ideal for these tests. This isolates favicon behavior from network variability without sacrificing type correctness.Also applies to: 39-58
lib/fetch-remote-asset.ts (1)
194-251: SSRF/IP range checks confirmed effectiveVerification confirms ipaddr.js
range()correctly classifies only globally-routable public addresses as"unicast"while reserving all non-public ranges (loopback, private, link-local, ULA, etc.) as non-"unicast". TheisBlockedIpcheck effectively blocks all SSRF vectors for both IPv4 and IPv6.
| it("returns buffer and content type for valid asset", async () => { | ||
| const body = new Uint8Array([1, 2, 3]); | ||
| fetchMock.mockResolvedValueOnce( | ||
| new Response(body, { | ||
| status: 200, | ||
| headers: { "content-type": "image/png" }, | ||
| }), | ||
| ); | ||
|
|
||
| const result = await fetchRemoteAsset({ | ||
| url: "https://example.com/image.png", | ||
| maxBytes: 1024, | ||
| }); | ||
|
|
||
| expect(Buffer.isBuffer(result.buffer)).toBe(true); | ||
| expect(result.contentType).toBe("image/png"); | ||
| expect(result.finalUrl).toBe("https://example.com/image.png"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding tests for remaining RemoteAssetError cases
Happy-path, protocol gating, redirect, private IP, and size_exceeded behaviors are well covered. To lock in the error surface, consider adding tests for invalid_url, host_not_allowed, host_blocked, dns_error, and redirect_limit so regressions on those branches are caught early.
Also applies to: 45-52, 74-97, 99-116
🤖 Prompt for AI Agents
In lib/fetch-remote-asset.test.ts around lines 26 to 43 (and similarly for
ranges 45-52, 74-97, 99-116), the test suite covers successful and several error
paths but omits unit tests asserting the RemoteAssetError variants; add focused
tests that exercise and assert each error type: invalid_url (pass malformed URL
and expect RemoteAssetError with code "invalid_url"),
host_not_allowed/host_blocked (set allowed/blocked host config and expect
respective error codes), dns_error (mock fetch or DNS resolution to throw a
network/DNS error and assert "dns_error"), and redirect_limit (mock fetch to
return a chain of redirects exceeding the redirect limit and assert
"redirect_limit"). For each test, use fetchMock to simulate responses/errors,
set maxBytes/allowed hosts/redirect settings as needed, and assert the thrown
error.code and any relevant properties (e.g., finalUrl) to lock the error
surface.
…name validation and error reporting
…ing relative URLs
…ngle failure simulation in favicon tests
…ng and enhance redirect validation
…ly for favicon retrieval
Summary by CodeRabbit
New Features
Bug Fixes
Tests