diff --git a/CHANGELOG.md b/CHANGELOG.md index 82c18e1..ce080be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `tools-call --task` now prints the task ID and recovery commands when interrupted with Ctrl+C, so you can fetch or cancel the server-side task later - `tasks-get` no longer suggests `tasks-cancel` when the task has already reached a terminal state (completed, failed, or cancelled) +### Fixed + +- Sessions using a static bearer token (via `--header "Authorization: ..."`) no longer flip between `unauthorized` and `connecting` on every `mcpc` invocation — they stay `unauthorized` since retrying the same rejected token cannot succeed without `mcpc login` or reconnecting. OAuth-profile sessions still auto-retry because tokens may have been refreshed by another session +- Server authentication errors now include the path to the bridge log file, so you can inspect it for more detail when investigating why a session was rejected + ### Removed - Removed `tools`, `resources`, and `prompts` shorthand commands — use the full names (`tools-list`, `resources-list`, `prompts-list`) instead diff --git a/src/cli/commands/sessions.ts b/src/cli/commands/sessions.ts index e3e52ad..142dbec 100644 --- a/src/cli/commands/sessions.ts +++ b/src/cli/commands/sessions.ts @@ -11,6 +11,7 @@ import { validateProfileName, isProcessAlive, getServerHost, + getLogsDir, redactHeaders, } from '../../lib/index.js'; import { DISCONNECTED_THRESHOLD_MS } from '../../lib/types.js'; @@ -457,7 +458,8 @@ export async function connectSession( // Fallback: check error message for auth patterns (error may have been wrapped // as ClientError/ServerError during bridge IPC serialization) if (detailsError instanceof Error && isAuthenticationError(detailsError.message)) { - throw createServerAuthError(serverConfig.url || target, { sessionName: name }); + const logPath = `${getLogsDir()}/bridge-${name}.log`; + throw createServerAuthError(serverConfig.url || target, { sessionName: name, logPath }); } // Non-auth failure: session was created but server didn't respond properly. diff --git a/src/lib/bridge-manager.ts b/src/lib/bridge-manager.ts index af79443..f54c245 100644 --- a/src/lib/bridge-manager.ts +++ b/src/lib/bridge-manager.ts @@ -72,8 +72,10 @@ async function classifyAndThrowSessionError( logger.warn(`Failed to mark session ${sessionName} as unauthorized:`, e) ); const target = session.server.url || session.server.command || sessionName; + const logPath = `${getLogsDir()}/bridge-${sessionName}.log`; throw createServerAuthError(target, { sessionName, + logPath, ...(originalError && { originalError }), }); } @@ -598,7 +600,8 @@ export async function ensureBridgeReady(sessionName: string): Promise { if (session.status === 'unauthorized') { const target = session.server.url || session.server.command || sessionName; - throw createServerAuthError(target, { sessionName }); + const logPath = `${getLogsDir()}/bridge-${sessionName}.log`; + throw createServerAuthError(target, { sessionName, logPath }); } if (session.status === 'expired') { diff --git a/src/lib/errors.ts b/src/lib/errors.ts index ff2448b..38144ea 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -97,12 +97,11 @@ export function isAuthenticationError(errorMessage: string): boolean { * Create an AuthError with helpful login guidance for server auth failures * * @param target - Server URL or target for login command - * @param options - Optional session name for session-specific guidance - * @param originalError - Original error for debugging + * @param options - Optional session name for session-specific guidance and log path for debugging */ export function createServerAuthError( target: string, - options?: { sessionName?: string; originalError?: Error } + options?: { sessionName?: string; logPath?: string; originalError?: Error } ): AuthError { let hint: string; if (options?.sessionName) { @@ -117,6 +116,10 @@ export function createServerAuthError( `To authenticate, run:\n` + ` mcpc login ${target}\n\n` + `Then run your command again.`; } + if (options?.logPath) { + hint += `\n\nFor details, check logs at ${options.logPath}`; + } + return new AuthError( `Authentication required by server.\n\n` + hint, options?.originalError ? { originalError: options.originalError } : undefined diff --git a/src/lib/sessions.ts b/src/lib/sessions.ts index 34b9bdb..23aa48c 100644 --- a/src/lib/sessions.ts +++ b/src/lib/sessions.ts @@ -381,10 +381,15 @@ export async function consolidateSessions( } // Identify crashed or unauthorized sessions eligible for automatic reconnection. - // Unauthorized sessions are included because another session sharing the same OAuth - // profile may have refreshed the tokens — the bridge reads from keychain on startup. + // Unauthorized sessions are only included when the session has an OAuth profile — + // another session sharing the same profile may have refreshed the tokens, so a + // retry can succeed as the bridge re-reads from keychain on startup. Sessions + // without a profile (e.g. static bearer token via --header) cannot self-heal, so + // retrying would just flip the status back to 'connecting' on every `mcpc` call + // and hide the real state from the user. for (const [name, session] of Object.entries(storage.sessions)) { - if ((session?.status === 'crashed' || session?.status === 'unauthorized') && !session.pid) { + const isRetryableUnauthorized = session?.status === 'unauthorized' && !!session.profileName; + if ((session?.status === 'crashed' || isRetryableUnauthorized) && !session.pid) { // Skip if a connection was already attempted within the cooldown window const lastAttempt = session.lastConnectionAttemptAt ? new Date(session.lastConnectionAttemptAt).getTime() diff --git a/test/e2e/suites/sessions/unauthorized-persist.test.sh b/test/e2e/suites/sessions/unauthorized-persist.test.sh new file mode 100755 index 0000000..c9eaa8a --- /dev/null +++ b/test/e2e/suites/sessions/unauthorized-persist.test.sh @@ -0,0 +1,98 @@ +#!/bin/bash +# Test: Sessions that fail with 401 and have no OAuth profile stay 'unauthorized' +# across `mcpc` invocations, even after the auto-retry cooldown has elapsed. +# Also verifies the auth error message points at the bridge log file. + +source "$(dirname "$0")/../../lib/framework.sh" +test_init "sessions/unauthorized-persist" --isolated + +# Start test server requiring auth. The server's auth check accepts any +# "Bearer " value, so sending an Authorization header that doesn't +# match that shape is enough to trigger 401. +start_test_server REQUIRE_AUTH=true + +SESSION=$(session_name "unauth") +_SESSIONS_CREATED+=("$SESSION") + +# ============================================================================= +# Test: bearer-only session fails with auth error containing log path +# ============================================================================= + +test_case "connect with bad bearer token fails with auth error + log path" +run_mcpc connect "$TEST_SERVER_URL" "$SESSION" \ + --header "X-Test: true" \ + --header "Authorization: InvalidScheme not-a-bearer-token" +assert_failure +assert_exit_code 4 "should fail with auth exit code (4)" +assert_contains "$STDERR" "Authentication required by server" +# The error should point at the bridge log file so the user can investigate +assert_contains "$STDERR" "check logs at" +assert_contains "$STDERR" "bridge-${SESSION}.log" +test_pass + +# ============================================================================= +# Test: session is marked unauthorized right after the failed connect +# ============================================================================= + +test_case "failed bearer session reports 'unauthorized' status" +run_mcpc --json +assert_success +session_status=$(echo "$STDOUT" | jq -r ".sessions[] | select(.name == \"$SESSION\") | .status") +if [[ "$session_status" != "unauthorized" ]]; then + test_fail "expected status 'unauthorized', got: '$session_status'" + exit 1 +fi +test_pass + +# ============================================================================= +# Test: unauthorized bearer session does NOT flip back to 'connecting' +# after the auto-retry cooldown expires. +# +# Before the fix, consolidateSessions() treated all unauthorized sessions as +# retry candidates and reset the status to 'connecting' on every `mcpc` call +# (after the 10s cooldown), which both hid the real state from the user and +# triggered pointless background bridge restarts. Bearer-only sessions cannot +# self-heal because the token never changes. +# ============================================================================= + +test_case "unauthorized bearer session stays unauthorized past the cooldown" +sessions_file="$MCPC_HOME_DIR/sessions.json" +assert_file_exists "$sessions_file" + +# Age the lastConnectionAttemptAt timestamp well past the 10s cooldown and +# clear the bridge pid to simulate the bridge having exited — that's the real +# state consolidateSessions() sees when the user runs `mcpc` well after the +# original failed connect. Leaving pid set would short-circuit the retry path +# via `!session.pid`, hiding the bug we are trying to catch. +old_iso="2000-01-01T00:00:00.000Z" +tmp_file="$TEST_TMP/sessions.json.$$" +jq --arg name "$SESSION" --arg when "$old_iso" \ + '.sessions[$name].lastConnectionAttemptAt = $when + | del(.sessions[$name].pid)' \ + "$sessions_file" > "$tmp_file" +mv "$tmp_file" "$sessions_file" + +# Re-run the list command — consolidateSessions() runs on every list. +run_mcpc --json +assert_success +session_status=$(echo "$STDOUT" | jq -r ".sessions[] | select(.name == \"$SESSION\") | .status") +if [[ "$session_status" != "unauthorized" ]]; then + test_fail "expected status to stay 'unauthorized' after cooldown, got: '$session_status'" + exit 1 +fi +test_pass + +# ============================================================================= +# Test: using an unauthorized session surfaces the auth error + log path +# ============================================================================= + +test_case "using unauthorized session surfaces auth error with log path" +run_mcpc "$SESSION" tools-list +assert_failure +assert_exit_code 4 "should fail with auth exit code (4)" +assert_contains "$STDERR" "Authentication required by server" +assert_contains "$STDERR" "check logs at" +assert_contains "$STDERR" "bridge-${SESSION}.log" +test_pass + +test_done