fix(http): address second-round code review findings

Findings addressed:

- VP_INSECURE_TLS=0 / false / empty used to enable insecure TLS
  because the check was `var_os().is_some()`. Now requires a truthy
  value (1/true/yes/on, case-insensitive). Matches user mental model
  and avoids a security footgun.

- extract_pem_cert_blocks was greedy: an orphan BEGIN followed by a
  valid cert produced one bogus merged block and lost the valid one.
  Now detects an intervening BEGIN before the next END and skips past
  the orphan, recovering the legitimate cert.

- vite_error::Error::Reqwest and vite_js_runtime::Error::Reqwest were
  #[error(transparent)] — Display fell through to reqwest, which only
  shows the top-level message. The full source chain (TLS handshake →
  UnknownIssuer, hyper IO) was lost for every error propagated via
  `?` from `HttpClient::get` (vite_install path) and from the body-
  streaming loop in `download_file`. Both now use
  vite_shared::format_error_chain via #[error("{}", ...)] — From and
  source() semantics intact, so 404 detection via e.status() still
  works.

- build_client used std::process::exit(1) inside OnceLock::get_or_init
  from a tokio worker — skipped Drop, leaked lockfiles/tempfiles, and
  killed an embedding NAPI Node host from native code. Now caches
  Result<Client, String> in the OnceLock and panics with the chain
  message; subsequent callers see the same cached failure rather than
  re-running build_client. The error message uses format_error_chain
  rather than reqwest's top-level Display.

- Shared client had no timeout or connect_timeout — one stuck stream
  could block every concurrent download through the shared HTTP/2
  connection. Added 30s connect / 2 min request timeouts.

- format_error_chain now caps recursion depth at 16 (guards against
  cyclic source chains) and skips a source whose Display is already
  contained in the accumulated message (de-dupes when a parent
  thiserror variant inlines its `#[from]` source via `{0}`).

- CI sfw step: ${{ matrix.vp_bin }} quoted in shell context.

vite_error gains a vite_shared dep (no cycle — vite_shared does not
depend on vite_error). Tests added for is_env_truthy, the orphan-
BEGIN recovery path, and the format_error_chain depth/dedup behavior.

Author: fengmk2

.github/workflows/ci.yml

@@ -999,11 +999,11 @@ jobs:
           # Force the registry-fetch path: install a pinned pnpm globally so
           # vp downloads it (and therefore traverses sfw) rather than reusing
           # whatever's preinstalled on the runner.
-          sfw ${{ matrix.vp_bin }} i -g pnpm@9.15.0
+          sfw "${{ matrix.vp_bin }}" i -g pnpm@9.15.0
           # Then exercise `vp install` inside a real repo, also through sfw.
           git clone --depth 1 https://github.com/vitejs/vite.git "$RUNNER_TEMP/vite"
           cd "$RUNNER_TEMP/vite"
-          sfw ${{ matrix.vp_bin }} install --no-frozen-lockfile
+          sfw "${{ matrix.vp_bin }}" install --no-frozen-lockfile
 
   done:
     runs-on: ubuntu-latest

Cargo.lock

@@ -21,6 +21,7 @@ serde_yml = { workspace = true }
 thiserror = { workspace = true }
 tokio = { workspace = true }
 vite_path = { workspace = true }
+vite_shared = { workspace = true }
 vite_str = { workspace = true }
 vite_workspace = { workspace = true }
 wax = { workspace = true }

crates/vite_error/Cargo.toml

@@ -106,7 +106,12 @@ pub enum Error {
     #[error(transparent)]
     Semver(#[from] semver::Error),
 
-    #[error(transparent)]
+    // `#[error("{}", ...)]` not `transparent`: surface the full `source()`
+    // chain (TLS handshake → UnknownIssuer, hyper IO errors, etc.) instead of
+    // just reqwest's top-level "error sending request for url (...)" message.
+    // Keeps `From<reqwest::Error>` and `source()` semantics intact, so 404
+    // detection via `e.status()` at call sites still works.
+    #[error("{}", vite_shared::format_error_chain(.0))]
     Reqwest(#[from] reqwest::Error),
 
     #[error(transparent)]

crates/vite_error/src/lib.rs

@@ -58,8 +58,13 @@ pub enum Error {
     #[error(transparent)]
     Io(#[from] std::io::Error),
 
-    /// HTTP request error
-    #[error(transparent)]
+    /// HTTP request error.
+    ///
+    /// Surface the full `source()` chain (TLS handshake / connect / hyper
+    /// IO) rather than reqwest's top-level message only. Body-streaming
+    /// failures inside `download_file` propagate via `?` into this variant,
+    /// so the chain has to be exposed here — not at the call site.
+    #[error("{}", vite_shared::format_error_chain(.0))]
     Reqwest(#[from] reqwest::Error),
 
     /// Join error from tokio

crates/vite_js_runtime/src/error.rs

@@ -2,20 +2,42 @@
 
 use std::error::Error;
 
+/// Maximum chain depth `format_error_chain` will walk.
+///
+/// Guards against pathological / cyclic `source()` chains (rare but possible
+/// when an error type holds itself via `Box<dyn Error>` or `Arc`).
+const MAX_CHAIN_DEPTH: usize = 16;
+
 /// Format an error and its full `source()` chain as `top: cause: deeper-cause`.
 ///
 /// Use this when stringifying an error into a field of a higher-level error
 /// type — otherwise the Display impl of types like `reqwest::Error` only shows
 /// the top-level message, hiding the actual cause (TLS handshake failure,
 /// connection refused, etc.).
+///
+/// Behavior notes:
+/// - Walks at most [`MAX_CHAIN_DEPTH`] levels; further sources are summarized
+///   as `: ...`.
+/// - Skips a source whose Display is already contained in the accumulated
+///   message — avoids duplicates when a parent thiserror variant inlines its
+///   `#[from]` source via `{0}`.
 #[must_use]
 pub fn format_error_chain(err: &(dyn Error + 'static)) -> String {
     let mut out = err.to_string();
     let mut current = err.source();
+    let mut depth = 0_usize;
     while let Some(source) = current {
-        out.push_str(": ");
-        out.push_str(&source.to_string());
+        if depth >= MAX_CHAIN_DEPTH {
+            out.push_str(": ...");
+            break;
+        }
+        let part = source.to_string();
+        if !part.is_empty() && !out.contains(&part) {
+            out.push_str(": ");
+            out.push_str(&part);
+        }
         current = source.source();
+        depth += 1;
     }
     out
 }
@@ -28,13 +50,24 @@ mod tests {
 
     #[derive(Debug)]
     struct Layer {
-        msg: &'static str,
+        msg: String,
         cause: Option<Box<Layer>>,
     }
 
+    impl Layer {
+        fn new(msg: &str) -> Self {
+            Self { msg: msg.to_string(), cause: None }
+        }
+
+        fn with_cause(mut self, cause: Layer) -> Self {
+            self.cause = Some(Box::new(cause));
+            self
+        }
+    }
+
     impl fmt::Display for Layer {
         fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-            f.write_str(self.msg)
+            f.write_str(&self.msg)
         }
     }
 
@@ -46,19 +79,46 @@ mod tests {
 
     #[test]
     fn single_error_no_chain() {
-        let e = Layer { msg: "top", cause: None };
+        let e = Layer::new("top");
         assert_eq!(format_error_chain(&e), "top");
     }
 
     #[test]
     fn walks_full_chain() {
-        let e = Layer {
-            msg: "send request",
-            cause: Some(Box::new(Layer {
-                msg: "tls handshake",
-                cause: Some(Box::new(Layer { msg: "UnknownIssuer", cause: None })),
-            })),
-        };
+        let e = Layer::new("send request")
+            .with_cause(Layer::new("tls handshake").with_cause(Layer::new("UnknownIssuer")));
         assert_eq!(format_error_chain(&e), "send request: tls handshake: UnknownIssuer");
     }
+
+    #[test]
+    fn dedupes_source_already_in_parent() {
+        // thiserror's `#[error("Wrapped: {0}")]` style: parent already
+        // contains the inner message — don't print it twice.
+        let inner = Layer::new("TLS error: UnknownIssuer");
+        let outer = Layer::new("Wrapped: TLS error: UnknownIssuer").with_cause(inner);
+        assert_eq!(format_error_chain(&outer), "Wrapped: TLS error: UnknownIssuer");
+    }
+
+    #[test]
+    fn dedupes_partial_overlap() {
+        // The source message appears as a substring of the parent — skip it.
+        let parent = Layer::new("top: foo bar").with_cause(Layer::new("foo bar"));
+        assert_eq!(format_error_chain(&parent), "top: foo bar");
+    }
+
+    #[test]
+    fn caps_at_max_depth() {
+        let mut chain = Layer::new("leaf");
+        for i in 0..(MAX_CHAIN_DEPTH + 5) {
+            chain = Layer::new(&format!("level-{i}")).with_cause(chain);
+        }
+        let out = format_error_chain(&chain);
+        assert!(out.ends_with(": ..."), "expected truncation marker, got {out}");
+    }
+
+    #[test]
+    fn skips_empty_source_messages() {
+        let parent = Layer::new("top").with_cause(Layer::new(""));
+        assert_eq!(format_error_chain(&parent), "top");
+    }
 }

crates/vite_shared/src/error.rs

@@ -14,35 +14,58 @@
 //!   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.
+//! - `VP_INSECURE_TLS` — when set to a *truthy* value (`1`, `true`, `yes`,
+//!   `on`, case-insensitive), disables cert verification entirely. Diagnostic
+//!   escape hatch only; emits a loud stderr warning. Any other value
+//!   (including `0`, `false`, `no`, `off`, empty string) leaves verification
+//!   enabled.
+//!
+//! Note: env vars are read exactly once at the first HTTP call. In long-lived
+//! processes (e.g. the NAPI binding embedded in Node), later
+//! `process.env.SSL_CERT_FILE = ...` mutations do *not* re-configure the
+//! client.
 
-use std::{ffi::OsStr, path::Path, sync::OnceLock};
+use std::{ffi::OsStr, path::Path, sync::OnceLock, time::Duration};
 
-use crate::{env_vars, output};
+use crate::{env_vars, error::format_error_chain, output};
 
 const PEM_CERT_BEGIN: &[u8] = b"-----BEGIN CERTIFICATE-----";
 const PEM_CERT_END: &[u8] = b"-----END CERTIFICATE-----";
 
+/// Per-request total timeout. Long enough for slow tarball downloads on
+/// constrained CI runners, short enough that a single stuck stream doesn't
+/// silently hang a build.
+const REQUEST_TIMEOUT: Duration = Duration::from_mins(2);
+
+/// TCP connect timeout. Distinct from the request timeout above — without
+/// this, a black-holed proxy can stall every HTTP call for kernel-level
+/// retries (multiple minutes).
+const CONNECT_TIMEOUT: Duration = Duration::from_secs(30);
+
 /// 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.
+/// Panics on the *first* call if reqwest fails to build the client (malformed
+/// `HTTPS_PROXY`, unusable TLS backend, etc.); subsequent calls in the same
+/// process panic with the same message. Panic — not `process::exit` — so
+/// destructors of in-flight work still run (lockfiles released, tempfiles
+/// cleaned) and an embedding Node host (NAPI) keeps the process alive.
 #[must_use]
 pub fn shared_http_client() -> &'static reqwest::Client {
-    static CLIENT: OnceLock<reqwest::Client> = OnceLock::new();
-    CLIENT.get_or_init(build_client)
+    static CLIENT: OnceLock<Result<reqwest::Client, String>> = OnceLock::new();
+    match CLIENT.get_or_init(build_client) {
+        Ok(client) => client,
+        Err(msg) => panic!("failed to initialize HTTP client: {msg}"),
+    }
 }
 
-fn build_client() -> reqwest::Client {
+fn build_client() -> Result<reqwest::Client, String> {
     crate::ensure_tls_provider();
 
-    let mut builder = reqwest::Client::builder();
+    let mut builder =
+        reqwest::Client::builder().timeout(REQUEST_TIMEOUT).connect_timeout(CONNECT_TIMEOUT);
 
     for var in [env_vars::SSL_CERT_FILE, env_vars::NODE_EXTRA_CA_CERTS] {
         let Some(value) = std::env::var_os(var) else { continue };
@@ -87,27 +110,40 @@ fn build_client() -> reqwest::Client {
         tracing::debug!("added {added} extra root certs from {var}");
     }
 
-    if std::env::var_os(env_vars::VP_INSECURE_TLS).is_some() {
+    if is_env_truthy(env_vars::VP_INSECURE_TLS) {
         output::warn(
             "VP_INSECURE_TLS is set — TLS certificate verification is disabled. \
              Do not use this in production.",
         );
         builder = builder.danger_accept_invalid_certs(true);
     }
 
-    match builder.build() {
-        Ok(client) => client,
-        Err(err) => {
-            output::error(&vite_str::format!("failed to initialize HTTP client: {err}"));
-            std::process::exit(1);
-        }
-    }
+    builder.build().map_err(|err| format_error_chain(&err))
+}
+
+/// Returns `true` only for clearly affirmative env-var values
+/// (`1`, `true`, `yes`, `on`, case-insensitive).
+///
+/// Avoids the footgun where `VP_INSECURE_TLS=0` or `VP_INSECURE_TLS=false`
+/// is interpreted as "the variable is set, so feature on" — users naturally
+/// expect those values to *disable* the flag.
+fn is_env_truthy(var: &str) -> bool {
+    let Some(value) = std::env::var_os(var) else { return false };
+    let Some(s) = value.to_str() else { return false };
+    let trimmed = s.trim();
+    ["1", "true", "yes", "on"].iter().any(|v| trimmed.eq_ignore_ascii_case(v))
 }
 
 fn os_str_is_blank(value: &OsStr) -> bool {
     value.to_str().is_some_and(|s| s.trim().is_empty())
 }
 
+/// Extract `-----BEGIN CERTIFICATE-----`…`-----END CERTIFICATE-----` blocks
+/// from a PEM bundle, byte-window-based.
+///
+/// Handles a malformed bundle where a `BEGIN` is not followed by a matching
+/// `END` before the next `BEGIN` — that orphan is skipped (logged at debug)
+/// rather than greedily consuming the next certificate's body.
 fn extract_pem_cert_blocks(bundle: &[u8]) -> Vec<&[u8]> {
     let mut blocks = Vec::new();
     let mut cursor = 0_usize;
@@ -119,11 +155,28 @@ fn extract_pem_cert_blocks(bundle: &[u8]) -> Vec<&[u8]> {
         };
         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 {
+        let search_slice = &bundle[body_start..];
+        let next_end = search_slice.windows(PEM_CERT_END.len()).position(|w| w == PEM_CERT_END);
+        let Some(end_rel) = next_end else {
+            // No END marker at all: orphan BEGIN — stop scanning, nothing
+            // valid can follow.
+            tracing::debug!("PEM bundle: unterminated BEGIN CERTIFICATE at byte {start}");
             break;
         };
+        // If a *new* BEGIN appears before this END, the current BEGIN is
+        // orphaned. Skip past just this orphan and resume scanning at the
+        // intervening BEGIN — without this, both certs are lost.
+        let next_begin =
+            search_slice.windows(PEM_CERT_BEGIN.len()).position(|w| w == PEM_CERT_BEGIN);
+        if let Some(next_begin_rel) = next_begin
+            && next_begin_rel < end_rel
+        {
+            tracing::debug!(
+                "PEM bundle: orphan BEGIN CERTIFICATE at byte {start} (no END before next BEGIN); skipping"
+            );
+            cursor = body_start + next_begin_rel;
+            continue;
+        }
         let end = body_start + end_rel + PEM_CERT_END.len();
         blocks.push(&bundle[start..end]);
         cursor = end;
@@ -189,4 +242,47 @@ two\n\
         let bundle = b"-----BEGIN CERTIFICATE-----\nno end marker\n";
         assert!(extract_pem_cert_blocks(bundle).is_empty());
     }
+
+    #[test]
+    fn extract_blocks_recovers_after_orphan_begin() {
+        // Hand-concatenated bundle missing a newline + END marker between
+        // two certs: the orphan first BEGIN must not swallow the second
+        // cert's body. The valid second cert is recovered.
+        let bundle = b"\
+-----BEGIN CERTIFICATE-----\n\
+truncated, no END\n\
+-----BEGIN CERTIFICATE-----\n\
+valid\n\
+-----END CERTIFICATE-----\n";
+        let blocks = extract_pem_cert_blocks(bundle);
+        assert_eq!(blocks.len(), 1, "expected to recover the second cert");
+        let recovered = std::str::from_utf8(blocks[0]).unwrap();
+        assert!(recovered.contains("valid"));
+        assert!(recovered.starts_with("-----BEGIN CERTIFICATE-----"));
+        assert!(recovered.ends_with("-----END CERTIFICATE-----"));
+    }
+
+    #[test]
+    #[serial_test::serial(env)]
+    fn is_env_truthy_accepts_only_affirmative_values() {
+        // Use unique var names per case to avoid test-ordering interference
+        // when std::env is process-global.
+        for affirmative in ["1", "true", "TRUE", "True", "yes", "Yes", "on", "ON", " 1 "] {
+            // SAFETY: tests are run serially within this module for env vars.
+            unsafe {
+                std::env::set_var("VP_TEST_TRUTHY_VALUE", affirmative);
+            }
+            assert!(is_env_truthy("VP_TEST_TRUTHY_VALUE"), "should be truthy: {affirmative:?}");
+        }
+        for negative in ["0", "false", "FALSE", "no", "off", "", "  "] {
+            unsafe {
+                std::env::set_var("VP_TEST_TRUTHY_VALUE", negative);
+            }
+            assert!(!is_env_truthy("VP_TEST_TRUTHY_VALUE"), "should be falsy: {negative:?}");
+        }
+        unsafe {
+            std::env::remove_var("VP_TEST_TRUTHY_VALUE");
+        }
+        assert!(!is_env_truthy("VP_TEST_TRUTHY_VALUE"));
+    }
 }