fix: address code-review findings + bring in the variant-apply-failure test

Adversarial review (PR #105) surfaced real bugs in the new code; fix them and
make the swept-in WIP test pass reliably so it can land.

VEX / setup-state (property 7):
- configured_ecosystems detected Python via plan_python, which applies the
  --ecosystems filter — so `vex --ecosystems cargo` in a python-set-up repo
  reported python as NOT set up and dropped its patches. Detect python on-disk
  state directly (is_python_project + choose_python_manifests + deps_contain_hook),
  bypassing the filter like every other ecosystem in that probe.
- ecosystem_from_manual_name was missing maven/nuget/deno — the apply-only
  ecosystems that are the *primary* use of `setup.manual` — so declaring them
  manual silently dropped their patches from VEX. Add them (feature-gated) +
  a unit test covering every compiled-in ecosystem.

Launchers (supply-chain hardening):
- The RubyGems + Composer launchers followed redirects without enforcing HTTPS;
  a MitM could redirect github.com -> http:// and serve a malicious binary AND
  a matching SHA256SUMS (both attacker-controlled), defeating the checksum.
  Enforce HTTPS on every hop: Ruby refuses a non-HTTPS (initial/redirect) URL
  and resolves relative redirects with URI.join; PHP sets
  CURLOPT_PROTOCOLS/REDIR_PROTOCOLS=HTTPS and the no-curl fallback follows
  redirects manually, vetting each hop's scheme. Documented the (accepted,
  user-owned) cache-trust model at the cache-hit path.

Cargo recursive glob:
- collect_manifests_recursive (crates/**) followed symlinked dirs via metadata,
  so a loop symlink could re-add the workspace root as a duplicate member and an
  escaping symlink could make setup EDIT an out-of-tree Cargo.toml (breaking the
  in-repo-only contract). Skip symlinked dirs in the recursive walk (matches the
  `glob` crate); single-level crates/* still follows a symlinked member. +unix test.

variant-apply-failure test (was unrelated broken WIP, now fixed + included):
- Fixed the `purl` borrow-after-move, then rewrote it to invoke the binary as a
  subprocess and capture the child's stdout instead of an in-process
  `gag::BufferRedirect` (which races libtest's stdout capture and failed
  deterministically under default `cargo test`). No `gag` dep; passes reliably.

Verified: clippy --workspace --all-features -D warnings exits 0; the full
setup/vex/cargo suites + the variant test pass; ruby `gem build` + `ruby -c`
clean; PHP `php -l` clean (docker php:8.2).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: mikolalysenko

composer/socket-patch/bin/socket-patch

@@ -117,14 +117,27 @@ function sp_cache_dir()
     return $base . DIRECTORY_SEPARATOR . 'socket-patch' . DIRECTORY_SEPARATOR . 'bin';
 }
 
-/** Download `$url`; write to `$dest` if given, else return the body. */
+/**
+ * Download `$url`; write to `$dest` if given, else return the body. HTTPS is
+ * enforced including across redirects: GitHub release downloads redirect to a
+ * CDN (still HTTPS), but a redirect to http:// would let a network attacker
+ * serve a malicious binary AND a matching SHA256SUMS (both attacker-controlled),
+ * defeating the checksum check — so a non-HTTPS URL (initial or redirect target)
+ * is refused.
+ */
 function sp_http_get($url, $dest = null)
 {
+    if (stripos($url, 'https://') !== 0) {
+        sp_fail("refusing non-HTTPS URL: $url");
+    }
     if (function_exists('curl_init')) {
         $ch = curl_init($url);
         curl_setopt_array($ch, [
             CURLOPT_FOLLOWLOCATION => true,
             CURLOPT_MAXREDIRS => 10,
+            // Only fetch/redirect over HTTPS — curl refuses an http:// redirect.
+            CURLOPT_PROTOCOLS => CURLPROTO_HTTPS,
+            CURLOPT_REDIR_PROTOCOLS => CURLPROTO_HTTPS,
             CURLOPT_RETURNTRANSFER => true,
             CURLOPT_FAILONERROR => true,
             CURLOPT_USERAGENT => 'socket-patch-composer',
@@ -137,15 +150,10 @@ function sp_http_get($url, $dest = null)
         }
         curl_close($ch);
     } else {
-        $ctx = stream_context_create(['http' => [
-            'follow_location' => 1,
-            'max_redirects' => 10,
-            'user_agent' => 'socket-patch-composer',
-        ]]);
-        $data = @file_get_contents($url, false, $ctx);
-        if ($data === false) {
-            sp_fail("download failed for $url");
-        }
+        // No curl: follow redirects manually so each hop's scheme can be checked
+        // (PHP streams can't whitelist redirect protocols). TLS verification is
+        // on by default; assert it explicitly.
+        $data = sp_stream_get($url, 10);
     }
     if ($dest !== null) {
         file_put_contents($dest, $data);
@@ -154,6 +162,44 @@ function sp_http_get($url, $dest = null)
     return $data;
 }
 
+/** Manual, HTTPS-only redirect-following GET for the no-curl fallback. */
+function sp_stream_get($url, $redirects)
+{
+    if ($redirects < 0) {
+        sp_fail("too many redirects for $url");
+    }
+    if (stripos($url, 'https://') !== 0) {
+        sp_fail("refusing non-HTTPS URL: $url");
+    }
+    $ctx = stream_context_create([
+        'http' => [
+            'follow_location' => 0, // we follow manually to vet each hop
+            'ignore_errors' => true,
+            'user_agent' => 'socket-patch-composer',
+        ],
+        'ssl' => ['verify_peer' => true, 'verify_peer_name' => true],
+    ]);
+    $http_response_header = [];
+    $data = @file_get_contents($url, false, $ctx);
+    $status = 0;
+    $location = null;
+    foreach ($http_response_header as $h) {
+        if (preg_match('#^HTTP/\S+\s+(\d+)#', $h, $m)) {
+            $status = (int) $m[1];
+        } elseif (stripos($h, 'Location:') === 0) {
+            $location = trim(substr($h, strlen('Location:')));
+        }
+    }
+    if ($status >= 300 && $status < 400 && $location !== null) {
+        // GitHub uses absolute HTTPS redirect targets; reject anything else.
+        return sp_stream_get($location, $redirects - 1);
+    }
+    if ($data === false || $status >= 400) {
+        sp_fail("download failed ($status) for $url");
+    }
+    return $data;
+}
+
 /** SHA256SUMS lines are "<hex>  <filename>" (filename may be `*`-prefixed). */
 function sp_verify_sha256($path, $archive, $sums)
 {
@@ -219,6 +265,10 @@ function sp_resolve_binary()
     $exe = SP_BINARY . (PHP_OS_FAMILY === 'Windows' ? '.exe' : '');
     $cached = sp_cache_dir() . DIRECTORY_SEPARATOR . $ver
         . DIRECTORY_SEPARATOR . $target . DIRECTORY_SEPARATOR . $exe;
+    // Cache hit: verified when first downloaded, under the user's own cache dir.
+    // Trusted without re-verification (re-verifying needs a network fetch each
+    // run), matching npx / pip / rustup; an attacker able to write here can
+    // already replace the installed package or the binary itself.
     if (is_executable($cached)) {
         return $cached;
     }

crates/socket-patch-cli/src/commands/setup.rs

@@ -318,9 +318,13 @@ pub(crate) async fn configured_ecosystems(
     }
 
     // pypi: a chosen python manifest carries the `socket-patch[hook]` dep.
-    if let Some(plan) = plan_python(common).await {
-        for (path, _) in &plan.manifests {
-            if let Ok(content) = tokio::fs::read_to_string(path).await {
+    // Detect on-disk state DIRECTLY — not via `plan_python`, which applies the
+    // `--ecosystems` filter; this probe must report real state regardless of it
+    // (e.g. `vex --ecosystems cargo` must still see a set-up python project).
+    if is_python_project(&common.cwd).await {
+        let pm = detect_python_pm(&common.cwd).await;
+        for (path, _) in choose_python_manifests(&common.cwd, pm).await {
+            if let Ok(content) = tokio::fs::read_to_string(&path).await {
                 if deps_contain_hook(&content) {
                     set.insert(Ecosystem::Pypi);
                     break;

crates/socket-patch-cli/src/commands/vex.rs

@@ -262,6 +262,15 @@ fn ecosystem_from_manual_name(name: &str) -> Option<Ecosystem> {
         "golang" | "go" => Some(Ecosystem::Golang),
         #[cfg(feature = "composer")]
         "composer" | "php" => Some(Ecosystem::Composer),
+        // The apply-only ecosystems are the primary use of `manual` (hand-applied
+        // patches with no auto-install hook); they must map too, feature-gated to
+        // match the compiled-in `Ecosystem` variants `from_purl` can return.
+        #[cfg(feature = "maven")]
+        "maven" | "java" => Some(Ecosystem::Maven),
+        #[cfg(feature = "nuget")]
+        "nuget" | "dotnet" => Some(Ecosystem::Nuget),
+        #[cfg(feature = "deno")]
+        "deno" | "jsr" => Some(Ecosystem::Deno),
         _ => None,
     }
 }
@@ -568,6 +577,30 @@ mod tests {
     use super::*;
     use clap::Parser;
 
+    // Property 7: every ecosystem the build can classify a PURL for must also be
+    // declarable `manual`. Apply-only maven/nuget/deno are the *primary* use of
+    // `manual`; they were missing originally, silently dropping their patches.
+    #[test]
+    fn ecosystem_from_manual_name_maps_compiled_in_ecosystems() {
+        assert_eq!(ecosystem_from_manual_name("npm"), Some(Ecosystem::Npm));
+        assert_eq!(ecosystem_from_manual_name("PyPI"), Some(Ecosystem::Pypi)); // case-insensitive
+        assert_eq!(ecosystem_from_manual_name("python"), Some(Ecosystem::Pypi));
+        assert_eq!(ecosystem_from_manual_name("ruby"), Some(Ecosystem::Gem));
+        assert_eq!(ecosystem_from_manual_name("nonsense"), None);
+        #[cfg(feature = "cargo")]
+        assert_eq!(ecosystem_from_manual_name("cargo"), Some(Ecosystem::Cargo));
+        #[cfg(feature = "golang")]
+        assert_eq!(ecosystem_from_manual_name("go"), Some(Ecosystem::Golang));
+        #[cfg(feature = "composer")]
+        assert_eq!(ecosystem_from_manual_name("composer"), Some(Ecosystem::Composer));
+        #[cfg(feature = "maven")]
+        assert_eq!(ecosystem_from_manual_name("maven"), Some(Ecosystem::Maven));
+        #[cfg(feature = "nuget")]
+        assert_eq!(ecosystem_from_manual_name("nuget"), Some(Ecosystem::Nuget));
+        #[cfg(feature = "deno")]
+        assert_eq!(ecosystem_from_manual_name("deno"), Some(Ecosystem::Deno));
+    }
+
     #[derive(Parser)]
     struct Wrap {
         #[command(subcommand)]

crates/socket-patch-cli/tests/in_process_variant_apply_failure.rs

@@ -22,14 +22,16 @@
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
-use serial_test::serial;
 use sha2::{Digest, Sha256};
-use socket_patch_cli::commands::apply::{run as apply_run, ApplyArgs};
 
 const PYPI_PACKAGE: &str = "six";
 const PYPI_VERSION: &str = "1.16.0";
 const UUID: &str = "12121212-1212-4121-8121-121212121212";
 
+fn binary() -> PathBuf {
+    env!("CARGO_BIN_EXE_socket-patch").into()
+}
+
 fn git_sha256(content: &[u8]) -> String {
     let header = format!("blob {}\0", content.len());
     let mut hasher = Sha256::new();
@@ -108,9 +110,8 @@ fn install_six(tmp: &Path) -> PathBuf {
 /// blob does not hash to its declared `afterHash` (so apply fails) must
 /// produce exactly one `failed` event and NO `package_not_installed`
 /// `skipped` event for the same PURL.
-#[tokio::test]
-#[serial]
-async fn failed_installed_variant_is_not_also_reported_not_installed() {
+#[test]
+fn failed_installed_variant_is_not_also_reported_not_installed() {
     if find_python().is_none() {
         println!("SKIP: python3 not on PATH");
         return;
@@ -140,9 +141,12 @@ async fn failed_installed_variant_is_not_also_reported_not_installed() {
         .expect("write decoy blob");
 
     let purl = format!("pkg:pypi/{PYPI_PACKAGE}@{PYPI_VERSION}");
+    // `serde_json::json!` consumes the key expression, so clone for the key and
+    // keep `purl` itself for the assertions further down.
+    let manifest_key = purl.clone();
     let manifest = serde_json::json!({
         "patches": {
-            purl: {
+            manifest_key: {
                 "uuid": UUID,
                 "exportedAt": "2024-01-01T00:00:00Z",
                 "files": {
@@ -161,37 +165,33 @@ async fn failed_installed_variant_is_not_also_reported_not_installed() {
     )
     .expect("write manifest");
 
-    // Capture stdout (the JSON envelope) from the in-process run.
-    let apply_args = ApplyArgs {
-        common: socket_patch_cli::args::GlobalArgs {
-            cwd: tmp.path().to_path_buf(),
-            dry_run: false,
-            silent: false,
-            manifest_path: ".socket/manifest.json".to_string(),
-            offline: true,
-            global: false,
-            global_prefix: None,
-            ecosystems: Some(vec!["pypi".to_string()]),
-            json: true,
-            verbose: false,
-            download_mode: "diff".to_string(),
-            ..socket_patch_cli::args::GlobalArgs::default()
-        },
-        // NOT forced: exercises the variant-matches-installed path, which
-        // is exactly where the misreport happened.
-        force: false,
-        check: false,
-        vex: Default::default(),
-    };
-
-    let buf = gag::BufferRedirect::stdout().expect("redirect stdout");
-    let code = apply_run(apply_args).await;
-    let mut out = String::new();
-    {
-        use std::io::Read;
-        let mut buf = buf;
-        buf.read_to_string(&mut out).expect("read captured stdout");
-    }
+    // Run the real binary as a subprocess and capture its JSON envelope from the
+    // child's stdout. This is reliable under cargo's test-output capture, unlike
+    // an in-process `gag`-based stdout redirect (which races libtest's own
+    // capture). NOT `--force`: exercises the variant-matches-installed path,
+    // exactly where the misreport happened. SOCKET_* are scrubbed so the flags
+    // decide behaviour.
+    let output = Command::new(binary())
+        .args([
+            "apply",
+            "--offline",
+            "--ecosystems",
+            "pypi",
+            "--json",
+            "--cwd",
+            tmp.path().to_str().unwrap(),
+        ])
+        .env_remove("SOCKET_API_TOKEN")
+        .env_remove("SOCKET_OFFLINE")
+        .env_remove("SOCKET_ECOSYSTEMS")
+        .env_remove("SOCKET_JSON")
+        .env_remove("SOCKET_FORCE")
+        .env_remove("SOCKET_CWD")
+        .env_remove("SOCKET_MANIFEST_PATH")
+        .output()
+        .expect("run socket-patch apply");
+    let code = output.status.code().unwrap_or(-1);
+    let out = String::from_utf8_lossy(&output.stdout).to_string();
 
     // The apply failed, so the command exits non-zero (partial failure).
     assert_eq!(code, 1, "a failed apply must exit 1; stdout: {out}");

crates/socket-patch-core/src/cargo_setup/discover.rs

@@ -173,16 +173,26 @@ async fn collect_manifests_recursive(dir: &Path, depth: usize, out: &mut Vec<Pat
         Err(_) => return,
     };
     while let Ok(Some(entry)) = rd.next_entry().await {
-        let path = entry.path();
-        // Stat (not `file_type`) so symlinked dirs are followed, mirroring `glob_dir`.
-        if !fs::metadata(&path).await.map(|m| m.is_dir()).unwrap_or(false) {
+        // Use the dir-entry's own type (does NOT follow symlinks): a `**` walk
+        // must not traverse a symlinked dir — it could loop back to an ancestor
+        // (so the workspace root's own `Cargo.toml` reappears as a duplicate
+        // member) or escape the repo entirely (so `setup` would edit an
+        // out-of-tree `Cargo.toml`, breaking the in-repo-only contract). The
+        // `glob` crate's `**` likewise does not follow symlinks. (The
+        // single-level `glob_dir` still follows a symlinked direct member.)
+        let ft = match entry.file_type().await {
+            Ok(ft) => ft,
+            Err(_) => continue,
+        };
+        if ft.is_symlink() || !ft.is_dir() {
             continue;
         }
         let name = entry.file_name();
         let name = name.to_string_lossy();
         if name.starts_with('.') || name == "target" {
             continue;
         }
+        let path = entry.path();
         let manifest = path.join("Cargo.toml");
         if fs::metadata(&manifest).await.is_ok() {
             out.push(manifest);
@@ -295,6 +305,48 @@ mod tests {
         assert_eq!(proj.members.len(), 2, "exactly leaf + top: {:?}", proj.members);
     }
 
+    // A recursive `crates/**` glob must NOT follow symlinked directories: a
+    // loop symlink back to the root would re-add the workspace manifest as a
+    // duplicate member, and an escaping symlink would let `setup` edit an
+    // out-of-tree `Cargo.toml`. (Contrast the single-level `crates/*` case,
+    // which intentionally follows a symlinked direct member.)
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn test_recursive_glob_does_not_follow_symlinks() {
+        let dir = tempfile::tempdir().unwrap();
+        let root = dir.path();
+        write(
+            &root.join("Cargo.toml"),
+            "[workspace]\nmembers = [\"crates/**\"]\n",
+        )
+        .await;
+        write(
+            &root.join("crates/real/Cargo.toml"),
+            "[package]\nname=\"real\"\nversion=\"0.1.0\"\n",
+        )
+        .await;
+        fs::create_dir_all(root.join("crates")).await.unwrap();
+        // Loop: crates/loop -> the workspace root (would re-discover root Cargo.toml).
+        std::os::unix::fs::symlink(root, root.join("crates/loop")).unwrap();
+        // Escape: crates/escape -> an unrelated dir OUTSIDE the repo.
+        let outside = tempfile::tempdir().unwrap();
+        write(
+            &outside.path().join("Cargo.toml"),
+            "[package]\nname=\"outside\"\nversion=\"0.1.0\"\n",
+        )
+        .await;
+        std::os::unix::fs::symlink(outside.path(), root.join("crates/escape")).unwrap();
+
+        let proj = discover_cargo_project(root).await.unwrap();
+        assert_eq!(
+            proj.members,
+            vec![root.join("crates/real/Cargo.toml")],
+            "recursive `**` must find only the real nested member — never the root \
+             via the loop symlink, never the out-of-tree crate via the escape symlink; got {:?}",
+            proj.members
+        );
+    }
+
     #[tokio::test]
     async fn test_virtual_manifest_explicit_members() {
         let dir = tempfile::tempdir().unwrap();