From 0ef53a4e157e9bdc5a4ca8993b84e89844af5183 Mon Sep 17 00:00:00 2001
From: clawbot <35+clawbot@noreply.example.org>
Date: Tue, 13 Oct 2026 04:24:32 +0000
Subject: [PATCH] Escape the database path in the SQLite connection string
 (closes #518)

SQLite reads a file: URI's path only up to the first ? or #, and
decodes %XX escapes in it. With a data directory such as
/srv/hooks#1, every database opened the one file /srv/hooks. The
connection string is now built with url.URL, which escapes the path,
and OmitHost keeps a relative path relative.

Tests open databases in a directory whose name holds #, ? and %, and
at a relative path, and check through PRAGMA database_list that each
opened the file asked for. A handlers test comment that blamed # in
the temporary directory's path is dropped, as that is no longer true.

Model: opus-5-5
---
 internal/database/sqlite_open.go            | 14 +++-
 internal/database/sqlite_open_test.go       | 75 +++++++++++++++++++++
 internal/handlers/delivery_attempts_test.go |  3 -
 3 files changed, 88 insertions(+), 4 deletions(-)

diff --git a/internal/database/sqlite_open.go b/internal/database/sqlite_open.go
index c162a82..e73ce57 100644
--- a/internal/database/sqlite_open.go
+++ b/internal/database/sqlite_open.go
@@ -242,7 +242,19 @@ func SQLiteDSN(path, mode string) string {
 	)
 	q.Add("_pragma", "journal_mode(WAL)")
 
-	return "file:" + path + "?" + q.Encode()
+	// SQLite reads a file: URI's path only up to the first ? or #, and
+	// decodes %XX escapes in it, so url.URL escapes the path. OmitHost
+	// keeps a relative path as "file:data/x.db"; written
+	// "file://data/x.db", SQLite would reject "data" as the URI's
+	// authority.
+	dsn := url.URL{
+		Scheme:   "file",
+		Path:     path,
+		OmitHost: true,
+		RawQuery: q.Encode(),
+	}
+
+	return dsn.String()
 }
 
 // OpenSQLite opens the SQLite file at path with the service's
diff --git a/internal/database/sqlite_open_test.go b/internal/database/sqlite_open_test.go
index 448507c..b16aa12 100644
--- a/internal/database/sqlite_open_test.go
+++ b/internal/database/sqlite_open_test.go
@@ -2,6 +2,8 @@ package database_test
 
 import (
 	"context"
+	"database/sql"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -112,6 +114,79 @@ func TestMainDBAppliesPragmasOnALiveHandle(t *testing.T) {
 	assert.Equal(t, "10000", busy)
 }
 
+// TestOpenSQLiteOpensThePathAskedFor uses a data directory whose name
+// holds ?, # and %. SQLite reads a file: URI's path only up to a ? or
+// #, and decodes %41 to A, so an unescaped path sent every database in
+// that directory to one file outside it.
+func TestOpenSQLiteOpensThePathAskedFor(t *testing.T) {
+	t.Parallel()
+
+	dir := filepath.Join(t.TempDir(), "hooks#1?x%41")
+	require.NoError(t, os.Mkdir(dir, 0o700))
+
+	for _, name := range []string{
+		database.MainDBFileName,
+		"events-" + uuid.New().String() + ".db",
+	} {
+		path := filepath.Join(dir, name)
+
+		sqlDB, err := database.OpenSQLite(
+			path, database.SQLiteModeCreate,
+		)
+		require.NoError(t, err)
+
+		t.Cleanup(func() { assert.NoError(t, sqlDB.Close()) })
+
+		assertOpenedFile(t, sqlDB, path)
+	}
+}
+
+// TestOpenSQLiteOpensARelativePath opens a database at a path relative
+// to the working directory, which is what a relative DATA_DIR such as
+// the README's DATA_DIR=../webhooker-data gives OpenSQLite.
+//
+//nolint:paralleltest // t.Chdir moves the whole process.
+func TestOpenSQLiteOpensARelativePath(t *testing.T) {
+	t.Chdir(t.TempDir())
+	require.NoError(t, os.Mkdir("data", 0o700))
+
+	path := filepath.Join("data", database.MainDBFileName)
+
+	sqlDB, err := database.OpenSQLite(path, database.SQLiteModeCreate)
+	require.NoError(t, err)
+
+	defer func() { require.NoError(t, sqlDB.Close()) }()
+
+	assertOpenedFile(t, sqlDB, path)
+}
+
+// assertOpenedFile checks that sqlDB is the SQLite file at path.
+// database_list names the file SQLite actually opened, as an absolute
+// path with symlinks resolved, so the files are compared, not the names.
+func assertOpenedFile(t *testing.T, sqlDB *sql.DB, path string) {
+	t.Helper()
+
+	var (
+		seq          int
+		schema, file string
+	)
+
+	require.NoError(t, sqlDB.
+		QueryRowContext(t.Context(), "pragma database_list").
+		Scan(&seq, &schema, &file))
+
+	asked, err := os.Stat(path)
+	require.NoError(t, err)
+
+	opened, err := os.Stat(file)
+	require.NoError(t, err)
+
+	assert.True(
+		t, os.SameFile(asked, opened),
+		"asked for %s, SQLite opened %s", path, file,
+	)
+}
+
 // TestConcurrentReaderDoesNotBlockWrites is the unit-scale form of the
 // reproduction in
 // https://git.eeqj.de/sneak/webhooker/issues/256: an operator's
diff --git a/internal/handlers/delivery_attempts_test.go b/internal/handlers/delivery_attempts_test.go
index bba56aa..55d82f1 100644
--- a/internal/handlers/delivery_attempts_test.go
+++ b/internal/handlers/delivery_attempts_test.go
@@ -42,9 +42,6 @@ func TestDeliveryAttempts_ReadInTheTargetTypesOwnTerms(t *testing.T) {
 		{database.TargetTypeLog, true, 0, "", "written to the log", ""},
 	}
 
-	// Named for type and outcome, so that no two cases share a name:
-	// Go adds "#01" to a repeated one, and a "#" in the temporary
-	// directory's path cuts the path SQLiteDSN gives SQLite short.
 	for _, tc := range cases {
 		t.Run(string(tc.targetType)+" "+tc.outcome, func(t *testing.T) {
 			t.Parallel()
-- 
2.54.0