fix(phase-1): redact API key query params from log output and add mutex thread-safety to providers map

2026-07-21 13:18:31 -04:00
parent 84a18f5866
commit 42b70621a1
6 changed files with 277 additions and 17 deletions
@@ -0,0 +1,96 @@
+# GopherGate Remediation Action Plan
+
+Based on the findings from the [Code Review Report](file:///home/newkirk/Projects/gophergate/code_review.md), this document outlines a phased execution plan to fix all security, concurrency, reliability, and code quality issues in **GopherGate**.
+
+---
+
+## Phase 1: Critical Security & Crash Fixes (Immediate)
+
+### Task 1.1: Redact API Keys in Gemini Provider Logs
+- **Target File:** [`internal/providers/gemini.go`](file:///home/newkirk/Projects/gophergate/internal/providers/gemini.go#L378)
+- **Problem:** `fmt.Printf("[Gemini] POST %s\n", url)` logs the raw request URL containing `?key=AIzaSy...`, leaking API secrets to logs.
+- **Action Items:**
+  1. Create a helper function `sanitizeURL(rawURL string) string` in `internal/providers/helpers.go` to strip or replace sensitive query parameters (`key=...`).
+  2. Replace all instances of raw URL printing in `gemini.go` (L378, L636) with `log.Printf("[Gemini] POST %s", sanitizeURL(url))`.
+
+### Task 1.2: Add Thread Safety to Server Providers Map
+- **Target File:** [`internal/server/server.go`](file:///home/newkirk/Projects/gophergate/internal/server/server.go#L30)
+- **Problem:** `s.providers` map is mutated during `RefreshProviders()` without mutex locking while HTTP handlers read from it concurrently (`selectProvider()`).
+- **Action Items:**
+  1. Add `providersMu sync.RWMutex` field to `Server` struct in `server.go`.
+  2. Acquire `s.providersMu.Lock()` before modifying `s.providers` in `RefreshProviders()`.
+  3. Acquire `s.providersMu.RLock()` in `selectProvider()` and deferred `s.providersMu.RUnlock()`.
+
+---
+
+## Phase 2: Thread-Safety & Race Condition Hardening
+
+### Task 2.1: Synchronize `ModelRegistry` Lookups and Reloads
+- **Target Files:** [`internal/models/registry.go`](file:///home/newkirk/Projects/gophergate/internal/models/registry.go#L24), [`internal/server/server.go`](file:///home/newkirk/Projects/gophergate/internal/server/server.go#L56)
+- **Problem:** Background goroutines update `s.registry` while concurrent requests access `r.FindModel()`.
+- **Action Items:**
+  1. Add `sync.RWMutex` to `ModelRegistry` struct in `registry.go`.
+  2. Wrap `FindModel()` methods with `r.mu.RLock()` and `r.mu.RUnlock()`.
+  3. Alternatively, implement `atomic.Pointer[models.ModelRegistry]` in `Server` to enable lock-free atomic pointer swaps on registry refresh.
+
+### Task 2.2: Synchronize `Router.Reload()`
+- **Target File:** [`internal/router/router.go`](file:///home/newkirk/Projects/gophergate/internal/router/router.go#L34)
+- **Problem:** `Router.Reload()` replaces `r.groups` map without holding a mutex lock during active routing requests.
+- **Action Items:**
+  1. Add `mu sync.RWMutex` to `Router` struct in `router.go`.
+  2. Acquire `r.mu.RLock()` during `Route()`, `RouteToConcrete()`, and `IsGroup()`.
+  3. Acquire `r.mu.Lock()` during `Reload()`.
+
+---
+
+## Phase 3: Reliability & Stream Fault Tolerance
+
+### Task 3.1: Enable Circuit Breaker Protection for Streaming Endpoints
+- **Target File:** [`internal/providers/circuit_breaker.go`](file:///home/newkirk/Projects/gophergate/internal/providers/circuit_breaker.go#L52)
+- **Problem:** `ChatCompletionStream` and `ResponsesStream` bypass `gobreaker` entirely.
+- **Action Items:**
+  1. Wrap the initial `ChatCompletionStream` call inside `cb.Execute()`.
+  2. Implement stream response wrapper that monitors streaming errors and reports failure back to the circuit breaker state tracker.
+
+### Task 3.2: Prevent Goroutine Leaks on Client Disconnect
+- **Target Files:** [`internal/providers/helpers.go`](file:///home/newkirk/Projects/gophergate/internal/providers/helpers.go#L379), [`internal/providers/deepseek.go`](file:///home/newkirk/Projects/gophergate/internal/providers/deepseek.go#L226)
+- **Problem:** Scanner loops push chunks via `ch <- chunk` without selecting on `ctx.Done()`.
+- **Action Items:**
+  1. Update channel writes in `StreamOpenAI` and `StreamGemini`:
+     ```go
+     select {
+     case ch <- chunk:
+     case <-ctx.Done():
+         return
+     }
+     ```
+
+---
+
+## Phase 4: Error Handling & Security Hygiene
+
+### Task 4.1: Fix Resty Response Body Draining in Error Handlers
+- **Target Files:** `internal/providers/gemini.go`, `deepseek.go`, `xiaomi.go`
+- **Problem:** Code attempts `io.ReadAll(resp.RawBody())` on consumed bodies.
+- **Action Items:**
+  1. Replace `io.ReadAll(resp.RawBody())` with `resp.Body()` or `resp.String()`.
+
+### Task 4.2: Handle Database Error Returns
+- **Target Files:** [`internal/server/users.go`](file:///home/newkirk/Projects/gophergate/internal/server/users.go#L73), [`internal/server/clients.go`](file:///home/newkirk/Projects/gophergate/internal/server/clients.go#L165)
+- **Problem:** Errors returned by `database.Exec()` are ignored in user and client token updates.
+- **Action Items:**
+  1. Check and log/return HTTP error responses for all `database.Exec()` calls.
+
+### Task 4.3: Mask Auth Tokens in System Settings API
+- **Target File:** [`internal/server/system.go`](file:///home/newkirk/Projects/gophergate/internal/server/system.go#L81)
+- **Problem:** `/api/system/settings` returns unmasked `auth_tokens`.
+- **Action Items:**
+  1. Mask static API tokens before returning in JSON output (e.g. `sk-***1234`).
+
+---
+
+## Verification & Validation Plan
+
+1. **Unit Testing:** Run `go test -v -race ./...` to verify zero data races.
+2. **Integration Verification:** Run test completions across all providers (`openai`, `gemini`, `deepseek`, `grok`, `ollama`) to confirm streaming and non-streaming responses work correctly.
+3. **Security Audit:** Verify log output contains no plain-text API keys or tokens.
@@ -0,0 +1,142 @@
+# GopherGate Comprehensive Code Review & Architecture Report
+
+**Date:** July 21, 2026  
+**Target Repository:** `LobotomyLabs/GopherGate`  
+**Language:** Go (1.23+) & JavaScript (Vanilla Frontend)  
+**Status:** Completed  
+
+---
+
+## 1. Executive Summary
+
+A comprehensive code review and architectural analysis of the **GopherGate** unified LLM proxy gateway codebase was conducted. The application is written in Go using `gin-gonic/gin`, `sqlx`, `sqlite`, `gobreaker`, and `resty`. 
+
+Overall, the codebase is well-structured, modular, and cleanly separated into logical packages (`cmd`, `internal/config`, `internal/db`, `internal/middleware`, `internal/models`, `internal/providers`, `internal/router`, `internal/server`, `internal/utils`). 
+
+However, several critical and high-severity security, concurrency, and reliability issues were uncovered during the review. Addressing these items will ensure production-grade security, thread safety, and resilience.
+
+---
+
+## 2. Findings Summary Matrix
+
+| ID | Category | Severity | Component | Issue Description |
+|---|---|---|---|---|
+| **SEC-01** | Security | 🔴 **Critical** | `internal/providers/gemini.go` | API Key exposure in plain-text debug log outputs (`?key=...`) |
+| **CONC-01** | Concurrency | 🟠 **High** | `internal/server/server.go` | Un-mutexed map access to `s.providers` during runtime updates |
+| **CONC-02** | Concurrency | 🟠 **High** | `internal/models/registry.go` | Data race on `ModelRegistry` map during concurrent model lookups |
+| **CONC-03** | Concurrency | 🟠 **High** | `internal/router/router.go` | Unsynchronized map replacement in `Router.Reload()` |
+| **REL-01** | Reliability | 🟠 **High** | `internal/providers/circuit_breaker.go` | Circuit breaker bypassed for all streaming completions |
+| **REL-02** | Reliability | 🟡 **Medium** | `internal/providers/helpers.go` | Goroutine leaks on stream disconnect (missing `ctx.Done()` check) |
+| **ERR-01** | Error Handling | 🟡 **Medium** | `internal/providers/*.go` | Empty error strings due to reading consumed `resp.RawBody()` |
+| **SEC-02** | Security | 🟡 **Medium** | `internal/server/users.go` | Swallowed DB error outputs on user updates & default credentials |
+| **SEC-03** | Security | 🔵 **Low** | `internal/server/system.go` | Raw auth tokens returned in settings API response |
+| **CODE-01**| Code Quality | 🔵 **Low** | `internal/server/logging.go` | Fixed channel buffer (100) drops log entries under load |
+
+---
+
+## 3. Detailed Findings & Remediation Guidelines
+
+### 🔴 SEC-01: Plain-Text API Key Exposure in Logs
+- **Location:** [`internal/providers/gemini.go:L378`](file:///home/newkirk/Projects/gophergate/internal/providers/gemini.go#L378), [`L636`](file:///home/newkirk/Projects/gophergate/internal/providers/gemini.go#L636)
+- **Impact:** Gemini REST endpoints accept authentication via query parameter `?key=YOUR_API_KEY`. The provider prints debugging information using `fmt.Printf("[Gemini] POST %s\n", url)`. This logs live API keys directly into server stdout / log aggregators.
+- **Remediation:** Remove plain-text URL prints or sanitize query parameters before logging:
+  ```go
+  // Sanitize key query parameter before logging
+  sanitizedURL := regexp.MustCompile(`key=[^&]+`).ReplaceAllString(url, "key=REDACTED")
+  log.Printf("[Gemini] POST %s", sanitizedURL)
+  ```
+
+---
+
+### 🟠 CONC-01: Concurrent Map Mutation on `s.providers`
+- **Location:** [`internal/server/server.go:L146-L185`](file:///home/newkirk/Projects/gophergate/internal/server/server.go#L146-L185)
+- **Impact:** During `RefreshProviders()` (triggered by background initialization or via `/api/providers/:name` admin updates), `delete(s.providers, id)` and `s.providers[id] = ...` mutate `s.providers` without holding a write lock. Concurrent HTTP requests accessing `s.selectProvider()` or `handleChatCompletions()` read from `s.providers`, causing a Go runtime fatal map panic (`fatal error: concurrent map read and map write`).
+- **Remediation:** Guard `s.providers` with a `sync.RWMutex`:
+  ```go
+  s.providersMu.Lock()
+  s.providers[id] = providers.NewCircuitBreakerProvider(p)
+  s.providersMu.Unlock()
+  ```
+
+---
+
+### 🟠 CONC-02: Data Race on `ModelRegistry` Dynamic Updates
+- **Location:** [`internal/models/registry.go:L175-L211`](file:///home/newkirk/Projects/gophergate/internal/models/registry.go#L175-L211) & [`internal/server/server.go:L56-L65`](file:///home/newkirk/Projects/gophergate/internal/server/server.go#L56-L65)
+- **Impact:** The server fetches `models.dev` in a background goroutine and overwrites `s.registry`. However, `ModelRegistry.FindModel()` traverses internal maps (`r.Providers`) without acquiring read locks. If a lookup occurs while `s.registry` or nested provider maps are being updated, a data race or nil-pointer dereference will occur.
+- **Remediation:** Protect registry lookups with `RWMutex` locks, or use `atomic.Pointer[models.ModelRegistry]` for lock-free hot swapping.
+
+---
+
+### 🟠 CONC-03: Unsynchronized `Router.Reload()`
+- **Location:** [`internal/router/router.go:L135-L140`](file:///home/newkirk/Projects/gophergate/internal/router/router.go#L135-L140)
+- **Impact:** Calling `r.Reload(groups)` instantiates a new `r.groups = make(...)` map directly on the existing `Router` struct while active HTTP requests are concurrently calling `r.IsGroup()` or `r.Route()`.
+- **Remediation:** Add a `sync.RWMutex` to `Router` and acquire `RLock()` during `Route()` / `IsGroup()` and `Lock()` during `Reload()`.
+
+---
+
+### 🟠 REL-01: Circuit Breaker Bypassed for Streaming Requests
+- **Location:** [`internal/providers/circuit_breaker.go:L52-L56`](file:///home/newkirk/Projects/gophergate/internal/providers/circuit_breaker.go#L52-L56), [`L78-L81`](file:///home/newkirk/Projects/gophergate/internal/providers/circuit_breaker.go#L78-L81)
+- **Impact:** Streaming methods (`ChatCompletionStream` and `ResponsesStream`) bypass `gobreaker` execution logic entirely. Upstream provider outages during streaming requests will not trigger the circuit breaker, leaving backends vulnerable to request flooding and thread exhaustion.
+- **Remediation:** Execute initial connection setup inside `cb.Execute()` and propagate stream channel failure signals back to the circuit breaker.
+
+---
+
+### 🟡 REL-02: Streaming Goroutine Leaks on Client Disconnect
+- **Location:** [`internal/providers/helpers.go:L379-L396`](file:///home/newkirk/Projects/gophergate/internal/providers/helpers.go#L379-L396), [`L474-L516`](file:///home/newkirk/Projects/gophergate/internal/providers/helpers.go#L474-L516)
+- **Impact:** In `StreamOpenAI` and `StreamGemini`, chunk forwarding goroutines execute `ch <- chunk` without selecting on `ctx.Done()`. If a client disconnects prematurely, writing to `ch` blocks indefinitely, causing goroutine leaks.
+- **Remediation:** Use select statement for channel writes:
+  ```go
+  select {
+  case ch <- chunk:
+  case <-ctx.Done():
+      return
+  }
+  ```
+
+---
+
+### 🟡 ERR-01: Empty Provider Error Messages
+- **Location:** [`internal/providers/gemini.go:L130`](file:///home/newkirk/Projects/gophergate/internal/providers/gemini.go#L130), [`deepseek.go:L115`](file:///home/newkirk/Projects/gophergate/internal/providers/deepseek.go#L115), [`xiaomi.go:L56`](file:///home/newkirk/Projects/gophergate/internal/providers/xiaomi.go#L56)
+- **Impact:** Non-streaming error handling reads `io.ReadAll(resp.RawBody())` on `resty.Response`. Resty auto-drains and closes `RawBody()` on request completion unless configured otherwise, causing `io.ReadAll` to return an empty slice and masking upstream API errors.
+- **Remediation:** Use `resp.Body()` or `resp.String()` instead of `resp.RawBody()`.
+
+---
+
+### 🟡 SEC-02: Swallowed Database Errors & Default Credentials
+- **Location:** [`internal/server/users.go:L73-L84`](file:///home/newkirk/Projects/gophergate/internal/server/users.go#L73-L84), [`internal/db/db.go:L179-L195`](file:///home/newkirk/Projects/gophergate/internal/db/db.go#L179-L195)
+- **Impact:** `handleUpdateUser` ignores error outputs from `s.database.Exec()`. Database update failures (e.g., constraint violations or locks) return HTTP 200 OK. Furthermore, initial setup seeds default admin credentials (`admin` / `admin123`).
+- **Remediation:** Always capture and handle `err` from `db.Exec()`. Require explicit password set on initial setup.
+
+---
+
+### 🔵 SEC-03: Sensitive Auth Tokens Exposed in Settings API
+- **Location:** [`internal/server/system.go:L81`](file:///home/newkirk/Projects/gophergate/internal/server/system.go#L81)
+- **Impact:** `/api/system/settings` includes the raw static `auth_tokens` slice from server config in the JSON response payload.
+- **Remediation:** Omit raw tokens from settings response or return masked representations (e.g., `["sk-***..."]`).
+
+---
+
+### 🔵 CODE-01: Fixed Request Logger Channel Buffer
+- **Location:** [`internal/server/logging.go:L39`](file:///home/newkirk/Projects/gophergate/internal/server/logging.go#L39)
+- **Impact:** The request logging channel is initialized with a capacity of 100 (`make(chan RequestLog, 100)`). High request concurrency causes non-blocking sends to drop analytics log entries silently.
+- **Remediation:** Increase channel capacity or introduce a dynamic ring buffer worker pool.
+
+---
+
+## 4. Verification & Testing Summary
+
+All existing unit tests in the codebase pass cleanly:
+```bash
+go test -cover ./...
+```
+- `gophergate/internal/models`: **61.2%** test coverage
+- `gophergate/internal/router`: **45.3%** test coverage
+- Static analysis check (`go vet ./...`): **Clean** (0 warnings)
+
+---
+
+## 5. Next Steps & Recommendations
+
+1. **Immediate Patch:** Sanitize logging in `gemini.go` (SEC-01) and add `sync.RWMutex` to `s.providers` (CONC-01).
+2. **Concurrency Audit:** Implement atomic/mutex locking for `ModelRegistry` (CONC-02) and `Router.Reload()` (CONC-03).
+3. **Resilience Patch:** Add `ctx.Done()` checks in streaming loops (REL-02) and wrap initial stream calls in `gobreaker` (REL-01).
@@ -375,7 +375,7 @@ func (p *GeminiProvider) ChatCompletion(ctx context.Context, req *models.Unified
 	}

 	url := fmt.Sprintf("%s/models/%s:generateContent?key=%s", baseURL, req.Model, p.apiKey)
-	fmt.Printf("[Gemini] POST %s\n", url)
+	fmt.Printf("[Gemini] POST %s\n", SanitizeURL(url))

 	resp, err := p.client.R().
 		SetContext(ctx).
@@ -633,7 +633,7 @@ func (p *GeminiProvider) ChatCompletionStream(ctx context.Context, req *models.U

 	// Use streamGenerateContent for streaming
 	url := fmt.Sprintf("%s/models/%s:streamGenerateContent?key=%s", baseURL, req.Model, p.apiKey)
-	fmt.Printf("[Gemini-Stream] POST %s\n", url)
+	fmt.Printf("[Gemini-Stream] POST %s\n", SanitizeURL(url))

 	resp, err := p.client.R().
 		SetContext(ctx).
@@ -5,11 +5,20 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"regexp"
 	"strings"

 	"gophergate/internal/models"
 )

+var keySanitizeRegex = regexp.MustCompile(`(?i)(key|api_key|secret)=[^&]+`)
+
+// SanitizeURL strips sensitive key query parameters from URLs before logging.
+func SanitizeURL(rawURL string) string {
+	return keySanitizeRegex.ReplaceAllString(rawURL, "$1=REDACTED")
+}
+
+
 func sanitizeFunctionName(name string) string {
 	var sb strings.Builder
 	for _, ch := range name {
@@ -82,7 +82,7 @@ func (s *Server) handleGetProviders(c *gin.Context) {

 		status := "disabled"
 		if enabled {
-			if _, ok := s.providers[id]; ok {
+			if _, ok := s.getProvider(id); ok {
 				status = "online"
 			} else {
 				status = "error"
@@ -203,7 +203,7 @@ func (s *Server) handleUpdateProvider(c *gin.Context) {

 func (s *Server) handleTestProvider(c *gin.Context) {
 	name := c.Param("name")
-	provider, ok := s.providers[name]
+	provider, ok := s.getProvider(name)
 	if !ok {
 		c.JSON(http.StatusNotFound, ErrorResponse(fmt.Sprintf("Provider %s not found or not enabled", name)))
 		return
@@ -24,15 +24,16 @@ import (
 )

 type Server struct {
-	router     *gin.Engine
-	cfg        *config.Config
-	database   *db.DB
-	providers  map[string]providers.Provider
-	sessions   *SessionManager
-	hub        *Hub
-	logger     *RequestLogger
-	registry   *models.ModelRegistry
-	registryMu sync.RWMutex
+	router      *gin.Engine
+	cfg         *config.Config
+	database    *db.DB
+	providers   map[string]providers.Provider
+	providersMu sync.RWMutex
+	sessions    *SessionManager
+	hub         *Hub
+	logger      *RequestLogger
+	registry    *models.ModelRegistry
+	registryMu  sync.RWMutex
 	modelRouter *router.Router
 }

@@ -88,6 +89,8 @@ func (s *Server) RefreshProviders() error {
 		dbMap[cfg.ID] = cfg
 	}

+	newProviders := make(map[string]providers.Provider)
+
 	providerIDs := []string{"openai", "gemini", "deepseek", "moonshot", "grok", "ollama", "xiaomi"}
 	for _, id := range providerIDs {
 		// Default values from config
@@ -143,7 +146,6 @@ func (s *Server) RefreshProviders() error {
 		}

 		if !enabled {
-			delete(s.providers, id)
 			continue
 		}

@@ -181,10 +183,14 @@ func (s *Server) RefreshProviders() error {
 		}

 		if p != nil {
-			s.providers[id] = providers.NewCircuitBreakerProvider(p)
+			newProviders[id] = providers.NewCircuitBreakerProvider(p)
 		}
 	}

+	s.providersMu.Lock()
+	s.providers = newProviders
+	s.providersMu.Unlock()
+
 	s.refreshRouter()
 	return nil
 }
@@ -368,7 +374,7 @@ func (s *Server) handleResponses(c *gin.Context) {
 		providerName = "ollama"
 	}

-	provider, ok := s.providers[providerName]
+	provider, ok := s.getProvider(providerName)
 	if !ok {
 		c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("Provider %s not enabled or supported", providerName)})
 		return
@@ -520,6 +526,13 @@ func (s *Server) handleListModels(c *gin.Context) {
 	})
 }

+func (s *Server) getProvider(name string) (providers.Provider, bool) {
+	s.providersMu.RLock()
+	defer s.providersMu.RUnlock()
+	p, ok := s.providers[name]
+	return p, ok
+}
+
 func (s *Server) selectProvider(modelID string) (providers.Provider, string, error) {
 	providerName := "openai" // default
 	modelLower := strings.ToLower(modelID)
@@ -546,7 +559,7 @@ func (s *Server) selectProvider(modelID string) (providers.Provider, string, err
 		providerName = "xiaomi"
 	}

-	p, ok := s.providers[providerName]
+	p, ok := s.getProvider(providerName)
 	if !ok {
 		return nil, "", fmt.Errorf("Provider %s not enabled or supported", providerName)
 	}