Propagate URL errors in verbatim parsing (#3720)

## Summary

Closes https://github.com/astral-sh/uv/issues/3715.

## Test Plan

```
❯ echo "/../test" | cargo run pip compile -
error: Couldn't parse requirement in `-` at position 0
  Caused by: path could not be normalized: /../test
/../test
^^^^^^^^

❯ echo "-e /../test" | cargo run pip compile -
error: Invalid URL in `-`: `/../test`
  Caused by: path could not be normalized: /../test
  Caused by: cannot normalize a relative path beyond the base directory
```
This commit is contained in:
Charlie Marsh 2024-05-21 15:58:59 -04:00 committed by GitHub
parent 19df1a4372
commit 558f628ef1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 97 additions and 40 deletions

View file

@ -236,6 +236,12 @@ fn preprocess_unnamed_url(
#[cfg(feature = "non-pep508-extensions")]
if let Some(working_dir) = working_dir {
let url = VerbatimUrl::parse_path(path.as_ref(), working_dir)
.map_err(|err| Pep508Error {
message: Pep508ErrorSource::<VerbatimUrl>::UrlError(err),
start,
len,
input: cursor.to_string(),
})?
.with_given(url.to_string());
return Ok((url, extras));
}
@ -270,6 +276,12 @@ fn preprocess_unnamed_url(
_ => {
if let Some(working_dir) = working_dir {
let url = VerbatimUrl::parse_path(expanded.as_ref(), working_dir)
.map_err(|err| Pep508Error {
message: Pep508ErrorSource::<VerbatimUrl>::UrlError(err),
start,
len,
input: cursor.to_string(),
})?
.with_given(url.to_string());
return Ok((url, extras));
}
@ -288,8 +300,14 @@ fn preprocess_unnamed_url(
} else {
// Ex) `../editable/`
if let Some(working_dir) = working_dir {
let url =
VerbatimUrl::parse_path(expanded.as_ref(), working_dir).with_given(url.to_string());
let url = VerbatimUrl::parse_path(expanded.as_ref(), working_dir)
.map_err(|err| Pep508Error {
message: Pep508ErrorSource::<VerbatimUrl>::UrlError(err),
start,
len,
input: cursor.to_string(),
})?
.with_given(url.to_string());
return Ok((url, extras));
}

View file

@ -38,25 +38,26 @@ impl VerbatimUrl {
/// Create a [`VerbatimUrl`] from a file path.
///
/// Assumes that the path is absolute.
pub fn from_path(path: impl AsRef<Path>) -> Self {
pub fn from_path(path: impl AsRef<Path>) -> Result<Self, VerbatimUrlError> {
let path = path.as_ref();
// Normalize the path.
let path = normalize_path(path).expect("path is absolute");
let path = normalize_path(path)
.map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;
// Extract the fragment, if it exists.
let (path, fragment) = split_fragment(&path);
// Convert to a URL.
let mut url = Url::from_file_path(path.clone())
.unwrap_or_else(|_| panic!("path is absolute: {}", path.display()));
.map_err(|_| VerbatimUrlError::UrlConversion(path.to_path_buf()))?;
// Set the fragment, if it exists.
if let Some(fragment) = fragment {
url.set_fragment(Some(fragment));
}
Self { url, given: None }
Ok(Self { url, given: None })
}
/// Parse a URL from a string, expanding any environment variables.
@ -67,7 +68,10 @@ impl VerbatimUrl {
/// Parse a URL from an absolute or relative path.
#[cfg(feature = "non-pep508-extensions")] // PEP 508 arguably only allows absolute file URLs.
pub fn parse_path(path: impl AsRef<Path>, working_dir: impl AsRef<Path>) -> Self {
pub fn parse_path(
path: impl AsRef<Path>,
working_dir: impl AsRef<Path>,
) -> Result<Self, VerbatimUrlError> {
let path = path.as_ref();
// Convert the path to an absolute path, if necessary.
@ -78,26 +82,22 @@ impl VerbatimUrl {
};
// Normalize the path.
let path = normalize_path(&path).expect("path is absolute");
let path = normalize_path(&path)
.map_err(|err| VerbatimUrlError::Normalization(path.to_path_buf(), err))?;
// Extract the fragment, if it exists.
let (path, fragment) = split_fragment(&path);
// Convert to a URL.
let mut url = Url::from_file_path(path.clone()).unwrap_or_else(|_| {
panic!(
"path is absolute: {}, {}",
path.display(),
working_dir.as_ref().display()
)
});
let mut url = Url::from_file_path(path.clone())
.map_err(|_| VerbatimUrlError::UrlConversion(path.to_path_buf()))?;
// Set the fragment, if it exists.
if let Some(fragment) = fragment {
url.set_fragment(Some(fragment));
}
Self { url, given: None }
Ok(Self { url, given: None })
}
/// Parse a URL from an absolute path.
@ -108,12 +108,12 @@ impl VerbatimUrl {
let path = if path.is_absolute() {
path.to_path_buf()
} else {
return Err(VerbatimUrlError::RelativePath(path.to_path_buf()));
return Err(VerbatimUrlError::WorkingDirectory(path.to_path_buf()));
};
// Normalize the path.
let Ok(path) = normalize_path(&path) else {
return Err(VerbatimUrlError::RelativePath(path));
return Err(VerbatimUrlError::WorkingDirectory(path));
};
// Extract the fragment, if it exists.
@ -226,7 +226,7 @@ impl Pep508Url for VerbatimUrl {
#[cfg(feature = "non-pep508-extensions")]
if let Some(working_dir) = working_dir {
return Ok(VerbatimUrl::parse_path(path.as_ref(), working_dir)
return Ok(VerbatimUrl::parse_path(path.as_ref(), working_dir)?
.with_given(url.to_string()));
}
@ -245,7 +245,7 @@ impl Pep508Url for VerbatimUrl {
_ => {
#[cfg(feature = "non-pep508-extensions")]
if let Some(working_dir) = working_dir {
return Ok(VerbatimUrl::parse_path(expanded.as_ref(), working_dir)
return Ok(VerbatimUrl::parse_path(expanded.as_ref(), working_dir)?
.with_given(url.to_string()));
}
@ -257,7 +257,7 @@ impl Pep508Url for VerbatimUrl {
// Ex) `../editable/`
#[cfg(feature = "non-pep508-extensions")]
if let Some(working_dir) = working_dir {
return Ok(VerbatimUrl::parse_path(expanded.as_ref(), working_dir)
return Ok(VerbatimUrl::parse_path(expanded.as_ref(), working_dir)?
.with_given(url.to_string()));
}
@ -275,7 +275,15 @@ pub enum VerbatimUrlError {
/// Received a relative path, but no working directory was provided.
#[error("relative path without a working directory: {0}")]
RelativePath(PathBuf),
WorkingDirectory(PathBuf),
/// Received a path that could not be converted to a URL.
#[error("path could not be converted to a URL: {0}")]
UrlConversion(PathBuf),
/// Received a path that could not be normalized.
#[error("path could not be normalized: {0}")]
Normalization(PathBuf, #[source] std::io::Error),
}
/// Expand all available environment variables.

View file

@ -297,10 +297,15 @@ impl EditableRequirement {
VerbatimUrl::parse_path(expanded.as_ref(), working_dir.as_ref())
};
let url = url.map_err(|err| RequirementsTxtParserError::VerbatimUrl {
source: err,
url: expanded.to_string(),
})?;
// Create a `PathBuf`.
let path = url
.to_file_path()
.map_err(|()| RequirementsTxtParserError::InvalidEditablePath(expanded.to_string()))?;
.map_err(|()| RequirementsTxtParserError::UrlConversion(expanded.to_string()))?;
// Add the verbatim representation of the URL to the `VerbatimUrl`.
let url = url.with_given(requirement.to_string());
@ -1034,7 +1039,11 @@ pub enum RequirementsTxtParserError {
start: usize,
end: usize,
},
InvalidEditablePath(String),
VerbatimUrl {
source: pep508_rs::VerbatimUrlError,
url: String,
},
UrlConversion(String),
UnsupportedUrl(String),
MissingRequirementPrefix(String),
NoBinary {
@ -1091,7 +1100,7 @@ impl RequirementsTxtParserError {
fn with_offset(self, offset: usize) -> Self {
match self {
Self::IO(err) => Self::IO(err),
Self::InvalidEditablePath(given) => Self::InvalidEditablePath(given),
Self::UrlConversion(given) => Self::UrlConversion(given),
Self::Url {
source,
url,
@ -1103,6 +1112,7 @@ impl RequirementsTxtParserError {
start: start + offset,
end: end + offset,
},
Self::VerbatimUrl { source, url } => Self::VerbatimUrl { source, url },
Self::UnsupportedUrl(url) => Self::UnsupportedUrl(url),
Self::MissingRequirementPrefix(given) => Self::MissingRequirementPrefix(given),
Self::NoBinary {
@ -1171,12 +1181,15 @@ impl Display for RequirementsTxtParserError {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match self {
Self::IO(err) => err.fmt(f),
Self::InvalidEditablePath(given) => {
write!(f, "Invalid editable path: {given}")
}
Self::Url { url, start, .. } => {
write!(f, "Invalid URL at position {start}: `{url}`")
}
Self::VerbatimUrl { source, url } => {
write!(f, "Invalid URL: `{url}`: {source}")
}
Self::UrlConversion(given) => {
write!(f, "Unable to convert URL to path: {given}")
}
Self::UnsupportedUrl(url) => {
write!(f, "Unsupported URL (expected a `file://` scheme): `{url}`")
}
@ -1231,7 +1244,8 @@ impl std::error::Error for RequirementsTxtParserError {
match &self {
Self::IO(err) => err.source(),
Self::Url { source, .. } => Some(source),
Self::InvalidEditablePath(_) => None,
Self::VerbatimUrl { source, .. } => Some(source),
Self::UrlConversion(_) => None,
Self::UnsupportedUrl(_) => None,
Self::MissingRequirementPrefix(_) => None,
Self::NoBinary { source, .. } => Some(source),
@ -1260,10 +1274,13 @@ impl Display for RequirementsTxtFileError {
self.file.user_display(),
)
}
RequirementsTxtParserError::InvalidEditablePath(given) => {
RequirementsTxtParserError::VerbatimUrl { url, .. } => {
write!(f, "Invalid URL in `{}`: `{url}`", self.file.user_display())
}
RequirementsTxtParserError::UrlConversion(given) => {
write!(
f,
"Invalid editable path in `{}`: {given}",
"Unable to convert URL to path `{}`: {given}",
self.file.user_display()
)
}

View file

@ -17,12 +17,20 @@ use crate::{Connectivity, Error, ErrorKind, RegistryClient};
#[derive(Debug, thiserror::Error)]
pub enum FlatIndexError {
#[error("Failed to read `--find-links` directory: {0}")]
FindLinksDirectory(PathBuf, #[source] std::io::Error),
FindLinksDirectory(PathBuf, #[source] FindLinksDirectoryError),
#[error("Failed to read `--find-links` URL: {0}")]
FindLinksUrl(Url, #[source] Error),
}
#[derive(Debug, thiserror::Error)]
pub enum FindLinksDirectoryError {
#[error(transparent)]
Io(#[from] std::io::Error),
#[error(transparent)]
VerbatimUrl(#[from] pep508_rs::VerbatimUrlError),
}
#[derive(Debug, Default, Clone)]
pub struct FlatIndexEntries {
/// The list of `--find-links` entries.
@ -202,10 +210,10 @@ impl<'a> FlatIndexClient<'a> {
}
/// Read a flat remote index from a `--find-links` directory.
fn read_from_directory(path: &PathBuf) -> Result<FlatIndexEntries, std::io::Error> {
fn read_from_directory(path: &PathBuf) -> Result<FlatIndexEntries, FindLinksDirectoryError> {
// Absolute paths are required for the URL conversion.
let path = fs_err::canonicalize(path)?;
let index_url = IndexUrl::Path(VerbatimUrl::from_path(&path));
let index_url = IndexUrl::Path(VerbatimUrl::from_path(&path)?);
let mut dists = Vec::new();
for entry in fs_err::read_dir(path)? {

View file

@ -21,6 +21,8 @@ pub enum Error {
// Network error
#[error("Failed to parse URL: {0}")]
Url(String, #[source] url::ParseError),
#[error("Expected an absolute path, but received: {}", _0.user_display())]
RelativePath(PathBuf),
#[error(transparent)]
JoinRelativeUrl(#[from] pypi_types::JoinRelativeError),
#[error("Git operation failed")]

View file

@ -94,10 +94,10 @@ impl<'a> RegistryWheelIndex<'a> {
.filter_map(|flat_index| match flat_index {
FlatIndexLocation::Path(path) => {
let path = fs_err::canonicalize(path).ok()?;
Some(IndexUrl::Path(VerbatimUrl::from_path(path)))
Some(IndexUrl::Path(VerbatimUrl::from_path(path).ok()?))
}
FlatIndexLocation::Url(url) => {
Some(IndexUrl::Url(VerbatimUrl::unknown(url.clone())))
Some(IndexUrl::Url(VerbatimUrl::from_url(url.clone())))
}
})
.collect();

View file

@ -105,7 +105,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
Url::parse(url).map_err(|err| Error::Url(url.clone(), err))?
}
FileLocation::Path(path) => {
let url = Url::from_file_path(path).expect("path is absolute");
let url = Url::from_file_path(path)
.map_err(|()| Error::RelativePath(path.clone()))?;
return self
.archive(
source,
@ -262,7 +263,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
Url::parse(url).map_err(|err| Error::Url(url.clone(), err))?
}
FileLocation::Path(path) => {
let url = Url::from_file_path(path).expect("path is absolute");
let url = Url::from_file_path(path)
.map_err(|()| Error::RelativePath(path.clone()))?;
return self
.archive_metadata(
source,

View file

@ -59,6 +59,8 @@ pub enum LoweringError {
InvalidEntry,
#[error(transparent)]
InvalidUrl(#[from] url::ParseError),
#[error(transparent)]
InvalidVerbatimUrl(#[from] pep508_rs::VerbatimUrlError),
#[error("Can't combine URLs from both `project.dependencies` and `tool.uv.sources`")]
ConflictingUrls,
#[error("Could not normalize path: `{0}`")]
@ -551,7 +553,7 @@ fn path_source(
project_dir: &Path,
editable: bool,
) -> Result<RequirementSource, LoweringError> {
let url = VerbatimUrl::parse_path(&path, project_dir).with_given(path.clone());
let url = VerbatimUrl::parse_path(&path, project_dir)?.with_given(path.clone());
let path_buf = PathBuf::from(&path);
let path_buf = path_buf
.absolutize_from(project_dir)

View file

@ -132,7 +132,7 @@ impl RequirementsSpecification {
project: None,
requirements: vec![UnresolvedRequirementSpecification {
requirement: UnresolvedRequirement::Unnamed(UnnamedRequirement {
url: VerbatimUrl::from_path(path),
url: VerbatimUrl::from_path(path)?,
extras: vec![],
marker: None,
origin: None,