Skip to content

Commit 514e3f3

Browse files
mcollinaaduh95
mcollina
authored and
aduh95
committed
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 f41509b commit 514e3f3

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
@@ -3387,6 +3387,35 @@ void SQLTagStore::SizeGetter(const FunctionCallbackInfo<Value>& args) {
33873387
args.GetReturnValue().Set(static_cast<double>(store->sql_tags_.Size()));
33883388
}
33893389

3390+
bool SQLTagStore::ResetAndBindStatement(
3391+
Environment* env,
3392+
StatementSync* stmt,
3393+
const FunctionCallbackInfo<Value>& args) {
3394+
Isolate* isolate = env->isolate();
3395+
int r = stmt->ResetStatement();
3396+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, false);
3397+
3398+
r = sqlite3_clear_bindings(stmt->statement_);
3399+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, false);
3400+
3401+
uint32_t n_params = args.Length() - 1;
3402+
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3403+
if (param_count != static_cast<int>(n_params)) {
3404+
THROW_ERR_INVALID_ARG_VALUE(
3405+
env,
3406+
"SQLite parameters must be bound using template literal placeholders.");
3407+
return false;
3408+
}
3409+
3410+
for (int i = 0; i < param_count; ++i) {
3411+
if (!stmt->BindValue(args[i + 1], i + 1)) {
3412+
return false;
3413+
}
3414+
}
3415+
3416+
return true;
3417+
}
3418+
33903419
void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
33913420
SQLTagStore* session;
33923421
ASSIGN_OR_RETURN_UNWRAP(&session, args.This());
@@ -3401,15 +3430,8 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
34013430
return;
34023431
}
34033432

3404-
uint32_t n_params = args.Length() - 1;
3405-
int r = stmt->ResetStatement();
3406-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
3407-
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3408-
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
3409-
Local<Value> value = args[i + 1];
3410-
if (!stmt->BindValue(value, i + 1)) {
3411-
return;
3412-
}
3433+
if (!ResetAndBindStatement(env, stmt.get(), args)) {
3434+
return;
34133435
}
34143436

34153437
Local<Object> result;
@@ -3434,15 +3456,8 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
34343456
return;
34353457
}
34363458

3437-
uint32_t n_params = args.Length() - 1;
3438-
int r = stmt->ResetStatement();
3439-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
3440-
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3441-
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
3442-
Local<Value> value = args[i + 1];
3443-
if (!stmt->BindValue(value, i + 1)) {
3444-
return;
3445-
}
3459+
if (!ResetAndBindStatement(env, stmt.get(), args)) {
3460+
return;
34463461
}
34473462

34483463
BaseObjectPtr<StatementSyncIterator> iter = StatementExecutionHelper::Iterate(
@@ -3469,18 +3484,8 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34693484
return;
34703485
}
34713486

3472-
uint32_t n_params = args.Length() - 1;
3473-
Isolate* isolate = env->isolate();
3474-
3475-
int r = stmt->ResetStatement();
3476-
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
3477-
3478-
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3479-
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
3480-
Local<Value> value = args[i + 1];
3481-
if (!stmt->BindValue(value, i + 1)) {
3482-
return;
3483-
}
3487+
if (!ResetAndBindStatement(env, stmt.get(), args)) {
3488+
return;
34843489
}
34853490

34863491
Local<Value> result;
@@ -3508,18 +3513,8 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
35083513
return;
35093514
}
35103515

3511-
uint32_t n_params = args.Length() - 1;
3512-
Isolate* isolate = env->isolate();
3513-
3514-
int r = stmt->ResetStatement();
3515-
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
3516-
3517-
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
3518-
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
3519-
Local<Value> value = args[i + 1];
3520-
if (!stmt->BindValue(value, i + 1)) {
3521-
return;
3522-
}
3516+
if (!ResetAndBindStatement(env, stmt.get(), args)) {
3517+
return;
35233518
}
35243519

35253520
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
@@ -396,6 +396,10 @@ class SQLTagStore : public BaseObject {
396396
private:
397397
static BaseObjectPtr<StatementSync> PrepareStatement(
398398
const v8::FunctionCallbackInfo<v8::Value>& args);
399+
static bool ResetAndBindStatement(
400+
Environment* env,
401+
StatementSync* stmt,
402+
const v8::FunctionCallbackInfo<v8::Value>& args);
399403
BaseObjectWeakPtr<DatabaseSync> database_;
400404
LRUCache<std::string, BaseObjectPtr<StatementSync>> sql_tags_;
401405
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)