fix(mcp): handle 400 auth errors, clear auth mode after OAuth, trim tokens by ilblackdragon · Pull Request #1158 · nearai/ironclaw

ilblackdragon · 2026-03-14T01:01:13Z

Summary

400 → auth-required: GitHub's MCP endpoint returns 400 "Authorization header is badly formatted" instead of 401 when auth is missing. Broadened auth detection in activate_mcp, send_request, and discover_via_401 to match 400 + authorization-related errors, triggering the OAuth flow instead of surfacing a raw error.
Clear auth mode after OAuth callback: oauth_callback_handler and extensions_setup_submit_handler now call clear_auth_mode() so the next user message reaches the LLM instead of being swallowed as a token.
Token trimming: Tokens with whitespace/newlines are trimmed before storage (configure) and before use in the Authorization header (build_request_headers), preventing malformed Bearer headers.

Test plan

cargo clippy --all --all-features — zero warnings
cargo test — all pass, including 2 new regression tests (test_build_headers_skips_empty_token, test_build_headers_trims_token)
E2E: pytest scenarios/test_mcp_auth_flow.py — 8 new tests covering install → activate → OAuth callback → LLM turn + GitHub-style 400 variant (mock MCP server added to mock_llm.py)
Manual: install GitHub MCP, verify OAuth prompt appears and tools load after auth

🤖 Generated with Claude Code

gemini-code-assist · 2026-03-14T01:01:17Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copilot

Pull request overview

Updates MCP authentication handling to better support real-world server behaviors (notably GitHub’s MCP returning auth-related 400s), prevents chat auth mode from persisting after OAuth completion, and hardens token handling to avoid malformed Authorization headers.

Changes:

Treat certain auth-related 400 responses similarly to 401 for MCP auth discovery/activation flows.
Clear pending “auth mode” after OAuth callback and after extension setup submission so subsequent user messages reach the LLM.
Trim/skip empty tokens when building/storing auth headers, plus add regression tests and E2E coverage with a mock MCP server.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/e2e/scenarios/test_mcp_auth_flow.py	New E2E scenario covering MCP install→activate→OAuth→tool registration, auth-mode clearing, and GitHub-style 400 behavior.
tests/e2e/mock_llm.py	Extends the mock server to simulate an OAuth-protected MCP server (including 401+WWW-Authenticate and a 400 “badly formatted auth header” variant).
src/tools/mcp/client.rs	Trims tokens before adding `Authorization` header; broadens auth-required detection; adds regression tests for header construction.
src/tools/mcp/auth.rs	Allows OAuth discovery via `WWW-Authenticate` on both 401 and 400 responses.
src/extensions/manager.rs	Broadens MCP activation auth-required detection (including auth-like 400s) and trims secret values before storage.
src/channels/web/server.rs	Clears pending auth mode after successful OAuth callback and after setup submission.

Comments suppressed due to low confidence (1)

src/tools/mcp/auth.rs:440

