-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add DisplaySafeUrl newtype to prevent leaking of credentials by default
#13560
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
Conversation
LogSafeUrl newtype to prevent leaking of credentialsLogSafeUrl newtype to prevent leaking of credentials by default
|
I continued to use the new |
7c81f9a to
79d9a84
Compare
| } | ||
| } | ||
|
|
||
| impl IndexUrl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it confusing that LogSafeUrl here is both working a URL that has credentials, but redacts them, and as a URL that had its credentials removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that it's a version of Url that you can log safely. It's not meant to imply that the credentials are in any other state. I wanted it to effectively be uv's replacement for using Url in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe I'm missing part of your point. The without_credentials() case is for when you want to display or log the URL without the masked credentials. There are some cases where we want to indicate the credentials were part of the raw URL by showing the masked version (e.g., for debugging requests) and some cases where we just want the URL itself (like in the lockfile).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that's already in the existing code: Sometimes Url refers to a URL with credentials, and sometimes to the same underlying "data" without credentials. What about returning a impl Display instead of a real type from without_credentials to make it clearer that this is an output-only method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there cases where we might want the URL but not the credentials? That's thinking ahead. For now, we'd add some extra allocations and lose the type information (i.e., that this is safe to display). So I'm hesitant to change it, but not a strong opinion.
| Installed 1 package in [TIME] | ||
| + iniconfig==2.0.0 (from https://public:heron@pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl) | ||
| "###); | ||
| + iniconfig==2.0.0 (from https://public:****@pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new redactions in action!
| } | ||
| } | ||
|
|
||
| impl IndexUrl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that's already in the existing code: Sometimes Url refers to a URL with credentials, and sometimes to the same underlying "data" without credentials. What about returning a impl Display instead of a real type from without_credentials to make it clearer that this is an output-only method?
| "hint".bold().cyan(), | ||
| ":".bold(), | ||
| index.redacted().cyan(), | ||
| index.without_credentials().cyan(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn here, on one side seeing *** here could be confusing cause those credentials are evidently the wrong ones, otoh I'd really want to know that there were credentials on the URL here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point. Note that this is not changing the existing behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reflecting more, I think it's helpful this way since it indicates the invalid credentials were on the URL.
LogSafeUrl newtype to prevent leaking of credentials by defaultDisplaySafeUrl newtype to prevent leaking of credentials by default
Extends PR #13560 by using the `DisplaySafeUrl` and new `DisplaySafeUrlRef` types in the `uv-auth` crate. The old `tracing_url` function in `middleware.rs` created a string from request URL and credentials. If tracing was enabled, it would also add credentials to that URL and then mask them. In this PR, this function instead creates a `DisplaySafeUrl` and sets its credentials to be equal to the passed in credentials. This removes special-case masking logic and also ensures that downstream functions take a `DisplaySafeUrl` instead of a `&str`. It comes at the cost of one extra clone per request (not including retries) when tracing mode is disabled. `DisplaySafeUrlRef` was added to be able to work with `reqwest::Request::url()` without having to clone. As an alternative, we could have `DisplaySafeUrl` wrap a `Cow`, but this would require new lifetime annotations across many points in the codebase. Given that the need to wrap a `&Url` is currently limited to a few cases in `uv_auth`, I decided against paying the ergonomic costs. We could also clone the request `Url` in each case and then wrap it in a `DisplaySafeUrl`. This would be a simple change and remove the need for `DisplaySafeUrlRef`, but would require multiple extra clones per request.
| /// // `Deref` implementation, you can still access the username and password | ||
| /// assert_eq!(url.username(), "user"); | ||
| /// assert_eq!(url.password(), Some("password")); | ||
| pub struct DisplaySafeUrlRef<'a>(&'a Url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type can / should be Copy. It doesn't really make sense for users to use &DisplaySafeUrlRef since it's already a reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented this and your other suggestions in a follow-up PR: #13683.
| query: None, | ||
| fragment: None, | ||
| }, | ||
| url: https://file.pypi.org/simple, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the intended debug representation?
| let mut url = Url::from_file_path(path.clone()) | ||
| .map_err(|()| VerbatimUrlError::UrlConversion(path.to_path_buf()))?; | ||
| let mut url = DisplaySafeUrl::from( | ||
| Url::from_file_path(path.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these use DisplaySafeUrl::from_file_path directly?
| /// Returns string representation without masking credentials. | ||
| #[inline] | ||
| pub fn to_string_with_credentials(&self) -> String { | ||
| self.0.to_string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this instead return a type that implements Display? It seems more flexible?
This PR implements a few review follow-ups from #13560. In particular, it * Makes `DisplaySafeUrlRef` implement `Copy` so that it can be passed by value. * Updates `to_string_with_credentials` methods with `displayable_with_credentials`, returning an `impl Display` instead of `String` for greater flexibility. * Updates the `DisplaySafeUrl` and `DisplaySafeUrlRef` `Debug` implementations to match the underlying `Url` `Debug` implementation (with the exception that credentials are masked). * Replaces an unnecessary `DisplaySafeUrl::from(Url::from_file_path` with `DisplaySafeUrl::from_file_path`
|
For example I now seeing strange behaviour such as this: The wheel installation that is rejected is not a direct dependency |
|
Can you please create a separate issue, ideally with a minimal reproduction that we can execute ourselves? |
|
My team and I are also running into an issue where |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.7.7` -> `0.7.13` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.7.13`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0713) [Compare Source](astral-sh/uv@0.7.12...0.7.13) ##### Python - Add Python 3.14.0b2 - Add Python 3.13.5 - Fix stability of `uuid.getnode` on 3.13 See the [`python-build-standalone` release notes](https://github.com/astral-sh/python-build-standalone/releases/tag/20250612) for more details. ##### Enhancements - Download versions in `uv python pin` if not found ([#​13946](astral-sh/uv#13946)) - Use TTY detection to determine if SIGINT forwarding is enabled ([#​13925](astral-sh/uv#13925)) - Avoid fetching an exact, cached Git commit, even if it isn't locked ([#​13748](astral-sh/uv#13748)) - Add `zstd` and `deflate` to `Accept-Encoding` ([#​13982](astral-sh/uv#13982)) - Build binaries for riscv64 ([#​12688](astral-sh/uv#12688)) ##### Bug fixes - Check if relative URL is valid directory before treating as index ([#​13917](astral-sh/uv#13917)) - Ignore Python discovery errors during `uv python pin` ([#​13944](astral-sh/uv#13944)) - Do not allow `uv add --group ... --script` ([#​13997](astral-sh/uv#13997)) ##### Preview changes - Build backend: Support namespace packages ([#​13833](astral-sh/uv#13833)) ##### Documentation - Add 3.14 to the supported platform reference ([#​13990](astral-sh/uv#13990)) - Add an `llms.txt` to uv ([#​13929](astral-sh/uv#13929)) - Add supported macOS version to the platform reference ([#​13993](astral-sh/uv#13993)) - Update platform support reference to include Python implementation list ([#​13991](astral-sh/uv#13991)) - Update pytorch.md ([#​13899](astral-sh/uv#13899)) - Update the CLI help and reference to include references to the Python bin directory ([#​13978](astral-sh/uv#13978)) ### [`v0.7.12`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0712) [Compare Source](astral-sh/uv@0.7.11...0.7.12) ##### Enhancements - Add `uv python pin --rm` to remove `.python-version` pins ([#​13860](astral-sh/uv#13860)) - Don't hint at versions removed by `excluded-newer` ([#​13884](astral-sh/uv#13884)) - Add hint to use `tool.uv.environments` on resolution error ([#​13455](astral-sh/uv#13455)) - Add hint to use `tool.uv.required-environments` on resolution error ([#​13575](astral-sh/uv#13575)) - Improve `python pin` error messages ([#​13862](astral-sh/uv#13862)) ##### Bug fixes - Lock environments during `uv sync`, `uv add` and `uv remove` to prevent race conditions ([#​13869](astral-sh/uv#13869)) - Add `--no-editable` to `uv export` for `pylock.toml` ([#​13852](astral-sh/uv#13852)) ##### Documentation - List `.gitignore` in project init files ([#​13855](astral-sh/uv#13855)) - Move the pip interface documentation into the concepts section ([#​13841](astral-sh/uv#13841)) - Remove the configuration section in favor of concepts / reference ([#​13842](astral-sh/uv#13842)) - Update Git and GitHub Actions docs to mention `gh auth login` ([#​13850](astral-sh/uv#13850)) ##### Preview - Fix directory glob traversal fallback preventing exclusion of all files ([#​13882](astral-sh/uv#13882)) ### [`v0.7.11`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0711) [Compare Source](astral-sh/uv@0.7.10...0.7.11) ##### Python - Add Python 3.14.0b1 - Add Python 3.13.4 - Add Python 3.12.11 - Add Python 3.11.13 - Add Python 3.10.18 - Add Python 3.9.23 ##### Enhancements - Add Pyodide support ([#​12731](astral-sh/uv#12731)) - Better error message for version specifier with missing operator ([#​13803](astral-sh/uv#13803)) ##### Bug fixes - Downgrade `reqwest` and `hyper-util` to resolve connection reset errors over IPv6 ([#​13835](astral-sh/uv#13835)) - Prefer `uv`'s binary's version when checking if it's up to date ([#​13840](astral-sh/uv#13840)) ##### Documentation - Use "terminal driver" instead of "shell" in `SIGINT` docs ([#​13787](astral-sh/uv#13787)) ### [`v0.7.10`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0710) [Compare Source](astral-sh/uv@0.7.9...0.7.10) ##### Enhancements - Add `--show-extras` to `uv tool list` ([#​13783](astral-sh/uv#13783)) - Add dynamically generated sysconfig replacement mappings ([#​13441](astral-sh/uv#13441)) - Add data locations to install wheel logs ([#​13797](astral-sh/uv#13797)) ##### Bug fixes - Avoid redaction of placeholder `git` username when using SSH authentication ([#​13799](astral-sh/uv#13799)) - Propagate credentials to files on devpi indexes ending in `/+simple` ([#​13743](astral-sh/uv#13743)) - Restore retention of credentials for direct URLs in `uv export` ([#​13809](astral-sh/uv#13809)) ### [`v0.7.9`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#079) [Compare Source](astral-sh/uv@0.7.8...0.7.9) ##### Python The changes reverted in [0.7.8](#​078) have been restored. See the [`python-build-standalone` release notes](https://github.com/astral-sh/python-build-standalone/releases/tag/20250529) for more details. ##### Enhancements - Improve obfuscation of credentials in URLs ([#​13560](astral-sh/uv#13560)) - Allow running non-default Python implementations via `uvx` ([#​13583](astral-sh/uv#13583)) - Add `uvw` as alias for `uv` without console window on Windows ([#​11786](astral-sh/uv#11786)) - Allow discovery of x86-64 managed Python builds on macOS ([#​13722](astral-sh/uv#13722)) - Differentiate between implicit vs explicit architecture requests ([#​13723](astral-sh/uv#13723)) - Implement ordering for Python architectures to prefer native installations ([#​13709](astral-sh/uv#13709)) - Only show the first match per platform (and architecture) by default in `uv python list` ([#​13721](astral-sh/uv#13721)) - Write the path of the parent environment to an `extends-environment` key in the `pyvenv.cfg` file of an ephemeral environment ([#​13598](astral-sh/uv#13598)) - Improve the error message when libc cannot be found, e.g., when using the distroless containers ([#​13549](astral-sh/uv#13549)) ##### Performance - Avoid rendering info log level ([#​13642](astral-sh/uv#13642)) - Improve performance of `uv-python` crate's manylinux submodule ([#​11131](astral-sh/uv#11131)) - Optimize `Version` display ([#​13643](astral-sh/uv#13643)) - Reduce number of reference-checks for `uv cache clean` ([#​13669](astral-sh/uv#13669)) ##### Bug fixes - Avoid reinstalling dependency group members with `--all-packages` ([#​13678](astral-sh/uv#13678)) - Don't fail direct URL hash checking with dependency metadata ([#​13736](astral-sh/uv#13736)) - Exit early on `self update` if global `--offline` is set ([#​13663](astral-sh/uv#13663)) - Fix cases where the uv lock is incorrectly marked as out of date ([#​13635](astral-sh/uv#13635)) - Include pre-release versions in `uv python install --reinstall` ([#​13645](astral-sh/uv#13645)) - Set `LC_ALL=C` for git when checking git worktree ([#​13637](astral-sh/uv#13637)) - Avoid rejecting Windows paths for remote Python download JSON targets ([#​13625](astral-sh/uv#13625)) ##### Preview - Add `uv add --bounds` to configure version constraints ([#​12946](astral-sh/uv#12946)) ##### Documentation - Add documentation about Python versions to Tools concept page ([#​7673](astral-sh/uv#7673)) - Add example of enabling Dependabot ([#​13692](astral-sh/uv#13692)) - Fix `exclude-newer` date format for persistent configuration files ([#​13706](astral-sh/uv#13706)) - Quote versions variables in GitLab documentation ([#​13679](astral-sh/uv#13679)) - Update Dependabot support status ([#​13690](astral-sh/uv#13690)) - Explicitly specify to add a new repo entry to the repos list item in the `.pre-commit-config.yaml` ([#​10243](astral-sh/uv#10243)) - Add integration with marimo guide ([#​13691](astral-sh/uv#13691)) - Add pronunciation to README ([#​5336](astral-sh/uv#5336)) ### [`v0.7.8`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#078) [Compare Source](astral-sh/uv@0.7.7...0.7.8) ##### Python We are reverting most of our Python changes from `uv 0.7.6` and `uv 0.7.7` due to a miscompilation that makes the Python interpreter behave incorrectly, resulting in spurious type-errors involving str. This issue seems to be isolated to x86\_64 Linux, and affected at least Python 3.12, 3.13, and 3.14. The following changes that were introduced in those versions of uv are temporarily being reverted while we test and deploy a proper fix for the miscompilation: - Add Python 3.14 on musl - free-threaded Python on musl - Add Python 3.14.0a7 - Statically link `libpython` into the interpreter on Linux for a significant performance boost See [the issue for details](astral-sh/uv#13610). ##### Documentation - Remove misleading line in pin documentation ([#​13611](astral-sh/uv#13611)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MC4yNi4xIiwidXBkYXRlZEluVmVyIjoiNDAuNTEuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
Prior to this PR, there were numerous places where uv would leak credentials in logs. We had a way to mask credentials by calling methods or a recently-added
redact_urlfunction, but this was not secure by default. There were a number of other types (likeGitUrl) that would leak credentials on display.This PR adds a
DisplaySafeUrlnewtype to prevent leaking credentials when logging by default. It takes a maximalist approach, replacing the use ofUrlalmost everywhere. This includes when first parsing config files, when storing URLs in types likeGitUrl, and also when storing URLs in types that in practice will never contain credentials (likeDirectorySourceUrl). The idea is to make it easy for developers to do the right thing and for the compiler to support this (and to minimize ever having to manually convert back and forth). Displaying credentials now requires an active step. Note that despite this maximalist approach, the use of the newtype should be zero cost.One conspicuous place this PR does not use
DisplaySafeUrlis in theuv-authcrate. That would require new clones since there are calls torequest.url()that return a&Url. One option would have been to makeDisplaySafeUrlwrap aCow, but this would lead to lifetime annotations all over the codebase. I've created a separate PR based on this one (#13576) that updatesuv-authto useDisplaySafeUrlwith one new clone. We can discuss the tradeoffs there.Most of this PR just replaces
UrlwithDisplaySafeUrl. The core isuv_redacted/lib.rs, where the newtype is implemented. To make it easier to review the rest, here are some points of note:DisplaySafeUrlhas aDisplayimplementation that masks credentials. Currently, it will still display the username when there is both a username and password. If we think is the wrong choice, it can now be changed in one place.DisplaySafeUrlhas aremove_credentials()method and also a.to_string_with_credentials()method. This allows us to use it in a variety of scenarios.IndexUrl::redacted()was renamed toIndexUrl::removed_credentials()to make it clearer that we are not masking.DisplaySafeUrlto aUrlwhen callingreqwestmethods like.get()and.head().DisplaySafeUrlto aUrlwhen creating auv_auth::Index. That is because, as mentioned above, I will be updating theuv_authcrate to use this newtype in a separate PR.pip_install.rs) that formerly used filters to mask tokens in the test output no longer need those filters since tokens in URLs are now masked automatically.pyproject.tomlis when a URL with credentials is passed touv addwith--raw. Since displaying credentials is no longer automatic, I have added ato_string_with_credentials()method to thePep508Urltrait. This is used when--rawis passed. Adding it to that trait is a bit weird, but it's the simplest way to achieve the goal. I'm open to suggestions on how to improve this, but note that because of the way we're using generic bounds, it's not as simple as just creating a separate trait for that method.