From 5e351343da8e8a585df6ebdbdb2d3f990bfc3198 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 29 Feb 2024 11:57:07 -0500 Subject: [PATCH] tweak the order of index priority (#2083) Previously, `uv` would always prioritize the index given by `--index-url`. It would then try any indexes after that given by zero or more `--extra-index-url` flags. This differed from `pip` in that any priority was given at all, where `pip` doesn't guarantee any priority ordering of indexes. We could go in the direction of mimicing `pip`'s behavior here, but it at present has issues with dependency confusion attacks where packages may get installed from indexes you don't control. More specifically, there is an issue of different trust levels. See discussion in #171 and [PEP-0708] for more on the security impact. In contrast, `uv` will only select versions for a package from a single index. That is, even if `foo` is in indexes `a` and `b`, it will only consider the versions from the index that it checks first. This probably helps with respect to dependency confusion attacks, but also means that `uv` doesn't quite cover all of the same use cases as `pip`. In this PR, we retain the notion of prioritizing indexes, but tweak it so that PyPI is preferred last as opposed to first. Or more precisely, the `--index-url` flag specifies a fallback index, not the primary index, and is deprioritized beneath every index specified by `--extra-index-url`. The ordering among indexes given by `--extra-index-url` remains the same: earlier indexes are prioritized over later indexes. While this tweak likely won't hit all use cases, I believe it will resolve some of the most common pain points without exacerbating dependency confusion problems. Ref #171, Fixes #1377, Fixes #1451, Fixes #1600 [PEP-0708]: https://peps.python.org/pep-0708/ --- crates/distribution-types/src/index_url.rs | 16 +++--- crates/uv-client/src/registry_client.rs | 5 +- crates/uv/src/main.rs | 60 ++++++++++++++++++++ crates/uv/tests/pip_compile.rs | 10 ++-- crates/uv/tests/pip_install.rs | 64 +++++++++++++++++++++- 5 files changed, 138 insertions(+), 17 deletions(-) diff --git a/crates/distribution-types/src/index_url.rs b/crates/distribution-types/src/index_url.rs index 169503dfa..4b206a576 100644 --- a/crates/distribution-types/src/index_url.rs +++ b/crates/distribution-types/src/index_url.rs @@ -288,12 +288,12 @@ impl Default for IndexUrls { } impl<'a> IndexUrls { - /// Return the primary [`IndexUrl`] entry. + /// Return the fallback [`IndexUrl`] entry. /// /// If `--no-index` is set, return `None`. /// /// If no index is provided, use the `PyPI` index. - pub fn index(&'a self) -> Option<&'a IndexUrl> { + fn index(&'a self) -> Option<&'a IndexUrl> { if self.no_index { None } else { @@ -305,7 +305,7 @@ impl<'a> IndexUrls { } /// Return an iterator over the extra [`IndexUrl`] entries. - pub fn extra_index(&'a self) -> impl Iterator + 'a { + fn extra_index(&'a self) -> impl Iterator + 'a { if self.no_index { Either::Left(std::iter::empty()) } else { @@ -314,13 +314,11 @@ impl<'a> IndexUrls { } /// Return an iterator over all [`IndexUrl`] entries. + /// + /// If `no_index` was enabled, then this always returns an empty + /// iterator. pub fn indexes(&'a self) -> impl Iterator + 'a { - self.index().into_iter().chain(self.extra_index()) - } - - /// Return `true` if no index is configured. - pub fn no_index(&self) -> bool { - self.no_index + self.extra_index().chain(self.index()) } } diff --git a/crates/uv-client/src/registry_client.rs b/crates/uv-client/src/registry_client.rs index 901bb90fc..be511b8e8 100644 --- a/crates/uv-client/src/registry_client.rs +++ b/crates/uv-client/src/registry_client.rs @@ -184,11 +184,12 @@ impl RegistryClient { &self, package_name: &PackageName, ) -> Result<(IndexUrl, OwnedArchive), Error> { - if self.index_urls.no_index() { + let mut it = self.index_urls.indexes().peekable(); + if it.peek().is_none() { return Err(ErrorKind::NoIndex(package_name.as_ref().to_string()).into()); } - for index in self.index_urls.indexes() { + for index in it { let result = self.simple_single_index(package_name, index).await?; return match result { diff --git a/crates/uv/src/main.rs b/crates/uv/src/main.rs index fbc87f588..1efbeb567 100644 --- a/crates/uv/src/main.rs +++ b/crates/uv/src/main.rs @@ -284,10 +284,25 @@ struct PipCompileArgs { refresh_package: Vec, /// The URL of the Python package index (by default: ). + /// + /// The index given by this flag is given lower priority than all other + /// indexes specified via the `--extra-index-url` flag. + /// + /// Unlike `pip`, `uv` will stop looking for versions of a package as soon + /// as it finds it in an index. That is, it isn't possible for `uv` to + /// consider versions of the same package across multiple indexes. #[clap(long, short, env = "UV_INDEX_URL")] index_url: Option, /// Extra URLs of package indexes to use, in addition to `--index-url`. + /// + /// All indexes given via this flag take priority over the index + /// in `--index-url` (which defaults to PyPI). And when multiple + /// `--extra-index-url` flags are given, earlier values take priority. + /// + /// Unlike `pip`, `uv` will stop looking for versions of a package as soon + /// as it finds it in an index. That is, it isn't possible for `uv` to + /// consider versions of the same package across multiple indexes. #[clap(long, env = "UV_EXTRA_INDEX_URL")] extra_index_url: Vec, @@ -421,10 +436,25 @@ struct PipSyncArgs { link_mode: install_wheel_rs::linker::LinkMode, /// The URL of the Python package index (by default: ). + /// + /// The index given by this flag is given lower priority than all other + /// indexes specified via the `--extra-index-url` flag. + /// + /// Unlike `pip`, `uv` will stop looking for versions of a package as soon + /// as it finds it in an index. That is, it isn't possible for `uv` to + /// consider versions of the same package across multiple indexes. #[clap(long, short, env = "UV_INDEX_URL")] index_url: Option, /// Extra URLs of package indexes to use, in addition to `--index-url`. + /// + /// All indexes given via this flag take priority over the index + /// in `--index-url` (which defaults to PyPI). And when multiple + /// `--extra-index-url` flags are given, earlier values take priority. + /// + /// Unlike `pip`, `uv` will stop looking for versions of a package as soon + /// as it finds it in an index. That is, it isn't possible for `uv` to + /// consider versions of the same package across multiple indexes. #[clap(long, env = "UV_EXTRA_INDEX_URL")] extra_index_url: Vec, @@ -619,10 +649,25 @@ struct PipInstallArgs { output_file: Option, /// The URL of the Python package index (by default: ). + /// + /// The index given by this flag is given lower priority than all other + /// indexes specified via the `--extra-index-url` flag. + /// + /// Unlike `pip`, `uv` will stop looking for versions of a package as soon + /// as it finds it in an index. That is, it isn't possible for `uv` to + /// consider versions of the same package across multiple indexes. #[clap(long, short, env = "UV_INDEX_URL")] index_url: Option, /// Extra URLs of package indexes to use, in addition to `--index-url`. + /// + /// All indexes given via this flag take priority over the index + /// in `--index-url` (which defaults to PyPI). And when multiple + /// `--extra-index-url` flags are given, earlier values take priority. + /// + /// Unlike `pip`, `uv` will stop looking for versions of a package as soon + /// as it finds it in an index. That is, it isn't possible for `uv` to + /// consider versions of the same package across multiple indexes. #[clap(long, env = "UV_EXTRA_INDEX_URL")] extra_index_url: Vec, @@ -892,10 +937,25 @@ struct VenvArgs { prompt: Option, /// The URL of the Python package index (by default: ). + /// + /// The index given by this flag is given lower priority than all other + /// indexes specified via the `--extra-index-url` flag. + /// + /// Unlike `pip`, `uv` will stop looking for versions of a package as soon + /// as it finds it in an index. That is, it isn't possible for `uv` to + /// consider versions of the same package across multiple indexes. #[clap(long, short, env = "UV_INDEX_URL")] index_url: Option, /// Extra URLs of package indexes to use, in addition to `--index-url`. + /// + /// All indexes given via this flag take priority over the index + /// in `--index-url` (which defaults to PyPI). And when multiple + /// `--extra-index-url` flags are given, earlier values take priority. + /// + /// Unlike `pip`, `uv` will stop looking for versions of a package as soon + /// as it finds it in an index. That is, it isn't possible for `uv` to + /// consider versions of the same package across multiple indexes. #[clap(long, env = "UV_EXTRA_INDEX_URL")] extra_index_url: Vec, diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 72d1fb9f4..0feecfafe 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -3339,15 +3339,17 @@ fn emit_index_urls() -> Result<()> { uv_snapshot!(context.compile() .arg("requirements.in") .arg("--emit-index-url") + .arg("--index-url") + .arg("https://test.pypi.org/simple/") .arg("--extra-index-url") - .arg("https://test.pypi.org/simple/"), @r###" + .arg("https://pypi.org/simple"), @r###" success: true exit_code: 0 ----- stdout ----- # This file was autogenerated by uv via the following command: - # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2023-11-18T12:00:00Z requirements.in --emit-index-url --extra-index-url https://test.pypi.org/simple/ - --index-url https://pypi.org/simple - --extra-index-url https://test.pypi.org/simple/ + # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2023-11-18T12:00:00Z requirements.in --emit-index-url --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple + --index-url https://test.pypi.org/simple/ + --extra-index-url https://pypi.org/simple black==23.10.1 click==8.1.7 diff --git a/crates/uv/tests/pip_install.rs b/crates/uv/tests/pip_install.rs index c18aca907..bb6f719e0 100644 --- a/crates/uv/tests/pip_install.rs +++ b/crates/uv/tests/pip_install.rs @@ -36,14 +36,24 @@ fn decode_token(content: &[&str]) -> String { /// Create a `pip install` command with options shared across scenarios. fn command(context: &TestContext) -> Command { + let mut command = command_without_exclude_newer(context); + command.arg("--exclude-newer").arg(EXCLUDE_NEWER); + command +} + +/// Create a `pip install` command with no `--exclude-newer` option. +/// +/// One should avoid using this in tests to the extent possible because +/// it can result in tests failing when the index state changes. Therefore, +/// if you use this, there should be some other kind of mitigation in place. +/// For example, pinning package versions. +fn command_without_exclude_newer(context: &TestContext) -> Command { let mut command = Command::new(get_bin()); command .arg("pip") .arg("install") .arg("--cache-dir") .arg(context.cache_dir.path()) - .arg("--exclude-newer") - .arg(EXCLUDE_NEWER) .env("VIRTUAL_ENV", context.venv.as_os_str()) .current_dir(&context.temp_dir); command @@ -802,6 +812,56 @@ fn install_no_index_version() { context.assert_command("import flask").failure(); } +/// Install a package via --extra-index-url. +/// +/// This is a regression test where previously `uv` would consult test.pypi.org +/// first, and if the package was found there, `uv` would not look at any other +/// indexes. We fixed this by flipping the priority order of indexes so that +/// test.pypi.org becomes the fallback (in this example) and the extra indexes +/// (regular PyPI) are checked first. +/// +/// (Neither approach matches `pip`'s behavior, which considers versions of +/// each package from all indexes. `uv` stops at the first index it finds a +/// package in.) +/// +/// Ref: +#[test] +fn install_extra_index_url_has_priority() { + let context = TestContext::new("3.12"); + + uv_snapshot!(command_without_exclude_newer(&context) + .arg("--index-url") + .arg("https://test.pypi.org/simple") + .arg("--extra-index-url") + .arg("https://pypi.org/simple") + // This tests what we want because BOTH of the following + // are true: `black` is on pypi.org and test.pypi.org, AND + // `black==24.2.0` is on pypi.org and NOT test.pypi.org. So + // this would previously check for `black` on test.pypi.org, + // find it, but then not find a compatible version. After + // the fix, `uv` will check pypi.org first since it is given + // priority via --extra-index-url. + .arg("black==24.2.0"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 6 packages in [TIME] + Downloaded 6 packages in [TIME] + Installed 6 packages in [TIME] + + black==24.2.0 + + click==8.1.7 + + mypy-extensions==1.0.0 + + packaging==23.2 + + pathspec==0.12.1 + + platformdirs==4.2.0 + "### + ); + + context.assert_command("import flask").failure(); +} + /// Install a package from a public GitHub repository #[test] #[cfg(feature = "git")]