sqlite: improve authorizer error handling

Author: araujogui

src/node_sqlite.cc

@@ -233,6 +233,27 @@ inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, DatabaseSync* db) {
   }
 }
 
+bool DatabaseSync::HasPendingAuthorizerError() const {
+  return has_pending_authorizer_error_;
+}
+
+void DatabaseSync::StoreAuthorizerError(Local<Value> error) {
+  if (!has_pending_authorizer_error_) {
+    pending_authorizer_error_.Reset(env()->isolate(), error);
+    has_pending_authorizer_error_ = true;
+  }
+}
+
+void DatabaseSync::RethrowPendingAuthorizerError() {
+  if (has_pending_authorizer_error_) {
+    Isolate* isolate = env()->isolate();
+    Local<Value> err = pending_authorizer_error_.Get(isolate);
+    pending_authorizer_error_.Reset();
+    has_pending_authorizer_error_ = false;
+    isolate->ThrowException(err);
+  }
+}
+
 inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, const char* message) {
   Local<Object> e;
   if (CreateSQLiteError(isolate, message).ToLocal(&e)) {
@@ -1126,6 +1147,15 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo<Value>& args) {
   Utf8Value sql(env->isolate(), args[0].As<String>());
   sqlite3_stmt* s = nullptr;
   int r = sqlite3_prepare_v2(db->connection_, *sql, -1, &s, 0);
+
+  if (db->HasPendingAuthorizerError()) {
+    db->RethrowPendingAuthorizerError();
+    if (s != nullptr) {
+      sqlite3_finalize(s);
+    }
+    return;
+  }
+
   CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
   BaseObjectPtr<StatementSync> stmt =
       StatementSync::Create(env, BaseObjectPtr<DatabaseSync>(db), s);
@@ -1147,6 +1177,10 @@ void DatabaseSync::Exec(const FunctionCallbackInfo<Value>& args) {
 
   Utf8Value sql(env->isolate(), args[0].As<String>());
   int r = sqlite3_exec(db->connection_, *sql, nullptr, nullptr, nullptr);
+  if (db->HasPendingAuthorizerError()) {
+    db->RethrowPendingAuthorizerError();
+    return;
+  }
   CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
 }
 
@@ -1906,9 +1940,7 @@ int DatabaseSync::AuthorizerCallback(void* user_data,
   Local<Value> cb =
       db->object()->GetInternalField(kAuthorizerCallback).template As<Value>();
 
-  if (!cb->IsFunction()) {
-    return SQLITE_DENY;
-  }
+  CHECK(cb->IsFunction());
 
   Local<Function> callback = cb.As<Function>();
   LocalVector<Value> js_argv(isolate);
@@ -1929,29 +1961,43 @@ int DatabaseSync::AuthorizerCallback(void* user_data,
       context, Undefined(isolate), js_argv.size(), js_argv.data());
 
   if (try_catch.HasCaught()) {
-    // If there's an exception in the callback, deny the operation
-    // TODO: Rethrow exception
+    db->StoreAuthorizerError(try_catch.Exception());
     return SQLITE_DENY;
   }
 
   Local<Value> result;
-  if (!retval.ToLocal(&result)) {
+  if (!retval.ToLocal(&result) || result->IsUndefined() || result->IsNull()) {
+    // Missing return value
+    Local<Value> err = Exception::TypeError(
+        String::NewFromUtf8(
+            isolate,
+            "Authorizer callback must return an integer authorization code")
+            .ToLocalChecked());
+    db->StoreAuthorizerError(err);
     return SQLITE_DENY;
   }
 
-  if (!result->IsNumber()) {
+  if (!result->IsInt32()) {
+    Local<Value> err = Exception::TypeError(
+        String::NewFromUtf8(
+            isolate, "Authorizer callback return value must be an integer")
+            .ToLocalChecked());
+    db->StoreAuthorizerError(err);
     return SQLITE_DENY;
   }
 
-  double num_result = result.As<Number>()->Value();
-  int int_result = static_cast<int>(num_result);
-
-  if (int_result == SQLITE_OK || int_result == SQLITE_DENY ||
-      int_result == SQLITE_IGNORE) {
-    return int_result;
+  int32_t int_result = result.As<Int32>()->Value();
+  if (int_result != SQLITE_OK && int_result != SQLITE_DENY &&
+      int_result != SQLITE_IGNORE) {
+    Local<Value> err = Exception::RangeError(
+        String::NewFromUtf8(
+            isolate, "Authorizer callback returned invalid authorization code")
+            .ToLocalChecked());
+    db->StoreAuthorizerError(err);
+    return SQLITE_DENY;
   }
 
-  return SQLITE_DENY;
+  return int_result;
 }
 
 StatementSync::StatementSync(Environment* env,

src/node_sqlite.h

@@ -171,6 +171,10 @@ class DatabaseSync : public BaseObject {
   void SetIgnoreNextSQLiteError(bool ignore);
   bool ShouldIgnoreSQLiteError();
 
+  bool HasPendingAuthorizerError() const;
+  void StoreAuthorizerError(v8::Local<v8::Value> error);
+  void RethrowPendingAuthorizerError();
+
   SET_MEMORY_INFO_NAME(DatabaseSync)
   SET_SELF_SIZE(DatabaseSync)
 
@@ -189,6 +193,9 @@ class DatabaseSync : public BaseObject {
   std::set<sqlite3_session*> sessions_;
   std::unordered_set<StatementSync*> statements_;
 
+  bool has_pending_authorizer_error_ = false;
+  v8::Global<v8::Value> pending_authorizer_error_;
+
   friend class Session;
   friend class SQLTagStore;
   friend class StatementExecutionHelper;

test/parallel/test-sqlite-authz.js

@@ -71,7 +71,7 @@ suite('DatabaseSync.prototype.setAuthorizer()', () => {
     });
   });
 
-  it('blocks operations when authorizer throws error', () => {
+  it('rethrows error when authorizer throws error', () => {
     const db = new DatabaseSync(':memory:');
     db.setAuthorizer(() => {
       throw new Error('Unknown error');
@@ -80,12 +80,23 @@ suite('DatabaseSync.prototype.setAuthorizer()', () => {
     assert.throws(() => {
       db.exec('SELECT 1');
     }, {
-      code: 'ERR_SQLITE_ERROR',
-      message: /not authorized/
+      message: 'Unknown error'
+    });
+  });
+
+  it('throws error when authorizer returns nothing', () => {
+    const db = new DatabaseSync(':memory:');
+    db.setAuthorizer(() => {
+    });
+
+    assert.throws(() => {
+      db.exec('SELECT 1');
+    }, {
+      message: 'Authorizer callback must return an integer authorization code'
     });
   });
 
-  it('blocks operations when authorizer returns NaN', () => {
+  it('throws error when authorizer returns NaN', () => {
     const db = new DatabaseSync(':memory:');
     db.setAuthorizer(() => {
       return '1';
@@ -94,12 +105,11 @@ suite('DatabaseSync.prototype.setAuthorizer()', () => {
     assert.throws(() => {
       db.exec('SELECT 1');
     }, {
-      code: 'ERR_SQLITE_ERROR',
-      message: /not authorized/
+      message: 'Authorizer callback return value must be an integer'
     });
   });
 
-  it('blocks operations when authorizer returns a invalid code', () => {
+  it('throws error when authorizer returns a invalid code', () => {
     const db = new DatabaseSync(':memory:');
     db.setAuthorizer(() => {
       return 3;
@@ -108,8 +118,7 @@ suite('DatabaseSync.prototype.setAuthorizer()', () => {
     assert.throws(() => {
       db.exec('SELECT 1');
     }, {
-      code: 'ERR_SQLITE_ERROR',
-      message: /not authorized/
+      message: 'Authorizer callback returned invalid authorization code'
     });
   });