From 78540b012f6ace2a377a00daadf05176d0335ce8 Mon Sep 17 00:00:00 2001
From: bbrodriges <bender.rodriges@gmail.com>
Date: Tue, 9 Sep 2025 16:47:53 +0300
Subject: [PATCH] security(sqlite): enable `secure_delete` pragma (#966)

Co-authored-by: Nuno Cruces <ncruces@users.noreply.github.com>
---
 internal/app/app.go             | 15 ++++++++++----
 internal/db/connect.go          | 36 +++++++++++++++++----------------
 internal/llm/agent/mcp-tools.go |  9 +++++++--
 3 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/internal/app/app.go b/internal/app/app.go
index 961ce5960e7d64e38c5d6548e881ed697f6283f9..cec42dc8610b4a6c72215766b7fd1c764381ef5a 100644
--- a/internal/app/app.go
+++ b/internal/app/app.go
@@ -50,7 +50,7 @@ type App struct {
 
 	// global context and cleanup functions
 	globalCtx    context.Context
-	cleanupFuncs []func()
+	cleanupFuncs []func() error
 }
 
 // New initializes a new applcation instance.
@@ -88,6 +88,9 @@ func New(ctx context.Context, conn *sql.DB, cfg *config.Config) (*App, error) {
 	// Initialize LSP clients in the background.
 	app.initLSPClients(ctx)
 
+	// cleanup database upon app shutdown
+	app.cleanupFuncs = append(app.cleanupFuncs, conn.Close)
+
 	// TODO: remove the concept of agent config, most likely.
 	if cfg.IsConfigured() {
 		if err := app.InitCoderAgent(); err != nil {
@@ -221,9 +224,10 @@ func (app *App) setupEvents() {
 	setupSubscriber(ctx, app.serviceEventsWG, "history", app.History.Subscribe, app.events)
 	setupSubscriber(ctx, app.serviceEventsWG, "mcp", agent.SubscribeMCPEvents, app.events)
 	setupSubscriber(ctx, app.serviceEventsWG, "lsp", SubscribeLSPEvents, app.events)
-	cleanupFunc := func() {
+	cleanupFunc := func() error {
 		cancel()
 		app.serviceEventsWG.Wait()
+		return nil
 	}
 	app.cleanupFuncs = append(app.cleanupFuncs, cleanupFunc)
 }
@@ -297,10 +301,11 @@ func (app *App) Subscribe(program *tea.Program) {
 
 	app.tuiWG.Add(1)
 	tuiCtx, tuiCancel := context.WithCancel(app.globalCtx)
-	app.cleanupFuncs = append(app.cleanupFuncs, func() {
+	app.cleanupFuncs = append(app.cleanupFuncs, func() error {
 		slog.Debug("Cancelling TUI message handler")
 		tuiCancel()
 		app.tuiWG.Wait()
+		return nil
 	})
 	defer app.tuiWG.Done()
 
@@ -350,7 +355,9 @@ func (app *App) Shutdown() {
 	// Call call cleanup functions.
 	for _, cleanup := range app.cleanupFuncs {
 		if cleanup != nil {
-			cleanup()
+			if err := cleanup(); err != nil {
+				slog.Error("Failed to cleanup app properly on shutdown", "error", err)
+			}
 		}
 	}
 }
diff --git a/internal/db/connect.go b/internal/db/connect.go
index 110c3e0f8805b218c0c71dc11ee284edc0a23fa0..bfe768c7ae9a399afd61a9d0692841fbacbe164c 100644
--- a/internal/db/connect.go
+++ b/internal/db/connect.go
@@ -7,7 +7,8 @@ import (
 	"log/slog"
 	"path/filepath"
 
-	_ "github.com/ncruces/go-sqlite3/driver"
+	"github.com/ncruces/go-sqlite3"
+	"github.com/ncruces/go-sqlite3/driver"
 	_ "github.com/ncruces/go-sqlite3/embed"
 
 	"github.com/pressly/goose/v3"
@@ -18,17 +19,6 @@ func Connect(ctx context.Context, dataDir string) (*sql.DB, error) {
 		return nil, fmt.Errorf("data.dir is not set")
 	}
 	dbPath := filepath.Join(dataDir, "crush.db")
-	// Open the SQLite database
-	db, err := sql.Open("sqlite3", dbPath)
-	if err != nil {
-		return nil, fmt.Errorf("failed to open database: %w", err)
-	}
-
-	// Verify connection
-	if err = db.PingContext(ctx); err != nil {
-		db.Close()
-		return nil, fmt.Errorf("failed to connect to database: %w", err)
-	}
 
 	// Set pragmas for better performance
 	pragmas := []string{
@@ -37,14 +27,25 @@ func Connect(ctx context.Context, dataDir string) (*sql.DB, error) {
 		"PRAGMA page_size = 4096;",
 		"PRAGMA cache_size = -8000;",
 		"PRAGMA synchronous = NORMAL;",
+		"PRAGMA secure_delete = ON;",
 	}
 
-	for _, pragma := range pragmas {
-		if _, err = db.ExecContext(ctx, pragma); err != nil {
-			slog.Error("Failed to set pragma", pragma, err)
-		} else {
-			slog.Debug("Set pragma", "pragma", pragma)
+	db, err := driver.Open(dbPath, func(c *sqlite3.Conn) error {
+		for _, pragma := range pragmas {
+			if err := c.Exec(pragma); err != nil {
+				return fmt.Errorf("failed to set pragma `%s`: %w", pragma, err)
+			}
 		}
+		return nil
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to open database: %w", err)
+	}
+
+	// Verify connection
+	if err = db.PingContext(ctx); err != nil {
+		db.Close()
+		return nil, fmt.Errorf("failed to connect to database: %w", err)
 	}
 
 	goose.SetBaseFS(FS)
@@ -58,5 +59,6 @@ func Connect(ctx context.Context, dataDir string) (*sql.DB, error) {
 		slog.Error("Failed to apply migrations", "error", err)
 		return nil, fmt.Errorf("failed to apply migrations: %w", err)
 	}
+
 	return db, nil
 }
diff --git a/internal/llm/agent/mcp-tools.go b/internal/llm/agent/mcp-tools.go
index 0f6d2d0ab31ec34df16c9837335425e1f3b195bb..bb50231da028e714c783f50cc7ebd8a1f4b595db 100644
--- a/internal/llm/agent/mcp-tools.go
+++ b/internal/llm/agent/mcp-tools.go
@@ -4,6 +4,7 @@ import (
 	"cmp"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"maps"
@@ -253,11 +254,15 @@ func updateMCPState(name string, state MCPState, err error, client *client.Clien
 }
 
 // CloseMCPClients closes all MCP clients. This should be called during application shutdown.
-func CloseMCPClients() {
+func CloseMCPClients() error {
+	var errs []error
 	for c := range mcpClients.Seq() {
-		_ = c.Close()
+		if err := c.Close(); err != nil {
+			errs = append(errs, err)
+		}
 	}
 	mcpBroker.Shutdown()
+	return errors.Join(errs...)
 }
 
 var mcpInitRequest = mcp.InitializeRequest{