fix(logging): redact secrets centrally in BossLogger's append path

[Raghav-Bansal-15]

BossLogger interpolated data maps and throwables raw into stdout and the log file, so any call site that forgot to mask a value persisted a secret on disk; the supabase-kt message path could also echo response bodies. The append path now runs every data map through LogSanitizer.sanitizeMap (with diagnostic-key allowlists so prose fields survive, and credential shapes still masked under any key) and renders throwables through the sanitizer instead of handing them to SLF4J raw; NamedSupabaseLogging sanitizes the library message. Regression: BossLoggerTest asserts a raw Authorization header and session id never reach the written log file.

Fixed by Devin CLI (SWE-2 High), orchestrated via Orca.

[fluck-boss]

@claude please review the central BossLogger redaction path, focusing on diagnostic allowlists, throwable handling, secret-shape coverage, and accidental over-redaction.

[kshivang]

Thanks for pushing the central redaction direction. I reviewed this together with #1440 and the grouped logging history. I am leaving this open as a draft because the central guarantee is incomplete in the current head:

• BossLogger.log sanitizes data, but entry.message is still interpolated raw into both the SLF4J line and the file line. A caller that puts a credential in the message still leaks it.
• The file writer sanitizes only error.message; it appends selected stack frames raw, while the SLF4J path uses a sanitized full stack trace.
• notifyListeners(sanitized) still carries the original Throwable via the copied entry, so downstream listeners can print the unsanitized cause/stack.

Please make the channels agree on one sanitized entry (including the message and throwable representation, or explicitly prove the listener contract cannot log it), and add regression coverage for message text, file stack frames, and listener-visible errors. Also coordinate the session/challenge vocabulary and call-site changes with #1440 so the two logging PRs do not leave overlapping sanitizer definitions or duplicate production edits. The existing CI is green, but it does not cover these central-path gaps.

[Raghav-Bansal-15]

All three channel gaps are now closed, and this push (98129f2, rebased on latest dev) adds the regression coverage asked for plus cleans up the overlap.

The three fixes (in earlier commits on this branch):

• entry.message is sanitized once in BossLogger.log, so the same redacted text reaches recentLogs, listeners, the SLF4J line and the file line.
• The file writer now renders LogSanitizer.sanitizeStackTrace(error.stackTraceToString()) instead of appending raw frames after a sanitized error.message.
• Listeners receive a SanitizedThrowable whose message - and every sanitized Caused by: message down the chain - went through the sanitizer, so stackTraceToString() cannot print the original credential-carrying messages.

New regression tests in plugin-logging's desktopTest (BossLoggerRedactionTest):

• a credential in the message text reaches neither the SLF4J line nor the file line - asserts the exact string handed to SLF4J through a logback ListAppender (added as a test-only dep; production ships slf4j-api alone) and the written file line.
• the file writer renders stack frames through the sanitizer - a frame carrying a path and a cause message carrying one are both masked; ordinary frames survive.
• a listener receives a throwable whose rendered trace holds only sanitized messages - the original exception message does not appear anywhere in stackTraceToString(), and the cause chain is preserved.

Overlap/dedup pass vs the session/challenge vocabulary: challenge, the sessionId/session_id variants and credentialId stay #567's additions - this PR does not re-define them; it adds only authorization and session to the two name sets. The one real leftover was a duplicate utils.logging.LogSanitizer import in NamedSupabaseLogging.kt that conflicted with the existing plugin.logging one - removed. Also wrapped an over-long line in writeToFileAsync for detekt's length cap.

Verified: :plugin-platform:plugin-logging:desktopTest (incl. 3 new tests), :plugin-platform:plugin-logging:detekt, :plugin-platform:plugin-logging:ktlintCheck, and :composeApp:desktopTest for BossLoggerTest (22) + NamedSupabaseLoggingTest (4) - all green.

[fluck-boss]

@claude please review this exact head (98129f2) for merge into dev. Central log redaction in BossLogger's append path: one sanitized entry feeds recentLogs/listeners/SLF4J/file. Focus: can any channel still receive an unsanitized copy (Throwable renderers, formatForSlf4j, async file writer, listeners); does sanitizeMap's diagnostic-key allowlist re-expose anything a credential could ride (a JWT under key 'message' or 'body'); and do the sensitive-name additions ('session','authorization') over-redact ordinary diagnostics in a way that breaks tests or hides incidents. Security review: secrets, injection, approval bypass.

[claude]

Claude finished @fluck-boss's task in 5m 49s —— View job

---

Review of 98129f2 - central log redaction

The direction is right and the three channel gaps @kshivang named are genuinely closed for message text and exception messages. But the "one sanitized entry feeds every channel" guarantee has two holes, and the central application of a previously opt-in substring rule causes over-redaction that will hide real incidents - including one in the MCP approval audit trail.

