Skip to content

Comments

Hotfix/sqli database tools clean#670

Open
mmcintosh wants to merge 2 commits intoSonicJs-Org:mainfrom
mmcintosh:hotfix/sqli-database-tools-clean
Open

Hotfix/sqli database tools clean#670
mmcintosh wants to merge 2 commits intoSonicJs-Org:mainfrom
mmcintosh:hotfix/sqli-database-tools-clean

Conversation

@mmcintosh
Copy link
Contributor

Summary

Security hotfix: prevents SQL injection in the database tools truncate handler. Table names from the request body were interpolated directly into DELETE FROM statements without validation, allowing arbitrary SQL execution if an admin session is compromised.

Changes

1. Table Name Validation via sqlite_master

  • Queries sqlite_master for the set of real database tables before executing any DELETE FROM
  • Each requested table name is checked against this set — names not found are rejected with { success: false, error: 'Table not found' }
  • Uses sqlite_master instead of a hardcoded allowlist so tables from plugins and user migrations are included
  • D1's .prepare().bind() doesn't support parameterized table names, so the DELETE FROM ${tableName} interpolation remains — but is now safe because only validated sqlite_master entries pass through

2. Unit Tests

  • 6 new tests covering valid truncation, invalid table rejection, SQL injection payloads, mixed valid/invalid inputs, empty input, and subquery injection attempts

Technical Details

Core Changes:

  • packages/core/src/routes/admin-settings.tssqlite_master lookup + Set-based validation before DELETE FROM execution (+14 lines)

New Tests:

  • packages/core/src/routes/admin-settings.test.ts — 6 unit tests with mocked D1 and Hono route testing (+199 lines)

Testing

Type Check: PASSED
Unit Tests: PASSED (6 new, all existing passing)
E2E Tests: PASSED (no regression)

New Unit Test Coverage:

  • Valid table name truncation succeeds
  • Invalid/nonexistent table name rejected
  • SQL injection payload (users; DROP TABLE content--) rejected
  • Mixed valid and invalid table names handled correctly
  • Empty table list returns 400
  • Subquery injection attempts (UNION, ATTACH, etc.) all rejected

Performance Impact

Metric Before After Impact
Extra DB query per truncate request 0 1 (sqlite_master lookup) <1ms
Truncate latency baseline ~unchanged Negligible

Breaking Changes

None — the only behavioral change is that invalid table names now return an error instead of attempting (and likely failing) the SQL execution.

Migration Notes

No action required. The fix activates immediately.

Known Issues

None.

Checklist

  • Code follows project coding standards
  • Tests added/updated and passing
  • No breaking changes
  • Backward compatible

The database tools truncate handler interpolated user-supplied table
names directly into DELETE FROM statements without validation, allowing
SQL injection. Now validates each table name against sqlite_master
before executing, rejecting any name not present as an actual table.
Tests validate that table names are checked against sqlite_master
before DELETE execution. Covers valid tables, invalid tables, mixed
lists, empty input, and various SQL injection payloads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant