Code review findings & feature request: Session config saving

[digiserg]

Summary

Comprehensive code review has identified 7 blocking, 5 high, 4 medium, and 8 nit-level issues. Additionally, a feature request for saving session/connection configurations.

---

🚨 Blocking Issues (Must Fix Before Merge)

B1. Build broken: Tests directory has colliding main packages

• Files: tests/producer.go, tests/consumer.go
• Issue: Both files declare package main, causing build failure
• Fix: Separate into tests/producer/main.go and tests/consumer/main.go

B2. Panic-prone bare type assertions in AI response parsing

• Files: pkg/ui/ai_assistant.go:629-632, 704-715, 784-787, 1629-1640
• Issue: All AI provider JSON parsing uses bare assertions without comma-ok pattern. Unexpected response structure → panic → TUI crash
• Example:

  firstChoice := choices[0].(map[string]interface{})  // panics if type wrong

• Fix: Use comma-ok pattern:

  firstChoice, ok := choices[0].(map[string]interface{})
  if !ok {
      return "", fmt.Errorf("unexpected response structure")
  }

B3. Gemini API key leaked in URL query parameter

• File: pkg/ui/ai_assistant.go:643
• Issue: API key embedded in URL: https://...?key=<KEY> → appears in proxy logs, error messages, debug output
• Fix: Use HTTP header x-goog-api-key instead of query parameter

B4. AI assistant executes destructive operations without confirmation

• File: pkg/ui/ai_assistant.go:1008-1790
• Issue: parseAndExecuteCommand directly mutates Kafka state (create topics, modify ALL topic configs, delete ACLs) based on AI-generated JSON. No user confirmation gate.
• Fix: Add confirmation dialog before any destructive operation

B5. os.Exit(0) in PreRunE callback

• File: cmd/kconduit/main.go:53
• Issue: Bypasses deferred cleanup in main. Use Cobra's built-in version support instead
• Fix: Use rootCmd.Version and rootCmd.SetVersionTemplate()

B6. Data race on topic cache

• File: pkg/kafka/client.go:188-216 (GetTopicDetails)
• Issue: c.topics and c.topicsLastFetched read/written without synchronization. Data race if multiple goroutines call this.
• Fix: Guard with sync.RWMutex

B7. Near-zero test coverage

• File: pkg/kafka/client_test.go (single 50-line test)
• Issue: No tests for Client methods, UI models, AI parsing/execution, message production/consumption
• Impact: Critical for a tool that executes Kafka mutations
• Fix: Add unit tests for client.go, create_topic, delete_topic, edit_config, ACL operations, AI command execution

---

🔴 Security Issues (High Priority)

S1. SASL password visible in process list

• File: cmd/kconduit/main.go:144
• Issue: --sasl-password flag visible in ps aux output
• Attack: Co-tenant reads /proc/<pid>/cmdline to harvest credentials
• Fix: Remove CLI flag; accept only via KCONDUIT_SASL_PASSWORD env var or --sasl-password-file

S2. TLS MinVersion not set (defaults to TLS 1.0)

• File: pkg/kafka/client.go:106-108
• Issue: Allows downgrade to TLS 1.0 (BEAST, POODLE vulnerabilities)
• Fix:

  tlsConf := &tls.Config{
      MinVersion: tls.VersionTLS12,
      InsecureSkipVerify: tlsConfig.InsecureSkipVerify,
  }

S3. InsecureSkipVerify has no runtime warning

• File: pkg/kafka/client.go:112
• Issue: Flag silently disables certificate validation; user may enable for testing and forget to remove
• Fix: Log prominent warning when enabled

S4. Ollama URL user-controlled with no validation (SSRF)

• File: pkg/ui/ai_assistant.go:146, 806
• Issue: OLLAMA_URL env var used directly in HTTP request: can point to internal services, cloud metadata endpoints
• Fix: Validate URL scheme (http/https only), reject loopback/link-local/RFC1918 unless explicitly allowed

S5. Unbounded io.ReadAll on AI responses

