Skip to content

Commit 8e514f3

Browse files
pmaxhoganclaude
andauthored
fix(net): redact proxy credentials from the diagnostic bundle (#190)
## The defect The SPEC s18 diagnostic bundle writes `settings_redacted.json` via `redact_settings()`, which built its output with `global: s.global.clone()` - the whole `GlobalSettings` struct verbatim. `GlobalSettings` carries the issue #34 `proxy_url`. In `manual` mode that URL may embed basic-auth userinfo: ``` http://corpuser:hunter2@proxy.corp.example:8080 ``` So the proxy password shipped **in plaintext** inside a bundle whose entire purpose is to be handed to support. Every other secret-bearing surface in the bundle is scrubbed (logs, crashes, and the activity CSV all go through `Redactor`; the telemetry `install_id` is hashed) - `settings_redacted.json` was the hole, and its own doc comment claimed "the only redaction here is the install id". Found while verifying the already-shipped #145 proxy work (issue #34). ## The fix Strip the userinfo before serializing, keeping scheme + `host:port` because that is the diagnostically useful part of a proxy setting: ``` http://<redacted>@proxy.corp.example:8080 ``` `redact_proxy_userinfo` is hand-rolled rather than routed through `url::Url` deliberately: a URL the parser rejects must still get scrubbed, never fall through to emitting the raw string. The authority is everything between `://` and the first `/`, `?` or `#`; the **last** `@` in it splits userinfo from host (a password may itself contain `@`). ## Tests Two new unit tests in `src-tauri/src/commands/settings.rs`: - `redact_settings_strips_proxy_basic_auth_credentials` - asserts the username and password are gone, `host:port` survives, and (the load-bearing one) that the **serialized JSON** carries no secret, since that is the artifact that actually leaves the machine. - `redact_proxy_userinfo_handles_every_url_shape` - no-credential passthrough, socks5, username-only, a password containing `@`, path/query not mistaken for the authority, an `@` inside a path left alone, and unparseable-garbage-with- an-`@` redacted wholesale rather than passed through. Ran locally: `cargo test -p driven-app redact_` (12 passed), `cargo clippy -p driven-app --all-targets -- -D warnings` (clean), `cargo fmt --all -- --check` (clean). ## Scope note Two adjacent fields in the same struct are also cloned verbatim into the bundle and may carry user PII, but are **out of scope here** and reported separately rather than silently swept in: `custom_root_ca_path` (an absolute path that can contain the OS username, and which the rest of the bundle would have hashed to `<path:...>`) and `pre_backup_hook` / `post_backup_hook` (free-text commands). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Qu8GxMwkuxF7JBzwRjtcw7 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 6b02650 commit 8e514f3

1 file changed

Lines changed: 115 additions & 2 deletions

File tree

src-tauri/src/commands/settings.rs

Lines changed: 115 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2136,10 +2136,17 @@ struct RedactedTelemetry {
21362136
}
21372137

21382138
/// Redact the secret-bearing fields of a [`SettingsDto`] for the bundle
2139-
/// (SPEC s18): the telemetry install id becomes `installid_<hash>`.
2139+
/// (SPEC s18): the telemetry install id becomes `installid_<hash>`, and the
2140+
/// issue #34 proxy URL has any `user:password@` userinfo stripped.
21402141
fn redact_settings(s: &SettingsDto) -> RedactedSettings {
2142+
let mut global = s.global.clone();
2143+
// Issue #34 follow-up: a `manual`-mode proxy URL may embed basic-auth
2144+
// credentials (`http://user:password@proxy.corp:8080`). The bundle is meant
2145+
// to be shareable with support, so the userinfo must never ride along - the
2146+
// host:port survives because that is the diagnostically useful part.
2147+
global.proxy_url = global.proxy_url.as_deref().map(redact_proxy_userinfo);
21412148
RedactedSettings {
2142-
global: s.global.clone(),
2149+
global,
21432150
telemetry: RedactedTelemetry {
21442151
enabled: s.telemetry.enabled,
21452152
install_id: format!("installid_{}", stable_hash(&s.telemetry.install_id)),
@@ -2151,6 +2158,33 @@ fn redact_settings(s: &SettingsDto) -> RedactedSettings {
21512158
}
21522159
}
21532160

2161+
/// Strip any `user:password@` userinfo from a proxy URL, keeping the scheme and
2162+
/// `host:port` (issue #34 follow-up; see [`redact_settings`]).
2163+
///
2164+
/// Hand-rolled rather than routed through `url::Url` so a URL the parser rejects
2165+
/// still gets scrubbed: a parse failure must never fall through to emitting the
2166+
/// raw string. The authority is everything between `://` and the first `/`, `?`
2167+
/// or `#`; the LAST `@` in it separates userinfo from the host (a password may
2168+
/// itself contain an `@`).
2169+
fn redact_proxy_userinfo(raw: &str) -> String {
2170+
let Some(sep) = raw.find("://") else {
2171+
// No scheme separator: not a URL we can split, so we cannot prove there
2172+
// is no credential in it. Redact wholesale rather than leak.
2173+
return if raw.contains('@') {
2174+
"<redacted>".to_string()
2175+
} else {
2176+
raw.to_string()
2177+
};
2178+
};
2179+
let (scheme, rest) = raw.split_at(sep + 3);
2180+
let authority_end = rest.find(['/', '?', '#']).unwrap_or(rest.len());
2181+
let (authority, tail) = rest.split_at(authority_end);
2182+
match authority.rfind('@') {
2183+
Some(at) => format!("{scheme}<redacted>@{}{tail}", &authority[at + 1..]),
2184+
None => raw.to_string(),
2185+
}
2186+
}
2187+
21542188
/// A short, stable, NON-reversible hex hash of `input` for the bundle's redacted
21552189
/// ids (SPEC s18 `<hash>` placeholders). A 64-bit FNV-1a digest rendered as hex:
21562190
/// good enough to correlate occurrences WITHIN one bundle without exposing the
@@ -3105,6 +3139,85 @@ mod tests {
31053139
);
31063140
}
31073141

3142+
#[test]
3143+
fn redact_settings_strips_proxy_basic_auth_credentials() {
3144+
// Issue #34 follow-up: `settings_redacted.json` carried `global` verbatim,
3145+
// so a manual-mode proxy URL with `user:password@` userinfo shipped its
3146+
// password in plaintext inside a bundle meant to be shared with support.
3147+
let mut global = default_global();
3148+
global.proxy_mode = "manual".to_string();
3149+
global.proxy_url = Some("http://corpuser:hunter2@proxy.corp.example:8080".to_string());
3150+
let dto = SettingsDto {
3151+
global,
3152+
telemetry: TelemetrySettings {
3153+
enabled: false,
3154+
install_id: "id".to_string(),
3155+
endpoint: "https://e".to_string(),
3156+
},
3157+
updater: default_updater(),
3158+
ui: default_ui(),
3159+
windows: None,
3160+
bundle_small_files: false,
3161+
};
3162+
let red = redact_settings(&dto);
3163+
let url = red.global.proxy_url.as_deref().expect("proxy url present");
3164+
assert!(!url.contains("hunter2"), "the password must be gone: {url}");
3165+
assert!(
3166+
!url.contains("corpuser"),
3167+
"the username must be gone: {url}"
3168+
);
3169+
assert!(
3170+
url.contains("proxy.corp.example:8080"),
3171+
"host:port is diagnostically useful and survives: {url}"
3172+
);
3173+
3174+
// The serialized bundle document must not carry the secret either - this
3175+
// is the artifact that actually leaves the machine.
3176+
let json = serde_json::to_string(&red).expect("serialize");
3177+
assert!(
3178+
!json.contains("hunter2"),
3179+
"the bundle JSON must not carry the proxy password: {json}"
3180+
);
3181+
}
3182+
3183+
#[test]
3184+
fn redact_proxy_userinfo_handles_every_url_shape() {
3185+
// No credentials: untouched.
3186+
assert_eq!(
3187+
redact_proxy_userinfo("http://proxy.corp:8080"),
3188+
"http://proxy.corp:8080"
3189+
);
3190+
// socks5 with credentials.
3191+
assert_eq!(
3192+
redact_proxy_userinfo("socks5://u:p@127.0.0.1:1080"),
3193+
"socks5://<redacted>@127.0.0.1:1080"
3194+
);
3195+
// Username only, no password.
3196+
assert_eq!(
3197+
redact_proxy_userinfo("http://justuser@h:1"),
3198+
"http://<redacted>@h:1"
3199+
);
3200+
// A password containing '@' - the LAST '@' in the authority splits it.
3201+
assert_eq!(
3202+
redact_proxy_userinfo("http://u:p@ss@h:1"),
3203+
"http://<redacted>@h:1"
3204+
);
3205+
// A path/query must not be mistaken for the authority, and an '@' after
3206+
// the authority (in a path) is not userinfo.
3207+
assert_eq!(
3208+
redact_proxy_userinfo("http://u:p@h:1/path?q=1"),
3209+
"http://<redacted>@h:1/path?q=1"
3210+
);
3211+
assert_eq!(
3212+
redact_proxy_userinfo("http://h:1/pa@th"),
3213+
"http://h:1/pa@th"
3214+
);
3215+
// Unparseable garbage that still contains an '@' is redacted wholesale
3216+
// rather than passed through.
3217+
assert_eq!(redact_proxy_userinfo("u:p@h:1"), "<redacted>");
3218+
assert_eq!(redact_proxy_userinfo("not a url"), "not a url");
3219+
}
3220+
31083221
#[tokio::test]
31093222
async fn schema_summary_includes_real_user_version() {
31103223
// C3 (SPEC s18): schema.txt must carry the REAL PRAGMA user_version, not

0 commit comments

Comments
 (0)