Skip to content

fix(daemon): mask the API token in the startup config log - #328

Merged
AnnatarHe merged 1 commit into
mainfrom
claude/laughing-wozniak-mbtnro
Oct 22, 2026
Merged

AnnatarHe merged 1 commit into
mainfrom
claude/laughing-wozniak-mbtnro

Conversation

@AnnatarHe

@AnnatarHe AnnatarHe commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Problem

At startup the daemon logs its whole config at Debug level to stdout (cmd/daemon/main.go:53):

slog.DebugContext(ctx, "daemon.config", slog.Any("config", cfg))

slog's text handler formats the struct with %+v, so the line contained Token:st_v1_... in plaintext. It also contained the tokens of any debug Endpoints and the exclude patterns. On Linux stdout goes to the systemd journal and on macOS to ~/.shelltime/logs/shelltime-daemon.log. It also ends up in any file stdout is redirected to, e.g. shelltime-daemon >>~/.shelltime/logs/shelltime-daemon.log.

This is the only place in the repo that logs the whole config (grep -rn 'slog.Any("config"').

Fix

  • ShellTimeConfig implements slog.LogValuer (model/config_log.go). The call in main.go is unchanged; any slog call that logs the config now writes a group of the fields worth seeing:

    • token and codeTracking.token are masked to their first and last 4 characters (st_v****…9876), using the existing maskToken.
    • proxy.url goes through RedactProxyURL.
    • exclude is logged only as a count (excludePatterns=2), because a pattern can contain the very secret it keeps out of history.
    • Debug endpoints are logged by URL only, without their tokens.
    • Nested configs (aiCodeOtel, codeTracking, logCleanup, storage, proxy, ai) are logged by value. Before, they printed as pointer addresses.
    • A new field stays out of the log until someone adds it to LogValue, so it is safe by default.
  • RedactProxyURL returned two kinds of input unchanged, password included:

    • a bare user:pass@host:port, which ParseProxyURL accepts;
    • a URL that doesn't parse, e.g. a password with an unescaped / or %.

    It now reads a URL without a scheme as http, like ParseProxyURL does, and otherwise hides everything before the last @. This also fixes the startup proxy enabled log line, shelltime doctor and shelltime config view.

Example startup line now:

level=DEBUG msg=daemon.config config.token=st_v*********************************9876 config.apiEndpoint=https://api.shelltime.xyz ... config.excludePatterns=2 config.aiCodeOtel.enabled=true config.aiCodeOtel.grpcPort=54027 config.proxy.url=user:xxxxx@proxy:3128 config.proxy.noProxy=[localhost]

Tests

  • TestShellTimeConfigLogValue_HidesSecrets logs a config holding secrets exactly the way the daemon does. It covers the text and JSON handlers, with the config passed both as a value and as a pointer. It asserts that no token, proxy password or exclude secret appears in the output, and that the useful fields do. With LogValue disabled, the test fails on the plaintext token.
  • TestShellTimeConfigLogValue_EmptyConfig checks that an empty config logs an empty token and <nil> for unset options.
  • TestRedactProxyURL adds cases for bare and unparseable URLs.
  • go vet ./... is clean (after mockery). go test ./cmd/... ./model/... ./commands/... passes.

Generated by Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

The daemon logs its whole config at Debug on stdout at startup with
slog.Any("config", cfg), which printed Token (and the tokens of debug
Endpoints and the exclude patterns) in plaintext to the systemd journal,
~/.shelltime/logs/shelltime-daemon.log, or wherever stdout was redirected.

ShellTimeConfig now implements slog.LogValuer, so any slog call that logs
it writes a group of the fields worth seeing instead: tokens masked (first
and last 4 characters), proxy URL redacted, exclude patterns only counted
(they may contain the very strings they keep out of history), debug
endpoints without their tokens, and nested configs by value rather than
as pointer addresses. New fields stay out of the log until added there.

