sqlite: validate the backup() source database

[lazerg]

backup() checked only that its first argument was an object before unwrapping it as a Database, so backup({}, path) crashed with a segfault. It now checks the argument against the Database constructor template and throws ERR_INVALID_ARG_TYPE instead. The template now lives behind Database::GetConstructorTemplate(), like Statement, Session and SQLTagStore.

ValidateDatabasePath() also lets an exception thrown by an href getter propagate instead of replacing it with ERR_INVALID_ARG_TYPE.

The duck-typed URL crash from the original version of this PR was fixed separately in 25b205b, so this rebase drops that part.

Fixes: #65830

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[github-actions]

Caution

AgentScan found account activity patterns that may be consistent with automation. This is a heuristic, not proof that this pull request was opened by an agent or violates policy. AI-assisted contributions are permitted, but automated tooling must not open pull requests without advance approval, and contributors must personally understand, test, verify, and take responsibility for every submitted change. See the AgentScan analysis, AI use policy, and automation policy for additional context.

[lazerg]

I used agent to resolve the problem. I confirm I revied code myself.

[TrevorBurnham]

FYI I posted a draft PR around the same time this one went up: #65832 A few things from it may be worth folding in here:

1. Consider storing the template behind a DatabaseSync::GetConstructorTemplate(env) accessor. That's consistent with established patterns and would avoid re-assigning the slot if Initialize() ever ran for more than one realm.
2. The foreign-BaseObject cases are worth pinning too. backup({}) covers the plain-object path, but the other variants in the issue fail differently: backup([]) hits FATAL ERROR: GetAlignedPointerFromInternalField() Internal field out of bounds rather than a segfault, and passing a different BaseObject subclass finds a valid pointer in the internal field and reinterprets it at the wrong offsets. { __proto__: DatabaseSync.prototype } would be cheap to add as well.
3. ValidateDatabasePath() has two callers. backup(db, { href: 'zzz' }) reaches the same CHECK.
4. Minor: elsewhere in src/ the wording is "must be an instance of X" rather than "must be a X instance".

[lazerg]

Thanks, this was useful. I went through all four against the source and pushed bf0648c.

1. Done. Four other classes in the file already have a GetConstructorTemplate(), and DatabaseSync was the only one building its template inline in Initialize(), so the accessor is the more consistent shape. The re-assignment point holds: a second Initialize() would build a fresh template and overwrite the slot, and instances from the first realm would then fail HasInstance(). I took your version of the accessor, including moving SetSideEffectFreeGetter() up, and credited you on the commit.

2. The guard already covered these, since HasInstance() is a template check and does not care what shape of object it gets. Confirmed on a build: [], { p0: 1, p1: 2, p2: 3, p3: 4 }, { __proto__: DatabaseSync.prototype }, a StatementSync and a Session all throw ERR_INVALID_ARG_TYPE. None of them were tested though, so all six are now in the backup test.

3. Same story. The CHECK is inside ValidateDatabasePath(), so replacing it fixed both callers at once. backup(db, { href: 'zzz' }) now has a test of its own.

4. Changed. lib/internal/errors.js builds "an instance of X" for ERR_INVALID_ARG_TYPE, which settles it. I also dropped the IsObject() check ahead of the guard, so backup() with no argument gives that message too instead of "must be an object.".

One more from your diff that was not in your list: ValidateDatabasePath() discarded an exception thrown by an href getter and put ERR_INVALID_ARG_TYPE over it. I took that fix as well.

The one place I kept my version is the unparseable href. You fall through to ERR_INVALID_ARG_TYPE; I throw ERR_INVALID_URL, following node_file.cc. Both read fine to me and I have no strong preference, so I am happy to switch if a reviewer prefers yours.

On the two PRs: yours was the more complete one, and most of what is now here came from it. If you would rather land yours, say so and I will close this in its favour. Otherwise I believe this one now covers the same ground.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.18%. Comparing base (6f41e41) to head (bf0648c).
⚠️ Report is 263 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65831      +/-   ##
==========================================
+ Coverage   90.17%   90.18%   +0.01%     
==========================================
  Files         770      770              
  Lines      264483   264492       +9     
  Branches    50262    50266       +4     
==========================================
+ Hits       238491   238544      +53     
+ Misses      16981    16938      -43     
+ Partials     9011     9010       -1     

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.32% <100.00%> (+0.08%)	⬆️
src/node_sqlite.h	86.40% <ø> (ø)

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lazerg]

Hey @geeksilva97 @jasnell, please take a look when you have time

[geeksilva97]

Hey @geeksilva97 @jasnell, please take a look when you have time

could you rebase this PR?