From 29b6e4de23b1ab0d5863ff465642455903499f20 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 18 Jul 2026 21:50:43 +0000 Subject: [PATCH 1/3] fix(mcp): logged, single-flight, proactive OAuth refresh for MCP servers --- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../references/diagnostics.md | 1 + .../Doctor/McpServersDoctorCheckTests.cs | 34 ++ .../Doctor/McpServersDoctorCheck.cs | 8 +- .../Mcp/McpClientManagerStatusTests.cs | 21 ++ .../McpEndpointRouteBuilderExtensionsTests.cs | 4 + .../Mcp/McpOAuthServiceTests.cs | 340 +++++++++++++++++- .../Mcp/McpReconnectionServiceTests.cs | 35 ++ src/Netclaw.Daemon/Mcp/IMcpReconnectable.cs | 8 + src/Netclaw.Daemon/Mcp/McpClientManager.cs | 167 ++++++++- src/Netclaw.Daemon/Mcp/McpOAuthService.cs | 170 +++++++-- .../Mcp/McpReconnectionService.cs | 22 ++ 12 files changed, 775 insertions(+), 37 deletions(-) diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index e4143a137..b5f20ce83 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.35.0" + version: "2.36.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md b/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md index 4439d4c74..aca9570bf 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md +++ b/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md @@ -74,6 +74,7 @@ debugging a daemon-wide problem → read `daemon.log`. |---------|-------| | No LLM responses | `netclaw doctor`; verify provider credentials | | Missing tools | `netclaw mcp list`; check MCP connection state | +| MCP server `auth failed` in `netclaw doctor` / `netclaw mcp status` | Fixed by `netclaw mcp auth ` either way, but the message tells you which case: "refresh rejected (invalid_grant)" means Netclaw's proactive refresh got a terminal rejection from the provider — the stored tokens were already cleared and the connection torn down (a revoked/rotated grant, not a config error). A generic "authentication rejected by server" means static credentials (token/headers) were rejected. `awaiting auth` is different from both — that server has never completed OAuth at all. A server reported **connected** with a "No refresh token" note is healthy now but cannot silently refresh; re-run `netclaw mcp auth ` before its current token expires. | | Memory recall degraded | `netclaw status` memory section | | Daemon won't start | crash logs at `/logs/crash-*.log` (`NETCLAW_HOME` defaults to `~/.netclaw`) | | Docker daemon cannot create `/home/netclaw/.netclaw/*` | Official image entrypoint repairs writable bind mounts to UID/GID `1654:1654`; if bypassed or read-only, run `sudo chown -R 1654:1654 ` or use a Docker named volume | diff --git a/src/Netclaw.Cli.Tests/Doctor/McpServersDoctorCheckTests.cs b/src/Netclaw.Cli.Tests/Doctor/McpServersDoctorCheckTests.cs index 378053525..277cebb8a 100644 --- a/src/Netclaw.Cli.Tests/Doctor/McpServersDoctorCheckTests.cs +++ b/src/Netclaw.Cli.Tests/Doctor/McpServersDoctorCheckTests.cs @@ -231,6 +231,40 @@ public async Task DaemonReportedAwaitingAuth_ReturnsWarning() Assert.Contains("awaiting auth", result.Message); } + [Fact] + public async Task DaemonReportedConnectedWithNoRefreshTokenAdvisory_ReturnsPassButSurfacesAdvisory() + { + WriteConfig(new + { + configVersion = 1, + McpServers = new + { + notion = new + { + Transport = "http", + Url = "https://mcp.example.com", + Enabled = true, + } + } + }); + + var check = new McpServersDoctorCheck(_paths, CreateDaemonApi(_ => FakeHttpMessageHandler.JsonResponse(new + { + notion = new + { + state = "Connected", + toolCount = 5, + error = "No refresh token — re-authorization will be required at expiry. Run: netclaw mcp auth notion" + } + }))); + + var result = await check.RunAsync(TestContext.Current.CancellationToken); + + Assert.Equal(DoctorSeverity.Pass, result.Severity); + Assert.Contains("connected (5 tools)", result.Message); + Assert.Contains("No refresh token", result.Message); + } + [Fact] public async Task OfflineOAuthProbe_DoesNotClaimAuthFailure() { diff --git a/src/Netclaw.Cli/Doctor/McpServersDoctorCheck.cs b/src/Netclaw.Cli/Doctor/McpServersDoctorCheck.cs index 708d2b09a..41d8a18b8 100644 --- a/src/Netclaw.Cli/Doctor/McpServersDoctorCheck.cs +++ b/src/Netclaw.Cli/Doctor/McpServersDoctorCheck.cs @@ -263,7 +263,13 @@ private static DoctorCheckResult EvaluateDaemonStatuses( switch (state) { case "Connected": - statusMessages.Add($"{name}: connected ({toolCount} tools)"); + // `error` doubles as an advisory slot for otherwise-healthy + // servers — e.g. an OAuth token with no refresh token, which + // will hard-fail at expiry with no chance of silent + // recovery. See McpClientManager.BuildOAuthAdvisory. + statusMessages.Add(string.IsNullOrEmpty(error) + ? $"{name}: connected ({toolCount} tools)" + : $"{name}: connected ({toolCount} tools) — {error}"); break; case "AwaitingAuth": hasAwaitingAuth = true; diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs index f3e99850a..6db93a5c3 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs @@ -74,4 +74,25 @@ public void BuildConnectionFailureStatus_ForNetworkFailure_ReturnsUnreachable() Assert.Equal(McpConnectionState.Unreachable, status.State); Assert.Contains("Connection refused", status.ErrorMessage); } + + [Fact] + public void CreateRefreshRejectedStatus_ReturnsAuthFailedWithDistinctInvalidGrantMessage() + { + var status = McpClientManager.CreateRefreshRejectedStatus(new McpServerName("notion")); + + // AuthFailed (not Unreachable) so McpReconnectionService's backoff loop + // — which only retries Unreachable servers — never auto-retries a dead + // grant into a loop. + Assert.Equal(McpConnectionState.AuthFailed, status.State); + Assert.Contains("invalid_grant", status.ErrorMessage); + Assert.Contains("netclaw mcp auth notion", status.ErrorMessage); + + // Distinct wording from the generic AuthFailed message so operators can + // tell "grant revoked by provider" apart from "never authenticated". + var genericAuthFailed = McpClientManager.CreateAuthFailedStatus( + new McpServerName("notion"), + new HttpRequestException(httpRequestError: HttpRequestError.Unknown, "Unauthorized", null, System.Net.HttpStatusCode.Unauthorized), + oauthManaged: true); + Assert.NotEqual(genericAuthFailed.ErrorMessage, status.ErrorMessage); + } } diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs index 27ce28cb0..2b2e6062c 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs @@ -406,6 +406,8 @@ public IReadOnlyDictionary GetServerStatuses() = public Task TryReconnectAsync(McpServerName serverName, CancellationToken ct = default) => Task.FromResult(false); + + public Task RefreshOAuthTokensAsync(CancellationToken ct = default) => Task.CompletedTask; } private sealed class TrackingReconnectable : IMcpReconnectable @@ -429,5 +431,7 @@ public Task TryReconnectAsync(McpServerName serverName, CancellationToken _tcs.TrySetResult(); return Task.FromResult(true); } + + public Task RefreshOAuthTokensAsync(CancellationToken ct = default) => Task.CompletedTask; } } diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs index a5685d07a..628d550e7 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs @@ -6,7 +6,9 @@ using System.Net; using System.Text; using System.Text.Json; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Time.Testing; using Netclaw.Configuration; using Netclaw.Configuration.Secrets; using Netclaw.Daemon.Mcp; @@ -21,6 +23,219 @@ public sealed class McpOAuthServiceTests : IDisposable { private readonly DisposableTempDir _dir = new(); + // ── Refresh: single-flight, invalid_grant, transient, proactive, missing-refresh-token ── + + [Fact] + public async Task GetValidTokenAsync_InvalidGrant_ClearsTokensAndDoesNotRetry() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var sink = new RecordingNotificationSink(); + var handler = new TokenEndpointHandler + { + RefreshResponse = _ => FakeHttpMessageHandler.JsonResponse(new { error = "invalid_grant" }, HttpStatusCode.BadRequest), + }; + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var service = CreateService(CreateDiscoveryClient(), new OAuthPkceService(new HttpClient(handler), time), + timeProvider: time, notificationSink: sink); + + await SeedTokenAsync(service, serverName, entry, handler, expiresInSeconds: 60, refreshToken: "refresh-old"); + time.Advance(TimeSpan.FromMinutes(5)); // well past expiry + + var token = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + + Assert.Null(token); + Assert.Null(service.GetTokenSet(serverName)); + Assert.Equal(1, handler.RefreshCallCount); + + var alert = Assert.Single(sink.Alerts); + Assert.Equal(AlertType.McpAuthExpired, alert.Category); + Assert.Equal("invalid_grant", alert.Context!["reason"]); + + // Subsequent calls must not re-attempt refresh against cleared tokens. + var second = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + Assert.Null(second); + Assert.Equal(1, handler.RefreshCallCount); + } + + [Fact] + public async Task GetValidTokenAsync_TransientFailure_RetainsTokensAndLogsStatus() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var handler = new TokenEndpointHandler + { + RefreshResponse = _ => new HttpResponseMessage(HttpStatusCode.ServiceUnavailable), + }; + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var logger = new RecordingLogger(); + var service = CreateService(CreateDiscoveryClient(), new OAuthPkceService(new HttpClient(handler), time), + timeProvider: time, logger: logger); + + await SeedTokenAsync(service, serverName, entry, handler, expiresInSeconds: 60, refreshToken: "refresh-old"); + time.Advance(TimeSpan.FromMinutes(5)); + + var token = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + + Assert.Null(token); + Assert.Equal(1, handler.RefreshCallCount); + + // Tokens retained — server stays retryable, not clobbered by a transient 503. + var retained = service.GetTokenSet(serverName); + Assert.NotNull(retained); + Assert.Equal("refresh-old", retained!.RefreshToken!.Value); + + Assert.Contains(logger.Entries, e => e.Level == LogLevel.Warning + && e.Message.Contains("notion", StringComparison.Ordinal) + && e.Message.Contains("ServiceUnavailable", StringComparison.Ordinal)); + } + + [Fact] + public async Task GetValidTokenAsync_ConcurrentCallsWithExpiredToken_SingleFlightsRefresh() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var handler = new GatedTokenEndpointHandler(); + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var service = CreateService(CreateDiscoveryClient(), new OAuthPkceService(new HttpClient(handler), time), + timeProvider: time); + + await SeedTokenAsync(service, serverName, entry, handler, expiresInSeconds: 60, refreshToken: "refresh-old"); + time.Advance(TimeSpan.FromMinutes(5)); // well past expiry + + handler.RefreshResponseBody = new { access_token = "access-new", refresh_token = "refresh-new", expires_in = 3600 }; + + var callTasks = new Task[5]; + for (var i = 0; i < callTasks.Length; i++) + callTasks[i] = service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + + await handler.WaitForFirstRefreshRequestAsync().WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + handler.ReleaseFirstRefreshRequest(); + + var results = await Task.WhenAll(callTasks); + + Assert.Equal(1, handler.RefreshCallCount); + Assert.All(results, r => Assert.Equal("access-new", r)); + + var persisted = service.GetTokenSet(serverName); + Assert.NotNull(persisted); + Assert.Equal("refresh-new", persisted!.RefreshToken!.Value); + } + + [Fact] + public async Task GetValidTokenAsync_WithinProactiveWindow_RefreshesAheadOfExpiry() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var handler = new TokenEndpointHandler + { + RefreshResponse = _ => FakeHttpMessageHandler.JsonResponse(new + { + access_token = "access-new", + refresh_token = "refresh-new", + expires_in = 3600, + }), + }; + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var service = CreateService(CreateDiscoveryClient(), new OAuthPkceService(new HttpClient(handler), time), + timeProvider: time); + + // Token is not yet expired, but its 15-minute remaining lifetime is + // inside the 10-minute ProactiveRefreshWindow. + await SeedTokenAsync(service, serverName, entry, handler, expiresInSeconds: (int)TimeSpan.FromMinutes(15).TotalSeconds, refreshToken: "refresh-old"); + time.Advance(TimeSpan.FromMinutes(6)); // 9 minutes remain — inside the window + + var token = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + + Assert.Equal("access-new", token); + Assert.Equal(1, handler.RefreshCallCount); + } + + [Fact] + public async Task GetValidTokenAsync_WellOutsideProactiveWindow_DoesNotRefresh() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var handler = new TokenEndpointHandler + { + RefreshResponse = _ => throw new InvalidOperationException("refresh should not be called"), + }; + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var service = CreateService(CreateDiscoveryClient(), new OAuthPkceService(new HttpClient(handler), time), + timeProvider: time); + + await SeedTokenAsync(service, serverName, entry, handler, expiresInSeconds: (int)TimeSpan.FromMinutes(30).TotalSeconds, refreshToken: "refresh-old"); + // 30 minutes remain — well outside the 10-minute proactive window. + + var token = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + + Assert.Equal("access-seed", token); + Assert.Equal(0, handler.RefreshCallCount); + } + + [Fact] + public async Task GetValidTokenAsync_MissingRefreshToken_WarnsAndDoesNotAttemptRefresh() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var sink = new RecordingNotificationSink(); + var handler = new TokenEndpointHandler + { + RefreshResponse = _ => throw new InvalidOperationException("refresh should not be called — no refresh token"), + }; + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var service = CreateService(CreateDiscoveryClient(), new OAuthPkceService(new HttpClient(handler), time), + timeProvider: time, notificationSink: sink); + + // Seed a token set with no refresh token at all (matches defect #4: + // a configured OAuth server whose persisted token set never got one). + await SeedTokenAsync(service, serverName, entry, handler, expiresInSeconds: 60, refreshToken: null); + time.Advance(TimeSpan.FromMinutes(5)); // past expiry + + var token = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + + Assert.Null(token); + Assert.Equal(0, handler.RefreshCallCount); + + var alert = Assert.Single(sink.Alerts); + Assert.Equal(AlertType.McpAuthExpired, alert.Category); + Assert.Equal("no_refresh_token", alert.Context!["reason"]); + } + + [Fact] + public async Task WarnIfMissingRefreshToken_TokenSetWithoutRefreshToken_WarnsImmediatelyRegardlessOfExpiryWindow() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var sink = new RecordingNotificationSink(); + var service = CreateService(CreateDiscoveryClient(), + CreatePkceService(JsonResponse(new { access_token = "access-tok", expires_in = 3600 })), + timeProvider: time, notificationSink: sink); + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + + // Far from expiry (1 hour, well outside the 10-minute window) but no + // refresh token was ever issued — the operator should be warned now, + // not left to discover it only once the token has already expired. + var (_, state) = await service.StartAuthorizationFlowAsync(serverName, entry, TestContext.Current.CancellationToken); + await service.CompleteAuthorizationAsync("code", state, TestContext.Current.CancellationToken); + + service.WarnIfMissingRefreshToken(serverName); + + var alert = Assert.Single(sink.Alerts); + Assert.Equal("no_refresh_token", alert.Context!["reason"]); + + // Calling again should not re-emit — de-duped until the condition changes. + service.WarnIfMissingRefreshToken(serverName); + Assert.Single(sink.Alerts); + } + [Fact] public async Task GetFlowStatusByState_ReauthWithExistingToken_RemainsPending() { @@ -113,18 +328,135 @@ public void Dispose() } private McpOAuthService CreateService(HttpClient discoveryClient, OAuthPkceService pkceService, - ISecretsProtector? protector = null) + ISecretsProtector? protector = null, + TimeProvider? timeProvider = null, + IOperationalNotificationSink? notificationSink = null, + ILogger? logger = null) { return new McpOAuthService( discoveryClient, new NetclawPaths(_dir.Path), - TimeProvider.System, - NullLogger.Instance, + timeProvider ?? TimeProvider.System, + logger ?? NullLogger.Instance, pkceService, - NullNotificationSink.Instance, + notificationSink ?? NullNotificationSink.Instance, protector); } + /// + /// Completes an OAuth authorization flow through 's + /// exchange route so the service ends up with a real, persisted token set — + /// the only way to populate McpOAuthService's private token/metadata + /// caches from outside the class. + /// + private static async Task SeedTokenAsync( + McpOAuthService service, + McpServerName serverName, + McpServerEntry entry, + ITokenEndpointHandler handler, + int expiresInSeconds, + string? refreshToken) + { + handler.ExchangeResponse = _ => refreshToken is null + ? FakeHttpMessageHandler.JsonResponse(new { access_token = "access-seed", expires_in = expiresInSeconds }) + : FakeHttpMessageHandler.JsonResponse(new { access_token = "access-seed", refresh_token = refreshToken, expires_in = expiresInSeconds }); + + var (_, state) = await service.StartAuthorizationFlowAsync(serverName, entry, CancellationToken.None); + await service.CompleteAuthorizationAsync("seed-code", state, CancellationToken.None); + } + + private interface ITokenEndpointHandler + { + Func? ExchangeResponse { get; set; } + } + + /// + /// Fake token endpoint that routes on grant_type so the same handler + /// can serve both the authorization-code exchange (to seed a token set) and + /// a separately-scriptable refresh response, mirroring a real OAuth server + /// backing a single token endpoint URL. + /// + private sealed class TokenEndpointHandler : HttpMessageHandler, ITokenEndpointHandler + { + private int _refreshCallCount; + + public Func? ExchangeResponse { get; set; } + public Func? RefreshResponse { get; set; } + + public int RefreshCallCount => Volatile.Read(ref _refreshCallCount); + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + var body = request.Content is null ? "" : await request.Content.ReadAsStringAsync(cancellationToken); + if (!body.Contains("grant_type=refresh_token", StringComparison.Ordinal)) + return ExchangeResponse!(request); + + Interlocked.Increment(ref _refreshCallCount); + return RefreshResponse!(request); + } + } + + /// + /// Like , but the first refresh request + /// blocks until the test releases it — used to prove single-flight + /// behavior by forcing genuine overlap between concurrent + /// GetValidTokenAsync callers instead of relying on incidental + /// scheduling. + /// + private sealed class GatedTokenEndpointHandler : HttpMessageHandler, ITokenEndpointHandler + { + private readonly TaskCompletionSource _firstRefreshReceived = + new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _releaseFirstRefresh = + new(TaskCreationOptions.RunContinuationsAsynchronously); + private int _refreshCallCount; + + public Func? ExchangeResponse { get; set; } + public object RefreshResponseBody { get; set; } = new { access_token = "access-new", expires_in = 3600 }; + + public int RefreshCallCount => Volatile.Read(ref _refreshCallCount); + + public Task WaitForFirstRefreshRequestAsync() => _firstRefreshReceived.Task; + public void ReleaseFirstRefreshRequest() => _releaseFirstRefresh.TrySetResult(); + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + var body = request.Content is null ? "" : await request.Content.ReadAsStringAsync(cancellationToken); + if (!body.Contains("grant_type=refresh_token", StringComparison.Ordinal)) + return ExchangeResponse!(request); + + if (Interlocked.Increment(ref _refreshCallCount) == 1) + { + _firstRefreshReceived.TrySetResult(); + await _releaseFirstRefresh.Task.WaitAsync(cancellationToken); + } + + return FakeHttpMessageHandler.JsonResponse(RefreshResponseBody); + } + } + + private sealed class RecordingNotificationSink : IOperationalNotificationSink + { + public List Alerts { get; } = []; + public void Emit(OperationalAlert alert) => Alerts.Add(alert); + } + + private sealed class RecordingLogger : ILogger + { + public List<(LogLevel Level, string Message)> Entries { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, EventId eventId, TState state, + Exception? exception, Func formatter) + => Entries.Add((logLevel, formatter(state, exception))); + } + private static McpServerEntry CreateHttpEntry() { return new McpServerEntry diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpReconnectionServiceTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpReconnectionServiceTests.cs index 34133206c..7d01c1afb 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpReconnectionServiceTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpReconnectionServiceTests.cs @@ -164,6 +164,31 @@ public async Task CleansUpBackoffWhenServerRecoveredExternally() Assert.Equal(countBefore + 1, _reconnectable.ReconnectCallCount); } + [Fact] + public async Task RefreshOAuthTokensAsync_DelegatesToReconnectable() + { + _reconnectable.OnRefreshOAuthTokens = _ => Task.CompletedTask; + + var service = CreateService(); + await service.RefreshOAuthTokensAsync(CancellationToken.None); + + Assert.Equal(1, _reconnectable.RefreshOAuthTokensCallCount); + } + + [Fact] + public async Task RefreshOAuthTokensAsync_SwallowsExceptionsWithoutCrashing() + { + _reconnectable.OnRefreshOAuthTokens = _ => throw new InvalidOperationException("boom"); + + var service = CreateService(); + + // Should not throw — a failing proactive-refresh sweep must not kill + // the background service loop. + await service.RefreshOAuthTokensAsync(CancellationToken.None); + + Assert.Equal(1, _reconnectable.RefreshOAuthTokensCallCount); + } + [Theory] [InlineData(0, 0)] [InlineData(1, 30_000)] @@ -181,10 +206,13 @@ private sealed class FakeMcpReconnectable : IMcpReconnectable { private readonly Dictionary _statuses = new(); private int _reconnectCallCount; + private int _refreshOAuthTokensCallCount; public int ReconnectCallCount => Volatile.Read(ref _reconnectCallCount); + public int RefreshOAuthTokensCallCount => Volatile.Read(ref _refreshOAuthTokensCallCount); public Func>? OnReconnect { get; set; } + public Func? OnRefreshOAuthTokens { get; set; } public void SetStatus(string name, McpConnectionState state, int toolCount = 0) { @@ -203,6 +231,13 @@ public async Task TryReconnectAsync(McpServerName serverName, Cancellation return await OnReconnect(serverName, ct); return false; } + + public async Task RefreshOAuthTokensAsync(CancellationToken ct = default) + { + Interlocked.Increment(ref _refreshOAuthTokensCallCount); + if (OnRefreshOAuthTokens is not null) + await OnRefreshOAuthTokens(ct); + } } private sealed class FakeNotificationSink : IOperationalNotificationSink diff --git a/src/Netclaw.Daemon/Mcp/IMcpReconnectable.cs b/src/Netclaw.Daemon/Mcp/IMcpReconnectable.cs index cd5cf7033..08442407e 100644 --- a/src/Netclaw.Daemon/Mcp/IMcpReconnectable.cs +++ b/src/Netclaw.Daemon/Mcp/IMcpReconnectable.cs @@ -12,4 +12,12 @@ internal interface IMcpReconnectable IReadOnlyDictionary GetServerStatuses(); Task TryReconnectAsync(McpServerName serverName, CancellationToken ct = default); + + /// + /// Proactively refreshes OAuth tokens for connected, OAuth-managed MCP + /// servers ahead of expiry, and surfaces advance warnings for token sets + /// that cannot self-heal (no refresh token, no known expiry). Called once + /// per tick. + /// + Task RefreshOAuthTokensAsync(CancellationToken ct = default); } diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index 7f0e3c08e..66492936a 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -33,6 +33,14 @@ internal sealed class McpClientManager : IHostedService, IDisposable, IMcpToolIn private readonly ConcurrentDictionary _statuses = new(); + // Serializes reconnect/teardown per server. Concurrent tool-invocation + // failures against the same server (see InvokeSharedAsync) can each + // independently reach TryReconnectAsync; without this gate they would race + // to tear down and rebuild the same McpClient, leaking the loser's + // client/process. Also shared by the proactive-refresh sweep's teardown on + // a terminal invalid_grant. + private readonly ConcurrentDictionary _reconnectGates = new(); + public McpClientManager( Dictionary serverEntries, ToolRegistry toolRegistry, @@ -112,17 +120,119 @@ public async Task TryReconnectAsync(McpServerName serverName, Cancellation if (!_serverEntries.TryGetValue(serverName.Value, out var entry) || !entry.Enabled) return false; - if (_clients.TryRemove(serverName, out var existing)) + var gate = _reconnectGates.GetOrAdd(serverName, static _ => new SemaphoreSlim(1, 1)); + await gate.WaitAsync(ct); + try + { + if (_clients.TryRemove(serverName, out var existing)) + { + try { await existing.DisposeAsync(); } + catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client '{Name}' during reconnect", serverName.Value); } + } + + _sharedToolFunctions.TryRemove(serverName, out _); + + return await ConnectAsync(serverName, entry, ct); + } + finally + { + gate.Release(); + } + } + + /// + /// Proactively refreshes OAuth tokens for connected, OAuth-managed servers + /// ahead of expiry (see ), + /// and surfaces advance warnings for token sets that cannot self-heal. If a + /// refresh comes back terminally rejected (invalid_grant), the connection + /// is torn down immediately and marked with a status distinct from a + /// generic connectivity failure or "never authenticated" — see + /// . Called once per + /// tick. + /// + public async Task RefreshOAuthTokensAsync(CancellationToken ct = default) + { + foreach (var (name, status) in _statuses) { - try { await existing.DisposeAsync(); } - catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client '{Name}' during reconnect", serverName.Value); } + if (status.State is not McpConnectionState.Connected) + continue; + + if (!_serverEntries.TryGetValue(name.Value, out var entry) || !entry.Enabled) + continue; + + if (_oauthService.GetTokenSet(name) is null) + continue; // not an OAuth-managed server + + try + { + _oauthService.NoteUnknownExpiryOnce(name); + _oauthService.WarnIfMissingRefreshToken(name); + + await _oauthService.GetValidTokenAsync(name, entry, ct); + + var tokenSet = _oauthService.GetTokenSet(name); + if (tokenSet is null) + { + // Tokens were just cleared: terminal invalid_grant (already + // logged and alerted by McpOAuthService). Tear the + // connection down now rather than waiting for the next + // tool call to 401 into a generic failure. + await TearDownConnectionAsync(name); + _statuses[name] = CreateRefreshRejectedStatus(name); + _logger.LogWarning( + "MCP server '{Name}' OAuth grant revoked (invalid_grant); disconnected pending re-authorization", + name.Value); + continue; + } + + var advisory = BuildOAuthAdvisory(name, tokenSet); + if (_statuses.TryGetValue(name, out var current) && current.ErrorMessage != advisory) + _statuses[name] = current with { ErrorMessage = advisory }; + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) + { + _logger.LogDebug(ex, "Proactive OAuth token refresh threw for MCP server '{Name}'", name.Value); + } } + } - _sharedToolFunctions.TryRemove(serverName, out _); + private async Task TearDownConnectionAsync(McpServerName name) + { + var gate = _reconnectGates.GetOrAdd(name, static _ => new SemaphoreSlim(1, 1)); + await gate.WaitAsync(); + try + { + if (_clients.TryRemove(name, out var client)) + { + try { await client.DisposeAsync(); } + catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client '{Name}' after auth revocation", name.Value); } + } - return await ConnectAsync(serverName, entry, ct); + _sharedToolFunctions.TryRemove(name, out _); + } + finally + { + gate.Release(); + } } + /// + /// Advisory text for an otherwise-healthy Connected status: null unless the + /// token set is time-limited but has no refresh token, in which case the + /// server will hard-fail at expiry with no chance of silent recovery. + /// Surfaced through McpServerStatusDto.Error even for the Connected + /// state so `netclaw doctor` and the daemon status API can show it ahead + /// of time (see McpServersDoctorCheck's "Connected" case). + /// + private static string? BuildOAuthAdvisory(McpServerName serverName, McpOAuthTokenSet? tokenSet) + => tokenSet is { RefreshToken: null, ExpiresAt: not null } + ? $"No refresh token — re-authorization will be required at expiry. Run: netclaw mcp auth {serverName.Value}" + : null; + public async Task InvokeAsync( string serverName, string toolName, @@ -227,7 +337,14 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, _sharedToolFunctions[name] = sharedFunctions; _clients[name] = client; client = null; - _statuses[name] = new McpServerStatus(name, McpConnectionState.Connected, tools.Count, null); + + // Advance warning for OAuth-managed servers whose token can never + // self-refresh, surfaced both in the log and in the Connected + // status text (doctor/status API) — see BuildOAuthAdvisory. + var tokenSet = _oauthService.GetTokenSet(name); + if (tokenSet is not null) + _oauthService.WarnIfMissingRefreshToken(name); + _statuses[name] = new McpServerStatus(name, McpConnectionState.Connected, tools.Count, BuildOAuthAdvisory(name, tokenSet)); _logger.LogInformation("MCP server '{Name}' connected ({ToolCount} tools)", name.Value, tools.Count); return true; @@ -326,6 +443,30 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, return null; } } + else + { + // Route the (re)connect through Netclaw's logged, single-flighted + // refresh path first, so the cached token the SDK's ITokenCache + // bridge (McpTokenCacheAdapter) sees is already fresh. The SDK's + // own on-401 refresh — silent, and headless-fatal on our stubbed + // AuthorizationRedirectDelegate — then almost never has to run. + await _oauthService.GetValidTokenAsync(name, entry, ct); + + if (_oauthService.GetTokenSet(name) is null) + { + // The refresh above came back terminally rejected + // (invalid_grant) and already cleared the tokens, logging + // and alerting. Fail fast with a status distinct from a + // generic connectivity failure instead of letting the + // doomed connection attempt fall through to the SDK's + // silent headless-auth failure path. + _statuses[name] = CreateRefreshRejectedStatus(name); + _logger.LogWarning( + "MCP server '{Name}' OAuth grant revoked (invalid_grant); re-authorization required", + name.Value); + return null; + } + } } var transport = CreateTransport(name, entry); @@ -458,6 +599,20 @@ internal static McpServerStatus CreateUnreachableStatus(McpServerName serverName => new(serverName, McpConnectionState.Unreachable, 0, string.IsNullOrWhiteSpace(ex.Message) ? "Failed to reach MCP server." : ex.Message); + /// + /// Distinct from 's generic "authentication + /// rejected by server" text: this fires specifically when Netclaw's own + /// refresh path (McpOAuthService) got a terminal invalid_grant response and + /// cleared the stored tokens, so operators can tell "grant revoked by + /// provider" apart from "never authenticated" (AwaitingAuth) or a generic + /// connectivity failure (Unreachable). Uses the AuthFailed state so the + /// reconnection service's backoff loop — which only retries Unreachable + /// servers — never auto-retries a dead grant. + /// + internal static McpServerStatus CreateRefreshRejectedStatus(McpServerName serverName) + => new(serverName, McpConnectionState.AuthFailed, 0, + $"Refresh rejected by provider (invalid_grant) — re-authorization required. Run: netclaw mcp auth {serverName.Value}"); + private void EmitAuthAlert(McpServerName serverName, string summary, string reason) { _notificationSink.Emit(OperationalAlert.Create( diff --git a/src/Netclaw.Daemon/Mcp/McpOAuthService.cs b/src/Netclaw.Daemon/Mcp/McpOAuthService.cs index 96a2b6580..9e29c27c0 100644 --- a/src/Netclaw.Daemon/Mcp/McpOAuthService.cs +++ b/src/Netclaw.Daemon/Mcp/McpOAuthService.cs @@ -332,9 +332,34 @@ public async Task CompleteAuthorizationAsync(string code, string state, Cancella // ── Token Access ─────────────────────────────────────────────────── /// - /// Get a valid access token for the given MCP server. Refreshes automatically - /// if the token is expired and a refresh token is available. Returns null - /// if no token is available or refresh fails. + /// How far ahead of the persisted to + /// refresh proactively, rather than waiting for the SDK's own on-401 fallback + /// (which is silent, and on our headless wiring terminates in a generic + /// failure — see ClientOAuthProvider). Ten minutes comfortably spans + /// several ticks (30s each) plus the + /// refresh request itself and clock skew against the provider. + /// + internal static readonly TimeSpan ProactiveRefreshWindow = TimeSpan.FromMinutes(10); + + // Per-server refresh single-flight. Providers such as Notion rotate the + // refresh token on every redemption and treat concurrent redemption of the + // same refresh token as theft, revoking the whole grant — so at most one + // refresh call per server may be in flight at a time. + private readonly ConcurrentDictionary _refreshGates = new(); + + // De-dupe advisory warnings that would otherwise re-log on every + // proactive-refresh tick (every 30s) for a server stuck in a state the + // operator already knows about. Cleared once the underlying condition is + // no longer true (e.g. a fresh refresh token or expiry is observed). + private readonly ConcurrentDictionary _warnedNoRefreshToken = new(); + private readonly ConcurrentDictionary _warnedUnknownExpiry = new(); + + /// + /// Get a valid access token for the given MCP server, refreshing proactively + /// (ahead of by ) + /// as well as reactively (already expired). Refreshes are single-flighted + /// per server. Returns null if no token is cached, no refresh token is + /// available, or the refresh attempt failed. /// public async Task GetValidTokenAsync( McpServerName serverName, McpServerEntry entry, CancellationToken ct) @@ -342,32 +367,108 @@ public async Task CompleteAuthorizationAsync(string code, string state, Cancella if (!_tokens.TryGetValue(serverName, out var tokenSet)) return null; - // If token hasn't expired, use it - if (tokenSet.ExpiresAt is null || tokenSet.ExpiresAt > _timeProvider.GetUtcNow()) + if (!NeedsRefresh(tokenSet)) return tokenSet.AccessToken.Value; - // Attempt refresh - if (tokenSet.RefreshToken is null) + var gate = _refreshGates.GetOrAdd(serverName, static _ => new SemaphoreSlim(1, 1)); + await gate.WaitAsync(ct); + try { - _logger.LogWarning("Access token expired for MCP server '{Name}' with no refresh token", serverName.Value); - - _notificationSink.Emit(OperationalAlert.Create( - _timeProvider, - "mcp.auth.expired", - AlertType.McpAuthExpired, - $"MCP server '{serverName.Value}' access token expired with no refresh token. Run: netclaw mcp auth {serverName.Value}", - AlertSeverity.Warning, - source: serverName.Value, - context: new Dictionary - { - ["serverName"] = serverName.Value, - ["reason"] = "no_refresh_token", - })); + // Re-read and re-check under the gate: a concurrent caller may have + // already refreshed — or a terminal invalid_grant may have just + // cleared the tokens entirely — while we were waiting. + if (!_tokens.TryGetValue(serverName, out tokenSet)) + return null; - return null; + if (!NeedsRefresh(tokenSet)) + return tokenSet.AccessToken.Value; + + if (tokenSet.RefreshToken is null) + { + WarnNoRefreshToken(serverName, tokenSet.ExpiresAt); + return null; + } + + return await RefreshTokenAsync(serverName, entry, tokenSet, ct); + } + finally + { + gate.Release(); } + } + + private bool NeedsRefresh(McpOAuthTokenSet tokenSet) + => tokenSet.ExpiresAt is not null + && tokenSet.ExpiresAt.Value - ProactiveRefreshWindow <= _timeProvider.GetUtcNow(); - return await RefreshTokenAsync(serverName, entry, tokenSet, ct); + /// + /// Structural check, independent of proactive-refresh timing: if a token + /// set is time-limited but has no refresh token, silent refresh is + /// impossible no matter how far off expiry is. Called at connect time and + /// on every proactive-refresh tick so operators get advance warning instead + /// of discovering it only once the token has already expired. + /// + public void WarnIfMissingRefreshToken(McpServerName serverName) + { + if (!_tokens.TryGetValue(serverName, out var tokenSet) || tokenSet.ExpiresAt is null) + return; + + if (tokenSet.RefreshToken is not null) + { + _warnedNoRefreshToken.TryRemove(serverName, out _); + return; + } + + WarnNoRefreshToken(serverName, tokenSet.ExpiresAt); + } + + /// + /// Notes (once) that a server's token set has no known expiry, so proactive + /// refresh cannot be scheduled for it — only the reactive on-401 path + /// applies. Called from the proactive-refresh sweep only; unlike + /// this is not connect-time + /// guidance, it is a guard against silently skipping proactive refresh. + /// + public void NoteUnknownExpiryOnce(McpServerName serverName) + { + if (!_tokens.TryGetValue(serverName, out var tokenSet)) + return; + + if (tokenSet.ExpiresAt is not null) + { + _warnedUnknownExpiry.TryRemove(serverName, out _); + return; + } + + if (!_warnedUnknownExpiry.TryAdd(serverName, 0)) + return; + + _logger.LogWarning( + "MCP server '{Name}' token set has no known expiry; cannot schedule proactive refresh ahead of time — reactive (on-401) refresh still applies", + serverName.Value); + } + + private void WarnNoRefreshToken(McpServerName serverName, DateTimeOffset? expiresAt) + { + if (!_warnedNoRefreshToken.TryAdd(serverName, 0)) + return; + + _logger.LogWarning( + "MCP server '{Name}' has no refresh token; silent refresh is impossible and manual re-authorization will be required at expiry ({ExpiresAt})", + serverName.Value, expiresAt); + + _notificationSink.Emit(OperationalAlert.Create( + _timeProvider, + "mcp.auth.expired", + AlertType.McpAuthExpired, + $"MCP server '{serverName.Value}' has no refresh token; access token expires at {expiresAt:u} and silent refresh is impossible. Run: netclaw mcp auth {serverName.Value}", + AlertSeverity.Warning, + source: serverName.Value, + context: new Dictionary + { + ["serverName"] = serverName.Value, + ["reason"] = "no_refresh_token", + })); } // ── Token Refresh ────────────────────────────────────────────────── @@ -402,7 +503,17 @@ public async Task CompleteAuthorizationAsync(string code, string state, Cancella if (result is null) { - _logger.LogWarning("Refresh token rejected for MCP server '{Name}' (invalid_grant). Re-authorization required.", serverName.Value); + // OAuthPkceService.RefreshTokenAsync surfaces invalid_grant as a + // null return; every other non-2xx response is thrown and + // handled in the catch below. invalid_grant is terminal per + // RFC 6749 §5.2 — the grant was revoked, or (with rotating + // refresh tokens) already consumed — so retrying with the same + // refresh token will never succeed. Clear it now so callers + // stop retrying and fail straight to `netclaw mcp auth` instead + // of looping against a dead grant. + _logger.LogWarning( + "Token refresh rejected for MCP server '{Name}': invalid_grant (terminal) — re-authorization required", + serverName.Value); _tokens.TryRemove(serverName, out _); PersistTokens(); @@ -433,13 +544,22 @@ public async Task CompleteAuthorizationAsync(string code, string state, Cancella _tokens[serverName] = newTokenSet; PersistTokens(); + _warnedNoRefreshToken.TryRemove(serverName, out _); + _warnedUnknownExpiry.TryRemove(serverName, out _); _logger.LogInformation("Token refreshed for MCP server '{Name}'", serverName.Value); return newTokenSet.AccessToken.Value; } catch (Exception ex) { - _logger.LogWarning(ex, "Failed to refresh token for MCP server '{Name}'", serverName.Value); + // Transient failure: network error, or a non-2xx/non-invalid_grant + // response (5xx, invalid_client, etc.) surfaced by EnsureSuccessStatusCode. + // Tokens are retained so the server stays retryable — the next + // proactive tick or reactive 401 will try again. + var statusCode = (ex as HttpRequestException)?.StatusCode; + _logger.LogWarning(ex, + "Token refresh failed for MCP server '{Name}' (status={StatusCode}); tokens retained, will retry", + serverName.Value, statusCode?.ToString() ?? "n/a"); return null; } } diff --git a/src/Netclaw.Daemon/Mcp/McpReconnectionService.cs b/src/Netclaw.Daemon/Mcp/McpReconnectionService.cs index 05151377e..676309578 100644 --- a/src/Netclaw.Daemon/Mcp/McpReconnectionService.cs +++ b/src/Netclaw.Daemon/Mcp/McpReconnectionService.cs @@ -45,6 +45,28 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) while (await timer.WaitForNextTickAsync(stoppingToken)) { await CheckAndReconnectAsync(stoppingToken); + await RefreshOAuthTokensAsync(stoppingToken); + } + } + + /// + /// Drives on the + /// same 30s cadence as reconnection — the natural existing host for + /// proactive OAuth refresh (see McpOAuthService.ProactiveRefreshWindow). + /// + internal async Task RefreshOAuthTokensAsync(CancellationToken ct) + { + try + { + await _mcpReconnectable.RefreshOAuthTokensAsync(ct); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) + { + _logger.LogDebug(ex, "Proactive MCP OAuth token refresh sweep threw an exception"); } } From d56a03e1255b21785ace3236c8e9ea92e47bdbd0 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 18 Jul 2026 22:05:09 +0000 Subject: [PATCH 2/3] fix(mcp): treat invalid_client/unauthorized_client as terminal refresh rejections Parse the RFC 6749 error body at the token endpoint (4xx only) and surface invalid_grant, invalid_client, and unauthorized_client as a typed OAuthTokenRefreshRejectedException. McpOAuthService handles all three in one shared terminal path (clear + persist + alert with the code as reason); client errors additionally drop the cached DCR client_id so re-auth performs a fresh registration instead of reusing the dead one. 5xx, network errors, and unrecognized/unparsable bodies stay transient. --- .../references/diagnostics.md | 2 +- .../Providers/OAuth/OAuthPkceServiceTests.cs | 71 +++++++++- .../Mcp/McpClientManagerStatusTests.cs | 17 ++- .../Mcp/McpOAuthServiceTests.cs | 101 ++++++++++++++ src/Netclaw.Daemon/Mcp/McpClientManager.cs | 64 +++++---- src/Netclaw.Daemon/Mcp/McpOAuthService.cs | 126 +++++++++++++----- .../OAuth/OAuthPkceService.cs | 58 +++++++- .../OAuthTokenRefreshRejectedException.cs | 35 +++++ 8 files changed, 395 insertions(+), 79 deletions(-) create mode 100644 src/Netclaw.Providers/OAuth/OAuthTokenRefreshRejectedException.cs diff --git a/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md b/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md index aca9570bf..e2fce49bb 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md +++ b/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md @@ -74,7 +74,7 @@ debugging a daemon-wide problem → read `daemon.log`. |---------|-------| | No LLM responses | `netclaw doctor`; verify provider credentials | | Missing tools | `netclaw mcp list`; check MCP connection state | -| MCP server `auth failed` in `netclaw doctor` / `netclaw mcp status` | Fixed by `netclaw mcp auth ` either way, but the message tells you which case: "refresh rejected (invalid_grant)" means Netclaw's proactive refresh got a terminal rejection from the provider — the stored tokens were already cleared and the connection torn down (a revoked/rotated grant, not a config error). A generic "authentication rejected by server" means static credentials (token/headers) were rejected. `awaiting auth` is different from both — that server has never completed OAuth at all. A server reported **connected** with a "No refresh token" note is healthy now but cannot silently refresh; re-run `netclaw mcp auth ` before its current token expires. | +| MCP server `auth failed` in `netclaw doctor` / `netclaw mcp status` | Fixed by `netclaw mcp auth ` either way, but the message tells you which case: "refresh rejected ()" means Netclaw's proactive refresh got a terminal rejection from the provider — the stored tokens were already cleared and the connection torn down. The code names the cause: `invalid_grant` = the grant itself was revoked/rotated; `invalid_client` / `unauthorized_client` = the provider no longer recognizes the client registration (Netclaw also dropped the cached client_id, so re-auth performs a fresh registration automatically). Neither is a config error. A generic "authentication rejected by server" means static credentials (token/headers) were rejected. `awaiting auth` is different from all of these — that server has never completed OAuth at all. A server reported **connected** with a "No refresh token" note is healthy now but cannot silently refresh; re-run `netclaw mcp auth ` before its current token expires. | | Memory recall degraded | `netclaw status` memory section | | Daemon won't start | crash logs at `/logs/crash-*.log` (`NETCLAW_HOME` defaults to `~/.netclaw`) | | Docker daemon cannot create `/home/netclaw/.netclaw/*` | Official image entrypoint repairs writable bind mounts to UID/GID `1654:1654`; if bypassed or read-only, run `sudo chown -R 1654:1654 ` or use a Docker named volume | diff --git a/src/Netclaw.Configuration.Tests/Providers/OAuth/OAuthPkceServiceTests.cs b/src/Netclaw.Configuration.Tests/Providers/OAuth/OAuthPkceServiceTests.cs index 7e472770b..6539e3bd4 100644 --- a/src/Netclaw.Configuration.Tests/Providers/OAuth/OAuthPkceServiceTests.cs +++ b/src/Netclaw.Configuration.Tests/Providers/OAuth/OAuthPkceServiceTests.cs @@ -288,20 +288,77 @@ public async Task RefreshToken_Success_ReturnsNewTokenPreservingRefresh() Assert.Equal("old-refresh", result.RefreshToken!.Value); } + [Theory] + [InlineData("invalid_grant", HttpStatusCode.BadRequest)] + [InlineData("invalid_client", HttpStatusCode.Unauthorized)] + [InlineData("unauthorized_client", HttpStatusCode.BadRequest)] + public async Task RefreshToken_TerminalOAuthError_ThrowsRejectedWithErrorCode( + string errorCode, HttpStatusCode status) + { + var handler = new FakeHttpMessageHandler(_ => + JsonResponse(new { error = errorCode }, status)); + + var service = new OAuthPkceService(new HttpClient(handler)); + + var ex = await Assert.ThrowsAsync(() => + service.RefreshTokenAsync( + "https://auth.example.com/token", + "test-client", + new SensitiveString("expired-refresh"), ct: TestContext.Current.CancellationToken)); + + Assert.Equal(errorCode, ex.ErrorCode); + Assert.Equal(status, ex.StatusCode); + } + [Fact] - public async Task RefreshToken_InvalidGrant_ReturnsNull() + public async Task RefreshToken_UnrecognizedOAuthError_ThrowsHttpRequestException() { + // "invalid_request" is a 4xx OAuth error but NOT a terminal refresh + // code — it must stay transient (retryable), not clear credentials. var handler = new FakeHttpMessageHandler(_ => - JsonResponse(new { error = "invalid_grant" }, HttpStatusCode.BadRequest)); + JsonResponse(new { error = "invalid_request" }, HttpStatusCode.BadRequest)); var service = new OAuthPkceService(new HttpClient(handler)); - var result = await service.RefreshTokenAsync( - "https://auth.example.com/token", - "test-client", - new SensitiveString("expired-refresh"), ct: TestContext.Current.CancellationToken); + await Assert.ThrowsAsync(() => + service.RefreshTokenAsync( + "https://auth.example.com/token", + "test-client", + new SensitiveString("expired-refresh"), ct: TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task RefreshToken_UnparsableErrorBody_ThrowsHttpRequestException() + { + var handler = new FakeHttpMessageHandler(_ => new HttpResponseMessage(HttpStatusCode.BadRequest) + { + Content = new StringContent("gateway error"), + }); + + var service = new OAuthPkceService(new HttpClient(handler)); + + await Assert.ThrowsAsync(() => + service.RefreshTokenAsync( + "https://auth.example.com/token", + "test-client", + new SensitiveString("expired-refresh"), ct: TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task RefreshToken_ServerErrorWithTerminalShapedBody_ThrowsHttpRequestException() + { + // A 5xx is a server-side fault worth retrying even when its body + // happens to carry a terminal-looking error code. + var handler = new FakeHttpMessageHandler(_ => + JsonResponse(new { error = "invalid_grant" }, HttpStatusCode.ServiceUnavailable)); + + var service = new OAuthPkceService(new HttpClient(handler)); - Assert.Null(result); + await Assert.ThrowsAsync(() => + service.RefreshTokenAsync( + "https://auth.example.com/token", + "test-client", + new SensitiveString("expired-refresh"), ct: TestContext.Current.CancellationToken)); } [Fact] diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs index 6db93a5c3..c5152b25b 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs @@ -75,20 +75,23 @@ public void BuildConnectionFailureStatus_ForNetworkFailure_ReturnsUnreachable() Assert.Contains("Connection refused", status.ErrorMessage); } - [Fact] - public void CreateRefreshRejectedStatus_ReturnsAuthFailedWithDistinctInvalidGrantMessage() + [Theory] + [InlineData("invalid_grant")] + [InlineData("invalid_client")] + [InlineData("unauthorized_client")] + public void CreateRefreshRejectedStatus_ReturnsAuthFailedNamingTheErrorCode(string errorCode) { - var status = McpClientManager.CreateRefreshRejectedStatus(new McpServerName("notion")); + var status = McpClientManager.CreateRefreshRejectedStatus(new McpServerName("notion"), errorCode); // AuthFailed (not Unreachable) so McpReconnectionService's backoff loop - // — which only retries Unreachable servers — never auto-retries a dead - // grant into a loop. + // — which only retries Unreachable servers — never auto-retries dead + // credentials into a loop. Assert.Equal(McpConnectionState.AuthFailed, status.State); - Assert.Contains("invalid_grant", status.ErrorMessage); + Assert.Contains(errorCode, status.ErrorMessage); Assert.Contains("netclaw mcp auth notion", status.ErrorMessage); // Distinct wording from the generic AuthFailed message so operators can - // tell "grant revoked by provider" apart from "never authenticated". + // tell "credentials rejected by provider" apart from "never authenticated". var genericAuthFailed = McpClientManager.CreateAuthFailedStatus( new McpServerName("notion"), new HttpRequestException(httpRequestError: HttpRequestError.Unknown, "Unauthorized", null, System.Net.HttpStatusCode.Unauthorized), diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs index 628d550e7..e60353b86 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs @@ -53,12 +53,113 @@ public async Task GetValidTokenAsync_InvalidGrant_ClearsTokensAndDoesNotRetry() Assert.Equal(AlertType.McpAuthExpired, alert.Category); Assert.Equal("invalid_grant", alert.Context!["reason"]); + // invalid_grant kills the grant, not the client registration — the + // cached client_id must survive for the re-auth flow. + Assert.Equal("test-client", service.GetCachedMetadata(serverName)!.ClientId); + // Subsequent calls must not re-attempt refresh against cleared tokens. var second = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); Assert.Null(second); Assert.Equal(1, handler.RefreshCallCount); } + [Fact] + public async Task GetValidTokenAsync_InvalidClient_ClearsTokensAndClientRegistrationAndDoesNotRetry() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var sink = new RecordingNotificationSink(); + var handler = new TokenEndpointHandler + { + // invalid_client MAY be a 401 per RFC 6749 §5.2. + RefreshResponse = _ => FakeHttpMessageHandler.JsonResponse(new { error = "invalid_client" }, HttpStatusCode.Unauthorized), + }; + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var service = CreateService(CreateDiscoveryClient(), new OAuthPkceService(new HttpClient(handler), time), + timeProvider: time, notificationSink: sink); + + await SeedTokenAsync(service, serverName, entry, handler, expiresInSeconds: 60, refreshToken: "refresh-old"); + Assert.Equal("test-client", service.GetCachedMetadata(serverName)!.ClientId); + time.Advance(TimeSpan.FromMinutes(5)); // well past expiry + + var token = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + + // Terminal like invalid_grant: tokens cleared, alert with the actual code. + Assert.Null(token); + Assert.Null(service.GetTokenSet(serverName)); + Assert.Equal(1, handler.RefreshCallCount); + Assert.True(service.TryGetTerminalRefreshRejection(serverName, out var errorCode)); + Assert.Equal("invalid_client", errorCode); + + var alert = Assert.Single(sink.Alerts); + Assert.Equal(AlertType.McpAuthExpired, alert.Category); + Assert.Equal("invalid_client", alert.Context!["reason"]); + + // The client registration itself is dead — the cached client_id must be + // dropped so the next `netclaw mcp auth` re-registers instead of + // reusing it (EnsureClientRegisteredAsync short-circuits on it). + Assert.Null(service.GetCachedMetadata(serverName)!.ClientId); + + // No retry loop against dead credentials. + var second = await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + Assert.Null(second); + Assert.Equal(1, handler.RefreshCallCount); + } + + [Fact] + public async Task GetValidTokenAsync_InvalidClient_ReauthPerformsFreshClientRegistration() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var registrationCount = 0; + // DCR-capable auth server, and a server entry with NO static client id, + // so the client_id in play is dynamically registered — the incident + // scenario where the provider purges DCR'd client IDs. + var discovery = new HttpClient(new FakeHttpMessageHandler(request => request.RequestUri!.ToString() switch + { + "https://mcp.example.com/" or "https://mcp.example.com" => new HttpResponseMessage(HttpStatusCode.Unauthorized), + "https://mcp.example.com/.well-known/oauth-protected-resource" => JsonResponse(new + { + authorization_servers = new[] { "https://auth.example.com" }, + resource = "https://mcp.example.com/resource" + }), + "https://auth.example.com/.well-known/oauth-authorization-server" => JsonResponse(new + { + authorization_endpoint = "https://auth.example.com/authorize", + token_endpoint = "https://auth.example.com/token", + registration_endpoint = "https://auth.example.com/register" + }), + "https://auth.example.com/register" => JsonResponse(new { client_id = $"dcr-client-{++registrationCount}" }), + _ => throw new InvalidOperationException($"Unexpected request URI: {request.RequestUri}") + })); + var handler = new TokenEndpointHandler + { + RefreshResponse = _ => FakeHttpMessageHandler.JsonResponse(new { error = "invalid_client" }, HttpStatusCode.Unauthorized), + }; + var serverName = new McpServerName("notion"); + var entry = new McpServerEntry { Transport = "http", Url = "https://mcp.example.com" }; + var service = CreateService(discovery, new OAuthPkceService(new HttpClient(handler), time), timeProvider: time); + + await SeedTokenAsync(service, serverName, entry, handler, expiresInSeconds: 60, refreshToken: "refresh-old"); + Assert.Equal(1, registrationCount); + Assert.Equal("dcr-client-1", service.GetCachedMetadata(serverName)!.ClientId); + + time.Advance(TimeSpan.FromMinutes(5)); + await service.GetValidTokenAsync(serverName, entry, TestContext.Current.CancellationToken); + Assert.Null(service.GetTokenSet(serverName)); + + // Re-auth must perform a FRESH dynamic registration, not reuse the dead + // client_id, and a completed flow clears the terminal-rejection record. + var (_, state) = await service.StartAuthorizationFlowAsync(serverName, entry, TestContext.Current.CancellationToken); + Assert.Equal(2, registrationCount); + Assert.Equal("dcr-client-2", service.GetCachedMetadata(serverName)!.ClientId); + + await service.CompleteAuthorizationAsync("code-2", state, TestContext.Current.CancellationToken); + Assert.NotNull(service.GetTokenSet(serverName)); + Assert.False(service.TryGetTerminalRefreshRejection(serverName, out _)); + } + [Fact] public async Task GetValidTokenAsync_TransientFailure_RetainsTokensAndLogsStatus() { diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index 66492936a..66fdebf70 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -171,17 +171,19 @@ public async Task RefreshOAuthTokensAsync(CancellationToken ct = default) await _oauthService.GetValidTokenAsync(name, entry, ct); var tokenSet = _oauthService.GetTokenSet(name); - if (tokenSet is null) + if (tokenSet is null + && _oauthService.TryGetTerminalRefreshRejection(name, out var errorCode)) { - // Tokens were just cleared: terminal invalid_grant (already - // logged and alerted by McpOAuthService). Tear the + // Tokens were just cleared: terminal rejection + // (invalid_grant / invalid_client / unauthorized_client — + // already logged and alerted by McpOAuthService). Tear the // connection down now rather than waiting for the next // tool call to 401 into a generic failure. await TearDownConnectionAsync(name); - _statuses[name] = CreateRefreshRejectedStatus(name); + _statuses[name] = CreateRefreshRejectedStatus(name, errorCode); _logger.LogWarning( - "MCP server '{Name}' OAuth grant revoked (invalid_grant); disconnected pending re-authorization", - name.Value); + "MCP server '{Name}' OAuth refresh terminally rejected ({ErrorCode}); disconnected pending re-authorization", + name.Value, errorCode); continue; } @@ -436,6 +438,17 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, var metadata = await _oauthService.TryDiscoverMetadataAsync(name, entry.Url, ct); if (metadata is not null && entry.Headers is not { Count: > 0 }) { + // A prior terminal refresh rejection cleared this server's + // tokens; keep the distinct "refresh rejected" status + // rather than downgrading to generic AwaitingAuth on every + // subsequent reconnect attempt. The terminal alert (with + // the actual error code) already fired at rejection time. + if (_oauthService.TryGetTerminalRefreshRejection(name, out var priorRejection)) + { + _statuses[name] = CreateRefreshRejectedStatus(name, priorRejection); + return null; + } + _statuses[name] = CreateAwaitingAuthStatus(name); _logger.LogWarning("MCP server '{Name}' requires OAuth authorization", name.Value); EmitAuthAlert(name, $"MCP server '{name.Value}' requires OAuth authorization. Run: netclaw mcp auth {name.Value}", "authorization_required"); @@ -452,18 +465,20 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, // AuthorizationRedirectDelegate — then almost never has to run. await _oauthService.GetValidTokenAsync(name, entry, ct); - if (_oauthService.GetTokenSet(name) is null) + if (_oauthService.GetTokenSet(name) is null + && _oauthService.TryGetTerminalRefreshRejection(name, out var errorCode)) { // The refresh above came back terminally rejected - // (invalid_grant) and already cleared the tokens, logging - // and alerting. Fail fast with a status distinct from a - // generic connectivity failure instead of letting the - // doomed connection attempt fall through to the SDK's - // silent headless-auth failure path. - _statuses[name] = CreateRefreshRejectedStatus(name); + // (invalid_grant / invalid_client / unauthorized_client) + // and already cleared the tokens, logging and alerting. + // Fail fast with a status distinct from a generic + // connectivity failure instead of letting the doomed + // connection attempt fall through to the SDK's silent + // headless-auth failure path. + _statuses[name] = CreateRefreshRejectedStatus(name, errorCode); _logger.LogWarning( - "MCP server '{Name}' OAuth grant revoked (invalid_grant); re-authorization required", - name.Value); + "MCP server '{Name}' OAuth refresh terminally rejected ({ErrorCode}); re-authorization required", + name.Value, errorCode); return null; } } @@ -602,16 +617,19 @@ internal static McpServerStatus CreateUnreachableStatus(McpServerName serverName /// /// Distinct from 's generic "authentication /// rejected by server" text: this fires specifically when Netclaw's own - /// refresh path (McpOAuthService) got a terminal invalid_grant response and - /// cleared the stored tokens, so operators can tell "grant revoked by - /// provider" apart from "never authenticated" (AwaitingAuth) or a generic - /// connectivity failure (Unreachable). Uses the AuthFailed state so the - /// reconnection service's backoff loop — which only retries Unreachable - /// servers — never auto-retries a dead grant. + /// refresh path (McpOAuthService) got a terminal token-endpoint rejection + /// and cleared the stored tokens. names the + /// cause — invalid_grant (grant revoked by provider) vs invalid_client / + /// unauthorized_client (client registration purged; re-auth performs a + /// fresh registration) — so operators can tell either apart from "never + /// authenticated" (AwaitingAuth) or a generic connectivity failure + /// (Unreachable). Uses the AuthFailed state so the reconnection service's + /// backoff loop — which only retries Unreachable servers — never + /// auto-retries dead credentials. /// - internal static McpServerStatus CreateRefreshRejectedStatus(McpServerName serverName) + internal static McpServerStatus CreateRefreshRejectedStatus(McpServerName serverName, string errorCode) => new(serverName, McpConnectionState.AuthFailed, 0, - $"Refresh rejected by provider (invalid_grant) — re-authorization required. Run: netclaw mcp auth {serverName.Value}"); + $"Refresh rejected by provider ({errorCode}) — re-authorization required. Run: netclaw mcp auth {serverName.Value}"); private void EmitAuthAlert(McpServerName serverName, string summary, string reason) { diff --git a/src/Netclaw.Daemon/Mcp/McpOAuthService.cs b/src/Netclaw.Daemon/Mcp/McpOAuthService.cs index 9e29c27c0..963f14ec4 100644 --- a/src/Netclaw.Daemon/Mcp/McpOAuthService.cs +++ b/src/Netclaw.Daemon/Mcp/McpOAuthService.cs @@ -306,6 +306,13 @@ public async Task CompleteAuthorizationAsync(string code, string state, Cancella _tokens[context.ServerName] = tokenSet; PersistTokens(); + // A fresh grant supersedes any prior terminal rejection, and its + // advisory conditions (missing refresh token / unknown expiry) + // should be re-evaluated — and re-warned if still true. + _terminalRefreshErrors.TryRemove(context.ServerName, out _); + _warnedNoRefreshToken.TryRemove(context.ServerName, out _); + _warnedUnknownExpiry.TryRemove(context.ServerName, out _); + // Cache the server name so callers can trigger reconnect after cleanup _lastCompletedServerName[state] = context.ServerName; @@ -354,6 +361,24 @@ public async Task CompleteAuthorizationAsync(string code, string state, Cancella private readonly ConcurrentDictionary _warnedNoRefreshToken = new(); private readonly ConcurrentDictionary _warnedUnknownExpiry = new(); + // OAuth error code of the last terminal refresh rejection per server, so + // the connection manager can surface the specific code (invalid_grant vs + // invalid_client vs unauthorized_client) in the server status. Written + // just before the tokens are cleared; removed when the operator completes + // a fresh auth flow. + private readonly ConcurrentDictionary _terminalRefreshErrors = new(); + + /// + /// True when the server's last refresh attempt was terminally rejected; + /// carries the OAuth error code. Callers + /// should pair this with a tokens-cleared check ( + /// returning null) — the record intentionally lingers until re-auth so the + /// distinct status survives reconnect attempts. + /// + internal bool TryGetTerminalRefreshRejection( + McpServerName serverName, [System.Diagnostics.CodeAnalysis.NotNullWhen(true)] out string? errorCode) + => _terminalRefreshErrors.TryGetValue(serverName, out errorCode); + /// /// Get a valid access token for the given MCP server, refreshing proactively /// (ahead of by ) @@ -501,38 +526,6 @@ private void WarnNoRefreshToken(McpServerName serverName, DateTimeOffset? expire extraParams, ct); - if (result is null) - { - // OAuthPkceService.RefreshTokenAsync surfaces invalid_grant as a - // null return; every other non-2xx response is thrown and - // handled in the catch below. invalid_grant is terminal per - // RFC 6749 §5.2 — the grant was revoked, or (with rotating - // refresh tokens) already consumed — so retrying with the same - // refresh token will never succeed. Clear it now so callers - // stop retrying and fail straight to `netclaw mcp auth` instead - // of looping against a dead grant. - _logger.LogWarning( - "Token refresh rejected for MCP server '{Name}': invalid_grant (terminal) — re-authorization required", - serverName.Value); - _tokens.TryRemove(serverName, out _); - PersistTokens(); - - _notificationSink.Emit(OperationalAlert.Create( - _timeProvider, - "mcp.auth.expired", - AlertType.McpAuthExpired, - $"MCP server '{serverName.Value}' refresh token rejected (invalid_grant). Run: netclaw mcp auth {serverName.Value}", - AlertSeverity.Warning, - source: serverName.Value, - context: new Dictionary - { - ["serverName"] = serverName.Value, - ["reason"] = "invalid_grant", - })); - - return null; - } - var newTokenSet = new McpOAuthTokenSet { AccessToken = result.AccessToken, @@ -550,12 +543,51 @@ private void WarnNoRefreshToken(McpServerName serverName, DateTimeOffset? expire _logger.LogInformation("Token refreshed for MCP server '{Name}'", serverName.Value); return newTokenSet.AccessToken.Value; } + catch (OAuthTokenRefreshRejectedException ex) + { + // Terminal per RFC 6749 §5.2 — one shared path for all three codes, + // with the code carried as data. invalid_grant: the grant was + // revoked or the rotating refresh token already consumed. + // invalid_client / unauthorized_client: the client registration + // itself is dead (e.g. a provider purged its DCR'd client IDs). + // Either way, retrying with the same credentials can never succeed: + // clear the stored tokens so callers stop retrying and fail + // straight to `netclaw mcp auth`. + _logger.LogWarning( + "Token refresh rejected for MCP server '{Name}': {ErrorCode} (terminal, HTTP {StatusCode}) — re-authorization required", + serverName.Value, ex.ErrorCode, (int)ex.StatusCode); + + // Record the code before clearing the tokens: readers pair the + // rejection record with a tokens-cleared check, so the record must + // be visible first. + _terminalRefreshErrors[serverName] = ex.ErrorCode; + _tokens.TryRemove(serverName, out _); + PersistTokens(); + + if (ex.ErrorCode is "invalid_client" or "unauthorized_client") + ClearRegisteredClient(serverName); + + _notificationSink.Emit(OperationalAlert.Create( + _timeProvider, + "mcp.auth.expired", + AlertType.McpAuthExpired, + $"MCP server '{serverName.Value}' token refresh rejected ({ex.ErrorCode}). Run: netclaw mcp auth {serverName.Value}", + AlertSeverity.Warning, + source: serverName.Value, + context: new Dictionary + { + ["serverName"] = serverName.Value, + ["reason"] = ex.ErrorCode, + })); + + return null; + } catch (Exception ex) { - // Transient failure: network error, or a non-2xx/non-invalid_grant - // response (5xx, invalid_client, etc.) surfaced by EnsureSuccessStatusCode. - // Tokens are retained so the server stays retryable — the next - // proactive tick or reactive 401 will try again. + // Transient failure: network error, 5xx, or a 4xx without a + // recognized terminal OAuth error code (surfaced by + // EnsureSuccessStatusCode). Tokens are retained so the server stays + // retryable — the next proactive tick or reactive 401 will try again. var statusCode = (ex as HttpRequestException)?.StatusCode; _logger.LogWarning(ex, "Token refresh failed for MCP server '{Name}' (status={StatusCode}); tokens retained, will retry", @@ -564,6 +596,28 @@ private void WarnNoRefreshToken(McpServerName serverName, DateTimeOffset? expire } } + /// + /// Drops the cached (DCR-issued) client_id after the auth server rejected + /// the client itself. + /// short-circuits on a cached , + /// so without this the next `netclaw mcp auth` would reuse the dead + /// client_id; clearing it forces a fresh dynamic registration. Statically + /// configured client IDs () take + /// precedence there and are re-applied regardless. + /// + private void ClearRegisteredClient(McpServerName serverName) + { + if (!_metadata.TryGetValue(serverName, out var meta) || string.IsNullOrEmpty(meta.ClientId)) + return; + + meta.ClientId = null; + _metadata[serverName] = meta; + PersistMetadata(); + _logger.LogWarning( + "Cleared cached OAuth client registration for MCP server '{Name}'; the next authorization will re-register", + serverName.Value); + } + // ── SDK Token Cache Bridge ────────────────────────────────────────── /// diff --git a/src/Netclaw.Providers/OAuth/OAuthPkceService.cs b/src/Netclaw.Providers/OAuth/OAuthPkceService.cs index f3ae915a3..17614c1d2 100644 --- a/src/Netclaw.Providers/OAuth/OAuthPkceService.cs +++ b/src/Netclaw.Providers/OAuth/OAuthPkceService.cs @@ -152,11 +152,27 @@ public async Task ExchangeCodeForTokensAsync( return OAuthTokenResponseParser.Parse(doc.RootElement, _timeProvider); } + /// + /// OAuth error codes (RFC 6749 §5.2) that terminally reject a refresh + /// request — retrying with the same credentials can never succeed. + /// invalid_grant: the grant was revoked or the rotating refresh + /// token already consumed. invalid_client / + /// unauthorized_client: the client registration itself is dead. + /// + private static readonly HashSet TerminalRefreshErrorCodes = new(StringComparer.Ordinal) + { + "invalid_grant", "invalid_client", "unauthorized_client", + }; + /// /// Exchange a refresh token for a new access token. - /// Returns null if the refresh token is invalid or revoked. + /// Throws (carrying the + /// OAuth error code) when the token endpoint terminally rejects the request + /// — invalid_grant, invalid_client, unauthorized_client. Everything else + /// (5xx, network errors, unrecognized or unparsable error bodies) throws + /// and should be treated as transient. /// - public async Task RefreshTokenAsync( + public async Task RefreshTokenAsync( string tokenEndpoint, string clientId, SensitiveString refreshToken, @@ -179,9 +195,18 @@ public async Task ExchangeCodeForTokensAsync( if (!response.IsSuccessStatusCode) { - var errorBody = await response.Content.ReadAsStringAsync(ct); - if (errorBody.Contains("invalid_grant", StringComparison.Ordinal)) - return null; + // Only classify 4xx responses as potentially terminal: a 5xx is a + // server-side fault worth retrying even if its body happens to be + // error-shaped. + if ((int)response.StatusCode is >= 400 and < 500) + { + var errorBody = await response.Content.ReadAsStringAsync(ct); + if (TryParseOAuthErrorCode(errorBody) is { } errorCode + && TerminalRefreshErrorCodes.Contains(errorCode)) + { + throw new OAuthTokenRefreshRejectedException(errorCode, response.StatusCode); + } + } response.EnsureSuccessStatusCode(); } @@ -199,6 +224,29 @@ public async Task ExchangeCodeForTokensAsync( return result; } + /// + /// Parses the error member of an RFC 6749 §5.2 error response body. + /// Returns null when the body is not a JSON object or has no string + /// error member — such responses are treated as transient, never + /// terminal. + /// + private static string? TryParseOAuthErrorCode(string body) + { + try + { + using var doc = JsonDocument.Parse(body); + return doc.RootElement.ValueKind == JsonValueKind.Object + && doc.RootElement.TryGetProperty("error", out var errorProp) + && errorProp.ValueKind == JsonValueKind.String + ? errorProp.GetString() + : null; + } + catch (JsonException) + { + return null; + } + } + /// /// Start a temporary HTTP listener on the redirect URI's port to receive the OAuth callback. /// Listens until the callback arrives or the cancellation token fires. diff --git a/src/Netclaw.Providers/OAuth/OAuthTokenRefreshRejectedException.cs b/src/Netclaw.Providers/OAuth/OAuthTokenRefreshRejectedException.cs new file mode 100644 index 000000000..65aa9b07a --- /dev/null +++ b/src/Netclaw.Providers/OAuth/OAuthTokenRefreshRejectedException.cs @@ -0,0 +1,35 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Net; + +namespace Netclaw.Providers.OAuth; + +/// +/// Thrown when an OAuth token endpoint terminally rejects a refresh request +/// (RFC 6749 §5.2): invalid_grant (the grant was revoked or the rotating +/// refresh token already consumed) or invalid_client / +/// unauthorized_client (the client registration itself is dead — e.g. a +/// provider purged its dynamically-registered client IDs). Retrying with the +/// same refresh token / client_id can never succeed, so callers must clear the +/// stored credentials and re-authorize. Transient failures (5xx, network +/// errors, unrecognized or unparsable error bodies) surface as +/// instead. +/// +public sealed class OAuthTokenRefreshRejectedException : Exception +{ + public OAuthTokenRefreshRejectedException(string errorCode, HttpStatusCode statusCode) + : base($"Token endpoint terminally rejected the refresh request: {errorCode} (HTTP {(int)statusCode})") + { + ErrorCode = errorCode; + StatusCode = statusCode; + } + + /// The OAuth error code returned by the token endpoint. + public string ErrorCode { get; } + + /// The HTTP status code of the rejection response. + public HttpStatusCode StatusCode { get; } +} From eeb1dadd218dfdcf3e07044dad0cf6bd2a2abcdf Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sun, 19 Jul 2026 01:51:58 +0000 Subject: [PATCH 3/3] fix(mcp): make OAuth refresh ownership single-flight --- .../references/diagnostics.md | 2 +- .../Mcp/McpOAuthServiceTests.cs | 201 +++++++++++++++++- .../Mcp/McpReconnectGateTests.cs | 127 +++++++++++ .../Mcp/McpTokenCacheAdapterTests.cs | 126 ----------- src/Netclaw.Daemon/Mcp/McpClientManager.cs | 154 ++++++-------- .../Mcp/McpOAuthAuthorizationHandler.cs | 102 +++++++++ src/Netclaw.Daemon/Mcp/McpOAuthService.cs | 64 ++++-- src/Netclaw.Daemon/Mcp/McpReconnectGate.cs | 56 +++++ .../Mcp/McpTokenCacheAdapter.cs | 89 -------- 9 files changed, 592 insertions(+), 329 deletions(-) create mode 100644 src/Netclaw.Daemon.Tests/Mcp/McpReconnectGateTests.cs delete mode 100644 src/Netclaw.Daemon.Tests/Mcp/McpTokenCacheAdapterTests.cs create mode 100644 src/Netclaw.Daemon/Mcp/McpOAuthAuthorizationHandler.cs create mode 100644 src/Netclaw.Daemon/Mcp/McpReconnectGate.cs delete mode 100644 src/Netclaw.Daemon/Mcp/McpTokenCacheAdapter.cs diff --git a/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md b/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md index e2fce49bb..cd1ef5704 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md +++ b/feeds/skills/.system/files/netclaw-operations/references/diagnostics.md @@ -74,7 +74,7 @@ debugging a daemon-wide problem → read `daemon.log`. |---------|-------| | No LLM responses | `netclaw doctor`; verify provider credentials | | Missing tools | `netclaw mcp list`; check MCP connection state | -| MCP server `auth failed` in `netclaw doctor` / `netclaw mcp status` | Fixed by `netclaw mcp auth ` either way, but the message tells you which case: "refresh rejected ()" means Netclaw's proactive refresh got a terminal rejection from the provider — the stored tokens were already cleared and the connection torn down. The code names the cause: `invalid_grant` = the grant itself was revoked/rotated; `invalid_client` / `unauthorized_client` = the provider no longer recognizes the client registration (Netclaw also dropped the cached client_id, so re-auth performs a fresh registration automatically). Neither is a config error. A generic "authentication rejected by server" means static credentials (token/headers) were rejected. `awaiting auth` is different from all of these — that server has never completed OAuth at all. A server reported **connected** with a "No refresh token" note is healthy now but cannot silently refresh; re-run `netclaw mcp auth ` before its current token expires. | +| MCP server `auth failed` in `netclaw doctor` / `netclaw mcp status` | Fixed by `netclaw mcp auth ` either way, but the message tells you which case: "refresh rejected ()" means Netclaw's proactive or on-401 refresh got a terminal rejection from the provider — the stored tokens were already cleared and the connection torn down. The code names the cause: `invalid_grant` = the grant itself was revoked/rotated; `invalid_client` / `unauthorized_client` = the provider no longer recognizes the client registration (Netclaw also dropped the cached client_id, so re-auth performs a fresh registration automatically). Neither is a config error. A generic "authentication rejected by server" means static credentials (token/headers) were rejected. `awaiting auth` is different from all of these — that server has never completed OAuth at all. A server reported **connected** with a "No refresh token" note is healthy now but cannot silently refresh; re-run `netclaw mcp auth ` before its current token expires. | | Memory recall degraded | `netclaw status` memory section | | Daemon won't start | crash logs at `/logs/crash-*.log` (`NETCLAW_HOME` defaults to `~/.netclaw`) | | Docker daemon cannot create `/home/netclaw/.netclaw/*` | Official image entrypoint repairs writable bind mounts to UID/GID `1654:1654`; if bypassed or read-only, run `sudo chown -R 1654:1654 ` or use a Docker named volume | diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs index e60353b86..ea987680b 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthServiceTests.cs @@ -9,6 +9,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Time.Testing; +using Netclaw.Actors.Tools; using Netclaw.Configuration; using Netclaw.Configuration.Secrets; using Netclaw.Daemon.Mcp; @@ -423,6 +424,150 @@ public async Task LoadTokensFromDisk_survives_encrypted_round_trip() } } + [Fact] + public async Task AuthorizationHandler_ConcurrentUnauthorizedResponses_SingleFlightRefreshAndRetry() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var tokenHandler = new GatedTokenEndpointHandler + { + RefreshResponseBody = new + { + access_token = "access-new", + refresh_token = "refresh-new", + expires_in = 3600, + }, + }; + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var service = CreateService( + CreateDiscoveryClient(), + new OAuthPkceService(new HttpClient(tokenHandler), time), + timeProvider: time); + + await SeedTokenAsync(service, serverName, entry, tokenHandler, + expiresInSeconds: 3600, refreshToken: "refresh-old"); + + var resourceHandler = new RejectStaleAccessTokenHandler(); + using var client = new HttpClient(new McpOAuthAuthorizationHandler( + serverName, entry, service, resourceHandler)); + + var requests = Enumerable.Range(0, 5) + .Select(_ => client.GetStringAsync("https://mcp.example.com/tools", + TestContext.Current.CancellationToken)) + .ToArray(); + + await tokenHandler.WaitForFirstRefreshRequestAsync() + .WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + tokenHandler.ReleaseFirstRefreshRequest(); + + var results = await Task.WhenAll(requests); + + Assert.All(results, result => Assert.Equal("ok", result)); + Assert.Equal(1, tokenHandler.RefreshCallCount); + Assert.Equal(5, resourceHandler.RejectedRequestCount); + Assert.Equal(5, resourceHandler.AcceptedRequestCount); + } + + [Fact] + public async Task AuthorizationHandler_UnauthorizedPost_RetryPreservesBody() + { + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var tokenHandler = new TokenEndpointHandler + { + RefreshResponse = _ => FakeHttpMessageHandler.JsonResponse(new + { + access_token = "access-new", + refresh_token = "refresh-new", + expires_in = 3600, + }), + }; + var serverName = new McpServerName("notion"); + var entry = CreateHttpEntry(); + var service = CreateService( + CreateDiscoveryClient(), + new OAuthPkceService(new HttpClient(tokenHandler), time), + timeProvider: time); + + await SeedTokenAsync(service, serverName, entry, tokenHandler, + expiresInSeconds: 3600, refreshToken: "refresh-old"); + + var resourceHandler = new RejectStaleAccessTokenHandler(); + using var client = new HttpClient(new McpOAuthAuthorizationHandler( + serverName, entry, service, resourceHandler)); + const string body = "{\"jsonrpc\":\"2.0\",\"method\":\"tools/list\"}"; + + using var response = await client.PostAsync( + "https://mcp.example.com/messages", + new StringContent(body, Encoding.UTF8, "application/json"), + TestContext.Current.CancellationToken); + + response.EnsureSuccessStatusCode(); + Assert.Equal([body, body], resourceHandler.RequestBodies); + Assert.Equal(1, tokenHandler.RefreshCallCount); + } + + [Fact] + public async Task ProactiveTerminalRefresh_TearsDownLiveClientAndSetsAuthFailedStatus() + { + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + var ct = cts.Token; + var now = new DateTimeOffset(2026, 6, 23, 12, 0, 0, TimeSpan.Zero); + var time = new FakeTimeProvider(now); + var tokenHandler = new TokenEndpointHandler + { + RefreshResponse = _ => FakeHttpMessageHandler.JsonResponse( + new { error = "invalid_grant" }, HttpStatusCode.BadRequest), + }; + var serverName = new McpServerName("notion"); + + await using var server = await SmokeHttpMcpServer.StartAsync(ct, requireAuth: true); + var entry = new McpServerEntry + { + Transport = "http", + Url = server.Url, + Enabled = true, + OAuthClientId = "test-client", + }; + var service = CreateService( + CreateDiscoveryClient(server.Url), + new OAuthPkceService(new HttpClient(tokenHandler), time), + timeProvider: time); + await SeedTokenAsync(service, serverName, entry, tokenHandler, + expiresInSeconds: 3600, refreshToken: "refresh-old"); + + using var manager = new McpClientManager( + new Dictionary { [serverName.Value] = entry }, + new ToolRegistry(), + new ToolConfig(), + service, + NullNotificationSink.Instance, + time, + NullLogger.Instance); + + try + { + await manager.StartAsync(ct); + Assert.NotNull(manager.GetClient(serverName)); + Assert.Equal(McpConnectionState.Connected, + manager.GetServerStatuses()[serverName].State); + + time.Advance(TimeSpan.FromMinutes(51)); + await manager.RefreshOAuthTokensAsync(ct); + + Assert.Null(manager.GetClient(serverName)); + var status = manager.GetServerStatuses()[serverName]; + Assert.Equal(McpConnectionState.AuthFailed, status.State); + Assert.Contains("invalid_grant", status.ErrorMessage, StringComparison.Ordinal); + Assert.Equal(1, tokenHandler.RefreshCallCount); + } + finally + { + await manager.StopAsync(ct); + } + } + public void Dispose() { _dir.Dispose(); @@ -544,6 +689,51 @@ private sealed class RecordingNotificationSink : IOperationalNotificationSink public void Emit(OperationalAlert alert) => Alerts.Add(alert); } + private sealed class RejectStaleAccessTokenHandler : HttpMessageHandler + { + private readonly List _requestBodies = []; + private readonly Lock _requestBodiesLock = new(); + private int _rejectedRequestCount; + private int _acceptedRequestCount; + + public int RejectedRequestCount => Volatile.Read(ref _rejectedRequestCount); + public int AcceptedRequestCount => Volatile.Read(ref _acceptedRequestCount); + public IReadOnlyList RequestBodies + { + get + { + lock (_requestBodiesLock) + return _requestBodies.ToList(); + } + } + + protected override async Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) + { + if (request.Content is not null) + { + var body = await request.Content.ReadAsStringAsync(cancellationToken); + lock (_requestBodiesLock) + _requestBodies.Add(body); + } + + var token = request.Headers.Authorization?.Parameter; + if (token == "access-seed") + { + Interlocked.Increment(ref _rejectedRequestCount); + return new HttpResponseMessage(HttpStatusCode.Unauthorized); + } + + Assert.Equal("access-new", token); + Interlocked.Increment(ref _acceptedRequestCount); + return new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("ok"), + }; + } + } + private sealed class RecordingLogger : ILogger { public List<(LogLevel Level, string Message)> Entries { get; } = []; @@ -569,14 +759,19 @@ private static McpServerEntry CreateHttpEntry() } private static HttpClient CreateDiscoveryClient() + => CreateDiscoveryClient("https://mcp.example.com"); + + private static HttpClient CreateDiscoveryClient(string mcpServerUrl) { + var normalizedServerUrl = mcpServerUrl.TrimEnd('/'); + var serverOrigin = new Uri(normalizedServerUrl).GetLeftPart(UriPartial.Authority); return new HttpClient(new FakeHttpMessageHandler(request => request.RequestUri!.ToString() switch { - "https://mcp.example.com/" or "https://mcp.example.com" => new HttpResponseMessage(HttpStatusCode.Unauthorized), - "https://mcp.example.com/.well-known/oauth-protected-resource" => JsonResponse(new + var url when url.TrimEnd('/') == normalizedServerUrl => new HttpResponseMessage(HttpStatusCode.Unauthorized), + var url when url == $"{serverOrigin}/.well-known/oauth-protected-resource" => JsonResponse(new { authorization_servers = new[] { "https://auth.example.com" }, - resource = "https://mcp.example.com/resource" + resource = normalizedServerUrl, }), "https://auth.example.com/.well-known/oauth-authorization-server" => JsonResponse(new { diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpReconnectGateTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpReconnectGateTests.cs new file mode 100644 index 000000000..58b0e270d --- /dev/null +++ b/src/Netclaw.Daemon.Tests/Mcp/McpReconnectGateTests.cs @@ -0,0 +1,127 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Netclaw.Daemon.Mcp; +using Xunit; + +namespace Netclaw.Daemon.Tests.Mcp; + +public sealed class McpReconnectGateTests +{ + [Fact] + public async Task ConcurrentReconnects_ReuseFirstSuccessfulReplacement() + { + var gate = new McpReconnectGate(); + var reconnectEntered = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var releaseReconnect = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var hasLiveConnection = false; + var reconnectCount = 0; + var observedVersion = gate.CaptureVersion(); + + async Task Reconnect(CancellationToken ct) + { + Interlocked.Increment(ref reconnectCount); + reconnectEntered.TrySetResult(); + await releaseReconnect.Task.WaitAsync(ct); + hasLiveConnection = true; + gate.MarkConnectionChanged(); + return true; + } + + var reconnects = Enumerable.Range(0, 5) + .Select(_ => gate.ReconnectAsync( + observedVersion, + () => hasLiveConnection, + Reconnect, + TestContext.Current.CancellationToken)) + .ToArray(); + + await reconnectEntered.Task.WaitAsync( + TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + releaseReconnect.TrySetResult(); + + var results = await Task.WhenAll(reconnects); + + Assert.All(results, Assert.True); + Assert.Equal(1, Volatile.Read(ref reconnectCount)); + } + + [Fact] + public async Task FailedLeader_AllowsWaitingCallerToRetry() + { + var gate = new McpReconnectGate(); + var firstReconnectEntered = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var releaseFirstReconnect = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var hasLiveConnection = false; + var reconnectCount = 0; + var observedVersion = gate.CaptureVersion(); + + async Task Reconnect(CancellationToken ct) + { + var attempt = Interlocked.Increment(ref reconnectCount); + if (attempt == 1) + { + firstReconnectEntered.TrySetResult(); + await releaseFirstReconnect.Task.WaitAsync(ct); + return false; + } + + hasLiveConnection = true; + gate.MarkConnectionChanged(); + return true; + } + + var first = gate.ReconnectAsync( + observedVersion, () => hasLiveConnection, Reconnect, + TestContext.Current.CancellationToken); + await firstReconnectEntered.Task.WaitAsync( + TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + var second = gate.ReconnectAsync( + observedVersion, () => hasLiveConnection, Reconnect, + TestContext.Current.CancellationToken); + + releaseFirstReconnect.TrySetResult(); + + Assert.False(await first); + Assert.True(await second); + Assert.Equal(2, Volatile.Read(ref reconnectCount)); + } + + [Fact] + public async Task TearDownWait_ObservesCancellation() + { + var gate = new McpReconnectGate(); + var reconnectEntered = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var releaseReconnect = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + var reconnect = gate.ReconnectAsync( + gate.CaptureVersion(), + static () => false, + async ct => + { + reconnectEntered.TrySetResult(); + await releaseReconnect.Task.WaitAsync(ct); + return false; + }, + TestContext.Current.CancellationToken); + await reconnectEntered.Task.WaitAsync( + TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + + using var cts = new CancellationTokenSource(); + var tearDown = gate.TearDownAsync(static () => Task.CompletedTask, cts.Token); + cts.Cancel(); + + await Assert.ThrowsAnyAsync(() => tearDown); + + releaseReconnect.TrySetResult(); + Assert.False(await reconnect); + } +} diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpTokenCacheAdapterTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpTokenCacheAdapterTests.cs deleted file mode 100644 index 864639b35..000000000 --- a/src/Netclaw.Daemon.Tests/Mcp/McpTokenCacheAdapterTests.cs +++ /dev/null @@ -1,126 +0,0 @@ -// ----------------------------------------------------------------------- -// -// Copyright (C) 2026 - 2026 Petabridge, LLC -// -// ----------------------------------------------------------------------- -using System.Collections.Concurrent; -using ModelContextProtocol.Authentication; -using Netclaw.Configuration; -using Netclaw.Daemon.Mcp; -using Netclaw.Tools; -using Xunit; - -namespace Netclaw.Daemon.Tests.Mcp; - -public sealed class McpTokenCacheAdapterTests -{ - private readonly ConcurrentDictionary _tokens = new(); - private int _persistCallCount; - - private McpTokenCacheAdapter CreateAdapter(string serverName = "test-server") - { - return new McpTokenCacheAdapter( - new McpServerName(serverName), - _tokens, - () => Interlocked.Increment(ref _persistCallCount), - TimeProvider.System); - } - - [Fact] - public async Task StoreAndRetrieve_RoundTrips() - { - var adapter = CreateAdapter(); - - var stored = new TokenContainer - { - TokenType = "Bearer", - AccessToken = "access-123", - RefreshToken = "refresh-456", - ExpiresIn = 3600, - ObtainedAt = DateTimeOffset.UtcNow, - }; - - await adapter.StoreTokensAsync(stored, CancellationToken.None); - var retrieved = await adapter.GetTokensAsync(CancellationToken.None); - - Assert.NotNull(retrieved); - Assert.Equal("Bearer", retrieved.TokenType); - Assert.Equal("access-123", retrieved.AccessToken); - Assert.Equal("refresh-456", retrieved.RefreshToken); - Assert.NotNull(retrieved.ExpiresIn); - Assert.True(retrieved.ExpiresIn > 0); - } - - [Fact] - public async Task GetTokensAsync_WhenEmpty_ReturnsNull() - { - var adapter = CreateAdapter(); - - var result = await adapter.GetTokensAsync(CancellationToken.None); - - Assert.Null(result); - } - - [Fact] - public async Task StoreTokensAsync_CallsPersist() - { - var adapter = CreateAdapter(); - - await adapter.StoreTokensAsync(new TokenContainer - { - TokenType = "Bearer", - AccessToken = "tok", - ObtainedAt = DateTimeOffset.UtcNow, - }, CancellationToken.None); - - Assert.Equal(1, Volatile.Read(ref _persistCallCount)); - } - - [Fact] - public async Task StoreTokensAsync_PreservesExistingClientIdAndUrl() - { - _tokens[new McpServerName("test-server")] = new McpOAuthTokenSet - { - AccessToken = new SensitiveString("old-token"), - ClientId = "my-client-id", - McpServerUrl = "https://mcp.example.com", - }; - - var adapter = CreateAdapter(); - - await adapter.StoreTokensAsync(new TokenContainer - { - TokenType = "Bearer", - AccessToken = "new-token", - ObtainedAt = DateTimeOffset.UtcNow, - }, CancellationToken.None); - - var tokenSet = _tokens[new McpServerName("test-server")]; - Assert.Equal("new-token", tokenSet.AccessToken.Value); - Assert.Equal("my-client-id", tokenSet.ClientId); - Assert.Equal("https://mcp.example.com", tokenSet.McpServerUrl); - } - - [Fact] - public async Task StoreTokensAsync_PreservesExistingRefreshTokenWhenResponseOmitsIt() - { - _tokens[new McpServerName("test-server")] = new McpOAuthTokenSet - { - AccessToken = new SensitiveString("old-token"), - RefreshToken = new SensitiveString("existing-refresh"), - }; - - var adapter = CreateAdapter(); - - await adapter.StoreTokensAsync(new TokenContainer - { - TokenType = "Bearer", - AccessToken = "new-token", - ObtainedAt = DateTimeOffset.UtcNow, - }, CancellationToken.None); - - var tokenSet = _tokens[new McpServerName("test-server")]; - Assert.Equal("new-token", tokenSet.AccessToken.Value); - Assert.Equal("existing-refresh", tokenSet.RefreshToken?.Value); - } -} diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index 66fdebf70..72ae70ff3 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -7,7 +7,6 @@ using Microsoft.Extensions.AI; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; -using ModelContextProtocol.Authentication; using ModelContextProtocol.Client; using Netclaw.Actors.Tools; using Netclaw.Configuration; @@ -33,13 +32,7 @@ internal sealed class McpClientManager : IHostedService, IDisposable, IMcpToolIn private readonly ConcurrentDictionary _statuses = new(); - // Serializes reconnect/teardown per server. Concurrent tool-invocation - // failures against the same server (see InvokeSharedAsync) can each - // independently reach TryReconnectAsync; without this gate they would race - // to tear down and rebuild the same McpClient, leaking the loser's - // client/process. Also shared by the proactive-refresh sweep's teardown on - // a terminal invalid_grant. - private readonly ConcurrentDictionary _reconnectGates = new(); + private readonly ConcurrentDictionary _reconnectGates = new(); public McpClientManager( Dictionary serverEntries, @@ -120,24 +113,24 @@ public async Task TryReconnectAsync(McpServerName serverName, Cancellation if (!_serverEntries.TryGetValue(serverName.Value, out var entry) || !entry.Enabled) return false; - var gate = _reconnectGates.GetOrAdd(serverName, static _ => new SemaphoreSlim(1, 1)); - await gate.WaitAsync(ct); - try - { - if (_clients.TryRemove(serverName, out var existing)) + var gate = GetReconnectGate(serverName); + var observedVersion = gate.CaptureVersion(); + return await gate.ReconnectAsync( + observedVersion, + () => _clients.ContainsKey(serverName), + async cancellationToken => { - try { await existing.DisposeAsync(); } - catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client '{Name}' during reconnect", serverName.Value); } - } + if (_clients.TryRemove(serverName, out var existing)) + { + try { await existing.DisposeAsync(); } + catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client '{Name}' during reconnect", serverName.Value); } + } - _sharedToolFunctions.TryRemove(serverName, out _); + _sharedToolFunctions.TryRemove(serverName, out _); - return await ConnectAsync(serverName, entry, ct); - } - finally - { - gate.Release(); - } + return await ConnectAsync(serverName, entry, cancellationToken); + }, + ct); } /// @@ -160,7 +153,7 @@ public async Task RefreshOAuthTokensAsync(CancellationToken ct = default) if (!_serverEntries.TryGetValue(name.Value, out var entry) || !entry.Enabled) continue; - if (_oauthService.GetTokenSet(name) is null) + if (!UsesManagedOAuth(name, entry)) continue; // not an OAuth-managed server try @@ -179,7 +172,7 @@ public async Task RefreshOAuthTokensAsync(CancellationToken ct = default) // already logged and alerted by McpOAuthService). Tear the // connection down now rather than waiting for the next // tool call to 401 into a generic failure. - await TearDownConnectionAsync(name); + await TearDownConnectionAsync(name, ct); _statuses[name] = CreateRefreshRejectedStatus(name, errorCode); _logger.LogWarning( "MCP server '{Name}' OAuth refresh terminally rejected ({ErrorCode}); disconnected pending re-authorization", @@ -202,26 +195,25 @@ public async Task RefreshOAuthTokensAsync(CancellationToken ct = default) } } - private async Task TearDownConnectionAsync(McpServerName name) + private async Task TearDownConnectionAsync(McpServerName name, CancellationToken ct) { - var gate = _reconnectGates.GetOrAdd(name, static _ => new SemaphoreSlim(1, 1)); - await gate.WaitAsync(); - try - { - if (_clients.TryRemove(name, out var client)) + await GetReconnectGate(name).TearDownAsync( + async () => { - try { await client.DisposeAsync(); } - catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client '{Name}' after auth revocation", name.Value); } - } + if (_clients.TryRemove(name, out var client)) + { + try { await client.DisposeAsync(); } + catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client '{Name}' after auth revocation", name.Value); } + } - _sharedToolFunctions.TryRemove(name, out _); - } - finally - { - gate.Release(); - } + _sharedToolFunctions.TryRemove(name, out _); + }, + ct); } + private McpReconnectGate GetReconnectGate(McpServerName name) + => _reconnectGates.GetOrAdd(name, static _ => new McpReconnectGate()); + /// /// Advisory text for an otherwise-healthy Connected status: null unless the /// token set is time-limited but has no refresh token, in which case the @@ -338,12 +330,15 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, _sharedToolFunctions[name] = sharedFunctions; _clients[name] = client; + GetReconnectGate(name).MarkConnectionChanged(); client = null; // Advance warning for OAuth-managed servers whose token can never // self-refresh, surfaced both in the log and in the Connected // status text (doctor/status API) — see BuildOAuthAdvisory. - var tokenSet = _oauthService.GetTokenSet(name); + var tokenSet = UsesManagedOAuth(name, entry) + ? _oauthService.GetTokenSet(name) + : null; if (tokenSet is not null) _oauthService.WarnIfMissingRefreshToken(name); _statuses[name] = new McpServerStatus(name, McpConnectionState.Connected, tools.Count, BuildOAuthAdvisory(name, tokenSet)); @@ -393,7 +388,7 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, EmitDisconnectedAlert(name, $"MCP server '{name.Value}' authentication failed: {failureStatus.ErrorMessage}"); } } - else + else if (!HasConfiguredAuthorizationHeader(entry)) { _logger.LogWarning(ex, "Failed to connect to MCP server '{Name}'", name.Value); EmitDisconnectedAlert(name, $"MCP server '{name.Value}' connection failed: {failureStatus.ErrorMessage}"); @@ -459,10 +454,9 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, else { // Route the (re)connect through Netclaw's logged, single-flighted - // refresh path first, so the cached token the SDK's ITokenCache - // bridge (McpTokenCacheAdapter) sees is already fresh. The SDK's - // own on-401 refresh — silent, and headless-fatal on our stubbed - // AuthorizationRedirectDelegate — then almost never has to run. + // refresh path before constructing the HTTP transport. Runtime + // requests use McpOAuthAuthorizationHandler, so the SDK never + // receives or independently redeems the refresh token. await _oauthService.GetValidTokenAsync(name, entry, ct); if (_oauthService.GetTokenSet(name) is null @@ -519,9 +513,7 @@ private IClientTransport CreateTransport(McpServerName serverName, McpServerEntr var headers = entry.Headers.ToRawValues(StringComparer.OrdinalIgnoreCase) ?? new Dictionary(StringComparer.OrdinalIgnoreCase); - // Identify Netclaw to the remote MCP server. The SDK's HttpClientTransport - // builds its own HttpClient internally, so this header dictionary is the - // only seam — DelegatingHandlers can't reach it. User-configured headers + // Identify Netclaw to the remote MCP server. User-configured headers // win: if an operator already sets User-Agent or X-Netclaw-Component, // we leave them alone. if (!headers.ContainsKey("User-Agent")) @@ -529,7 +521,7 @@ private IClientTransport CreateTransport(McpServerName serverName, McpServerEntr if (!headers.ContainsKey(NetclawUserAgent.ComponentHeader)) headers[NetclawUserAgent.ComponentHeader] = "mcp"; - return new HttpClientTransport(new HttpClientTransportOptions + var options = new HttpClientTransportOptions { Endpoint = new Uri(entry.Url!), Name = serverName.Value, @@ -537,38 +529,24 @@ private IClientTransport CreateTransport(McpServerName serverName, McpServerEntr TransportMode = entry.Transport is "sse" ? HttpTransportMode.Sse : HttpTransportMode.AutoDetect, - OAuth = BuildOAuthOptions(serverName, entry), - }); - } - - private ClientOAuthOptions? BuildOAuthOptions(McpServerName serverName, McpServerEntry entry) - { - var metadata = _oauthService.GetCachedMetadata(serverName); - - // Only wire OAuth if server is known to need it (has metadata or static config) - if (metadata is null && string.IsNullOrWhiteSpace(entry.OAuthClientId)) - return null; + }; - var serverNameCapture = serverName; - return new ClientOAuthOptions + // Static Authorization headers are operator-owned and retain precedence. + // Otherwise a cached OAuth grant is applied by Netclaw's handler, which + // owns both proactive and reactive refresh under one per-server gate. + if (_oauthService.GetTokenSet(serverName) is not null + && !headers.ContainsKey("Authorization")) { - RedirectUri = new Uri("http://127.0.0.1:5199/api/mcp/oauth/callback"), - ClientId = metadata?.ClientId ?? entry.OAuthClientId, - Scopes = ParseScopes(entry.OAuthScope), - TokenCache = _oauthService.CreateTokenCache(serverName), - // Return null to suppress the SDK's default browser-open behavior; - // Netclaw handles interactive auth via `netclaw mcp auth`. - AuthorizationRedirectDelegate = static (_, _, _) => Task.FromResult(null), - DynamicClientRegistration = new DynamicClientRegistrationOptions - { - ClientName = "netclaw", - ResponseDelegate = (response, _) => - { - _oauthService.UpdateMetadataClientId(serverNameCapture, response.ClientId); - return Task.CompletedTask; - }, - }, - }; + var authorizationHandler = new McpOAuthAuthorizationHandler( + serverName, + entry, + _oauthService, + new HttpClientHandler()); + var httpClient = new HttpClient(authorizationHandler, disposeHandler: true); + return new HttpClientTransport(options, httpClient, ownsHttpClient: true); + } + + return new HttpClientTransport(options); } private bool HasOAuthRuntimeHints(McpServerName serverName, McpServerEntry entry) @@ -576,6 +554,14 @@ private bool HasOAuthRuntimeHints(McpServerName serverName, McpServerEntry entry || !string.IsNullOrWhiteSpace(entry.OAuthScope) || _oauthService.GetCachedMetadata(serverName) is not null; + private bool UsesManagedOAuth(McpServerName serverName, McpServerEntry entry) + => _oauthService.GetTokenSet(serverName) is not null + && !HasConfiguredAuthorizationHeader(entry); + + private static bool HasConfiguredAuthorizationHeader(McpServerEntry entry) + => entry.Headers?.Keys.Any( + static header => string.Equals(header, "Authorization", StringComparison.OrdinalIgnoreCase)) is true; + internal static McpServerStatus BuildConnectionFailureStatus( McpServerName serverName, McpServerEntry entry, @@ -659,14 +645,6 @@ private void EmitDisconnectedAlert(McpServerName serverName, string summary) context: new Dictionary { ["serverName"] = serverName.Value })); } - private static IEnumerable? ParseScopes(string? scopeString) - { - if (string.IsNullOrWhiteSpace(scopeString)) - return null; - - return scopeString.Split(' ', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); - } - private static bool IsAuthFailure(Exception ex) { // HttpRequestException with 401/403 diff --git a/src/Netclaw.Daemon/Mcp/McpOAuthAuthorizationHandler.cs b/src/Netclaw.Daemon/Mcp/McpOAuthAuthorizationHandler.cs new file mode 100644 index 000000000..df84ee633 --- /dev/null +++ b/src/Netclaw.Daemon/Mcp/McpOAuthAuthorizationHandler.cs @@ -0,0 +1,102 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Net; +using System.Net.Http.Headers; +using Netclaw.Configuration; +using Netclaw.Tools; + +namespace Netclaw.Daemon.Mcp; + +/// +/// Makes the sole runtime owner of MCP OAuth +/// refresh. The MCP SDK's OAuth provider cannot participate here because it +/// redeems cached refresh tokens outside Netclaw's per-server single-flight. +/// +internal sealed class McpOAuthAuthorizationHandler : DelegatingHandler +{ + private readonly McpServerName _serverName; + private readonly McpServerEntry _entry; + private readonly McpOAuthService _oauthService; + + public McpOAuthAuthorizationHandler( + McpServerName serverName, + McpServerEntry entry, + McpOAuthService oauthService, + HttpMessageHandler innerHandler) + : base(innerHandler) + { + _serverName = serverName; + _entry = entry; + _oauthService = oauthService; + } + + protected override async Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) + { + var accessToken = await _oauthService.GetValidTokenAsync( + _serverName, _entry, cancellationToken); + + // A transient proactive refresh failure retains the prior token. Send + // it once so a provider that still accepts it can keep serving traffic; + // any 401 below goes through the logged forced-refresh path. + accessToken ??= _oauthService.GetTokenSet(_serverName)?.AccessToken.Value; + + if (accessToken is not null) + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", accessToken); + + using var retryRequest = await CloneAsync(request, cancellationToken); + var response = await base.SendAsync(request, cancellationToken); + if (response.StatusCode is not HttpStatusCode.Unauthorized || accessToken is null) + return response; + + string? replacementToken; + try + { + replacementToken = await _oauthService.RefreshAfterUnauthorizedAsync( + _serverName, _entry, accessToken, cancellationToken); + } + catch + { + response.Dispose(); + throw; + } + if (replacementToken is null) + return response; + + retryRequest.Headers.Authorization = new AuthenticationHeaderValue("Bearer", replacementToken); + response.Dispose(); + return await base.SendAsync(retryRequest, cancellationToken); + } + + private static async Task CloneAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) + { + var clone = new HttpRequestMessage(request.Method, request.RequestUri) + { + Version = request.Version, + VersionPolicy = request.VersionPolicy, + }; + + foreach (var header in request.Headers) + clone.Headers.TryAddWithoutValidation(header.Key, header.Value); + + foreach (var option in request.Options) + clone.Options.Set(new HttpRequestOptionsKey(option.Key), option.Value); + + if (request.Content is not null) + { + var content = new ByteArrayContent( + await request.Content.ReadAsByteArrayAsync(cancellationToken)); + foreach (var header in request.Content.Headers) + content.Headers.TryAddWithoutValidation(header.Key, header.Value); + clone.Content = content; + } + + return clone; + } +} diff --git a/src/Netclaw.Daemon/Mcp/McpOAuthService.cs b/src/Netclaw.Daemon/Mcp/McpOAuthService.cs index 963f14ec4..8f8a56001 100644 --- a/src/Netclaw.Daemon/Mcp/McpOAuthService.cs +++ b/src/Netclaw.Daemon/Mcp/McpOAuthService.cs @@ -8,7 +8,6 @@ using System.Text.Json; using System.Text.Json.Serialization; using Microsoft.Extensions.Logging; -using ModelContextProtocol.Authentication; using Netclaw.Configuration; using Netclaw.Configuration.Secrets; using Netclaw.Providers.OAuth; @@ -422,6 +421,44 @@ internal bool TryGetTerminalRefreshRejection( } } + /// + /// Refreshes the access token after the MCP resource server rejected the + /// specific token carried by a request. Concurrent 401 responses for the + /// same token coalesce under the normal per-server refresh gate; callers + /// waiting behind the winner reuse its replacement token. + /// + public async Task RefreshAfterUnauthorizedAsync( + McpServerName serverName, + McpServerEntry entry, + string rejectedAccessToken, + CancellationToken ct) + { + var gate = _refreshGates.GetOrAdd(serverName, static _ => new SemaphoreSlim(1, 1)); + await gate.WaitAsync(ct); + try + { + if (!_tokens.TryGetValue(serverName, out var tokenSet)) + return null; + + // A concurrent request already redeemed the refresh token. Reuse + // its result instead of presenting the rotated credential again. + if (!string.Equals(tokenSet.AccessToken.Value, rejectedAccessToken, StringComparison.Ordinal)) + return tokenSet.AccessToken.Value; + + if (tokenSet.RefreshToken is null) + { + WarnNoRefreshToken(serverName, tokenSet.ExpiresAt); + return null; + } + + return await RefreshTokenAsync(serverName, entry, tokenSet, ct); + } + finally + { + gate.Release(); + } + } + private bool NeedsRefresh(McpOAuthTokenSet tokenSet) => tokenSet.ExpiresAt is not null && tokenSet.ExpiresAt.Value - ProactiveRefreshWindow <= _timeProvider.GetUtcNow(); @@ -582,6 +619,10 @@ private void WarnNoRefreshToken(McpServerName serverName, DateTimeOffset? expire return null; } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } catch (Exception ex) { // Transient failure: network error, 5xx, or a 4xx without a @@ -618,16 +659,6 @@ private void ClearRegisteredClient(McpServerName serverName) serverName.Value); } - // ── SDK Token Cache Bridge ────────────────────────────────────────── - - /// - /// Creates an adapter that bridges the SDK's - /// token management to Netclaw's existing - /// persistence for the given server. - /// - internal ITokenCache CreateTokenCache(McpServerName serverName) - => new McpTokenCacheAdapter(serverName, _tokens, PersistTokens, _timeProvider); - /// Returns the cached token set for the given server, or null. internal McpOAuthTokenSet? GetTokenSet(McpServerName serverName) => _tokens.TryGetValue(serverName, out var ts) ? ts : null; @@ -636,17 +667,6 @@ internal ITokenCache CreateTokenCache(McpServerName serverName) internal McpOAuthServerMetadata? GetCachedMetadata(McpServerName serverName) => _metadata.TryGetValue(serverName, out var m) ? m : null; - /// Persists a DCR-issued client_id into the metadata cache. - internal void UpdateMetadataClientId(McpServerName serverName, string clientId) - { - if (_metadata.TryGetValue(serverName, out var meta)) - { - meta.ClientId = clientId; - _metadata[serverName] = meta; - PersistMetadata(); - } - } - // ── Flow Status (for CLI polling) ────────────────────────────────── public McpOAuthFlowStatus GetFlowStatus(McpServerName serverName) diff --git a/src/Netclaw.Daemon/Mcp/McpReconnectGate.cs b/src/Netclaw.Daemon/Mcp/McpReconnectGate.cs new file mode 100644 index 000000000..a4c3a814a --- /dev/null +++ b/src/Netclaw.Daemon/Mcp/McpReconnectGate.cs @@ -0,0 +1,56 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- + +namespace Netclaw.Daemon.Mcp; + +/// +/// Serializes connection replacement and teardown for one MCP server while +/// allowing overlapping reconnect callers to reuse the first successful +/// replacement instead of tearing it down again. +/// +internal sealed class McpReconnectGate +{ + private readonly SemaphoreSlim _gate = new(1, 1); + private long _connectionVersion; + + public long CaptureVersion() => Interlocked.Read(ref _connectionVersion); + + public void MarkConnectionChanged() => Interlocked.Increment(ref _connectionVersion); + + public async Task ReconnectAsync( + long observedVersion, + Func hasLiveConnection, + Func> reconnect, + CancellationToken ct) + { + await _gate.WaitAsync(ct); + try + { + if (CaptureVersion() != observedVersion && hasLiveConnection()) + return true; + + return await reconnect(ct); + } + finally + { + _gate.Release(); + } + } + + public async Task TearDownAsync(Func tearDown, CancellationToken ct) + { + await _gate.WaitAsync(ct); + try + { + await tearDown(); + MarkConnectionChanged(); + } + finally + { + _gate.Release(); + } + } +} diff --git a/src/Netclaw.Daemon/Mcp/McpTokenCacheAdapter.cs b/src/Netclaw.Daemon/Mcp/McpTokenCacheAdapter.cs deleted file mode 100644 index 2949ff9e7..000000000 --- a/src/Netclaw.Daemon/Mcp/McpTokenCacheAdapter.cs +++ /dev/null @@ -1,89 +0,0 @@ -// ----------------------------------------------------------------------- -// -// Copyright (C) 2026 - 2026 Petabridge, LLC -// -// ----------------------------------------------------------------------- -using System.Collections.Concurrent; -using ModelContextProtocol.Authentication; -using Netclaw.Configuration; -using Netclaw.Tools; - -namespace Netclaw.Daemon.Mcp; - -/// -/// Bridges the MCP SDK's to Netclaw's existing -/// persistence. One instance per MCP server. -/// -internal sealed class McpTokenCacheAdapter : ITokenCache -{ - private readonly McpServerName _serverName; - private readonly ConcurrentDictionary _tokens; - private readonly Action _persistTokens; - private readonly TimeProvider _timeProvider; - - public McpTokenCacheAdapter( - McpServerName serverName, - ConcurrentDictionary tokens, - Action persistTokens, - TimeProvider timeProvider) - { - _serverName = serverName; - _tokens = tokens; - _persistTokens = persistTokens; - _timeProvider = timeProvider; - } - - public ValueTask StoreTokensAsync(TokenContainer tokens, CancellationToken cancellationToken) - { - var now = _timeProvider.GetUtcNow(); - var expiresAt = tokens.ExpiresIn is { } expiresIn - ? now.AddSeconds(expiresIn) - : (DateTimeOffset?)null; - - var tokenSet = new McpOAuthTokenSet - { - AccessToken = new SensitiveString(tokens.AccessToken), - RefreshToken = tokens.RefreshToken is not null - ? new SensitiveString(tokens.RefreshToken) - : null, - ExpiresAt = expiresAt, - }; - - // Preserve existing metadata and refresh token when the auth server omits it. - if (_tokens.TryGetValue(_serverName, out var existing)) - { - tokenSet.ClientId = existing.ClientId; - tokenSet.McpServerUrl = existing.McpServerUrl; - tokenSet.RefreshToken ??= existing.RefreshToken; - } - - _tokens[_serverName] = tokenSet; - _persistTokens(); - - return default; - } - - public ValueTask GetTokensAsync(CancellationToken cancellationToken) - { - if (!_tokens.TryGetValue(_serverName, out var tokenSet)) - return new ValueTask((TokenContainer?)null); - - var now = _timeProvider.GetUtcNow(); - var expiresIn = tokenSet.ExpiresAt is { } expiresAt - ? (int)Math.Max(0, (expiresAt - now).TotalSeconds) - : (int?)null; - - var container = new TokenContainer - { - TokenType = "Bearer", - AccessToken = tokenSet.AccessToken.Value, - RefreshToken = tokenSet.RefreshToken?.Value, - ExpiresIn = expiresIn, - ObtainedAt = tokenSet.ExpiresAt is { } ea && expiresIn is { } ei - ? ea.AddSeconds(-ei) - : now, - }; - - return new ValueTask(container); - } -}