Skip to content

Diagnostic bundle still ships PII and possible secrets in settings_redacted.json #204

Description

@pmaxhogan

Follow-up to #190, which fixed ONE field of this bug class. Found during the adversarial
review of that PR; filing durably because the remaining fields were deliberately scoped out
of #190 to keep its diff single-purpose and verifiable.

redact_settings() builds its output by cloning GlobalSettings verbatim. #190 stripped
basic-auth userinfo from proxy_url, but three sibling fields in the same struct still ship
raw inside the bundle that users hand to support:

  1. pre_backup_hook / post_backup_hook - free-text command lines. This is the worst of
    the three and is credential-class, not merely PII: command lines are a classic home for
    embedded secrets (curl -H "Authorization: ...", rclone flags, passwords passed as CLI
    args).
  2. custom_root_ca_path - an absolute path that routinely embeds the OS username. The
    rest of the bundle already hashes paths to <path:...>, so shipping this one verbatim is
    an internal inconsistency, not a judgement call.
  3. proxy_url in PAC mode - the field fix(net): redact proxy credentials from the diagnostic bundle #190 touched, but PAC mode makes it a local file
    path
    rather than a URL, so userinfo-stripping does not apply and the path (username and
    all) still goes out verbatim.

Note that the bundle's log Redactor does NOT catch any of these: it scrubs by prefix
(ya29., 1//, drive-id shapes, emails), so an arbitrary secret in a hook command or a
username in a path passes straight through.

Also worth fixing at the same time

ProxyError's Display embeds the full raw URL including userinfo
(crates/driven-tls/src/proxy.rs:64-75, InvalidUrl / UnsupportedScheme). This is latent
rather than live today - apply_proxy only errors on an invalid URL, and save-time validation
prevents persisting one, so the credentialed message currently reaches UI dialogs only, never
logs. But a single future tracing::warn!(%err) on a proxy error re-opens exactly the leak
#190 closed. Redact userinfo at construction or in Display so the trap cannot spring.

Suggested fix

Route the path fields through the existing path-hashing used elsewhere in the bundle, and
redact the hook commands wholesale (their presence is diagnostically useful; their contents
are not). Add a test asserting on the serialized JSON, as #190 does - the artifact that leaves
the machine is what matters.

Why this should not sit

The whole point of a diagnostic bundle is to be handed to a stranger. Every other secret-bearing
surface in it is scrubbed; these are the remaining holes in the same wall.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    • Status
      Done

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions