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

Wrapper type for data that should never be logged #11545

Merged
merged 10 commits into from
Jan 20, 2023
Merged

Wrapper type for data that should never be logged #11545

merged 10 commits into from
Jan 20, 2023

Conversation

kylematsuda
Copy link
Contributor

Fixes #11519.

So far this is just creating the new wrapper type. If this looks okay, I'll start adding this wrapper in places where tokens and secret keys are held or passed.

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2023

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2023
@kylematsuda
Copy link
Contributor Author

r? @Eh2406

@rustbot rustbot assigned Eh2406 and unassigned weihanglo Jan 5, 2023
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 6, 2023

This is more general than I had in mind. I would love to see what you end up doing with it. I was imagining a new type around String, so if your generic version gets difficult consider simplifying.

@kylematsuda
Copy link
Contributor Author

Thanks for taking a look! Sounds good, I'll give it a shot like this for now, but keeping the newtype approach in mind if things get hard (or if we decide that having this be generic is unnecessary).

@kylematsuda kylematsuda marked this pull request as ready for review January 11, 2023 03:13
@kylematsuda kylematsuda changed the title [WIP] Wrapper type for data that should never be logged Wrapper type for data that should never be logged Jan 11, 2023
@kylematsuda
Copy link
Contributor Author

I added the wrapper to most of the interfaces in ops::registry and util::auth. If this looks roughly like what you were envisioning, I'm happy to add this to other modules too as needed 😃

Eh2406
Eh2406 approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@Eh2406 Eh2406 left a comment

Choose a reason for hiding this comment

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

Looks good! Ping me when you are ready for me to look again.

src/bin/cargo/commands/login.rs Outdated Show resolved Hide resolved
src/cargo/ops/registry.rs Show resolved Hide resolved
src/cargo/ops/registry.rs Outdated Show resolved Hide resolved
@kylematsuda
Copy link
Contributor Author

Hi! I changed back to using Secret<&str> and tried to improve the docs a bit. I also added a doctest to ensure that the Debug impl is working as expected.

I also noticed a few places where tokens were being read or generated and then put inside a Secret, so I refactored those a little bit in the latest commit. I think it might be a little better, but also a little less readable, so I'd be interested to see what you think.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 20, 2023

Thank you! This looks good.

@bors r+

( ^ Tells our CI to run a few more checks and then merger this PR. )

@bors
Copy link
Contributor

bors commented Jan 20, 2023

📌 Commit efb972a has been approved by Eh2406

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 Jan 20, 2023
@bors
Copy link
Contributor

bors commented Jan 20, 2023

⌛ Testing commit efb972a with merge d6ba7a3...

bors added a commit that referenced this pull request Jan 20, 2023
@bors
Auto merge of #11545 - kylematsuda:secret-type, r=Eh2406
d6ba7a3 
Wrapper type for data that should never be logged

Fixes #11519.

So far this is just creating the new wrapper type. If this looks okay, I'll start adding this wrapper in places where tokens and secret keys are held or passed.
@bors
Copy link
Contributor

bors commented Jan 20, 2023

💔 Test failed - checks-actions

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

Eh2406 commented Jan 20, 2023

@bors retry

@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 Jan 20, 2023
@bors
Copy link
Contributor

bors commented Jan 20, 2023

⌛ Testing commit efb972a with merge ba6217e...

@bors
Copy link
Contributor

bors commented Jan 20, 2023

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing ba6217e to master...

@bors bors merged commit ba6217e into rust-lang:master Jan 20, 2023
@weihanglo weihanglo mentioned this pull request Jan 20, 2023
Merged
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2023
@weihanglo
Update cargo
ea4620a 
3 commits in 50eb688c2bbea5de5a2e8496230a7428798089d1..985d561f0bb9b76ec043a2b12511790ec7a2b954

2023-01-19 10:09:05 +0000 to 2023-01-20 14:39:28 +0000
- Stabilize sparse-registry (rust-lang/cargo#11224)
- Wrapper type for data that should never be logged (rust-lang/cargo#11545)
- Add semver rule for lints (rust-lang/cargo#11596)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2023
@bors
Auto merge of rust-lang#107120 - weihanglo:update-cargo, r=weihanglo
5ce39f4 
Update cargo

3 commits in 50eb688c2bbea5de5a2e8496230a7428798089d1..985d561f0bb9b76ec043a2b12511790ec7a2b954

2023-01-19 10:09:05 +0000 to 2023-01-20 14:39:28 +0000
- Stabilize sparse-registry (rust-lang/cargo#11224)
- Wrapper type for data that should never be logged (rust-lang/cargo#11545)
- Add semver rule for lints (rust-lang/cargo#11596)

r? `@ghost`
@ehuss ehuss added this to the 1.68.0 milestone Jan 28, 2023
@weihanglo weihanglo mentioned this pull request Jun 19, 2023
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eh2406 Eh2406 Eh2406 approved these changes

Assignees

@Eh2406 Eh2406

Labels
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.68.0
Development

Successfully merging this pull request may close these issues.

A new type for data that should never be logged.
6 participants
@kylematsuda @rustbot @Eh2406 @bors @ehuss @weihanglo