Skip to content

db: add STRICT tables with migration for old databases#8559

Open
wqxoxo wants to merge 1 commit intoElementsProject:masterfrom
wqxoxo:feat/strict-tables-security
Open

db: add STRICT tables with migration for old databases#8559
wqxoxo wants to merge 1 commit intoElementsProject:masterfrom
wqxoxo:feat/strict-tables-security

Conversation

@wqxoxo
Copy link
Contributor

@wqxoxo wqxoxo commented Sep 21, 2025

Turns out old databases (~2019) can have BLOB values stuck in TEXT columns due to SQLite type affinity, so we can't just slap STRICT on existing tables. This adds STRICT to new CREATE TABLE statements in --developer mode, with a migration that cleans up the BLOB faildetail values in payments first. Also enables trusted_schema=OFF and cell_size_check=ON while we're at it.

For upgraded databases, STRICT is skipped entirely to avoid breaking legacy data.

Fixes #5390.

@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch 9 times, most recently from 415d91c to dec2ae7 Compare September 21, 2025 14:36
@rustyrussell
Copy link
Contributor

  1. STRICT Tables: Enable for developer/testing environments (SQLite 3.37.0+)

This seems good, except that it should be enabled by the --developer flag, not random environment variables. We get much of this checking already by using Postgresql as the backend, but it's definitely an improvement.

  1. Type Safety: Automatic conversion of VARCHAR→TEXT, BIGINT→INTEGER, BIGSERIAL→INTEGER, INT→INTEGER

This is done in the wrong place. We use devtools/sql-rewrite.py to translate our SQL statements into local dialects already. The exception is the sql plugin, but that's also up to the user.

  1. Security Hardening: Enable trusted_schema=OFF, cell_size_check=ON, secure_delete=ON

We don't load untrusted databases, but trusted_schema isn't harmful. cell_size_check will slow us down, and only helps if the db is corrupted (hopefully catching it earlier), so I like that one. secure_delete is not meaningful for us, that I can tell.

  1. Enhanced Error Messages: Clear constraint violation reporting for STRICT tables

Except that doesn't matter since only developers changing the code will ever see such messages, when they add an invalid sql statement.

  1. Memory Safety: Buffer overflow protection and proper error path cleanup

Hi ChatGPT? Or maybe claude?

To be honest, this entire thing should be a several-line patch, which enables the pragmas inside db/db_sqlite3.c

@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch 2 times, most recently from b01b22e to 55927b7 Compare September 22, 2025 13:58
@wqxoxo
Copy link
Contributor Author

wqxoxo commented Sep 22, 2025

Thank you for the feedback @rustyrussell . I've reworked the implementation based on your guidance, I hope this is more aligned with what you had in mind.

@madelinevibes madelinevibes added BOUNTY! 🫰 A bounty is available for this PR 25.09.1 labels Sep 26, 2025
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch from 55927b7 to eac29a0 Compare October 2, 2025 20:58
@madelinevibes madelinevibes added this to the v25.12 milestone Oct 20, 2025
@rustyrussell rustyrussell modified the milestones: v25.12, v26.03 Nov 13, 2025
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch 2 times, most recently from 03dd29c to 03a2736 Compare November 24, 2025 09:31
wqxoxo added a commit to wqxoxo/lightning that referenced this pull request Nov 25, 2025
Enables STRICT tables in developer mode, but old databases (~2019) may
have BLOB values in TEXT columns. Migration converts BLOB faildetail
to TEXT with UTF-8 validation, NULLs invalid data.

Also adds security pragmas in developer mode: trusted_schema=OFF,
cell_size_check=ON, mmap_size=0, quick_check on startup.

Fixes ElementsProject#7913, fixes migration failures in PR ElementsProject#8559.

Changelog-Added: Database: SQLite STRICT tables in developer mode
Changelog-Added: Database: Security pragmas in developer mode
Changelog-Fixed: Database migration for old BLOB-typed faildetail values
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch from 03a2736 to 878d7c7 Compare November 25, 2025 15:11
@wqxoxo wqxoxo requested a review from cdecker as a code owner November 25, 2025 15:11
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch 5 times, most recently from e6264a8 to 1c3cdc5 Compare November 25, 2025 20:39
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch from 1c3cdc5 to 18c0d94 Compare November 26, 2025 07:35
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch 6 times, most recently from 582a105 to 76d6d7c Compare December 11, 2025 15:14
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch 4 times, most recently from 11edd24 to 64e0272 Compare February 9, 2026 18:06
@wqxoxo wqxoxo closed this Feb 9, 2026
@wqxoxo wqxoxo reopened this Feb 9, 2026
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch from 64e0272 to 08154c5 Compare February 9, 2026 18:11
@wqxoxo wqxoxo changed the title db: implement SQLite STRICT tables and security hardening db: add STRICT tables with migration for old databases Feb 17, 2026
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch from 08154c5 to 1374314 Compare February 17, 2026 16:11
Enables STRICT tables in developer mode, but old databases (~2019) may
have BLOB values in TEXT columns. Migration converts BLOB faildetail
to TEXT with UTF-8 validation, NULLs invalid data.

STRICT is only applied to fresh databases; existing databases being
upgraded skip STRICT to avoid type affinity issues with legacy data.

Also adds security pragmas in developer mode: trusted_schema=OFF,
cell_size_check=ON.

Fixes ElementsProject#5390.

Changelog-Added: Database: STRICT tables and security pragmas in developer mode
Changelog-Fixed: Database migration for old BLOB-typed faildetail values
@wqxoxo wqxoxo force-pushed the feat/strict-tables-security branch from 1374314 to 2d8cca4 Compare February 17, 2026 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BOUNTY! 🫰 A bounty is available for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Turn on strict tables for SQLITE3 when running integration tests

3 participants