Skip to content

Commit 4ffd973

Browse files
authored
Merge pull request #196 from samsoir/chore/162-mask-sensitive-config
chore(config): mask sensitive credentials in config list and diagnostics (#162)
2 parents ebba5c8 + de97135 commit 4ffd973

4 files changed

Lines changed: 236 additions & 26 deletions

File tree

‎xearthlayer-cli/src/commands/config.rs‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
77
use clap::Subcommand;
88
use xearthlayer::config::{
9-
analyze_config, config_file_path, upgrade_config, ConfigFile, ConfigKey,
9+
analyze_config, config_file_path, upgrade_config, ConfigFile, ConfigKey, SENSITIVE_VALUE_MASK,
1010
};
1111

1212
use crate::error::CliError;
@@ -120,14 +120,22 @@ fn run_list() -> Result<(), CliError> {
120120
current_section = section;
121121
}
122122

123-
let value = key.get(&config);
123+
let raw_value = key.get(&config);
124124
let key_name = key.key_name();
125125

126-
if value.is_empty() {
127-
println!(" {} = (not set)", key_name);
126+
// Mask credentials so the output is safe to paste into bug
127+
// reports. `(not set)` shows through unmasked because absence
128+
// isn't a leak and is informative for users debugging
129+
// "why isn't my Google provider working." See issue #162.
130+
let display_value = if raw_value.is_empty() {
131+
"(not set)".to_string()
132+
} else if key.is_sensitive() {
133+
SENSITIVE_VALUE_MASK.to_string()
128134
} else {
129-
println!(" {} = {}", key_name, value);
130-
}
135+
raw_value
136+
};
137+
138+
println!(" {} = {}", key_name, display_value);
131139
}
132140

133141
Ok(())

‎xearthlayer/src/config/keys.rs‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,14 @@ use super::file::ConfigFile;
1111
use super::size::{format_size, parse_size};
1212
use crate::dds::DdsFormat;
1313

