-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Retain passwords in Git URLs #1717
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
d260643
to
304ffdf
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ac1fa25
to
e868ce8
Compare
e868ce8
to
8aa27b6
Compare
d176fa6
to
ee4f164
Compare
// Clone to avoid borrow checker issues | ||
let immutable_url = url.clone(); | ||
|
||
// If a Git URL ends in a reference (like a branch, tag, or commit), remove it. | ||
if url.scheme().starts_with("git+") { | ||
if let Some((prefix, _)) = url.as_str().rsplit_once('@') { | ||
url = prefix.parse().unwrap(); | ||
if let Some((prefix, _)) = immutable_url.path().rsplit_once('@') { | ||
url.set_path(prefix); | ||
} | ||
} |
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 awkward, is there a better pattern?
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'll play with it, I'm not sure...
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.
One option:
if url.scheme().starts_with("git+") {
if let Some(prefix) = url.path().rsplit_once('@').map(|(prefix, _suffix)| prefix.to_string()) {
url.set_path(&prefix);
}
}
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.
Still kind of strange but it seems better.
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.
Thanks!
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.
Like... mildly better because it doesn't clone the suffix, I guess, and it's contained without another binding.
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.
In the other case we need to clone the suffix too lol
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.
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 cloned original URL bit is weird though I prefer this
17b1f1a
to
03a1e07
Compare
03a1e07
to
2a5bebe
Compare
…st SSH support (#1781) Closes #1775 Closes #1452 Closes #1514 Follows #1717 libgit2 does not support host names with extra identifiers during SSH lookup (e.g. [`g.yxqyang.asia-some_identifier`]( https://docs.github.com/en/authentication/connecting-to-github-with-ssh/managing-deploy-keys#using-multiple-repositories-on-one-server)) so we use the `git` command instead for fetching. This is required for `pip` parity. See the [Cargo documentation](https://doc.rust-lang.org/nightly/cargo/reference/config.html#netgit-fetch-with-cli) for more details on using the `git` CLI instead of libgit2. We may want to try to use libgit2 first in the future, as it is more performant (#1786). We now support authentication with: ``` git+ssh://git@<hostname>/... git+ssh://git@<hostname>-<identifier>/... ``` Tested with a deploy key e.g. ``` cargo run -- \ pip install uv-private-pypackage@git+ssh://[email protected]/astral-test/uv-private-pypackage.git \ --reinstall --no-cache -v ``` and ``` cargo run -- \ pip install uv-private-pypackage@git+ssh://[email protected]/astral-test/uv-private-pypackage.git \ --reinstall --no-cache -v ``` with a ssh config like ``` Host github.com Hostname github.com IdentityFile=/Users/mz/.ssh/id_ed25519 Host g.yxqyang.asia-test-uv-private-pypackage Hostname github.com IdentityFile=/Users/mz/.ssh/id_ed25519 ``` It seems quite hard to add test coverage for this to the test suite, as we'd need to add the SSH key and I don't know how to isolate that from affecting other developer's machines.
Fixes handling of GitHub PATs in HTTPS URLs, which were otherwise dropped. We now supporting the following authentication schemes:
On Windows, the username is required. We can consider adding a special-case for this in the future, but this just matches libgit2's behavior.
I tested with fine-grained tokens, OAuth tokens, and "classic" tokens. There's test coverage for fine-grained tokens in CI where we use a real private repository and PAT. Yes, the PAT is committed to make this test usable by anyone. It has read-only permissions to the single repository, expires Feb 1 2025, and is in an isolated organization and GitHub account.
Does not yet address SSH authentication (see #1781)
Related: