Skip to content

fix(net): redact userinfo from PAC source in logs - #208

Merged
pmaxhogan merged 1 commit into
mainfrom
fix/pac-source-userinfo-logs
Jul 29, 2026
Merged

fix(net): redact userinfo from PAC source in logs#208
pmaxhogan merged 1 commit into
mainfrom
fix/pac-source-userinfo-logs

Conversation

@pmaxhogan

Copy link
Copy Markdown
Owner

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:

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

A PAC source can be a URL carrying basic-auth credentials
(http://user:pass@wpad.corp/proxy.pac). That string was written verbatim
into the PAC runtime warning, the refresh-failure warning, ProxyConfig's
Debug, and every ProxyError Display message - and Driven's rolling logs are
collected into the SPEC s18 diagnostic bundle. The bundle's Redactor scrubs
by prefix/shape (ya29., 1//, drive-id shapes, emails, paths), so it does NOT
catch a proxy password: this leak was live, not latent.

Redact at the point of writing:
- PacEngine stores only the redacted source. The field exists purely to be
  printed; refetching keys off the caller's own string, not this one.
- The refresh-failure warning redacts its source.
- Every ProxyError variant carrying a url/location redacts it in Display
  (via a thiserror format expression, so the struct fields keep the raw
  value for programmatic use). This also covers the #204 item about
  ProxyError::Display embedding the raw URL.

The redactor moves to driven-tls as `redact_userinfo` and the copy added to
src-tauri in #190 now delegates to it - two copies of a redaction rule are
two chances for one to drift.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qu8GxMwkuxF7JBzwRjtcw7
@github-actions

Copy link
Copy Markdown
Contributor

Coverage

Area main this PR delta
Rust (lib crates) 81.21% 81.24% +0.04 (OK)
UI (vue/ts) 91.39% 91.39% +0.00 (OK)

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

@pmaxhogan
pmaxhogan merged commit 8692bc0 into main Jul 29, 2026
18 checks passed
@pmaxhogan
pmaxhogan deleted the fix/pac-source-userinfo-logs branch July 29, 2026 18:02
@github-project-automation github-project-automation Bot moved this from Todo to Done in Driven Jul 29, 2026
pmaxhogan added a commit that referenced this pull request Jul 30, 2026
🤖 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).
pmaxhogan added a commit that referenced this pull request Aug 18, 2026
…dles

Closes #309, closes #204.

instead of cloning GlobalSettings and patching one field, so a future
secret-bearing field fails to compile here rather than leaking silently.
Fixes three concrete leaks: pre/post_backup_hook command lines (a classic
home for embedded secrets) are now redacted wholesale, custom_root_ca_path
is hashed like every other path in the bundle, and proxy_url in PAC mode
(a local file path, not a URL) is now hashed instead of passing the
userinfo-strip-only path through untouched. The ProxyError Display leak
the issue also flagged was already fixed by #208 - verified, not touched.

always-visible amber warning, backed by a real runtime-reloadable tracing
filter (logging.rs) that raises Driven's own crates to trace level while
on. The toggle persists an epoch-ms expiry and auto-turns-off 24h after
being enabled, enforced by a boot-time reconcile plus a periodic watchdog
(debug_mode.rs) so the window is honoured even across a restart. The
rolling log cap widens from 25 MB to 250 MB while debug mode is on. The
diagnostic bundle gains a DEBUG_MODE.txt notice and an unredacted
debug/engine_state.txt when debug mode is on - the one deliberate
exception to the #204 redaction rules, gated on the user's explicit
opt-in. Every bundle now also ships manifest.txt (entry name + size).
Activity's export button shows an amber "Debug data included" chip while
debug mode is on.

Also closes the long-documented gap where global.log_level only exported
RUST_LOG for the next launch - it now reloads the live filter too.

Testing: 18 Rust redaction tests (leak-shaped fixtures for hooks, CA path,
PAC-mode path, PAC-mode URL, and one full end-to-end fixture asserting the
serialized bundle JSON), 5 debug_mode watchdog/expiry tests, 5
settings-persistence round-trip tests. cargo test -p driven-app --lib:
458 passed. cargo clippy --workspace --all-targets -- -D warnings: clean.
cargo fmt --all -- --check: clean. pnpm vitest run: 793 passed across 60
files (new: activity-debug-chip.test.ts, plus PrivacyPage toggle tests in
settings-pages.test.ts). vue-tsc --noEmit: clean. Linux visual baselines
regenerated via `just visual-update` (privacy.png light+dark) and pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v
pmaxhogan added a commit that referenced this pull request Aug 18, 2026
…dles (#314)

## Summary

Closes #309, closes #204. Part of the v2.12.0 wave (PR5).

**#204 (diagnostic bundle PII/secret leaks)** - `redact_settings()` used
to clone `GlobalSettings` verbatim and patch only `proxy_url`. It now
builds a field-for-field `RedactedGlobalSettings` struct, so a future
secret-bearing field added to `GlobalSettings` fails to *compile* here
until someone decides how to redact it, instead of leaking silently.
Fixes the three concrete leaks the issue named:
- `pre_backup_hook` / `post_backup_hook` (command lines - a classic home
for embedded secrets) are now redacted wholesale (`<hook-redacted: N
chars>`), not shipped raw.
- `custom_root_ca_path` is now hashed through the same `<path:hash>`
scheme the rest of the bundle already uses.
- `proxy_url` in PAC mode (a local file path, not a URL) is now hashed
instead of only having userinfo-stripping applied (which never matched a
bare path).
- The issue's "also worth fixing" item (`ProxyError`'s `Display`
embedding raw userinfo) was already fixed by #208 - verified via `git
blame`, not touched again.

**#309 (debug logging mode)** - a new Settings > Privacy & Data toggle
("Debug logging") with an always-visible amber warning panel (shown
before the toggle is ever switched on, per the approved mockup), backed
by:
- A real runtime-reloadable tracing filter (`logging.rs`,
`tracing_subscriber::reload`) - flipping the toggle now actually changes
the live process's verbosity, no restart needed. This also closes a
long-documented gap where `global.log_level` only ever exported
`RUST_LOG` for the *next* launch; it now reloads the live filter too
(deferred while debug mode is active, so it doesn't undo the debug-mode
filter).
- A persisted epoch-ms expiry + a boot-time reconcile and periodic
watchdog (`debug_mode.rs`) that auto-turns the toggle off 24h after
enabling - honoured across a restart, not just while the app keeps
running. The watchdog is registered on `AppState` and joined by #312's
no-orphan quit drain (`ShutdownHandles`/`drain_shutdown_handles`), the
same pattern #311's bottleneck sampler uses.
- A rolling log cap that widens from 25 MB to 250 MB while debug mode is
on.
- The diagnostic bundle gains `DEBUG_MODE.txt` and an unredacted
`debug/engine_state.txt` while debug mode is on - the one deliberate
exception to the #204 redaction rules, gated on the user's explicit
opt-in (every other bundle file stays redacted regardless).
- Every bundle now also ships `manifest.txt` (entry name + size), a
small bundle-usefulness improvement.
- Activity's "Export diagnostic bundle" button shows an amber "Debug
data included" chip while debug mode is on.

## Also in this PR

- **Rebased onto `main`** after #310-#316 merged. Re-homed the
debug-mode watchdog from a detached `memlog.rs`-style task into #312's
`ShutdownHandles`/`drain_shutdown_handles` no-orphan quit drain (new
`DebugModeRuntime` on `AppState`,
`set_debug_mode_task`/`shutdown_debug_mode_task` mirroring
`set_bottleneck_task`/`shutdown_bottleneck_task`).
- **CodeQL `rust/path-injection` fix** (not a dismissal): two test
helpers (`settings.rs`'s pre-existing `seeded_repo()` and this PR's new
`debug_mode.rs` one) hand-rolled a temp dir via
`std::env::temp_dir().join(format!(...))` before feeding it to
`SqliteStateRepo::open` - exactly the pattern this repo's CodeQL rule
flags (see the `tempfile` dependency comment in `src-tauri/Cargo.toml`,
and PR 151 precedent). Switched both to `tempfile::tempdir().keep()`, an
opaque external call CodeQL's dataflow can't see into, so the taint
chain never forms.
- **Also carries the h2 advisory fix** (RUSTSEC-2026-0258, low severity,
unbounded empty DATA frames) - `cargo update -p h2` (0.4.15 -> 0.4.16),
lockfile-only, no `Cargo.toml` changes. This advisory is unrelated to
this PR's own diff (`git diff` against the pre-PR base shows zero
`Cargo.lock` changes before this commit) and would fail `cargo deny`
repo-wide on `main` too; landing it here unblocks this PR's `cargo deny`
check and delivers the fix to `main` in the same step.

## Test plan

- [x] `cargo test -p driven-app --lib` - 494 passed (18 #204 redaction
tests with leak-shaped fixtures, incl. one asserting the full serialized
bundle JSON end-to-end; 5 debug-mode watchdog/expiry tests; 6
settings-persistence round-trip tests; 1 `AppState` debug-mode
task/shutdown round-trip test)
- [x] `cargo clippy --workspace --all-targets -- -D warnings` - clean
- [x] `cargo fmt --all -- --check` - clean
- [x] `cargo build --workspace --tests` - clean
- [x] `cargo deny check` - clean (advisories ok, bans ok, licenses ok,
sources ok)
- [x] `pnpm lint` / `pnpm format:check` / `pnpm test:unit` (861 passed,
64 files) / `pnpm build` (vue-tsc + vite) - all clean, run in the CI
job's exact order
- [x] Linux visual baselines regenerated via `just visual-update`
(Docker) - `privacy.png` (light+dark) plus 9 `shell.spec.ts` baselines
(light+dark) that had drifted independently of this PR; all 106 visual
specs pass
- [x] README updated (Features list + comparison-table footnote ³⁴)

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

https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant