Skip to content

Commit 9ce209b

Browse files
committed
fix(mcp): stop server-access save mutating the live in-memory ACL profile
BuildAllowedServerList wrote McpServersMode=Allowlist and AllowedMcpServers back onto the live ToolAudienceProfile object returned by ResolveProfile — the same object IsServerAllowed/IsToolGranted/GetEffectiveMode read for access decisions. A save interrupted by an exception after that mutation but before the file write would leave the in-memory ACL coerced to allowlist mode. Fold the helper into SaveServerAccess, which now accumulates each audience's allow-list in a local working dictionary (seeded once from the original profile so multiple changes to the same audience still build on each other) and writes the mode and list straight to the serialization dictionary. The live profile is never mutated.
1 parent a371c5b commit 9ce209b

3 files changed

Lines changed: 58 additions & 24 deletions

File tree

openspec/changes/harden-config-tui-io-and-failloud/tasks.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ Cited file:line numbers are from the review doc and may drift as fixes land. -->
3636
- [x] 3.3 `SlackStepViewModel.BuildChannelAudiences` (review:299) — do not write an unresolved channel name as an ACL key; omit/flag it (or block) so it grants nothing. Test: unresolved channel → no inert name key in `ChannelAudiences`. — `ResolveChannelAudienceKey` (which returned the channel NAME when `LastChannelResolution` was null or the name was unresolved) is now `TryResolveChannelAudienceKey`: it yields a key only for the DM row (`"dm"`) or a resolved canonical channel ID, and `BuildChannelAudiences` omits any entry it can't resolve — so a dead, name-keyed ACL entry the Slack runtime can never match is never written. The health-check phase already warns about unresolved channels. New `ContributeConfig_OmitsUnresolvedChannelNameFromAudiences_KeepsResolvedById`.
3737
- [x] 3.4 `ChannelsConfigViewModel.ApplyAddChannelAsync` (review:582) — replace `Single()` with a safe predicate so a resolved id of `"dm"` with DMs enabled does not throw. Test: add a `"dm"`-resolving channel → no exception. — The row-focus lookup that positioned `_channelRowIndex` used `Single(row.Id == channelId)`; a resolved id of exactly `"dm"` with DMs enabled collided with the DM row (also `Id="dm"`) → `InvalidOperationException` crashing the add flow. Now `FirstOrDefault` over `!IsDirectMessage && !IsAction && Id==channelId` (matches only real channel rows), guarded against not-found. New `Add_channel_resolving_to_dm_with_dms_enabled_does_not_throw`.
3838
- [x] 3.5 `TelemetryAlertingConfigViewModel` (review:289) — add an explicit gesture to clear a webhook auth header (blank-preserve still keeps it). Test: clear gesture removes the header; blank keeps it. — `SaveWebhookForm` now treats a single `-` in the auth field as an explicit clear (`target.Headers = null`); blank still preserves the stored header. The edit-form placeholder now reads `(stored header preserved — enter - to clear)` so the gesture is discoverable. New `Editing_a_webhook_clears_the_auth_header_with_the_dash_gesture`; the existing blank-preserve test still passes.
39-
- [ ] 3.6 `McpToolPermissionsViewModel.BuildAllowedServerList` (review:533) — operate on a copy, not the live in-memory profile object. Test: building the list does not mutate the source profile.
39+
- [x] 3.6 `McpToolPermissionsViewModel.BuildAllowedServerList` (review:533) — operate on a copy, not the live in-memory profile object. Test: building the list does not mutate the source profile. — `BuildAllowedServerList` mutated `profile.McpServersMode`/`AllowedMcpServers` on the live `Profiles.Public/Team/Personal` objects that back runtime ACL queries; folded it into `SaveServerAccess`, which now accumulates each audience's allow-list in a local working dict (seeded once from the original profile so multi-change-per-audience still accumulates correctly) and writes mode/list straight to the serialization dict — the in-memory ACL profile is never touched. New `Save_DoesNotMutateTheLiveInMemoryProfile` (live profile stays `All` after a save that converts the persisted config to `Allowlist`); existing All→Allowlist + allowlist-preserve tests still pass.
4040

4141
## 4. Verification & close
4242

src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,41 @@ public void Save_DisablingServerFromAllProfileConvertsToAllowlist()
327327
Assert.False(reloaded.IsServerAllowedForSelectedAudience());
328328
}
329329