In discover_via_401, the status check now allows both 401 and 400, but the error message still says "Expected 401". This becomes misleading when the server returns other statuses. Update the message to reflect the accepted statuses (e.g., "Expected 401 or 400").

    // Accept 401 (standard) and 400 (some servers like GitHub MCP use this).
    // In both cases, look for WWW-Authenticate header with discovery metadata.
    if status != 401 && status != 400 {
        return Err(AuthError::DiscoveryFailed(format!(
            "Expected 401, got {}",
            response.status()
        )));

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ilblackdragon · 2026-03-14T01:17:50Z

+                    if msg.contains("401")
+                        || msg.contains("Unauthorized")
+                        || (msg.contains("400")
+                            && (msg.to_ascii_lowercase().contains("authorization")
+                                || msg.to_ascii_lowercase().contains("authenticate"))) =>


Fixed in 6395911 — now computes msg.to_ascii_lowercase() once via a block expression inside the match guard, avoiding the duplicate allocation.

ilblackdragon · 2026-03-14T01:17:57Z

+    message = data.get("message", "")
+    assert "400" not in message or "auth" in message.lower(), (
+        f"400 error should trigger auth flow, not raw error: {data}"


Fixed in 6395911 — now asserts auth_url is not None or awaiting_token (matching the 401 variant test) instead of the loose message string check.

zmanian

Review

The core fixes (400 detection, clear_auth_mode after OAuth, token trimming) are sound and well-motivated. Good unit test coverage for build_request_headers and the E2E mock MCP server is well-structured. Two issues need addressing before merge.

Blocking

1. 403 should not trigger auth re-flow

In manager.rs, the activate_mcp error mapping now treats 403 as AuthRequired:

msg.contains("403")

403 Forbidden means the server recognized credentials but denied access (insufficient scopes/permissions). Triggering OAuth re-auth on 403 creates a loop: user authenticates -> gets valid token -> server returns 403 (forbidden) -> code re-triggers auth. The PR description doesn't mention 403, and neither send_request retry logic nor discover_via_401 were updated for it. Please remove the || msg.contains("403") check.

2. discover_via_401 doc comment vs mock mismatch

The doc comment says "Some servers (e.g. GitHub MCP) may return 400 instead of 401 but still include a WWW-Authenticate header with discovery metadata." But the mcp_endpoint_400 mock does NOT include a WWW-Authenticate header on its 400 response -- it only returns plain text.

If GitHub really does include WWW-Authenticate on 400: update the mock to match.
If GitHub does NOT: fix the doc comment, since discover_via_401 will fail at header extraction returning DiscoveryFailed("No WWW-Authenticate header in 400 response").

Non-blocking

String matching on error messages is fragile (msg.contains("401"), msg_lower.contains("authorization")). Pre-existing pattern, but consider a structured error variant (e.g., ToolError::AuthRequired { status: u16 }) in a follow-up.
Duplicate clear_auth_mode in both server.rs and handlers/chat.rs -- pre-existing, but should be consolidated.
E2E assertion is loose -- "400" not in message or "auth" in message.lower() would pass on raw error surfaces. Consider asserting on auth_url is not None like the 401 variant test.
Token trimming applies to all secrets, not just tokens. The comment should note this.

Security

No token leakage in logs. Auth state race window between OAuth completion and clear_auth_mode is acceptably small. Token trimming is safe for OAuth tokens.

- Remove 403 from auth-required detection (403 = forbidden, not unauthenticated; re-triggering OAuth on 403 creates a loop) - Compute msg.to_ascii_lowercase() once in send_request retry guard - Fix discover_via_401 doc comment: GitHub doesn't send WWW-Authenticate on 400; discovery falls through to RFC 9728 strategy 2/3 - Update error message from "Expected 401" to "Expected 401 or 400" - Strengthen E2E 400 test assertion: check auth_url/awaiting_token fields instead of loose message string matching Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ilblackdragon · 2026-03-14T01:18:34Z

All review feedback addressed in 6395911:

Blocking items — fixed:

Removed 403 from auth-required detection — agreed, 403 = forbidden (valid creds, insufficient permissions) would cause an auth loop. Removed msg.contains("403") from activate_mcp.
Fixed discover_via_401 doc comment — updated to clarify that GitHub doesn't send WWW-Authenticate on 400; discovery falls through to RFC 9728 strategy 2 (protected resource metadata) or strategy 3 (direct).

Non-blocking items addressed:

Duplicate to_ascii_lowercase() — now computed once via block expression in the match guard.
Error message "Expected 401" — updated to "Expected 401 or 400".
Loose E2E assertion — now asserts auth_url is not None or awaiting_token (matching the 401 variant test).

Non-blocking items acknowledged for follow-up:

Structured error variant (ToolError::AuthRequired { status }) — agreed, string matching is fragile. Will address in a follow-up.
Duplicate clear_auth_mode across server.rs and handlers/chat.rs — pre-existing, consolidation deferred.
Token trimming comment — applies to all secrets stored via configure(), not just auth tokens; the trim is appropriate for all secret values.

Copilot

Pull request overview

Improves MCP OAuth/auth handling to better interoperate with GitHub’s MCP endpoint quirks, prevents malformed Bearer headers, and ensures chat auth mode doesn’t swallow post-OAuth user messages.

Changes:

Treat certain 400 auth-related failures as auth-required (alongside 401) to trigger OAuth instead of surfacing raw errors.
Clear per-thread “auth mode” after successful OAuth callback and after successful extensions setup submission.
Trim/skip empty tokens when storing secrets and when building the Authorization header; adds unit + E2E regression coverage and extends the E2E mock server.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`src/tools/mcp/client.rs`	Trims tokens before header injection; treats GitHub-style `400` auth errors as auth-required; adds regression tests for empty/whitespace tokens.
`src/tools/mcp/auth.rs`	Broadens 401-based OAuth discovery to accept 400 responses (still requiring `WWW-Authenticate`).
`src/extensions/manager.rs`	Maps GitHub-style 400 auth failures to `AuthRequired`; trims secret values before storing.
`src/channels/web/server.rs`	Clears auth mode after successful OAuth callback and after successful setup submit so the next user message reaches the LLM.
`tests/e2e/mock_llm.py`	Adds a mock MCP server (401 + WWW-Authenticate and a GitHub-style 400 variant) plus OAuth discovery endpoints.
`tests/e2e/scenarios/test_mcp_auth_flow.py`	Adds end-to-end coverage for MCP install → activate → OAuth callback → LLM turn, plus GitHub-style 400 regression.
`.gitignore`	Ignores Python bytecode artifacts from E2E runs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ilblackdragon · 2026-03-14T19:13:31Z

+    r = await api_post(
+        ironclaw_server,
+        "/api/extensions/mock-mcp-400/activate",
+        timeout=30,
+    )
+    data = r.json()
+


Fixed — added assert r.status_code == 200 before .json() in the 400-variant test.

zmanian

Re-review: Blocking items resolved, pending full CI

Both items from my previous review have been addressed:

403 removed from auth detection -- Fixed in commit 6395911. The msg.contains("403") check is gone.
Doc comment vs mock mismatch -- Fixed. Doc comment now correctly notes that 400 rarely yields WWW-Authenticate in practice (GitHub doesn't send it), so discovery falls through to strategy 2/3. Mock updated to match reality.

Also addressed from non-blocking notes:

E2E assertion now checks auth_url is not None or awaiting_token instead of the loose string match. Good.
Minor formatting cleanup in test code.

Blocking: CI hasn't run

Only classify and scope checks passed -- no Clippy, Formatting, or regression test enforcement. This is the fork-PR pattern where GitHub Actions secrets aren't available. Full CI must run before merge. Either push a maintainer commit to this branch or trigger a re-run from a trusted context.

Once CI is green, this is ready to merge.

Copilot

Pull request overview

Improves MCP OAuth usability and robustness by treating GitHub-style 400 Authorization header is badly formatted responses as auth-required, ensuring auth interception mode is cleared after OAuth/setup, and trimming tokens to avoid malformed Authorization: Bearer headers.

Changes:

Broadened MCP auth-required detection (401 + certain 400 variants) across activation/request/discovery paths.
Cleared pending_auth after OAuth callback and after extension setup submission to prevent swallowing the next user message.
Trimmed tokens before storage and before header injection; added regression tests and E2E coverage with a mock MCP server.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/e2e/scenarios/test_mcp_auth_flow.py	Adds E2E coverage for MCP install→activate→OAuth callback, auth-mode clearing, and 400-variant auth detection.
tests/e2e/mock_llm.py	Extends E2E mock server with MCP + OAuth discovery/token endpoints for MCP auth flow simulation.
src/tools/mcp/client.rs	Trims tokens before adding Authorization header; treats certain 400s as auth-required; adds regression tests for header building.
src/tools/mcp/auth.rs	Allows 400 responses in 401-challenge-based OAuth discovery to accommodate servers that return 400 for unauthenticated requests.
src/extensions/manager.rs	Treats certain 400 auth-related activation failures as `AuthRequired`; trims secret values before storage.
src/channels/web/server.rs	Clears auth mode after OAuth callback and after setup submit to ensure next user message goes to the LLM.

Comments suppressed due to low confidence (1)

src/channels/web/server.rs:2208

extensions_setup_submit_handler clears pending_auth only on successful configure(). If configure() fails (validation/storage error), the active thread can remain in auth interception mode and the next user message will still be consumed as a token. Consider calling clear_auth_mode(&state).await in the error branch as well (or move it outside the match so it always runs), and if you still want retries, re-emit an AuthRequired SSE event similar to chat_auth_token_handler's validation-failure path.

    match ext_mgr.configure(&name, &req.secrets).await {
        Ok(result) => {
            // Clear auth mode so the next user message goes through to the LLM
            // instead of being intercepted as a token.
            clear_auth_mode(&state).await;

            // Broadcast auth_completed so the chat UI can dismiss any in-progress
            // auth card or setup modal that was triggered by tool_auth/tool_activate.
            state.sse.broadcast(SseEvent::AuthCompleted {
                extension_name: name.clone(),
                success: true,
                message: result.message.clone(),
            });
            let mut resp = ActionResponse::ok(result.message);
            resp.activated = Some(result.activated);
            resp.auth_url = result.auth_url;
            Ok(Json(resp))
        }
        Err(e) => Ok(Json(ActionResponse::fail(e.to_string()))),
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ilblackdragon · 2026-03-15T03:29:59Z

+    app.router.add_get("/.well-known/oauth-protected-resource", mcp_protected_resource)
+    app.router.add_get("/.well-known/oauth-authorization-server", mcp_auth_server_metadata)


Good catch — fixed in c16ffeb. Added wildcard routes (/.well-known/oauth-protected-resource/{tail:.*} and /.well-known/oauth-authorization-server/{tail:.*}) so path-appended discovery URLs (e.g. /.well-known/oauth-protected-resource/mcp-400) are served correctly, matching production's build_well_known_uri behavior.

Copilot

Pull request overview

Improves MCP OAuth/auth handling across the gateway, MCP client, and extension activation so that GitHub-style 400 auth failures trigger the auth flow, tokens are sanitized before use/storage, and auth interception mode is cleared after OAuth/setup so chat messages reach the LLM.

Changes:

Treat 400 authorization-related failures as “auth required” (in MCP client retry/auth detection, OAuth discovery, and extension activation).
Trim tokens before adding Authorization: Bearer … headers and before storing secrets.
Add regression coverage: new Rust unit tests for header building + E2E mock MCP server endpoints and auth-flow scenarios.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/e2e/scenarios/test_mcp_auth_flow.py	Adds E2E coverage for MCP install → activate → OAuth callback → tools available + GitHub-style 400 variant and auth-mode clearing.
tests/e2e/mock_llm.py	Extends mock server with MCP JSON-RPC endpoints and OAuth discovery/DCR/token endpoints, including a 400 “badly formatted” variant.
src/tools/mcp/client.rs	Trims tokens before header injection; expands auth-required detection to include 400 auth-related errors; adds unit tests for header behavior.
src/tools/mcp/auth.rs	Allows discovery-via-challenge to accept 400 as well as 401 when attempting to read `WWW-Authenticate`.
src/extensions/manager.rs	Maps additional 400 auth-related failures to `AuthRequired` and trims secret values before storing.
src/channels/web/server.rs	Clears auth mode after OAuth callback and after successful extension setup submission.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    // Clear auth mode regardless of outcome so the next user message goes
+    // through to the LLM instead of being intercepted as a token.
+    clear_auth_mode(&state).await;


…okens Three bugs prevented MCP server authentication (e.g. GitHub MCP) from working correctly: 1. **400 treated as auth-required**: GitHub's MCP endpoint returns 400 "Authorization header is badly formatted" instead of 401 when auth is missing. Broadened auth detection in activate_mcp, send_request, and discover_via_401 to also match 400+authorization errors. 2. **Auth mode not cleared after OAuth callback**: The OAuth callback handler and setup submit handler did not call clear_auth_mode(), leaving pending_auth on the thread. The next user message was intercepted as a token instead of triggering an LLM turn. 3. **Token trimming**: Tokens with leading/trailing whitespace or newlines produced malformed Authorization headers. Now trimmed before storage (configure) and before use (build_request_headers). Adds E2E tests with a mock MCP server (JSON-RPC + OAuth discovery + DCR + token exchange) covering install -> activate -> OAuth callback -> LLM turn lifecycle, plus a GitHub-style 400 error variant. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…aths Auth mode (pending_auth on a Thread) had no timeout and several code paths that failed to clear it, causing user messages to be swallowed indefinitely. This adds defense-in-depth: - Add created_at + 5-minute TTL to PendingAuth; auto-clear on next message if expired (safety net for edge cases like user closing browser mid-OAuth) - Clear auth mode on OAuth callback failure paths (unknown/consumed state, expired flow) - Move clear_auth_mode before configure() match in setup_submit so it runs on failure too (addresses Copilot review feedback) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The pre-commit safety script only excluded files in tests/ but not #[cfg(test)] mod tests blocks inside src/ files. Use the git diff @@ hunk header context (which includes the enclosing function name) to detect and skip test hunks. Also removes unnecessary // safety: comments from test assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Improves MCP authentication ergonomics and resilience by broadening auth-required detection (including GitHub’s 400 variant), ensuring auth mode is cleared after OAuth flows, and preventing malformed Authorization headers by trimming/skipping empty tokens.

Changes:

Treat certain 400 responses as auth-required (alongside 401) in MCP client/auth discovery and extension activation paths.
Clear thread auth interception mode after OAuth callback and extension setup submission to avoid swallowing the next user message as a token.
Trim tokens before storage/use; add unit + E2E regression coverage and a mock MCP OAuth server in the E2E harness.
Add TTL expiry for pending_auth and enforce it in the agent loop.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/e2e/scenarios/test_mcp_auth_flow.py	Adds E2E coverage for MCP install→activate→OAuth→tools + GitHub-style 400 behavior + auth-mode clearing.
tests/e2e/mock_llm.py	Extends mock server with MCP JSON-RPC endpoints and OAuth discovery/token/DCR endpoints (including RFC 9728 well-known paths).
src/tools/mcp/client.rs	Trims/skips empty Bearer tokens; treats certain 400s like auth-required; adds regression unit tests for header building.
src/tools/mcp/auth.rs	Allows 400 in 401-challenge discovery strategy and improves related error messaging.
src/extensions/manager.rs	Detects auth-required for GitHub-style 400s; trims secret values before storing.
src/channels/web/server.rs	Clears auth mode in more OAuth/setup handler paths to prevent subsequent user messages being intercepted.
src/agent/session.rs	Adds `pending_auth` TTL tracking (`created_at`) + serialization coverage.
src/agent/agent_loop.rs	Enforces `pending_auth` TTL expiration and clears stale auth mode.
scripts/pre-commit-safety.sh	Attempts to exclude test hunks from panic-call scanning using @@ function/module context.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ilblackdragon · 2026-03-15T04:57:02Z

 PROD_DIFF=$(echo "$PROD_DIFF" | grep -v '^+++ b/tests/' || true)
+# Strip hunks whose @@ context line indicates a test function or module
+# (git diff -U0 includes the enclosing function name after @@).
+PROD_DIFF=$(echo "$PROD_DIFF" | awk '
+    /^@@ / { in_test = ($0 ~ /fn test_/ || $0 ~ /mod tests/) }


Fixed in 808bdf4 — tightened to only match mod tests in hunk context, not fn test_*.

ilblackdragon · 2026-03-15T04:57:16Z


        thread.enter_auth_mode("telegram".to_string());
-        assert!(thread.pending_auth.is_some());
-        assert_eq!(
-            thread.pending_auth.as_ref().unwrap().extension_name,
-            "telegram"
-        );
-    }
+        assert!(thread.pending_auth.is_some());        let pending = thread.pending_auth.as_ref().unwrap();        assert_eq!(pending.extension_name, "telegram");        assert!(pending.created_at >= before);        assert!(!pending.is_expired());    }



Already fixed in 831f987 — formatting was restored to multi-line before this review ran.

ilblackdragon · 2026-03-15T04:57:16Z

+/// Auth mode TTL — matches `OAUTH_FLOW_EXPIRY` (5 minutes).
+const AUTH_MODE_TTL: TimeDelta = TimeDelta::minutes(5);
+


Fixed in 808bdf4 — extracted AUTH_MODE_TTL_SECS = 300 with a doc comment linking to crate::cli::oauth_defaults::OAUTH_FLOW_EXPIRY. Kept as a separate constant to avoid a session→cli module dependency.

ilblackdragon · 2026-03-15T04:57:16Z

+        // Backdate beyond the TTL
+        pending.created_at = Utc::now() - AUTH_MODE_TTL - TimeDelta::seconds(1);
+        assert!(pending.is_expired());    }



Already fixed in 831f987.

The replace_all edit that removed // safety: comments collapsed newlines. Restore proper line breaks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

… sync - pre-commit-safety.sh: only exclude `mod tests` hunks (not `fn test_*`) to avoid hiding unwrap/assert in production functions like test_server() - session.rs: extract AUTH_MODE_TTL_SECS constant and add doc comment linking to OAUTH_FLOW_EXPIRY to prevent silent drift [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR improves MCP server authentication robustness across the MCP client, extension activation/configure flows, and the web OAuth callback path—particularly to handle GitHub-style 400 auth failures, prevent malformed Authorization headers, and ensure auth interception mode is cleared so normal chat turns resume.

Changes:

Broaden “auth-required” detection to include certain 400 authorization-related failures and improve OAuth discovery handling.
Clear web session auth-interception mode after OAuth callback / setup submit, and trim tokens before storage and header injection.
Add regression coverage (unit tests + E2E mock MCP server + E2E auth-flow scenario tests).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/e2e/scenarios/test_mcp_auth_flow.py	Adds end-to-end tests for MCP install → activate → OAuth callback → tools available, including GitHub-style 400 behavior.
tests/e2e/mock_llm.py	Extends mock LLM server with a mock MCP server + OAuth discovery/DCR/token endpoints for E2E coverage.
src/tools/mcp/client.rs	Trims/skips empty tokens when building headers; expands auth-required detection on request failures; adds regression unit tests.
src/tools/mcp/auth.rs	Allows `discover_via_401` to accept 400 responses (for servers that don’t use 401 for missing auth).
src/extensions/manager.rs	Treats certain 400 auth failures as `AuthRequired`; trims secret values before storing.
src/channels/web/server.rs	Clears auth mode after OAuth callback and extensions setup submit to prevent subsequent messages being intercepted as tokens.
src/agent/session.rs	Adds TTL-aware `PendingAuth` metadata (`created_at`) and expiry helper.
src/agent/agent_loop.rs	Clears expired `pending_auth` on message handling to avoid stale auth interception.
scripts/pre-commit-safety.sh	Filters `mod tests` hunks out of the “panic in production code” pre-commit check.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ilblackdragon · 2026-03-15T05:06:21Z

        if let Some(pending) = pending_auth {
-            match &submission {
-                Submission::UserInput { content } => {
-                    return self
-                        .process_auth_token(message, &pending, content, session, thread_id)
-                        .await;
+            if pending.is_expired() {
+                // TTL exceeded — clear stale auth and fall through to normal handling
+                tracing::warn!(
+                    extension = %pending.extension_name,
+                    "Auth mode expired after TTL, clearing"
+                );
+                let mut sess = session.lock().await;
+                if let Some(thread) = sess.threads.get_mut(&thread_id) {
+                    thread.pending_auth = None;
                }


Fixed in aff6b81 — when auth mode is expired and the submission is UserInput, we now return an explicit "Authentication for X expired. Please try again." response instead of forwarding the content to the LLM/history. Control submissions still fall through to normal handling.

ilblackdragon · 2026-03-15T05:06:21Z

+    // Clear auth mode regardless of outcome so the next user message goes
+    // through to the LLM instead of being intercepted as a token.
+    clear_auth_mode(&state).await;


Fixed in aff6b81 — added clear_auth_mode() to all 4 remaining early-return paths (provider error, missing state, missing code, no extension manager). Every exit path in oauth_callback_handler now clears pending auth.

… paths - When auth mode TTL expires and the user sends a message (possibly a pasted token), return an explicit "expired, please retry" response instead of forwarding the content to the LLM/history - Add clear_auth_mode() to all early-return paths in oauth_callback_handler (provider error, missing state/code, no extension manager) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

zmanian

Re-review: All items resolved, CI fully green, new improvements are sound

The PR was reworked with a force-push (6 commits vs previous 3). All original blocking items remain resolved, plus meaningful new improvements.

Original items (still resolved)

403 removed from auth detection -- confirmed still absent in the new diff
Doc comment vs mock mismatch -- doc correctly notes 400 rarely yields WWW-Authenticate; mock matches

New improvements since last review

TTL on PendingAuth (5 minutes) -- defense-in-depth against stuck auth mode. If user closes browser mid-OAuth, auth mode auto-clears on next message. This is important -- without it, a failed OAuth flow leaves the thread permanently stuck in auth mode swallowing all user messages.
Clear auth mode on ALL failure paths -- OAuth callback handler now clears auth on unknown/consumed state and expired flow, not just on success. The setup_submit_handler clears before configure() so it runs on failure too.
Expired auth returns error to user -- when TTL exceeds and user sends a message, they get a clear error ("auth session expired, please try again") instead of the message being silently processed without auth context.
E2E test expanded -- mock MCP server now has wildcard .well-known routes for path-suffixed discovery (e.g., /.well-known/oauth-protected-resource/mcp-400). Tests cover the full OAuth lifecycle for both 401 and 400 variants, plus cleanup.

CI

Full CI is now green -- all Clippy variants, Formatting, E2E (core + extensions + features), regression test enforcement.

Title check

Accurate: "fix(mcp): handle 400 auth errors, clear auth mode after OAuth, trim tokens" -- covers the three core bugs. The TTL addition is a natural extension of "clear auth mode" and doesn't warrant a title change.

LGTM.

…okens (nearai#1158) * fix(mcp): handle 400 auth errors, clear auth mode after OAuth, trim tokens Three bugs prevented MCP server authentication (e.g. GitHub MCP) from working correctly: 1. **400 treated as auth-required**: GitHub's MCP endpoint returns 400 "Authorization header is badly formatted" instead of 401 when auth is missing. Broadened auth detection in activate_mcp, send_request, and discover_via_401 to also match 400+authorization errors. 2. **Auth mode not cleared after OAuth callback**: The OAuth callback handler and setup submit handler did not call clear_auth_mode(), leaving pending_auth on the thread. The next user message was intercepted as a token instead of triggering an LLM turn. 3. **Token trimming**: Tokens with leading/trailing whitespace or newlines produced malformed Authorization headers. Now trimmed before storage (configure) and before use (build_request_headers). Adds E2E tests with a mock MCP server (JSON-RPC + OAuth discovery + DCR + token exchange) covering install -> activate -> OAuth callback -> LLM turn lifecycle, plus a GitHub-style 400 error variant. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(mcp): add TTL to PendingAuth and clear auth mode on all failure paths Auth mode (pending_auth on a Thread) had no timeout and several code paths that failed to clear it, causing user messages to be swallowed indefinitely. This adds defense-in-depth: - Add created_at + 5-minute TTL to PendingAuth; auto-clear on next message if expired (safety net for edge cases like user closing browser mid-OAuth) - Clear auth mode on OAuth callback failure paths (unknown/consumed state, expired flow) - Move clear_auth_mode before configure() match in setup_submit so it runs on failure too (addresses Copilot review feedback) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ci): exclude test hunks from unwrap/assert pre-commit check The pre-commit safety script only excluded files in tests/ but not #[cfg(test)] mod tests blocks inside src/ files. Use the git diff @@ hunk header context (which includes the enclosing function name) to detect and skip test hunks. Also removes unnecessary // safety: comments from test assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: restore formatting in test assertions The replace_all edit that removed // safety: comments collapsed newlines. Restore proper line breaks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address Copilot review - tighten pre-commit filter, document TTL sync - pre-commit-safety.sh: only exclude `mod tests` hunks (not `fn test_*`) to avoid hiding unwrap/assert in production functions like test_server() - session.rs: extract AUTH_MODE_TTL_SECS constant and add doc comment linking to OAUTH_FLOW_EXPIRY to prevent silent drift [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(mcp): return error on expired auth input, clear auth on all OAuth paths - When auth mode TTL expires and the user sends a message (possibly a pasted token), return an explicit "expired, please retry" response instead of forwarding the content to the LLM/history - Add clear_auth_mode() to all early-return paths in oauth_callback_handler (provider error, missing state/code, no extension manager) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 14, 2026 01:01

Copilot started reviewing on behalf of ilblackdragon March 14, 2026 01:01 View session

Copilot AI reviewed Mar 14, 2026

View reviewed changes

zmanian requested changes Mar 14, 2026

View reviewed changes

github-actions Bot added the size: XL 500+ changed lines label Mar 14, 2026

Copilot AI review requested due to automatic review settings March 14, 2026 01:17

Copilot started reviewing on behalf of ilblackdragon March 14, 2026 01:18 View session

Copilot AI reviewed Mar 14, 2026

View reviewed changes

github-actions Bot mentioned this pull request Mar 14, 2026

🦞 Bản tin hàng ngày hệ sinh thái OpenClaw 2026-03-14 compasify/agents-radar#41

Open

zmanian reviewed Mar 14, 2026

View reviewed changes

zmanian closed this Mar 14, 2026

zmanian reopened this Mar 14, 2026

Copilot AI review requested due to automatic review settings March 14, 2026 19:13

Copilot started reviewing on behalf of ilblackdragon March 15, 2026 03:23 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

ilblackdragon force-pushed the fix/mcp-auth-flow branch from dbef86b to c16ffeb Compare March 15, 2026 03:29

Copilot AI review requested due to automatic review settings March 15, 2026 03:31

ilblackdragon force-pushed the fix/mcp-auth-flow branch from c16ffeb to 8b89176 Compare March 15, 2026 03:31

Copilot started reviewing on behalf of ilblackdragon March 15, 2026 03:32 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

Comment thread src/channels/web/server.rs

Comment on lines +693 to +695

// Clear auth mode regardless of outcome so the next user message goes

// through to the LLM instead of being intercepted as a token.

clear_auth_mode(&state).await;

ilblackdragon and others added 3 commits March 14, 2026 21:36

Copilot AI review requested due to automatic review settings March 15, 2026 04:40

ilblackdragon force-pushed the fix/mcp-auth-flow branch from 273205c to c20e987 Compare March 15, 2026 04:40

Copilot started reviewing on behalf of ilblackdragon March 15, 2026 04:41 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

ilblackdragon and others added 2 commits March 14, 2026 21:46

fix: restore formatting in test assertions

831f987

The replace_all edit that removed // safety: comments collapsed newlines. Restore proper line breaks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 15, 2026 04:56

Copilot started reviewing on behalf of ilblackdragon March 15, 2026 04:56 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

ilblackdragon requested a review from zmanian March 15, 2026 05:39

zmanian approved these changes Mar 15, 2026

View reviewed changes

ilblackdragon merged commit 62d16e6 into staging Mar 15, 2026
19 checks passed

ilblackdragon deleted the fix/mcp-auth-flow branch March 15, 2026 05:42

		app.router.add_get("/.well-known/oauth-protected-resource", mcp_protected_resource)
		app.router.add_get("/.well-known/oauth-authorization-server", mcp_auth_server_metadata)

		/// Auth mode TTL — matches `OAUTH_FLOW_EXPIRY` (5 minutes).
		const AUTH_MODE_TTL: TimeDelta = TimeDelta::minutes(5);

Conversation

ilblackdragon commented Mar 14, 2026

Summary

Test plan

Uh oh!

gemini-code-assist Bot commented Mar 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmanian left a comment

Choose a reason for hiding this comment

Review

Blocking

Non-blocking

Security

Uh oh!

ilblackdragon commented Mar 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmanian left a comment

Choose a reason for hiding this comment

Re-review: Blocking items resolved, pending full CI

Blocking: CI hasn't run

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmanian left a comment

Choose a reason for hiding this comment

Re-review: All items resolved, CI fully green, new improvements are sound

Original items (still resolved)

New improvements since last review

CI

Title check

Uh oh!

Uh oh!