Skip to content

fix(net): redact proxy credentials from the diagnostic bundle - #190

Merged
pmaxhogan merged 1 commit into
mainfrom
fix/proxy-credential-leak-bundle
Jul 29, 2026
Merged

fix(net): redact proxy credentials from the diagnostic bundle#190
pmaxhogan merged 1 commit into
mainfrom
fix/proxy-credential-leak-bundle

Conversation

@pmaxhogan

@pmaxhogan pmaxhogan commented Jul 29, 2026

Copy link
Copy Markdown
Owner

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.ai/code/session_01Qu8GxMwkuxF7JBzwRjtcw7

@pmaxhogan @claude
fix(net): redact proxy credentials from the diagnostic bundle
4e6dedf 
The SPEC s18 diagnostic bundle writes settings_redacted.json via
redact_settings(), which cloned `global` verbatim. GlobalSettings carries
the issue #34 `proxy_url`, and a manual-mode proxy URL may embed basic-auth
userinfo (http://user:password@proxy.corp:8080), so the password shipped in
plaintext inside a bundle explicitly meant to be shared with support.

Strip the userinfo, keeping scheme + host:port (the diagnostically useful
part). The helper is hand-rolled rather than url::Url-based so a URL the
parser rejects is still scrubbed instead of falling through to the raw
string.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qu8GxMwkuxF7JBzwRjtcw7
@github-project-automation github-project-automation Bot moved this to Todo in Driven Jul 29, 2026
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Coverage

Area main this PR delta
Rust (lib crates) 80.94% 80.96% +0.02 (OK)
UI (vue/ts) 91.40% 91.40% +0.00 (OK)

Gate: passed - no coverage regression (epsilon 0.1 pp).

@pmaxhogan
pmaxhogan merged commit 8e514f3 into main Jul 29, 2026
18 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in Driven Jul 29, 2026
@pmaxhogan
pmaxhogan deleted the fix/proxy-credential-leak-bundle branch July 29, 2026 16:57
This was referenced Jul 29, 2026
Merged
Closed
Merged
pmaxhogan added a commit that referenced this pull request Jul 29, 2026
@pmaxhogan @claude
fix(net): redact userinfo from PAC source in logs (#208)
8692bc0 
## The defect

A PAC source can be a URL carrying basic-auth credentials:

```
http://corpuser:hunter2@wpad.corp/proxy.pac
```

That string was written **verbatim** into:

- the PAC runtime warning (`PAC evaluation failed at request time...`),
- the refresh-failure warning added in #191,
- `ProxyConfig`'s `Debug` (the `Pac` arm printed `engine.source`),
- and every `ProxyError` `Display` message carrying a `url` /
`location`,
  which are logged *and* surfaced to the settings UI.

**This one is live, not latent.** Driven's rolling logs are collected
into the
SPEC s18 diagnostic bundle, and the bundle's `Redactor` scrubs by
prefix/shape -
OAuth tokens (`ya29.`, `1//`), drive-id shapes, emails, absolute paths.
None of
those patterns match a proxy password, so it would ride straight through
into a
bundle shared with support. Same defect class as #190, different escape
route:
#190 leaked it through the settings document, this leaks it through the
logs.

## The fix

Redact at the point of **writing**, never after:

- **`PacEngine` no longer retains the raw source.** The field exists
purely to
be printed, and refetching keys off the caller's own source string, so
the
engine stores only the redacted form. That fixes the runtime warning and
  `ProxyConfig`'s `Debug` at the root rather than at each print site.
- **The refresh-failure warning** redacts its source.
- **Every `ProxyError` variant** with a `url` / `location` redacts it in
  `Display`, via a `thiserror` format expression
(`#[error("...{}...", redact_userinfo(.url))]`). The struct fields keep
the
raw value, so redaction is a property of the *message*, not of the data.

**This covers the `ProxyError::Display` item from #204** - it fell out
naturally,
since that is the same set of messages. The other #204 items
(`custom_root_ca_path`, `pre`/`post_backup_hook`, PAC-mode `proxy_url`
as a local
path) are untouched and remain #204's scope.

### One deliberate cross-crate change

The redactor now lives in `driven-tls` as `redact_userinfo`, and the
copy added
to `src-tauri` in #190 delegates to it. It was written there first, but
the same
scrub is now needed in the proxy layer itself, and two copies of a
redaction rule
are two chances for one of them to drift. Behaviour is unchanged -
#190's tests
pass untouched against the shared implementation. Easy to revert to two
copies if
you would rather keep the crates independent.

## Tests

Four new tests in `crates/driven-tls/src/proxy.rs`:

- `redact_userinfo_handles_every_url_shape` - no-credential passthrough,
a bare
filesystem path (the PAC-from-disk case), 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.
- `pac_engine_never_retains_the_raw_source` - the engine's stored source
and
  `ProxyConfig`'s `Debug` are both clean, while the host survives for
  diagnostics.
- `proxy_error_display_redacts_credentials` - all six URL-bearing
variants, plus
an assertion that the underlying field is still unredacted (redaction is
a
  property of the message).
- `a_failing_pac_fetch_does_not_leak_credentials_in_its_message` -
drives a
**real** transport failure (loopback port 1 refuses instantly) rather
than a
synthetic string, then pins both halves of the resulting message
separately:

  ```
PAC file could not be fetched from
`http://<redacted>@127.0.0.1:1/p.pac`:
  error sending request for url (http://127.0.0.1:1/p.pac)
  ```

The first half is our `Display` doing its job. The second half is
reqwest's own
text, which embeds the URL but strips the userinfo itself - we depend on
that,
  so the test asserts it. A future reqwest that stopped stripping would
reintroduce the leak through that nested field, and this test is where
we would
  find out.

**Negative control**: with `redact_userinfo` neutered to return its
input
unchanged, all four new tests fail (36 passed / 4 failed) and pass again
once
restored - so none of them is vacuous.

Gates run locally: `cargo test --workspace` (1238 passed, 0 failed),
`cargo clippy --workspace --all-targets -- -D warnings` (clean),
`cargo fmt --all -- --check` (clean). No UI changes, so the pnpm gates
do not
apply. No dependency changes, so `cargo deny` is unaffected.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01Qu8GxMwkuxF7JBzwRjtcw7

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
pmaxhogan added a commit that referenced this pull request Jul 30, 2026
@pmaxhogan
chore(main): release 2.5.0 (#199)
81ec538 
🤖 I have created a release *beep* *boop*
---


## [2.5.0](v2.4.0...v2.5.0)
(2026-07-30)


### Features

* **cli:** import destinations from an existing rclone config
([#213](#213))
([baaf7bd](baaf7bd))
* **core:** enable macOS locked-file backup via the APFS snapshot broker
([#201](#201))
([ada822e](ada822e))
* **core:** local and removable-folder backup destination
([#212](#212))
([c416a24](c416a24))
* **core:** macOS APFS snapshot broker for locked files
([#196](#196))
([a5f105e](a5f105e))
* **core:** pluggable backup destination backends
([#200](#200))
([871df59](871df59))
* **core:** S3-compatible backup destination
([#207](#207))
([37acb03](37acb03))
* **core:** scheduled integrity scrub of remote objects
([#203](#203))
([049c62a](049c62a))
* **ui:** guide macOS users to grant Full Disk Access when files are
denied ([#216](#216))
([aa5327e](aa5327e))


### Bug Fixes

* **ci:** wait for MinIO readiness before the S3 integration suite
([#226](#226))
([fff471a](fff471a))
* **core:** classify macOS locked and permission-denied opens into the
skip-and-report path
([#195](#195))
([08d2864](08d2864))
* **core:** downgrade the APFS helper-dir check from fatal to advisory
([#211](#211))
([65010ac](65010ac))
* **net:** redact proxy credentials from the diagnostic bundle
([#190](#190))
([8e514f3](8e514f3))
* **net:** redact userinfo from PAC source in logs
([#208](#208))
([8692bc0](8692bc0))
* **net:** refresh stale PAC scripts instead of pinning them for the
process ([#191](#191))
([18b0d43](18b0d43))
* **scanner:** route the deep-verify hash through the platform-open
helper ([#193](#193))
([3af5c65](3af5c65))
* **ui:** do not offer versioning on destinations that cannot honour it
([#224](#224))
([857c8ba](857c8ba))
* **ui:** make the destination step backend-driven and stop copy
claiming Drive behaviour
([#219](#219))
([9d67765](9d67765))
* **ui:** tear down exclusion-preview listeners lost to an unmount race
([#206](#206))
([2656c9f](2656c9f))
* **ui:** use a template tray icon on macOS
([#202](#202))
([eaefa9a](eaefa9a))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

Driven
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@pmaxhogan