Skip to content

Bug: sseManager.get() dereferences nil session pointer — panics on unknown sessionId #2548

@marlonbarreto-git

Description

@marlonbarreto-git

Bug Description

sseManager.get() in internal/server/mcp.go (lines ~60-65) unconditionally sets session.lastActive = time.Now() without checking if session is nil. When the session ID is not found in the map, Go returns the zero value (nil) for the pointer, and the field assignment panics.

Affected Code

File: internal/server/mcp.go, lines 60-66

func (m *sseManager) get(id string) (*sseSession, bool) {
    m.mu.Lock()
    defer m.mu.Unlock()
    session, ok := m.sseSessions[id]
    session.lastActive = time.Now()   // PANICS when session is nil
    return session, ok
}

Trigger

Any HTTP POST to /mcp with a ?sessionId=<invalid-or-expired-id> query parameter that doesn't match an active SSE session. The caller in httpHandler (line ~434) checks ok after calling get(), but the panic fires inside get() before the guard can execute:

session, ok = s.sseManager.get(sessionId)
if !ok {   // <-- too late, already panicked
    s.logger.DebugContext(ctx, "sse session not available")
}

Impact

  • Severity: Critical — Any malformed, expired, or guessed session ID crashes the HTTP handler
  • Any client reconnecting after a server restart with a stale session cookie triggers this
  • Trivially reproducible: curl -X POST "https://server/mcp?sessionId=nonexistent"

Steps to Reproduce

  1. Start a genai-toolbox MCP server with SSE transport enabled
  2. Send a POST request with an invalid session ID:
    curl -X POST "http://localhost:5000/mcp?sessionId=does-not-exist" \
      -H "Content-Type: application/json" \
      -d '{"jsonrpc":"2.0","method":"tools/list","id":1}'
  3. Server panics with nil pointer dereference

Expected Behavior

get() should guard against nil session before accessing fields.

Proposed Solution

func (m *sseManager) get(id string) (*sseSession, bool) {
    m.mu.Lock()
    defer m.mu.Unlock()
    session, ok := m.sseSessions[id]
    if ok {
        session.lastActive = time.Now()
    }
    return session, ok
}

This is a one-line fix that matches the existing pattern used in delete() (which already checks ok before accessing the session).

Happy to submit a PR for this fix.

Metadata

Metadata

Assignees

Labels

priority: p0Highest priority. Critical issue. P0 implies highest priority.type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions