fix(tui): show MCP servers that failed to start in /mcp - #835
fix(tui): show MCP servers that failed to start in /mcp#835Vasanthdev2004 wants to merge 23 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMCP startup failures now flow from the CLI runtime into TUI state. The ChangesMCP failure visibility
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to The change can display an MCP credential in the /mcp panel and hide the next argument instead, potentially persisting a secret in the transcript. This security issue makes the PR unsafe to merge until packed sensitive arguments are redacted correctly. Sequence Diagram(s)sequenceDiagram
participant MCPRuntime
participant CLI
participant TUIModel
participant MCPState
participant MCPView
MCPRuntime->>CLI: Return skipped server failures
CLI->>TUIModel: Pass MCPSkipped through tui.Options
TUIModel->>MCPState: Build failed server state
MCPState->>MCPView: Provide redacted failure reason
MCPView->>MCPView: Sanitize and bound rendered output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/app_mcp_skipped_test.go (1)
63-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the failure reason is forwarded.
The test only verifies the server name. Also assert
MCPSkipped[0].Errcontains"connection refused"so a regression that drops the recorded error cannot pass.Proposed test strengthening
if len(launchedOptions.MCPSkipped) != 1 || - launchedOptions.MCPSkipped[0].Name != "docs" { + launchedOptions.MCPSkipped[0].Name != "docs" || + launchedOptions.MCPSkipped[0].Err == nil || + launchedOptions.MCPSkipped[0].Err.Error() != "connection refused" { t.Fatalf("MCPSkipped = %#v, want the failure startup recorded", launchedOptions.MCPSkipped) }As per coding guidelines, add a regression test for behavior changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/app_mcp_skipped_test.go` around lines 63 - 66, Strengthen the existing MCPSkipped assertion in the test by also verifying that MCPSkipped[0].Err contains “connection refused,” while preserving the current server-name check and failure-count validation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/app_mcp_skipped_test.go`:
- Around line 63-66: Strengthen the existing MCPSkipped assertion in the test by
also verifying that MCPSkipped[0].Err contains “connection refused,” while
preserving the current server-name check and failure-count validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6847c24c-e809-446d-80a2-a6db7325507d
📒 Files selected for processing (8)
internal/cli/app.gointernal/cli/app_mcp_skipped_test.gointernal/tui/command_views.gointernal/tui/mcp_failed_state_test.gointernal/tui/mcp_state.gointernal/tui/mcp_view.gointernal/tui/model.gointernal/tui/options.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
The failed-state wiring and secret redaction look good, but the new failure-reason rendering needs terminal sanitization before merge.
BuildMCPViewState passes redaction.ErrorMessage(err, ...) into MCPServerView.Error, and mcpManagerServerLines inserts that value directly into the rendered lines. Redaction removes credentials but does not remove ANSI/OSC sequences, other control characters, or embedded newlines. I reproduced this with an MCP error containing connection refused\x1b[2J\n› forged · enabled; the resulting server line retained both the escape sequence and newline unchanged. A server-controlled handshake error can therefore manipulate the terminal or forge extra /mcp rows.
Please normalize the displayed reason to safe single-line terminal text: strip ANSI/OSC and control characters, flatten CR/LF, apply a reasonable length cap, and add a regression covering escape and newline injection.
Everything else in the change looks correct, and the focused tests and CI are green.
c7421d9
|
@anandh8x fixed in Before the fix the panel rendered your payload as: Escape sequence intact, forged row on its own line.
Two regressions, both of which fail on the previous commit: one asserts no escape byte survives, no line carries its own newline, the forged text never starts a row, and the real reason still shows. The other pushes 5000 characters through and asserts the line stays bounded. Re-requesting you and @kevincodex1, since the push dismissed his approval. One thing worth flagging beyond this PR: the same class exists at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/mcp_view.go`:
- Around line 192-238: Bound MCPServerView.Error before processing in the
sanitizer around the visible rune conversion and strings.Builder accumulation.
Verify whether Runtime.Skipped() already imposes a strict size limit; if not,
limit the raw input before converting to []rune and stop accumulating once the
maxMCPReasonLen display budget is reached, while preserving ANSI stripping,
whitespace normalization, and truncation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63dc33b5-a906-45b6-b233-5fc22f09514c
📒 Files selected for processing (2)
internal/tui/mcp_failed_state_test.gointernal/tui/mcp_view.go
|
Pushed one more commit for the bot's finding, which was real. The 400 rune cap ran at the end, so the sanitizer walked the whole server string first, and escape sequences get consumed without producing output. 64KB of The bot's other claim on #866 (duplicate |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve
Verified empirically on the branch (checked out, built).
What I checked
- Gut-the-fix: forcing the failure branch off (
mcp_state.go:71) so a skipped server renders as "enabled" turns the TUI MCP tests red. The tests exercise the behavior. - Precedence is right and tested: a server that is both disabled and recorded-failed shows as "disabled", not "failed" (
mcp_state.go:66-69), and this is asserted directly (mcp_failed_state_test.go:45— "disabled to win over a recorded failure"). Correct — a server you turned off was never expected to connect. - Reason is redacted before display (
redaction.ErrorMessage,mcp_state.go:73) — invariant 6, so a path/credential in a startup error can't leak into the panel. Empty-error fallback ("server did not start") is handled too. - Skipped set reaches the panel via
MCPSkipped: mcpRuntime.Skipped(); companion to #822 which did the same forzero mcp check. - Clean scope — every file is the MCP panel or its plumbing.
Worth a quick confirm (non-blocking)
- On reconnect (
zero mcp enableafter a failure), the panel is rebuilt from a freshSkippedset, so it should flip back to enabled — worth a sanity check that the runtime clears the entry on a successful re-register.
Good fix.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge and review state
- Review decision: GitHub still reports
CHANGES_REQUESTED(anandh8x’s review onbe580766is not dismissed). Terminal sanitization and raw-input bounding from that review look addressed on the current head, but the merge gate still needs a fresh approval on6521c367. gnanam1990 approved on6521c367; CodeRabbit approved on the same head. - Mergeability:
MERGEABLE, no conflict markers in the PR diff.mergeStateStatusisBLOCKEDpending required reviews. - Checks: All CI / CodeQL / Zero Review / CodeRabbit checks passed on head.
Prior review alignment
gnanam1990’s approval is valid for what they checked: skipped servers map to failed, disabled wins over recorded failure, reasons are redacted at build (redaction.ErrorMessage), MCPSkipped reaches the TUI, and the focused tests go red when the failure branch is gutted. Those checks exercise buildMCPServerViews, renderMCPView, and m.mcpText() — not the bare /mcp manager overlay entry point. Their non-blocking note about enable clearing Skipped on reconnect is separate from this finding; live reconnect from the TUI is out of scope per the author, and mcpSkipped remains a startup snapshot.
That approval does not negate the remaining gap below: state and redaction work on the transcript/renderMCPView path, but the overlay users get from bare /mcp still omits the reason line.
Findings
- [P2] Bare
/mcpshowsfailedin the manager overlay but not the failure reason
internal/tui/model.go(commandMCP→openMCPManager),internal/tui/mcp_manager.go(mcpManagerOverlay,mcpManagerServerMeta,mcpManagerSelectionDetail),internal/tui/mcp_view.go(mcpManagerServerLines,renderMCPView)
Empty/mcphas long routed toopenMCPManager()— this PR did not change that. What it did change is meaningful:buildMCPServerViewsnow marks skipped servers asfailed, so the overlay meta and detail pane correctly sayfailedinstead of the pre-PRenabledwith missing tools. The recorded reason, however, is rendered only inmcpManagerServerLinesinsiderenderMCPView(). The overlay never readsserver.Erroror callssanitizeTerminalReason, so a user who types/mcpafter a startup warning sees the right state but not the “why” issue #825 and this PR’s description target. TherenderMCPViewpath does show the sanitized reason — on/mcp listand other transcript subcommands, and in transcript output appended after manager actions such as check or list — but the primary overlay surface is still incomplete relative to the stated fix.TestModelMCPPanelReportsStartupFailuresexercisesm.mcpText()/renderMCPView(), not the bare/mcpentry point. Please surface the sanitized reason in the manager overlay (list meta, selection detail, or both), reusing the samesanitizeTerminalReasonpathmcpManagerServerLinesalready uses.
ff70596
6521c36 to
ff70596
Compare
An empty /mcp opens the manager overlay, and it reported the state without the reason. The recorded "why" was rendered only by mcpManagerServerLines inside renderMCPView, which serves /mcp list and the transcript, so the panel said "failed" and stopped exactly where someone goes to find out why after the startup warning has scrolled away. The reason now sits in the selection detail, directly under the header and above the target, through the same sanitizeTerminalReason path the transcript uses. TestModelMCPPanelReportsStartupFailures drives m.mcpText() and passes with or without this, which is how the gap survived review. The new tests drive openMCPManager().mcpManagerOverlay() instead. Mutation-verified: feeding the sanitizer an empty reason fails the first one. The sanitization test deliberately does not search for a bare escape byte. The overlay is lipgloss-styled and therefore full of escape sequences it wrote itself, so the assertion is that the SERVER's payload did not survive: no clear-screen sequence, and no row carrying the forged text on its own. The sanitizer collapses the newline, so the forged text stays inert on the reason line rather than becoming an entry of its own. Reported by jatmn on #835.
|
Rebased onto main and fixed the overlay gap. Thanks @jatmn, that was exactly right and the reason it survived review is worth stating. The finding. Bare Why it got through. One thing I got wrong while writing the sanitization test, worth recording. My first version asserted the overlay contained no The rebase. Two conflicts against #884:
@anandh8x your changes-requested predates the terminal-sanitization work you asked for, which landed a while back; a re-look when you have a moment would unblock this. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Redact the configured server credentials before retaining its error
internal/tui/mcp_state.go:73
ErrorMessageis called with empty options, even though remote MCP clients send every value inMCPServerConfig.Headers. A server can echo an arbitrary configured value (for exampleX-Workspace-Credential: <value>) in its failed startup response; that value is not covered by the generic redaction patterns, is saved inMCPServerView.Error, and is newly rendered in both/mcpsurfaces and the session transcript. Pass the configured secret values into redaction (and cover an echoed custom header); this also avoids relying on the syntacticAuthorization:matcher, which terminal control bytes can evade before the later sanitizer removes them.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Overall guidance
The remaining findings are not three unrelated corner cases. This change crosses two boundaries that currently use different definitions of the same data:
- An MCP server has both an exact configuration identity (the raw map key used to update or remove it) and a canonical runtime identity (the trimmed name used by registration, skipped observations, tool ownership, and tool counts). The current implementation sometimes stores only the canonical name and elsewhere reimplements collision rules from raw keys. That makes each individual consumer look locally correct while the end-to-end config → runtime → view → action round trip is inconsistent.
- A skipped-server error is remote-controlled text that is now rendered and persisted. The redaction code correctly recognizes that a known composite credential may be echoed in a derived spelling, but its work bounds can currently change which spellings receive protection. A resource bound must limit computation without weakening the confidentiality invariant at the threshold.
Please address those invariants centrally rather than adding another special case at each failing call site. For identity, carry the raw config key and canonical runtime name as distinct values, and use one shared definition of whether a server claims an active runtime identity across validation, writes, observation matching, view construction, and manager actions. For failure safety, define one bounded candidate policy in which every value already classified as a credential retains protection for the supported whole and derived spellings regardless of size.
The regression matrix should exercise the public behavior on both sides of each boundary: a single padded key; an enabled key plus a disabled canonical alias; two enabled aliases; add/update/enable/disable/remove/check through the manager and CLI; and composite credentials just below, at, and above the 8 KiB boundary when the server echoes the whole value or only its token suffix. Testing those cases through the assembled view/action and config-write paths—not only the normalization and candidate helpers—should prevent another locally correct fix from leaving a downstream consumer inconsistent.
Merge readiness
- [P1] Rebase onto current
mainbefore merge
AGENTS.md:75
This head is based on27b319ca, while the live target iseeea3308. The synthetic merge is conflict-free, but the repository explicitly treats a stale review base as a hard blocker. Please rebase onto the current target and rerun the required validation against the combined tree.
Findings
-
[P1] Keep derived tokens protected past the candidate input bound
internal/tui/mcp_state.go:1104
addKnownrecords the whole configured value and then relies oncredentialCandidatesto add the shorter spellings a server may echo. At this branch, however, any value longer than 8 KiB returns only the whole composite. With anAuthorizationvalue ofBearer <opaque-token>, a failed server that echoes only<opaque-token>no longer matches the whole candidate; an opaque token also has no generic shape for the fallback redactor to recognize. The first 400 characters then reach both/mcprender paths and the persisted transcript. This reproduces with an 8,198-byte opaque token, while the existing oversized regression covers only a server echoing the whole configured value.The root cause is that the work bound doubles as a change in the security semantics: crossing a length threshold silently removes a supported credential spelling. Keep the candidate count and processing cost bounded, but preserve a bounded number of semantic tails for values already classified as credentials—for example, the token portion of the supported
<scheme> <credential>and<header>: <scheme> <credential>forms. Add boundary regressions that echo only the suffix atlimit-1,limit, andlimit+1; whole-value redaction alone does not cover this contract. -
[P2] Preserve the raw config key for manager actions
internal/tui/mcp_state.go:127
This assigns the trimmed runtime name to the only identity carried byMCPServerView. The manager copies that value intomcpManagerItem.Name, uses it to look the selected server up again, and passes it tocheck,enable,disable, andremove. Those CLI operations address the configuration map by exact key. As a result, a supported single key such as" docs "renders correctly as runtime serverdocs, but every action is sent for a different, nonexistent config key. With enabled"docs"plus disabled" docs"—a configurationValidateUniqueNamesdeliberately accepts—the view produces two items both nameddocs; detail lookup returns the first match, and an action selected from the disabled row can instead operate on the enabled entry.The root cause is overloading one string with three roles: display label, runtime join key, and persistence/action key. Preserve the canonical name for display, skipped-failure lookup, and tool accounting, but carry the exact raw config key (or another unambiguous row/action identity) alongside it. Manager selection and commands must use that stable identity rather than re-resolving a row by its non-unique display name. Cover both a single padded key and the enabled/disabled alias pair through the actual manager item, detail, and command-dispatch paths.
-
[P2] Match the write collision rule to the disabled-server policy
internal/cli/mcp_config.go:648
ValidateUniqueNamesskips disabled entries because registration also skips them: a disabled server claims no runtime identity. This new write-side loop sees only key spellings and rejects every trimmed collision regardless of either server's disabled state. Therefore, with disabled" docs"already configured,zero mcp add docs ...fails even though the prospective combined configuration passes runtime validation. It also rejects a normal update of exact enabled"docs"when a disabled alias is present, and the current helper cannot correctly decide the inverse case because it receives the new name but not whether the new server is disabled.The root cause is a second, weaker implementation of the active-identity rule at the write boundary. Build the prospective combined configuration and validate it with the same shared rule used by the read/startup path, or make the collision helper consume both existing and incoming server state and the same
claims active identitypredicate. The resulting invariant should be: two enabled keys with the same canonical name are rejected, while a disabled entry on either side does not block the active server that replaces it. Add write-path tests for both directions and for updating the enabled entry while its disabled alias remains present.
The panel derived every server's state from config alone: `disabled` if the
user turned it off, `enabled` otherwise. MCP registration is best-effort —
a server that cannot be reached is recorded and startup continues — so a
server that never connected was listed as enabled with its tools silently
missing and nothing in the panel to explain it.
Startup already knows: it prints a warning per skipped server to stderr.
That scrolls away behind the first screen of output, and /mcp is where a
user goes afterwards to ask what is actually running.
Thread the skipped set from the MCP runtime through to the panel and render
a third state, `failed`, with the recorded reason underneath the server:
› docs · failed · stdio
exec: "docs-mcp": executable file not found in $PATH
The reason comes from the server, so it goes through redaction — a
handshake error that echoes back the Authorization header would otherwise
print the token into the transcript. Disabled still wins over failed: the
user turned that one off, so it was never expected to connect.
The stderr warning is unchanged; the panel is an addition to it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The failure reason is the only value on the /mcp panel that the MCP server writes itself, and it went to the terminal with nothing but TrimSpace. redaction.ErrorMessage strips credentials, not control bytes, so a hostile handshake error could clear the screen, move the cursor, or embed a newline followed by text shaped like a real entry and forge a row for a server that does not exist. Reproduced with @anandh8x's payload from the review. Before the fix the rendered panel was: > evil . failed . http connection refused\x1b[2J > forged . enabled actions: zero mcp check evil | ... The escape sequence and the forged row both survived intact. sanitizeTerminalReason consumes escape sequences whole rather than dropping ESC alone, since removing the ESC and leaving "[2J" behind would print visible junk and an abandoned OSC payload can still smuggle a title-set or hyperlink. CSI runs to its final byte, OSC to BEL or ST. Newlines and tabs collapse to spaces so the reason stays on the single row the panel counted for it, other control bytes are dropped, and the result is capped at 400 runes so one verbose server cannot push the panel off screen. Truncation is by rune, not byte, so a multi-byte character is never cut in half. Two regressions cover it. The injection test asserts no escape byte survives, no rendered line carries its own newline, the forged text never begins a row, and the real reason is still shown. The cap test drives 5000 characters through and asserts the rendered line stays bounded. Both fail on the code before this commit.
The display cap runs at the end, so the sanitizer walked the whole server-authored string first. Escape sequences are consumed without producing output, so they spend input against a budget that never fills: 64KB of "\x1b[2J" was walked in full and the text after it still rendered. Nothing upstream bounds the handshake error, and the panel re-runs this on every redraw. Cap the raw input at 16KB before the walk, well above the 400 rune display cap so a long error is still truncated by display rules. Trim back a character the cut splits so the panel never renders a replacement character it produced itself.
An empty /mcp opens the manager overlay, and it reported the state without the reason. The recorded "why" was rendered only by mcpManagerServerLines inside renderMCPView, which serves /mcp list and the transcript, so the panel said "failed" and stopped exactly where someone goes to find out why after the startup warning has scrolled away. The reason now sits in the selection detail, directly under the header and above the target, through the same sanitizeTerminalReason path the transcript uses. TestModelMCPPanelReportsStartupFailures drives m.mcpText() and passes with or without this, which is how the gap survived review. The new tests drive openMCPManager().mcpManagerOverlay() instead. Mutation-verified: feeding the sanitizer an empty reason fails the first one. The sanitization test deliberately does not search for a bare escape byte. The overlay is lipgloss-styled and therefore full of escape sequences it wrote itself, so the assertion is that the SERVER's payload did not survive: no clear-screen sequence, and no row carrying the forged text on its own. The sanitizer collapses the newline, so the forged text stays inert on the reason line rather than becoming an entry of its own. Reported by jatmn on #835.
…error
Reported by jatmn. BuildMCPViewState passed redaction.Options{} when rendering a
failed server's startup error, so only the generic patterns applied.
Those patterns match shapes they recognise. A remote MCP server is configured
with ARBITRARY headers, so a credential can sit under a name nobody can predict:
X-Workspace-Credential matches nothing. A server that echoes the request it
failed on puts that value into MCPServerView.Error, which this PR newly renders
in both /mcp surfaces AND the session transcript. So the exposure is one this
change introduces rather than one it inherits.
The configured values are now passed as ExtraSecretValues, which redacts by
equality instead of by shape, so the header name does not have to be guessable.
That also drops the dependence on the syntactic Authorization: matcher, which
terminal control bytes can split before the later sanitizer strips them.
Env values are included for the same reason on the stdio path: the child is
launched with them and a failure to exec commonly reports the environment it was
given. Auth and an OAuth client secret are included too.
Values shorter than eight characters are skipped. A configured "1" or "true" is
not a credential, and redacting it by equality would punch holes through
unrelated text, which is its own way of making an error useless. There is a test
for that, because the fix would otherwise be free to shred the message.
Tests cover an echoed custom header, an echoed env secret, the short-value case,
and a healthy server carrying no error text at all. Verified by mutation:
dropping the options renders the credential verbatim.
The reason is written by the server that failed, and it is both rendered and persisted to the transcript, so it is untrusted text that can carry back whatever Zero sent. Four separate ways a credential survived it. Redaction ran before the display sanitizer, and matched configured values literally. A server echoing a secret with a control byte pushed into the middle of it matched nothing, and the sanitizer then removed that byte WITHOUT leaving a gap and reassembled the intact credential on screen. Every control byte it drops rejoins the same way, so this was never specific to ANSI. The reason is now normalized to what the reader will see and redacted again against that. The stripping half is split out of sanitizeTerminalReason as stripTerminalRejoiners so redaction does not inherit the display truncation, which would cut a secret in half and leave the head of it unmatched. Only the OAuth client secret was redacted, not the bearer that is actually sent. A failed OAuth server can echo an opaque token in its error body, and no pattern can recognize one by shape. TokenStore.SecretValues reads that material. It enumerates rather than looking up by server name, because the login path saves identity-bound and a per-name Load finds nothing for exactly the servers holding a real bearer. It is read only: LoadForServer, which the runtime bearer path uses, migrates a legacy entry as a side effect, and opening a panel must not rewrite the token store. Args were not collected at all, though a stdio child that rejects its own invocation prints it back and connectStdio appends that stderr to the error. sensitiveMCPArgValues collects the values behind a sensitive flag, sharing the predicates with the display pass so the two cannot drift, and handling the shapes the display pass gets wrong or would answer with an already-redacted string. Extra secret values were replaced in slice order, so a secret that is a prefix of another consumed its head and left the tail of a real credential printed as [REDACTED]XYZ. The partial replacement also destroyed the token shape, so the pattern passes could not recover it. Callers collect from maps and Go randomizes iteration, so which happened was decided per run. RedactString now applies values longest-first and deduped, which fixes every caller rather than this one. Each fix is falsified independently by its regression: reverting the ordering prints [REDACTED]XYZ, reverting the normalization reassembles wk-live-4f9c2b7ae1d8 for display, reverting the arg collection prints the argument secret, and reverting the token read prints the stored bearer.
Adversarial review of the previous commit found three holes in it. Invisible Unicode rejoins like a control byte, and more comfortably. A zero-width space, soft hyphen, word joiner, bidi control or byte order mark inside an echoed credential is not equal to the configured value, so redaction misses it, and the reader sees an unbroken secret because the character renders as nothing. The whole Cf category is now dropped alongside the control bytes. Combining marks are deliberately kept: they are ordinary content in most of the world's scripts, and deleting them to close a redaction hole would corrupt error messages written in those languages. The configured value is usually not the credential. Zero's own documented config spells an authenticated server as "Authorization": "Bearer <token>", so a server quoting back only the token, without the scheme word, matched nothing. Same shape through a composite --header argument, which carries a header name too. Each tail after a space or colon is now offered as its own candidate, which covers both without needing to know the scheme vocabulary, and the length floor keeps "Bearer" and the header name out of the set so those words are not blanked out of unrelated text. Collecting argument values over-reached. isSensitiveMCPDisplayFlag strips leading dashes before matching, so it says yes to a bare positional word, and the documented GitHub server config passes the env var NAME positionally: the docker image name went into the redaction set and the pull failure lost the one string that explained it. A positional argument is not a flag and no longer introduces a value. Two of the tests for this were vacuous when first written and were rewritten after reverting the fix did not fail them. The Unicode one asserted on bytes, but these characters rejoin in the reader's eye rather than in the string, so it now asserts on the perceived text. The scheme one used an sk- style value that the shape patterns already caught, so it proved the pattern list rather than the fix; it now uses an opaque credential only equality can match.
…ored tokens of any length
…re, and stop reusing stale startup failures Five findings from review, plus one the review did not raise. Header flags were classified by their NAME. isSensitiveMCPDisplayKey matches token/secret/auth/credential and friends against the flag, and neither "header" nor "H" is any of those, so a credential riding in a header whose name the operator chose was never collected. All seven forms leaked once the header name itself carried no matching word: separated, equals and packed, long and short, and with no space after the colon. A stdio child that rejects its invocation echoes it into captured stderr, which this panel renders and the transcript keeps. The short form is matched case-sensitively on purpose, because -h is help and folding case there would put the next argument into the redaction set. The endpoint was never examined at all. HTTP and SSE send the configured URL verbatim and it accepts userinfo and arbitrary query keys, so ?workspace=<token> walked through the generic query redaction, which only recognises conventional key names. Userinfo passwords and conventionally-named parameters were already covered; the arbitrary name and the userinfo username were not. Every query value is collected now, with the existing length floor keeping v=1 and mode=sse readable. WHAT THE REVIEW DID NOT RAISE, found while verifying the first two: the Target row prints the same credential verbatim, one line under the Error row that is correctly redacted, on the same panel and into the same transcript. Fixing the error alone would have handed it straight back. The display path now redacts header values while keeping the header name, and long query values while keeping the host and path. The raw bound sat at the very end, inside sanitizeTerminalReason, so the whole server-controlled string was redacted, walked rune by rune into a fresh builder and a fresh []rune, and redacted again before being cut, for a panel that shows at most 400 runes. It is applied at ingress now, with a lookahead margin sized to the longest secret so a credential straddling the cut cannot lose its tail and leave a matching prefix visible. Failures were matched against raw config-map keys while registration records the trimmed name, so a server configured as " docs " that failed to start rendered as enabled, and lost its tool count the same way. One canonical identity now, and it is the registry's. And a skipped entry is an observation about a server, not about a name. The startup snapshot was never invalidated, so removing a failed endpoint and adding a different one under the same name made the replacement inherit the dead endpoint's error and failed state, in the panel and in the command transcript. Observations are dropped when their subject is removed or changed.
… forms Two problems, both in the path that turns a failed MCP server's error into panel text. The raw bound was outside redaction.ErrorMessage rather than around the error going into it, so the whole server-controlled string was redacted, walked rune by rune, redacted again, and only then cut. An eight megabyte reason took 2.2s and allocated 286MB for a panel that shows 400 runes; it is now 9ms and 1.5MB, flat across input sizes. The lookahead margin past the cut was sized to the longest configured secret, which made the real limit "the cap plus whatever the other side configured". A two megabyte credential raised the retained error to 65546 bytes against a nominal cap of 16384. It is a fixed 4KB now. A credential longer than that can still straddle the cut and leave a prefix, which is a stated limit rather than an oversight, and a far smaller one than an unbounded margin. Credential collection only ever saw decoded values. url.Parse and url.ParseQuery decode, so a token configured as opaque%2Dworkspace%2Dtoken was collected as opaque-workspace-token and matched nothing when the server echoed back the escaped spelling it was given. parsed.User.String() is not a way out either: it re-escapes by Go's rules and leaves unreserved characters alone, so %2D comes back as a hyphen. Both forms are collected now, the raw one taken from RawQuery and from the original string's userinfo.
The existing regressions call redactMCPFailureReason directly, which proves the helper and nothing else. BuildMCPViewState is the path the panel and the transcript actually take, and it is where a second surface could reintroduce either problem: the row renderer inspects the raw query field separately from the error pipeline. Four cases through the entry point: an arbitrary percent-encoded query key, percent-encoded userinfo, a multi-megabyte failure, and a multi-megabyte configured secret. Each one fails without its fix. Reverting the fixed window reproduces the 65546-byte retained error against a 20480-byte budget, and reverting the ingress bound puts state building back over six seconds. One honest note on coverage: the percent-encoded userinfo PASSWORD case passes either way, because generic URL redaction already covered passwords. The query key and the username are the two that were actually leaking.
…edential material Three things, all in the failure-display pipeline. The bound cuts the raw error before redaction, and redaction matches whole values, so a credential the cut sliced in half matched nothing and its surviving prefix was ordinary text. The fixed overlap made that need a secret longer than the window, and nothing caps a configured header, URL, environment or stored token value, so it was a configuration away rather than impossible. A server can also spend the raw budget on control sequences that later vanish, putting the start of the credential right at the cut and its prefix at the top of the panel. Only the final cut can split anything, so the fix looks at the tail alone and drops any run that begins a configured secret. It costs one comparison per secret against a bounded window and does not care how long the secret is, which is the property a wider overlap could never give. credentialCandidates walked every suffix after every space or colon and kept them all, so a delimiter-heavy value produced thousands of candidates and RedactString ran a replacement pass for each. That expansion is on the config side, outside the raw-error bound, so the cost did not depend on the server's error being long: a value with 4000 delimiters yielded 7999 candidates however short the failure was. Input size and candidate count are bounded now. The value itself is still redacted whole; only the suffix enumeration is dropped, and the tails exist for one narrow case, a header configured as "Bearer <token>" whose server echoes only the token. And the path was treated as an identifier while query and userinfo were treated as secret-bearing. The configuration contract accepts an arbitrary HTTP or SSE path and opaque path-segment credentials are an ordinary endpoint convention. This needs no crafted response body: a failing http.Client.Do returns a *url.Error carrying the request URL, which the failed-server path wraps and renders, so the token reached the reason, the panel and the transcript, with the target row showing it too. Opaque segments are collected for redaction and replaced in the displayed target, by the same length floor used elsewhere, so a route like /v1/sse survives and the operator can still tell what failed. One note on the first test I wrote for the oversized case: it used an "sk-live-" prefix, which the generic patterns catch whatever the bound does, so it passed with the fix removed and proved nothing. The fixture is opaque now and fails with a 2800-character prefix reaching the panel.
…open Five leaks, four of them the same mistake in different places: a rule that was sized to a constant, or to a heuristic, instead of to the thing it was guarding. The tail repair inspected a fixed 4 KiB at the end of the rendered text, so a credential beginning before that window could never be matched: the inspected span starts partway through it, and a middle is not a prefix. Measured here, a 6000-byte value positioned across the cut left 5000 of its bytes on the panel. The search is now sized to the credential and answered in one KMP pass, so the work is linear in the operator's own configured value rather than in anything the remote server sent. The flat eight-byte floor went with it: seven bytes of an eight-byte credential is the credential, so the rule is proportional as well as absolute. Values already known by provenance to be secret, an OAuth client secret and the value of a credential-bearing flag, were routed through the ambiguity heuristic that exists to keep v=1 and mode=sse readable, and were discarded for being short. They skip it now; genuinely ambiguous values still do not. The OAuth endpoints were outside the candidate set entirely, although a refresh posts to TokenEndpoint during startup and a dial failure comes back wrapped in a url.Error that keeps the path and query. The collector and the target row each derived the accepted header spellings separately and neither recognised the conventional attached form, so the value was missing from the redaction set and printed verbatim one row below. Both go through one parser now. And the retained startup failure kept the raw error, re-redacted on every render from whatever the token store held at that moment, so logging out deleted the bearer that was hiding itself and the next render wrote it into the panel and the transcript. The observation now carries a fingerprint of the material that made it safe, and withholds the reason rather than re-deriving a weaker one. A fingerprint, not a copy: a second plaintext store would be its own problem.
Three findings with one shape: a classification made after the evidence for it had already been discarded. Header, environment and query values were flattened into bare strings before the readability heuristic ran, so a value under a key that names it as a credential went through the floor that exists to keep mode=sse and v=1 readable. API_KEY=s3cr3t contributed no exact candidate, and a child echoing the value on its own reached the panel and the transcript where generic shape matching has nothing to recognise. Keys now travel with their values to the decision, and the endpoint parser returns the key-classified parts separately from the ambiguous ones. The userinfo password is classified by position, since no key names it. The Target row recognised a flag packed with its value as sensitive, printed the whole element verbatim, and redacted the NEXT argument instead, so the row under the redacted reason carried the credential and blanked an unrelated flag. The display now parses the packed form the way the collector already did. And raw.Auth was being treated as credential material although it is the public authentication MODE selector, whose only accepted value is the word the panel itself displays. Every failure from the OAuth stack lost the token naming the subsystem: "oauth: fetch authorization server metadata" became "[REDACTED]: fetch authorization server metadata". That was invisible while ambiguous values ran through the length floor, which discarded a five-character string on its own; removing the floor for known provenance is what surfaced it, which is the tell that the field was miscategorised rather than the floor load-bearing.
…reached Startup now splits MCP into a critical set registered before the TUI launches and an optional set, the unconfigured built-in defaults, registered on a background goroutine. The critical branch only runs when something is configured, and this test stubbed registration without configuring any server, so after the rebase the stub was never called and the assertion failed against a nil list. It stubs resolveMCPConfig with a configured server now, which is what puts the failure in the half startup registers synchronously and hands to the TUI. Worth recording what this does NOT yet cover: an unconfigured default that fails is registered asynchronously, so its skipped entry does not exist when the model is constructed and never reaches the panel. That is the same observation-timing boundary as the outstanding review finding about binding the redaction context at capture, and it is fixed there rather than here.
…observed with Two structural problems behind the panel's redaction, both about identity. The runtime name is an identity, so it has to be unique. Registration trims the config key, so "docs" and " docs" were two configured entries and one runtime server: they shared a tool count and a failure, map iteration decided which configuration survived, and each row redacted that shared error with its own candidate set, so the row that did not fail could print the other's credential. NormalizeConfig now refuses two names that resolve to one identity, and the config writer refuses a key that collides with an existing one, since validation on the way in only sees the incoming server and cannot detect the collision. A single padded name still works; trimming was never the problem. The context that makes an error safe has to be recorded where the error is produced. A skipped entry keeps the raw failure and is redacted at display time against whatever the token store holds then, so the surface needs to know whether that set is still the one that was hiding the credential. It was sampled when the surface was built, which is after registration and after anything in between could have rotated the store, and a 401 during connect refreshes the bearer that the same attempt's error text quotes. SkippedServer now carries a fingerprint sampled before connecting, and the panel prefers it over its own sample. Also: optional servers register on a background goroutine, and the runtime wrapper returned nil for its skipped list unconditionally. Moving them off the critical path is a scheduling decision, not a visibility one, so every one of them rendered from configuration alone: enabled, unexplained, for a server that never connected. The panel now pulls those failures and refreshes when one arrives.
…up split Startup separates unconfigured built-in defaults from the servers the user asked for, and the two halves are normalized by separate calls. A collision that straddles the split is invisible to both: a user-configured " exa" is critical while the built-in "exa" is optional, and they are one runtime server with two panel rows sharing a failure, a tool count, and each other's redaction context. The check moves out of NormalizeConfig into ValidateUniqueNames, which NormalizeConfig still calls, and startup runs it on the merged configuration before splitting.
…mpletes Optional MCP registration runs on its own goroutine so a slow server cannot delay the first response, which means its result arrives with no user input behind it. Bubble Tea renders only in response to a message, so reporting late failures through a getter was not enough on its own: a manager opened while an optional server was still connecting kept showing the configuration-derived enabled state until unrelated input or a resize happened to redraw it. The startup's completion channel is now surfaced to the model, Init schedules a wait on it, and the resulting message rebuilds the MCP view state. Startup stays non-blocking, tool readiness is untouched, and unconfigured built-in defaults still produce no startup warning. The regression asserts on the rebuilt cache rather than on a render. Rendering calls mcpViewState, which invalidates on demand, so an overlay drawn after the message looks correct even when the handler does nothing: the first version of this test passed with the rebuild deleted, which is the failure mode it exists to catch.
…alias ValidateUniqueNames deliberately accepts an enabled "docs" alongside a disabled " docs": a disabled entry claims no runtime identity during registration. canonicalMCPServers copied both into a map keyed by the trimmed name, so they collided, and Go randomises map iteration. The before and after snapshots each picked a winner independently, so an unrelated /mcp operation that left the enabled entry untouched could compare it against the disabled alias, find them different, and discard the failure. A server that was still unavailable then showed as fine, on roughly one run in five. Exclude disabled entries, the way NormalizeConfig does when it decides what to register, so aging operates on the same active-server set. A canonical name claimed by two ENABLED entries is ambiguous rather than arbitrary: ValidateUniqueNames rejects that config, but if it arrives there is no way to say which entry an observation was about, so the name is dropped and the observation ages out with it.
dropTrailingSecretPrefix runs on every /mcp state rebuild, once per configured or stored credential, and longestPrefixSuffix built pattern+sentinel+text with an []int over the whole thing. The candidate's length therefore drove the work, and configured headers, env values, URL components, OAuth fields and token-store values have no size limit. Three 2 MiB candidates measured 54.5 MiB and 33ms on every rebuild, against a render budget that is nominally fixed. The raw-error cap bounds the server-controlled message but not this pass, so a bound sized to the attacker's input was no bound. The longest prefix of the candidate that is a suffix of the text can be at most len(text) long, so any longer prefix is unreachable and scanning it changes no answer. Truncate the pattern to the text length before the search. The same three candidates now measure under 1 MiB. A differential test checks the truncated search against the definition across many inputs so the bound cannot change a result, and it catches a one-byte-wrong bound.
The input cap returned the whole value and nothing else once a configured value crossed 8 KiB, which made a work bound into a change of security semantics. An Authorization value of "Bearer <opaque token>" longer than the cap stopped offering the bare token as a candidate, so a failed server echoing only the token matched nothing; an opaque token has no shape for the fallback redactor to catch either, and the first 400 characters reached both /mcp render paths and the persisted transcript. Measured before the fix: an 8205-byte value yields 1 candidate with the bare token absent, while the same shape at 71 bytes yields 2 with it present. The threshold alone decided whether the credential was redactable. Bound where a separator is LOOKED FOR instead of how long a value may be. Every convention this walks, "<scheme> <credential>" and "<header>: <scheme> <credential>", puts its separators in a short prefix, so scanning that prefix keeps the cost fixed while the tails survive at any length. The candidate count stays capped, and the delimiter-heavy cost regression that motivated the original bound still passes. The old oversized test asserted the defect as the contract: it required exactly one candidate for a long value. It is replaced by tests that a long credential still yields its token, that the header form yields both tails, and that the scan boundary is where it says it is.
MCPServerView carried only the canonical runtime name. That is the right spelling for display, for joining against skipped-failure records and for tool counts, and it is deliberately not unique: ValidateUniqueNames accepts an enabled "docs" beside a disabled " docs" because a disabled entry claims no runtime identity. The manager copied that name into its item, looked the selected server up again by it, and passed it to check, enable, disable and remove. Those address the configuration map by exact key. So a single padded key such as " docs " rendered correctly and sent every action to a key that does not exist, and with an alias pair both rows rendered as "docs", detail lookup returned whichever came first, and an action chosen on the disabled row could operate on the enabled entry. One string was doing three jobs: display label, runtime join key, and persistence identity. The first two are the same and stay on Name; the third is now ConfigKey, the exact map key, and every action and lookup uses it. The existing view expectations gained the new field rather than being loosened: the value they now assert is the exact key each fixture is configured under.
refuseColliding compared key spellings and rejected every trimmed collision. That is a second and weaker implementation of a rule the read and startup paths already own: ValidateUniqueNames skips disabled entries, because registration skips them and a disabled server claims no runtime identity. So the write side refused combinations the running configuration accepts. With a disabled " docs" configured, `zero mcp add docs ...` failed, and even a plain update of the enabled "docs" was refused while that alias existed. It also could not decide the inverse case at all, because it never received whether the incoming server was itself disabled. Build the prospective combined configuration and hand it to the shared rule, rather than teaching the local copy the same exceptions. The invariant is then identical on both sides by construction: two enabled keys resolving to one canonical name are refused, and a disabled entry on either side does not block the active server. Covered in both directions, including updating the enabled entry while its disabled alias remains, which is the case the old rule got wrong most quietly.
99b3d67 to
072e036
Compare
|
All four addressed on P1, the credential tailsConfirmed and fixed. The input cap turned a work bound into a change of security semantics, exactly as you describe. Measured before the fix, with an opaque token and only the length varying: So the threshold alone decided whether the credential was redactable, and an opaque token has no shape for the fallback to catch. Your point that whole-value redaction does not cover the contract is the operative one. The bound is now on where a separator is LOOKED FOR rather than how long a value may be. Both conventions this walks put their separators in a short prefix, so the cost stays fixed and the tails survive at any length. The delimiter-heavy regression that motivated the original bound still passes, which was the thing I most wanted to keep. Worth flagging: the existing oversized test asserted the defect as the contract. It required exactly one candidate for a long value, so it would have gone green against the bug forever. It is replaced by tests that a long credential still yields its token, that the header form yields both tails, and that the scan boundary is where it claims to be. P2, the row identityRight, and the root cause framing is right too: one string doing three jobs. The canonical name is correct for display, for joining against skipped-failure records and for tool counts, and it is deliberately not unique. Actions address the config map by exact key, so a padded single key sent every action to a key that does not exist, and with an alias pair an action chosen on the disabled row could operate on the enabled one.
One note on a side effect, since it is the kind of thing worth being explicit about: adding the field broke an existing view expectation. I added the field to the expectations rather than relaxing the comparison, so the test now asserts the exact key each fixture is configured under. Loosening it would have been easier and would have left the new field unpinned. P2, the write collision ruleFixed by deleting the second implementation rather than teaching it the same exceptions. Covered in both directions, including updating the enabled entry while its disabled alias remains, which is the case the old rule got wrong most quietly. RebaseDone. Nine checks green with the last still running when I posted this. |
|
@anandh8x this is still blocked on your review from August 3, and I believe the finding is closed. Flagging it rather than letting it sit. Your reproduction runs clean on the current head I re-ran your exact input. No rush, but whenever you have a moment, could you take another look? jatmn approved at this head. |
6 participants
Fixes #825. Companion to #822, which fixed the same blind spot in
zero mcp check./mcpworked out each server's state from the config file — disabled if you turned it off, enabled otherwise. But MCP registration is best-effort: a server that can't be reached gets recorded and startup carries on. So a server that never connected showed up as enabled, its tools quietly missing, and nothing in the panel said why.Startup does know — it prints a warning per skipped server to stderr. That's gone by the time you notice, and
/mcpis exactly where you go afterwards to ask what's actually running.So the skipped set now reaches the panel, and a server that failed renders as failed with the reason under it:
Two details worth calling out:
The reason comes from the server, so it goes through
redaction.ErrorMessagebefore it's rendered. A handshake error that echoes theAuthorizationheader back would otherwise print the bearer token straight into the transcript. There's a test for that.Disabled wins over failed. If you turned a server off it was never expected to connect, and calling it failed would be misleading.
The stderr warning is unchanged — non-interactive users still get it, and the panel is an addition rather than a replacement.
Still not fixed, and out of scope here: enabling a server from inside the TUI updates the config but doesn't reconnect anything, so it'll show as enabled while not actually running until you restart. That's pre-existing and a bigger change; happy to file it separately if you'd like.
Verified with mutation testing — eight mutations across the state builder, the renderer, and both wiring points, all killed.
TestAltScreenTranscriptScrollKeepsFooterFixedandTestBuildServeScopeKeepsLexicalPathsfail on my Windows box on cleanmaintoo (the second needs symlink privilege).Summary by CodeRabbit
New Features
/mcppanel.Bug Fixes