Skip to content

Commit

Permalink
sqlite: disable DQS misfeature by default
Browse files Browse the repository at this point in the history
Double-quoted string (DQS) literals are not allowed by the SQL standard,
which defines that text enclosed in double quotes is to be interpreted
as an identifier only and never as a string literal. Nevertheless, for
historical reasons, SQLite allows double-quoted string literals in some
cases, which leads to inconsistent behavior and subtle bugs.

This commit changes the behavior of the built-in Node.js API for SQLite
such that the DQS misfeature is disabled by default. This is recommended
by the developers of SQLite. Users can explicitly enable DQS for
compatibility with legacy database schemas if necessary.

PR-URL: #55297
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
  • Loading branch information
tniessen authored and aduh95 committed Oct 9, 2024
1 parent d51e5a8 commit 57a9d3f
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 4 deletions.
5 changes: 5 additions & 0 deletions doc/api/sqlite.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ added: v22.5.0
legacy database schemas. The enforcement of foreign key constraints can be
enabled and disabled after opening the database using
[`PRAGMA foreign_keys`][]. **Default:** `true`.
* `enableDoubleQuotedStringLiterals` {boolean} If `true`, SQLite will accept
[double-quoted string literals][]. This is not recommended but can be
enabled for compatibility with legacy database schemas.
**Default:** `false`.

Constructs a new `DatabaseSync` instance.

Expand Down Expand Up @@ -332,6 +336,7 @@ exception.
[`sqlite3_sql()`]: https://www.sqlite.org/c3ref/expanded_sql.html
[connection]: https://www.sqlite.org/c3ref/sqlite3.html
[data types]: https://www.sqlite.org/datatype3.html
[double-quoted string literals]: https://www.sqlite.org/quirks.html#dblquote
[in memory]: https://www.sqlite.org/inmemorydb.html
[parameters are bound]: https://www.sqlite.org/c3ref/bind_blob.html
[prepared statement]: https://www.sqlite.org/c3ref/stmt.html
42 changes: 39 additions & 3 deletions src/node_sqlite.cc
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,15 @@ DatabaseSync::DatabaseSync(Environment* env,
Local<Object> object,
Local<String> location,
bool open,
bool enable_foreign_keys_on_open)
bool enable_foreign_keys_on_open,
bool enable_dqs_on_open)
: BaseObject(env, object) {
MakeWeak();
node::Utf8Value utf8_location(env->isolate(), location);
location_ = utf8_location.ToString();
connection_ = nullptr;
enable_foreign_keys_on_open_ = enable_foreign_keys_on_open;
enable_dqs_on_open_ = enable_dqs_on_open;

if (open) {
Open();
Expand Down Expand Up @@ -132,6 +134,17 @@ bool DatabaseSync::Open() {
int r = sqlite3_open_v2(location_.c_str(), &connection_, flags, nullptr);
CHECK_ERROR_OR_THROW(env()->isolate(), connection_, r, SQLITE_OK, false);

r = sqlite3_db_config(connection_,
SQLITE_DBCONFIG_DQS_DML,
static_cast<int>(enable_dqs_on_open_),
nullptr);
CHECK_ERROR_OR_THROW(env()->isolate(), connection_, r, SQLITE_OK, false);
r = sqlite3_db_config(connection_,
SQLITE_DBCONFIG_DQS_DDL,
static_cast<int>(enable_dqs_on_open_),
nullptr);
CHECK_ERROR_OR_THROW(env()->isolate(), connection_, r, SQLITE_OK, false);

int foreign_keys_enabled;
r = sqlite3_db_config(connection_,
SQLITE_DBCONFIG_ENABLE_FKEY,
Expand Down Expand Up @@ -182,6 +195,7 @@ void DatabaseSync::New(const FunctionCallbackInfo<Value>& args) {

bool open = true;
bool enable_foreign_keys = true;
bool enable_dqs = false;

if (args.Length() > 1) {
if (!args[1]->IsObject()) {
Expand Down Expand Up @@ -222,10 +236,32 @@ void DatabaseSync::New(const FunctionCallbackInfo<Value>& args) {
}
enable_foreign_keys = enable_foreign_keys_v.As<Boolean>()->Value();
}

Local<String> enable_dqs_string = FIXED_ONE_BYTE_STRING(
env->isolate(), "enableDoubleQuotedStringLiterals");
Local<Value> enable_dqs_v;
if (!options->Get(env->context(), enable_dqs_string)
.ToLocal(&enable_dqs_v)) {
return;
}
if (!enable_dqs_v->IsUndefined()) {
if (!enable_dqs_v->IsBoolean()) {
node::THROW_ERR_INVALID_ARG_TYPE(
env->isolate(),
"The \"options.enableDoubleQuotedStringLiterals\" argument must be "
"a boolean.");
return;
}
enable_dqs = enable_dqs_v.As<Boolean>()->Value();
}
}

new DatabaseSync(
env, args.This(), args[0].As<String>(), open, enable_foreign_keys);
new DatabaseSync(env,
args.This(),
args[0].As<String>(),
open,
enable_foreign_keys,
enable_dqs);
}

void DatabaseSync::Open(const FunctionCallbackInfo<Value>& args) {
Expand Down
4 changes: 3 additions & 1 deletion src/node_sqlite.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ class DatabaseSync : public BaseObject {
v8::Local<v8::Object> object,
v8::Local<v8::String> location,
bool open,
bool enable_foreign_keys_on_open);
bool enable_foreign_keys_on_open,
bool enable_dqs_on_open);
void MemoryInfo(MemoryTracker* tracker) const override;
static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
static void Open(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand All @@ -45,6 +46,7 @@ class DatabaseSync : public BaseObject {
sqlite3* connection_;
std::unordered_set<StatementSync*> statements_;
bool enable_foreign_keys_on_open_;
bool enable_dqs_on_open_;
};

class StatementSync : public BaseObject {
Expand Down
28 changes: 28 additions & 0 deletions test/parallel/test-sqlite-database-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,34 @@ suite('DatabaseSync() constructor', () => {
t.after(() => { db.close(); });
db.exec('INSERT INTO bar (foo_id) VALUES (1)');
});

test('throws if options.enableDoubleQuotedStringLiterals is provided but is not a boolean', (t) => {
t.assert.throws(() => {
new DatabaseSync('foo', { enableDoubleQuotedStringLiterals: 5 });
}, {
code: 'ERR_INVALID_ARG_TYPE',
message: /The "options\.enableDoubleQuotedStringLiterals" argument must be a boolean/,
});
});

test('disables double-quoted string literals by default', (t) => {
const dbPath = nextDb();
const db = new DatabaseSync(dbPath);
t.after(() => { db.close(); });
t.assert.throws(() => {
db.exec('SELECT "foo";');
}, {
code: 'ERR_SQLITE_ERROR',
message: /no such column: "foo"/,
});
});

test('allows enabling double-quoted string literals', (t) => {
const dbPath = nextDb();
const db = new DatabaseSync(dbPath, { enableDoubleQuotedStringLiterals: true });
t.after(() => { db.close(); });
db.exec('SELECT "foo";');
});
});

suite('DatabaseSync.prototype.open()', () => {
Expand Down

0 comments on commit 57a9d3f

Please sign in to comment.