RedactProxyURL returned a bare "user:pass@host:port" (accepted by
ParseProxyURL) and URLs that fail to parse (a password with an unescaped
'/' or '%') unchanged. It now reads a scheme-less URL as http, like
ParseProxyURL, and otherwise hides everything before the last '@'.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VqGMwLkjnydsiEvDrRcMt8
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-22T09:04:35.766709Z
We couldn’t complete this review. Comment @codex review to try again.
080ea5e PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions

Copy link
Copy Markdown
Contributor

⏱️ shelltime track performance

✅ No significant change in shelltime track latency

Comparing main @ d6d620a → #328 @ 080ea5e. Each scenario execs the binary the way the shell hooks do; values are the median of 10 interleaved rounds. Lower is better.

Scenario base head Δ p
Startup/floor 153 µs ±5% 150 µs ±7% ~ 0.353 A/A
Startup/version 1.81 ms ±7% 1.78 ms ±4% ~ 0.739 ✅
Track/daemon/pre 1.98 ms ±6% 2.08 ms ±7% ~ 0.123 ✅
Track/daemon/post 1.97 ms ±9% 1.97 ms ±8% ~ 0.579 ✅
Track/direct/pre 1.97 ms ±13% 1.91 ms ±12% ~ 1.000 ✅
Track/direct/post 3.87 ms ±14% 3.74 ms ±7% ~ 0.739 ✅
Track/direct/post-sync 4.96 ms ±5% 5.02 ms ±7% ~ 0.315 ✅

Binary size: 19.1 MiB → 19.2 MiB (+0.1%).

CPU, tail latency and memory

Per exec, base → head (Δ when significant).

Scenario p95 wall user CPU sys CPU peak RSS
Startup/floor 180 µs → 176 µs 135 µs → 132 µs 3 µs → 5 µs 9.2 MiB → 9.2 MiB
Startup/version 2.15 ms → 1.97 ms 1.37 ms → 1.37 ms 474 µs → 446 µs 15.3 MiB → 15.6 MiB (+2.19%)
Track/daemon/pre 2.35 ms → 2.55 ms 1.46 ms → 1.54 ms 567 µs → 578 µs 16.1 MiB → 16.5 MiB (+2.34%)
Track/daemon/post 2.26 ms → 2.43 ms 1.47 ms → 1.46 ms 540 µs → 563 µs 16.0 MiB → 16.4 MiB (+2.36%)
Track/direct/pre 2.34 ms → 2.34 ms 1.45 ms → 1.46 ms 527 µs → 507 µs 16.0 MiB → 16.4 MiB (+2.34%)
Track/direct/post 4.37 ms → 4.41 ms 2.90 ms → 2.75 ms 1.01 ms → 1.15 ms 17.3 MiB → 17.6 MiB (+1.74%)
Track/direct/post-sync 5.89 ms → 6.02 ms 3.55 ms → 3.76 ms 1.59 ms → 1.46 ms 19.1 MiB → 19.5 MiB (+1.90%)
benchstat
goos: linux
goarch: amd64
pkg: github.com/malamtime/cli/perf
cpu: AMD EPYC
                         │     base     │                head                 │
                         │    sec/op    │    sec/op     vs base               │
Startup/floor-2            153.3µ ±  5%   150.4µ ±  7%       ~ (p=0.353 n=10)
Startup/version-2          1.814m ±  7%   1.781m ±  4%       ~ (p=0.739 n=10)
Track/daemon/pre-2         1.981m ±  6%   2.076m ±  7%       ~ (p=0.123 n=10)
Track/daemon/post-2        1.968m ±  9%   1.974m ±  8%       ~ (p=0.579 n=10)
Track/direct/pre-2         1.970m ± 13%   1.908m ± 12%       ~ (p=1.000 n=10)
Track/direct/post-2        3.871m ± 14%   3.743m ±  7%       ~ (p=0.739 n=10)
Track/direct/post-sync-2   4.956m ±  5%   5.024m ±  7%       ~ (p=0.315 n=10)
geomean                    1.700m         1.690m        -0.55%

                         │     base     │                head                 │
                         │  p50-sec/op  │  p50-sec/op   vs base               │
