Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,39 @@ public void BuildConnectionFailureStatus_ForNetworkFailure_ReturnsUnreachable()
Assert.Equal(ErrorAt, status.LastErrorAt);
}

[Fact]
public void BuildConnectionFailureStatus_ForStdioSpawnFailureWithEmbeddedStatusLikeDigits_ReturnsUnreachable()
{
var entry = new McpServerEntry
{
Transport = "stdio",
Command = "netclaw-missing-mcp-server-632401b4aa2f4c1e9c1b2a3d4e5f6789",
Enabled = true,
};

// A stdio process-spawn failure carries the command name inside its message. The
// command name is caller-supplied config data, not an HTTP signal, and can
// coincidentally embed digits that look like a status code -- here "401" inside
// the GUID suffix. No HTTP request ever occurs for stdio, so this must never be
// misread as an HTTP 401 failure.
var spawnFailure = new IOException(
$"An error occurred trying to start process '{entry.Command}' with working " +
"directory '/tmp'. No such file or directory");

var status = McpClientManager.BuildConnectionFailureStatus(
new McpServerName("notifications"),
entry,
spawnFailure,
hasCachedTokens: false,
hasOAuthRuntimeHints: false,
ErrorAt);

Assert.Equal(McpConnectionState.Unreachable, status.State);
Assert.Equal("Failed to reach MCP server. Check daemon logs for details.", status.ErrorMessage);
Assert.DoesNotContain("401", status.ErrorMessage, StringComparison.Ordinal);
Assert.Equal(ErrorAt, status.LastErrorAt);
}

[Fact]
public void PublicErrorsNeverIncludeProviderBodySecrets()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ public async Task RawListenAcknowledgementAndToolEvent_RefreshLiveCatalog()
[Fact]
public async Task FailedStdioStartup_IsReportedBeforeLeaseAssertions()
{
// The GUID suffix on this command name is intentional and must stay random: it
// regression-guards McpClientManager.FindHttpStatus against sniffing a status code
// out of caller-supplied data. A prior bug made a bare Contains("401")/Contains("403")
// check misclassify a random GUID substring (e.g. "632401b4...") as an HTTP auth
// failure. See McpClientManagerStatusTests for the targeted unit coverage.
var entry = new McpServerEntry
{
Transport = "stdio",
Expand Down
39 changes: 26 additions & 13 deletions src/Netclaw.Daemon/Mcp/McpClientManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1326,9 +1326,16 @@ internal static McpServerStatus BuildConnectionFailureStatus(
bool hasOAuthRuntimeHints,
DateTimeOffset errorAt)
{
// A stdio server is a local child process: no HTTP request is ever made for it, so
// no HTTP status can genuinely appear in its failure. The spawn-failure message
// routinely embeds caller-supplied data -- the command name, a working directory --
// that can coincidentally look like a status code (e.g. a GUID substring landing on
// "401"), so status sniffing is skipped outright for this transport.
var isStdioTransport = entry.Transport is "stdio";

if (IsAuthFailure(ex))
{
if (!hasCachedTokens && entry.Transport is not "stdio" && hasOAuthRuntimeHints)
if (!hasCachedTokens && !isStdioTransport && hasOAuthRuntimeHints)
{
// "Awaiting auth" -- which sends the operator to `netclaw mcp auth` -- is only
// correct for a genuine OAuth challenge: a Bearer WWW-Authenticate response (or
Expand All @@ -1340,7 +1347,7 @@ internal static McpServerStatus BuildConnectionFailureStatus(
// carrying the HTTP status instead.
return IsOAuthChallenge(ex)
? CreateAwaitingAuthStatus(serverName, errorAt)
: CreateUnreachableStatus(serverName, ex, errorAt);
: CreateUnreachableStatus(serverName, ex, errorAt, isStdioTransport);
}

return CreateAuthFailedStatus(
Expand All @@ -1350,7 +1357,7 @@ internal static McpServerStatus BuildConnectionFailureStatus(
errorAt);
}

return CreateUnreachableStatus(serverName, ex, errorAt);
return CreateUnreachableStatus(serverName, ex, errorAt, isStdioTransport);
}

internal static McpServerStatus CreateAwaitingAuthStatus(
Expand Down Expand Up @@ -1387,17 +1394,18 @@ internal static McpServerStatus CreateAuthFailedStatus(
internal static McpServerStatus CreateUnreachableStatus(
McpServerName serverName,
Exception ex,
DateTimeOffset errorAt)
DateTimeOffset errorAt,
bool isStdioTransport)
=> new(
serverName,
McpConnectionState.Unreachable,
0,
GetSafeConnectionFailure(ex),
GetSafeConnectionFailure(ex, isStdioTransport),
errorAt);

private static string GetSafeConnectionFailure(Exception ex)
private static string GetSafeConnectionFailure(Exception ex, bool isStdioTransport)
{
var status = FindHttpStatus(ex);
var status = isStdioTransport ? null : FindHttpStatus(ex);
if (status is not null)
return $"MCP server request failed (HTTP {(int)status.Value} {status.Value}).";
if (ex is TimeoutException or TaskCanceledException)
Expand Down Expand Up @@ -1625,6 +1633,17 @@ private static IEnumerable<Exception> EnumerateExceptionTree(Exception root)
}
}

/// <summary>
/// Reads an HTTP status from an exception chain. Only two anchored shapes are trusted: a
/// typed <see cref="HttpRequestException.StatusCode"/>, and the literal "status {Name}" /
/// "HTTP {code}" text the MCP SDK and .NET's own <c>HttpClient</c> use when they report one
/// (see the SDK's <c>HttpResponseMessageExtensions.CreateHttpRequestException</c> and
/// <see cref="McpOAuthClientRegistrar"/>, both of which set the typed status too). A bare
/// digit or word match (e.g. <c>Contains("401")</c>) is deliberately not used: exception
/// messages routinely embed caller-supplied data -- command names, file paths, GUIDs -- and
/// a coincidental "401"/"403" substring there would misreport an unrelated failure as an
/// HTTP auth rejection.
/// </summary>
private static HttpStatusCode? FindHttpStatus(Exception ex)
{
if (ex is HttpRequestException { StatusCode: { } status })
Expand All @@ -1635,12 +1654,6 @@ private static IEnumerable<Exception> EnumerateExceptionTree(Exception root)
|| ex.Message.Contains($"HTTP {(int)candidate}", StringComparison.OrdinalIgnoreCase))
return candidate;
}
if (ex.Message.Contains("403", StringComparison.Ordinal)
|| ex.Message.Contains("Forbidden", StringComparison.OrdinalIgnoreCase))
return HttpStatusCode.Forbidden;
if (ex.Message.Contains("401", StringComparison.Ordinal)
|| ex.Message.Contains("Unauthorized", StringComparison.OrdinalIgnoreCase))
return HttpStatusCode.Unauthorized;
return ex.InnerException is null ? null : FindHttpStatus(ex.InnerException);
}

Expand Down
Loading