fix: address critical security and code quality issues

This commit resolves multiple HIGH and MEDIUM severity issues found during
security audit. All tests pass after changes.

**HIGH Severity Fixes:**

1. Fix nil pointer panic with --print-openapi flag
   - Added nil check before process.Wait() to prevent crash
   - File: cmd/server/server.go

2. Fix goroutine leak in snapshot loop
   - Added context cancellation with select statement
   - Changed time.Sleep to ticker for proper cleanup
   - File: lib/httpapi/server.go

3. Fix race condition in terminal screen reading
   - Always hold lock when reading terminal state
   - Prevents concurrent read/write on p.xp.State
   - File: lib/termexec/termexec.go

4. Add defensive checks for unsafe reflection
   - Added defer/recover to catch reflection failures
   - Improved error logging with recovery attempts
   - Warns about unsafe xpty dependency
   - File: lib/termexec/termexec.go

**MEDIUM Severity Fixes:**

5. Add message size validation
   - MaxMessageSize: 10MB for user messages
   - MaxRawMessageSize: 1KB for raw terminal input
   - Prevents DoS via oversized messages
   - File: lib/httpapi/server.go

6. Add HTTP server timeouts
   - ReadTimeout: 15s (prevent slow header attacks)
   - ReadHeaderTimeout: 5s (specifically for headers)
   - IdleTimeout: 60s (close idle connections)
   - WriteTimeout: 0 (disabled for SSE long-polling)
   - File: lib/httpapi/server.go

7. Replace panics with error handling
   - logctx.From() returns default logger instead of panic
   - conversation.statusInner() logs errors and returns safe fallback
   - convertStatus() logs unknown status instead of panic
   - Files: lib/logctx/logctx.go, lib/screentracker/conversation.go,
     lib/httpapi/events.go

All existing tests pass (CGO_ENABLED=0 go test ./...).

Author: claude

cmd/server/server.go

