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

Don't retry invalid credentials from git credential helpers #6681

Merged
merged 2 commits into from
Feb 19, 2019

Conversation

hmarr
Copy link
Contributor

@hmarr hmarr commented Feb 19, 2019

If a git credential helper returns invalid credentials, we currently get stuck in an infinite retry loop by calling the credentials callback over and over, each time returning the same invalid credentials. This change means we only invoke the credential helper once.

How to reproduce

  1. Create a git credential store with some invalid credentials:
echo "https://example-user:invalid-credentials@github.com" > ~/invalid-store
  1. Tell git to use that as your credential store by adding this to your ~/.gitconfig:
[credential]
	helper = store --file=/home/<user>/invalid-store
  1. Add an invalid Git dependency to a Cargo.toml. For instance:
[dependencies.fake-repository]
git = "https://github.com/fake-user/fake-repository"
version = ">= 1.0.0"
  1. Try to update the dependencies (e.g. with cargo update or cargo build).

Cargo hangs forever, retrying the invalid credentials.

If the git credential helper returns invalid credentials, we currently
get stuck in an infinite retry loop by calling the credentials callback
over and over, each time returning the same invalid credentials. This
change means we only invoke the credential helper once.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@hmarr
Copy link
Contributor Author

hmarr commented Feb 19, 2019

Looks like the test failure is unrelated - quoting the comment above the test that failed:

/// This is a form of randomized testing, if you are unlucky it can fail.
/// A failure on its own is not a big deal. If you did not change the
/// registry_strategy then feel free to retry without concern.

@alexcrichton
Copy link
Member

Thanks for this! Could this piggy-back off cred_helper_bad perhaps? Additionally, could the comment above this function be updated to indicate why this is being checked?

@hmarr
Copy link
Contributor Author

hmarr commented Feb 19, 2019

Thanks for your quick reply!

I've updated the comment and reused the existing variable. Let me know if there's anything else that could be improved 🙂

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 19, 2019

📌 Commit 96ab67b has been approved by alexcrichton

bors added a commit that referenced this pull request Feb 19, 2019
Don't retry invalid credentials from git credential helpers

If a git credential helper returns invalid credentials, we currently get stuck in an infinite retry loop by calling the credentials callback over and over, each time returning the same invalid credentials. This change means we only invoke the credential helper once.

## How to reproduce

1. Create a git credential store with some invalid credentials:

```
echo "https://example-user:invalid-credentials@github.com" > ~/invalid-store
```

2. Tell git to use that as your credential store by adding this to your `~/.gitconfig`:

```
[credential]
	helper = store --file=/home/<user>/invalid-store
```

3. Add an invalid Git dependency to a `Cargo.toml`. For instance:

```
[dependencies.fake-repository]
git = "https://github.com/fake-user/fake-repository"
version = ">= 1.0.0"
```

4. Try to update the dependencies (e.g. with `cargo update` or `cargo build`).

Cargo hangs forever, retrying the invalid credentials.
@bors
Copy link
Contributor

bors commented Feb 19, 2019

⌛ Testing commit 96ab67b with merge b33ce7f...

@bors
Copy link
Contributor

bors commented Feb 19, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing b33ce7f to master...

@bors bors merged commit 96ab67b into rust-lang:master Feb 19, 2019
bors added a commit to rust-lang/rust that referenced this pull request Feb 21, 2019
Update cargo

9 commits in 865cb70106a6b1171a500ff68f93ab52eea56e72..b33ce7fc9092962b0657b4c25354984b5e5c47e4
2019-02-10 15:49:37 +0000 to 2019-02-19 18:42:50 +0000
- Don't retry invalid credentials from git credential helpers (rust-lang/cargo#6681)
- Fix some typos in resolver tests (rust-lang/cargo#6682)
- Add an unstable option to build proc macros for both the host and the target (rust-lang/cargo#6547)
- Test cases proving RUSTC_WRAPPER can be a relative path (rust-lang/cargo#6638)
- Add support for Azure DevOps (rust-lang/cargo#6264)
- Update docs for removed `patch` restriction. (rust-lang/cargo#6663)
- Fix incorrect help message (rust-lang/cargo#6555)
- Stabilize Alternative Registries (rust-lang/cargo#6654)
- Having a [patch] section when publishing is not an error (rust-lang/cargo#6535)
@ehuss ehuss added this to the 1.34.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants