fix(http): address code review findings on shared client

- Replace `.expect()` on Client::build() with `output::error` + exit(1).
  Pre-PR, build failure (malformed HTTPS_PROXY, TLS init error) returned
  Err and was propagated; the OnceLock wrapper turned it into a panic
  that would re-fire on every subsequent call. Now a clean error and
  exit instead of a stack trace.
- Surface CA-bundle read/parse failures via `output::warn` instead of
  `tracing::warn!`. tracing is silent unless VITE_LOG is set, hiding the
  misconfiguration from end users.
- Parse SSL_CERT_FILE / NODE_EXTRA_CA_CERTS block-by-block via
  `Certificate::from_pem`. reqwest's `from_pem_bundle` fails the whole
  bundle on the first non-cert PEM block (e.g. a private key in the
  same file), dropping every cert silently. Now per-block: bad blocks
  warn, good blocks are added.
- Use `std::env::var_os` so non-UTF-8 cert paths on Unix are honored.
- Skip whitespace-only env values.
- Enable reqwest's `system-proxy` feature so macOS System Settings and
  Windows registry proxies are honored, not just HTTPS_PROXY/HTTP_PROXY.
- Add `stream` and `json` reqwest features to vite_shared so the API
  owner declares them (feature unification still keeps consumers
  working when their crates redeclare).
- Add `error_for_status()?` to download_text so 4xx/5xx becomes an
  error instead of returning the error body as the "text".
- Document SSL_CERT_FILE's additive semantics (differs from OpenSSL).
- CI: move VP_INSECURE_TLS from job-level env to the single sfw step so
  unrelated build/setup steps don't run with cert verification off.
- CI: add `--remove-on-error` to the sfw curl so a failed download
  doesn't leave a 0-byte file that the next step tries to exec.

Author: fengmk2

.github/workflows/ci.yml

@@ -906,12 +906,6 @@ jobs:
             target: x86_64-pc-windows-msvc
             sfw_asset: sfw-free-windows-x86_64.exe
     runs-on: ${{ matrix.os }}
-    # TODO(SocketDev/sfw-free#30, SocketDev/sfw-free#43): drop `VP_INSECURE_TLS`
-    # once sfw ships the EKU fix. Until then the cert sfw issues is rejected by
-    # rustls/native-tls; the proxy + CA-injection plumbing is still exercised
-    # via HTTPS_PROXY + SSL_CERT_FILE, only certificate *validity* is skipped.
-    env:
-      VP_INSECURE_TLS: '1'
     steps:
       - uses: taiki-e/checkout-action@7d1e50e93dc4fb3bba58f85018fadf77898aee8b # v1.4.2
       - uses: ./.github/actions/clone
@@ -954,7 +948,10 @@ jobs:
         run: |
           set -euo pipefail
           mkdir -p "$RUNNER_TEMP/sfw-bin"
-          curl --fail --location --retry 3 --retry-delay 2 \
+          # `--remove-on-error` (curl 7.83+) prevents leaving a zero-byte file
+          # on failure so the next step's `sfw --version` fails clearly rather
+          # than with "exec format error" on an empty binary.
+          curl --fail --location --remove-on-error --retry 3 --retry-delay 2 \
             --output "$RUNNER_TEMP/sfw-bin/sfw${{ runner.os == 'Windows' && '.exe' || '' }}" \
             "https://github.com/SocketDev/sfw-free/releases/latest/download/${{ matrix.sfw_asset }}"
           if [[ "${{ runner.os }}" != "Windows" ]]; then
@@ -966,6 +963,14 @@ jobs:
         run: sfw --version
 
       - name: Run `sfw vp install` against a real repo
+        # TODO(SocketDev/sfw-free#30, SocketDev/sfw-free#43): drop `VP_INSECURE_TLS`
+        # once sfw ships the EKU fix. Until then the cert sfw issues is rejected
+        # by rustls/native-tls; the proxy + CA-injection plumbing is still
+        # exercised via HTTPS_PROXY + SSL_CERT_FILE, only certificate *validity*
+        # is skipped. Scoped to this step so unrelated build/setup steps in this
+        # job never see an HTTP client with cert verification disabled.
+        env:
+          VP_INSECURE_TLS: '1'
         run: |
           set -euo pipefail
           # Force the registry-fetch path: install a pinned pnpm globally so

Cargo.lock