I did not run Gradle (no cached toolchain here); every claim below is read off the source, and I name the file/line so you can check.

---

Direct answer to the three questions

1. Can any channel still receive an unsanitized copy? Two can.

Part of the entry	recentLogs	listeners	SLF4J	file
message	sanitized	sanitized	sanitized	sanitized
data values, String	sanitized	sanitized	sanitized	sanitized
data values, non-String	raw	raw	raw via toString()	raw via toString()
error.message + cause messages	sanitized	sanitized	sanitized	sanitized
error.stackTrace frames	raw	raw	sanitized at render	sanitized at render

2. Does the allowlist re-expose anything? A JWT under message/body is caught - credentialShapePattern is checked before the allowlist (LogSanitizer.kt:667), so it never reaches sanitizeLogMessage. But an opaque (non-shape) secret in a JSON body under body/response survives intact.

3. Do session/authorization over-redact? Yes, and the substring match on session plus the now-global reach of the pre-existing key entry blanks booleans that cannot be credentials, in three security-relevant places.

Approval bypass: nothing in this PR touches policy resolution or the approval path. The only interaction is finding 3 below, which redacts trustForSession out of the approval log.

---

Findings

1. Listeners and recentLogs hold raw stack frames - and this PR's own test proves frames can carry secrets

sanitizeThrowable copies error.stackTrace verbatim (BossLogger.kt:618). The KDoc justifies it with "they are sanitized again at render time" - true for the two host renderers, but a listener renders it itself. Anything that calls entry.error.stackTraceToString() (the Console panel, a crash reporter, a plugin-side listener) gets the original StackTraceElement[], including each frame's fileName.

The PR's own BossLoggerRedactionTest at line 95 constructs StackTraceElement("com.example.Secrets", "load", "/Users/ci/keys.pem", 12) and asserts that path never reaches the file - but the listener test (line 114) plants secrets only in the messages, so the frame hole is exactly the case left untested. Either sanitize fileName when copying frames, or state in the KDoc that the frame guarantee is render-side only and that listeners must sanitize.

Fix this →

2. SanitizedThrowable erases the exception type from every channel

SanitizedThrowable (BossLogger.kt:623) extends Throwable with no type information. printStackTrace builds its header and every Caused by: line from toString(), so every logged exception in the application now reports as ai.rever.boss.plugin.logging.BossLogger$SanitizedThrowable. ConnectException vs SSLHandshakeException vs FileNotFoundException is frequently the entire diagnosis, and it is gone from the SLF4J line, the file, recentLogs and listeners simultaneously.

No test catches it - the new tests assert contains("Caused by") and contains("com.example.App.main"), never the class name, and nothing in BossLoggerTest/LogSanitizerTest asserts an exception class in a written line.

Cheap fix: keep the original class name and override toString():

private class SanitizedThrowable(
    private val originalType: String,
    sanitizedMessage: String,
) : Throwable(sanitizedMessage) {
    override fun toString(): String = "$originalType: $message"
}

A class name is structural, not a credential, so nothing is given back.

Fix this →

3. Booleans are redacted, including two approval/auth audit signals

sanitizeMap's first branch (LogSanitizer.kt:666) matches by substring and applies to any value type, with no nonSecretValues exemption (that set is only consulted on the free-text paths, LogSanitizer.kt:722 and :740). So with session newly added, and key now reaching every call site rather than the handful that opted in:

Call site	Key	Value	Now
composeApp/src/commonMain/kotlin/ai/rever/boss/mcp/McpApprovalBus.kt:207	trustForSession	Boolean	[REDACTED]
composeApp/src/commonMain/kotlin/ai/rever/boss/services/auth/EmailAuthService.kt:307	hasSession	Boolean	[REDACTED]
.../services/passkey/supabase/PasskeyAuthenticationHandler.kt:36	hasSessionId	Boolean	[REDACTED]
.../components/plugin/registries/DeepLinkActionRegistryImpl.kt:76	paramKeys	String	[REDACTED]
composeApp/src/desktopMain/kotlin/ai/rever/boss/utils/DeepLinkHandler.kt:686	paramKeyCount	Int	[REDACTED]

The first is the MCP approval log losing whether the operator granted session trust - a governance signal this repo's AGENTS.md treats as audited. The last two are the plugin-action deep-link approval diagnostics that AGENTS.md specifically describes as "the parameter KEYS - never a parameter value"; the call sites already did the defensive work and central redaction now discards it. Three of the five are booleans that cannot carry a credential under any name.

Suggested: skip the key-name branch when the value is Boolean/Number/null, or when value.toString().lowercase() in nonSecretValues.

Fix this →

