diff --git a/VERSION b/VERSION index 85582af..ed4c184 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.21.6 \ No newline at end of file +0.21.7 \ No newline at end of file diff --git a/docs/AUDIT-0.22.0-PREFLIGHT.md b/docs/AUDIT-0.22.0-PREFLIGHT.md new file mode 100644 index 0000000..245926f --- /dev/null +++ b/docs/AUDIT-0.22.0-PREFLIGHT.md @@ -0,0 +1,302 @@ +# Pre-Flight Audit — v0.22.0 Kickoff + +**Date:** 2026-03-01 +**Current version:** 0.21.6 +**Test instance:** `switchboard-be-test` in `gobha-ai-chat` namespace +**Schema:** `012_v0214_git.sql` — up to date + +--- + +## 1. Bug: Channel List Corruption (SafeJSON 500) + +### Symptoms + +``` +23:24:44 ⚠ SafeJSON: marshal failed: json: error calling MarshalJSON for type + json.RawMessage: invalid character 'i' looking for beginning of value +23:24:44 | 500 | GET "/test/api/v1/channels?page=1&per_page=100&type=direct" +``` + +One channel in the test database has invalid JSON in its `settings` column. The +content starts with the character `i` (likely a bare word like `invalid`, +`inactive`, or `in_progress` — not valid JSON). This kills the entire paginated +channel list with a 500. + +### Root Cause Analysis + +**Defense layer 1 — `scanJSON`**: The `ListChannels` scan path at +`channels.go:236` correctly uses `scanJSON(&ch.Settings)`, which calls +`json.Valid()` and defaults to `{}` on failure. This should catch corrupt data. + +**Defense layer 2 — `SafeJSON`**: Pre-marshals the entire response and returns +a clean 500 instead of truncated output. This layer IS firing, meaning layer 1 +did NOT catch the problem. + +**Inconsistent scan paths**: The Postgres `CreateChannel` RETURNING path at +`channels.go:323` scans `&ch.Settings` directly WITHOUT `scanJSON`: + +```go +// line 323 — Postgres CreateChannel +pq.Array(&tags), &ch.Settings, &ch.CreatedAt, &ch.UpdatedAt, +``` + +Compare to the SQLite path at line 306 which correctly uses `scanJSON`. + +**How the data got corrupted**: Unknown without DB access. Postgres JSONB +columns reject invalid JSON at write time (`::jsonb` cast), so the corruption +likely predates the current schema — possibly from a migration, manual DB edit, +or an older code version that used TEXT instead of JSONB. Run this to find it: + +```sql +SELECT id, title, settings +FROM channels +WHERE user_id = '664c71e8-cd8b-47fb-b421-9c48abb815ba' + AND settings::text NOT LIKE '{%' + AND settings::text NOT LIKE '[%' + AND settings::text NOT LIKE '"null"'; +``` + +### Fixes Required + +**P0 — Fix the data** (immediate): +```sql +UPDATE channels +SET settings = '{}'::jsonb +WHERE settings IS NULL + OR NOT (settings::text ~ '^[\[{"]'); +``` + +**P0 — Consistent scanJSON usage** (`channels.go:323`): +```go +// Before (Postgres CreateChannel): +pq.Array(&tags), &ch.Settings, &ch.CreatedAt, &ch.UpdatedAt, + +// After: +pq.Array(&tags), scanJSON(&ch.Settings), &ch.CreatedAt, &ch.UpdatedAt, +``` + +**P1 — Graceful list degradation**: When `SafeJSON` encounters a corrupt item +in a paginated list, the current behavior drops the entire response. Instead, +add a `SafeMarshalList` helper that: +1. Marshals items individually +2. Skips corrupt items with a warning log (include the row ID) +3. Returns the healthy items with a `warnings` field in the response + +**P1 — Input validation on settings write** (`channels.go:487`): +```go +if req.Settings != nil { + if !json.Valid([]byte(*req.Settings)) { + c.JSON(http.StatusBadRequest, gin.H{"error": "settings must be valid JSON"}) + return + } + // ... existing merge logic +} +``` + +**P2 — Audit all RawMessage scan sites**: Search for `&ch.Settings`, +`&*.Settings`, or any `json.RawMessage` scan that doesn't go through +`scanJSON`. Every JSONB column scanned into `json.RawMessage` must use the +wrapper. + +### Additional Log Noise + +The git endpoints are returning 400s for workspaces without git repos: + +``` +23:23:40 | 400 | GET ".../git/status" +23:23:56 | 400 | GET ".../git/branches" +``` + +These are expected (workspace has no `.git` directory), but the frontend should +suppress these calls when the workspace has no `git_remote_url` configured. +Frontend fix: check `workspace.git_remote_url` before calling git endpoints. + +--- + +## 2. CI Variables → Deployment Audit + +Cross-referencing CI secrets/variables (from Gitea) against `backend.yaml`, +`config.go`, and runtime logs. + +### Secrets (Gitea Secrets → K8s Secrets) + +| CI Secret | K8s Secret | Key | Status | +|-----------|------------|-----|--------| +| ENCRYPTION_KEY | switchboard-encryption | ENCRYPTION_KEY | ✅ Active | +| POSTGRES_USER | switchboard-db-credentials | POSTGRES_USER | ✅ Active | +| POSTGRES_PASSWORD | switchboard-db-credentials | POSTGRES_PASSWORD | ✅ Active | +| POSTGRES_ADMIN_USER | — | — | ⚠ Unused by app — likely for CI DB bootstrap | +| POSTGRES_ADMIN_PASSWORD | — | — | ⚠ Unused by app — likely for CI DB bootstrap | +| S3_ACCESS_KEY | switchboard-s3 | S3_ACCESS_KEY | ✅ Active | +| S3_SECRET_KEY | switchboard-s3 | S3_SECRET_KEY | ✅ Active | +| SWITCHBOARD_ADMIN_EMAIL | switchboard-admin | email | ✅ Active | +| SWITCHBOARD_ADMIN_PASSWORD | switchboard-admin | password | ✅ Active | +| SWITCHBOARD_ADMIN_USERNAME | switchboard-admin | username | ✅ Active | + +### Variables (Gitea Variables → envsubst / K8s env) + +| CI Variable | Used In | Value | Status | +|-------------|---------|-------|--------| +| DOMAIN | ingress.yaml | gobha.ai | ✅ | +| NAMESPACE | all manifests | gobha-ai-chat | ✅ | +| POSTGRES_HOST | backend.yaml env | postgres-primary.postgres.svc.cluster.local | ✅ | +| PROVIDER | ? | venice | ⚠ **Orphan** — not referenced in any manifest or config.go | +| S3_BUCKET | switchboard-s3 secret | integration-test | ✅ | +| S3_ENDPOINT | switchboard-s3 secret | http://192.168.10.29:7480 | ✅ | +| SEED_PROVIDERS | backend.yaml env (direct) | venice:...:micro | ✅ New (Mar 1) | +| SEED_USERS | switchboard-seed-users secret | darthvader:...,lukeskywalker:... | ✅ | +| STORAGE_BACKEND | backend.yaml env | s3 | ✅ | +| STORAGE_CLASS | storage-pvc.yaml | cephfs | ✅ | +| STORAGE_SIZE | storage-pvc.yaml | (visible, value cut off) | ✅ | + +### Variables Expected by backend.yaml but NOT in CI Screenshots + +These are likely set in the CI pipeline YAML directly (not as repo variables): + +| Variable | Used In | Expected Source | +|----------|---------|-----------------| +| ENVIRONMENT | backend.yaml env | Pipeline per-branch | +| GIN_MODE | backend.yaml env | Pipeline (release/debug) | +| BASE_PATH | backend.yaml env | Pipeline per-deploy (/test, /prod, etc.) | +| POSTGRES_PORT | backend.yaml env | Default 5432 (config.go fallback) | +| DB_NAME | backend.yaml env | Pipeline per-deploy | +| DEPLOY_SUFFIX | all manifests | Pipeline (-test, -prod, etc.) | +| BE_IMAGE | backend.yaml image | Pipeline / registry | +| IMAGE_TAG | backend.yaml image | Pipeline / git SHA | +| BE_REPLICAS | backend.yaml replicas | Pipeline | +| BE_MEMORY_REQUEST | backend.yaml resources | Pipeline | +| BE_MEMORY_LIMIT | backend.yaml resources | Pipeline | +| BE_CPU_REQUEST | backend.yaml resources | Pipeline | +| BE_CPU_LIMIT | backend.yaml resources | Pipeline | +| EXTRACTION_CONCURRENCY | backend.yaml env | Pipeline (default 3) | + +### Issues Found + +1. **`PROVIDER` variable is orphaned.** Value is "venice" but nothing in + backend.yaml or config.go references it. Likely a legacy variable from + before `SEED_PROVIDERS` was added. Clean up or document. + +2. **`SEED_PROVIDERS` contains an API key in a CI Variable (not Secret).** + The screenshot shows the Venice API key visible in plaintext in the Variables + section. This should be moved to Gitea Secrets for proper masking. The + backend.yaml needs a corresponding mapping (currently SEED_PROVIDERS may be + injected via envsubst or direct env, verify the CI pipeline). + +3. **`SEED_USERS` passwords are plaintext in CI.** `password1234` for both + test users. Acceptable for test environment but should be documented as + test-only. Production pipeline should NOT have SEED_USERS set at all (and + `SeedProviders` already blocks production). + +4. **Missing `SEED_PROVIDERS` in backend.yaml env block.** The K8s manifest + doesn't have a `SEED_PROVIDERS` env var entry. It works because the CI + pipeline likely injects it via envsubst into a direct env entry, but this + should be made explicit in backend.yaml for documentation parity. + +5. **No `JWT_SECRET` in CI secrets.** Config.go defaults to + `"dev-secret-change-me"`. The test instance may be running with this + default. Add a proper secret for test environments. + +6. **No `S3_REGION` or `S3_PREFIX` in CI.** Config.go defaults to + `us-east-1` and `""` respectively. Fine for Ceph RGW but should be + documented. + +### Recommendations + +- Move `SEED_PROVIDERS` from CI Variable to CI Secret +- Remove or document the orphaned `PROVIDER` variable +- Add `JWT_SECRET` to CI Secrets and wire through backend.yaml +- Add `SEED_PROVIDERS` env entry to backend.yaml (with secretKeyRef) +- Add a CI pipeline comment documenting which variables are set inline + vs. in repo settings + +--- + +## 3. Test Hardening — Unhappy Paths + +Current test coverage is strong on the happy path (164+ integration tests). +The corruption bug exposes gaps in unhappy-path coverage. + +### Missing Test Categories + +**A. Data corruption resilience** (integration_test.go): + +- [ ] **Corrupt JSONB in channels**: INSERT a channel with valid settings, then + UPDATE settings directly via SQL with invalid text. Verify GET /channels + still returns 200 with the corrupt channel included (settings defaulted to + `{}`). +- [ ] **Corrupt JSONB in projects**: Same pattern for project settings. +- [ ] **Corrupt JSONB in teams**: Same for team settings/policies. +- [ ] **NULL settings column**: Verify channels with NULL settings serialize + correctly (should be `{}` or `null`, not crash). +- [ ] **Corrupt tags column**: Insert channel with malformed tags array. + Verify list doesn't 500. + +**B. Settings write validation**: + +- [ ] **Invalid JSON in channel settings update**: PUT /channels/:id with + `{"settings": "not-json"}` — should 400, not write to DB. +- [ ] **Oversized settings**: PUT with a 10MB settings blob — should be + rejected. +- [ ] **SQL injection via settings merge**: PUT with settings containing SQL + metacharacters — verify parameterized query prevents injection. + +**C. Scan alignment tests** (safe_json_test.go): + +- [ ] **Column count mismatch**: Mock a query result with wrong column count, + verify clean error instead of panic. +- [ ] **Type mismatch**: Scan a TEXT column into json.RawMessage without + scanJSON — verify SafeJSON catches it. +- [ ] **Driver buffer reuse**: Scan multiple rows with RawMessage, verify each + row's data is independent (copy, not reference). + +**D. Startup resilience** (config/seed tests): + +- [ ] **SEED_PROVIDERS with invalid format**: `"openai"` (no key), `":"`, + `":::"`, empty string — verify graceful skip. +- [ ] **SEED_PROVIDERS with unknown provider**: `"unknown:key123"` — verify + warning log, not crash. +- [ ] **SEED_USERS with invalid format**: Malformed CSV — verify skip. +- [ ] **DATABASE_URL with unreachable host**: Verify startup logs error and + serves 503 on API endpoints. + +**E. WebSocket reconnection**: + +- [ ] **Rapid disconnect/reconnect**: The logs show multiple ws connect/ + disconnect cycles in quick succession. Verify no goroutine leaks, + no duplicate event delivery. +- [ ] **Auth token expiry during active connection**: Verify clean disconnect + with appropriate error, not silent data loss. + +**F. Frontend error handling** (manual or e2e): + +- [ ] **500 on channel list**: Frontend should show error state, not blank + screen. Currently unknown behavior. +- [ ] **400 on git endpoints for non-git workspace**: Frontend should not + call git endpoints when workspace has no git_remote_url. +- [ ] **Network timeout during streaming completion**: Verify partial + response is preserved, user can retry. + +### Test Infrastructure Needed + +1. **SQL fixture injection**: Helper function to INSERT corrupt data directly + via SQL, bypassing store layer validation. For corruption resilience tests. + +2. **SafeJSON audit script**: `grep -rn '&.*\.Settings\|json.RawMessage'` to + find all RawMessage scan sites. Run as CI lint check to catch regressions. + +3. **Integration test for each migration**: Both fresh-install path AND + upgrade path from previous version. Currently tested manually — automate. + +--- + +## 4. Quick Wins Before v0.22.0 Work Starts + +Priority-ordered items that should land as v0.21.7 patches: + +1. **Fix corrupt data in test DB** — SQL UPDATE (5 min) +2. **Add scanJSON to CreateChannel Postgres path** — one-line fix (5 min) +3. **Add json.Valid check on settings write** — channels.go UpdateChannel (10 min) +4. **Move SEED_PROVIDERS to CI Secret** — CI UI change (5 min) +5. **Suppress git endpoint calls for non-git workspaces** — frontend (15 min) +6. **Add corruption resilience integration tests** — 2-3 tests (1 hr) +7. **Add JWT_SECRET to CI Secrets** — CI + backend.yaml (15 min) diff --git a/server/handlers/channels.go b/server/handlers/channels.go index 47fcbf9..d154883 100644 --- a/server/handlers/channels.go +++ b/server/handlers/channels.go @@ -320,7 +320,7 @@ func (h *ChannelHandler) CreateChannel(c *gin.Context) { ).Scan( &ch.ID, &ch.UserID, &ch.Title, &ch.Type, &ch.Description, &ch.Model, &ch.APIConfigID, &ch.SystemPrompt, &ch.IsArchived, &ch.IsPinned, &ch.Folder, &ch.ProjectID, &ch.WorkspaceID, - pq.Array(&tags), &ch.Settings, &ch.CreatedAt, &ch.UpdatedAt, + pq.Array(&tags), scanJSON(&ch.Settings), &ch.CreatedAt, &ch.UpdatedAt, ) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to create channel"}) @@ -478,6 +478,11 @@ func (h *ChannelHandler) UpdateChannel(c *gin.Context) { addClause("tags", writeTagsArg(req.Tags)) } if req.Settings != nil { + // Validate settings is well-formed JSON before writing + if !json.Valid([]byte(*req.Settings)) { + c.JSON(http.StatusBadRequest, gin.H{"error": "settings must be valid JSON"}) + return + } // JSONB merge: new settings keys overwrite existing, unmentioned keys preserved if database.IsSQLite() { setClauses = append(setClauses, "settings = json_patch(settings, ?)") diff --git a/server/handlers/safe_json.go b/server/handlers/safe_json.go index 23d3638..da935e6 100644 --- a/server/handlers/safe_json.go +++ b/server/handlers/safe_json.go @@ -46,7 +46,15 @@ func (s *jsonScanner) Scan(src interface{}) error { *s.dest = json.RawMessage("{}") return nil } - *s.dest = json.RawMessage(v) + // CRITICAL: copy the driver buffer. json.RawMessage(v) aliases the + // underlying []byte owned by database/sql. The driver may reuse that + // memory on the next rows.Scan() call, corrupting our data after the + // fact. This caused intermittent SafeJSON 500s on paginated list + // endpoints where row N's Settings pointed to row N+1's overwritten + // buffer. Fixed in v0.21.7. + cp := make([]byte, len(v)) + copy(cp, v) + *s.dest = json.RawMessage(cp) case string: b := []byte(v) if len(b) == 0 || !json.Valid(b) { diff --git a/server/handlers/safe_json_test.go b/server/handlers/safe_json_test.go index 16586c5..d7bcff2 100644 --- a/server/handlers/safe_json_test.go +++ b/server/handlers/safe_json_test.go @@ -183,3 +183,70 @@ func TestSafeJSON_PaginatedWithCorrupt(t *testing.T) { t.Fatalf("expected 500 for paginated response with corrupt item, got %d", w.Code) } } + +// ── Driver buffer aliasing tests ──────────── +// These verify the fix for the v0.21.7 corruption bug: scanJSON must +// copy []byte from the database driver, not alias it. Without the copy, +// rows.Next() can overwrite the buffer and corrupt previously-scanned +// json.RawMessage values in a paginated list. + +func TestScanJSON_ByteSliceNotAliased(t *testing.T) { + var dest json.RawMessage + s := scanJSON(&dest) + + // Simulate driver buffer with valid JSON + buf := []byte(`{"key":"value"}`) + if err := s.Scan(buf); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Simulate driver reusing the buffer on next rows.Scan() + copy(buf, []byte(`invalid_json!!`)) + + // dest must still hold the original valid value + if !json.Valid(dest) { + t.Fatalf("dest corrupted after source buffer reuse: %q", dest) + } + if string(dest) != `{"key":"value"}` { + t.Fatalf("expected original value, got %q", dest) + } +} + +func TestScanJSON_ByteSliceIsolation_MultiRow(t *testing.T) { + // Simulate scanning two rows through the same driver buffer, + // as happens in ListChannels when lib/pq reuses memory. + var dest1, dest2 json.RawMessage + + // Row 1 + buf := make([]byte, 64) + row1 := []byte(`{"row":1,"selector":"abc"}`) + copy(buf, row1) + s1 := scanJSON(&dest1) + if err := s1.Scan(buf[:len(row1)]); err != nil { + t.Fatalf("row 1 scan error: %v", err) + } + + // Row 2 — driver reuses same buffer + row2 := []byte(`{"row":2}`) + copy(buf, row2) + s2 := scanJSON(&dest2) + if err := s2.Scan(buf[:len(row2)]); err != nil { + t.Fatalf("row 2 scan error: %v", err) + } + + // Both must retain their original values + if string(dest1) != `{"row":1,"selector":"abc"}` { + t.Fatalf("dest1 corrupted by row 2 scan: %q", dest1) + } + if string(dest2) != `{"row":2}` { + t.Fatalf("dest2 unexpected value: %q", dest2) + } + + // Both must be independently valid JSON + if !json.Valid(dest1) { + t.Fatalf("dest1 not valid JSON after multi-row scan: %q", dest1) + } + if !json.Valid(dest2) { + t.Fatalf("dest2 not valid JSON after multi-row scan: %q", dest2) + } +}