Skip to content

Commit 8fc7341

Browse files
mcollinamcollina
authored
sqlite: clear SQLTagStore bindings
SQLTagStore reused cached statements without clearing values bound by a previous execution. Clear bindings before binding new values and reject parameters that do not correspond to template substitutions. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #65041 Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
1 parent d7ed670 commit 8fc7341

3 files changed

Lines changed: 78 additions & 42 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 37 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -3431,6 +3431,35 @@ void SQLTagStore::SizeGetter(const FunctionCallbackInfo<Value>& args) {
34313431
args.GetReturnValue().Set(static_cast<double>(store->sql_tags_.Size()));
34323432
}
34333433

3434+
bool SQLTagStore::ResetAndBindStatement(
3435+
Environment* env,
3436+
StatementSync* stmt,
3437+
const FunctionCallbackInfo<Value>& args) {
3438+
Isolate* isolate = env->isolate();
3439+
int r = stmt->ResetStatement();
3440+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, false);
3441+
3442+
r = sqlite3_clear_bindings(stmt->statement_);
3443+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, false);
3444+
3445+
uint32_t n_params = args.Length() - 1;
3446+
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3447+
if (param_count != static_cast<int>(n_params)) {
3448+
THROW_ERR_INVALID_ARG_VALUE(
3449+
env,
3450+
"SQLite parameters must be bound using template literal placeholders.");
3451+
return false;
3452+
}
3453+
3454+
for (int i = 0; i < param_count; ++i) {
3455+
if (!stmt->BindValue(args[i + 1], i + 1)) {
3456+
return false;
3457+
}
3458+
}
3459+
3460+
return true;
3461+
}
3462+
34343463
void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
34353464
SQLTagStore* session;
34363465
ASSIGN_OR_RETURN_UNWRAP(&session, args.This());
@@ -3445,15 +3474,8 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
34453474
return;
34463475
}
34473476

3448-
uint32_t n_params = args.Length() - 1;
3449-
int r = stmt->ResetStatement();
3450-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
3451-
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3452-
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
3453-
Local<Value> value = args[i + 1];
3454-
if (!stmt->BindValue(value, i + 1)) {
3455-
return;
3456-
}
3477+
if (!ResetAndBindStatement(env, stmt.get(), args)) {
3478+
return;
34573479
}
34583480

34593481
Local<Object> result;
@@ -3478,15 +3500,8 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
34783500
return;
34793501
}
34803502

3481-
uint32_t n_params = args.Length() - 1;
3482-
int r = stmt->ResetStatement();
3483-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
3484-
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3485-
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
3486-
Local<Value> value = args[i + 1];
3487-
if (!stmt->BindValue(value, i + 1)) {
3488-
return;
3489-
}
3503+
if (!ResetAndBindStatement(env, stmt.get(), args)) {
3504+
return;
34903505
}
34913506

34923507
BaseObjectPtr<StatementSyncIterator> iter = StatementExecutionHelper::Iterate(
@@ -3513,18 +3528,8 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
35133528
return;
35143529
}
35153530

3516-
uint32_t n_params = args.Length() - 1;
3517-
Isolate* isolate = env->isolate();
3518-
3519-
int r = stmt->ResetStatement();
3520-
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
3521-
3522-
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3523-
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
3524-
Local<Value> value = args[i + 1];
3525-
if (!stmt->BindValue(value, i + 1)) {
3526-
return;
3527-
}
3531+
if (!ResetAndBindStatement(env, stmt.get(), args)) {
3532+
return;
35283533
}
35293534

35303535
Local<Value> result;
@@ -3552,18 +3557,8 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35523557
return;
35533558
}
35543559

3555-
uint32_t n_params = args.Length() - 1;
3556-
Isolate* isolate = env->isolate();
3557-
3558-
int r = stmt->ResetStatement();
3559-
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
3560-
3561-
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3562-
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
3563-
Local<Value> value = args[i + 1];
3564-
if (!stmt->BindValue(value, i + 1)) {
3565-
return;
3566-
}
3560+
if (!ResetAndBindStatement(env, stmt.get(), args)) {
3561+
return;
35673562
}
35683563

35693564
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });

‎src/node_sqlite.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,10 @@ class SQLTagStore : public BaseObject {
404404
private:
405405
static BaseObjectPtr<StatementSync> PrepareStatement(
406406
const v8::FunctionCallbackInfo<v8::Value>& args);
407+
static bool ResetAndBindStatement(
408+
Environment* env,
409+
StatementSync* stmt,
410+
const v8::FunctionCallbackInfo<v8::Value>& args);
407411
BaseObjectWeakPtr<DatabaseSync> database_;
408412
LRUCache<std::string, BaseObjectPtr<StatementSync>> sql_tags_;
409413
friend class StatementExecutionHelper;

‎test/parallel/test-sqlite-template-tag.js‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,43 @@ test('queries with no results', () => {
9090
assert.strictEqual(count, 0);
9191
});
9292

93+
test('rejects parameters outside of template expressions', () => {
94+
const ldb = new DatabaseSync(':memory:');
95+
const lsql = ldb.createTagStore();
96+
ldb.exec(`
97+
CREATE TABLE secrets(owner TEXT, token TEXT);
98+
INSERT INTO secrets VALUES ('victim', 'secret');
99+
CREATE TABLE transfers(from_user TEXT, amount INTEGER);
100+
`);
101+
102+
const expectedError = {
103+
code: 'ERR_INVALID_ARG_VALUE',
104+
message: /must be bound using template literal placeholders/,
105+
};
106+
107+
for (const method of ['get', 'all', 'iterate']) {
108+
// Prime the cached statement with a bound value before each attempt.
109+
// eslint-disable-next-line no-unused-expressions
110+
lsql.all`SELECT token FROM secrets WHERE owner = ${'victim'}`;
111+
assert.throws(() => {
112+
// eslint-disable-next-line no-unused-expressions
113+
lsql[method]`SELECT token FROM secrets WHERE owner = ?`;
114+
}, expectedError);
115+
}
116+
117+
// eslint-disable-next-line no-unused-expressions
118+
lsql.run`INSERT INTO transfers VALUES (${'victim'},${100})`;
119+
assert.throws(() => {
120+
// eslint-disable-next-line no-unused-expressions
121+
lsql.run`INSERT INTO transfers VALUES (?,?)`;
122+
}, expectedError);
123+
assert.strictEqual(
124+
ldb.prepare('SELECT COUNT(*) AS count FROM transfers').get().count,
125+
1);
126+
127+
ldb.close();
128+
});
129+
93130
test('TagStore capacity, size, and clear', () => {
94131
assert.strictEqual(sql.capacity, 10);
95132
assert.strictEqual(sql.size, 0);

0 commit comments

Comments
 (0)