14+
/// Display value used in place of any [`ConfigKey::is_sensitive`] value
15+
/// when the call site has no business showing the raw secret (notably
16+
/// `xearthlayer config list`, which is commonly pasted into bug reports).
17+
///
18+
/// Eight `x` characters: short enough to be obviously a placeholder, long
19+
/// enough that it can't be confused for a short real value.
20+
pub const SENSITIVE_VALUE_MASK: &str = "xxxxxxxx";
21+
1422
/// Errors that can occur when getting or setting configuration values.
1523
#[derive(Debug, Error)]
1624
pub enum ConfigKeyError {
@@ -276,6 +284,22 @@ impl ConfigKey {
276284
self.name().split('.').nth(1).unwrap_or(self.name())
277285
}
278286

287+
/// Whether this key holds a credential or other secret that should
288+
/// never be displayed verbatim in places that might end up in user
289+
/// bug reports (notably `xearthlayer config list`).
290+
///
291+
/// Per-call display sites should swap the value for
292+
/// [`SENSITIVE_VALUE_MASK`] when this returns true. `config get` and
293+
/// `config set` are intentionally NOT masked: they are authoritative
294+
/// user actions, and masking them would prevent users from
295+
/// inspecting or echoing back the value they just configured.
296+
pub fn is_sensitive(&self) -> bool {
297+
matches!(
298+
self,
299+
ConfigKey::ProviderGoogleApiKey | ConfigKey::ProviderMapboxAccessToken
300+
)
301+
}
302+
279303
/// Get the value from a config file as a string.
280304
pub fn get(&self, config: &ConfigFile) -> String {
281305
match self {
@@ -1010,6 +1034,54 @@ mod tests {
10101034
assert_eq!(ConfigKey::ProviderType.key_name(), "type");
10111035
}
10121036

1037+
#[test]
1038+
fn is_sensitive_returns_true_for_provider_credentials() {
1039+
assert!(ConfigKey::ProviderGoogleApiKey.is_sensitive());
1040+
assert!(ConfigKey::ProviderMapboxAccessToken.is_sensitive());
1041+
}
1042+
1043+
#[test]
1044+
fn is_sensitive_returns_false_for_non_credential_keys() {
1045+
// Spot-check across sections — anything that isn't a credential
1046+
// must round-trip as non-sensitive so that masked output stays
1047+
// narrowly scoped.
1048+
assert!(!ConfigKey::GeneralUpdateCheck.is_sensitive());
1049+
assert!(!ConfigKey::ProviderType.is_sensitive());
1050+
assert!(!ConfigKey::CacheDirectory.is_sensitive());
1051+
assert!(!ConfigKey::PackagesLibraryUrl.is_sensitive());
1052+
assert!(!ConfigKey::LoggingFile.is_sensitive());
1053+
assert!(!ConfigKey::PrefetchEnabled.is_sensitive());
1054+
}
1055+
1056+
#[test]
1057+
fn is_sensitive_covers_every_key_exhaustively() {
1058+
// Guard against a future ConfigKey variant being added without
1059+
// a deliberate sensitivity decision: every key in `all()` must
1060+
// be classifiable, and the only `true` results allowed are the
1061+
// two known credentials.
1062+
let sensitive: Vec<ConfigKey> = ConfigKey::all()
1063+
.iter()
1064+
.copied()
1065+
.filter(|k| k.is_sensitive())
1066+
.collect();
1067+
assert_eq!(
1068+
sensitive,
1069+
vec![
1070+
ConfigKey::ProviderGoogleApiKey,
1071+
ConfigKey::ProviderMapboxAccessToken,
1072+
]
1073+
);
1074+
}
1075+
1076+
#[test]
1077+
fn sensitive_value_mask_is_eight_x_chars() {
1078+
// The mask is treated as a constant by the CLI; if it changes,
1079+
// the change must be deliberate (and probably wants to update
1080+
// diagnostics redaction too).
1081+
assert_eq!(SENSITIVE_VALUE_MASK, "xxxxxxxx");
1082+
assert_eq!(SENSITIVE_VALUE_MASK.len(), 8);
1083+
}
1084+
10131085
#[test]
10141086
fn test_get_value() {
10151087
let config = ConfigFile::default();

‎xearthlayer/src/config/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ pub use file::{
100100
DEFAULT_RETRY_BASE_DELAY_MS,
101101
DEFAULT_WEB_API_PORT,
102102
};
103-
pub use keys::{ConfigKey, ConfigKeyError};
103+
pub use keys::{ConfigKey, ConfigKeyError, SENSITIVE_VALUE_MASK};
104104
pub use size::{format_size, parse_size, Size, SizeParseError};
105105
pub use storage::{
106106
DiskIoProfile, DEFAULT_CPU_FALLBACK, HDD_BLOCKING_CEILING, HDD_BLOCKING_SCALING_FACTOR,

‎xearthlayer/src/diagnostics/report.rs‎

Lines changed: 149 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -378,32 +378,84 @@ impl ConfigInfo {
378378
info.config_path = Some(config_path.display().to_string());
379379

380380
if let Ok(content) = fs::read_to_string(&config_path) {
381-
let mut redacted = String::new();
382-
for line in content.lines() {
383-
let trimmed = line.trim();
384-
if trimmed.is_empty() || trimmed.starts_with('#') || trimmed.starts_with(';') {
385-
continue;
386-
}
387-
if trimmed.to_lowercase().contains("api_key")
388-
|| trimmed.to_lowercase().contains("secret")
389-
|| trimmed.to_lowercase().contains("token")
390-
{
391-
if let Some(key) = trimmed.split('=').next() {
392-
redacted.push_str(&format!("{} = [REDACTED]\n", key.trim()));
393-
}
394-
} else {
395-
redacted.push_str(line);
396-
redacted.push('\n');
397-
}
398-
}
399-
info.config_contents = Some(redacted);
381+
info.config_contents = Some(redact_sensitive_ini(&content));
400382
}
401383
}
402384

403385
info
404386
}
405387
}
406388

389+
/// Track the current INI section while walking lines so we can construct
390+
/// `section.key` names and look them up via [`ConfigKey`].
391+
fn parse_section_header(line: &str) -> Option<&str> {
392+
let trimmed = line.trim();
393+
if trimmed.starts_with('[') && trimmed.ends_with(']') && trimmed.len() >= 2 {
394+
Some(&trimmed[1..trimmed.len() - 1])
395+
} else {
396+
None
397+
}
398+
}
399+
400+
/// Redact sensitive credential values from raw INI text by checking each
401+
/// `key = value` line against [`ConfigKey::is_sensitive`] and substituting
402+
/// [`SENSITIVE_VALUE_MASK`] for the value.
403+
///
404+
/// Section headers, comments, blank lines, non-sensitive entries, and
405+
/// entries with empty values are preserved verbatim — empty values aren't
406+
/// secrets and the absence is informative when reading a diagnostics
407+
/// dump. See issue #162 for why we drive both this and `config list`
408+
/// from the same `is_sensitive` predicate.
409+
fn redact_sensitive_ini(content: &str) -> String {
410+
use crate::config::{ConfigKey, SENSITIVE_VALUE_MASK};
411+
412+
let mut out = String::with_capacity(content.len());
413+
let mut current_section: Option<String> = None;
414+
415+
for line in content.lines() {
416+
if let Some(section) = parse_section_header(line) {
417+
current_section = Some(section.to_string());
418+
out.push_str(line);
419+
out.push('\n');
420+
continue;
421+
}
422+
423+
let trimmed = line.trim();
424+
if trimmed.is_empty() || trimmed.starts_with('#') || trimmed.starts_with(';') {
425+
out.push_str(line);
426+
out.push('\n');
427+
continue;
428+
}
429+
430+
let Some(eq_idx) = line.find('=') else {
431+
out.push_str(line);
432+
out.push('\n');
433+
continue;
434+
};
435+
436+
let key_part = line[..eq_idx].trim();
437+
let value_part = line[eq_idx + 1..].trim();
438+
let qualified = match &current_section {
439+
Some(s) => format!("{}.{}", s, key_part),
440+
None => key_part.to_string(),
441+
};
442+
443+
let is_sensitive = qualified
444+
.parse::<ConfigKey>()
445+
.map(|k| k.is_sensitive())
446+
.unwrap_or(false);
447+
448+
if is_sensitive && !value_part.is_empty() {
449+
out.push_str(&format!("{} = {}\n", key_part, SENSITIVE_VALUE_MASK));
450+
} else {
451+
out.push_str(line);
452+
out.push('\n');
453+
}
454+
}
455+
456+
out
457+
}
458+
407459
impl fmt::Display for SystemReport {
408460
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
409461
writeln!(f, "XEarthLayer Diagnostics")?;
@@ -687,4 +739,82 @@ mod tests {
687739
resolved.display()
688740
);
689741
}
742+
743+
#[test]
744+
fn redact_sensitive_ini_masks_provider_credentials() {
745+
let input = "\
746+
[provider]
747+
type = google
748+
google_api_key = AIzaTEST_KEY_NOT_REAL_xxxxxx
749+
mapbox_access_token = pk.eyJ1IjoidGVzdCJ9.fake
750+
";
751+
let redacted = redact_sensitive_ini(input);
752+
assert!(redacted.contains("type = google"));
753+
assert!(redacted.contains("google_api_key = xxxxxxxx"));
754+
assert!(redacted.contains("mapbox_access_token = xxxxxxxx"));
755+
assert!(!redacted.contains("AIzaTEST"));
756+
assert!(!redacted.contains("pk.eyJ1"));
757+
}
758+
759+
#[test]
760+
fn redact_sensitive_ini_preserves_empty_credential_values() {
761+
// Empty values aren't secrets and the absence is informative
762+
// when reading a diagnostics dump (e.g., "user hasn't set their
763+
// Google key yet, that's why google provider isn't working").
764+
let input = "[provider]\ngoogle_api_key =\n";
765+
let redacted = redact_sensitive_ini(input);
766+
assert!(
767+
redacted.contains("google_api_key ="),
768+
"empty value must pass through verbatim, got: {:?}",
769+
redacted
770+
);
771+
assert!(!redacted.contains("xxxxxxxx"));
772+
}
773+
774+
#[test]
775+
fn redact_sensitive_ini_preserves_non_credentials_and_structure() {
776+
let input = "\
777+
# top comment
778+
[cache]
779+
directory = /tmp/cache
780+
memory_size = 512MB
781+
782+
; section comment
783+
[provider]
784+
type = bing
785+
";
786+
let redacted = redact_sensitive_ini(input);
787+
// Comments, headers, and non-sensitive values all preserved.
788+
assert!(redacted.contains("# top comment"));
789+
assert!(redacted.contains("[cache]"));
790+
assert!(redacted.contains("directory = /tmp/cache"));
791+
assert!(redacted.contains("memory_size = 512MB"));
792+
assert!(redacted.contains("; section comment"));
793+
assert!(redacted.contains("[provider]"));
794+
assert!(redacted.contains("type = bing"));
795+
assert!(!redacted.contains("xxxxxxxx"));
796+
}
797+
798+
#[test]
799+
fn redact_sensitive_ini_passes_through_unknown_keys() {
800+
// Unknown keys (e.g., from a future XEL version, or user typos)
801+
// pass through unchanged. The exhaustive `is_sensitive` test in
802+
// config::keys guarantees we never miss a real sensitive variant.
803+
let input = "[provider]\nfuture_unknown_key = some_value\n";
804+
let redacted = redact_sensitive_ini(input);
805+
assert!(redacted.contains("future_unknown_key = some_value"));
806+
}
807+
808+
#[test]
809+
fn redact_sensitive_ini_only_masks_within_correct_section() {
810+
// A key literally named `google_api_key` outside `[provider]`
811+
// would be a parse error in ConfigKey, so passes through. This
812+
// proves we are NOT falling back to substring matching.
813+
let input = "\
814+
[cache]
815+
google_api_key = THIS_IS_NOT_A_REAL_KEY_AND_NOT_SENSITIVE
816+
";
817+
let redacted = redact_sensitive_ini(input);
818+
assert!(redacted.contains("THIS_IS_NOT_A_REAL_KEY_AND_NOT_SENSITIVE"));
819+
}
690820
}

0 commit comments

Comments
 (0)