refactor(repo): wrap credentials in SecretString to prevent leaks#2931
Open
grainier wants to merge 1 commit intoapache:masterfrom
Open
refactor(repo): wrap credentials in SecretString to prevent leaks#2931grainier wants to merge 1 commit intoapache:masterfrom
grainier wants to merge 1 commit intoapache:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2931 +/- ##
==========================================
Coverage 70.27% 70.28%
Complexity 862 862
==========================================
Files 1028 1032 +4
Lines 85279 85422 +143
Branches 62656 62808 +152
==========================================
+ Hits 59932 60037 +105
- Misses 22833 22864 +31
- Partials 2514 2521 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hubcio
requested changes
Mar 15, 2026
| pub enabled: bool, | ||
| pub address: String, | ||
| #[config_env(secret)] | ||
| pub api_key: String, |
Contributor
There was a problem hiding this comment.
any reason why this is still plain String while RuntimeContext.api_key is SecretString?
| #[derive(Serialize, Deserialize)] | ||
| pub struct TokenInfo { | ||
| /// The value of token. | ||
| pub token: String, |
Contributor
There was a problem hiding this comment.
critical issue: you overidden debug, but users of this token can still leak this via Display, Serialize or cloning into log contexts
same issue in RawPersonalAccessToken.token
| #[derive(Debug, Clone)] | ||
| pub struct PersonalAccessTokenState { | ||
| pub name: String, | ||
| pub token_hash: String, |
Contributor
There was a problem hiding this comment.
preexisting issue: if anyone logs UserState with {:?}, the bcrypt/argon2 hash leaks. The Display impl is safe (doesn't include it), but Debug is not.
Comment on lines
+38
to
+53
| pub fn serialize_secret<S: serde::Serializer>( | ||
| secret: &SecretString, | ||
| serializer: S, | ||
| ) -> Result<S::Ok, S::Error> { | ||
| serializer.serialize_str(secret.expose_secret()) | ||
| } | ||
|
|
||
| pub fn serialize_optional_secret<S: serde::Serializer>( | ||
| secret: &Option<SecretString>, | ||
| serializer: S, | ||
| ) -> Result<S::Ok, S::Error> { | ||
| match secret { | ||
| Some(s) => serializer.serialize_some(s.expose_secret()), | ||
| None => serializer.serialize_none(), | ||
| } | ||
| } |
| auto_login: AutoLogin::Enabled(Credentials::UsernamePassword( | ||
| username.to_owned(), | ||
| password.to_owned(), | ||
| SecretString::from(password.to_owned()), |
Contributor
There was a problem hiding this comment.
cant you just use SecretString::from(password)? SecretString implements From<&str>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #2728
Rationale
Passwords, tokens, and connection strings were stored as plain
String, risking exposure viaDebug,Display, logs, and error messages.What changed?
Sensitive credential fields (passwords, PATs, connection URIs, API keys) were plain
Stringvalues that could leak through derivedDebug, log interpolation, or error formatting.All such fields now use
secrecy::SecretString, which redacts inDebugand requires explicitexpose_secret()to access the inner value. CustomDebugimpls were added for types that hold sensitiveStringfields not converted toSecretString(e.g.,RawPersonalAccessToken,TokenInfo). Aserde_secrethelper module was added for fields that must be serialized over the wire.Local Execution
All quality checks mentioned in CONTRIBUTING.md pass locally:
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningscargo buildcargo testcargo machetecargo sort --workspacetyposAI Usage
Stringpasswords/tokens needed conversion toSecretString, for documentation (i.e. core/common/src/utils/serde_secret.rs doc string), and commit review.