fix(logger): redact paths and scrub err.* before pino emits (closes #52) - #89
Conversation
![chrisleekr-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Configures the root pino instance with a path-based redact list and a custom err serializer so secret-bearing fields and free-text leakage (GitHub tokens, App JWTs, Valkey credentials) are scrubbed at the logging chokepoint instead of relying on per-call-site helpers. The redact list covers authorization headers (top-level and nested), the x-hub-signature-256 webhook signature, response.data.token, and the named credential fields from src/config.ts. The err serializer defers to pino.stdSerializers.err and then runs message/stack/headers/data through redactGitHubTokens (already exported from src/utils/sanitize.ts) plus an inline redis://user:pass@... scrubber that mirrors redactValkeyUrl. Operates on a copy so the original Error is never mutated. REDACT_PATHS and errSerializer are exported so tests can build an equivalent capturing logger and assert on the emitted JSON.
Builds a capturing pino logger using the exported REDACT_PATHS + errSerializer from src/logger.ts and asserts that App JWTs, ghs_ installation tokens, the x-hub-signature-256 webhook header, PEM private keys, response.data.token echoes, and Valkey URLs with embedded credentials never reach the emitted JSON. Includes a non-mutation assertion so future refactors cannot accidentally modify the original Error instance.
…point Adds a "Log redaction" section explaining the two layers (path-based + err-serializer scrubbing), citing src/logger.ts:17 and src/logger.ts:113 so operators know what will NOT appear in logs. Cross-links the point helpers redactGitHubTokens (src/utils/sanitize.ts:77) and redactValkeyUrl (src/orchestrator/valkey.ts:64) so the docs make clear the logger is now the system-wide default and the point helpers cover only their non-log call sites (prompt sanitisation and the Valkey startup info log).
Tracking summary used by the bot's `implement` workflow as the body of the in-issue tracking comment. Replaces the stale issue-#51 contents.
|
bot workflow review failed: review pipeline execution failed |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request implements credential redaction for the Pino logger to prevent secrets from leaking in structured logs. It adds path-based redaction configuration, a custom error serializer that scrubs sensitive fields from error messages and stacks, and comprehensive tests validating that GitHub tokens, app JWTs, and other credentials are properly redacted. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/operate/observability.md`:
- Around line 7-11: Replace fragile line-number citations with stable symbol
references: mention the exported root pino logger symbol (the module's canonical
logger instance), the logger's redact.paths setting name, the configured err
serializer (the err serializer function used when creating the logger), and the
sanitizer helpers `redactGitHubTokens` and `redactValkeyUrl`; update the text to
cite those symbol names instead of `src/logger.ts:174` and `src/logger.ts:113`
so future edits won't break the doc.
In `@src/logger.ts`:
- Around line 17-44: REDACT_PATHS is exported as a mutable array; make it
immutable by changing its type to a readonly array (e.g., use "readonly
string[]" or "ReadonlyArray<string>" for REDACT_PATHS) and freeze the runtime
value with Object.freeze so consumers cannot push/splice it; update the export
of REDACT_PATHS in src/logger.ts to use the readonly type and wrap the literal
with Object.freeze (or assign then freeze) to ensure both compile-time and
runtime immutability while keeping the same contents and export name.
- Around line 95-113: scrubResponseData currently only censors the literal
"token" key and lets other secret keys under err.response.data leak; update
scrubResponseData to check each entry key against the configured redact paths
(e.g., the same list used by the logger, referenced as redact.paths or
redactPaths) and replace any matching key (case-insensitively and supporting
nested dot-notation like "privateKey" or "installationToken") with CENSOR
instead of only handling "token"; preserve the existing fallback to
scrubString(v) for non-matching strings and keep using scrubString and CENSOR
symbols; add a regression test asserting that err.response.data.privateKey (and
a sample installationToken/webhookSecret) are censored by the serializer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff0150f8-29f4-4336-b1d8-1fb999f1aa43
📒 Files selected for processing (4)
IMPLEMENT.mddocs/operate/observability.mdsrc/logger.tstest/utils/logger.test.ts
| The root pino instance at `src/logger.ts:174` is the canonical chokepoint for secret scrubbing — every child logger inherits its `redact.paths` list and its custom `err` serializer, so individual call sites do not need to remember to scrub. Two layers run on every emitted line: | ||
|
|
||
| 1. **Path-based redaction** (`src/logger.ts:17`) — pino replaces matching field values with `[Redacted]` before the JSON is serialised. Paths covered: `authorization` and its `*.authorization` / `headers.authorization` / `*.headers.authorization` / `req.headers.authorization` / `request.headers.authorization` variants; the webhook signature header `x-hub-signature-256` (also wildcard-prefixed); `response.data.token`; and the named credential fields `token`, `installationToken`, `privateKey`, `webhookSecret`, `anthropicApiKey`, `claudeCodeOauthToken`, `daemonAuthToken`, `awsSecretAccessKey`, `awsSessionToken`, `awsBearerTokenBedrock`, `*.password`. | ||
|
|
||
| 2. **`err` serializer scrubbing** (`src/logger.ts:113`) — defers to pino's `stdSerializers.err` and then runs the result's `message`, `stack`, `request.headers.*`, and `response.data` through `redactGitHubTokens` (`src/utils/sanitize.ts:77`) plus an inline credential-URL scrubber that mirrors `redactValkeyUrl` (`src/orchestrator/valkey.ts:64`). This catches free-text leakage that path-based rules cannot match — notably an Octokit `RequestError` whose `err.request.headers.authorization` sits 4 segments below the log root, and `ghs_…` installation tokens echoed inside `err.message` / `err.stack`. |
There was a problem hiding this comment.
Update these src/logger.ts citations before merge.
src/logger.ts:174 and src/logger.ts:113 no longer point at the root logger and errSerializer in the current file, so this doc is already sending readers to the wrong place. Please refresh the line numbers here or switch to symbol-name references so the guidance does not drift on the next edit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/operate/observability.md` around lines 7 - 11, Replace fragile
line-number citations with stable symbol references: mention the exported root
pino logger symbol (the module's canonical logger instance), the logger's
redact.paths setting name, the configured err serializer (the err serializer
function used when creating the logger), and the sanitizer helpers
`redactGitHubTokens` and `redactValkeyUrl`; update the text to cite those symbol
names instead of `src/logger.ts:174` and `src/logger.ts:113` so future edits
won't break the doc.
There was a problem hiding this comment.
Valid (minor). Fixed in da9f215 — switched to symbol-name references (logger, REDACT_PATHS, errSerializer) in docs/operate/observability.md so the doc no longer carries fragile line numbers that drift on the next refactor inside src/logger.ts. The path-anchor citation guard only validates in-range, so this kind of drift would have slipped through again — the symbol-name form sidesteps that entirely.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| export const REDACT_PATHS: string[] = [ | ||
| // Generic auth tokens — Octokit RequestError carries these on err.request.headers | ||
| "authorization", | ||
| "*.authorization", | ||
| "headers.authorization", | ||
| "*.headers.authorization", | ||
| "req.headers.authorization", | ||
| "request.headers.authorization", | ||
| // Webhook signature header — octokit lowercases incoming header names | ||
| 'headers["x-hub-signature-256"]', | ||
| '*.headers["x-hub-signature-256"]', | ||
| 'req.headers["x-hub-signature-256"]', | ||
| 'request.headers["x-hub-signature-256"]', | ||
| // GitHub 401 bodies sometimes echo a token field | ||
| "response.data.token", | ||
| // Generic credential fields used throughout the codebase | ||
| "token", | ||
| "installationToken", | ||
| "privateKey", | ||
| "webhookSecret", | ||
| "anthropicApiKey", | ||
| "claudeCodeOauthToken", | ||
| "daemonAuthToken", | ||
| "awsSecretAccessKey", | ||
| "awsSessionToken", | ||
| "awsBearerTokenBedrock", | ||
| "*.password", | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Freeze the exported redact-path policy.
REDACT_PATHS is the canonical security control here, but it is exported by mutable reference. Any accidental push/splice from another import will silently change both the runtime logger behavior and the test helper configuration. Make it readonly at the type level and frozen at runtime.
Suggested change
-export const REDACT_PATHS: string[] = [
+export const REDACT_PATHS: readonly string[] = Object.freeze([
// ...
-];
+]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/logger.ts` around lines 17 - 44, REDACT_PATHS is exported as a mutable
array; make it immutable by changing its type to a readonly array (e.g., use
"readonly string[]" or "ReadonlyArray<string>" for REDACT_PATHS) and freeze the
runtime value with Object.freeze so consumers cannot push/splice it; update the
export of REDACT_PATHS in src/logger.ts to use the readonly type and wrap the
literal with Object.freeze (or assign then freeze) to ensure both compile-time
and runtime immutability while keeping the same contents and export name.
There was a problem hiding this comment.
Valid (major). Fixed in da9f215 — REDACT_PATHS is now readonly string[] = Object.freeze([...]), so a stray push/splice from another module trips a TypeError at runtime instead of silently weakening the policy. Pino's redact.paths is typed as mutable string[], so the consumer side spreads into a fresh array ({ paths: [...REDACT_PATHS] }) — pino owns its own copy, the canonical export stays frozen. Regression test freezes the exported REDACT_PATHS list at runtime added in test/utils/logger.test.ts.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| /** | ||
| * Scrub a `response.data` object on an Octokit error: `data.token` is | ||
| * replaced wholesale (mirrors the path-based `response.data.token` rule | ||
| * for the err-namespace case), and other string values are passed through | ||
| * the GitHub-token / URL-credential regex. | ||
| */ | ||
| function scrubResponseData(data: Record<string, unknown>): Record<string, unknown> { | ||
| const out: Record<string, unknown> = {}; | ||
| for (const [k, v] of Object.entries(data)) { | ||
| /* eslint-disable security/detect-object-injection -- key originates from Object.entries on the err serializer output; not user-controlled. */ | ||
| if (k === "token") { | ||
| out[k] = CENSOR; | ||
| } else { | ||
| out[k] = typeof v === "string" ? scrubString(v) : v; | ||
| } | ||
| /* eslint-enable security/detect-object-injection */ | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
Redact all configured secret keys under err.response.data, not just token.
Line 105 only censors "token". A payload like err.response.data.privateKey, installationToken, webhookSecret, or awsSecretAccessKey will currently survive unchanged because root redact.paths does not reach through err.*, and scrubString() only catches GitHub-token patterns / credential URLs. That leaves a real leak path in the exact namespace this serializer is supposed to close.
Suggested direction
+const NESTED_SENSITIVE_KEYS = new Set([
+ "token",
+ "installationToken",
+ "privateKey",
+ "webhookSecret",
+ "anthropicApiKey",
+ "claudeCodeOauthToken",
+ "daemonAuthToken",
+ "awsSecretAccessKey",
+ "awsSessionToken",
+ "awsBearerTokenBedrock",
+ "password",
+]);
+
+function scrubStructuredValue(value: unknown): unknown {
+ if (typeof value === "string") {
+ return scrubString(value);
+ }
+ if (Array.isArray(value)) {
+ return value.map(scrubStructuredValue);
+ }
+ if (value !== null && typeof value === "object") {
+ return scrubResponseData(value as Record<string, unknown>);
+ }
+ return value;
+}
+
function scrubResponseData(data: Record<string, unknown>): Record<string, unknown> {
const out: Record<string, unknown> = {};
for (const [k, v] of Object.entries(data)) {
- if (k === "token") {
+ if (NESTED_SENSITIVE_KEYS.has(k)) {
out[k] = CENSOR;
} else {
- out[k] = typeof v === "string" ? scrubString(v) : v;
+ out[k] = scrubStructuredValue(v);
}
}
return out;
}Please also add a regression test for something like err.response.data.privateKey.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/logger.ts` around lines 95 - 113, scrubResponseData currently only
censors the literal "token" key and lets other secret keys under
err.response.data leak; update scrubResponseData to check each entry key against
the configured redact paths (e.g., the same list used by the logger, referenced
as redact.paths or redactPaths) and replace any matching key (case-insensitively
and supporting nested dot-notation like "privateKey" or "installationToken")
with CENSOR instead of only handling "token"; preserve the existing fallback to
scrubString(v) for non-matching strings and keep using scrubString and CENSOR
symbols; add a regression test asserting that err.response.data.privateKey (and
a sample installationToken/webhookSecret) are censored by the serializer.
There was a problem hiding this comment.
Valid (major). Fixed in da9f215 — replaced the scrubResponseData/scrubHeaders pair with a single scrubStructured walker that checks every key against SENSITIVE_FIELD_NAMES_LC (lower-cased copy of every bare-name entry in REDACT_PATHS: token, installationToken, privateKey, webhookSecret, anthropicApiKey, claudeCodeOauthToken, daemonAuthToken, awsSecretAccessKey, awsSessionToken, awsBearerTokenBedrock, password, authorization, x-hub-signature-256). Regression test redacts non-token sensitive keys directly under err.response.data added — it plants privateKey, installationToken, and webhookSecret and asserts each is replaced with [Redacted].
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
|
bot workflow 🔍 Code review complete — 4 files, +446/-86. Review — PR #89: fix(logger): redact paths and scrub err.* before pino emits (closes #52)SummaryThe PR moves secret scrubbing into the root pino logger via path-based What was checkedFiles read in full:
Cross-references performed:
Validations run:
Findings
|
![chrisleekr-bot[bot]](https://avatars.githubusercontent.com/in/2890262?s=60&v=4){kind=small}
| const out: Record<string, unknown> = {}; | ||
| for (const [k, v] of Object.entries(data)) { | ||
| /* eslint-disable security/detect-object-injection -- key originates from Object.entries on the err serializer output; not user-controlled. */ | ||
| if (k === "token") { |
There was a problem hiding this comment.
[major] scrubResponseData only redacts the literal "token" key — but the redact-paths list above (installationToken, privateKey, webhookSecret, claudeCodeOauthToken, daemonAuthToken, awsBearerTokenBedrock, awsSecretAccessKey, awsSessionToken, anthropicApiKey, *.password) is supposed to be the canonical list of secret-bearing field names. Pino's path-based redact.paths cannot match those names when they sit four segments deep on err.response.data.*, so the err serializer is the only thing left to catch them — and right now it doesn't.
Verified by direct test against the exported errSerializer: an Octokit-shaped error with response.data: { privateKey: "MIIEowIBAAKCAQEA_SECRET_PEM" } emits the PEM verbatim in JSON output. Same result for installationToken, webhookSecret, etc. Anything that's not the literal string "token" and isn't a ghX_…-format GitHub token survives.
Fix: check the key against a Set of all sensitive names (the same names already enumerated in REDACT_PATHS, minus the path-syntax decorations), and [Redacted] on hit. CodeRabbit's suggested patch on this hunk is essentially correct. Please add a regression test for err.response.data.privateKey (or installationToken) to test/utils/logger.test.ts so this doesn't silently regress.
There was a problem hiding this comment.
Valid (major). Same root cause as the CodeRabbit thread above — fixed in da9f215. The new scrubStructured walker checks each key against SENSITIVE_FIELD_NAMES_LC, sourced from the bare-name entries in REDACT_PATHS (kept in a sibling list with a doc-comment that calls out the sync requirement). Regression coverage: the new redacts non-token sensitive keys directly under err.response.data test covers privateKey / installationToken / webhookSecret exactly as suggested.
| if (k === "token") { | ||
| out[k] = CENSOR; | ||
| } else { | ||
| out[k] = typeof v === "string" ? scrubString(v) : v; |
There was a problem hiding this comment.
[major] Neither scrubResponseData nor scrubHeaders recurses into nested objects, so err.response.data.meta.token = "ghs_…" survives. The for/of Object.entries loop above only does out[k] = scrubString(v) for string values; an object value passes through untouched.
Verified: emitting an Octokit-shaped error with response.data: { meta: { token: "ghs_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" } } leaks the token verbatim. The path-based response.data.token rule doesn't help here either — it requires token to be a direct child of response.data, not nested.
Fix: walk objects/arrays recursively (a small scrubStructured(value) that returns scrubString(string) / array.map(scrubStructured) / Object.fromEntries(Object.entries(o).map(...))) so a nested token / privateKey / etc. still gets [Redacted]. This pairs with the previous comment about the sensitive-keys set; one helper covers both.
There was a problem hiding this comment.
Valid (major). Fixed in da9f215 — scrubStructured recurses through arrays (via map) and plain objects (via Object.entries), so err.response.data.meta.token, err.response.data.meta.details.awsSecretAccessKey, and err.request.headers.forwarded.authorization are all caught at any depth. Two new regression tests cover the response-data and request-headers nested cases. Existing non-mutation test still passes (the walker returns fresh objects/arrays at every level).
|
|
||
| 1. **Path-based redaction** (`src/logger.ts:17`) — pino replaces matching field values with `[Redacted]` before the JSON is serialised. Paths covered: `authorization` and its `*.authorization` / `headers.authorization` / `*.headers.authorization` / `req.headers.authorization` / `request.headers.authorization` variants; the webhook signature header `x-hub-signature-256` (also wildcard-prefixed); `response.data.token`; and the named credential fields `token`, `installationToken`, `privateKey`, `webhookSecret`, `anthropicApiKey`, `claudeCodeOauthToken`, `daemonAuthToken`, `awsSecretAccessKey`, `awsSessionToken`, `awsBearerTokenBedrock`, `*.password`. | ||
|
|
||
| 2. **`err` serializer scrubbing** (`src/logger.ts:113`) — defers to pino's `stdSerializers.err` and then runs the result's `message`, `stack`, `request.headers.*`, and `response.data` through `redactGitHubTokens` (`src/utils/sanitize.ts:77`) plus an inline credential-URL scrubber that mirrors `redactValkeyUrl` (`src/orchestrator/valkey.ts:64`). This catches free-text leakage that path-based rules cannot match — notably an Octokit `RequestError` whose `err.request.headers.authorization` sits 4 segments below the log root, and `ghs_…` installation tokens echoed inside `err.message` / `err.stack`. |
There was a problem hiding this comment.
[minor] These two citations point at the wrong lines in the current src/logger.ts. src/logger.ts:174 is a blank line — the root pino instance starts at src/logger.ts:193. src/logger.ts:113 is the closing } of scrubResponseData — errSerializer starts at src/logger.ts:152. The scripts/check-docs-citations.ts guard only verifies that the line is in-range, so it won't catch this kind of drift.
Fix: either bump the line numbers (193 for the root logger, 152 for errSerializer) and the path-based-redaction line is fine at 17, or — preferably — switch to symbol-name references ("the root logger / REDACT_PATHS / errSerializer in src/logger.ts") so the prose doesn't break the next time someone shifts a function inside this file. Same fix applies to the citation on the next line.
There was a problem hiding this comment.
Valid (minor). Fixed in da9f215 — both citations on docs/operate/observability.md are now symbol-name references (logger, REDACT_PATHS, errSerializer) rather than src/logger.ts:<line> anchors, so the prose doesn't break the next time someone shifts a function inside that file. The check-docs-citations.ts guard still passes because the surviving cross-file citations to src/utils/sanitize.ts and src/orchestrator/valkey.ts were also converted to bare paths.
|
bot workflow 🔎 Resolve iteration complete — 0 failing checks, 6 open comment threads (some may already be resolved). Resolve iteration — PR #89 (
|
| Comment ID | File:line | Severity | Classification | Action |
|---|---|---|---|---|
| 3175419635 | docs/operate/observability.md:11 |
Minor | Valid | Switched to symbol-name references; reply 3175768663 |
| 3175419639 | src/logger.ts:44 |
Major | Valid | REDACT_PATHS is now readonly + Object.freezed; pino consumer spreads into fresh array. Reply 3175768705 |
| 3175419644 | src/logger.ts:113 |
Major | Valid | New SENSITIVE_FIELD_NAMES_LC set replaces literal "token" check; regression test added. Reply 3175769308 |
| 3175745708 | src/logger.ts:105 |
Major | Valid (dup) | Same root cause as 3175419644; same fix covers it. Reply 3175769384 |
| 3175745937 | src/logger.ts:108 |
Major | Valid | New scrubStructured walker recurses through arrays/objects; two new tests cover nested response.data + request.headers cases. Reply 3175769438 |
| 3175746154 | docs/operate/observability.md:11 |
Minor | Valid (dup) | Same as 3175419635; symbol-name references applied to both citations. Reply 3175769477 |
Commits pushed
da9f215· fix(logger): recurse + freeze redact policy, drop wrong doc citations
Touches: src/logger.ts (+108/-49), test/utils/logger.test.ts (+108/-1), docs/operate/observability.md (+3/-3). 12/12 logger tests pass; 30/30 sanitize regression suite pass; typecheck clean; lint 0 errors / 289 warnings (was 277 — 12 added by the new test cases, all @typescript-eslint/explicit-function-return-type style warnings on inline destructuring like the rest of the test suite); prettier clean; both docs guards (check-docs-citations.ts, check-docs-versions.ts) clean.
Outstanding
Waiting on CI rerun to confirm green. No further code changes planned. reviewDecision is empty (no formal approval state) — merge is a human action per FR-017.
cost: $4.5747 · turns: 65 · duration: 657s
Closes the residual leak path flagged on #89 review: - `scrubResponseData` only matched the literal `"token"` key, so any other secret-bearing field name from REDACT_PATHS (`installationToken`, `privateKey`, `webhookSecret`, etc.) sitting on `err.response.data.*` survived. Pino's path-based `redact.paths` cannot reach four-segments-deep on `err.*`, so the `err` serializer is the only chokepoint. - Neither `scrubHeaders` nor `scrubResponseData` recursed into nested objects, so `err.response.data.meta.token` and `err.request.headers.forwarded.authorization` leaked. Replace both with a single `scrubStructured` walker that recurses through strings/arrays/objects and replaces any key whose lower-cased name is in `SENSITIVE_FIELD_NAMES_LC` (the same names enumerated in REDACT_PATHS, minus the path-syntax decorations). Mark `REDACT_PATHS` as `readonly` and `Object.freeze` it so accidental `push`/`splice` from another module can't silently weaken the policy. Spread to a fresh array where pino consumes it because pino's `redact.paths` typing is mutable. Switch the observability docs to symbol-name references (`logger`, `REDACT_PATHS`, `errSerializer`) so prose doesn't drift on the next refactor; the `check-docs-citations.ts` guard only verifies in-range, not anchor text. Adds 4 regression tests: - `err.response.data.{privateKey,installationToken,webhookSecret}` redacted. - nested `err.response.data.meta.{token,privateKey,details.awsSecretAccessKey}` redacted at any depth. - nested `err.request.headers.forwarded.authorization` redacted. - `Object.isFrozen(REDACT_PATHS)` is true. 12/12 logger tests pass. Sanitize regression suite still 30/30. Co-Authored-By: chrisleekr-bot[bot] <chrisleekr-bot[bot]@users.noreply.github.com>
# [1.8.0](v1.7.0...v1.8.0) (2026-05-02) ### Bug Fixes * **logger:** redact paths and scrub err.* before pino emits (closes [#52](#52)) ([#89](#89)) ([641f138](641f138)) * **security:** redact raw error messages from public PR comments ([#90](#90)) ([cc70949](cc70949)) ### Features * **workflows:** unify bot reply format and harden research/resolve guards ([#91](#91)) ([7d39fb4](7d39fb4))
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
Configures the root pino instance with path-based redaction and a composed
errserializer so secrets carried on OctokitRequestErrorinstances (App JWTs,ghs_…installation tokens,x-hub-signature-256webhook signatures) and free-text token leakage in error messages, stacks, and Valkey URLs are scrubbed at the logging chokepoint instead of relying on per-call-site helpers. Reuses the already-testedredactGitHubTokensregex fromsrc/utils/sanitize.ts:77-89and adds an inlinescheme://user:pass@hostscrubber that mirrorsredactValkeyUrl. Closes the information-disclosure gap reported in #52 — no new npm dependencies, no env-var changes, no edits to the ~27 callers oflogger.*.Changes
REDACT_PATHSlist insrc/logger.tscovering every named field the issue called out:authorization(top-level +*.headers.*/req.headers.*/request.headers.*variants),headers["x-hub-signature-256"](and wildcard forms — bracket syntax because pino path syntax can't dotted-split a hyphenated key),response.data.token, plustoken,installationToken,privateKey,webhookSecret,anthropicApiKey,claudeCodeOauthToken,daemonAuthToken,awsBearerTokenBedrock,awsSecretAccessKey,awsSessionToken, and*.password.errserializer that defers topino.stdSerializers.errand then runsredactGitHubTokens+ an inline credential-URL regex overmessage,stack,request.headers, andresponse.data. Runs on a copy so the original Error is never mutated. Required because pino's*.foo.barwildcards only match 3-segment paths, whileerr.request.headers.authorizationis 4 segments deep.REDACT_PATHSanderrSerializerexported so tests can rebuild the same configuration against an in-memory destination.test/utils/logger.test.ts(8 cases) covering App JWT,ghs_token in message + stack, top-levelprivateKey,x-hub-signature-256, Valkey URL credentials in message, non-mutation of the original Error,response.data.token, and the non-error pass-through branch.docs/operate/observability.mddocumenting both layers and noting the logger is now the canonical chokepoint; cross-links the point helpersredactGitHubTokensandredactValkeyUrlfor their remaining non-log call sites.Files changed
src/logger.ts· AddsREDACT_PATHS+ composederrSerializer, wires both into the root pino instance, exports them for tests.test/utils/logger.test.ts· New file — eight unit tests covering every leak vector from the issue.docs/operate/observability.md· New "Log redaction" section citingsrc/logger.ts:17andsrc/logger.ts:113.IMPLEMENT.md· Bot tracking-comment body for issue security(observability): pino logger has no redact paths so octokit error stacks can leak App JWTs and installation tokens #52.Commits
36a23bf· fix(logger): redact paths and scrub err.* before pino emits854c8df· test(logger): cover redact paths and err serializer scrubbing8e13042· docs(observability): document the logger as canonical redaction chokepoint6726c46· chore: refresh IMPLEMENT.md for issue security(observability): pino logger has no redact paths so octokit error stacks can leak App JWTs and installation tokens #52Tests run
bun run typecheck· clean (0 errors)bun run lint· 0 errors / 277 warnings (identical to pre-change baseline of 277)bun run format·All matched files use Prettier code style!bun test test/utils/logger.test.ts· 8 pass / 0 fail / 25 expect() callsbun test test/utils/sanitize.test.ts· 30 pass / 0 fail (regression check)bun run scripts/check-docs-citations.ts· OK (everysrc/<path>:<line>citation in docs is in-range)bun run scripts/check-docs-versions.ts· OK (Bun version pins agree across.tool-versions,package.json, Dockerfiles)mkdocs build --strict· cleanThe wider
bun testsuite has 186 fail / 24 errors that are infrastructure-dependent (require real Postgres + Valkey) and were present onmainbefore this PR — verified bygit stash && bun testbaseline of 187 fail / 25 errors. This PR actually removes one failure and one error.Verification
src/logger.ts:17enumerates every path requested in the issue. The list lives next to the logger so a new secret-bearing config field added insrc/config.tsis one place to update.src/logger.ts:131defers topino.stdSerializers.errand only then runs string-scrubbers, preserving downstream tooling compatibility while catching the four-segment-deeperr.request.headers.authorizationpath that pino's wildcard syntax cannot reach. Verified by theredacts request.headers.authorization carrying an App JWTtest.redactCredentialUrlsinsrc/logger.ts:54is invoked byscrubString, which the err serializer applies tomessage,stack, and string values insiderequest.headers/response.data. The point helper atsrc/orchestrator/valkey.ts:64stays in place for the info-log call site atsrc/orchestrator/valkey.ts:33(which runs at startup before the logger emits, on a value that is otherwise safe to log structurally). Verified byscrubs Valkey URL credentials embedded in err.message. The optional plan item to importredactValkeyUrldirectly into the logger was deliberately implemented inline instead, to avoid alogger → valkey → loggerimport cycle.test/utils/logger.test.tscovers all five plan-mandated assertions plus theresponse.data.tokenand non-error pass-through branches. Coverage onsrc/logger.tsis 100% functions / 98.97% lines (above the 90% per-file gate inbunfig.toml).docs/operate/observability.mdgains a "Log redaction" section that describes both layers and cross-links to the point helpers;mkdocs build --strictpasses, citation guard passes.request/responseobjects, leaving the originalErrorinstance untouched. Verified by thedoes not mutate the original Error instancetest.Nothing was deferred. Diff scope matches the plan's bound:
src/logger.ts,test/utils/logger.test.ts(new),docs/operate/observability.md, andIMPLEMENT.md. No new npm dependencies, no env-var changes, no edits to the callers oflogger.*.Related Issues
Test plan
bun run typecheckcleanbun run lintno new errorsSummary by CodeRabbit
Release Notes
New Features
Documentation
Tests