diff --git a/CHANGELOG.md b/CHANGELOG.md index 1decda0..77f69ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ## [Unreleased] +### Security + +- Redact secret-shaped tokens (at minimum HuggingFace `hf_...` tokens) from `service.log`, the raw child `service-stderr.log` tee, and `app.log` before they are written, so the shareable **Export Diagnostics…** bundle cannot leak a credential embedded in third-party output (e.g. a token surfaced in a dependency's traceback). + ## [1.2.0] - 2026-05-08 ### Added diff --git a/backend/services/logging_setup.py b/backend/services/logging_setup.py index 72ce481..aba0297 100644 --- a/backend/services/logging_setup.py +++ b/backend/services/logging_setup.py @@ -15,6 +15,7 @@ from __future__ import annotations import logging +import re from logging.handlers import RotatingFileHandler from pathlib import Path @@ -28,6 +29,61 @@ # transcription pipeline spawns workers that re-exec this binary). _HANDLER_MARKER = "_mt_service_log_handler" +# Secret-shaped tokens to mask before anything reaches disk (story #139). Our own +# code never logs the HuggingFace token, but a third-party dependency could embed +# it in a traceback (e.g. a 401 body), which would then land in service.log and +# the user-shareable diagnostics zip. Keep every pattern in this one list so more +# credential shapes can be added later. The Swift side mirrors this list in +# macos/Sources/MeetingTranscriberKit/Logging/SecretRedaction.swift — keep them +# in sync (both are exercised with the shared `hf_TESTTOKEN...` test vector). +_SECRET_PATTERNS: list[tuple[re.Pattern[str], str]] = [ + (re.compile(r"hf_[A-Za-z0-9]+"), "hf_***"), +] + + +def redact_secrets(text: str) -> str: + """Mask every known secret shape in ``text``. Safe on arbitrary strings.""" + for pattern, replacement in _SECRET_PATTERNS: + text = pattern.sub(replacement, text) + return text + + +class SecretRedactingFilter(logging.Filter): + """Mask secrets in a record's message, traceback, and stack info. + + Installed on the file handler (not the root logger): a filter added to a + logger only runs for records logged *directly* on it, so records propagated + up from child loggers — notably ``uvicorn`` — would bypass it. A handler + filter runs for every record the handler emits. + + The traceback is pre-rendered here and stashed in ``record.exc_text`` so the + handler's formatter reuses the redacted text instead of re-rendering the raw + ``exc_info``. Fails open: any formatting error leaves the record as-is rather + than raising back at the log call site. + """ + + def filter(self, record: logging.LogRecord) -> bool: + try: + if record.args: + try: + record.msg = record.getMessage() + except Exception: # noqa: BLE001 + # Malformed format record (e.g. mismatched %-args): keep the + # raw template but drop args so the formatter cannot raise. + record.msg = str(record.msg) + record.args = None + if isinstance(record.msg, str): + record.msg = redact_secrets(record.msg) + if record.exc_info and not record.exc_text: + record.exc_text = logging.Formatter().formatException(record.exc_info) + if record.exc_text: + record.exc_text = redact_secrets(record.exc_text) + if record.stack_info: + record.stack_info = redact_secrets(record.stack_info) + except Exception: # noqa: BLE001 — redaction must never break logging. + pass + return True + def _already_installed(root: logging.Logger, log_file: Path) -> bool: for handler in root.handlers: @@ -62,6 +118,7 @@ def configure_service_logging(log_dir: Path | None = None) -> Path: ) handler.setLevel(logging.INFO) handler.setFormatter(logging.Formatter(_LOG_FORMAT)) + handler.addFilter(SecretRedactingFilter()) setattr(handler, _HANDLER_MARKER, True) root.addHandler(handler) return log_file diff --git a/docs/plans/139-redact-secrets-from-logs.md b/docs/plans/139-redact-secrets-from-logs.md new file mode 100644 index 0000000..bde7f60 --- /dev/null +++ b/docs/plans/139-redact-secrets-from-logs.md @@ -0,0 +1,70 @@ +# Plan: Redact secrets from logs and the diagnostics export + +**Story**: #139 +**Spec**: N/A (follow-up to #137 / PR #138) +**Branch**: feature/139-redact-secrets-from-logs +**Date**: 2026-09-03 +**Mode**: TDD — both redactors are pure functions with clear input/output. + +## Technical Decisions + +### TD-1: Attach the redacting filter to the handler, not the root logger +- **Context**: AC1 asks for a filter "installed on the root logger". A `logging.Filter` added via `Logger.addFilter` only runs for records logged *directly* on that logger — records propagated up from child loggers (notably `uvicorn`/`uvicorn.access`) skip it. +- **Decision**: Add the filter to the `RotatingFileHandler` (which lives on the root logger). Handler-level filters run for every record the handler emits, including propagated ones. +- **Alternatives considered**: `root.addFilter(...)` — rejected, misses propagated records; a `RedactingFormatter` subclass — works but AC explicitly asks for a filter, and a filter keeps message/traceback handling in one place. + +### TD-2: Pre-render redacted traceback into `record.exc_text` +- **Context**: A filter runs before the Formatter renders `record.exc_info`, so redacting `record.msg` alone leaves the traceback unmasked. +- **Decision**: In the filter, format the exception via `logging.Formatter().formatException(record.exc_info)`, redact it, and assign to `record.exc_text`. `Formatter.format` reuses a non-empty `exc_text` instead of re-rendering, so the on-disk traceback is redacted. +- **Alternatives considered**: clearing `exc_info` — rejected, loses structured info other handlers may want. + +### TD-3: Fail-open filter +- **Context**: `record.getMessage()` / `formatException()` can raise (e.g. mismatched `%` args). Filters run before the handler's emit try/except, so a raise escapes to the log call site. +- **Decision**: Wrap the redaction work in try/except and return `True` on failure — never drop or raise from the redaction filter. + +### TD-4: Per-chunk Swift tee redaction +- **Context**: The raw child stderr tee (`service-stderr.log`) bypasses Python logging entirely. For stderr that never passes through Python (uncaught interpreter tracebacks, C-level/third-party direct prints), the Swift tee is the *sole* redaction sink, not a backstop. +- **Decision**: Redact each `availableData` chunk at the single tee write site. Accepted limitation: a token split across two reads could evade masking (low risk — tracebacks arrive as a burst). +- **Alternatives considered**: newline-buffered tee with EOF flush — more robust but adds a second buffer to a concurrency-sensitive drain loop; deferred for simplicity. + +## Files to Create or Modify + +- `backend/services/logging_setup.py` — add `_SECRET_PATTERNS`, `redact_secrets()`, `SecretRedactingFilter`; attach filter to the handler. +- `tests/unit/test_logging_setup.py` — message/traceback on-disk redaction, fail-open, `redact_secrets` unit. +- `macos/Sources/MeetingTranscriberKit/Logging/SecretRedaction.swift` (new) — `SecretRedaction.redact(_:)`. +- `macos/Sources/MeetingTranscriberKit/Service/ServiceSupervisor.swift` — redact chunk before teeing. +- `macos/Sources/MeetingTranscriberKit/Logging/AppLog.swift` — redact lines before writing (app.log is bundled too). +- `macos/Sources/MeetingTranscriberKitTests/SecretRedactionTests.swift` (new) + `main.swift` registration. +- `CHANGELOG.md` — Unreleased entry. + +## Approach per AC + +### AC1/AC2: Redacting filter on the root logger covering message + traceback +`SecretRedactingFilter` attached to the `RotatingFileHandler`; redacts `record.msg` (args collapsed via `getMessage()`), `record.exc_text` (pre-rendered), and `record.stack_info`. + +### AC3: Raw stderr tee redacted Swift-side +`SecretRedaction.redact` applied at the tee write site in `drainAndTee`; `AppLog` writes also routed through it. + +### AC4: Unit test on-disk +Log a record/exception containing `hf_TESTTOKEN`; assert the on-disk `service.log` contains `hf_***`, not the token. + +### AC5: Diagnostics zip free of the token +Python on-disk test as automated coverage; documented manual check in the PR (seed token, export, `unzip -p | grep`). Shared `hf_TESTTOKEN` vector in Python and Swift tests to surface pattern drift. + +## Commit Sequence + +1. Python redacting filter + tests +2. Swift redactor + tee/AppLog redaction + tests +3. CHANGELOG + plan doc + +## Risks and Trade-offs + +- Per-chunk Swift redaction: token split across reads could evade masking (documented, low risk). +- Two pattern lists (Python + Swift) can drift; mitigated by cross-reference comments and a shared test token. +- Redaction is forward-only — existing/rotated on-disk log content is not rewritten (our code never logged tokens historically). +- app.log carries only Swift-authored lifecycle strings, but is routed through the redactor anyway as near-free defense-in-depth. + +## Deviations from Plan + +- The fail-open test (`test_filter_neutralizes_malformed_format_record`) exercises `SecretRedactingFilter.filter()` directly rather than through the logging pipeline: pytest's own capture handler re-raises a malformed-`%`-args record independent of our filter, so a full-pipeline test could not isolate our filter's behavior. The filter now also drops `record.args` when `getMessage()` raises, so no downstream formatter can raise either. +- The Swift redactor also covers `app.log` (via `AppLog.line`), not only the `service-stderr.log` tee — near-free defense-in-depth for the third bundled sink. diff --git a/macos/Sources/MeetingTranscriberIntegrationTests/main.swift b/macos/Sources/MeetingTranscriberIntegrationTests/main.swift index d1d9469..450cdd7 100644 --- a/macos/Sources/MeetingTranscriberIntegrationTests/main.swift +++ b/macos/Sources/MeetingTranscriberIntegrationTests/main.swift @@ -181,11 +181,57 @@ func scenarioPostHandshakeFloodDoesNotBlock() { check(!supervisor.isRunning, "flood: child terminated") } +// MARK: - Scenario 6: secrets in child stderr are redacted in the tee (story #139) + +func scenarioStderrTokenIsRedacted() { + // The child prints an hf_ token to stderr (as a third-party dep might in a + // traceback). The supervisor's tee must write the placeholder, not the token, + // to service-stderr.log — the file bundled into the diagnostics export. This + // drives the real drainAndTee → SecretRedaction seam end-to-end. + let tmp = FileManager.default.temporaryDirectory + .appendingPathComponent("supervisor-redact-\(UUID().uuidString)", isDirectory: true) + try? FileManager.default.createDirectory(at: tmp, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: tmp) } + + let token = "hf_TESTTOKEN0123456789abcdef" + let nonce = UUID().uuidString + let code = """ + import json, os, sys, time + print(json.dumps({"event": "ready", "port": 45998, "nonce": os.environ.get("MT_SERVICE_NONCE", "")}), flush=True) + sys.stderr.write("401 Unauthorized for \(token)\\n"); sys.stderr.flush() + time.sleep(30) + """ + + let stderrLog = FileLog(fileName: "service-stderr.log", directory: tmp) + let supervisor = ServiceSupervisor(stderrLog: stderrLog) + do { + let port = try supervisor.start(python(code, nonce: nonce), timeout: 10) + check(port == 45998, "redact: handshake parsed (got \(port))") + + // Give the tee a moment to drain the stderr line, then flush. + let deadline = Date().addingTimeInterval(5) + var body = "" + repeat { + stderrLog.flush() + body = (try? String(contentsOf: stderrLog.url, encoding: .utf8)) ?? "" + if body.contains("hf_***") { break } + Thread.sleep(forTimeInterval: 0.05) + } while Date() < deadline + check(!body.contains(token), "redact: token absent from service-stderr.log") + check(body.contains("hf_***"), "redact: placeholder written to service-stderr.log") + } catch { + check(false, "redact: start threw \(error)") + } + supervisor.terminate(gracePeriod: 2) + check(!supervisor.isRunning, "redact: child terminated") +} + await scenarioStubService() scenarioNoisyStdout() scenarioSigkillEscalation() scenarioNonceMismatch() scenarioPostHandshakeFloodDoesNotBlock() +scenarioStderrTokenIsRedacted() let summary = "\n\(passed) passed, \(failed) failed\n" FileHandle.standardOutput.write(Data(summary.utf8)) diff --git a/macos/Sources/MeetingTranscriberKit/Logging/AppLog.swift b/macos/Sources/MeetingTranscriberKit/Logging/AppLog.swift index 1b48b72..7bd3219 100644 --- a/macos/Sources/MeetingTranscriberKit/Logging/AppLog.swift +++ b/macos/Sources/MeetingTranscriberKit/Logging/AppLog.swift @@ -32,6 +32,10 @@ public enum AppLog { } private static func line(_ level: String, _ message: String) -> String { - "\(ISO8601DateFormatter().string(from: Date())) \(level) app: \(message)" + // app.log is bundled into the diagnostics export; redact as + // defense-in-depth even though these are Swift-authored breadcrumbs that + // never interpolate tokens today (story #139). + let safe = SecretRedaction.redact(message) + return "\(ISO8601DateFormatter().string(from: Date())) \(level) app: \(safe)" } } diff --git a/macos/Sources/MeetingTranscriberKit/Logging/SecretRedaction.swift b/macos/Sources/MeetingTranscriberKit/Logging/SecretRedaction.swift new file mode 100644 index 0000000..bc1465d --- /dev/null +++ b/macos/Sources/MeetingTranscriberKit/Logging/SecretRedaction.swift @@ -0,0 +1,38 @@ +// Story #139 — redact secrets from logs and the diagnostics export. +// Plan: docs/plans/139-redact-secrets-from-logs.md + +import Foundation + +/// Masks secret-shaped tokens before text is written to a shareable log file. +/// +/// The raw child stderr tee (`service-stderr.log`) and the app's own `app.log` +/// are both bundled into the user-shareable diagnostics zip, and the raw stderr +/// bypasses Python's logging formatter entirely — so for third-party output that +/// never passes through Python (e.g. an uncaught traceback printed by +/// `huggingface_hub`/`pyannote`), this is the *sole* redaction sink, not a +/// backstop. Keep the pattern list in sync with the Python side in +/// `backend/services/logging_setup.py` (`_SECRET_PATTERNS`); both are exercised +/// with the shared `hf_TESTTOKEN...` test vector so a divergence surfaces. +public enum SecretRedaction { + /// (pattern, replacement) pairs. Add more credential shapes here. + private static let patterns: [(NSRegularExpression, String)] = { + let specs = [ + (#"hf_[A-Za-z0-9]+"#, "hf_***"), + ] + return specs.compactMap { pattern, replacement in + (try? NSRegularExpression(pattern: pattern)).map { ($0, replacement) } + } + }() + + /// Return `text` with every known secret shape masked. Safe on any string. + public static func redact(_ text: String) -> String { + var result = text + for (regex, replacement) in patterns { + let range = NSRange(result.startIndex..., in: result) + result = regex.stringByReplacingMatches( + in: result, range: range, withTemplate: replacement + ) + } + return result + } +} diff --git a/macos/Sources/MeetingTranscriberKit/Service/ServiceSupervisor.swift b/macos/Sources/MeetingTranscriberKit/Service/ServiceSupervisor.swift index f404e6c..53480b6 100644 --- a/macos/Sources/MeetingTranscriberKit/Service/ServiceSupervisor.swift +++ b/macos/Sources/MeetingTranscriberKit/Service/ServiceSupervisor.swift @@ -167,7 +167,18 @@ public final class ServiceSupervisor { while true { let chunk = handle.availableData if chunk.isEmpty { break } // EOF - if matched || teeBeforeMatch { self?.stderrLog?.write(chunk) } + // Redact secrets before teeing: this raw child output bypasses + // Python's logging redaction, and the file is bundled into the + // shareable diagnostics zip (story #139). This single write site + // serves both the stderr tee and the post-handshake stdout tee. + // Per-chunk redaction accepts one boundary limitation: a token + // split across two availableData reads could evade masking. + if matched || teeBeforeMatch { + let redacted = SecretRedaction.redact( + String(decoding: chunk, as: UTF8.self) + ) + self?.stderrLog?.write(Data(redacted.utf8)) + } if matched { continue } // keep draining + teeing, stop parsing buffer.append(chunk) while let newline = buffer.firstIndex(of: 0x0A) { diff --git a/macos/Sources/MeetingTranscriberKitTests/DiagnosticsExporterTests.swift b/macos/Sources/MeetingTranscriberKitTests/DiagnosticsExporterTests.swift index 83881f6..6b1f638 100644 --- a/macos/Sources/MeetingTranscriberKitTests/DiagnosticsExporterTests.swift +++ b/macos/Sources/MeetingTranscriberKitTests/DiagnosticsExporterTests.swift @@ -35,5 +35,36 @@ func runDiagnosticsExporterTests() { // Summary carries the identifying header. expect(DiagnosticsExporter.summary().contains("MeetingTranscriber diagnostics"), "summary has header") + + // AC5 (story #139): a token that passed through the redacting write path + // must be absent from the exported zip. Seed a log the way the stderr tee + // would write it (raw child output run through SecretRedaction.redact), + // export, extract, and grep the extracted tree for the raw token. + let seededToken = "hf_TESTTOKEN0123456789abcdef" + let teed = SecretRedaction.redact("Traceback: 401 Unauthorized for \(seededToken)\n") + try? Data(teed.utf8).write(to: logs.appendingPathComponent("service-stderr.log")) + + let secretDest = tmp.appendingPathComponent("diag-secret.zip") + do { + try DiagnosticsExporter.exportDiagnostics(to: secretDest, logsDirectory: logs) + } catch { + expect(false, "secret export threw \(error)") + } + + let extracted = tmp.appendingPathComponent("extracted-\(UUID().uuidString)", isDirectory: true) + let ditto = Process() + ditto.executableURL = URL(fileURLWithPath: "/usr/bin/ditto") + ditto.arguments = ["-x", "-k", secretDest.path, extracted.path] + try? ditto.run() + ditto.waitUntilExit() + + var zipBody = "" + if let files = FileManager.default.enumerator(at: extracted, includingPropertiesForKeys: nil) { + for case let url as URL in files { + zipBody += (try? String(contentsOf: url, encoding: .utf8)) ?? "" + } + } + expect(!zipBody.contains(seededToken), "exported zip is free of the seeded token") + expect(zipBody.contains("hf_***"), "exported zip carries the redacted placeholder") } } diff --git a/macos/Sources/MeetingTranscriberKitTests/SecretRedactionTests.swift b/macos/Sources/MeetingTranscriberKitTests/SecretRedactionTests.swift new file mode 100644 index 0000000..5f82a84 --- /dev/null +++ b/macos/Sources/MeetingTranscriberKitTests/SecretRedactionTests.swift @@ -0,0 +1,43 @@ +// Story #139 — redact secrets from logs and the diagnostics export. +// Plan: docs/plans/139-redact-secrets-from-logs.md + +import Foundation +import MeetingTranscriberKit + +// Shared canonical vector — the Python test (`_SEEDED_TOKEN`) uses the same +// string so a pattern divergence between the two runtimes surfaces. +private let seededToken = "hf_TESTTOKEN0123456789abcdef" + +func runSecretRedactionTests() { + suite("SecretRedaction") { + // Masks an hf_ token embedded in surrounding text. + let redacted = SecretRedaction.redact("401 Unauthorized for \(seededToken) here") + expect(!redacted.contains(seededToken), "token removed, got \(redacted.debugDescription)") + expect(redacted.contains("hf_***"), "placeholder present, got \(redacted.debugDescription)") + + // Leaves non-secret text untouched. + expectEqual( + SecretRedaction.redact("plain line, no secrets"), + "plain line, no secrets", + "non-secret text unchanged" + ) + + // Masks every occurrence, not just the first. + let multi = SecretRedaction.redact("\(seededToken) and \(seededToken)") + expect(!multi.contains(seededToken), "all occurrences masked, got \(multi.debugDescription)") + + // The stderr tee redacts before writing to the bundled log file. + let dir = FileManager.default.temporaryDirectory + .appendingPathComponent("redaction-tests-\(UUID().uuidString)", isDirectory: true) + try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + + let stderrLog = FileLog(fileName: "service-stderr.log", directory: dir) + let chunk = "Traceback: RuntimeError 401 for \(seededToken)\n" + stderrLog.write(Data(SecretRedaction.redact(chunk).utf8)) + stderrLog.flush() + let onDisk = (try? String(contentsOf: stderrLog.url, encoding: .utf8)) ?? "" + expect(!onDisk.contains(seededToken), "tee redacts token on disk, got \(onDisk.debugDescription)") + expect(onDisk.contains("hf_***"), "tee wrote placeholder, got \(onDisk.debugDescription)") + } +} diff --git a/macos/Sources/MeetingTranscriberKitTests/main.swift b/macos/Sources/MeetingTranscriberKitTests/main.swift index b6e461d..480a863 100644 --- a/macos/Sources/MeetingTranscriberKitTests/main.swift +++ b/macos/Sources/MeetingTranscriberKitTests/main.swift @@ -17,6 +17,7 @@ runPlainTextRendererTests() runOverviewAggregationsTests() runFileLogTests() runDiagnosticsExporterTests() +runSecretRedactionTests() // (unnamed-speaker suite runs inside runSpeakerColorTests) TestRunner.shared.finish() diff --git a/tests/unit/test_logging_setup.py b/tests/unit/test_logging_setup.py index f6415cd..e0b5d50 100644 --- a/tests/unit/test_logging_setup.py +++ b/tests/unit/test_logging_setup.py @@ -85,3 +85,63 @@ def test_defaults_to_app_paths_logs_dir(self, tmp_path: Path, monkeypatch: pytes log_file = logging_setup.configure_service_logging() assert log_file == (tmp_path / "Logs" / "service.log").resolve() + + +# Shared canonical token; the Swift redaction test uses the same string so a +# pattern divergence between the two runtimes surfaces (story #139). +_SEEDED_TOKEN = "hf_TESTTOKEN0123456789abcdef" + + +class TestRedactSecrets: + def test_masks_hf_token(self): + redacted = logging_setup.redact_secrets(f"auth failed for {_SEEDED_TOKEN} now") + + assert _SEEDED_TOKEN not in redacted + assert "hf_***" in redacted + + def test_leaves_non_secret_text_untouched(self): + assert logging_setup.redact_secrets("plain message, no secrets") == ("plain message, no secrets") + + +class TestSecretRedactingFilterOnDisk: + def test_message_token_is_redacted_on_disk(self, tmp_path: Path): + log_file = logging_setup.configure_service_logging(log_dir=tmp_path) + + logging.getLogger("backend.services.redaction_probe").info("received token %s from provider", _SEEDED_TOKEN) + + contents = log_file.read_text() + assert _SEEDED_TOKEN not in contents + assert "hf_***" in contents + + def test_exception_traceback_token_is_redacted_on_disk(self, tmp_path: Path): + log_file = logging_setup.configure_service_logging(log_dir=tmp_path) + logger = logging.getLogger("backend.services.redaction_probe") + + try: + raise RuntimeError(f"401 Unauthorized: {_SEEDED_TOKEN}") + except RuntimeError: + logger.exception("diarization call failed") + + contents = log_file.read_text() + assert _SEEDED_TOKEN not in contents + assert "hf_***" in contents + # The traceback itself must be present (redaction, not omission). + assert "Traceback" in contents + + def test_filter_neutralizes_malformed_format_record(self): + # A mismatched-args record makes getMessage() raise. The filter must not + # raise and must drop args so a downstream formatter cannot raise either. + record = logging.LogRecord( + name="probe", + level=logging.INFO, + pathname=__file__, + lineno=1, + msg="value is %d and %d", # too few args on purpose + args=(1,), + exc_info=None, + ) + + assert logging_setup.SecretRedactingFilter().filter(record) is True + assert record.args is None + # getMessage() must now be safe (no formatting) — proves no downstream raise. + assert record.getMessage() == "value is %d and %d"