sqlite: reject connection access from authorizer callbacks - #65156
Draft
TrevorBurnham wants to merge 1 commit into
Draft
sqlite: reject connection access from authorizer callbacks#65156TrevorBurnham wants to merge 1 commit into
TrevorBurnham wants to merge 1 commit into
Conversation
SQLite requires that an authorizer callback not modify the connection that invoked it, and counts sqlite3_prepare_v2() and sqlite3_step() as modifications. node:sqlite let an authorizer callback call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync. Track authorizer depth on DatabaseSync with an RAII guard around the callback, and throw ERR_INVALID_STATE from the affected entry points while the callback is on the stack. The depth is per-connection, so other connections stay usable from the callback. The guard covers every authorizer invocation, not just those from an explicit prepare(), since SQLite may re-prepare a statement during sqlite3_step() after a schema change. That re-prepare case also made statement.close() and statement[Symbol.dispose]() crash rather than throw, because finalizing a statement frees the VM that the enclosing sqlite3_step() is still executing. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Fixes: nodejs#63207 Assisted-by: claude:opus-5
Collaborator
|
Review requested:
|
meixg
approved these changes
Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Per the
sqlite3_set_authorizer()docs, the authorizer callback must not do anything that modifies the database connection that invoked it, andsqlite3_prepare_v2()andsqlite3_step()both count as modifications.node:sqliteallowed an authorizer callback to callprepare(),exec(), the statement execution methods, and other connection-mutating APIs on the sameDatabaseSync.Track authorizer depth on
DatabaseSyncwith an RAII guard around the callback, and throwERR_INVALID_STATEfrom the affected entry points while the callback is on the stack. The depth is per-connection, so a differentDatabaseSyncstays usable from inside the callback.Guarded:
prepare,exec,deserialize,setAuthorizer,createSession,applyChangeset,createTagStore,function,aggregate,enableLoadExtension,enableDefensive,loadExtension, thelimitssetter;stmt.run/get/all/iterate/closeandstmt[Symbol.dispose];iter.next/return;sqlTagStore.run/get/all/iterate/clear.db.close()was already rejected by the existing callback-depth guard, so it keeps its current message.The guard covers every authorizer invocation, not just those from an explicit
prepare(), since SQLite may re-prepare a statement duringsqlite3_step()after a schema change.Crash found while testing
That re-prepare case turned out to be worse than a contract violation: calling
statement.close()orstatement[Symbol.dispose]()from an authorizer invoked duringsqlite3_step()segfaulted, because finalizing the statement frees the VM the enclosingstep()is still executing. Both are now guarded.Note that the same finalize-during-step crash is reachable from a user-defined function without any authorizer involved:
That path is outside the scope of this PR and is left unfixed; it likely wants the deferred-finalize approach from #63183 rather than a throw, since UDFs are otherwise permitted to call back into the connection. Happy to file it separately.
Notes for reviewers
node:sqliteis Stability 1.2, this changes behavior directly rather than going through a deprecation cycle, per the discussion in the issue. Throwing immediately (rather than deferring) matches the preference expressed by both participants in that thread.sqlTagStore.clear()only clears a JS-side statement cache and doesn't touch SQLite, so guarding it is arguably unnecessary. I included it because the issue lists it and because finalizing cached statements mid-callback is the kind of thing the contract exists to prevent. Easy to drop if reviewers prefer.Verification
test-webstorage.make format-cpp,lint-cpp,lint-md, and eslint are clean.Prepareguard and rebuilt: 3 tests failed, then passed again on restore.Fixes: #63207