@@ -117,7 +117,7 @@ pub async fn download_text(url: &str) -> Result<String, Error> {
 
     tracing::debug!("Downloading text from {url}");
 
-    let content = (|| async { client.get(url).send().await?.text().await })
+    let content = (|| async { client.get(url).send().await?.error_for_status()?.text().await })
         .retry(
             ExponentialBuilder::default()
                 .with_jitter()

crates/vite_js_runtime/src/download.rs

@@ -21,10 +21,10 @@ vite_str = { workspace = true }
 which = { workspace = true }
 
 [target.'cfg(target_os = "windows")'.dependencies]
-reqwest = { workspace = true, features = ["native-tls-vendored"] }
+reqwest = { workspace = true, features = ["native-tls-vendored", "stream", "json", "system-proxy"] }
 
 [target.'cfg(not(target_os = "windows"))'.dependencies]
-reqwest = { workspace = true, features = ["rustls-no-provider"] }
+reqwest = { workspace = true, features = ["rustls-no-provider", "stream", "json", "system-proxy"] }
 rustls = { workspace = true }
 
 [dev-dependencies]

crates/vite_shared/Cargo.toml

@@ -81,12 +81,19 @@ pub const VP_GLOBAL_VERSION: &str = "VP_GLOBAL_VERSION";
 /// Path to a PEM bundle of extra CA certificates to trust for HTTPS.
 ///
 /// Industry-standard env var also set by tools like Socket Firewall Free.
+///
+/// Note on semantics: vp treats this as **additive** to the system trust
+/// store (matches Node.js's `NODE_EXTRA_CA_CERTS`), not as a replacement.
+/// This differs from OpenSSL/curl/git, which use `SSL_CERT_FILE` as the
+/// *sole* trusted bundle. Users who want strict isolation should also
+/// restrict outbound traffic at the network layer.
 pub const SSL_CERT_FILE: &str = "SSL_CERT_FILE";
 
 /// Path to a PEM bundle of extra CA certificates to trust for HTTPS.
 ///
 /// Node.js convention; honored alongside `SSL_CERT_FILE` for setups that only
-/// configure the Node-flavored variable.
+/// configure the Node-flavored variable. Always additive to the system trust
+/// store.
 pub const NODE_EXTRA_CA_CERTS: &str = "NODE_EXTRA_CA_CERTS";
 
 /// Disable HTTPS certificate verification in vp's shared HTTP client.

crates/vite_shared/src/env_vars.rs

@@ -7,22 +7,32 @@
 //!
 //! Configuration sources (all read at first call):
 //! - `HTTPS_PROXY` / `HTTP_PROXY` / `NO_PROXY` — honored automatically by
-//!   reqwest; no explicit wiring needed.
-//! - `SSL_CERT_FILE`, `NODE_EXTRA_CA_CERTS` — each may point to a PEM bundle
-//!   (one or more concatenated certs). Every cert is added as a trusted root.
-//!   A read/parse failure logs a warning and is otherwise ignored so a
-//!   malformed env var never blocks startup.
+//!   reqwest. With the `system-proxy` feature enabled, macOS System Settings
+//!   proxies and Windows registry proxies are also picked up.
+//! - `SSL_CERT_FILE`, `NODE_EXTRA_CA_CERTS` — each may point to a PEM bundle.
+//!   Every `-----BEGIN CERTIFICATE-----` block is parsed independently and
+//!   added as an *additional* trusted root (system store is also kept —
+//!   unlike OpenSSL's `SSL_CERT_FILE` which replaces it). Per-block parse
+//!   failures emit a stderr warning and the remaining blocks are still added.
 //! - `VP_INSECURE_TLS` — when set to any value, disables cert verification
 //!   entirely. Diagnostic escape hatch only; emits a loud stderr warning.
 
-use std::sync::OnceLock;
+use std::{ffi::OsStr, path::Path, sync::OnceLock};
 
 use crate::{env_vars, output};
 
+const PEM_CERT_BEGIN: &[u8] = b"-----BEGIN CERTIFICATE-----";
+const PEM_CERT_END: &[u8] = b"-----END CERTIFICATE-----";
+
 /// Get the process-wide `reqwest::Client`.
 ///
 /// The client is built on first call and reused thereafter. See module docs
 /// for the env vars it honors.
+///
+/// If reqwest fails to build the client (e.g. malformed `HTTPS_PROXY`,
+/// unusable TLS backend) the process exits with a clean error message rather
+/// than panicking — the first HTTP call cannot proceed and there is nothing
+/// useful to fall back to.
 #[must_use]
 pub fn shared_http_client() -> &'static reqwest::Client {
     static CLIENT: OnceLock<reqwest::Client> = OnceLock::new();
@@ -35,25 +45,46 @@ fn build_client() -> reqwest::Client {
     let mut builder = reqwest::Client::builder();
 
     for var in [env_vars::SSL_CERT_FILE, env_vars::NODE_EXTRA_CA_CERTS] {
-        let Ok(path) = std::env::var(var) else { continue };
-        if path.is_empty() {
+        let Some(value) = std::env::var_os(var) else { continue };
+        if value.is_empty() || os_str_is_blank(&value) {
             continue;
         }
-        match std::fs::read(&path) {
-            Ok(bytes) => match reqwest::Certificate::from_pem_bundle(&bytes) {
-                Ok(certs) => {
-                    for cert in certs {
-                        builder = builder.add_root_certificate(cert);
-                    }
+        let path = Path::new(&value);
+        let bytes = match std::fs::read(path) {
+            Ok(bytes) => bytes,
+            Err(err) => {
+                output::warn(&vite_str::format!(
+                    "failed to read CA bundle from {var}={}: {err}",
+                    path.display()
+                ));
+                continue;
+            }
+        };
+        let blocks = extract_pem_cert_blocks(&bytes);
+        if blocks.is_empty() {
+            output::warn(&vite_str::format!(
+                "no PEM certificate blocks found in {var}={}",
+                path.display()
+            ));
+            continue;
+        }
+        let mut added = 0_usize;
+        for (idx, block) in blocks.iter().enumerate() {
+            match reqwest::Certificate::from_pem(block) {
+                Ok(cert) => {
+                    builder = builder.add_root_certificate(cert);
+                    added += 1;
                 }
                 Err(err) => {
-                    tracing::warn!("failed to parse extra CA bundle from {var}={path}: {err}");
+                    output::warn(&vite_str::format!(
+                        "failed to parse certificate #{} from {var}={}: {err}",
+                        idx + 1,
+                        path.display()
+                    ));
                 }
-            },
-            Err(err) => {
-                tracing::warn!("failed to read extra CA bundle from {var}={path}: {err}");
             }
         }
+        tracing::debug!("added {added} extra root certs from {var}");
     }
 
     if std::env::var_os(env_vars::VP_INSECURE_TLS).is_some() {
@@ -64,5 +95,98 @@ fn build_client() -> reqwest::Client {
         builder = builder.danger_accept_invalid_certs(true);
     }
 
-    builder.build().expect("failed to build shared reqwest client")
+    match builder.build() {
+        Ok(client) => client,
+        Err(err) => {
+            output::error(&vite_str::format!("failed to initialize HTTP client: {err}"));
+            std::process::exit(1);
+        }
+    }
+}
+
+fn os_str_is_blank(value: &OsStr) -> bool {
+    value.to_str().is_some_and(|s| s.trim().is_empty())
+}
+
+fn extract_pem_cert_blocks(bundle: &[u8]) -> Vec<&[u8]> {
+    let mut blocks = Vec::new();
+    let mut cursor = 0_usize;
+    while cursor < bundle.len() {
+        let Some(start_rel) =
+            bundle[cursor..].windows(PEM_CERT_BEGIN.len()).position(|w| w == PEM_CERT_BEGIN)
+        else {
+            break;
+        };
+        let start = cursor + start_rel;
+        let body_start = start + PEM_CERT_BEGIN.len();
+        let Some(end_rel) =
+            bundle[body_start..].windows(PEM_CERT_END.len()).position(|w| w == PEM_CERT_END)
+        else {
+            break;
+        };
+        let end = body_start + end_rel + PEM_CERT_END.len();
+        blocks.push(&bundle[start..end]);
+        cursor = end;
+    }
+    blocks
+}
+
+#[cfg(test)]
+mod tests {
+    use std::ffi::OsString;
+
+    use super::*;
+
+    const SAMPLE_CERT: &[u8] =
+        b"-----BEGIN CERTIFICATE-----\nMIIBkTCB+wIJAKHHIglt\n-----END CERTIFICATE-----";
+
+    #[test]
+    fn os_str_is_blank_matches_whitespace_only() {
+        assert!(os_str_is_blank(&OsString::from("")));
+        assert!(os_str_is_blank(&OsString::from("   ")));
+        assert!(os_str_is_blank(&OsString::from("\t\n")));
+        assert!(!os_str_is_blank(&OsString::from("/etc/ssl/cert.pem")));
+    }
+
+    #[test]
+    fn extract_blocks_finds_single_cert() {
+        let blocks = extract_pem_cert_blocks(SAMPLE_CERT);
+        assert_eq!(blocks.len(), 1);
+        assert_eq!(blocks[0], SAMPLE_CERT);
+    }
+
+    #[test]
+    fn extract_blocks_skips_non_cert_pem() {
+        let bundle = b"\
+-----BEGIN PRIVATE KEY-----\n\
+ignored\n\
+-----END PRIVATE KEY-----\n\
+-----BEGIN CERTIFICATE-----\n\
+keepme\n\
+-----END CERTIFICATE-----\n";
+        let blocks = extract_pem_cert_blocks(bundle);
+        assert_eq!(blocks.len(), 1);
+        assert!(blocks[0].starts_with(b"-----BEGIN CERTIFICATE-----"));
+        assert!(blocks[0].ends_with(b"-----END CERTIFICATE-----"));
+    }
+
+    #[test]
+    fn extract_blocks_finds_multiple_certs() {
+        let bundle = b"\
+-----BEGIN CERTIFICATE-----\n\
+one\n\
+-----END CERTIFICATE-----\n\
+junk in between\n\
+-----BEGIN CERTIFICATE-----\n\
+two\n\
+-----END CERTIFICATE-----\n";
+        let blocks = extract_pem_cert_blocks(bundle);
+        assert_eq!(blocks.len(), 2);
+    }
+
+    #[test]
+    fn extract_blocks_drops_unterminated_block() {
+        let bundle = b"-----BEGIN CERTIFICATE-----\nno end marker\n";
+        assert!(extract_pem_cert_blocks(bundle).is_empty());
+    }
 }