Commit 0895392f authored by cznic's avatar cznic
Browse files

sqlite: validate all DSN parameters before applying any of them 

applyQueryParams validated each parameter as it reached it, so a DSN whose
last parameter was bad still executed every PRAGMA ahead of it before
failing. PRAGMA journal_mode and auto_vacuum are persistent changes to the
database file, so

	file:x.db?_journal_mode=wal&_synchronous=bogus

failed the connection and left x.db converted to WAL regardless. A failed
Open must not change the database.

Split the function into a validation phase and an apply phase: everything
checkable is rejected before the first c.exec. Assignments to c stay in the
validation phase, since newConn closes and discards the connection when this
returns an error and they cannot outlive the failure. The documented apply
order is unchanged and still covered by the combined-DSN test.

This predates the !134 shorthand keys - master already behaved this way for
_pragma combined with a late-rejected _txlock, which is why the new test
covers that case too. _pragma remains the one parameter that can still fail
partway, as its values are executed verbatim and cannot be checked up front.

Co-Authored-By: default avatarClaude Opus 4.8 <noreply@anthropic.com>
parent 63a57e47

driver.go

+8 −1
Original line number Diff line number Diff line
@@ -74,9 +74,16 @@ func newDriver() *Driver { return d } 
//	_auto_vacuum, _vacuum     -> PRAGMA auto_vacuum    (0 NONE 1 FULL 2 INCREMENTAL)

//	_query_only               -> PRAGMA query_only     (0 1 false true no yes off on)

//

// All DSN parameters that can be validated are validated before any of them is

// applied, so a DSN carrying a typo fails without having executed the PRAGMAs

// that precede it -- a rejected DSN does not leave the database converted to WAL

// or with auto_vacuum already set.

//

// Unlike these validated shorthand keys, each _pragma value is executed verbatim

// (with PRAGMA prepended) and is not validated, so a DSN that includes _pragma

// must come from a trusted source.

// must come from a trusted source. It is also the one case that can still fail

// partway: a bad _pragma is only rejected by SQLite as it runs, after any

// earlier _pragma in the list has taken effect.

//

// _time_format: The name of a format to use when writing time values to the database.

// The currently supported values are (1) "sqlite" for YYYY-MM-DD HH:MM:SS.SSS[+-]HH:MM

dsn_test.go

+59 −0
Original line number Diff line number Diff line
@@ -206,4 +206,63 @@ func TestDSNPragmas(t *testing.T) { 
		// An empty value is not validated, so it is not an error either.

		verifyPragma(t, "_synchronous=", "synchronous", "2")

	})



	t.Run("Test validation precedes application", func(t *testing.T) {

		// A DSN carrying a valid persistent PRAGMA and an invalid value elsewhere

		// must fail without having applied the valid one: journal_mode and

		// auto_vacuum change the database file, and a failed Open must not leave

		// it half-configured. Applying lazily per key would convert the file to

		// WAL and only then reject _synchronous.

		verifyPersistentUnchanged := func(t *testing.T, query, wantErrSubstr string) {

			t.Helper()

			dbPath := filepath.Join(t.TempDir(), "tmp.db")



			db, err := sql.Open("sqlite", dbPath+"?"+query)

			if err != nil {

				t.Fatal(err)

			}

			var x int

			err = db.QueryRow("SELECT 1").Scan(&x)

			if err == nil {

				t.Fatalf("query %q: expected an error, got none", query)

			}

			if !strings.Contains(err.Error(), wantErrSubstr) {

				t.Fatalf("query %q: error %q does not contain %q", query, err.Error(), wantErrSubstr)

			}

			db.Close()



			// Reopen without parameters and confirm the file was left alone.

			db2, err := sql.Open("sqlite", dbPath)

			if err != nil {

				t.Fatal(err)

			}

			defer db2.Close()



			var journalMode string

			if err := db2.QueryRow("PRAGMA journal_mode").Scan(&journalMode); err != nil {

				t.Fatal(err)

			}

			if journalMode != "delete" {

				t.Errorf("query %q: failed open still converted the database to journal_mode %q", query, journalMode)

			}



			if _, err := db2.Exec("CREATE TABLE t(x)"); err != nil {

				t.Fatal(err)

			}

			var autoVacuum string

			if err := db2.QueryRow("PRAGMA auto_vacuum").Scan(&autoVacuum); err != nil {

				t.Fatal(err)

			}

			if autoVacuum != "0" {

				t.Errorf("query %q: failed open still set auto_vacuum to %q", query, autoVacuum)

			}

		}



		verifyPersistentUnchanged(t, "_journal_mode=wal&_synchronous=bogus", "invalid _synchronous")

		verifyPersistentUnchanged(t, "_auto_vacuum=1&_foreign_keys=nope", "invalid _foreign_keys")

		verifyPersistentUnchanged(t, "_journal_mode=wal&_auto_vacuum=1&_query_only=maybe", "invalid _query_only")



		// A non-PRAGMA parameter rejected late must not apply the PRAGMAs either.

		verifyPersistentUnchanged(t, "_journal_mode=wal&_txlock=bogus", "unknown _txlock")

	})

}

sqlite.go

+83 −53
Original line number Diff line number Diff line
@@ -245,11 +245,22 @@ func applyQueryParams(c *conn, query string) error { 
		return err

	}



	// mattn-compatible shorthand PRAGMA keys. Each value is validated against the

	// same set github.com/mattn/go-sqlite3 accepts (case-insensitive); an

	// unrecognized value is a hard error rather than a silent no-op. When a key

	// and its alias are both present the alias wins, matching mattn. See the

	// Driver documentation in driver.go for the full apply order and precedence.

	// Validation phase. Everything that can be rejected is rejected here, before

	// the apply phase below executes a single statement. PRAGMA journal_mode and

	// auto_vacuum are persistent changes to the database file, so validating

	// lazily as each key is applied would let a typo in a later parameter fail

	// the connection only after the file had already been converted -- a failed

	// Open must not leave the database half-configured. Assignments to c are

	// exempt from that concern: newConn closes and discards the connection when

	// this returns an error, so they cannot outlive the failure.

	//

	// Each shorthand value is validated against the same set

	// github.com/mattn/go-sqlite3 accepts (case-insensitive). When a key and its

	// alias are both present the alias wins, matching mattn. See the Driver

	// documentation in driver.go for the full apply order and precedence.

	//

	// _pragma values are the one exception: they are executed verbatim and

	// cannot be checked here, so a malformed _pragma can still fail partway.

	busyKey, busyTimeout := dsnPick(q, "_busy_timeout", "_timeout")

	if busyTimeout != "" {

		if _, err := strconv.ParseInt(busyTimeout, 10, 64); err != nil {
@@ -257,50 +268,37 @@ func applyQueryParams(c *conn, query string) error { 
		}

	}



	// Busy timeout must be one of the first PRAGMAs set from query params as some

	// that make changes to the database might otherwise unexpectedly fail with

	// SQLITE_BUSY.

	if busyTimeout != "" {

		if _, err := c.exec(context.Background(), "pragma busy_timeout = "+busyTimeout, nil); err != nil {

			return err

		}

	}



	// auto_vacuum must be applied while the database is still new: a journal_mode

	// change or the first table materialises page 1 and locks the setting in, so

	// it runs before the _pragma list and the other shorthand keys.

	autoVacuumKey, autoVacuum := dsnPick(q, "_auto_vacuum", "_vacuum")

	if autoVacuum != "" {

		if err := dsnEnum(autoVacuumKey, autoVacuum, []string{"0", "NONE", "1", "FULL", "2", "INCREMENTAL"}); err != nil {

			return err

		}

		if _, err := c.exec(context.Background(), "pragma auto_vacuum = "+autoVacuum, nil); err != nil {

	}



	foreignKeysKey, foreignKeys := dsnPick(q, "_foreign_keys", "_fk")

	if foreignKeys != "" {

		if err := dsnBool(foreignKeysKey, foreignKeys); err != nil {

			return err

		}

	}



	var a []string

	for _, v := range q["_pragma"] {

		a = append(a, v)

	journalModeKey, journalMode := dsnPick(q, "_journal_mode", "_journal")

	if journalMode != "" {

		if err := dsnEnum(journalModeKey, journalMode, []string{"DELETE", "TRUNCATE", "PERSIST", "MEMORY", "WAL", "OFF"}); err != nil {

			return err

		}

	// Push 'busy_timeout' first, the rest in lexicographic order, case insenstive.

	// See https://gitlab.com/cznic/sqlite/-/issues/198#note_2233423463 for

	// discussion.

	sort.Slice(a, func(i, j int) bool {

		x, y := strings.TrimSpace(strings.ToLower(a[i])), strings.TrimSpace(strings.ToLower(a[j]))

		if strings.HasPrefix(x, "busy_timeout") {

			return true

	}

		if strings.HasPrefix(y, "busy_timeout") {

			return false



	synchronousKey, synchronous := dsnPick(q, "_synchronous", "_sync")

	if synchronous != "" {

		if err := dsnEnum(synchronousKey, synchronous, []string{"0", "OFF", "1", "NORMAL", "2", "FULL", "3", "EXTRA"}); err != nil {

			return err

		}

	}



		return x < y

	})

	for _, v := range a {

		cmd := "pragma " + v

		_, err := c.exec(context.Background(), cmd, nil)

		if err != nil {

	queryOnly := q.Get("_query_only")

	if queryOnly != "" {

		if err := dsnBool("_query_only", queryOnly); err != nil {

			return err

		}

	}
@@ -358,31 +356,66 @@ func applyQueryParams(c *conn, query string) error { 
		c.textToTime = onoff

	}



	foreignKeysKey, foreignKeys := dsnPick(q, "_foreign_keys", "_fk")

	if foreignKeys != "" {

		if err := dsnBool(foreignKeysKey, foreignKeys); err != nil {

	// Apply phase. The order here is the documented one and is independent of

	// the order the keys appear in the DSN.

	//

	// Busy timeout must be one of the first PRAGMAs set from query params as some

	// that make changes to the database might otherwise unexpectedly fail with

	// SQLITE_BUSY.

	if busyTimeout != "" {

		if _, err := c.exec(context.Background(), "pragma busy_timeout = "+busyTimeout, nil); err != nil {

			return err

		}

		if _, err := c.exec(context.Background(), "pragma foreign_keys = "+foreignKeys, nil); err != nil {

	}



	// auto_vacuum must be applied while the database is still new: a journal_mode

	// change or the first table materialises page 1 and locks the setting in, so

	// it runs before the _pragma list and the other shorthand keys.

	if autoVacuum != "" {

		if _, err := c.exec(context.Background(), "pragma auto_vacuum = "+autoVacuum, nil); err != nil {

			return err

		}

	}



	journalModeKey, journalMode := dsnPick(q, "_journal_mode", "_journal")

	if journalMode != "" {

		if err := dsnEnum(journalModeKey, journalMode, []string{"DELETE", "TRUNCATE", "PERSIST", "MEMORY", "WAL", "OFF"}); err != nil {

	var a []string

	for _, v := range q["_pragma"] {

		a = append(a, v)

	}

	// Push 'busy_timeout' first, the rest in lexicographic order, case insenstive.

	// See https://gitlab.com/cznic/sqlite/-/issues/198#note_2233423463 for

	// discussion.

	sort.Slice(a, func(i, j int) bool {

		x, y := strings.TrimSpace(strings.ToLower(a[i])), strings.TrimSpace(strings.ToLower(a[j]))

		if strings.HasPrefix(x, "busy_timeout") {

			return true

		}

		if strings.HasPrefix(y, "busy_timeout") {

			return false

		}



		return x < y

	})

	for _, v := range a {

		cmd := "pragma " + v

		_, err := c.exec(context.Background(), cmd, nil)

		if err != nil {

			return err

		}

		if _, err := c.exec(context.Background(), "pragma journal_mode = "+journalMode, nil); err != nil {

	}



	if foreignKeys != "" {

		if _, err := c.exec(context.Background(), "pragma foreign_keys = "+foreignKeys, nil); err != nil {

			return err

		}

	}



	synchronousKey, synchronous := dsnPick(q, "_synchronous", "_sync")

	if synchronous != "" {

		if err := dsnEnum(synchronousKey, synchronous, []string{"0", "OFF", "1", "NORMAL", "2", "FULL", "3", "EXTRA"}); err != nil {

	if journalMode != "" {

		if _, err := c.exec(context.Background(), "pragma journal_mode = "+journalMode, nil); err != nil {

			return err

		}

	}



	if synchronous != "" {

		if _, err := c.exec(context.Background(), "pragma synchronous = "+synchronous, nil); err != nil {

			return err

		}
@@ -390,11 +423,8 @@ func applyQueryParams(c *conn, query string) error { 


	// query_only is applied last: it makes the connection read-only, so it must

	// not precede the write-capable pragmas above (notably auto_vacuum).

	if v := q.Get("_query_only"); v != "" {

		if err := dsnBool("_query_only", v); err != nil {

			return err

		}

		if _, err := c.exec(context.Background(), "pragma query_only = "+v, nil); err != nil {

	if queryOnly != "" {

		if _, err := c.exec(context.Background(), "pragma query_only = "+queryOnly, nil); err != nil {

			return err

		}

	}