Normalize proxy env for Remote-SSH backend spawns#10087
Normalize proxy env for Remote-SSH backend spawns#10087QR-0W wants to merge 2 commits intoKilo-Org:mainfrom
Conversation
Remote-SSH extension hosts can expose only one proxy casing (for example http_proxy) to the VS Code extension process. The bundled CLI backend may then miss https_proxy/HTTPS_PROXY when it is spawned as `kilo serve`, causing HTTPS streaming requests to bypass or misread the configured proxy. Normalize proxy-related environment variables after merging process.env and VS Code http.proxy settings so the child process consistently receives the common lowercase and uppercase variants. Constraint: Preserve explicit empty values from http.proxySupport=off. Confidence: medium Scope-risk: narrow Tested: Added server-manager proxy env unit cases; locally ran equivalent JS behavior check Not-tested: bun test unavailable in the current container (bun command missing)
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| export function normalizeProxyEnv<T extends EnvMap>(env: T): T { | ||
| const next = { ...env } | ||
| const httpProxy = normalizeProxyUrl(next.http_proxy) ?? normalizeProxyUrl(next.HTTP_PROXY) | ||
| const httpsProxy = normalizeProxyUrl(next.https_proxy) ?? normalizeProxyUrl(next.HTTPS_PROXY) ?? httpProxy |
There was a problem hiding this comment.
WARNING: httpsProxy silently inherits httpProxy as its fallback — this means if a user has only set http_proxy (no HTTPS proxy at all), normalizeProxyEnv will write HTTPS_PROXY=<http_proxy_value> and https_proxy=<http_proxy_value> into the child env.
This is intentional per the PR description, but it can break setups where HTTP and HTTPS traffic must route differently (e.g. HTTPS goes direct while HTTP is proxied). It also silently overrides an explicit HTTPS_PROXY='' clear written by buildProxyEnv() — if proxySupport=off sets HTTPS_PROXY='' but http_proxy is non-empty in process.env, the fallback will reinstate a non-empty value.
Consider guarding the fallback: only derive httpsProxy from httpProxy when there is no existing HTTPS proxy key in the input at all (including empty-string ones):
const httpsProxy =
normalizeProxyUrl(next.https_proxy) ??
normalizeProxyUrl(next.HTTPS_PROXY) ??
(!("https_proxy" in env) && !("HTTPS_PROXY" in env) ? httpProxy : undefined)There was a problem hiding this comment.
Fixed in 20e613a90e58a7ac49b4f19a575f28aba965c5d4: HTTPS now derives from HTTP only when neither HTTPS_PROXY nor https_proxy exists in the final env. Empty HTTPS keys are treated as explicit clears.
| function normalizeProxyUrl(value: string | undefined): string | undefined { | ||
| const trimmed = value?.trim() | ||
| if (!trimmed) return undefined | ||
| return trimmed.replace(/\/$/, "") |
There was a problem hiding this comment.
SUGGESTION: normalizeProxyUrl only strips a single trailing slash. A URL like http://proxy:8080// (double slash) would pass through as http://proxy:8080/, still carrying a trailing slash. Replace replace(/\/$/, "") with replace(/\/+$/, "") to handle the degenerate case:
| return trimmed.replace(/\/$/, "") | |
| return trimmed.replace(/\/+$/, "") |
There was a problem hiding this comment.
Fixed in 20e613a90e58a7ac49b4f19a575f28aba965c5d4: normalizeProxyUrl now uses /\/+$/ to strip multiple trailing slashes.
| }) | ||
| }) | ||
|
|
||
| it("preserves explicit empty proxy clears", () => { |
There was a problem hiding this comment.
WARNING: The "preserves explicit empty proxy clears" test passes today but does not actually exercise the intended guard against the proxySupport=off + ambient http_proxy interaction described above (line 290 in server-manager.ts). The input has no non-empty http_proxy / HTTP_PROXY, so the fallback path is never reached. A dedicated test with { HTTP_PROXY: '', http_proxy: '', http_proxy_lower: 'http://ambient:3128' } (or similar) would expose the bug and verify the fix.
There was a problem hiding this comment.
Fixed in 20e613a90e58a7ac49b4f19a575f28aba965c5d4: added a regression case for ambient http_proxy plus explicitly cleared HTTPS_PROXY/https_proxy, which verifies the guard does not reintroduce HTTPS proxying.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Resolved Issues (from previous review)All 3 previously flagged issues were addressed in commit
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by claude-4.6-sonnet-20260217 · 445,909 tokens |
The Remote-SSH proxy normalization should fill in missing HTTPS proxy casing for ambient http_proxy, but it must not undo a user or VS Code setting that explicitly cleared HTTPS proxy variables. Treat the presence of an HTTPS proxy key as intent, even when the value is empty, and only derive HTTPS from HTTP when no HTTPS key exists. Constraint: VS Code http.proxySupport=off and empty proxy settings use empty env values as explicit clears. Rejected: Always deriving HTTPS from HTTP | would ignore explicit clears and surprise users who intentionally disabled HTTPS proxying. Confidence: high Scope-risk: narrow Directive: Do not derive HTTPS proxy from HTTP when HTTPS_PROXY or https_proxy exists unless its value is non-empty. Tested: Equivalent Node behavior check for lowercase-only Remote-SSH proxy, multi-slash stripping, uppercase mirroring, and explicit HTTPS clear. Not-tested: Official bun test suite; bun is not installed in this environment. Related: Kilo-Org#10087 Related: Kilo-Org#10086
|
Addressed the review feedback in
Validation in my environment:
|
|
Current status after the review-fix commit:
Could a maintainer with write access please approve the workflows and review when available? |
1 participant
Summary
Normalize proxy-related environment variables before spawning the bundled
kilo servebackend from the VS Code extension.Remote-SSH extension hosts can expose only one proxy casing/value (for example lowercase
http_proxy) toprocess.env. If the backend child only receiveshttp_proxy, HTTPS streaming clients may misshttps_proxy/HTTPS_PROXYand fail behind local/corporate proxies.This patch mirrors non-empty proxy values across common lowercase/uppercase variants after merging
process.envand VS Codehttp.proxysettings:http_proxy<->HTTP_PROXYhttps_proxy<->HTTPS_PROXY, deriving from HTTP proxy when HTTPS proxy is missingall_proxy<->ALL_PROXYno_proxy<->NO_PROXYIt also strips a trailing slash from proxy URLs to avoid
http://127.0.0.1:58119/vshttp://127.0.0.1:58119inconsistencies.Why
Fixes/addresses #10086.
Testing
tests/unit/server-manager-proxy-env.test.tsfor:http_proxyno_proxycasing mirroringNot run:
bun test tests/unit/server-manager-proxy-env.test.tsbecause the current container does not havebuninstalled (/bin/bash: bun: command not found).