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.

Sign up for GitHub

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

Jump to bottom

fix(credential): trim newlines in tokens from stdin #13770

Merged
merged 2 commits into from
Apr 17, 2024
Merged

fix(credential): trim newlines in tokens from stdin #13770

merged 2 commits into from
Apr 17, 2024

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Apr 17, 2024

What does this PR try to resolve?

cargo login when using a credential provider other than cargo:token does not automatically trim whitespace from tokens.

This can lead to extra whitespace being included in the pasted token value (usually a trailing newline) that makes the token invalid.

How should we test and review this PR?

First commit adds a test showing the problematic behavior. Second commit fixes it.

@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-interacts-with-crates.io Area: interaction with registries Command-login S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2024
@epage
Copy link
Contributor

epage commented Apr 17, 2024

I know this is unlikely but is there any case where newlines could be significant to these tokens?

@arlosi
Copy link
Contributor Author

arlosi commented Apr 17, 2024

I know this is unlikely but is there any case where newlines could be significant to these tokens?

The token is directly sent as the Authorization HTTP header, which doesn't allow newlines.

If you try to use it, you get:

cargo yank asdf --version 0.0.1
    Updating crates.io index
        Yank asdf@0.0.1
error: failed to yank from the registry at https://crates.io

Caused by:
  token contains invalid characters.
  Only printable ISO-8859-1 characters are allowed as it is sent in a HTTPS header.

epage
epage reviewed Apr 17, 2024
View reviewed changes
tests/testsuite/login.rs
@@ -113,6 +113,7 @@ fn empty_login_token() {
.with_stderr(
"\
[UPDATING] crates.io index
please paste the token found on [..] below
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this test passing today?

Copy link
Contributor Author

@arlosi arlosi Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the read_token function, if a token is provided by stdin or by the command line, then no prompt is made.

The change causes login_options.token to be None, instead of a Some("\n"), so Cargo now prints the "please paste" message in this case.
 

cargo/credential/cargo-credential/src/lib.rs

Lines 281 to 290 in 852a316

pub fn read_token(
login_options: &LoginOptions<'_>,
registry: &RegistryInfo<'_>,
) -> Result<Secret<String>, Error> {
if let Some(token) = &login_options.token {
return Ok(token.to_owned());
}
if let Some(url) = login_options.login_url {
eprintln!("please paste the token found on {url} below");

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move that change into the commit that caused it so that each commit passes tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I should have amended that instead of adding a commit.

@epage
Copy link
Contributor

epage commented Apr 17, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 17, 2024

📌 Commit 6207f93 has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2024
@bors
Copy link
Contributor

bors commented Apr 17, 2024

⌛ Testing commit 6207f93 with merge 5afc53a...

@bors
Copy link
Contributor

bors commented Apr 17, 2024

☀️ Test successful - checks-actions
Approved by: epage
Pushing 5afc53a to master...

@bors bors merged commit 5afc53a into rust-lang:master Apr 17, 2024
21 checks passed
This was referenced Apr 19, 2024
Merged
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2024
@bors
Auto merge of rust-lang#124177 - weihanglo:update-cargo, r=weihanglo
4edb607 
Update cargo

8 commits in 6f06fe908a5ee0f415c187f868ea627e82efe07d..80d5b607dde6ef97dfff4e23923822c01d2bb036
2024-04-16 18:47:44 +0000 to 2024-04-19 18:39:22 +0000
- fix 13773 - 'cargo build' fails when list_files() with gix is triggered (rust-lang/cargo#13777)
- fix(toml): Don't crash on parse errors that point to multi-byte character (rust-lang/cargo#13780)
- fix(toml)!: Disallow source-less dependencies (rust-lang/cargo#13775)
- fix(msrv): Put MSRV-aware resolver behind a config (rust-lang/cargo#13769)
- fix(msrv): Error, rather than panic, on rust-version 'x' (rust-lang/cargo#13771)
- fix(credential): trim newlines in tokens from stdin (rust-lang/cargo#13770)
- test(msrv): Re-organize MSRV tests (rust-lang/cargo#13767)
- feat(install): Including Locking message (rust-lang/cargo#13764)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2024
@bors
Auto merge of rust-lang#124177 - weihanglo:update-cargo, r=weihanglo
c979885 
Update cargo

8 commits in 6f06fe908a5ee0f415c187f868ea627e82efe07d..80d5b607dde6ef97dfff4e23923822c01d2bb036
2024-04-16 18:47:44 +0000 to 2024-04-19 18:39:22 +0000
- fix 13773 - 'cargo build' fails when list_files() with gix is triggered (rust-lang/cargo#13777)
- fix(toml): Don't crash on parse errors that point to multi-byte character (rust-lang/cargo#13780)
- fix(toml)!: Disallow source-less dependencies (rust-lang/cargo#13775)
- fix(msrv): Put MSRV-aware resolver behind a config (rust-lang/cargo#13769)
- fix(msrv): Error, rather than panic, on rust-version 'x' (rust-lang/cargo#13771)
- fix(credential): trim newlines in tokens from stdin (rust-lang/cargo#13770)
- test(msrv): Re-organize MSRV tests (rust-lang/cargo#13767)
- feat(install): Including Locking message (rust-lang/cargo#13764)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2024
@bors
Auto merge of rust-lang#124177 - weihanglo:update-cargo, r=weihanglo
c8d19a9 
Update cargo

8 commits in 6f06fe908a5ee0f415c187f868ea627e82efe07d..80d5b607dde6ef97dfff4e23923822c01d2bb036
2024-04-16 18:47:44 +0000 to 2024-04-19 18:39:22 +0000
- fix 13773 - 'cargo build' fails when list_files() with gix is triggered (rust-lang/cargo#13777)
- fix(toml): Don't crash on parse errors that point to multi-byte character (rust-lang/cargo#13780)
- fix(toml)!: Disallow source-less dependencies (rust-lang/cargo#13775)
- fix(msrv): Put MSRV-aware resolver behind a config (rust-lang/cargo#13769)
- fix(msrv): Error, rather than panic, on rust-version 'x' (rust-lang/cargo#13771)
- fix(credential): trim newlines in tokens from stdin (rust-lang/cargo#13770)
- test(msrv): Re-organize MSRV tests (rust-lang/cargo#13767)
- feat(install): Including Locking message (rust-lang/cargo#13764)

r? ghost
@rustbot rustbot added this to the 1.79.0 milestone Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@ehuss ehuss

Labels
A-interacts-with-crates.io Area: interaction with registries Command-login S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.
5 participants
@arlosi @rustbot @epage @bors @ehuss