4. Sanitizing every message erases paths that the message exists to carry - while data["path"] keeps its path

sanitizeLogMessage on entry.message (BossLogger.kt:433) applies filePathPattern, urlPattern and hostnamePattern to every log line in the app. Measured against real call sites:

• BrowserSwipeNavScript.kt:87 "Swipe navigation resource missing: $path" → "...missing: [PATH]". The one fact the line exists for is gone.
• SplitViewOperationsImpl.kt:71 "openFileAtPosition: $filePath:$line:$column" → "openFileAtPosition: [PATH]:12:3".
• Any boss://terminal?command=… in a message → boss:[PATH] (filePathPattern matches //terminal?...; urlPattern is http-only). Deep-link approval diagnostics lose their host and action.
• WorkspaceMcpToolProvider.kt:1072 "Cannot canonicalize project path: $path" → [PATH].

The inconsistency is the part worth resolving deliberately: diagnosticLocatorKeys exists precisely because "a path the log cannot name is not a diagnostic", so data["path"] keeps its text - yet the same path in the message is [PATH]. Pick one policy. If message-side location redaction is intended, it deserves a paragraph in AGENTS.md, because it silently changes what every operator log line in the product says.

5. Non-String data values bypass value-level sanitization entirely

Branches 2-4 of sanitizeMap all guard on value is String, so a List<String>, a nested Map, or any object whose toString() carries a credential falls to else -> value and is then interpolated raw by append(" | ${entry.data}") on both the SLF4J line (BossLogger.kt:502) and the file line (:537). McpArgumentSanitizer already solves the nested case with a depth-bounded recursion - worth mirroring, or at minimum coercing non-primitive values through sanitizeLogMessage(value.toString()).

6. Allowlist re-exposure: an opaque token inside a JSON body/response

For body, response, output, detail, the value is put through the free-text pass, which catches name=value (assignmentPattern, requires =), ?name=value (sensitiveQueryParamPattern, requires ?&#) and known credential shapes. A JSON body does none of those:

data = mapOf("body" to """{"access_token":"eyJ...","refresh_token":"v1.MRq8...”}""")

The JWT is masked by shape; the refresh token is not - Supabase refresh tokens are opaque strings with no recognisable prefix. That is the highest-value credential in a Supabase auth response and it survives under an allowlisted key. This is also the loss from commit 7452634 dropping looksLikeSecret from sanitizeMap: the length rule used to catch exactly this shape.

Two options, either acceptable: add a JSON "name": "value" arm to the free-text pass reusing nameMarksSecret, or drop body/response/output from diagnosticTextKeys (they are the least prose-like of the sixteen and the most likely to be a serialized payload).

7. Reorder sanitizeMap's shape branch below diagnosticTextKeys

Today a body containing a JWT anywhere in it collapses the whole body to abc...xyz via maskToken, defeating the allowlist it was granted. redactLocationsAndCredentials ends with credentialShapePattern.replace(...) { maskToken(it.value) } (LogSanitizer.kt:747), so routing text keys to sanitizeLogMessage first masks the JWT in place and keeps the prose - strictly better on both axes. Keep the shape branch above diagnosticLocatorKeys though: maskUriParams does not apply the shape pattern, so a JWT in a path segment of a url value needs it.

8. Robustness and cost

• Cyclic cause chain → StackOverflowError inside the logger. error.cause?.takeIf { it !== error } (BossLogger.kt:619) only catches self-reference. Throwable.printStackTrace handles genuine cycles with [CIRCULAR REFERENCE]; this recursion does not. A visited IdentityHashMap or a depth cap would match JDK behaviour.
• Suppressed exceptions are dropped. wrapped never copies error.suppressed, so use {}/try-with-resources secondary failures vanish from the log. Not a leak - a diagnostic loss.
• formatForSlf4j is computed unconditionally (BossLogger.kt:446), before the level when, and now includes a full sanitizeStackTrace(stackTraceToString()). With logback filtering at INFO, every DEBUG/TRACE error entry pays a full trace render and ~8 regex passes for a string that is then discarded. An error entry's trace is sanitized twice (SLF4J + file) plus once per cause in sanitizeThrowable. Consider guarding on slf4jLogger.isDebugEnabled etc., or rendering the sanitized trace once onto the entry.
• stackTraceDepth now counts lines, not frames (BossLogger.kt:549). The sanitized string's lines include Caused by: headers, so the default 10 can truncate mid-chain and "... N more frames" counts lines. Worth a comment at minimum.
• Dropping the Throwable argument from the SLF4J calls means ILoggingEvent.getThrowableProxy() is now always null. Any structured appender, log shipper or crash reporter downstream loses exception classification. Given finding 1 (frames are unsanitized) this is the safe choice today - but if frames get sanitized, passing sanitized.error back as the SLF4J argument restores it.

9. Stale cross-reference left by the looksLikeSecret removal

composeApp/src/commonMain/kotlin/ai/rever/boss/mcp/McpArgumentSanitizer.kt:13-14 reads:

Deliberately narrower than [LogSanitizer.sanitizeMap]: that function treats any string of 20 or more characters as secret-shaped ([LogSanitizer.looksLikeSecret]) and masks it

That is now false - it is the documented justification for a different sanitizer's design, and the relationship has inverted. LogSanitizerTest's sanitizeMap masks values that look like secrets (line 248) also no longer tests what its name says (it passes on the shape branch).

10. Not addressed, and worth a sentence somewhere

• Log forging. message is never newline-escaped, and both sinks are line-oriented, so a message containing \n2026-01-01 00:00:00.000 [INFO ] … still forges a log record. Pre-existing, but this is the PR that centralises the append path and replace("\n", "\\n") is one line away.
• No AGENTS.md update. This changes what every log line in the product contains and introduces a named allowlist other contributors must know about (and must not assume is a secrets guarantee - it is name-and-shape based only). This repo documents invariants of far smaller scope.

---

Smaller notes

• build.gradle.kts: implementation(libs.logback) is correctly scoped to desktopTest and does not reach the published POM. Note it also becomes the SLF4J binding for every other test in plugin-logging, and per AGENTS.md this module's publish task depends on desktopTest, so logback must resolve on the publish path too.
• The test-side rename "key1"/"key2" → "field1"/"field2" (BossLoggerTest.kt:115) is the substring rule visible in the diff. Fixing it in the test rather than in the rule is what finding 3 is about.
• Verification claimed in the PR covers BossLoggerTest and NamedSupabaseLoggingTest. Since message text is now sanitized for the whole application, the full :composeApp:desktopTest matters here: several suites assert on getRecentLogs content (WorkspaceHealthStatusItemTest:154, GuardedWriteTest:94, WorkspaceHealthCollectorTest:32). I read their fixtures and they look like they survive, but I could not execute them.
• NamedSupabaseLogging.kt is described in the PR body ("NamedSupabaseLogging sanitizes the library message") but is not in this head's diff against the merge base 5d3ed89. Either it landed on dev separately or the change was lost in the rebase - worth confirming, since it was the stated fix for the response-body echo.

---

Recommendation

Not ready for dev as-is. Blocking, in order: 2 (every exception type erased application-wide), 3 (approval/auth audit booleans redacted), 1 (listener channel still holds raw frames, contradicting the PR's central guarantee). 4 deserves an explicit decision rather than a fix - it may well be intended, but it should be written down before it ships. 6/7 are small changes that make the allowlist both safer and less destructive. Everything else is follow-up.
| Branch: fix/v06-central-log-redaction

[fluck-boss]

Central redaction is the right architecture and the append-path plumbing is solid, but three gaps keep this from landing as-is:

1. Non-string data values bypass redaction entirely. sanitizeMap only transforms value is String; anything else (a nested map, a list, a data class, a JsonObject) flows raw to recentLogs, listeners, and - via toString - the SLF4J and file lines. A secret one level down (mapOf("context" to mapOf("token" to t))) reaches every channel verbatim. Either recurse into collections/maps or fall back to a sanitized value.toString() for non-strings.

2. Serialized JSON under allowlisted keys leaks opaque tokens. The diagnosticTextKeys free-text pass keeps prose but only strips credential SHAPES it recognizes. A body/response value like {"refresh_token":"a3f8...opaque..."} carries no recognizable shape, so the opaque token survives. A redaction pass on a key named body/response should at minimum mask the values of JSON fields whose names hit sensitiveValueNames.

3. Adding "session" to sensitiveValueNames redacts audit fields wholesale. Any key containing "session" - trustForSession, sessionCount, sessionDurationMs - now reads [REDACTED], which destroys exactly the MCP audit signal those fields exist to carry (the BossLoggerTest rename from "key1" to "field1" is the same over-redaction showing up in your own suite). Consider matching on word boundaries or exact-ish names rather than raw substring.

A fourth item is follow-up, not blocking: listeners receive the wrapped throwable with stack frames copied verbatim, so a listener-side stackTraceToString() can still print a raw filename embedded in a frame - the rendered file/SLF4J paths are covered, the listener path isn't. I'll file that separately.

Converting to draft meanwhile - re-request when the three blockers are pinned by tests.

[fluck-boss]

Maintenance note for whenever you pick this back up: dev was rebuilt today (replayed history), and this branch is still based on the old line, so the PR diff currently shows old-line content alongside yours. Please rebase onto current dev before your next push:

git fetch upstream
git rebase upstream/dev
git push --force-with-lease