From d5590a4ce57e7d145c0f2a2979374fc2f9b8abc2 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 22 Oct 2026 17:18:01 +0300 Subject: [PATCH] fix(oauth): keep a pending login alive across scans and the quarantine exemption (#1722) A remote OAuth upstream's browser approval failed at the code exchange with "instance retired" because two background paths tore down the in-flight login: - Every scan (admission, informational rescan, manual) that found the server disconnected called EnsureConnected, which restarts it. A server waiting on a person's login can never connect, so the scan waited 30 s, replaced the instance and retired the flow. The scanner now asks the optional oauthLoginAwareProvider capability (Runtime.ServerAwaitsOAuthLogin: manual flow in flight, or parked in Pending Auth) and scans without reconnecting such a server. - A new server's one-hour inspection exemption was overwritten by the admission scan's 15-minute grant, and the expiry-driven disconnect landed while the operator was still signing in. A grant never shortens an exemption in force, and both expiry paths hold an expired exemption (extending it in 5-minute steps) while a login for the server is in flight. When an instance is still retired mid-login (operator removal, restart or quarantine), the callback waiter now notices through its gate's context, invalidates its state, and the callback page says the sign-in was invalidated instead of showing a success page before a silent exchange failure. RT-02 untouched: nothing here approves, unquarantines or blocks anything. --- docs/features/oauth-authentication.md | 29 +++++ docs/features/security-quarantine.md | 11 ++ internal/oauth/callback_invalidate_test.go | 47 ++++++++ internal/oauth/config.go | 56 ++++++++- internal/runtime/runtime.go | 23 ++++ .../supervisor/exemption_login_test.go | 114 ++++++++++++++++++ internal/runtime/supervisor/supervisor.go | 90 ++++++++++++-- .../security/scanner/scan_login_guard_test.go | 81 +++++++++++++ internal/security/scanner/service.go | 29 ++++- internal/server/scan_login_guard_test.go | 28 +++++ internal/server/server.go | 10 ++ internal/upstream/core/connection_oauth.go | 12 ++ .../upstream/core/oauth_wait_retired_test.go | 67 ++++++++++ 13 files changed, 581 insertions(+), 16 deletions(-) create mode 100644 internal/oauth/callback_invalidate_test.go create mode 100644 internal/runtime/supervisor/exemption_login_test.go create mode 100644 internal/security/scanner/scan_login_guard_test.go create mode 100644 internal/server/scan_login_guard_test.go create mode 100644 internal/upstream/core/oauth_wait_retired_test.go diff --git a/docs/features/oauth-authentication.md b/docs/features/oauth-authentication.md index e88ff060e..d072eea33 100644 --- a/docs/features/oauth-authentication.md +++ b/docs/features/oauth-authentication.md @@ -337,6 +337,35 @@ Without `callback_base_url`, loopback callbacks behave as before. A configured `redirect_uri` continues to pin the private loopback listener; the external callback takes precedence for provider registration and token exchange. +### The browser says the sign-in was invalidated + +The callback page reports **Authorization Failed** with "This sign-in was +invalidated before it could finish: the server's connection was retired +(restarted, replaced, removed or quarantined) while the sign-in was in +progress" when the upstream's instance was torn down between the moment you +started the login and the moment the provider redirected back. The code it +delivered belongs to a flow that no longer exists, so nothing was signed in; +start the login again and approve it. + +Two background paths used to cause this on their own (GH #1722) and no longer +do: + +- A security scan (the admission scan, the informational rescan after a tool + change, or a manual scan) does not restart a server that is waiting on your + OAuth login — a login flow in flight, or a server parked in **Pending Auth** — + because the restart would retire the pending flow. The scan runs with what it + can export without a reconnect and logs + `Server is waiting on an OAuth login; scanning without reconnecting it`. +- A new server starts quarantined with a temporary review connection. That + connection window is held open while a login for the server is in flight (it + is extended in 5-minute steps and outlives the login by at most that grace), + and a later, shorter grant never cuts an earlier one short. The log line is + `Inspection exemption held: an OAuth login for the server is in flight`. + +An operator action that retires the instance during the login — removing or +restarting the server by hand, or quarantining it — still invalidates the +flow; the page above is how you learn about it. + ### Browser Doesn't Open ```bash diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index 82179422a..6599c19b4 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -624,6 +624,17 @@ so every server ends up with a security verdict instead of an empty badge: quarantined `trust_mode: "scan"` servers (the admission gate owns those) are skipped. Failed starts share the per-server retry cap of the admission scan. +A scan never reconnects a server that is waiting on a person's OAuth login (a +login flow in flight, or a server parked in **Pending Auth**): reconnecting +replaces the server's instance, which retires the pending flow and turns the +browser approval into a failed code exchange (GH #1722). Such a server is +scanned with what can be exported without a restart (the tool contracts +discovery already recorded, if any) and the reason is logged. The temporary +review connection a newly quarantined server gets is likewise held open while +a login for it is in flight, so its expiry cannot disconnect the server +underneath the sign-in. Neither rule approves, unquarantines or blocks +anything. + These scans are **informational**: the verdict fills in the scan summary and the UI badge and never quarantines, approves, or blocks anything. **A clean verdict never approves a changed or new tool**: a changed contract is held for review in diff --git a/internal/oauth/callback_invalidate_test.go b/internal/oauth/callback_invalidate_test.go new file mode 100644 index 000000000..bf33e3c18 --- /dev/null +++ b/internal/oauth/callback_invalidate_test.go @@ -0,0 +1,47 @@ +package oauth + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// GH #1722: when the instance that owns a pending login is retired (restarted, +// replaced, removed or quarantined) the waiting flow invalidates its state. The +// browser callback that arrives afterwards must then say the login was +// invalidated, not "Authorization Successful" followed by a silent exchange +// failure in the log. +func TestCallback_InvalidatedStateRendersFailureWithReason(t *testing.T) { + const state = "STATEinvalidated1234567890" + cs, _ := newObservedCallbackServer(t, state) + + cs.InvalidateState(state, "the server was restarted while the sign-in was in progress") + assert.False(t, cs.HasState(state), "an invalidated state has no waiter") + + req := httptest.NewRequest(http.MethodGet, DefaultRedirectPath+"?code=abc&state="+state, nil) + rec := httptest.NewRecorder() + cs.handleCallbackWithFailureNotification(rec, req, false) + + require.Equal(t, http.StatusBadRequest, rec.Code) + body := rec.Body.String() + assert.Contains(t, body, callbackTitleFailed) + assert.NotContains(t, body, callbackTitleSuccess) + assert.Contains(t, body, "the server was restarted while the sign-in was in progress") + assert.Contains(t, body, "alpha", "the page names the server to log in to again") +} + +func TestCallback_InvalidateUnknownStateIsNoop(t *testing.T) { + cs, _ := newObservedCallbackServer(t, "") + cs.InvalidateState("never-registered", "reason") + + req := httptest.NewRequest(http.MethodGet, DefaultRedirectPath+"?code=abc&state=never-registered", nil) + rec := httptest.NewRecorder() + cs.handleCallbackWithFailureNotification(rec, req, false) + + require.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "unknown or expired state", + "a state that was never waited on keeps the generic message") +} diff --git a/internal/oauth/config.go b/internal/oauth/config.go index 6969fba9b..2c7a81126 100644 --- a/internal/oauth/config.go +++ b/internal/oauth/config.go @@ -165,8 +165,17 @@ type CallbackServer struct { waitersMu sync.Mutex waiters map[string]chan map[string]string remoteCallbackURL string // protected by waitersMu + // invalidated maps a state whose flow gave up (its instance was retired + // while it waited, GH #1722) to the reason, so the callback that arrives + // later can say why the login was invalidated. Protected by waitersMu; + // bounded by maxInvalidatedStates, oldest dropped first. + invalidated map[string]string + invalidatedOrder []string } +// maxInvalidatedStates bounds CallbackServer.invalidated. +const maxInvalidatedStates = 16 + var globalCallbackManager = &CallbackServerManager{ servers: make(map[string]*CallbackServer), logger: zap.L().Named(oauthCallbackLoggerName), @@ -1832,6 +1841,42 @@ func (c *CallbackServer) UnregisterState(state string) { } } +// InvalidateState drops the waiter for a state whose flow cannot finish any +// more (GH #1722: the instance that owns it was retired while the person was +// still signing in) and remembers reason, so the browser callback that lands +// afterwards renders a failure page that says so instead of the generic +// "unknown or expired state". A state nobody is waiting for is left alone. +func (c *CallbackServer) InvalidateState(state, reason string) { + c.waitersMu.Lock() + defer c.waitersMu.Unlock() + if _, exists := c.waiters[state]; !exists { + return + } + delete(c.waiters, state) + if c.invalidated == nil { + c.invalidated = make(map[string]string) + } + if _, known := c.invalidated[state]; !known { + c.invalidatedOrder = append(c.invalidatedOrder, state) + for len(c.invalidatedOrder) > maxInvalidatedStates { + delete(c.invalidated, c.invalidatedOrder[0]) + c.invalidatedOrder = c.invalidatedOrder[1:] + } + } + c.invalidated[state] = reason + c.logger.Warn("OAuth login invalidated while waiting for the callback", + zap.String("state", StateFingerprint(state)), + zap.String("reason", reason)) +} + +// invalidationReason returns why a state's flow was invalidated, "" when it +// was not. +func (c *CallbackServer) invalidationReason(state string) string { + c.waitersMu.Lock() + defer c.waitersMu.Unlock() + return c.invalidated[state] +} + // waiterCount reports how many flows are currently parked on this server. // Used to decide whether a cached server may be torn down: replacing one that // still has a waiter drops a live flow, whose browser then lands on a closed @@ -1993,10 +2038,13 @@ func (c *CallbackServer) handleCallbackWithFailureNotification(w http.ResponseWr GetTokenStoreManager().RecordOAuthFailure(c.ServerName, reason) } - c.writePage(w, http.StatusBadRequest, callbackPage( - callbackTitleFailed, - fmt.Sprintf("mcpproxy is not waiting for this sign-in (unknown or expired state). "+ - "Nothing was signed in. Start the login again from mcpproxy for '%s'.", c.ServerName))) + message := fmt.Sprintf("mcpproxy is not waiting for this sign-in (unknown or expired state). "+ + "Nothing was signed in. Start the login again from mcpproxy for '%s'.", c.ServerName) + if why := c.invalidationReason(state); why != "" { + message = fmt.Sprintf("This sign-in was invalidated before it could finish: %s. "+ + "Nothing was signed in. Start the login again from mcpproxy for '%s'.", why, c.ServerName) + } + c.writePage(w, http.StatusBadRequest, callbackPage(callbackTitleFailed, message)) return } diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 0f1a04405..63fd0de0e 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -54,6 +54,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/core" "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/limiter" "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/managed" + upstreamtypes "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/types" ) // Status captures high-level state for API consumers. @@ -504,6 +505,9 @@ func New(cfg *config.Config, cfgPath string, logger *zap.Logger) (*Runtime, erro // Phase 7.3: Initialize Supervisor with ActorPoolSimple (delegates to UpstreamManager) actorPool := supervisor.NewActorPoolSimple(upstreamManager, logger) supervisorInstance := supervisor.New(configSvc, actorPool, logger) + // GH #1722: a quarantined server's inspection exemption must not expire + // underneath an OAuth login the operator is completing for it. + supervisorInstance.SetInspectionExemptionHold(oauth.IsManualFlowActive) // Initialize OAuth refresh manager for proactive token refresh // Uses storageManager as the token store and global coordinator for flow coordination @@ -3422,6 +3426,25 @@ func (r *Runtime) TriggerOAuthLogin(serverName string) error { return nil } +// ServerAwaitsOAuthLogin reports whether the named server cannot connect until +// a person completes its OAuth login: a manual login flow is in flight for it, +// or its live instance is parked in Pending Auth (GH #1722). Background +// restarts (the security scan's reconnect) stand down for such a server, +// because replacing its instance retires the pending flow. +func (r *Runtime) ServerAwaitsOAuthLogin(serverName string) bool { + if serverName == "" { + return false + } + if oauth.IsManualFlowActive(serverName) { + return true + } + if r.upstreamManager == nil { + return false + } + client, exists := r.upstreamManager.GetClient(serverName) + return exists && client != nil && !client.Retired() && client.GetState() == upstreamtypes.StatePendingAuth +} + // TriggerOAuthLoginQuick implements RuntimeOperations interface for management service. // Returns OAuthStartResult with actual browser status, auth URL, and any errors. // This is the synchronous version that provides immediate feedback about browser opening. diff --git a/internal/runtime/supervisor/exemption_login_test.go b/internal/runtime/supervisor/exemption_login_test.go new file mode 100644 index 000000000..3f0aa6b62 --- /dev/null +++ b/internal/runtime/supervisor/exemption_login_test.go @@ -0,0 +1,114 @@ +package supervisor + +import ( + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/configsvc" +) + +// GH #1722 cause 1: a quarantined server's inspection exemption expired while +// the operator was still completing its OAuth login, and the expiry-driven +// disconnect retired the in-flight flow. While a login is in flight for the +// server the exemption is held (extended by a grace) instead of expiring. + +func newExemptionTestSupervisor(t *testing.T) *Supervisor { + t.Helper() + cfg := &config.Config{Listen: "127.0.0.1:8080", Servers: []*config.ServerConfig{}} + configSvc := configsvc.NewService(cfg, "/tmp/config.json", zap.NewNop()) + t.Cleanup(configSvc.Close) + mockUpstream := NewMockUpstreamAdapter() + t.Cleanup(mockUpstream.Close) + return New(configSvc, mockUpstream, zap.NewNop()) +} + +func (s *Supervisor) exemptionExpiry(name string) (time.Time, bool) { + s.inspectionExemptionsMu.Lock() + defer s.inspectionExemptionsMu.Unlock() + exp, ok := s.inspectionExemptions[name] + return exp, ok +} + +func TestInspectionExemption_HeldWhileLoginInFlight(t *testing.T) { + s := newExemptionTestSupervisor(t) + var loginActive atomic.Bool + s.SetInspectionExemptionHold(func(name string) bool { + return name == "srv" && loginActive.Load() + }) + s.exemptionHoldGrace = 40 * time.Millisecond + + require.NoError(t, s.RequestInspectionExemption("srv", 20*time.Millisecond)) + loginActive.Store(true) + time.Sleep(50 * time.Millisecond) + + assert.True(t, s.IsInspectionExempted("srv"), + "an expired exemption must be held while the server's OAuth login is in flight") + exp, ok := s.exemptionExpiry("srv") + require.True(t, ok, "the held exemption must stay recorded") + assert.True(t, exp.After(time.Now()), "the hold must extend the expiry into the future") + + // Once the login is over the exemption runs out after the grace. + loginActive.Store(false) + assert.Eventually(t, func() bool { return !s.IsInspectionExempted("srv") }, 2*time.Second, 5*time.Millisecond, + "without a login in flight the exemption must expire again") +} + +func TestInspectionExemption_CleanupLoopHoldsLoginInFlight(t *testing.T) { + s := newExemptionTestSupervisor(t) + var loginActive atomic.Bool + s.SetInspectionExemptionHold(func(string) bool { return loginActive.Load() }) + s.exemptionHoldGrace = time.Minute + + s.inspectionExemptionsMu.Lock() + s.inspectionExemptions["srv"] = time.Now().Add(-time.Second) // already past + s.inspectionExemptionsMu.Unlock() + + loginActive.Store(true) + assert.Empty(t, s.collectExpiredExemptions(time.Now()), + "the cleanup loop must not disconnect a server whose login is in flight") + exp, ok := s.exemptionExpiry("srv") + require.True(t, ok) + assert.True(t, exp.After(time.Now().Add(30*time.Second)), "held exemption extended by the grace") + + loginActive.Store(false) + s.inspectionExemptionsMu.Lock() + s.inspectionExemptions["srv"] = time.Now().Add(-time.Second) + s.inspectionExemptionsMu.Unlock() + assert.Equal(t, []string{"srv"}, s.collectExpiredExemptions(time.Now()), + "with no login in flight the expired exemption is collected as before") + _, ok = s.exemptionExpiry("srv") + assert.False(t, ok, "a collected exemption is removed") +} + +func TestInspectionExemption_NoHoldWithoutPredicate(t *testing.T) { + s := newExemptionTestSupervisor(t) + require.NoError(t, s.RequestInspectionExemption("srv", 20*time.Millisecond)) + time.Sleep(40 * time.Millisecond) + assert.False(t, s.IsInspectionExempted("srv"), "no predicate: the historical expiry applies") +} + +// A later, shorter grant (the scan's 15 minutes on top of the add's hour) must +// not cut an exemption short: the add's review window is what the operator +// relies on while logging in. +func TestRequestInspectionExemption_NeverShortens(t *testing.T) { + s := newExemptionTestSupervisor(t) + require.NoError(t, s.RequestInspectionExemption("srv", time.Hour)) + long, ok := s.exemptionExpiry("srv") + require.True(t, ok) + + require.NoError(t, s.RequestInspectionExemption("srv", 15*time.Minute)) + after, ok := s.exemptionExpiry("srv") + require.True(t, ok) + assert.Equal(t, long, after, "a shorter re-grant must keep the longer expiry") + + require.NoError(t, s.RequestInspectionExemption("srv", 2*time.Hour)) + longer, ok := s.exemptionExpiry("srv") + require.True(t, ok) + assert.True(t, longer.After(long), "a longer re-grant extends the exemption") +} diff --git a/internal/runtime/supervisor/supervisor.go b/internal/runtime/supervisor/supervisor.go index b77e54cec..5a299fb9d 100644 --- a/internal/runtime/supervisor/supervisor.go +++ b/internal/runtime/supervisor/supervisor.go @@ -174,6 +174,16 @@ type Supervisor struct { // Inspection exemptions for temporary connections to quarantined servers inspectionExemptions map[string]time.Time inspectionExemptionsMu sync.RWMutex + // exemptionHold reports whether serverName's exemption must be held past + // its expiry (GH #1722: an OAuth login for the server is in flight, and the + // expiry-driven disconnect would invalidate it). Nil: no hold. Set once at + // construction (SetInspectionExemptionHold), read under + // inspectionExemptionsMu; it must be cheap and must not call back into the + // supervisor. + exemptionHold func(serverName string) bool + // exemptionHoldGrace is how far a held exemption is extended each time its + // expiry is reached, and the most it outlives the login that held it. + exemptionHoldGrace time.Duration // Circuit breaker for inspection failures (Phase 2: Issue #105 stability) inspectionFailures map[string]*inspectionFailureInfo @@ -265,6 +275,7 @@ func New(configSvc *configsvc.Service, upstream UpstreamInterface, logger *zap.L eventCh: make(chan Event, 500), // Phase 6: Increased buffer for async operations listeners: make([]chan Event, 0), inspectionExemptions: make(map[string]time.Time), + exemptionHoldGrace: inspectionExemptionHoldGrace, inspectionFailures: make(map[string]*inspectionFailureInfo), ctx: ctx, cancel: cancel, @@ -401,16 +412,7 @@ func (s *Supervisor) exemptionCleanupLoop() { return case <-ticker.C: - // Check for expired exemptions - s.inspectionExemptionsMu.Lock() - expiredServers := make([]string, 0) - for serverName, expiryTime := range s.inspectionExemptions { - if time.Now().After(expiryTime) { - expiredServers = append(expiredServers, serverName) - delete(s.inspectionExemptions, serverName) - } - } - s.inspectionExemptionsMu.Unlock() + expiredServers := s.collectExpiredExemptions(time.Now()) // Trigger reconciliation for each expired exemption to disconnect servers for _, serverName := range expiredServers { @@ -1854,12 +1856,75 @@ func (s *Supervisor) ReconcileQuiescent() bool { return s.startupReconciled.Load() && s.actionsInFlight.Load() == 0 } +// inspectionExemptionHoldGrace is the default exemptionHoldGrace: a held +// exemption is extended by this much each time it reaches its expiry while +// the login is in flight, so once the login completes the freshly +// authenticated connection has this long to be reviewed before the quarantine +// policy disconnects it again. +const inspectionExemptionHoldGrace = 5 * time.Minute + +// SetInspectionExemptionHold installs the predicate that holds a server's +// inspection exemption past its expiry (GH #1722). The runtime passes "an +// OAuth login is in flight for this server". Call it before Start. +func (s *Supervisor) SetInspectionExemptionHold(hold func(serverName string) bool) { + s.inspectionExemptionsMu.Lock() + defer s.inspectionExemptionsMu.Unlock() + s.exemptionHold = hold +} + +// heldPastExpiryLocked is the one place an expired exemption is either +// extended (the hold predicate says a login is in flight: the expiry moves out +// by exemptionHoldGrace and the entry stays) or reported expired. Caller holds +// inspectionExemptionsMu. +func (s *Supervisor) heldPastExpiryLocked(serverName string, now time.Time) bool { + if s.exemptionHold == nil || !s.exemptionHold(serverName) { + return false + } + grace := s.exemptionHoldGrace + if grace <= 0 { + grace = inspectionExemptionHoldGrace + } + s.inspectionExemptions[serverName] = now.Add(grace) + s.logger.Info("Inspection exemption held: an OAuth login for the server is in flight", + zap.String("server", serverName), + zap.Duration("extended_by", grace)) + return true +} + +// collectExpiredExemptions removes and returns the exemptions whose expiry +// has passed as of now, skipping (and extending) the ones the hold predicate +// keeps alive. The cleanup loop disconnects what it returns. +func (s *Supervisor) collectExpiredExemptions(now time.Time) []string { + s.inspectionExemptionsMu.Lock() + defer s.inspectionExemptionsMu.Unlock() + expiredServers := make([]string, 0) + for serverName, expiryTime := range s.inspectionExemptions { + if !now.After(expiryTime) { + continue + } + if s.heldPastExpiryLocked(serverName, now) { + continue + } + expiredServers = append(expiredServers, serverName) + delete(s.inspectionExemptions, serverName) + } + return expiredServers +} + // RequestInspectionExemption grants temporary connection permission for a quarantined server. // This allows security inspection to temporarily connect to quarantined servers. // Triggers immediate reconciliation to connect the server. +// +// A grant never shortens an exemption that is already in force (GH #1722): +// the admission scan's 15-minute grant used to overwrite the hour a new +// server's add had granted, and the operator's login then ran into the +// shortened expiry. func (s *Supervisor) RequestInspectionExemption(serverName string, duration time.Duration) error { s.inspectionExemptionsMu.Lock() expiryTime := time.Now().Add(duration) + if current, exists := s.inspectionExemptions[serverName]; exists && current.After(expiryTime) { + expiryTime = current + } s.inspectionExemptions[serverName] = expiryTime s.inspectionExemptionsMu.Unlock() @@ -1927,7 +1992,10 @@ func (s *Supervisor) IsInspectionExempted(serverName string) bool { } // Check if exemption has expired - if time.Now().After(expiryTime) { + if now := time.Now(); now.After(expiryTime) { + if s.heldPastExpiryLocked(serverName, now) { + return true + } delete(s.inspectionExemptions, serverName) s.logger.Warn("⚠️ Inspection exemption expired, forcing disconnect", zap.String("server", serverName)) diff --git a/internal/security/scanner/scan_login_guard_test.go b/internal/security/scanner/scan_login_guard_test.go new file mode 100644 index 000000000..958a75298 --- /dev/null +++ b/internal/security/scanner/scan_login_guard_test.go @@ -0,0 +1,81 @@ +package scanner + +import ( + "context" + "sync/atomic" + "testing" + + "go.uber.org/zap" +) + +// GH #1722 cause 2: a scan of a server that cannot connect until a person +// completes its OAuth login (a flow in flight, or parked in Pending Auth) used +// to call EnsureConnected, which RESTARTS the server and retires the instance +// that owns the pending flow (PKCE verifier, callback waiter). The scan must +// not reconnect such a server; it scans what it can export without a restart. + +type loginGuardProvider struct { + awaiting atomic.Bool + ensureCalls atomic.Int32 + connectedAfter bool +} + +func (p *loginGuardProvider) GetServerInfo(string) (*ServerInfo, error) { + return &ServerInfo{Name: "oauth-srv", Protocol: "http", URL: "https://example.test/mcp"}, nil +} + +func (p *loginGuardProvider) GetServerTools(string) ([]map[string]interface{}, error) { + return []map[string]interface{}{{"name": "list_teams", "description": "List teams."}}, nil +} + +func (p *loginGuardProvider) EnsureConnected(context.Context, string) error { + p.ensureCalls.Add(1) + return nil +} + +func (p *loginGuardProvider) IsConnected(string) bool { return p.connectedAfter } + +func (p *loginGuardProvider) AwaitingOAuthLogin(string) bool { return p.awaiting.Load() } + +func newLoginGuardService(t *testing.T, provider ServerInfoProvider) *Service { + t.Helper() + dir := t.TempDir() + logger := zap.NewNop() + store := newMockStorage() + docker := NewDockerRunner(logger) + registry := NewRegistry(dir, logger) + svc := NewService(store, registry, docker, dir, logger) + svc.SetServerInfoProvider(provider) + return svc +} + +func TestStartScan_DoesNotReconnectServerAwaitingOAuthLogin(t *testing.T) { + provider := &loginGuardProvider{} + provider.awaiting.Store(true) + svc := newLoginGuardService(t, provider) + + if _, err := svc.StartScan(context.Background(), "oauth-srv", false, nil, ""); err != nil { + t.Fatalf("StartScan: %v", err) + } + waitForScanIdle(t, svc, "oauth-srv") + + if got := provider.ensureCalls.Load(); got != 0 { + t.Fatalf("a scan must not restart a server awaiting an OAuth login (would retire the flow); EnsureConnected called %d time(s)", got) + } +} + +// Control: the same disconnected server with no login pending is reconnected +// for the export, as before. +func TestStartScan_ReconnectsDisconnectedServerWithoutLogin(t *testing.T) { + provider := &loginGuardProvider{} + svc := newLoginGuardService(t, provider) + + if _, err := svc.StartScan(context.Background(), "oauth-srv", false, nil, ""); err != nil { + t.Fatalf("StartScan: %v", err) + } + waitForScanIdle(t, svc, "oauth-srv") + + if got := provider.ensureCalls.Load(); got == 0 { + t.Fatal("a disconnected server with no login pending is still reconnected for the scan export") + } +} diff --git a/internal/security/scanner/service.go b/internal/security/scanner/service.go index 159017feb..4abcf5400 100644 --- a/internal/security/scanner/service.go +++ b/internal/security/scanner/service.go @@ -107,6 +107,25 @@ type ServerInfoProvider interface { IsConnected(serverName string) bool } +// oauthLoginAwareProvider is an OPTIONAL capability a ServerInfoProvider may +// implement (GH #1722): report that a server cannot connect until a person +// completes its OAuth login — a manual login flow is in flight for it, or it +// is parked in Pending Auth. A scan never reconnects such a server: +// EnsureConnected RESTARTS it, which retires the instance that owns the +// pending flow (PKCE verifier, callback waiter), so the code the browser +// delivers afterwards cannot be exchanged. The scan proceeds with whatever it +// can export without a restart (the ingested contracts) or fails closed. +type oauthLoginAwareProvider interface { + AwaitingOAuthLogin(serverName string) bool +} + +// awaitingOAuthLogin reports the optional capability, false when the provider +// does not implement it. +func (s *Service) awaitingOAuthLogin(serverName string) bool { + provider, ok := s.serverInfo.(oauthLoginAwareProvider) + return ok && provider.AwaitingOAuthLogin(serverName) +} + // ingestedToolContractsProvider is an OPTIONAL capability a ServerInfoProvider // may also implement (RT-06, red-team 2026-10-11): return the tool contracts // discovery actually ingested for a server — the definitions recorded on its @@ -1568,7 +1587,12 @@ func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool, req.ToolDefinitions = e.Data return nil } - if !s.serverInfo.IsConnected(serverName) { + if !s.serverInfo.IsConnected(serverName) && s.awaitingOAuthLogin(serverName) { + // GH #1722: a restart would retire the pending login flow, and a + // server waiting on a person can never connect for the scan. + s.logger.Info("Server is waiting on an OAuth login; scanning without reconnecting it (a restart would invalidate the login)", + zap.String("server", serverName)) + } else if !s.serverInfo.IsConnected(serverName) { s.logger.Info("Server is disconnected, attempting to connect for scan", zap.String("server", serverName)) if err := s.serverInfo.EnsureConnected(ctx, serverName); err != nil { @@ -1593,6 +1617,9 @@ func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool, s.logger.Info("Tool export returned 0 for a connected server, retrying export without restarting it", zap.String("server", serverName)) exportErr = exportInto(scanCtx) + } else if s.awaitingOAuthLogin(serverName) { + s.logger.Info("Tool export returned 0 for a server waiting on an OAuth login; not restarting it (a restart would invalidate the login)", + zap.String("server", serverName)) } else { s.logger.Info("Tool export returned 0, retrying after EnsureConnected", zap.String("server", serverName)) diff --git a/internal/server/scan_login_guard_test.go b/internal/server/scan_login_guard_test.go new file mode 100644 index 000000000..6176b06a4 --- /dev/null +++ b/internal/server/scan_login_guard_test.go @@ -0,0 +1,28 @@ +package server + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" +) + +// GH #1722 cause 2: the scanner asks the server layer whether a server is +// waiting on a person's OAuth login before it reconnects (restarts) it for the +// tool export. A manual login in flight is one such wait. +func TestConfigServerInfoProvider_AwaitingOAuthLogin_ManualFlow(t *testing.T) { + s := newInformationalTestServer(t, newFakeSecurityScanner(), nil, enabledServer("srv", config.TrustModeManual)) + p := &configServerInfoProvider{server: s} + + assert.False(t, p.AwaitingOAuthLogin("srv"), "no login, no wait") + + release := oauth.BeginManualFlow("srv", time.Minute) + defer release() + assert.True(t, p.AwaitingOAuthLogin("srv"), "a manual OAuth login in flight blocks scan restarts") + + release() + assert.False(t, p.AwaitingOAuthLogin("srv"), "released login: restarts allowed again") +} diff --git a/internal/server/server.go b/internal/server/server.go index a3d6fb202..556fb4ab9 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -5312,6 +5312,16 @@ func (p *configServerInfoProvider) EnsureConnected(ctx context.Context, serverNa return p.server.runtime.RestartServer(serverName) } +// AwaitingOAuthLogin implements the scanner's optional oauthLoginAwareProvider +// capability (GH #1722): the scan must not restart a server that is waiting on +// a person's OAuth login, because the restart retires the pending flow. +func (p *configServerInfoProvider) AwaitingOAuthLogin(serverName string) bool { + if p.server == nil || p.server.runtime == nil { + return false + } + return p.server.runtime.ServerAwaitsOAuthLogin(serverName) +} + // IsConnected checks if the server has an active MCP connection via the stateview. func (p *configServerInfoProvider) IsConnected(serverName string) bool { if p.server == nil { diff --git a/internal/upstream/core/connection_oauth.go b/internal/upstream/core/connection_oauth.go index abef861cb..1f336f267 100644 --- a/internal/upstream/core/connection_oauth.go +++ b/internal/upstream/core/connection_oauth.go @@ -2363,6 +2363,18 @@ func (c *Client) waitForOAuthCallbackAsync(ctx context.Context, oauthHandler *up tokenManager := oauth.GetTokenStoreManager() tokenManager.MarkOAuthCompletedBound(c.config.Name, c.CredentialBinding()) + case <-c.Gate().Context().Done(): + // GH #1722: the instance this login runs behind was retired (the + // server was restarted, replaced, removed or quarantined) while the + // person was still signing in. The code the browser delivers later + // could not be exchanged through the retired gate, so stop waiting + // and tell the callback page why. + const reason = "the server's connection was retired (restarted, replaced, removed or quarantined) while the sign-in was in progress" + c.logger.Warn("⚠️ OAuth login invalidated: upstream instance retired while waiting for the callback", + zap.String("server", c.config.Name), + zap.String("correlation_id", correlationID)) + callbackServer.InvalidateState(state, reason) + case <-ctx.Done(): if errors.Is(ctx.Err(), context.DeadlineExceeded) { c.logger.Warn("⏱️ OAuth authorization timeout", diff --git a/internal/upstream/core/oauth_wait_retired_test.go b/internal/upstream/core/oauth_wait_retired_test.go new file mode 100644 index 000000000..1a2fcd693 --- /dev/null +++ b/internal/upstream/core/oauth_wait_retired_test.go @@ -0,0 +1,67 @@ +package core + +import ( + "context" + "fmt" + "io" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/dialgate" +) + +// GH #1722: the background callback waiter runs behind the registered +// instance's gate. When that instance is retired (the server was restarted by +// a scan, replaced, removed or quarantined) the waiter must stop waiting and +// invalidate its state, so the callback that arrives later tells the person +// the login was invalidated instead of showing a success page for a code +// nobody can exchange any more. +func TestWaitForOAuthCallbackAsync_RetiredInstanceInvalidatesTheLogin(t *testing.T) { + serverName := "wait-retired-server" + manager := oauth.GetGlobalCallbackManager() + cs, err := manager.StartCallbackServer(serverName, 0) + require.NoError(t, err) + t.Cleanup(func() { _ = manager.StopCallbackServer(serverName) }) + + ctx, cancel := context.WithTimeout(context.Background(), time.Hour) + defer cancel() + + c := newOAuthWaitTestClient(serverName) + gate := dialgate.New() + c.SetGate(gate) + + const state = "state-retired-mid-login" + cs.RegisterState(state) + + done := make(chan struct{}) + go func() { + defer close(done) + c.waitForOAuthCallbackAsync(ctx, nil, "verifier", state, "corr-retired") + }() + require.Eventually(t, func() bool { return cs.HasState(state) }, 2*time.Second, 5*time.Millisecond) + + gate.Retire() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("the callback waiter kept waiting after its instance was retired") + } + assert.False(t, cs.HasState(state), "a retired flow must not keep its callback waiter") + + // The browser lands on the callback after the retirement: not a success. + url := fmt.Sprintf("http://%s:%d%s?code=late-code&state=%s", cs.BindHost, cs.Port, cs.Path, state) + resp, err := http.Get(url) //nolint:gosec // loopback test listener + require.NoError(t, err) + body, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Contains(t, string(body), "Authorization Failed") + assert.NotContains(t, string(body), "Authorization Successful") + assert.Contains(t, string(body), "retired", "the page says why the login was invalidated") +}