Skip to content
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
15 changes: 10 additions & 5 deletions crates/uv-auth/src/middleware.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use tracing::{debug, trace, warn};

use uv_preview::{Preview, PreviewFeatures};
use uv_redacted::DisplaySafeUrl;
use uv_static::EnvVars;
use uv_warnings::owo_colors::OwoColorize;

use crate::credentials::Authentication;
Expand Down Expand Up @@ -352,11 +353,15 @@ impl Middleware for AuthMiddleware {
.is_some_and(|token_store| token_store.is_known_url(request.url()));

let must_authenticate = self.only_authenticated
|| match auth_policy {
AuthPolicy::Auto => is_known_url,
AuthPolicy::Always => true,
AuthPolicy::Never => false,
};
|| (match auth_policy {
AuthPolicy::Auto => is_known_url,
AuthPolicy::Always => true,
AuthPolicy::Never => false,
}
// Dependabot intercepts HTTP requests and injects credentials, which means that we
// cannot eagerly enforce an `AuthPolicy` as we don't know whether credentials will be
// added outside of uv.
&& !std::env::var(EnvVars::DEPENDABOT).is_ok_and(|value| value == "true"));
Copy link
Member

Choose a reason for hiding this comment

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

Should we avoid doing re-reading this in this hot path?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh we could. I assumed it was very cheap. I guess we're validating that the value is utf-8 and allocating a string?

I'm a little wary of the indirection of moving it out, I'm not sure where I'd put it.

Copy link
Member

Choose a reason for hiding this comment

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

Make it a once cell?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, environment accesses are somewhat expensive in Rust, since Rust acquires a lock on the entire environment (at least on macOS/Linux) for individual variable accesses. That wouldn't matter much in a lot of workloads, but I can imagine we'd have environment read contention in our setting because of work-splitting across multiple tokio-managed threads.

(It's probably still not a ton, but a OnceCell or LazyLock would avoid it as a concern.)

Ref: https://doc.rust-lang.org/src/std/sys/env/unix.rs.html


let (mut retry_request, response) = if !must_authenticate {
let url = tracing_url(&request, credentials.as_deref());
Expand Down
4 changes: 4 additions & 0 deletions crates/uv-static/src/env_vars.rs
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,10 @@ impl EnvVars {
#[attr_added_in("0.8.18")]
pub const CONDA_ROOT: &'static str = "_CONDA_ROOT";

/// Used to determine if we're running in Dependabot.
#[attr_added_in("next release")]
pub const DEPENDABOT: &'static str = "DEPENDABOT";

/// If set to `1` before a virtual environment is activated, then the
/// virtual environment name will not be prepended to the terminal prompt.
#[attr_added_in("0.0.5")]
Expand Down
5 changes: 5 additions & 0 deletions docs/reference/environment.md
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,11 @@ Used to determine the name of the active Conda environment.

Used to detect the path of an active Conda environment.

### `DEPENDABOT`
<small class="added-in">added in `next release`</small>

Used to determine if we're running in Dependabot.

### `FISH_VERSION`
<small class="added-in">added in `0.1.28`</small>

Expand Down
Loading