Skip to content

deps: cherry-pick e807d4e379 from SQLite#63525

Open
junius-sec wants to merge 1 commit into
nodejs:mainfrom
junius-sec:sqlite-changeset-null-pk
Open

deps: cherry-pick e807d4e379 from SQLite#63525
junius-sec wants to merge 1 commit into
nodejs:mainfrom
junius-sec:sqlite-changeset-null-pk

Conversation

@junius-sec
Copy link
Copy Markdown

Backport the SQLite session extension fix for malformed changesets that omit
old values for primary-key columns. The upstream fix avoids passing NULL to
sessionBindValue() while applying UPDATE changesets.

This adds a regression test for DatabaseSync#applyChangeset() to verify that
the malformed changeset returns SQLITE_CORRUPT instead of crashing.

Refs: https://sqlite.org/src/info/e807d4e3798efd53
Refs: https://hackerone.com/reports/3736889

Tested on Linux x64:

$ python3 configure.py --without-npm --without-lief
$ make -j16
$ python3 tools/test.py --mode=release test/parallel/test-sqlite-session.js
All tests passed.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg
  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels May 23, 2026
Copy link
Copy Markdown
Contributor

@louwers louwers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just stick to the regular SQLite release cadence I think.

@junius-sec
Copy link
Copy Markdown
Author

junius-sec commented May 23, 2026

Thanks for reviewing, @louwers.

I opened this PR because the disclosed HackerOne thread suggested that the
upstream fix could be cherry-picked publicly for the next regular releases and
that opening the PR could allow changelog credit.

If the project prefers to wait for the regular SQLite release cadence instead,
I’m happy to defer to that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm agnostic towards merging, but we definitely shouldn't be regression testing on sqlite3's behalf, as it'll cause havoc with shared builds.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, thanks. I pushed an update to skip this regression test when Node is
built with shared SQLite, so it only runs against the bundled SQLite copy that
this PR patches.

If you would prefer not to carry this SQLite regression test in Node at all,
I’m happy to remove it.

Backport the SQLite session extension fix for corrupt changesets that
omit old values for primary-key columns. This avoids passing NULL to
sessionBindValue() while applying UPDATE changesets.

Refs: https://sqlite.org/src/info/e807d4e3798efd53
Signed-off-by: junius-sec <sksch323@naver.com>
@junius-sec junius-sec force-pushed the sqlite-changeset-null-pk branch from 0844bba to f0cdb8e Compare May 23, 2026 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants