Skip to content

Redact the webhook URL in every log line (v0.2.1) - #17

Merged
MichalAFerber merged 3 commits into
mainfrom
fix/redact-webhook-in-logs
Sep 16, 2026
Merged

MichalAFerber merged 3 commits into
mainfrom
fix/redact-webhook-in-logs

Conversation

@tgwab-claude

Copy link
Copy Markdown
Collaborator

The first CI run on this PR is a deliberate control and is expected to fail. HEAD currently carries a CONTROL: commit that disables the redaction, to prove the new tests actually discriminate. The next push reverts it and the run should go green.

What v0.2.0 actually did

PR #15 moved the notify token out of argv to satisfy §16, and #16 released it. It did not remove the token — it relocated it. src/lib.rs logged the full webhook URL at startup, so the token now lands in the journal instead:

  • argv dies with the process; the journal survives restarts and reboots, and is rotated and archived.
  • On the deployed host it is readable by an unprivileged user in adm via the ACL on /var/log/journal — the same class of reader as the original finding.

So the resting state after v0.2.0 was worse than before it. The token rides in the URL's ?token= query string, which means anything that prints the URL prints the credential.

The token is being rotated separately, because it has already been disclosed. This PR stops the disclosure recurring; it does not undo the one that happened.

The fix

A WebhookUrl newtype (src/webhook.rs) whose Display and Debug both render scheme://host:port#fingerprint. Getting the credential-bearing form takes an explicit expose(), which only the HTTP client calls — so this is safe by construction rather than by a discipline someone has to remember at the next tracing:: call. The path is dropped along with the query string, since some relays (ntfy) carry the secret as a path segment. The fingerprint is FNV-1a truncated to 32 bits: an identity check for an operator ("is this the endpoint I configured?"), not a secret.

Three sites, found by sweeping the crate rather than grepping for "webhook":

  1. The startup line (src/lib.rs) — logged the full URL. Now goes through webhook::log_field, still printing - when unset.
  2. The delivery-failure warning (src/alert.rs) — logged error = %e, and a reqwest error's Display appends the URL it was for, token and all. without_url() is load-bearing here; the redacted host is logged alongside so an operator still knows which endpoint failed. This is the redaction half of Let --webhook take its credential from a file or a header, not the URL #14. This path has never fired on the deployed host, so nothing had ever looked at what it prints — the test exercises it deliberately against a closed local port rather than assuming coverage.
  3. #[derive(Debug)] on Args and AlertConfig, both of which hold the URL and print every field. A grep for "webhook" would never have found these; the wrapper covers them for free, and a test pins Args.

Version bumped to 0.2.1 in Cargo.toml and Cargo.lock (that field only, hand-edited — no Rust toolchain on the build host).

Not in scope

Related to #14, not closing it: this is only the redaction half. #14's substantive ask — --webhook-token-file / --webhook-header so the credential stops riding in the URL at all — is untouched and still open.

No host was touched: no ssh, no journal reads, no deploy. No tag or release.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XE6Up1JPpFrhM5tC8hvHDE

tgwab-claude and others added 2 commits September 16, 2026 11:39
v0.2.0 moved the notify token out of argv (§16) but the startup line in
`src/lib.rs` logged the full webhook URL, query string and all, so the
credential landed in the journal instead — readable by the same
unprivileged user, and surviving restarts and reboots rather than dying
with the process.

Introduce `WebhookUrl`, a newtype whose `Display` and `Debug` both render
`scheme://host:port#fingerprint` and nothing else. Reaching the
credential-bearing form takes an explicit `expose()`, which only the HTTP
client calls, so present and future log sites are safe by construction
rather than by discipline. The path is dropped along with the query
string, because some relays carry the secret as a path segment.

Three sites, each with a sentinel test:

- the startup line, via `webhook::log_field`;
- the delivery-failure warning in `src/alert.rs`, which logged
  `error = %e` — a reqwest error's `Display` appends the URL it was for,
  so `without_url()` is load-bearing. This path has never fired on the
  deployed host, so the test exercises it deliberately against a closed
  local port rather than assuming it is covered. Related to #14;
- `#[derive(Debug)]` on `Args` and `AlertConfig`, both of which hold the
  URL and print every field.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XE6Up1JPpFrhM5tC8hvHDE
Temporary, reverted in the next commit. `Display`/`Debug`/`log_field`
print the full URL again and the failure warning drops `without_url()`,
so the sentinel tests should go red. A redaction test that passes without
the redaction certifies nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XE6Up1JPpFrhM5tC8hvHDE
Reverts the previous commit. With the redaction disabled, all three sites
failed as intended: `startup_log_field_hides_the_token`,
`failed_delivery_log_line_hides_the_token`, and
`debug_of_args_hides_the_webhook_value`, plus
`display_and_debug_both_redact` — 20 passed, 4 failed. The failure-path
output confirmed the mechanism: a reqwest error's Display really does
append the full URL, token and all.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XE6Up1JPpFrhM5tC8hvHDE
@MichalAFerber
MichalAFerber enabled auto-merge (squash) September 16, 2026 15:43
@MichalAFerber
MichalAFerber merged commit 9cadefa into main Sep 16, 2026
2 checks passed
@MichalAFerber
MichalAFerber deleted the fix/redact-webhook-in-logs branch September 16, 2026 15:44
@tgwab-claude

Copy link
Copy Markdown
Collaborator Author

Discrimination proof, both runs on this PR:

  • Red (control commit 13f11d2… redaction disabled): run 35116930174 — 20 passed; 4 failed. Failures: startup_log_field_hides_the_token, failed_delivery_log_line_hides_the_token, debug_of_args_hides_the_webhook_value, display_and_debug_both_redact — one per site. fmt and clippy were green, so the failure was the assertion, not the build.
  • Green (control reverted): run 35117203455 — 24 passed; 0 failed in the lib, 10 passed in tests/protocols.rs.

The red run also confirmed the delivery-failure mechanism rather than assuming it: with without_url() removed, the rendered warning was error=error sending request for url (http://127.0.0.1:1/notify/honeypot?token=FAILPATHSENTINEL…). A reqwest error's Display does append the full URL.

The reverting commit's tree is byte-identical to the fix commit (git diff --quiet HEAD~2 HEAD), so the control changed nothing that shipped.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants