Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect allow insecure host in publish #8440

Merged
merged 1 commit into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions crates/uv-client/src/base_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,16 +350,6 @@ enum Security {
}

impl BaseClient {
/// The underlying [`ClientWithMiddleware`] for secure requests.
pub fn client(&self) -> ClientWithMiddleware {
self.client.clone()
}

/// The underlying [`Client`] without middleware.
pub fn raw_client(&self) -> Client {
self.raw_client.clone()
}
Comment on lines -353 to -361
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to break a downstream consumer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who would be affected? I can't find anything

$ git log -S "fn raw_client"
commit e7ae0f50d218aede8d7283d8cf414e8f7782fbba (origin/main, origin/HEAD, main)
Author: konsti <[email protected]>
Date:   Tue Oct 22 13:36:18 2024 +0200

    Respect allow insecure host in publish (#8440)

commit 205bf8cabef5c931203df15bca5e524dbfaac08a
Author: konsti <[email protected]>
Date:   Tue Sep 24 18:07:20 2024 +0200

    Implement trusted publishing (#7548)
    
    Co-authored-by: Charlie Marsh <[email protected]>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you just added it? Okee


/// Selects the appropriate client based on the host's trustworthiness.
pub fn for_host(&self, url: &Url) -> &ClientWithMiddleware {
if self
Expand Down
13 changes: 8 additions & 5 deletions crates/uv-client/tests/it/user_agent_version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ use hyper::service::service_fn;
use hyper::{Request, Response};
use hyper_util::rt::TokioIo;
use insta::{assert_json_snapshot, assert_snapshot, with_settings};
use std::str::FromStr;
use tokio::net::TcpListener;

use url::Url;
use uv_cache::Cache;
use uv_client::LineHaul;
use uv_client::RegistryClientBuilder;
Expand Down Expand Up @@ -53,11 +54,12 @@ async fn test_user_agent_has_version() -> Result<()> {
let client = RegistryClientBuilder::new(cache).build();

// Send request to our dummy server
let url = Url::from_str(&format!("http://{addr}"))?;
let res = client
.cached_client()
.uncached()
.client()
.get(format!("http://{addr}"))
.for_host(&url)
.get(url)
.send()
.await?;

Expand Down Expand Up @@ -149,11 +151,12 @@ async fn test_user_agent_has_linehaul() -> Result<()> {
let client = builder.build();

// Send request to our dummy server
let url = Url::from_str(&format!("http://{addr}"))?;
let res = client
.cached_client()
.uncached()
.client()
.get(format!("http://{addr}"))
.for_host(&url)
.get(url)
.send()
.await?;

Expand Down
15 changes: 8 additions & 7 deletions crates/uv-publish/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use itertools::Itertools;
use reqwest::header::AUTHORIZATION;
use reqwest::multipart::Part;
use reqwest::{Body, Response, StatusCode};
use reqwest_middleware::{ClientWithMiddleware, RequestBuilder};
use reqwest_middleware::RequestBuilder;
use reqwest_retry::{Retryable, RetryableStrategy};
use rustc_hash::FxHashSet;
use serde::Deserialize;
Expand All @@ -24,7 +24,7 @@ use tokio::io::AsyncReadExt;
use tokio_util::io::ReaderStream;
use tracing::{debug, enabled, trace, Level};
use url::Url;
use uv_client::UvRetryableStrategy;
use uv_client::{BaseClient, UvRetryableStrategy};
use uv_configuration::{KeyringProviderType, TrustedPublishing};
use uv_distribution_filename::{DistFilename, SourceDistExtension, SourceDistFilename};
use uv_fs::{ProgressReader, Simplified};
Expand Down Expand Up @@ -246,7 +246,7 @@ pub async fn check_trusted_publishing(
keyring_provider: KeyringProviderType,
trusted_publishing: TrustedPublishing,
registry: &Url,
client: &ClientWithMiddleware,
client: &BaseClient,
) -> Result<Option<TrustedPublishingToken>, PublishError> {
match trusted_publishing {
TrustedPublishing::Automatic => {
Expand All @@ -264,7 +264,7 @@ pub async fn check_trusted_publishing(
// We could check for credentials from the keyring or netrc the auth middleware first, but
// given that we are in GitHub Actions we check for trusted publishing first.
debug!("Running on GitHub Actions without explicit credentials, checking for trusted publishing");
match trusted_publishing::get_token(registry, client).await {
match trusted_publishing::get_token(registry, client.for_host(registry)).await {
Ok(token) => Ok(Some(token)),
Err(err) => {
// TODO(konsti): It would be useful if we could differentiate between actual errors
Expand All @@ -283,7 +283,7 @@ pub async fn check_trusted_publishing(
);
}

let token = trusted_publishing::get_token(registry, client).await?;
let token = trusted_publishing::get_token(registry, client.for_host(registry)).await?;
Ok(Some(token))
}
TrustedPublishing::Never => Ok(None),
Expand All @@ -300,7 +300,7 @@ pub async fn upload(
raw_filename: &str,
filename: &DistFilename,
registry: &Url,
client: &ClientWithMiddleware,
client: &BaseClient,
retries: u32,
username: Option<&str>,
password: Option<&str>,
Expand Down Expand Up @@ -504,7 +504,7 @@ async fn build_request(
raw_filename: &str,
filename: &DistFilename,
registry: &Url,
client: &ClientWithMiddleware,
client: &BaseClient,
username: Option<&str>,
password: Option<&str>,
form_metadata: &[(&'static str, String)],
Expand Down Expand Up @@ -543,6 +543,7 @@ async fn build_request(
};

let mut request = client
.for_host(&url)
.post(url)
.multipart(form)
// Ask PyPI for a structured error messages instead of HTML-markup error messages.
Expand Down
4 changes: 2 additions & 2 deletions crates/uv-publish/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ async fn upload_request_source_dist() {
raw_filename,
&filename,
&Url::parse("https://example.org/upload").unwrap(),
&BaseClientBuilder::new().build().client(),
&BaseClientBuilder::new().build(),
Some("ferris"),
Some("F3RR!S"),
&form_metadata,
Expand Down Expand Up @@ -229,7 +229,7 @@ async fn upload_request_wheel() {
raw_filename,
&filename,
&Url::parse("https://example.org/upload").unwrap(),
&BaseClientBuilder::new().build().client(),
&BaseClientBuilder::new().build(),
Some("ferris"),
Some("F3RR!S"),
&form_metadata,
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-python/src/downloads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ async fn read_url(

Ok((Either::Left(reader), Some(size)))
} else {
let response = client.client().get(url.clone()).send().await?;
let response = client.for_host(url).get(url.clone()).send().await?;

// Ensure the request was successful.
response.error_for_status_ref()?;
Expand Down
18 changes: 16 additions & 2 deletions crates/uv-requirements-txt/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -798,9 +798,11 @@ async fn read_url_to_string(
url: path.as_ref().to_owned(),
})?;

let url = Url::from_str(path_utf8)
.map_err(|err| RequirementsTxtParserError::InvalidUrl(path_utf8.to_string(), err))?;
Ok(client
.client()
.get(path_utf8)
.for_host(&url)
.get(url)
.send()
.await?
.error_for_status()?
Expand Down Expand Up @@ -890,6 +892,8 @@ pub enum RequirementsTxtParserError {
},
#[cfg(feature = "http")]
Reqwest(reqwest_middleware::Error),
#[cfg(feature = "http")]
InvalidUrl(String, url::ParseError),
}

impl Display for RequirementsTxtParserError {
Expand Down Expand Up @@ -956,6 +960,10 @@ impl Display for RequirementsTxtParserError {
Self::Reqwest(err) => {
write!(f, "Error while accessing remote requirements file {err}")
}
#[cfg(feature = "http")]
Self::InvalidUrl(url, err) => {
write!(f, "Not a valid URL, {err}: `{url}`")
}
}
}
}
Expand All @@ -982,6 +990,8 @@ impl std::error::Error for RequirementsTxtParserError {
Self::NonUnicodeUrl { .. } => None,
#[cfg(feature = "http")]
Self::Reqwest(err) => err.source(),
#[cfg(feature = "http")]
Self::InvalidUrl(_, err) => err.source(),
}
}
}
Expand Down Expand Up @@ -1114,6 +1124,10 @@ impl Display for RequirementsTxtFileError {
self.file.user_display(),
)
}
#[cfg(feature = "http")]
RequirementsTxtParserError::InvalidUrl(url, err) => {
write!(f, "Not a valid URL, {err}: `{url}`")
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/uv/src/commands/project/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1185,7 +1185,7 @@ impl RunCommand {
.connectivity(connectivity)
.native_tls(native_tls)
.build();
let response = client.client().get(url.clone()).send().await?;
let response = client.for_host(&url).get(url.clone()).send().await?;

// Stream the response to the file.
let mut writer = file.as_file();
Expand Down
4 changes: 2 additions & 2 deletions crates/uv/src/commands/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ pub(crate) async fn publish(
keyring_provider,
trusted_publishing,
&publish_url,
&oidc_client.client(),
&oidc_client,
)
.await?;

Expand Down Expand Up @@ -104,7 +104,7 @@ pub(crate) async fn publish(
&raw_filename,
&filename,
&publish_url,
&upload_client.client(),
&upload_client,
DEFAULT_RETRIES,
username.as_deref(),
password.as_deref(),
Expand Down
Loading