• Files: pkg/ui/ai_assistant.go:610, 684, 765, 825
• Issue: No size limit → OOM if provider returns large response
• Fix: Use io.LimitReader(resp.Body, 10*1024*1024) for 10MB max

---

🟠 Important Issues

I1. File sizes exceed maintainability threshold

• ai_assistant.go: 1790 lines (4 AI providers + command execution + rendering)
• model.go: 1447 lines (all tab views in one file)
• client.go: 1401 lines (client + ACLs + types)
• Fix: Split into separate files by responsibility

I2. parseAndExecuteCommand has extreme cyclomatic complexity

• File: pkg/ui/ai_assistant.go:1008-1790 (~780 lines, ~50+ cyclomatic complexity)
• Fix: Extract into dispatch table with per-command handler methods

I3. updateListView high cyclomatic complexity

• File: pkg/ui/model.go:292-773 (~480 lines)
• Fix: Extract tab key handling into handleBrokersTabKey(), handleTopicsTabKey(), etc.

I4. HTTP clients lack context.Context

• Files: pkg/ui/ai_assistant.go (all AI providers)
• Issue: User presses ESC but in-flight HTTP request blocks until timeout (~30-60s)
• Fix: Use http.NewRequestWithContext with cancellable context

I5. Global mutable logger singleton with TOCTOU race

• File: pkg/logger/logger.go
• Issue: Get() checks Log == nil without lock before Init. File handle leak if error occurs.
• Fix: Always call Init via sync.Once, close file handles on error

I6. Password capture race in AI command closure

• File: pkg/ui/ai_assistant.go:549-569
• Issue: processAIQuery captures pointer receiver m in closure running in separate goroutine while main loop modifies m
• Fix: Capture provider and config by value in closure

I7. Consumer goroutine leak potential

• File: pkg/ui/consumer.go:150-161
• Issue: Spawns goroutine in Cmd but immediately returns nil. No WaitGroup to verify exit.
• Fix: Track goroutines with sync.WaitGroup on model shutdown

I8. confluent-kafka-go unused in main code

• File: go.mod:11
• Issue: Pulls in librdkafka (C library, CGO requirement) only for test utilities in tests/
• Fix: Move test utilities to separate module or use build tags

---

📋 Feature Request: Session Config Saving

Goal: Allow users to save and reuse connection configurations instead of typing flags every time.

Design

Config file: ~/.config/kconduit/sessions.yaml (XDG-compliant)

sessions:
  local-dev:
    brokers: "localhost:9092"
    ai_engine: "ollama"
    ai_model: "llama3"

  staging-kafka:
    brokers: "broker1:9092,broker2:9092"
    sasl:
      enabled: true
      mechanism: "SCRAM-SHA-256"
      username: "admin"
      protocol: "SASL_SSL"
    tls:
      enabled: true
      ca_cert: "/path/to/ca.pem"

New CLI Flags

• --session <name> — load a named session
• --save-session <name> — save current flags as a session
• --list-sessions — list available sessions
• --delete-session <name> — remove a session

Security Constraints

• Passwords never stored in config file; use password_file: references or env vars
• File permissions checked (warn if > 0600)
• AI API keys stay as env vars only

Implementation

1. New pkg/config/config.go package with session load/save/list/delete
2. Integrate with Viper precedence: CLI flags > env vars > session config > defaults
3. Optional: TUI session picker at startup if no --session or --brokers provided

---

NITs

• N1: Unused isSearchResult variable in consumer.go:455-464
• N2: Naming cfgTlsEnabled should be cfgTLSEnabled (acronym convention)
• N3: Redundant _ = err after if err != nil check
• N4: Magic lipgloss color codes — use named constants
• N5: Import ordering inconsistency
• N6: topicSet computed but unused for deduplication in GetConsumerGroups
• N7: parseTimeToMilliseconds fragile on space-before-unit input
• N8: Client.Close() error formatting should use errors.Join (Go 1.20+)

---

Recommended Priority

1. Blocking issues (B1-B7) — prevents merge
2. Security issues (S1-S5) — blocks production use
3. Important issues (I1-I8) — affects maintainability/reliability
4. Feature — session config saving (nice-to-have)
5. NITs — low priority cleanup