Skip to content

Lite: the MCP server uses a token once turned on, with a Copy MCP config button in Settings - #6557

Merged
erikdarlingdata merged 6 commits into
devfrom
fix/lite-mcp-loopback-token
Oct 23, 2026
Merged

erikdarlingdata merged 6 commits into
devfrom
fix/lite-mcp-loopback-token

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 22, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Lite's MCP server uses a token once it is turned on, like Darling's (the same change in Darling is #6507).

What changes

  • Gate. Every MCP request needs Authorization: Bearer <token>. No token, a wrong token or a malformed header is answered 401 with WWW-Authenticate: Bearer, before the JSON guard and MapMcp. The compare is constant-time. The whole pipeline (Host guard, token gate, JSON guard, MapMcp) is one method, McpHostService.ConfigurePipeline, that ExecuteAsync and the tests both run.

  • The token is required. McpHostService takes the token and allowTokenlessLoopback as required arguments. A missing token without the explicit opt-out throws, so MCP does not start. There is no default that installs no gate.

  • Token name. The token is stored for the signed-in Windows user as PerformanceMonitorLite.Mcp_McpToken, its own name, apart from the PerformanceMonitorLite_ names a server's saved password uses. No server Id can produce it. It has not shipped, so nothing is migrated. 32 random bytes as 64 lowercase hex characters, made the first time the server starts with none.

  • Saved passwords need a GUID Id. A server's saved password is read, saved or deleted only for a server whose Id is a GUID (ServerCredentialIds). Any other Id stays in the list and is monitored, with no saved password and one log line naming its display name. The older fixed names (pseudonym key, webhook URLs, SMTP) are untouched and not migrated. Covered: connection string and stored-credential check (ServerConnection), add, edit and delete (ServerManager), the edit dialog's pre-fill and its delete.

  • Opt-out serves reads only. mcp_allow_tokenless_loopback (default false, file only) serves the read tools with no token. The write tools need mcp_allow_writes and a token. The investigation tools need mcp_allow_investigations, a token and mcp_allow_writes off. A token counts only at 16 characters or more with at least 6 different characters (a shorter or more repetitive one still gates requests but serves no write or investigation tool). One Critical line per group that was asked for and stays off. This is decided in McpHostService.DecideServing, from the constructor, the same rule as Darling's DarlingMcpWritePolicy.ResolveForStart.

  • New token. Settings, Access has a New token button next to Copy Token. It makes and stores a new token. The running host refuses the old one from the next request, and one line says the MCP client needs the new config.

  • Clipboard. Copy Token and Copy MCP config (when it carries a token) set ExcludeClipboardContentFromMonitorProcessing, CanIncludeInClipboardHistory = 0 and CanUploadToCloudClipboard = 0 (DWORD values, per Microsoft's "Cloud Clipboard and Clipboard History Formats"), so the text stays out of Clipboard History and cloud sync.

  • Import Settings. An imported settings.json loses mcp_allow_tokenless_loopback, mcp_allow_writes and mcp_allow_investigations; the current user's values stay. Other keys are copied as before. The file is read as text, so a UTF-8 BOM or UTF-16 file is handled; a settings.json that is not a readable JSON object is skipped (logged, not counted as copied).

  • Text. The host comment, the write-tools log line (now "to any client that has the MCP token") and the docs and sample settings say the token is not kept in a Lite file and that the MCP client keeps its own copy in its config.

  • New token reloads the host. Closing Settings after New token always restarts the host from the stored token, even when the window holds no host reference. The status line says "The old one stops working when you close Settings" when the live swap was not available.

  • Copy from the token box. Ctrl+C and the context-menu Copy in the token box go through the same clipboard path as Copy Token (kept out of Clipboard History and cloud sync).

  • Startup log lines name the count of write tools instead of a partial list.

Review round 2 fixes: import reads BOM/UTF-16 files and skips unparseable ones; New token always reloads; copy from the box uses the same clipboard path; the serving record never prints the token.

Not tested: the New token reload needs the Settings window, whose New token click writes the real Credential Manager token (ForApp), so no test can drive it safely; the change is one line, McpSettingsChanged = true.

Server Id sources

Lite makes every new server Id with Guid.NewGuid() (the model default): Add Server and Add Multiple Servers do not set it, and the edit dialog keeps the existing one. The one way a non-GUID Id reaches the list is a servers.json that was edited by hand or imported (Import Settings and ImportServersFromFile keep the Id from the file as-is, so existing saved passwords still match). No CLI, sample file or doc assigns one. Credential profile keys are profile_<id>, so they cannot spell the token's name.

Test plan

  • McpHostGateLiveTests runs the real McpHostService on a free localhost port: no token, a wrong token and the right token on tools/list; opt-out with writes and investigations asked lists no write or investigation tool; a 15-character token serves none, a 16-character one serves the writes; investigations need writes off; New token makes the old token fail at once; no token without the opt-out throws; one Critical line per group left off.
  • McpTokenIsolationTests: the token entry is separate from a server entry named McpToken; a server with Id McpToken (and one with another non-GUID Id) gets no saved password while a GUID server does; adding a non-GUID server keeps it and saves nothing; the three clipboard formats; Import Settings drops the three settings and keeps the others.
  • Round 2: SettingsImportTests, McpHostGateLiveTests, McpTokenIsolationTests (BOM and UTF-16 import, unreadable file skipped, ToString has no token, production-shaped prefixes), McpTokenGateTests, McpWritePolicyTests, McpInvestigationGateTests, Settings window classes, every Census and Ratchet class: 146 run, 0 failed.
  • Ran 131 tests, 0 failed: McpHostGateLiveTests, McpTokenIsolationTests, McpTokenGateTests, McpWritePolicyTests, McpInvestigationGateTests, SettingsImportTests, McpToolsListBudgetTests, PseudonymKeyTests, and every Census and Ratchet class in Lite.Tests. Build: 0 warnings, 0 errors.
  • Removed one spelling test in McpWritePolicyTests (it pinned the host's argument list as source text); the behaviour tests above replace it. McpInvestigationGateTests calls the investigation start line with the new token argument.
  • Not run: the full suite (CI decides), and a click-through of the Settings window (the XAML builds; no UI session was run).

Proposed CHANGELOG line

  • Lite's MCP server uses a token once it is turned on ([Lite: the MCP server uses a token once turned on, with a Copy MCP config button in Settings #6557]) - Lite makes a token the first time the MCP server starts, keeps it in Windows Credential Manager for your Windows user under its own name, and refuses requests without it. Settings, Access has Copy Token, Copy MCP config and New token. With mcp_allow_tokenless_loopback on, only the read tools are served. A saved server password is used only for a server whose Id is a GUID. Import Settings no longer brings in the MCP gate settings.

Merge of dev after #6504

Merged dev after #6504: every saved-credential read runs both the GUID-Id check and the confirmed-host check.

The one textual conflict was ServerManager.DeleteServer. Kept both: the saved password is deleted only when the Id is a GUID (ServerCredentialIds.Allows), then the host binding is forgotten (_hostBindings.Forget). ServerConnection.cs, AddServerDialog.xaml.cs and MainWindow.xaml.cs merged cleanly and were read after the merge.

Path GUID-Id check Confirmed-host check
CredentialResolver.GetConnectionString, GetQueryStoreReplicaConnectionString, GetUtilityConnectionString, GetConnectionStringForDatabase (used by the collectors, plan fetcher, MCP plan tools, FinOps, ServerTab, ExcludedDatabasesDialog, ServerManager connection checks) ServerConnection.ResolveAuth reads the saved password only if ServerCredentialIds.Allows(Id, DisplayName) RequireConfirmedHost runs first in each method
Edit dialog, SQL password prefill (AddServerDialog) ServerCredentialIds.Allows before GetCredential password left out of the box when _hostChange is set
Edit dialog, service principal secret prefill same same
ServerManager.CheckConnectionAsync through the resolver above HostChange(server) returns early with the unconfirmed sentence
ServerConnection.HasStoredCredentials / CredentialResolver.HasStoredCredentials ServerCredentialIds.Allows before CredentialExists none: it reads no secret, only whether an entry exists
Profile secrets (profile_{id} keys) in ResolveAuth, AddMultipleServersDialog, EditCredentialProfileDialog not applicable: keyed by profile id, not server Id resolver path as above; the two dialogs edit profiles and send nothing to a server
Save and delete (ServerManager.AddServer, UpdateServer, DeleteServer, dialog save) ServerCredentialIds.Allows host is confirmed on save (dev's code)

Tests after the merge (Lite.Tests, targeted): McpHostGateLiveTests, McpTokenIsolationTests, McpTokenGateTests, SettingsImportTests, McpInvestigationGateTests, McpWritePolicyTests, ServerHostBindingTests, LiteInvestigationGoldenTests, CredentialProfileTests, CredentialStore*, ServerManagerDetectionQueryTests, AlertSettingsCredentialLoadTests, NotificationRouteRealCredentialTests, AzureAuthConnectionStringTests, and every Census and Ratchet class: Total 235, Failed 0. Lite and Lite.Tests build with 0 warnings and 0 errors. Full suite not run; CI decides the rest.

CI fix

A token must now have at least 16 characters and at least 6 different characters before the write or investigation tools are served. These are the same two conditions as Darling's DescribeTokenProblem(...) is null, mirrored in McpTokenStore.IsUsableToken because Lite does not reference the Darling service project. A 16-character token of one repeated character is treated as Darling treats it (it still gates requests; writes and investigations are not served), and the startup line states both requirements. New host tests cover the repeated-character, five-distinct and six-distinct cases (they failed with the distinct-character check removed). The token store's read-failure text no longer uses a bare severity-word literal, which the severity census flagged. The two source-text order checks (TheHost_InstallsTheGuard_AfterTheHostGuard_AndBeforeMapMcp, McpHost_InstallsTheHostHeaderGuard_AsItsFirstMiddleware_BeforeMapMcp) and their csproj fixture link are replaced by two behavior tests on the running host: a foreign Host header is refused 400 before the token gate, and a non-JSON POST with the right token is refused 415 while the same POST with no token is refused 401. McpHostGateLiveTests now disposes its DuckDbInitializers. Ran: the touched Lite classes, McpHostGateLiveTests, McpTokenGateTests, McpTokenIsolationTests, and every Census and Ratchet class in both test projects, all passing.

…fig button in Settings

Lite's MCP server now needs 'Authorization: Bearer <token>' on every request, like Darling's. The token is made the first time the server starts with none and kept in Windows Credential Manager for the signed-in user. Settings, Access shows it with Copy Token and Copy MCP config buttons. mcp_allow_tokenless_loopback in settings.json restores serving without a token.
…reads-only with the opt-out, New token, clipboard formats

- The token is stored as PerformanceMonitorLite.Mcp_McpToken, a name no server Id can produce.
- A saved password is read, saved or deleted only for a server whose Id is a GUID; other servers stay in the list with no saved password and one log line.
- With mcp_allow_tokenless_loopback on, only read tools are served. Write tools need mcp_allow_writes and a token; investigation tools need their setting, a token and writes off; a token counts at 16 characters or more. One Critical line per group asked for and left off.
- McpHostService takes the token as a required argument; no token without the opt-out throws. The pipeline is one method the tests also run.
- Settings, Access: New token button replaces the token; the running host refuses the old one at once.
- Copy Token and Copy MCP config keep the text out of Clipboard History and the cloud clipboard.
- Import Settings drops mcp_allow_tokenless_loopback, mcp_allow_writes and mcp_allow_investigations from an imported settings.json.
- Tests run the real host on a free localhost port.
…es and skips unreadable ones, New token always reloads the host, copy from the token box uses the same clipboard path, the serving record never prints the token
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 22, 2026 20:59
@erikdarlingdata
erikdarlingdata enabled auto-merge (squash) October 22, 2026 20:59
…token

# Conflicts:
#	Lite/Services/ServerManager.cs
@claude

claude Bot commented Oct 22, 2026

Copy link
Copy Markdown

Two existing tests pin source text this diff deletes — the suite goes red

ExecuteAsync no longer contains the strings two Lite.Tests classes read out of the real McpHostService.cs (copied beside the test binary by Lite.Tests/Lite.Tests.csproj:95). After this change the file has app.MapMcp(); and UseJsonPostGuard(app); inside ConfigurePipeline, and no _app.Use( / _app.MapMcp(); / UseJsonPostGuard(_app); anywhere:

$ grep -n "_app\.\|UseJsonPostGuard\|MapMcp" Lite/Mcp/McpHostService.cs
365:        UseJsonPostGuard(app);
367:        app.MapMcp();
467:            await _app.RunAsync(stoppingToken);
495:            await _app.StopAsync(cancellationToken);
496:            await _app.DisposeAsync();
  1. Lite.Tests/HostHeaderGuardTests.cs:70 — ReadMcpHostSource() asserts Assert.Contains("_app.MapMcp();", source). That assertion now fails, which fails McpHost_InstallsTheHostHeaderGuard_AsItsFirstMiddleware_BeforeMapMcp (its only caller). Lines 80 and 83 would also miss: afterBuild.IndexOf("_app.Use(") and afterBuild.IndexOf("_app.MapMcp();") are both -1 now.
  2. Lite.Tests/McpJsonPostGuardTests.cs:127-128 — TheHost_InstallsTheGuard_AfterTheHostGuard_AndBeforeMapMcp takes IndexOf("UseJsonPostGuard(_app);") and IndexOf("_app.MapMcp();"), both -1, so Assert.True(jsonGuard > hostGuard) fails.

.github/ci-quarantine.txt has no entries, and the quarantine would not cover this PR anyway — it touches the file those tests read.

Both are spelling tests of the kind #5833 is deleting and .github/REVIEW-TRAPS.md rules out as coverage ("never answer a trap with a test that reads source text or asserts how code is spelled"), and the PR already deletes one of them from McpWritePolicyTests. The two remaining ones are the same shape and the pipeline order they pinned is now covered for real: the 401-before-MapMcp order by McpHostGateLiveTests, the 415 and the Host-header decision by the runtime tests in those same two files. Deleting these two methods (and ReadMcpHostSource, which nothing else calls) is consistent with the rest of the change; re-pointing them at the new spellings would just re-arm the trap.

The test plan lists McpWritePolicyTests, McpInvestigationGateTests, SettingsImportTests "and every Census and Ratchet class" — neither of these two classes is in that set, and the full suite was explicitly not run, so nothing in the plan would have caught it.

Comment thread Lite/Mcp/McpTokenStore.cs
/// <summary>Constant-time check of an <c>Authorization</c> header against the token: true only for <c>Bearer &lt;token&gt;</c>
/// (scheme in any case) with the right token. An empty expected token never authorizes. Compares SHA-256 digests, so it
/// leaks neither the token nor its length.</summary>
internal static bool IsAuthorized(string? authorizationHeader, string expectedToken)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, but it is the shape .github/REVIEW-TRAPS.md names: "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy."

IsAuthorized plus the "Bearer " parse below it plus MinimumTokenLength = 16 are a third copy of three functions Darling already has:

  • DarlingMcpHostService.IsBearerTokenAuthorized (line 2237) — same empty/empty-presented guards, same delegation to a constant-time compare;
  • DarlingMcpHostService.ExtractBearerToken (line 2257) — same Trim(), same StartsWith(prefix, OrdinalIgnoreCase), same Trim() of the remainder, same blank-is-null;
  • DarlingHostBinding.FixedTimeTokenEquals (line 363) — the identical SHA-256 + CryptographicOperations.FixedTimeEquals body;
  • DarlingHostBinding.MinimumTokenLength = 16 (line 245).

ConfigurePipeline's own doc comment two files over says the Host guard runs "the SAME shared decision Darling's web dashboard and MCP host install (HostHeaderGuard; never a second copy of the rule)" — HostHeaderGuard lives in PerformanceMonitor.Common, which both SKUs already reference, and this is the same kind of rule in the same pipeline.

The copy has already drifted on one point: Darling's floor is DescribeTokenProblem, which is 16 characters and MinimumDistinctTokenCharacters = 6 (DarlingHostBinding.cs:249). McpHostService.DecideServing checks only Length >= MinimumTokenLength, so a 16-character token of one repeated letter serves Lite's write and investigation tools and would be refused by Darling. Dormant today — Generate() is the only writer and makes 64 random hex characters — but it is the half of the rule the PR body says is "the same rule as Darling's".

Comment thread Lite/Mcp/McpTokenStore.cs
case CredentialReadOutcome.Found:
if (string.IsNullOrWhiteSpace(token))
{
failure = "the stored entry is empty; it is left as it is";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking. A Found entry whose password is blank is reported as a failure, so GetOrCreate returns null with failure set and MainWindow.StartMcpServerAsync refuses the start permanently (MainWindow.xaml.cs:803-808) until someone repairs Credential Manager by hand.

The class doc gives the reason for never replacing an unreadable entry — "a new token would silently break every client that holds the old one" — and that reason holds for Unreadable, where the stored value might be a token clients are using. It does not hold here: an empty entry is a token no client can be presenting, so regenerating costs nothing and the refusal buys nothing. Unreachable from Lite's own writers (Write is only ever called with Generate()'s output), so this is about what happens when the entry is damaged from outside, which is the only way to reach this arm at all.

{
var credentials = new CredentialService(new CredentialStore(_prefix, "Performance Monitor Lite test"));
credentials.DeleteCredential("McpToken");
credentials.DeleteCredential("Mcp_McpToken");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking test hygiene: Dispose cleans the server-password prefix (_prefix) but never _tokenPrefix. TheToken_LivesApartFromTheServerPasswordEntries_AndNewTokenReplacesIt creates a real token under _tokenPrefix and deletes it on its last line (TestCredentialsOf(_tokenPrefix).DeleteCredential("McpToken")), so any assertion that fails before then leaves a live token in the developer's Credential Manager under a per-run GUID name that nothing will ever look for again. Moving that delete into Dispose beside the other two would make the cleanup unconditional.

claude[bot]
claude Bot previously requested changes Oct 22, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Claims

  • VERIFIED — every request needs Authorization: Bearer <token>; no token, a wrong one or a malformed header is 401 with WWW-Authenticate: Bearer, before the JSON guard and MapMcp; the compare is constant-time — McpHostService.cs:173-193, ConfigurePipeline:352-367, McpTokenStore.cs:163-186 (SHA-256 + FixedTimeEquals); McpTokenGateTests.NoToken_IsRefused401_AndNeverReachesTheHandler, AWrongOrMalformedToken_IsRefused401_AndNeverReachesTheHandler
  • VERIFIED — one pipeline method that ExecuteAsync and the tests both run — :448 is its only production caller; McpHostGateLiveTests drives the real service over HTTP, not a copy
  • VERIFIED — token and allowTokenlessLoopback are required arguments, both ahead of the first optional one, and a missing token without the opt-out throws — :55-56, :149-154; McpHostGateLiveTests.TheHostWithNoToken_AndNoOptOut_DoesNotStart
  • VERIFIED — its own Credential Manager name, apart from the server-password names; 64 lowercase hex from 32 random bytes, made at the first start with none — "PerformanceMonitorLite.Mcp_" vs CredentialService.CredentialPrefix, MainWindow.xaml.cs:801-815; McpTokenIsolationTests.TheToken_LivesApartFromTheServerPasswordEntries_AndNewTokenReplacesIt, McpTokenGateTests.FirstStartWithMcpOn_AndNoToken_MakesOne_AndKeepsIt
  • VERIFIED — a saved password is read, saved or deleted only for a GUID Id, at every site the body lists — ServerConnection.cs:379, :731; ServerManager.cs:443, :525, :620; AddServerDialog.xaml.cs:148, :160, :177, :1010. A grep of every credential call in Lite/ leaves no unguarded server-Id site; the rest are ProfileCredentialId or fixed names (PseudonymKey, SMTP), none a GUID. CredentialResolver.cs:63,92,98 delegate to ServerConnection. McpTokenIsolationTests.AServerWhoseIdIsNotAGuid_GetsNoSavedPassword_WhateverIsStoredUnderThatName
  • VERIFIED — opt-out serves reads only; writes need the setting and a token; investigations need the setting, a token and writes off; one Critical line per group asked for and left off — DecideServing:156-163, StartupLines:371-408; McpHostGateLiveTests.WithTheOptOutOn_WritesAndInvestigationsAsked_..., AFifteenCharacterToken_StillGatesRequests_..., EveryToolGroupAskedForAndOff_GetsOneCriticalLine
  • VERIFIED — New token refuses the old one at once, and closing Settings always restarts from the stored token — ReplaceToken:325-334 re-read per request at :448; SettingsWindow.xaml.cs:751 + MainWindow.xaml.cs:1959-1963; McpHostGateLiveTests.NewToken_MakesTheOldTokenFailAtOnce_AndTheNewOneWork
  • VERIFIED — the three clipboard formats with two DWORD zeros, the token box's Copy on the same path, Import Settings dropping the three gate keys and skipping a non-object settings.json, the write-tool count in the log line, and mcp_allow_tokenless_loopback defaulting off and loud on a wrong kind — SecretClipboard.cs:317-325, SettingsWindow.xaml.cs:721-732, SettingsImport.cs:57-68,86-122, StartupLines:373, McpSettings.cs:194-202; the three ImportSettings_* tests and McpTokenGateTests.TheTokenIsRequiredByDefault / AQuotedOptOut_KeepsTheEndpointOff_AndNamesTheKey. The strip is as strict as the reader it claims to match: SettingsFileGuard.Read uses default JsonDocumentOptions, so Lite's settings.json is not JSONC
  • UNVERIFIED — "146 run, 0 failed", "131 tests, 0 failed", "Total 235, Failed 0", "0 warnings, 0 errors" — net10.0-windows with WPF and Credential Manager; none of it builds here, it needs a Windows dotnet test. The named sets exclude the two classes this diff breaks
  • UNVERIFIED — the Settings token panel, New token, Copy Token and Copy MCP config behave as described — the body says no UI session was run; only a WPF click-through shows it
  • UNVERIFIED — "the same change in Darling is #6507" — another pull request, not in this tree
  • REFUTED — DecideServing is "the same rule as Darling's DarlingMcpWritePolicy.ResolveForStart" — Darling gates on DescribeTokenProblem(...) is null (DarlingMcpWritePolicy.cs:290-291): 16 characters AND MinimumDistinctTokenCharacters = 6 (DarlingHostBinding.cs:249,273-277). DecideServing:157 checks only Length >= MinimumTokenLength, so a 16-character token of one repeated character serves Lite's writes and investigations and would be refused by Darling. Dormant while Generate() is the only writer; the body's own "16 characters or more" is accurate, the comparison is not

Riskiest lines

  1. Lite/Mcp/McpHostService.cs:147 — DecideServing alone decides whether a gate exists and which tool groups are served — covered by McpHostGateLiveTests.WithTheOptOutOn_WritesAndInvestigationsAsked_NoWriteOrInvestigationToolIsListed and TheHostWithNoToken_AndNoOptOut_DoesNotStart
  2. Lite/Mcp/McpTokenStore.cs:163 — IsAuthorized is the whole check; an off-by-one in the prefix strip or a leaky compare is the hole this closes — covered by McpTokenGateTests.AWrongOrMalformedToken_IsRefused401_AndNeverReachesTheHandler (six header shapes, including "Bearer ", Basic, a bare token and a 16-character prefix of the right one)
  3. Lite/Models/ServerConnection.cs:379 — the ServerCredentialIds.Allows arm decides whether a monitored server gets its saved password; a wrong predicate silently breaks every SQL-auth connection — covered by McpTokenIsolationTests.AServerWhoseIdIsNotAGuid_GetsNoSavedPassword_WhateverIsStoredUnderThatName

Traps near this diff

  • "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — this moves Lite TO Darling's behaviour (DarlingMcpHostService.cs:2074); and no counterpart is needed for the Lite-only halves — grep 'new CredentialStore(' finds no Darling project, and the Darling viewer's copy button carries no token (Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml.cs:611-614)
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies — McpTokenStore.cs:163, the "Bearer " parse at :170 and MinimumTokenLength at :64 re-implement IsBearerTokenAuthorized (:2237), ExtractBearerToken (:2257), FixedTimeTokenEquals (DarlingHostBinding.cs:363) and MinimumTokenLength (:245) rather than sharing them from PerformanceMonitor.Common, where the same method's own comment puts the Host-header rule ("never a second copy of the rule"). Already drifted on the distinct-character floor. Non-blocking alone; see the inline comment
  • "Ruling (#5833): tests are load-bearing. … never answer a trap with a test that reads source text or asserts how code is spelled" — applies — the diff rightly deletes one such test from McpWritePolicyTests but leaves two of the same shape pinned to spellings it just removed: HostHeaderGuardTests.cs:70 and McpJsonPostGuardTests.cs:127-128. See the top-level comment
  • "A test that encodes the lie … Ask what defect a test would fail ON" — does not apply — McpTokenGateTests counts handler invocations through a TestServer and McpHostGateLiveTests speaks HTTP to the real host
  • "A test that reads the clock twice measures the CI scheduler" — does not apply — nothing here times an interval; the live test only polls for the listener to accept

Divergence (blind read vs PR body)

  • The test plan names McpWritePolicyTests, McpInvestigationGateTests, SettingsImportTests, the three new token classes, "Settings window classes" and "every Census and Ratchet class", and says the full suite was not run. HostHeaderGuardTests and McpJsonPostGuardTests are in none of those sets, and both read the real McpHostService.cs as a fixture and assert on three strings this diff deletes. The account of what ran is accurate; the gap it leaves is where the regression is.
  • The body calls DecideServing the same rule as Darling's; Darling's floor is two conditions and Lite's is one (REFUTED above).
  • ClientCommand's doc comment calls it "the same shape as the Darling Viewer's button", but that button copies a command with no token. Comment text only; the body does not repeat it.

Reviewer checklist

  • Run Lite.Tests on Windows, HostHeaderGuardTests and McpJsonPostGuardTests included, and confirm the two source-text assertions are the only breakage
  • Click through Settings → Access: a real 64-character token in a Width="320" box, Copy Token, Copy MCP config, New token, and the disabled state with mcp_allow_tokenless_loopback on
  • With Win+V and a cloud-clipboard account, confirm a token copied through SecretClipboard stays out of Clipboard History and cloud sync — TrySetDataObject is called with copy: false, so Windows reads these formats off a data object the app still owns, which no test here can observe
  • Confirm New token's restart holds live: it sets McpSettingsChanged, and StartMcpServerAsync refuses to restart if IsTcpPortListeningAsync still sees the port held after StopMcpServerAsync's 5-second budget
  • Upgrade rehearsal with mcp_enabled: true already set: the first start makes the token and logs the Warning at MainWindow.xaml.cs:812, and an already-registered tokenless client gets 401 until Copy MCP config is re-run

…ing requires; fix the CI failures

A 16-character token made of one repeated character, or of fewer than 6 distinct
characters, is no longer used for the write or investigation tools (the same two
conditions Darling applies). The token store no longer spells a severity word as a
literal. The two source-text order checks are replaced by host tests that assert the
order by outcome (foreign Host refused 400 before the token gate; non-JSON POST with a
valid token refused 415 before MapMcp). The host test disposes its DuckDbInitializers.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

  • VERIFIED — "A 16-character token made of one repeated character, or of fewer than 6 distinct characters, is no longer used for the write or investigation tools (the same two conditions Darling applies)." — Lite/Mcp/McpTokenStore.cs:76-85 adds MinimumDistinctTokenCharacters = 6 and IsUsableToken, wired into McpHostService.DecideServing (Lite/Mcp/McpHostService.cs:158); matches Darling's pre-existing DarlingHostBinding.MinimumDistinctTokenCharacters / DescribeTokenProblem (Darling/PerformanceMonitor.Darling.Service/Hosting/DarlingHostBinding.cs:250-274). New tests ASixteenCharacterTokenWithFewerThanSixDistinctCharacters_StillGatesRequests_ButServesNoWriteOrInvestigationTool and ...WithSixDistinctCharacters_ServesTheWriteAndInvestigationTools_WhenAsked (Lite.Tests/McpHostGateLiveTests.cs) exercise the boundary via the real host.
  • VERIFIED — "The two source-text order checks are replaced by host tests that assert the order by outcome (foreign Host refused 400 before the token gate; non-JSON POST with a valid token refused 415 before MapMcp)." — Lite.Tests/HostHeaderGuardTests.cs and Lite.Tests/McpJsonPostGuardTests.cs each lose their File.ReadAllText(...Fixtures\McpHostService.cs...) test, the csproj's fixture link is removed, and Lite.Tests/McpHostGateLiveTests.cs gains ARequestWithAForeignHost_IsRefused400_BeforeTheTokenGateLooksAtIt and ANonJsonPost_WithTheRightToken_IsRefused415_AndWithoutOneIsRefused401, which drive the real ConfigurePipeline over HTTP and distinguish ordering by status code (confirmed no other file still reads the deleted Fixtures\McpHostService.cs link).
  • VERIFIED — "McpHostGateLiveTests now disposes its DuckDbInitializers." — _ducks list added and drained in Dispose() (Lite.Tests/McpHostGateLiveTests.cs); DuckDbInitializer is IDisposable (Lite/Database/DuckDbInitializer.cs:20).
  • UNVERIFIED — "The token store's read-failure text no longer uses a bare severity-word literal, which the severity census flagged." — the diff does change "unknown" to "no reason was given" in McpTokenStore.cs:138, but I could not locate a test named as a "severity census" in a read-only tree to confirm what it flags or why "unknown" qualified as a severity-word literal.

Riskiest lines

  1. Lite/Mcp/McpTokenStore.cs:76-80 (IsUsableToken) — the length/distinct-count boundary that now gates every write/investigation tool decision — covered by ASixteenCharacterTokenWithFewerThanSixDistinctCharacters_StillGatesRequests_ButServesNoWriteOrInvestigationTool and ASixteenCharacterTokenWithSixDistinctCharacters_ServesTheWriteAndInvestigationTools_WhenAsked.
  2. Lite/Mcp/McpHostService.cs:158 (var usable = McpTokenStore.IsUsableToken(token); in DecideServing) — the one call site that must not regress to the old length-only check — covered by the same two tests above (driven through StartAsync/ListedAsync).
  3. Lite/Mcp/McpHostService.cs:388 (the AskedWrites Critical startup line now using McpTokenStore.TokenRequirement) — wrong wording here would mislabel why writes stayed off — covered by ARepetitiveToken_GetsTheCriticalLine_ThatNamesTheDistinctCharacterRequirement.

Traps near this diff

  • "Ruling (#5833): tests are load-bearing. ... never answer a trap with a test that reads source text or asserts how code is spelled ... A trap is answered with a behaviour test that fails on the defect and passes on the fix" — applies here, and the diff complies: it deletes the two source-text-reading tests (HostHeaderGuardTests.cs, McpJsonPostGuardTests.cs) and their csproj fixture link, replacing them with the two outcome-based live tests described above.
  • "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding" — does not apply as a violation: Darling already carries MinimumDistinctTokenCharacters = 6 / DescribeTokenProblem (Darling/PerformanceMonitor.Darling.Service/Hosting/DarlingHostBinding.cs:250-274, untouched by this commit); this diff brings Lite into parity rather than creating a gap.

Divergence (blind read vs PR body)

None — the body's account matches the diff on every claim graded above (the "severity census" wording is vague but the underlying code change it describes is present and consistent).

Reviewer checklist

  • Run the full Lite.Tests suite (build + McpHostGateLiveTests, McpTokenGateTests, McpWritePolicyTests, McpInvestigationGateTests, and the Census/Ratchet classes) to confirm the PR body's "all passing" claim, which a read-only tree cannot execute.

@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 23, 2026 02:19

Stale: this review is on baf0f11. The bot's review of the current head d8872fd is LGTM.

@erikdarlingdata
erikdarlingdata merged commit 22eba38 into dev Oct 23, 2026
35 of 37 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/lite-mcp-loopback-token branch October 23, 2026 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant