fix(security): sanitize the API-key prefix logged on authentication failure

[Chris0Jeky]

Summary

The MCP key-not-found branch in ApiKeyMiddleware logged a raw 8-character slice of the presented
bearer token. That token is unauthenticated caller input, so the slice could carry CR, LF, U+2028 and
C1 controls straight into a plain-text log sink and forge an extra log line (CWE-117).

• backend/src/Taskdeck.Api/Middleware/ApiKeyMiddleware.cs now slices the 8-character prefix first
  and sanitizes it afterwards, through the existing LogSanitizer.StripControlChars. Taking the
  slice first means stripping can only shorten the prefix, never pull further token material into the
  log line, so at most 8 characters of the presented token are ever logged whatever the input. A
  prefix that sanitizes to nothing is reported as short rather than as an empty value, so an
  operator never sees a blank field; that branch is defensive only, since the format gate above
  requires the token to start with the printable tdsk_ literal.
• LogControlCharacterSanitizer.Strip (behind both LogSanitizer and LogValueSanitizer) now also
  drops Unicode format characters, general category Cf. That covers the zero-width and bidirectional
  set called out on the issue: U+200B..U+200F, U+202A..U+202E, U+2060..U+2064, U+FEFF, plus the soft
  hyphen. They render as nothing while hiding or reordering the text around them. Surrogates are
  category Cs, not Cf, so valid surrogate pairs are untouched and unpaired halves stay with the
  existing IsUnpairedSurrogate rule. U+2028/U+2029 were already handled and are unchanged.
• docs/security/SECURITY_LOGGING_REDACTION.md gains one bullet recording what that sanitizer
  strips, plus the prefix-before-sanitize ordering.

Exception messages passed as the exception argument to LogError are deliberately left alone. That
is standard structured logging: the exception is a first-class argument and the sink owns its
rendering. It is recorded here as declined rather than fixed.

Root cause

ApiKeyMiddleware.InvokeAsync built the log argument as token.Length >= 8 ? token[..8] : "short"
with no sanitizer in the path, unlike every other request-derived log argument in the Api layer.
token is Authorization header content after Bearer is stripped and the value is trimmed, so
interior control characters survive to the log call. The trailing gap was the sanitizer itself: it
stripped C0, DEL, C1 and the line/paragraph separators, but not the invisible Cf format characters.

Verification

All commands run from the worktree
C:/Users/jekyt/source/Taskdeck-Beta/.worktrees/codex-2519-apikey-log-prefix.

Red first, against the unfixed middleware:

dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~ApiKeyMiddlewareLogSanitizationTests"
Failed!  - Failed: 2, Passed: 1, Skipped: 0, Total: 3

Failing assertions:

KeyNotFound_WithControlCharactersInToken_LogsOneSanitizedSingleLineEntry
  Did not expect entry "MCP API key authentication failed: key not found (prefix: tdsk_ <CR><LS><LF>)"
  to contain "<CR>" because a carriage return would start a forged line in a plain-text sink.

KeyNotFound_WithBidiAndZeroWidthCharactersInToken_LogsNoFormatCharacters
  Did not expect entry "MCP API key authentication failed: key not found (prefix: tdsk_<ZWSP><RLM><RLO>)"
  to contain "<ZWSP>".

Green after the fix:

dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~ApiKeyMiddleware"
Passed!  - Failed: 0, Passed: 14, Skipped: 0, Total: 14

dotnet test backend/tests/Taskdeck.Application.Tests/Taskdeck.Application.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~Sanitiz"
Passed!  - Failed: 0, Passed: 37, Skipped: 0, Total: 37

dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1
Passed!  - Failed: 0, Passed: 2833, Skipped: 4, Total: 2837   (the 4 known LlmQuota concurrency skips)

dotnet test backend/Taskdeck.sln -c Release -m:1
Domain        Passed: 1603, Failed: 0
Application   Passed: 4169, Failed: 0
Api           Passed: 2833, Failed: 0, Skipped: 4
Cli           Passed:  191, Failed: 0
Architecture  Passed:   28, Failed: 0, Skipped: 1
Integration   Passed:   36, Failed: 0

