From 6bbea52b1efefdb9e30242b22411a2f1c11bca77 Mon Sep 17 00:00:00 2001
From: Kenton Varda <kenton@cloudflare.com>
Date: Tue, 8 Oct 2024 17:08:58 -0500
Subject: [PATCH] SQLite: Throw better exception message when people use BEGIN
 TRANSACTION.

People mistakenly believed that we didn't support transactions!
---
 src/workerd/api/sql-test.js | 10 ++++++++--
 src/workerd/api/sql.c++     | 14 ++++++--------
 src/workerd/util/sqlite.c++ | 22 +++++++++++++++++++---
 src/workerd/util/sqlite.h   | 10 ++++++++--
 4 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/src/workerd/api/sql-test.js b/src/workerd/api/sql-test.js
index b8a2243509b..6f66014c570 100644
--- a/src/workerd/api/sql-test.js
+++ b/src/workerd/api/sql-test.js
@@ -456,8 +456,14 @@ async function test(state) {
   );
 
   // Can't start transactions or savepoints.
-  assert.throws(() => sql.exec('BEGIN TRANSACTION'), /not authorized/);
-  assert.throws(() => sql.exec('SAVEPOINT foo'), /not authorized/);
+  assert.throws(
+    () => sql.exec('BEGIN TRANSACTION'),
+    /please use the state.storage.transaction\(\) or state.storage.transactionSync\(\) APIs/
+  );
+  assert.throws(
+    () => sql.exec('SAVEPOINT foo'),
+    /please use the state.storage.transaction\(\) or state.storage.transactionSync\(\) APIs/
+  );
 
   // Virtual tables
   // Only fts5 and fts5vocab modules are allowed
diff --git a/src/workerd/api/sql.c++ b/src/workerd/api/sql.c++
index 6a993917cf5..b308c74bbe4 100644
--- a/src/workerd/api/sql.c++
+++ b/src/workerd/api/sql.c++
@@ -52,14 +52,12 @@ void SqlStorage::onError(kj::Maybe<int> sqliteErrorCode, kj::StringPtr message)
 }
 
 bool SqlStorage::allowTransactions() const {
-  if (IoContext::hasCurrent()) {
-    IoContext::current().logWarningOnce(
-        "To execute a transaction, please use the state.storage.transaction() API instead of the "
-        "SQL BEGIN TRANSACTION or SAVEPOINT statements. The JavaScript API is safer because it "
-        "will automatically roll back on exceptions, and because it interacts correctly with "
-        "Durable Objects' automatic atomic write coalescing.");
-  }
-  return false;
+  JSG_FAIL_REQUIRE(Error,
+      "To execute a transaction, please use the state.storage.transaction() or "
+      "state.storage.transactionSync() APIs instead of the SQL BEGIN TRANSACTION or SAVEPOINT "
+      "statements. The JavaScript API is safer because it will automatically roll back on "
+      "exceptions, and because it interacts correctly with Durable Objects' automatic atomic "
+      "write coalescing.");
 }
 
 jsg::JsValue SqlStorage::wrapSqlValue(jsg::Lock& js, SqlValue value) {
diff --git a/src/workerd/util/sqlite.c++ b/src/workerd/util/sqlite.c++
index 82d014dfae0..bc2a1597750 100644
--- a/src/workerd/util/sqlite.c++
+++ b/src/workerd/util/sqlite.c++
@@ -571,7 +571,18 @@ SqliteDatabase::StatementAndEffect SqliteDatabase::prepareSql(
     sqlite3_stmt* result;
     const char* tail;
 
-    SQLITE_CALL(sqlite3_prepare_v3(db, sqlCode.begin(), sqlCode.size(), prepFlags, &result, &tail));
+    auto prepareResult =
+        sqlite3_prepare_v3(db, sqlCode.begin(), sqlCode.size(), prepFlags, &result, &tail);
+    if (prepareResult == SQLITE_AUTH) {
+      KJ_IF_SOME(error, parseContext.authError) {
+        // Throw the tailored auth error.
+        kj::throwFatalException(kj::mv(error));
+      }
+    }
+    if (prepareResult != SQLITE_OK) {
+      SQLITE_CALL_FAILED("sqlite3_prepare_v3", prepareResult);
+    }
+
     SQLITE_REQUIRE(result != nullptr, kj::none, "SQL code did not contain a statement.", sqlCode);
     auto ownResult = ownSqlite(result);
 
@@ -1044,8 +1055,13 @@ void SqliteDatabase::setupSecurity(sqlite3* db) {
           ? SQLITE_OK
           : SQLITE_DENY;
     } catch (kj::Exception& e) {
-      // We'll crash if we throw to SQLite. Instead, log the error and deny.
-      KJ_LOG(ERROR, e);
+      // We'll crash if we throw to SQLite. Instead, shove the error into the parse context and
+      // report authorization denied. We'll pull it back out later.
+      KJ_IF_SOME(context, reinterpret_cast<SqliteDatabase*>(userdata)->currentParseContext) {
+        context.authError = kj::mv(e);
+      } else {
+        KJ_LOG(ERROR, e);
+      }
       return SQLITE_DENY;
     }
   },
diff --git a/src/workerd/util/sqlite.h b/src/workerd/util/sqlite.h
index b393075df50..ac7cd0eacc4 100644
--- a/src/workerd/util/sqlite.h
+++ b/src/workerd/util/sqlite.h
@@ -67,8 +67,11 @@ class SqliteDatabase {
 
   // Class which regulates a SQL query, especially to control how queries created in JavaScript
   // application code are handled.
+  //
+  // Note that any of the methods that check if actions are allowed my throw an exception instead
+  // of returning false. If they do, this exception will pass through to the caller in place of
+  // a generic "not authorized" exception.
   class Regulator {
-
   public:
     // Returns whether the given name (which may be a table, index, view, etc.) is allowed to be
     // accessed. Typically, this is used to deny access to names containing special prefixes
@@ -340,7 +343,10 @@ class SqliteDatabase {
     // What kind of state change does this statement cause, if any?
     StateChange stateChange = NoChange();
 
-    // TODO(soon): We could also use this to generate better errors.
+    // If the parse fails because the authorizer rejects it, it may fill in `authError` to provide
+    // a more friendly error message. This error will be thrown by the overal query. Otherwise,
+    // a generic "not authorized" error is thrown.
+    kj::Maybe<kj::Exception> authError;
   };
 };