Mark MCP servers AuthFailed on a typed 401 and keep result-text demotion OAuth-only (#2057) - #2062
Merged
Merged
Conversation
Aaronontheweb
added
bug
Aaronontheweb
force-pushed
the
fix/mcp-auth-guard-oauth-capable
branch
from
August 26, 2026 05:36
0611cc6 to
f879bcf
Compare
Aaronontheweb
force-pushed
the
fix/mcp-auth-guard-oauth-capable
branch
from
August 26, 2026 05:48
f879bcf to
5f01d4e
Compare
Aaronontheweb
marked this pull request as ready for review
August 26, 2026 05:48
Aaronontheweb
force-pushed
the
fix/mcp-auth-guard-oauth-capable
branch
from
August 26, 2026 08:39
5f01d4e to
86a68cb
Compare
Aaronontheweb
force-pushed
the
fix/mcp-auth-guard-oauth-capable
branch
from
August 26, 2026 14:34
86a68cb to
5ad4387
Compare
Aaronontheweb
changed the title
Aaronontheweb
force-pushed
the
fix/mcp-auth-guard-oauth-capable
branch
3 times, most recently
from
August 26, 2026 22:14
bf2d1f0 to
50b785c
Compare
Aaronontheweb
commented
Aug 26, 2026
Aaronontheweb
left a comment
Aaronontheweb
left a comment
Collaborator
Author
There was a problem hiding this comment.
Still looking but already found a big stinker
`ReportToolFailure` ran a substring test over the text of an `isError` tool result. A match moved the server to `AuthFailed`. Every later call then answered `Run: netclaw mcp auth <name>`. A tool that proxies a REST API answers "Forbidden" for an ordinary business error. A false demotion fires an `authentication_failed` alert. It makes `netclaw mcp list` and `netclaw doctor` report "auth failed" until the next invocation. That invocation then tears the healthy client down and reconnects for nothing. A stdio server and a static-header server cannot use `netclaw mcp auth`, so the remedy is wrong for them. `ReportToolFailure` now reads the server entry and requires `HasOAuthRuntimeHints`. Only an HTTP server without an operator-configured Authorization header can move to `AuthFailed`. The Warning line still records every tool failure. `IsAuthFailureMessage`, `MarkToolAuthFailure`, and the connect path do not change. Tests: `ManagerHarness` accepts a server entry. The two expired-token tests move to an HTTP entry without an Authorization header, which is the server kind they document. Two new tests prove that a static-header server and a stdio server each stay `Connected` after an `isError` result with auth words. Deviation: `InvocationAgainstAnAuthFailedServer_NamesTheRemedy` queued a bare 401 for the reconnect that follows. On an HTTP server with no cached token, a bare 401 means Unreachable, because #1908 requires a Bearer challenge for an auth verdict. The queued failure is now the `McpException` that the SDK raises for a Bearer challenge, which the repository already uses in two other tests. The reconnect then reports `AwaitingAuth`, so the test is now `InvocationAgainstAnAwaitingAuthServer_NamesTheRemedy` and it asserts that state. Its remedy assertions do not change.
Design D4 now has two signals on the tool-call path. An HTTP 401 on a tool call marks any HTTP server AuthFailed through the connect path's CreateAuthFailedStatus, so the remedy matches the auth scheme. Tool-declared error text still demotes only an OAuth-capable server. A 403 does not change the server state. The netclaw-mcp delta, the glossary, and the tasks list carry the same rule. This covers a static bearer that expires mid-session.
A static bearer that expires mid-session fails every tool call with HTTP 401. Nothing marked the server, so `netclaw mcp list` kept the report `Connected`. The tool-call path now carries two auth signals. A typed HTTP 401 moves any HTTP server to `AuthFailed`. Tool-declared error text moves an OAuth-capable server only. An HTTP 403 moves no server, because it denies one action and not the credential. A stdio server has no HTTP status, so the 401 signal skips it. `MarkToolAuthFailure` builds its status with the connect path's `CreateAuthFailedStatus`, so the remedy matches the auth scheme. The factory gets an overload that takes the HTTP status text. The alert kind mirrors `ReportConnectionFailure`. `CreateUnavailableException` reads the remedy from the published status message, so a static-header server names its header. Tests: a static-header 401 (`AuthFailed`, header remedy, reconnect on the next call), an OAuth-capable 401 (`netclaw mcp auth` remedy), and a static-header 403 (stays `Connected`).
Design D4 and the netclaw-mcp delta now state that a 401 on a catalog refresh follows the same scheme rule as a tool call, and that netclaw doctor picks its remediation text by the same test. The design example status string matches the code. The risks list records the per-call reconnect and alert cost while a header stays dead.
A catalog refresh marked any server `AwaitingAuth` on an auth failure. That state names `netclaw mcp auth`, and `StartAuthorizationAsync` refuses a server with a configured Authorization header. Such a server now gets the `AuthFailed` status that a tool-call 401 publishes, so the remedy names the credential the operator owns. An OAuth-capable server keeps the old path. A stdio server also keeps it, because it carries no HTTP status. `netclaw doctor` picked one remediation for every rejected credential. It now names `netclaw mcp auth` only for a server that can run the command, a credential check for any other server, and both when both kinds fail. The test mirrors the daemon rule: an HTTP server with no operator Authorization header. Tests: a static-header refresh 401 (`AuthFailed`, header remedy), a static-header `AuthFailed` in doctor (no `netclaw mcp auth`), and an adapter 401 (`AccessDenied`).
The daemon and the CLI each held a private copy of the rule. Two copies drift, and a drift makes `netclaw doctor` name a remedy the daemon did not publish. `McpServerEntry` now owns `IsOAuthCapable` and `HasConfiguredAuthorizationHeader`. The daemon call sites read the entry, and `HasOAuthRuntimeHints` and the private header test are gone. The doctor check reads the same member. Both properties carry `JsonIgnore`, because `netclaw mcp add` writes the serialized entry and the config schema rejects an unknown property. The glossary code anchor names the new member. Tests: the three scheme cases on the entry, and a guard that the computed properties stay out of the written config.
) A header named `Authorization` decided the remedy. A server that authenticates with `X-Api-Key` has no such header, so Netclaw told the operator to run `netclaw mcp auth`, and that command refuses the server. Netclaw now reads two facts: the failure, and whether it holds OAuth tokens for the server. A typed HTTP 401 or an OAuth challenge marks the server `AuthFailed`. Stored tokens or a challenge name `netclaw mcp auth`. Anything else names the credential the operator configured. An HTTP 403 changes no state. The result-text signal demotes a server only while tokens exist. The connect path follows the same rule. A bare 401 or 403 at `initialize` with no tokens is now `AuthFailed` with the credential remedy, not `Unreachable`. `netclaw doctor` points at each server's status line, which already carries the daemon's remedy. This commit also restores `McpServerEntry` to its `dev` shape. The two computed properties are gone. `HasOAuthRuntimeHints` survives for one job only: it decides whether the SDK OAuth handler owns the Authorization header.
… glossary (#2057) The glossary term becomes OAuth-managed server: the daemon holds OAuth tokens for the server, or the SDK reported a genuine challenge. Header names decide nothing. Design D4, the netclaw-mcp delta, the proposal, and the task list now state the rule the code implements, including the connect path, the catalog refresh, and doctor. An intermediate header-name rule was rejected in review.
Aaronontheweb
force-pushed
the
fix/mcp-auth-guard-oauth-capable
branch
from
August 27, 2026 21:56
3fa7d68 to
7a28cfe
Compare
Aaronontheweb
commented
Aug 27, 2026
| /// nothing to debug from. | ||
| /// Three signals move a server to <see cref="McpConnectionState.AuthFailed"/> on the | ||
| /// tool-call path: a typed HTTP 401, an OAuth challenge, and this result text. An HTTP | ||
| /// 403 moves no server, because it denies one action and not the credential. |
| } | ||
| else | ||
| { | ||
| EmitDisconnectedAlert( |
Collaborator
Author
There was a problem hiding this comment.
for scenarios where there's a 401 and no OAuth presence available, so something like a revoked manually encoded bearer token
| var error = await Assert.ThrowsAsync<InvalidOperationException>( | ||
| () => InvokeAsync(harness.Manager, TestContext.Current.CancellationToken)); | ||
|
|
||
| Assert.Contains("credentials or headers", error.Message, StringComparison.Ordinal); |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A tool-declared error that contained "Forbidden" moved a server to
AuthFailedwith an OAuth remedy it could not use. A static credential that expired mid-session failed every call with HTTP 401, but nothing marked the server. This change decides the remedy from the daemon's OAuth state and the failure shape, never from header names.Changes
initializemoves the server toAuthFailed. The remedy isnetclaw mcp authwhen the daemon holds OAuth tokens for the server or the SDK reported a Bearer challenge; otherwise it is "Check configured credentials or headers." A 403 on a tool call changes no state.Connected.initializewith no tokens now reportsAuthFailedwith the credentials remedy instead ofUnreachable.CreateUnavailableExceptionandnetclaw doctorread the remedy from the published status instead of a fixed string. The tool-path alert reason is nowcredentials_rejected.Authorizationheader, andnetclaw mcp authis refused when one is configured.Verification
Netclaw.Daemon.Tests1053,Netclaw.Cli.Tests1392,Netclaw.Configuration.Tests602 passed; 0 failed. Slopwatch and header checks pass.Closes #2057. Part of #2058.