node scripts/check-docs-governance.mjs
Docs governance check passed.

git diff --check
(no output)

The BatchExecuteProposalsApiTests command-shape flake from #2399 did not appear in either full run.

Not verified

• No CI run yet at the time of writing; ci-required will repeat the solution test.
• No manual run of the API against a real log sink. The regression asserts on the message the
  ILogger formatter produces, not on rendered console or file output, so per-sink escaping
  behaviour is unproven either way.
• The Cf change is exercised by the new unit tests and by the whole backend suite, but no audit was
  made of every existing caller of LogValueSanitizer to confirm none of them logs a value where a
  format character is load-bearing. Nothing in the suite depends on one surviving.
• Frontend and E2E suites were not run; nothing in this change touches them.

Risk notes

• The sanitizer is shared, so widening it to Cf affects every sanitized log value, not just the
  API-key prefix. The effect is subtractive only: characters that render as nothing are dropped.
  Length-sensitive callers are unaffected in kind, since stripping already could shorten a value.
• The logged prefix can now be shorter than 8 characters when a caller injects control or format
  characters into it. That is the intended trade: the value is a triage aid, not an identifier, and
  the persisted ApiKey.KeyPrefix_ column is unrelated and unchanged.
• CharUnicodeInfo.GetUnicodeCategory is called per character on the sanitizing path. It is a table
  lookup, and Strip is only on error and diagnostic paths.
• No behaviour change to the authentication decision itself: status codes, budget charging and query
  shape are untouched, and the existing ApiKeyMiddleware tests that pin them still pass.

Closes #2519

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Review (agent half of the gate; Codex credits exhausted, SC-9). One fresh-context reviewer (read-only, Opus 5) at exact head 9e63535: verdict SHIP, no CRITICAL/HIGH.

Confirmed: the prefix is sliced to eight characters before it is sanitized, so stripping can only shorten it and no token material past index 7 can reach the log on any branch (short token, empty-after-strip, split surrogate pair); no other log statement in ApiKeyMiddleware writes request-derived text; authentication decisions, status codes, budget charging and the query shape are untouched; every production consumer of the widened LogControlCharacterSanitizer is log-only (ILogger arguments and Activity tags), no existing test encoded the old Cf-surviving behaviour, and the U+2028/U+2029 clause is not duplicated (Zl/Zp, not Cf); layer purity holds (System.Globalization only); the new Api test asserts on the rendered message, not the template, so a raw CR or U+2028 in the argument fails it.

Findings and dispositions:

1. MEDIUM, fixed in 2410962: no test discriminated the slice-before-sanitize ordering (a sanitize-then-slice refactor would have stayed green). Added KeyNotFound_WithControlCharactersInsideTheWindow_DoesNotPullLaterTokenMaterialIntoTheLog: token tdsk_ plus three carriage returns then secret material; the entry must contain (prefix: tdsk_) and must not contain the next three secret characters. Focused rerun at 2410962: --filter "FullyQualifiedName~ApiKeyMiddleware" 15/15; node scripts/check-docs-governance.mjs passed.
2. LOW, fixed in 2410962: the middleware comment claimed the empty-prefix branch was unreachable because of the tdsk_ format gate; that gate uses a culture-sensitive StartsWith, so the comment now says the fallback stays. The culture-sensitive comparison itself is pre-existing and out of scope here.
3. LOW, fixed in 2410962: the sanitizer comment and the policy sentence said "every Unicode format character"; the check is per UTF-16 code unit, so both now say Basic Multilingual Plane format characters. All named attack characters (U+200B..U+200F, U+202A..U+202E, U+2060..U+2064, U+FEFF) are BMP and were already stripped.

The fix commit adds one test and rewords two comments and one doc sentence; no production logic changed, so no further review pass is owed. Merge gate: hosted ci-required green at 2410962 plus the aging floor.