330+
[Fact]
331+
public void Save_DoesNotMutateTheLiveInMemoryProfile()
332+
{
333+
File.WriteAllText(_paths.NetclawConfigPath,
334+
"""
335+
{
336+
"configVersion": 1,
337+
"McpServers": { "github": { "Transport": "stdio" } },
338+
"Tools": {
339+
"AudienceProfiles": {
340+
"Personal": { "McpServersMode": "All" }
341+
}
342+
}
343+
}
344+
""");
345+
346+
var vm = CreateVm();
347+
vm.Servers.Add(("notion", "running", 1));
348+
vm.InitializeForTests(new McpServerName("notion"), new[] { "create-pages" });
349+
vm.SetSelectedAudienceForTests(TrustAudience.Personal);
350+
Assert.Equal(ToolProfileMode.All, vm.Profiles.Personal.McpServersMode);
351+
352+
vm.ToggleServerAccess(); // disable notion -> pending All->Allowlist conversion
353+
Assert.True(vm.Save());
354+
355+
// The save writes the Allowlist conversion to disk, but must NOT coerce the live in-memory
356+
// profile that backs runtime ACL queries (IsServerAllowed, etc.). The prior code mutated it
357+
// mid-save, so a mid-save exception would leave the ACL in a post-save allowlist state.
358+
Assert.Equal(ToolProfileMode.All, vm.Profiles.Personal.McpServersMode);
359+
Assert.Empty(vm.Profiles.Personal.AllowedMcpServers);
360+
361+
using var doc = JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath));
362+
Assert.Equal("Allowlist", GetAudienceProfile(doc, "Personal").GetProperty("McpServersMode").GetString());
363+
}
364+
330365
private static void CycleServerDefault(McpToolPermissionsViewModel vm, bool reverse)
331366
{
332367
if (reverse)

src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -509,37 +509,36 @@ public bool Save()
509509
private void SaveServerAccess(Dictionary<string, object> config, Dictionary<string, object> profilesSection)
510510
{
511511
var knownServers = GetKnownMcpServers(config);
512+
513+
// Accumulate per-audience working lists WITHOUT mutating the live in-memory profile objects
514+
// (Profiles.Public/Team/Personal back the runtime ACL queries — IsServerAllowed, etc. — so
515+
// coercing them here would leave the ACL in a post-save state if Save throws before the file
516+
// write). Seed each audience's working list from its ORIGINAL profile the first time it is
517+
// touched; later changes for the same audience build on the working list rather than re-reading
518+
// a profile that an earlier iteration would have coerced.
519+
var workingLists = new Dictionary<string, List<string>>(StringComparer.Ordinal);
512520
foreach (var ((audienceName, serverName), allowed) in _pendingServerAccess)
513521
{
514522
var audienceSection = ConfigFileHelper.GetOrCreateSection(profilesSection, audienceName);
515-
var profile = ResolveProfile(AudienceFromName(audienceName));
516-
var serverList = BuildAllowedServerList(profile, knownServers, serverName, allowed);
523+
if (!workingLists.TryGetValue(audienceName, out var serverList))
524+
{
525+
var profile = ResolveProfile(AudienceFromName(audienceName));
526+
serverList = profile.McpServersMode == ToolProfileMode.All
527+
? knownServers.ToList()
528+
: profile.AllowedMcpServers.ToList();
529+
workingLists[audienceName] = serverList;
530+
}
517531

518-
audienceSection["McpServersMode"] = profile.McpServersMode.ToString();
532+
if (allowed)
533+
AddServer(serverList, serverName);
534+
else
535+
serverList.RemoveAll(s => s.Equals(serverName, StringComparison.OrdinalIgnoreCase));
536+
537+
audienceSection["McpServersMode"] = ToolProfileMode.Allowlist.ToString();
519538
audienceSection["AllowedMcpServers"] = serverList;
520539
}
521540
}
522541

523-
private List<string> BuildAllowedServerList(
524-
ToolAudienceProfile profile,
525-
IReadOnlyList<string> knownServers,
526-
string serverName,
527-
bool allowed)
528-
{
529-
var serverList = profile.McpServersMode == ToolProfileMode.All
530-
? knownServers.ToList()
531-
: profile.AllowedMcpServers.ToList();
532-
533-
profile.McpServersMode = ToolProfileMode.Allowlist;
534-
if (allowed)
535-
AddServer(serverList, serverName);
536-
else
537-
serverList.RemoveAll(s => s.Equals(serverName, StringComparison.OrdinalIgnoreCase));
538-
539-
profile.AllowedMcpServers = serverList;
540-
return serverList;
541-
}
542-
543542
private IReadOnlyList<string> GetKnownMcpServers(Dictionary<string, object> config)
544543
{
545544
var names = new List<string>();

0 commit comments

Comments
 (0)