[SECURITY] Database security - connection pool, atomic transactions, health monitor - closes #54#83
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new SQLite-focused database utility module (connection pooling, atomic transactions w/ retry, sanitization, and health monitoring) plus unit tests to validate core behaviors.
Changes:
- Introduces
pt_database_manager.pywith connection pooling,atomic_transaction, input sanitization, and a background integrity-check monitor. - Adds
test_database_manager.pycovering connection pool behavior, transaction commit/rollback, sanitizer behavior, and health monitor lifecycle.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| app/pt_database_manager.py | Implements SQLite connection pool, atomic transaction context manager with retry/backoff, input sanitization, and periodic integrity health monitoring. |
| app/test_database_manager.py | Adds unit tests for the new database manager components. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sjackson0109
added
high-priority
security
component-security
sjackson0109
left a comment
sjackson0109
left a comment
There was a problem hiding this comment.
A logical set of changes. Appreciate your efforts here, Ibrahim.
|
@Ibrahim-3d Brilliant effort. Can you review the copilot comments/suggestions here - #83 (review) |
|
All 11 Copilot review comments addressed in the fix commit. Inline replies posted directly on each thread. All threads resolved.
|
Local CI Simulation ResultsRan all workflow steps locally against this branch prior to owner approval. Results: Code Quality & Testing
Unit Test Results
CI/CD Pipeline
|
|
Would welcome your thoughts @Ibrahim-3d - Gould I permit the code quality ci pipelines to run automatically? |
…ealth monitor pt_database_manager.py: - DatabaseConnectionPool with thread-local connections and serialized creation (prevents SQLITE_LOCKED during concurrent PRAGMA journal_mode=WAL setup) - atomic_transaction() context manager with exponential-backoff retry on SQLITE_BUSY - InputSanitizer with SQL injection heuristics and identifier validation - DatabaseHealthMonitor daemon thread with PRAGMA integrity_check and corrupt callback - 28 unit tests, all passing
…ion safety - atomic_transaction re-raises original sqlite3.OperationalError for non-contention errors; only wraps as TransactionError when retry limit exceeded on busy/locked - check_health uses try/finally to close connection even when integrity_check raises - Log interval format changed from %d to %s (float-safe) - Module docstring updated: remove "deadlock detection" claim (SQLite uses locking, not deadlocks); describe retry-on-contention behavior accurately - Tests: unittest.mock imported at module level (not inside __main__ guard) - Tests: test_non_contention_error propagates sqlite3.OperationalError (not TransactionError) - Tests: test_contention_retries uses real write lock + TransactionError assertion - Tests: tearDown closes thread-local connections before rmtree (prevents Windows lock) - Tests: threading.Barrier ensures concurrent thread test reliability
Ibrahim-3d
force-pushed
the
feat/54-database-security-transactions
branch
from
2cbb859 to
d6b5d59
Compare
|
@Ibrahim-3d loiks like all CI pipelines passed. Please can you review copilot’s comments here #83 (review) |
- atomic_transaction: add COMMIT contention retry loop so SQLITE_BUSY on COMMIT triggers backoff-retry up to max_retries, not silent failure - check_same_thread: change False->True; thread-local pool guarantees single-thread access, letting SQLite enforce this as a safety net - check_sql_injection: replace substring match with word-boundary regex for alphabetic keywords to eliminate false positives on values like "dropbox", "selection", "truncate_me"; punctuation tokens (-- ;) still use exact substring match - stop(): check is_alive() after join timeout and log warning if thread did not terminate within 5s - tests: close victim connection in contention test to prevent handle leak; add test_commit_contention_retries; add test_check_sql_injection_no_false_positives; add test_check_sql_injection_punctuation_tokens
Manual Testing Guide — PR #83: Database Security (Connection Pool, Atomic Transactions, Health Monitor)Prerequisitesgit fetch fork && git checkout feat/54-database-security-transactions
cd app1. Run unit testspython -m pytest test_database_manager.py -vExpected: All 29 tests pass, including new 2. Smoke test — atomic transaction commit + rollbackpython -c "
import sqlite3, tempfile, os
from pt_database_manager import DatabaseConnectionPool, atomic_transaction
with tempfile.TemporaryDirectory() as d:
db = os.path.join(d, 'test.db')
pool = DatabaseConnectionPool(db)
conn = pool.get_connection()
conn.execute('CREATE TABLE t (id INTEGER PRIMARY KEY, val TEXT)')
# Commit path
with atomic_transaction(conn) as c:
c.execute(\"INSERT INTO t VALUES (1, 'hello')\")
row = conn.execute('SELECT val FROM t WHERE id=1').fetchone()
print('commit ok:', row[0])
# Rollback path
try:
with atomic_transaction(conn) as c:
c.execute(\"INSERT INTO t VALUES (2, 'will rollback')\")
raise ValueError('forced')
except ValueError:
pass
row2 = conn.execute('SELECT * FROM t WHERE id=2').fetchone()
assert row2 is None, 'rollback failed'
print('rollback ok')
pool.close_thread_connection()
"3. Verify SQL injection heuristic — no false positives (key change)python -c "
from pt_database_manager import InputSanitizer
safe = ['dropbox', 'selection', 'truncate_me', 'executor', 'BTC-USD', 'user@email.com']
for v in safe:
hit = InputSanitizer.check_sql_injection(v)
print(f' {v!r}: {\"FALSE POSITIVE\" if hit else \"ok\"}')
# Real injections must still be detected
injections = [\"DROP TABLE users\", \"1' UNION SELECT--\", \"val; DELETE\"]
for v in injections:
hit = InputSanitizer.check_sql_injection(v)
print(f' {v!r}: {\"detected\" if hit else \"MISSED\"}')
"Expected: all safe values show 4. Health monitor start/stoppython -c "
import tempfile, os, sqlite3, time
from pt_database_manager import DatabaseConnectionPool, DatabaseHealthMonitor
with tempfile.TemporaryDirectory() as d:
db = os.path.join(d, 'health.db')
sqlite3.connect(db).execute('CREATE TABLE x (id INTEGER)')
mon = DatabaseHealthMonitor(db, check_interval=1)
mon.start()
print('running:', mon._thread.is_alive())
time.sleep(0.2)
mon.stop()
print('stopped:', not mon._thread.is_alive())
print('last_check_ok:', mon._last_check_ok)
"Rollbackgit checkout main -- app/pt_database_manager.py app/test_database_manager.py |
|
/gemini review |
|
@Ibrahim-3d And the review (copilot) brings a small number of high risk items to the table: mind having a look? |
- pt_database_manager: replace literal backspace char in _SQL_KEYWORD_RE with proper \b word boundaries (raw string was missing escape, compiled to \x08 instead of regex boundary, breaking all SQL keyword detection) - pt_database_manager: build _SQL_KEYWORD_RE from _SQL_KEYWORDS_WORD set to eliminate duplicate keyword list and prevent drift - pt_database_manager: restrict atomic_transaction retry detection to "database is busy" / "database is locked" only; "cannot start ..." no longer triggers retry (was matching nested-transaction config errors) - test_database_manager: rewrite test_commit_contention_retries with a transparent connection wrapper; sqlite3.Connection.execute became read-only in Python 3.12 and could no longer be patch.object'd - gitignore: add security_audit.jsonl and rotated variants so runtime audit logs never get committed accidentally
|
@sjackson0109 round-3 done — addressed in 84a36e3:
Also fixed CI: Python 3.12 made 32 unit tests pass locally. Threads resolved. |
Ibrahim-3d
force-pushed
the
feat/54-database-security-transactions
branch
from
84a36e3 to
c1b140a
Compare
|
@Ibrahim-3d sorry there are two more from copilot’s review. You should be able to tag copilot to ‘restart the review’. |
|
@sjackson0109 quick one - on the Copilot restart-review thing you mentioned earlier, idk why Your triggers work, mine don't. I have Copilot Pro on my account but tagging |
…ilability) Two Copilot review threads: 1) atomic_transaction docstring overstated retry scope. The retry loop only re-runs BEGIN IMMEDIATE / COMMIT contention. Exceptions raised inside the with block (including busy/locked) are rolled back and re-raised unchanged - they cannot be retried because @contextmanager generators can only yield once. Clarified in docstring under a new 'Retry scope' section so callers know to wrap body statements in their own retry loop if they need contention retry there. 2) DatabaseHealthMonitor.on_corrupt fired for any check_health failure, including missing file or transient sqlite IO errors. This could trigger emergency-stop on non-corruption conditions. Added new IntegrityStatus enum (OK / CORRUPT / UNAVAILABLE) and check_integrity() method on the pool. Monitor now only fires on_corrupt for true CORRUPT result; UNAVAILABLE routes to a new optional on_unavailable callback so callers can alert / retry rather than emergency-stop. Old check_health() / check_now() kept as back-compat boolean shims. Tests: 4 new cases covering UNAVAILABLE classification, on_corrupt guard against missing-file, OK classification, last_status exposure. 36 total pass.
can we chat offline about this. Discord? |
Summary
Creates
pt_database_manager.py— standalone database security layer that complements the existingOrderManagementDB/SQLAlchemy stack without modifying it.Changes
app/pt_database_manager.pyapp/test_database_manager.py— 28 testsKey components
DatabaseConnectionPoolatomic_transaction()BEGIN IMMEDIATE+ exponential-backoff retry onSQLITE_BUSYInputSanitizerDatabaseHealthMonitorPRAGMA integrity_checkon schedule with corrupt callbackUsage
Test plan
Closes #54