sqlite: validate backup() source and URL-like paths by TrevorBurnham · Pull Request #65832 · nodejs/node · GitHub
Skip to content

sqlite: validate backup() source and URL-like paths - #65832

Draft
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite-argument-validation
Draft

sqlite: validate backup() source and URL-like paths#65832
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite-argument-validation

Conversation

@TrevorBurnham

Copy link
Copy Markdown
Contributor

Fixes: #65830

Two argument-validation gaps in node:sqlite, both reachable from a single call with no callbacks or timing involved.

backup() reinterprets any object as a DatabaseSync*

Backup() checked only args[0]->IsObject() before ASSIGN_OR_RETURN_UNWRAP. Passing a plain object crashed; passing a different BaseObject subclass found a valid pointer in the internal field and read IsOpen() and connection_ at the wrong offsets:

sourceDb Before
{} SIGSEGV / SIGBUS
[] FATAL ERROR: GetAlignedPointerFromInternalField() Internal field out of bounds
db.prepare('SELECT 1') SIGSEGV, occasionally ERR_INVALID_STATE
db.createSession() spurious ERR_INVALID_STATE: database is not open

This is the only one of the unwrap sites in src/node_sqlite.cc that takes a value out of args[...]; the rest use args.This() or an interceptor holder, both already covered by V8's signature check. DatabaseSync was the one class in the file with no constructor template stored on the Environment, so Backup() had nothing to test against. This adds the Environment slot and DatabaseSync::GetConstructorTemplate() alongside the ones StatementSync, StatementSyncIterator, and Session already have, and checks HasInstance() before unwrapping.

The error message changes from The "sourceDb" argument must be an object. to The "sourceDb" argument must be an instance of DatabaseSync., matching what the docs already specify for the parameter.

ValidateDatabasePath() aborts on a duck-typed URL

Any object with a string href is treated as a URL, and the parse result was asserted rather than checked, so new DatabaseSync({ href: 'zzz' }) and backup(db, { href: 'zzz' }) hit CHECK(ada::can_parse(location)) and aborted. An unparseable href now falls through to the same ERR_INVALID_ARG_TYPE that other non-URL objects get.

While in that branch: a throwing href getter had its exception discarded and replaced by ERR_INVALID_ARG_TYPE. It now propagates.

Moved code

SetSideEffectFreeGetter() moved up to the top of the file so DatabaseSync::GetConstructorTemplate() can use it. The body is unchanged.

Validation

All nine repro cases now throw TypeErrors. parallel/test-sqlite*, parallel/test-webstorage*, and parallel/test-permission-sqlite-load-extension pass, plus a check that a real URL path, Symbol.for('sqlite-type'), and backup() on a DatabaseSync subclass still behave as before.

backup() checked only that its first argument was an object before
unwrapping it as a DatabaseSync, so passing any other object
reinterpreted foreign memory as a database handle. Results ranged from
SIGSEGV to a spurious ERR_INVALID_STATE, depending on the object's
layout. It is the only unwrap site in node_sqlite.cc that takes a value
out of args[], and so the only one that V8's signature check for
args.This() does not already protect. DatabaseSync had no constructor
template on the Environment to test against, so add one alongside the
other sqlite classes and use it.

ValidateDatabasePath() treats any object with a string href as a URL
and asserted that the href parsed, aborting the process on, for
example, new DatabaseSync({ href: 'zzz' }). Reject an unparseable href
the same way other non-URL objects are rejected, and stop replacing an
exception thrown by the href getter with ERR_INVALID_ARG_TYPE.

Fixes: nodejs#65830

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
Assisted-by: Claude Opus 5
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:sqlite: backup() unwraps any object as a DatabaseSync, and a duck-typed URL aborts the process

2 participants