Skip to content

fix(diag): stop over-redacting lowercase identifiers in Doctor report (#701)#702

Open
ShadowsTT wants to merge 1 commit into
FerroxLabs:mainfrom
ShadowsTT:fix/701-diag-over-redaction
Open

fix(diag): stop over-redacting lowercase identifiers in Doctor report (#701)#702
ShadowsTT wants to merge 1 commit into
FerroxLabs:mainfrom
ShadowsTT:fix/701-diag-over-redaction

Conversation

@ShadowsTT

Copy link
Copy Markdown

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.source is the literal constant model_registry_providers (exactly 24 chars) → rendered as **Providers** (••••ders): 10
  • Long reverse-DNS MCP server names (e.g. com.<longname>-mcp) → rendered as com.••••-mcp, hiding the very server name the report exists to name.

Root cause

SHAPE_REGEXES contained /[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:

/(?=[A-Za-z0-9_-]*[A-Z0-9])[A-Za-z0-9_-]{24,}={0,2}/g

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/DEP0190 deprecation 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.ts locking in both directions (identifiers preserved, secrets masked).

Closes #701

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
@github-actions github-actions Bot added the area:core Wayland Core engine / backends label Jul 5, 2026
@FerroxLabs

Copy link
Copy Markdown
Owner

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):

https://hooks.example.com/trigger/abcdefghijklmnopqrstuvwx

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):

  1. Preferred: keep the internally generated labels out of the generic redact() pipeline at the call site. model_registry_providers and the MCP server ids / reverse-DNS names are a small, enumerable, non-secret set the diag server generates itself, so exempt those specifically rather than loosening the global heuristic that also runs over free-form error text and URLs where real secrets live.

  2. If you would rather keep the regex heuristic: add a regression test proving the bypass class is still masked (redact of the hooks.example.com URL above must still mask the slug), and narrow the exemption so a bare lowercase run outside a recognized identifier / URL-userinfo / key-value context is still caught (only exempt the specific internal-label shapes Doctot Report #701 names).

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core Wayland Core engine / backends

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Doctot Report

2 participants