diff --git a/ACTION_PLAN.md b/ACTION_PLAN.md new file mode 100644 index 00000000..7204217e --- /dev/null +++ b/ACTION_PLAN.md @@ -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. diff --git a/code_review.md b/code_review.md new file mode 100644 index 00000000..4e43a2eb --- /dev/null +++ b/code_review.md @@ -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). diff --git a/internal/providers/gemini.go b/internal/providers/gemini.go index 27454bc7..26b38fc7 100644 --- a/internal/providers/gemini.go +++ b/internal/providers/gemini.go @@ -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). diff --git a/internal/providers/helpers.go b/internal/providers/helpers.go index add460b5..05559070 100644 --- a/internal/providers/helpers.go +++ b/internal/providers/helpers.go @@ -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 { diff --git a/internal/server/providers_admin.go b/internal/server/providers_admin.go index d785cb72..feb40dec 100644 --- a/internal/server/providers_admin.go +++ b/internal/server/providers_admin.go @@ -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 diff --git a/internal/server/server.go b/internal/server/server.go index dc4db7d9..efa28325 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -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) }