node_runtime: Improve proxy mapping (#41807)

Closes #ISSUE

More accurately map localhost to `127.0.0.1`. Previously we would
lowercase and simply replace all instances of localhost inside of the
URL string, meaning query parameters, username, password etc. could not
contain the string `localhost` or contain uppercase letters without
getting modified. Added a test ensuring the mapping logic works. The
previous implementation would fail this new test.

Release Notes:

- Improved the behavior of mapping `localhost` to `127.0.0.1` when
passing configured proxy urls to `node`

crates/http_client/src/http_client.rs

@@ -16,7 +16,7 @@ use parking_lot::Mutex;
 #[cfg(feature = "test-support")]
 use std::fmt;
 use std::{any::type_name, sync::Arc};
-pub use url::Url;
+pub use url::{Host, Url};
 
 #[derive(Default, Debug, Clone, PartialEq, Eq, Hash)]
 pub enum RedirectPolicy {

crates/node_runtime/src/node_runtime.rs

@@ -2,7 +2,7 @@ use anyhow::{Context as _, Result, anyhow, bail};
 use async_compression::futures::bufread::GzipDecoder;
 use async_tar::Archive;
 use futures::{AsyncReadExt, FutureExt as _, channel::oneshot, future::Shared};
-use http_client::{HttpClient, Url};
+use http_client::{Host, HttpClient, Url};
 use log::Level;
 use semver::Version;
 use serde::Deserialize;
@@ -13,6 +13,7 @@ use std::{
     env::{self, consts},
     ffi::OsString,
     io,
+    net::{IpAddr, Ipv4Addr},
     path::{Path, PathBuf},
     process::Output,
     sync::Arc,
@@ -799,17 +800,18 @@ fn configure_npm_command(
         command.args(["--prefix".into(), directory.to_path_buf()]);
     }
 
-    if let Some(proxy) = proxy {
+    if let Some(mut proxy) = proxy.cloned() {
         // Map proxy settings from `http://localhost:10809` to `http://127.0.0.1:10809`
         // NodeRuntime without environment information can not parse `localhost`
         // correctly.
         // TODO: map to `[::1]` if we are using ipv6
-        let proxy = proxy
-            .to_string()
-            .to_ascii_lowercase()
-            .replace("localhost", "127.0.0.1");
+        if matches!(proxy.host(), Some(Host::Domain(domain)) if domain.eq_ignore_ascii_case("localhost"))
+        {
+            // When localhost is a valid Host, so is `127.0.0.1`
+            let _ = proxy.set_ip_host(IpAddr::V4(Ipv4Addr::LOCALHOST));
+        }
 
-        command.args(["--proxy", &proxy]);
+        command.args(["--proxy", proxy.as_str()]);
     }
 
     #[cfg(windows)]
@@ -830,3 +832,46 @@ fn configure_npm_command(
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use http_client::Url;
+
+    use super::configure_npm_command;
+
+    // Map localhost to 127.0.0.1
+    // NodeRuntime without environment information can not parse `localhost` correctly.
+    #[test]
+    fn test_configure_npm_command_map_localhost_proxy() {
+        const CASES: [(&str, &str); 4] = [
+            // Map localhost to 127.0.0.1
+            ("http://localhost:9090/", "http://127.0.0.1:9090/"),
+            ("https://google.com/", "https://google.com/"),
+            (
+                "http://username:password@proxy.thing.com:8080/",
+                "http://username:password@proxy.thing.com:8080/",
+            ),
+            // Test when localhost is contained within a different part of the URL
+            (
+                "http://username:localhost@localhost:8080/",
+                "http://username:localhost@127.0.0.1:8080/",
+            ),
+        ];
+
+        for (proxy, mapped_proxy) in CASES {
+            let mut dummy = smol::process::Command::new("");
+            let proxy = Url::parse(proxy).unwrap();
+            configure_npm_command(&mut dummy, None, Some(&proxy));
+            let proxy = dummy
+                .get_args()
+                .skip_while(|&arg| arg != "--proxy")
+                .skip(1)
+                .next();
+            let proxy = proxy.expect("Proxy was not passed to Command correctly");
+            assert_eq!(
+                proxy, mapped_proxy,
+                "Incorrectly mapped localhost to 127.0.0.1"
+            );
+        }
+    }
+}