fix(oauth): keep a pending login alive across scans and the quarantine exemption

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

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

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")
+}

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
 	}
 

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.

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")
+}