Startup/floor-2            143.7µ ±  5%   144.0µ ±  4%       ~ (p=0.579 n=10)
Startup/version-2          1.764m ±  6%   1.744m ±  3%       ~ (p=0.481 n=10)
Track/daemon/pre-2         1.925m ±  4%   1.993m ±  8%       ~ (p=0.315 n=10)
Track/daemon/post-2        1.930m ±  9%   1.895m ±  8%       ~ (p=0.796 n=10)
Track/direct/pre-2         1.908m ± 12%   1.829m ± 16%       ~ (p=1.000 n=10)
Track/direct/post-2        3.758m ± 13%   3.639m ±  6%       ~ (p=0.579 n=10)
Track/direct/post-sync-2   4.835m ±  5%   4.932m ±  8%       ~ (p=0.123 n=10)
geomean                    1.645m         1.634m        -0.67%

                         │     base     │                head                 │
                         │  p95-sec/op  │  p95-sec/op   vs base               │
Startup/floor-2            179.7µ ± 13%   176.1µ ± 14%       ~ (p=0.481 n=10)
Startup/version-2          2.152m ± 13%   1.966m ± 21%       ~ (p=0.796 n=10)
Track/daemon/pre-2         2.350m ± 15%   2.553m ±  8%       ~ (p=0.052 n=10)
Track/daemon/post-2        2.256m ± 22%   2.433m ± 16%       ~ (p=0.436 n=10)
Track/direct/pre-2         2.335m ± 22%   2.345m ± 15%       ~ (p=0.684 n=10)
Track/direct/post-2        4.371m ± 22%   4.409m ± 11%       ~ (p=0.971 n=10)
Track/direct/post-sync-2   5.889m ±  8%   6.017m ± 13%       ~ (p=0.971 n=10)
geomean                    1.989m         2.013m        +1.18%

                         │     base     │                head                 │
                         │  peak-rss-B  │  peak-rss-B   vs base               │
Startup/floor-2            9.215Mi ± 1%   9.199Mi ± 2%       ~ (p=0.645 n=10)
Startup/version-2          15.26Mi ± 0%   15.60Mi ± 0%  +2.19% (p=0.000 n=10)
Track/daemon/pre-2         16.13Mi ± 0%   16.51Mi ± 0%  +2.34% (p=0.000 n=10)
Track/daemon/post-2        16.02Mi ± 0%   16.40Mi ± 0%  +2.36% (p=0.000 n=10)
Track/direct/pre-2         16.01Mi ± 0%   16.39Mi ± 0%  +2.34% (p=0.000 n=10)
Track/direct/post-2        17.31Mi ± 0%   17.61Mi ± 0%  +1.74% (p=0.000 n=10)
Track/direct/post-sync-2   19.13Mi ± 0%   19.49Mi ± 0%  +1.90% (p=0.000 n=10)
geomean                    15.26Mi        15.54Mi       +1.81%

                         │     base      │                 head                  │
                         │  sys-sec/op   │  sys-sec/op   vs base                 │
Startup/floor-2            3.300µ ± 110%   4.640µ ± 62%       ~ (p=0.968 n=9+10)
Startup/version-2          473.9µ ±  14%   446.2µ ±  9%       ~ (p=0.436 n=10)
Track/daemon/pre-2         567.4µ ±  29%   577.9µ ± 19%       ~ (p=0.353 n=10)
Track/daemon/post-2        540.1µ ±  25%   563.5µ ± 20%       ~ (p=0.481 n=10)
Track/direct/pre-2         526.7µ ±  30%   506.7µ ± 54%       ~ (p=0.739 n=10)
Track/direct/post-2        1.007m ±  38%   1.155m ± 20%       ~ (p=0.481 n=10)
Track/direct/post-sync-2   1.587m ±  17%   1.455m ± 19%       ~ (p=0.353 n=10)
geomean                    327.4µ          344.3µ        +5.16%

                         │     base     │                head                 │
                         │ user-sec/op  │ user-sec/op   vs base               │
Startup/floor-2            134.6µ ±  5%   132.0µ ±  5%       ~ (p=0.481 n=10)
Startup/version-2          1.373m ±  4%   1.367m ±  3%       ~ (p=0.971 n=10)
Track/daemon/pre-2         1.460m ± 13%   1.541m ±  9%       ~ (p=0.247 n=10)
Track/daemon/post-2        1.472m ±  4%   1.456m ± 13%       ~ (p=0.631 n=10)
Track/direct/pre-2         1.448m ±  6%   1.456m ±  7%       ~ (p=0.912 n=10)
Track/direct/post-2        2.896m ± 12%   2.753m ±  8%       ~ (p=0.529 n=10)
Track/direct/post-sync-2   3.549m ±  9%   3.764m ±  8%       ~ (p=0.143 n=10)
geomean                    1.289m         1.295m        +0.47%

                │    base     │
                │  sys-ns/op  │
Startup/floor-2   0.000 ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

Δ compares medians; ~ means no significant difference (Mann-Whitney U, p ≥ 0.05). A scenario is flagged when it is more than 10% and more than 250µs slower with p < 0.05. Startup/floor runs true, not shelltime: it is an A/A check of runner noise. Runner: linux/amd64, AMD EPYC. go1.27.1, 10 rounds × 100 execs per scenario and binary.

@codecov

codecov Bot commented Oct 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.53846% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
model/config_log.go 76.47% 12 Missing ⚠️
Flag Coverage Δ
unittests 86.18% <81.53%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
model/http_client.go 97.91% <100.00%> (+0.61%) ⬆️
model/config_log.go 76.47% <76.47%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AnnatarHe

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @AnnatarHe's task in 29s —— View job


Review of PR #328: mask the API token in the startup config log

This looks good. Making ShellTimeConfig a slog.LogValuer fixes the leak at its source, and new fields stay out of the log until someone adds them to LogValue. I only read the diff and did not run the tests. I found no blockers, just a few small things.

Minor issues

  1. A password containing :// leaks part of itself. This is in model/http_client.go, in the RedactProxyURL fallback. It looks for the first :// in the string, so an unparseable URL like user:pa://ss@host gives start = index of the :// inside the password. The result is user:pa://xxxxx@host, which exposes the username and the start of the password. Using the first :// only when it comes before any :/@ in the string would avoid this. A simpler option is to apply it only when raw matches ^[a-zA-Z][a-zA-Z0-9+.-]*://. A test case would be useful.
  2. A nil *ShellTimeConfig panics inside LogValue. LogValue has a value receiver, so a nil pointer dereferences when it is promoted to the pointer type. slog recovers from the panic and prints !PANIC, so nothing crashes, but a log line is lost. The daemon passes a non-nil config, so this is low risk. If you want it hardened, add a pointer-receiver LogValue with a nil check.
  3. Missing coverage. Codecov reports 12 lines missing in config_log.go, probably the ai, logCleanup and storage groups and the codeTracking token masking. The tests cover the secrets, so this is not urgent. Adding one fully populated config to the test would close the gap and also lock in that codeTracking.token is masked.
  4. The unparseable fallback drops the username too. For example, http://user:p/ss@host becomes http://xxxxx@host. This errs on the safe side and is fine. The doc comment says "everything between the scheme and the last '@'", which already describes it.

Looks good

  • Debug endpoints are logged by URL only, so their tokens never reach the log.
  • Logging exclude as a count only is the right call, since a pattern can contain the secret it keeps out of history.
  • boolPtrAttr logs the value instead of a pointer address.
  • Bare user:pass@host is now redacted. That also fixes doctor and config view.

@AnnatarHe
AnnatarHe merged commit 4d80820 into main Oct 22, 2026
5 checks passed
@AnnatarHe
AnnatarHe deleted the claude/laughing-wozniak-mbtnro branch October 22, 2026 09:14
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