@@ -118,15 +118,18 @@ func runServer(ctx context.Context, logger *slog.Logger, argsToPass []string) er
 	processExitCh := make(chan error, 1)
 	go func() {
 		defer close(processExitCh)
-		if err := process.Wait(); err != nil {
-			if errors.Is(err, termexec.ErrNonZeroExitCode) {
-				processExitCh <- xerrors.Errorf("========\n%s\n========\n: %w", strings.TrimSpace(process.ReadScreen()), err)
-			} else {
-				processExitCh <- xerrors.Errorf("failed to wait for process: %w", err)
+		// Only wait for process if it exists (not in --print-openapi mode)
+		if process != nil {
+			if err := process.Wait(); err != nil {
+				if errors.Is(err, termexec.ErrNonZeroExitCode) {
+					processExitCh <- xerrors.Errorf("========\n%s\n========\n: %w", strings.TrimSpace(process.ReadScreen()), err)
+				} else {
+					processExitCh <- xerrors.Errorf("failed to wait for process: %w", err)
+				}
+			}
+			if err := srv.Stop(ctx); err != nil {
+				logger.Error("Failed to stop server", "error", err)
 			}
-		}
-		if err := srv.Stop(ctx); err != nil {
-			logger.Error("Failed to stop server", "error", err)
 		}
 	}()
 	if err := srv.Start(); err != nil && err != context.Canceled && err != http.ErrServerClosed {

lib/httpapi/events.go

@@ -1,7 +1,7 @@
 package httpapi
 
 import (
-	"fmt"
+	"log/slog"
 	"strings"
 	"sync"
 	"time"
@@ -75,7 +75,9 @@ func convertStatus(status st.ConversationStatus) AgentStatus {
 	case st.ConversationStatusChanging:
 		return AgentStatusRunning
 	default:
-		panic(fmt.Sprintf("unknown conversation status: %s", status))
+		// Don't panic - log and return safe default
+		slog.Error("Unknown conversation status", "status", status)
+		return AgentStatusRunning
 	}
 }
 

lib/httpapi/server.go

@@ -61,6 +61,14 @@ func (s *Server) GetOpenAPI() string {
 // because the action of taking a snapshot takes time too.
 const snapshotInterval = 25 * time.Millisecond
 
+const (
+	// MaxMessageSize is the maximum size for user messages (10MB)
+	MaxMessageSize = 10 * 1024 * 1024
+	// MaxRawMessageSize is the maximum size for raw terminal input (1KB)
+	// Raw messages go directly to the terminal, so we're more conservative
+	MaxRawMessageSize = 1024
+)
+
 type ServerConfig struct {
 	AgentType      mf.AgentType
 	Process        *termexec.Process
@@ -264,11 +272,19 @@ func hostAuthorizationMiddleware(allowedHosts []string, badHostHandler http.Hand
 func (s *Server) StartSnapshotLoop(ctx context.Context) {
 	s.conversation.StartSnapshotLoop(ctx)
 	go func() {
+		ticker := time.NewTicker(snapshotInterval)
+		defer ticker.Stop()
+
 		for {
-			s.emitter.UpdateStatusAndEmitChanges(s.conversation.Status())
-			s.emitter.UpdateMessagesAndEmitChanges(s.conversation.Messages())
-			s.emitter.UpdateScreenAndEmitChanges(s.conversation.Screen())
-			time.Sleep(snapshotInterval)
+			select {
+			case <-ctx.Done():
+				s.logger.Info("Snapshot loop exiting")
+				return
+			case <-ticker.C:
+				s.emitter.UpdateStatusAndEmitChanges(s.conversation.Status())
+				s.emitter.UpdateMessagesAndEmitChanges(s.conversation.Messages())
+				s.emitter.UpdateScreenAndEmitChanges(s.conversation.Screen())
+			}
 		}
 	}()
 }
@@ -354,6 +370,18 @@ func (s *Server) getMessages(ctx context.Context, input *struct{}) (*MessagesRes
 
 // createMessage handles POST /message
 func (s *Server) createMessage(ctx context.Context, input *MessageRequest) (*MessageResponse, error) {
+	// Validate message size based on type
+	maxSize := MaxMessageSize
+	if input.Body.Type == MessageTypeRaw {
+		maxSize = MaxRawMessageSize
+	}
+
+	if len(input.Body.Content) > maxSize {
+		return nil, huma.Error400BadRequest(
+			fmt.Sprintf("message too large (max %d bytes)", maxSize),
+		)
+	}
+
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
@@ -447,8 +475,12 @@ func (s *Server) subscribeScreen(ctx context.Context, input *struct{}, send sse.
 func (s *Server) Start() error {
 	addr := fmt.Sprintf(":%d", s.port)
 	s.srv = &http.Server{
-		Addr:    addr,
-		Handler: s.router,
+		Addr:              addr,
+		Handler:           s.router,
+		ReadTimeout:       15 * time.Second, // Prevent slow header attacks
+		WriteTimeout:      0,                 // Disabled for SSE long-polling
+		IdleTimeout:       60 * time.Second,  // Close idle connections
+		ReadHeaderTimeout: 5 * time.Second,   // Specifically for headers
 	}
 
 	return s.srv.ListenAndServe()

lib/logctx/logctx.go

@@ -16,10 +16,11 @@ func WithLogger(ctx context.Context, logger *slog.Logger) context.Context {
 	return context.WithValue(ctx, loggerKey, logger)
 }
 
-// From retrieves the logger from the context or panics if no logger is found
+// From retrieves the logger from the context or returns the default logger if none is found
 func From(ctx context.Context) *slog.Logger {
 	if logger, ok := ctx.Value(loggerKey).(*slog.Logger); ok {
 		return logger
 	}
-	panic("no logger found in context")
+	// Return default logger instead of panicking
+	return slog.Default()
 }

lib/screentracker/conversation.go

@@ -2,7 +2,7 @@ package screentracker
 
 import (
 	"context"
-	"fmt"
+	"log/slog"
 	"strings"
 	"sync"
 	"time"
@@ -355,12 +355,16 @@ func (c *Conversation) SendMessage(messageParts ...MessagePart) error {
 
 // Assumes that the caller holds the lock
 func (c *Conversation) statusInner() ConversationStatus {
-	// sanity checks
+	// sanity checks - log errors and return safe fallback instead of panicking
 	if c.snapshotBuffer.Capacity() != c.stableSnapshotsThreshold {
-		panic(fmt.Sprintf("snapshot buffer capacity %d is not equal to snapshot threshold %d. can't check stability", c.snapshotBuffer.Capacity(), c.stableSnapshotsThreshold))
+		slog.Error("BUG: snapshot buffer capacity mismatch",
+			"capacity", c.snapshotBuffer.Capacity(),
+			"threshold", c.stableSnapshotsThreshold)
+		return ConversationStatusChanging // Safe fallback
 	}
 	if c.stableSnapshotsThreshold == 0 {
-		panic("stable snapshots threshold is 0. can't check stability")
+		slog.Error("BUG: stable snapshots threshold is 0")
+		return ConversationStatusChanging // Safe fallback
 	}
 
 	snapshots := c.snapshotBuffer.GetAll()

lib/termexec/termexec.go

@@ -74,16 +74,36 @@ func StartProcess(ctx context.Context, args StartProcessConfig) (*Process, error
 		//
 		// Warning: This depends on xpty internals and may break if xpty changes.
 		// A proper fix would require forking xpty or getting upstream changes.
+		// Add defensive programming to catch reflection failures
+		defer func() {
+			if r := recover(); r != nil {
+				logger.Error("CRITICAL: unsafe reflection failed - xpty library may have changed",
+					"panic", r,
+					"field", "pp",
+					"action", "Terminal will become unresponsive. Please update agentapi or pin xpty version.")
+				// Attempt graceful shutdown
+				if process.execCmd.Process != nil {
+					_ = process.execCmd.Process.Signal(os.Interrupt)
+				}
+			}
+		}()
+
 		pp := util.GetUnexportedField(xp, "pp").(*xpty.PassthroughPipe)
+		logger.Warn("Using unsafe reflection to access xpty internals - this may break on xpty updates",
+			"xpty_version", "0.6.0",
+			"field", "pp")
+
 		for {
 			r, _, err := pp.ReadRune()
 			if err != nil {
 				if err != io.EOF {
 					logger.Error("Error reading from pseudo terminal", "error", err)
+					// Attempt recovery: signal process termination
+					if process.execCmd.Process != nil {
+						logger.Info("Attempting to terminate unresponsive process")
+						_ = process.execCmd.Process.Signal(os.Interrupt)
+					}
 				}
-				// TODO: handle this error better. if this happens, the terminal
-				// state will never be updated anymore and the process will appear
-				// unresponsive.
 				return
 			}
 			process.screenUpdateLock.Lock()
@@ -114,15 +134,20 @@ func (p *Process) Signal(sig os.Signal) error {
 func (p *Process) ReadScreen() string {
 	for range 3 {
 		p.screenUpdateLock.RLock()
-		if time.Since(p.lastScreenUpdate) >= 16*time.Millisecond {
-			state := p.xp.State.String()
-			p.screenUpdateLock.RUnlock()
-			return state
-		}
+		timeSinceUpdate := time.Since(p.lastScreenUpdate)
 		p.screenUpdateLock.RUnlock()
+
+		if timeSinceUpdate >= 16*time.Millisecond {
+			break
+		}
 		time.Sleep(16 * time.Millisecond)
 	}
-	return p.xp.State.String()
+
+	// Always read with lock held to prevent race condition
+	p.screenUpdateLock.RLock()
+	state := p.xp.State.String()
+	p.screenUpdateLock.RUnlock()
+	return state
 }
 
 // Write sends input to the process via the pseudo terminal.