Repository navigation
fix(runtimeConfig): filter localhost URLs from external service config - #108
danielheene wants to merge 1 commit into
Conversation
Localhost values in .env.test are format-valid placeholders — treating them as real URLs causes consumers (UmamiWidget, ServiceStatus) to attempt network connections to addresses that don't exist in test and E2E environments, filling logs with ECONNREFUSED noise. readExternalUrl() now returns undefined for any localhost/127.0.0.1/::1 URL, so those consumers skip their network calls without needing per-consumer guards.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| statusPageUrl: readEnv('STATUS_PAGE_URL'), | ||
| umamiUrl: readEnv('NEXT_PUBLIC_UMAMI_URL'), | ||
| statusPageUrl: readExternalUrl('STATUS_PAGE_URL'), | ||
| umamiUrl: readExternalUrl('NEXT_PUBLIC_UMAMI_URL'), |
There was a problem hiding this comment.
CRITICAL: umamiUrl can now be undefined, which makes new URL(path, base) throw in consumers
readExternalUrl now maps NEXT_PUBLIC_UMAMI_URL to undefined in E2E, but two consumers pass the value directly as the base of new URL(path, base) with no guard:
src/widgets/UmamiWidget/UmamiWidget.data.ts:119(buildApiUrl)src/lib/fetchers/fetchTrendingBlogPosts.ts:40(buildApiUrl)
new URL('/api/...', undefined) throws TypeError: Invalid URL because a relative URL requires a base. These calls run before fetcher's try/catch, so fetchWebsite, fetchStats, fetchEvents, fetchPaths, fetchPageViews and fetchTrendingBlogPosts now throw instead of returning null. UmamiWidget.tsx:12 and TrendingBlogPostsBlock/Renderer/Renderer.tsx:26 await them unguarded, so the admin Umami widget and any page using the trending-posts block fail to render in E2E — before this change they degraded gracefully to null (the ECONNREFUSED happened inside the caught fetcher). Guard the missing URL in these buildApiUrl helpers or in the fetch* functions.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (!value) return undefined | ||
| try { | ||
| const { hostname } = new URL(value) | ||
| if (hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '::1') return undefined |
There was a problem hiding this comment.
SUGGESTION: hostname === '::1' never matches — IPv6 loopback URLs are not filtered
The WHATWG URL API serializes IPv6 hosts with brackets: new URL('http://[::1]:3001').hostname is '[::1]', not '::1'. This check is therefore dead code and IPv6-loopback placeholder URLs pass through, even though the commit message claims ::1 is handled. Compare against '[::1]' as well.
| if (hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '::1') return undefined | |
| if (hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '[::1]') return undefined |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // Localhost URLs in env files are format-valid placeholders (e.g. .env.test). | ||
| // Treat them as unset so consumers don't attempt real network calls to them. | ||
| const readExternalUrl = (name: string): string | undefined => { |
There was a problem hiding this comment.
SUGGESTION: No regression test for the new localhost filtering
src/lib/runtimeConfig/index.test.ts already covers readRuntimeConfigFromEnv mapping, but nothing asserts the behavior this PR adds: a localhost/127.0.0.1 URL mapping to undefined while a real URL passes through. A case such as vi.stubEnv('NEXT_PUBLIC_UMAMI_URL', 'http://localhost:3002') with expect(readRuntimeConfigFromEnv().umamiUrl).toBeUndefined() would lock the filter in and matches the repo's regression-test convention for bug fixes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| serverUrl: readEnv('SERVER_URL'), | ||
| statusPageUrl: readEnv('STATUS_PAGE_URL'), | ||
| umamiUrl: readEnv('NEXT_PUBLIC_UMAMI_URL'), | ||
| statusPageUrl: readExternalUrl('STATUS_PAGE_URL'), |
There was a problem hiding this comment.
SUGGESTION: This does not silence the ServiceStatus heartbeat fetch described in the PR
The PR says this stops ServiceStatus from fetching from localhost:3001, but that fetch uses STATUS_PAGE_HEARTBEAT_URL (http://localhost:3001/api/push/test in .env.test), which is read directly from process.env in src/components/ServiceStatus/ServiceStatus.tsx:25 — not via getRuntimeConfig(). Only the Link href (which never made network calls) is filtered, so the E2E ECONNREFUSED noise from the heartbeat fetch continues. If silencing it is part of the goal, STATUS_PAGE_HEARTBEAT_URL needs the same filtering or a guard in fetchStatus.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Reviewed by free · Input: 65.2K · Output: 28.7K · Cached: 572K |
This branch was successfully deployed
1 participant
Summary
readExternalUrl()helper insrc/lib/runtimeConfig/index.tsthat returnsundefinedfor anylocalhost/127.0.0.1/::1URLumamiUrl,statusPageUrl, andiconifyApi— the fields whose.env.testvalues are placeholder-only addresses pointing at services not present in CI/E2EUmamiWidget.data.tsfrom attempting Umami API login, the/stats/[...path]proxy from trying to forward tolocalhost:3002, andServiceStatusfrom fetching fromlocalhost:3001, without swallowing errors in catch blocksTest plan
runtimeConfigunit tests pass (no changes needed — test values use non-localhost URLs):3002