newkirk
97 lines
5.5 KiB
Markdown
97 lines
5.5 KiB
Markdown
# 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.
|