From 30eefaedab17af6bece02a7f28dd8531f32719c1 Mon Sep 17 00:00:00 2001 From: Lazizbek Ergashev Date: Sun, 6 Sep 2026 04:46:18 +0500 Subject: [PATCH 1/2] sqlite: fix crashes on invalid backup and URL args Signed-off-by: Lazizbek Ergashev --- src/env_properties.h | 1 + src/node_sqlite.cc | 13 ++++++++++++- test/parallel/test-sqlite-backup.mjs | 9 +++++++++ test/parallel/test-sqlite-database-sync.js | 9 +++++++++ 4 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/env_properties.h b/src/env_properties.h index fc1772f926e5..8a1fa2daa117 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -478,6 +478,7 @@ V(socketaddress_constructor_template, v8::FunctionTemplate) \ V(space_stats_template, v8::DictionaryTemplate) \ V(sqlite_column_template, v8::DictionaryTemplate) \ + V(sqlite_database_sync_constructor_template, v8::FunctionTemplate) \ V(sqlite_limits_template, v8::ObjectTemplate) \ V(sqlite_run_result_template, v8::DictionaryTemplate) \ V(sqlite_statement_sync_constructor_template, v8::FunctionTemplate) \ diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 76accf1c2731..adbe66d02693 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1284,7 +1284,10 @@ std::optional ValidateDatabasePath(Environment* env, Utf8Value location_value(env->isolate(), href.As()); auto location = location_value.ToStringView(); if (!has_null_bytes(location)) { - CHECK(ada::can_parse(location)); + if (!ada::can_parse(location)) { + THROW_ERR_INVALID_URL(env->isolate(), "Invalid URL"); + return std::nullopt; + } if (!location.starts_with("file:")) { THROW_ERR_INVALID_URL_SCHEME(env->isolate()); return std::nullopt; @@ -2425,6 +2428,13 @@ void Backup(const FunctionCallbackInfo& args) { return; } + if (!env->sqlite_database_sync_constructor_template()->HasInstance(args[0])) { + THROW_ERR_INVALID_ARG_TYPE( + env->isolate(), + "The \"sourceDb\" argument must be a DatabaseSync instance."); + return; + } + DatabaseSync* db; ASSIGN_OR_RETURN_UNWRAP(&db, args[0].As()); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); @@ -4573,6 +4583,7 @@ static void Initialize(Local target, NewFunctionTemplate(isolate, DatabaseSync::New); db_tmpl->InstanceTemplate()->SetInternalFieldCount( DatabaseSync::kInternalFieldCount); + env->set_sqlite_database_sync_constructor_template(db_tmpl); Local constants = Object::New(isolate); DefineConstants(constants); diff --git a/test/parallel/test-sqlite-backup.mjs b/test/parallel/test-sqlite-backup.mjs index f995ae3ca72a..28eaa4d9b01a 100644 --- a/test/parallel/test-sqlite-backup.mjs +++ b/test/parallel/test-sqlite-backup.mjs @@ -52,6 +52,15 @@ describe('backup()', () => { }); }); + test('throws if the source database is not a DatabaseSync', (t) => { + t.assert.throws(() => { + backup({}, nextDb()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "sourceDb" argument must be a DatabaseSync instance.' + }); + }); + test('throws if path is not a string, URL, or Buffer', (t) => { const database = makeSourceDb(); diff --git a/test/parallel/test-sqlite-database-sync.js b/test/parallel/test-sqlite-database-sync.js index 08a636c9cbdc..5b29169a354a 100644 --- a/test/parallel/test-sqlite-database-sync.js +++ b/test/parallel/test-sqlite-database-sync.js @@ -51,6 +51,15 @@ suite('DatabaseSync() constructor', () => { }); }); + test('throws if the database location has an unparsable href', (t) => { + t.assert.throws(() => { + new DatabaseSync({ href: 'zzz' }); + }, { + code: 'ERR_INVALID_URL', + message: 'Invalid URL', + }); + }); + test('throws if options is provided but is not an object', (t) => { t.assert.throws(() => { new DatabaseSync('foo', null); From bf0648ca3505af53565a56ca92b52dff1ab2f529 Mon Sep 17 00:00:00 2001 From: Lazizbek Ergashev Date: Sun, 6 Sep 2026 05:52:51 +0500 Subject: [PATCH 2/2] sqlite: address review feedback on backup() validation Store the DatabaseSync constructor template behind DatabaseSync::GetConstructorTemplate(), matching StatementSync, StatementSyncIterator, Session and SQLTagStore, which all already have one. DatabaseSync was the only class in the file setting its template up inline in Initialize(), which also meant the Environment slot would be reassigned if Initialize() ever ran for more than one realm, leaving instances from the earlier realm failing HasInstance(). This moves SetSideEffectFreeGetter() to the top of the file so the accessor can use it; its body is unchanged. Drop the now-redundant IsObject() check in Backup() so a bad sourceDb reports one message instead of two, and use the wording lib/internal/errors.js generates for ERR_INVALID_ARG_TYPE. Stop discarding an exception thrown by an href getter in ValidateDatabasePath() and replacing it with ERR_INVALID_ARG_TYPE. Cover the remaining unwrap cases in tests: an array, a padded plain object, an object with DatabaseSync.prototype, a StatementSync and a Session, plus the second ValidateDatabasePath() caller reached through backup(db, { href: 'zzz' }). Co-authored-by: Trevor Burnham Signed-off-by: Lazizbek Ergashev --- src/node_sqlite.cc | 170 +++++++++++---------- src/node_sqlite.h | 2 + test/parallel/test-sqlite-backup.mjs | 37 ++++- test/parallel/test-sqlite-database-sync.js | 9 ++ 4 files changed, 131 insertions(+), 87 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index adbe66d02693..4ae04615f073 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -82,6 +82,23 @@ inline MaybeLocal Utf8StringMaybeOneByte(Isolate* isolate, isolate, input.data(), NewStringType::kNormal, len); } +static inline void SetSideEffectFreeGetter( + Isolate* isolate, + Local class_template, + Local name, + FunctionCallback fn) { + Local getter = + FunctionTemplate::New(isolate, + fn, + Local(), + v8::Signature::New(isolate, class_template), + /* length */ 0, + ConstructorBehavior::kThrow, + SideEffectType::kHasNoSideEffect); + class_template->InstanceTemplate()->SetAccessorProperty( + name, getter, Local(), DontDelete); +} + BindingData::BindingData(Realm* realm, Local wrap) : BaseObject(realm, wrap) { MakeWeak(); @@ -1279,11 +1296,17 @@ std::optional ValidateDatabasePath(Environment* env, } else if (path->IsObject()) { // When is URL auto url = path.As(); Local href; - if (url->Get(env->context(), env->href_string()).ToLocal(&href) && - href->IsString()) { + // Let an exception thrown by the href getter propagate instead of + // replacing it with ERR_INVALID_ARG_TYPE. + if (!url->Get(env->context(), env->href_string()).ToLocal(&href)) { + return std::nullopt; + } + if (href->IsString()) { Utf8Value location_value(env->isolate(), href.As()); auto location = location_value.ToStringView(); if (!has_null_bytes(location)) { + // A real URL always has a parseable href, but any object with a string + // href reaches this branch, so the value cannot be assumed to be one. if (!ada::can_parse(location)) { THROW_ERR_INVALID_URL(env->isolate(), "Invalid URL"); return std::nullopt; @@ -1306,6 +1329,62 @@ std::optional ValidateDatabasePath(Environment* env, return std::nullopt; } +Local DatabaseSync::GetConstructorTemplate(Environment* env) { + Local tmpl = + env->sqlite_database_sync_constructor_template(); + if (tmpl.IsEmpty()) { + Isolate* isolate = env->isolate(); + tmpl = NewFunctionTemplate(isolate, DatabaseSync::New); + tmpl->InstanceTemplate()->SetInternalFieldCount( + DatabaseSync::kInternalFieldCount); + SetProtoMethod(isolate, tmpl, "open", DatabaseSync::Open); + SetProtoMethod(isolate, tmpl, "close", DatabaseSync::Close); + SetProtoDispose(isolate, tmpl, DatabaseSync::Dispose); + SetProtoMethod(isolate, tmpl, "prepare", DatabaseSync::Prepare); + SetProtoMethod(isolate, tmpl, "exec", DatabaseSync::Exec); + SetProtoMethod(isolate, tmpl, "function", DatabaseSync::CustomFunction); + SetProtoMethod( + isolate, tmpl, "createTagStore", DatabaseSync::CreateTagStore); + SetProtoMethodNoSideEffect( + isolate, tmpl, "location", DatabaseSync::Location); + SetProtoMethod(isolate, tmpl, "aggregate", DatabaseSync::AggregateFunction); + SetProtoMethod(isolate, tmpl, "createSession", DatabaseSync::CreateSession); + SetProtoMethod( + isolate, tmpl, "applyChangeset", DatabaseSync::ApplyChangeset); + SetProtoMethod(isolate, + tmpl, + "enableLoadExtension", + DatabaseSync::EnableLoadExtension); + SetProtoMethod( + isolate, tmpl, "enableDefensive", DatabaseSync::EnableDefensive); + SetProtoMethod(isolate, tmpl, "loadExtension", DatabaseSync::LoadExtension); + SetProtoMethod(isolate, tmpl, "serialize", DatabaseSync::Serialize); + SetProtoMethod(isolate, tmpl, "deserialize", DatabaseSync::Deserialize); + SetProtoMethod(isolate, tmpl, "setAuthorizer", DatabaseSync::SetAuthorizer); + SetSideEffectFreeGetter(isolate, + tmpl, + FIXED_ONE_BYTE_STRING(isolate, "isOpen"), + DatabaseSync::IsOpenGetter); + SetSideEffectFreeGetter(isolate, + tmpl, + FIXED_ONE_BYTE_STRING(isolate, "isTransaction"), + DatabaseSync::IsTransactionGetter); + SetSideEffectFreeGetter(isolate, + tmpl, + FIXED_ONE_BYTE_STRING(isolate, "limits"), + DatabaseSync::LimitsGetter); + Local sqlite_type_key = + FIXED_ONE_BYTE_STRING(isolate, "sqlite-type"); + Local sqlite_type_symbol = + v8::Symbol::For(isolate, sqlite_type_key); + Local database_sync_string = + FIXED_ONE_BYTE_STRING(isolate, "node:sqlite"); + tmpl->InstanceTemplate()->Set(sqlite_type_symbol, database_sync_string); + env->set_sqlite_database_sync_constructor_template(tmpl); + } + return tmpl; +} + void DatabaseSync::New(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (!args.IsConstructCall()) { @@ -2422,16 +2501,13 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { void Backup(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 1 || !args[0]->IsObject()) { - THROW_ERR_INVALID_ARG_TYPE(env->isolate(), - "The \"sourceDb\" argument must be an object."); - return; - } - - if (!env->sqlite_database_sync_constructor_template()->HasInstance(args[0])) { + // Unlike the other unwrap sites in this file, which rely on V8's signature + // check for args.This(), this one takes a value out of args[] and so has to + // check the type itself before unwrapping it. + if (!DatabaseSync::GetConstructorTemplate(env)->HasInstance(args[0])) { THROW_ERR_INVALID_ARG_TYPE( env->isolate(), - "The \"sourceDb\" argument must be a DatabaseSync instance."); + "The \"sourceDb\" argument must be an instance of DatabaseSync."); return; } @@ -3821,23 +3897,6 @@ SQLTagStore::SQLTagStore(Environment* env, MakeWeak(); } -static inline void SetSideEffectFreeGetter( - Isolate* isolate, - Local class_template, - Local name, - FunctionCallback fn) { - Local getter = - FunctionTemplate::New(isolate, - fn, - Local(), - v8::Signature::New(isolate, class_template), - /* length */ 0, - ConstructorBehavior::kThrow, - SideEffectType::kHasNoSideEffect); - class_template->InstanceTemplate()->SetAccessorProperty( - name, getter, Local(), DontDelete); -} - SQLTagStore::~SQLTagStore() {} Local SQLTagStore::GetConstructorTemplate(Environment* env) { @@ -4579,63 +4638,14 @@ static void Initialize(Local target, } }); } - Local db_tmpl = - NewFunctionTemplate(isolate, DatabaseSync::New); - db_tmpl->InstanceTemplate()->SetInternalFieldCount( - DatabaseSync::kInternalFieldCount); - env->set_sqlite_database_sync_constructor_template(db_tmpl); Local constants = Object::New(isolate); DefineConstants(constants); - SetProtoMethod(isolate, db_tmpl, "open", DatabaseSync::Open); - SetProtoMethod(isolate, db_tmpl, "close", DatabaseSync::Close); - SetProtoDispose(isolate, db_tmpl, DatabaseSync::Dispose); - SetProtoMethod(isolate, db_tmpl, "prepare", DatabaseSync::Prepare); - SetProtoMethod(isolate, db_tmpl, "exec", DatabaseSync::Exec); - SetProtoMethod(isolate, db_tmpl, "function", DatabaseSync::CustomFunction); - SetProtoMethod( - isolate, db_tmpl, "createTagStore", DatabaseSync::CreateTagStore); - SetProtoMethodNoSideEffect( - isolate, db_tmpl, "location", DatabaseSync::Location); - SetProtoMethod( - isolate, db_tmpl, "aggregate", DatabaseSync::AggregateFunction); - SetProtoMethod( - isolate, db_tmpl, "createSession", DatabaseSync::CreateSession); - SetProtoMethod( - isolate, db_tmpl, "applyChangeset", DatabaseSync::ApplyChangeset); - SetProtoMethod(isolate, - db_tmpl, - "enableLoadExtension", - DatabaseSync::EnableLoadExtension); - SetProtoMethod( - isolate, db_tmpl, "enableDefensive", DatabaseSync::EnableDefensive); - SetProtoMethod( - isolate, db_tmpl, "loadExtension", DatabaseSync::LoadExtension); - SetProtoMethod(isolate, db_tmpl, "serialize", DatabaseSync::Serialize); - SetProtoMethod(isolate, db_tmpl, "deserialize", DatabaseSync::Deserialize); - SetProtoMethod( - isolate, db_tmpl, "setAuthorizer", DatabaseSync::SetAuthorizer); - SetSideEffectFreeGetter(isolate, - db_tmpl, - FIXED_ONE_BYTE_STRING(isolate, "isOpen"), - DatabaseSync::IsOpenGetter); - SetSideEffectFreeGetter(isolate, - db_tmpl, - FIXED_ONE_BYTE_STRING(isolate, "isTransaction"), - DatabaseSync::IsTransactionGetter); - SetSideEffectFreeGetter(isolate, - db_tmpl, - FIXED_ONE_BYTE_STRING(isolate, "limits"), - DatabaseSync::LimitsGetter); - Local sqlite_type_key = FIXED_ONE_BYTE_STRING(isolate, "sqlite-type"); - Local sqlite_type_symbol = - v8::Symbol::For(isolate, sqlite_type_key); - Local database_sync_string = - FIXED_ONE_BYTE_STRING(isolate, "node:sqlite"); - db_tmpl->InstanceTemplate()->Set(sqlite_type_symbol, database_sync_string); - - SetConstructorFunction(context, target, "DatabaseSync", db_tmpl); + SetConstructorFunction(context, + target, + "DatabaseSync", + DatabaseSync::GetConstructorTemplate(env)); SetConstructorFunction(context, target, "StatementSync", diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 306a47f6c47f..a80ec479fac6 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -226,6 +226,8 @@ class DatabaseSync : public BaseObject { bool open, bool allow_load_extension); void MemoryInfo(MemoryTracker* tracker) const override; + static v8::Local GetConstructorTemplate( + Environment* env); static void New(const v8::FunctionCallbackInfo& args); static void Open(const v8::FunctionCallbackInfo& args); static void IsOpenGetter(const v8::FunctionCallbackInfo& args); diff --git a/test/parallel/test-sqlite-backup.mjs b/test/parallel/test-sqlite-backup.mjs index 28eaa4d9b01a..6528418eaf65 100644 --- a/test/parallel/test-sqlite-backup.mjs +++ b/test/parallel/test-sqlite-backup.mjs @@ -48,17 +48,29 @@ describe('backup()', () => { backup(); }, { code: 'ERR_INVALID_ARG_TYPE', - message: 'The "sourceDb" argument must be an object.' + message: 'The "sourceDb" argument must be an instance of DatabaseSync.' }); }); test('throws if the source database is not a DatabaseSync', (t) => { - t.assert.throws(() => { - backup({}, nextDb()); - }, { - code: 'ERR_INVALID_ARG_TYPE', - message: 'The "sourceDb" argument must be a DatabaseSync instance.' - }); + const database = makeSourceDb(); + const values = [ + {}, + [], + { p0: 1, p1: 2, p2: 3, p3: 4 }, + { __proto__: DatabaseSync.prototype }, + database.prepare('SELECT 1'), + database.createSession(), + ]; + + for (const value of values) { + t.assert.throws(() => { + backup(value, nextDb()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "sourceDb" argument must be an instance of DatabaseSync.' + }); + } }); test('throws if path is not a string, URL, or Buffer', (t) => { @@ -97,6 +109,17 @@ describe('backup()', () => { }); }); + test('throws if the database path has an unparsable href', (t) => { + const database = makeSourceDb(); + + t.assert.throws(() => { + backup(database, { href: 'zzz' }); + }, { + code: 'ERR_INVALID_URL', + message: 'Invalid URL' + }); + }); + test('throws if options is not an object', (t) => { const database = makeSourceDb(); diff --git a/test/parallel/test-sqlite-database-sync.js b/test/parallel/test-sqlite-database-sync.js index 5b29169a354a..4e0c7f742f85 100644 --- a/test/parallel/test-sqlite-database-sync.js +++ b/test/parallel/test-sqlite-database-sync.js @@ -60,6 +60,15 @@ suite('DatabaseSync() constructor', () => { }); }); + test('propagates an exception thrown by the href getter', (t) => { + t.assert.throws(() => { + new DatabaseSync({ get href() { throw new RangeError('boom'); } }); + }, { + name: 'RangeError', + message: 'boom', + }); + }); + test('throws if options is provided but is not an object', (t) => { t.assert.throws(() => { new DatabaseSync('foo', null);