Fix URL enconding in repodata.json (#3076)

* Fix filename URL encoding

* Remove dead code

* Fix Channel URL encoding

* Improve test_create caching

libmamba/src/core/repo.cpp

@@ -4,10 +4,7 @@
 //
 // The full license is in the file LICENSE, distributed with this software.
 
-#include <algorithm>
 #include <array>
-#include <fstream>
-#include <map>
 #include <string_view>
 #include <tuple>
 
@@ -30,6 +27,7 @@ extern "C"  // Incomplete header
 #include "mamba/core/repo.hpp"
 #include "mamba/core/util.hpp"
 #include "mamba/fs/filesystem.hpp"
+#include "mamba/specs/conda_url.hpp"
 #include "mamba/util/build.hpp"
 #include "mamba/util/string.hpp"
 #include "mamba/util/url_manip.hpp"
@@ -142,22 +140,20 @@ namespace mamba
             return util::lstrip_if_parts(tail, [&](char c) { return !is_sep(c); });
         }
 
-        [[nodiscard]] bool set_solvable(
+        [[nodiscard]] auto set_solvable(
             MPool& pool,
+            const std::string& repo_url_str,
+            const specs::CondaURL& repo_url,
             solv::ObjSolvableView solv,
-            std::string_view repo_url,
-            std::string_view filename,
-            const std::string& default_subdir,
+            const std::string& filename,
             const simdjson::dom::element& pkg,
-            std::string& tmp_buffer
-        )
+            const std::string& default_subdir
+        ) -> bool
         {
             // Not available from RepoDataPackage
-            tmp_buffer = filename;
-            solv.set_file_name(tmp_buffer);
-            tmp_buffer = repo_url;
-            solv.set_url(util::join_url(tmp_buffer, filename));
-            solv.set_channel(tmp_buffer);
+            solv.set_file_name(filename);
+            solv.set_url((repo_url / filename).str(specs::CondaURL::Credentials::Show));
+            solv.set_channel(repo_url_str);
 
             if (auto name = pkg["name"].get_string(); !name.error())
             {
@@ -307,16 +303,26 @@ namespace mamba
         void set_repo_solvables(
             MPool& pool,
             solv::ObjRepoView repo,
-            std::string_view repo_url,
+            const std::string& repo_url_str,
+            const specs::CondaURL& repo_url,
             const std::string& default_subdir,
-            const simdjson::dom::object& packages,
-            std::string& tmp_buffer
+            const simdjson::dom::object& packages
         )
         {
+            std::string filename = {};
             for (const auto& [fn, pkg] : packages)
             {
                 auto [id, solv] = repo.add_solvable();
-                const bool parsed = set_solvable(pool, solv, repo_url, fn, default_subdir, pkg, tmp_buffer);
+                filename = fn;
+                const bool parsed = set_solvable(
+                    pool,
+                    repo_url_str,
+                    repo_url,
+                    solv,
+                    filename,
+                    pkg,
+                    default_subdir
+                );
                 if (parsed)
                 {
                     LOG_DEBUG << "Adding package record to repo " << fn;
@@ -334,12 +340,14 @@ namespace mamba
             // WARNING cannot call ``url()`` at this point because it has not been internalized.
             // Setting the channel url on where the solvable so that we can retrace
             // where it came from
+            const auto url = specs::CondaURL::parse(repo_url);
             repo.for_each_solvable(
                 [&](solv::ObjSolvableView s)
                 {
                     // The solvable url, this is not set in libsolv parsing so we set it manually
                     // while we still rely on libsolv for parsing
-                    s.set_url(util::join_url(repo_url, s.file_name()));
+                    // TODO
+                    s.set_url((url / s.file_name()).str(specs::CondaURL::Credentials::Show));
                     // The name of the channel where it came from, may be different from repo name
                     // for instance with the installed repo
                     s.set_channel(repo_url);
@@ -472,18 +480,31 @@ namespace mamba
             default_subdir = std::string(subdir.value_unsafe());
         }
 
-        // An temporary buffer for managing null terminated strings
-        std::string tmp_buffer = {};
+        const auto repo_url = specs::CondaURL::parse(m_metadata.url);
 
         if (auto pkgs = repodata["packages"].get_object(); !pkgs.error())
         {
-            set_repo_solvables(m_pool, srepo(*this), m_metadata.url, default_subdir, pkgs.value(), tmp_buffer);
+            set_repo_solvables(
+                m_pool,
+                srepo(*this),
+                m_metadata.url,
+                repo_url,
+                default_subdir,
+                pkgs.value()
+            );
         }
 
         if (auto pkgs = repodata["packages.conda"].get_object();
             !pkgs.error() && !m_pool.context().use_only_tar_bz2)
         {
-            set_repo_solvables(m_pool, srepo(*this), m_metadata.url, default_subdir, pkgs.value(), tmp_buffer);
+            set_repo_solvables(
+                m_pool,
+                srepo(*this),
+                m_metadata.url,
+                repo_url,
+                default_subdir,
+                pkgs.value()
+            );
         }
     }
 

libmamba/src/specs/channel.cpp

@@ -43,7 +43,7 @@ namespace mamba::specs
     {
         auto p = m_url.clear_path();
         p = util::rstrip(p, '/');
-        m_url.set_path(std::move(p));
+        m_url.set_path(std::move(p), CondaURL::Encode::no);
     }
 
     auto Channel::is_package() const -> bool

libmamba/tests/src/specs/test_channel.cpp

@@ -731,5 +731,23 @@ TEST_SUITE("specs::channel")
             CHECK_EQ(chan.platforms(), platform_list());
             CHECK_EQ(chan.display_name(), "<unknown>");
         }
+
+        SUBCASE("https://conda.anaconda.org/conda-forge/linux-64/x264-1%21164.3095-h166bdaf_2.tar.bz2")
+        {
+            // Version 1!164.3095 is URL encoded
+            const auto url = "https://conda.anaconda.org/conda-forge/linux-64/x264-1%21164.3095-h166bdaf_2.tar.bz2"sv;
+            auto specs = ChannelSpec(std::string(url), {}, ChannelSpec::Type::PackageURL);
+
+            SUBCASE("Typical parameters")
+            {
+                auto params = make_typical_params();
+                auto channels = Channel::resolve(specs, params);
+                REQUIRE_EQ(channels.size(), 1);
+                const auto& chan = channels.front();
+                CHECK_EQ(chan.url(), CondaURL::parse(url));
+                CHECK_EQ(chan.platforms(), platform_list());  // Empty because package
+                CHECK_EQ(chan.display_name(), "conda-forge/linux-64/x264-1!164.3095-h166bdaf_2.tar.bz2");
+            }
+        }
     }
 }

micromamba/tests/test_create.py

@@ -5,7 +5,6 @@
 from pathlib import Path
 
 import pytest
-import requests
 import yaml
 
 from . import helpers
@@ -365,7 +364,8 @@ def test_multiple_yaml_specs_only_one_has_channels(tmp_home, tmp_root_prefix, tm
     assert res["specs"] == ["xtensor", "xsimd"]
 
 
-def test_multiprocessing():
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
+def test_multiprocessing(tmp_home, tmp_root_prefix):
     if platform.system() == "Windows":
         return
 
@@ -421,6 +421,7 @@ def test_create_base(tmp_home, tmp_root_prefix, already_exists, is_conda_env, ha
         assert (tmp_root_prefix / "conda-meta").exists()
 
 
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
 @pytest.mark.skipif(
     helpers.dry_run_tests is helpers.DryRun.ULTRA_DRY,
     reason="Running only ultra-dry tests",
@@ -673,6 +674,7 @@ def test_spec_with_channel_and_subdir():
         ) in e.stderr.decode()
 
 
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
 def test_spec_with_multichannel(tmp_home, tmp_root_prefix):
     "https://github.com/mamba-org/mamba/pull/2927"
     helpers.create("-n", "myenv", "defaults::zlib", "--dry-run")
@@ -834,6 +836,7 @@ def test_pyc_compilation(tmp_home, tmp_root_prefix, version, build, cache_tag):
     assert pyc_fn.name in six_meta
 
 
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
 def test_create_check_dirs(tmp_home, tmp_root_prefix):
     env_name = "myenv"
     env_prefix = tmp_root_prefix / "envs" / env_name
@@ -1009,6 +1012,7 @@ def copy_channels_osx():
                 f.write(repodata)
 
 
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
 def test_dummy_create(add_glibc_virtual_package, copy_channels_osx, tmp_home, tmp_root_prefix):
     env_name = "myenv"
 
@@ -1061,6 +1065,7 @@ def test_create_with_non_existing_subdir(tmp_home, tmp_root_prefix, tmp_path):
         helpers.create("-p", env_prefix, "--dry-run", "--json", "conda-forge/noarch::xtensor")
 
 
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
 def test_create_with_multiple_files(tmp_home, tmp_root_prefix, tmpdir):
     env_name = "myenv"
     tmp_root_prefix / "envs" / env_name
@@ -1088,7 +1093,7 @@ def test_create_with_multiple_files(tmp_home, tmp_root_prefix, tmpdir):
         no_rc=False,
     )
 
-    names = {x["name"] for x in res["actions"]["FETCH"]}
+    names = {x["name"] for x in res["actions"]["LINK"]}
     assert names == {"a", "b"}
 
 
@@ -1098,6 +1103,7 @@ def test_create_with_multiple_files(tmp_home, tmp_root_prefix, tmpdir):
 }
 
 
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
 def test_create_with_multi_channels(tmp_home, tmp_root_prefix, tmp_path):
     env_name = "myenv"
     tmp_root_prefix / "envs" / env_name
@@ -1116,12 +1122,11 @@ def test_create_with_multi_channels(tmp_home, tmp_root_prefix, tmp_path):
         no_rc=False,
     )
 
-    for pkg in res["actions"]["FETCH"]:
-        assert pkg["channel"].startswith("https://conda.anaconda.org/conda-forge/")
     for pkg in res["actions"]["LINK"]:
         assert pkg["url"].startswith("https://conda.anaconda.org/conda-forge/")
 
 
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
 def test_create_with_multi_channels_and_non_existing_subdir(tmp_home, tmp_root_prefix, tmp_path):
     env_name = "myenv"
     tmp_root_prefix / "envs" / env_name
@@ -1142,21 +1147,26 @@ def test_create_with_multi_channels_and_non_existing_subdir(tmp_home, tmp_root_p
         )
 
 
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
 def test_create_with_unicode(tmp_home, tmp_root_prefix):
     env_name = "320 áγђß家固êôōçñ한"
     env_prefix = tmp_root_prefix / "envs" / env_name
 
     res = helpers.create("-n", env_name, "--json", "xtensor", no_rc=False)
 
     assert res["actions"]["PREFIX"] == str(env_prefix)
-    assert any(pkg["name"] == "xtensor" for pkg in res["actions"]["FETCH"])
-    assert any(pkg["name"] == "xtl" for pkg in res["actions"]["FETCH"])
+    assert any(pkg["name"] == "xtensor" for pkg in res["actions"]["LINK"])
+    assert any(pkg["name"] == "xtl" for pkg in res["actions"]["LINK"])
 
 
-def download(url: str, out: Path):
-    response = requests.get(url)
-    if response.status_code == 200:
-        with open(out, "wb") as file:
-            file.write(response.content)
-    else:
-        raise Exception(f'Failed to download URL "{url}"')
+@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
+def test_create_package_with_non_url_char(tmp_home, tmp_root_prefix):
+    """Specific filename char are properly URL encoded.
+
+    Version with epoch such as `x264-1!164.3095-h166bdaf_2.tar.bz2` are not properly URL encoded.
+
+    https://github.com/mamba-org/mamba/issues/3072
+    """
+    res = helpers.create("-n", "myenv", "-c", "conda-forge", "x264>=1!0", "--json")
+
+    assert any(pkg["name"] == "x264" for pkg in res["actions"]["LINK"])