fix(diag): stop over-redacting lowercase identifiers in Doctor report (#701)#702
fix(diag): stop over-redacting lowercase identifiers in Doctor report (#701)#702ShadowsTT wants to merge 1 commit into
Conversation
The base64url catch-all masker matched any 24+ char [A-Za-z0-9_-] run, including all-lowercase identifiers. This garbled the Doctor/bug report: the 'model_registry_providers' source label rendered as '(••••ders)' and long reverse-DNS MCP server names as 'com.••••-mcp', hiding the very names the report exists to surface. Gate the base64url rule on a lookahead requiring at least one uppercase letter or digit in the run. Real base64/hex tokens always carry that entropy; snake/kebab identifiers do not. Prefixed keys, named secrets, URL userinfo and hex runs are unaffected (separate rules). Closes FerroxLabs#701
|
Thanks for this, and for the clear write-up. #701 is a real bug: the Doctor report genuinely shouldn't be turning model_registry_providers into dots or hiding the MCP server names it exists to surface. The direction is right. An independent security cross-audit did surface one issue that needs closing before this can land on a report surface, so holding it for a revision. The problem: the new lookahead exempts any 24+ char all-lowercase, no-digit run from the base64url rule. That is safe for cryptographically random tokens (they virtually always carry an uppercase letter or digit, and lowercase hex is still covered by the separate 32+ hex rule). But it also stops masking bare lowercase-only secrets that appear in free-form text with no key name, no URL userinfo, and no leading colon/equals/at, which nothing else in the pipeline then catches. Concrete failing case (masked before this PR, not masked after): Walking the rules: the key-value rule needs a recognized key name (token/secret/password/bearer) nearby, absent here. The URL-userinfo rules only fire on user:pass@host, but this is a path segment. The delimiter backstop only fires when the token is immediately preceded by colon, equals, or at, but here it is preceded by a slash. No prefix rule matches and it is not hex. So the 24-char lowercase slug slips every rule. Webhook relays, magic-link/invite URLs, and share links commonly use lowercase-only slugs and land verbatim in connection-failure text, which is exactly what a Doctor report is meant to be pasted publicly. The same gap applies to diceware-style passphrases appearing as bare tokens. Recommended fix (either works; the first is cleaner and carries no security tradeoff):
Minor: the three still-masked cases in the new test all contain digits, so none actually exercise the lowercase-only backstop the code comment relies on. Worth adding a digit-free case. Really appreciate the contribution. This is close, it just needs the leak vector closed before it can land on a surface that gets pasted into public reports. |
Problem (#701)
The Doctor / bug report ran its output through the concierge-diag secret masker, whose base64url catch-all rule matched any 24+ character
[A-Za-z0-9_-]run — including legitimate all-lowercase identifiers. This garbled the report the user reads to figure out what's broken:providers.sourceis the literal constantmodel_registry_providers(exactly 24 chars) → rendered as**Providers** (••••ders): 10com.<longname>-mcp) → rendered ascom.••••-mcp, hiding the very server name the report exists to name.Root cause
SHAPE_REGEXEScontained/[A-Za-z0-9_-]{24,}={0,2}/g. "Over-redaction is safe" holds for a machine-consumed tool, but not for a human-read diagnostics report — it destroys the diagnostic signal (section labels, server ids).Fix
Gate the base64url rule on a leading lookahead requiring at least one uppercase letter or digit in the run:
Real base64/hex/JWT tokens always carry that entropy and are still masked; all-lowercase snake/kebab identifiers are spared. Prefixed keys (
sk-,ghp_, …), named secrets (token=…), URL userinfo passwords, and long hex runs are handled by separate rules and are unaffected.Verified against the two reported cases plus JWT / AWS / base64-with-digits / stripe-style tokens: identifiers preserved, secrets still masked.
Note on the other Doctor diagnostics
The remaining report lines (MCP servers exposing 0 tools, Supabase MCP
Unauthorized, temp-workspace warnings,punycode/DEP0190deprecation warnings) are the diagnostics working as intended — they reflect the user's local config (unauthenticated/misconfigured MCP servers, chats opened outside a project). This PR fixes the reporting bug that made those lines harder to read; the underlying config guidance belongs in a support reply on the issue.Test
Adds
conciergeDiagServer.redact.bun.test.tslocking in both directions (identifiers preserved, secrets masked).Closes #701