fix: remove hardcoded fallback password from DB connection #84

cal · 2026-03-27T04:15:42Z

cal commented

2026-03-27 04:15:42 +00:00

Summary

Removes 'sba_dev_password_2024' fallback default from PostgreSQL connection setup in db_engine.py
App now raises RuntimeError on startup if POSTGRES_PASSWORD env var is not set
Other non-sensitive connection defaults (host, port, db name, user) are left as-is

Test plan

Verify API fails with clear error when POSTGRES_PASSWORD is unset
Verify API starts normally when POSTGRES_PASSWORD is set
Confirm dev and prod docker-compose files set POSTGRES_PASSWORD

🤖 Generated with Claude Code

## Summary - Removes `'sba_dev_password_2024'` fallback default from PostgreSQL connection setup in `db_engine.py` - App now raises `RuntimeError` on startup if `POSTGRES_PASSWORD` env var is not set - Other non-sensitive connection defaults (host, port, db name, user) are left as-is ## Test plan - [ ] Verify API fails with clear error when `POSTGRES_PASSWORD` is unset - [ ] Verify API starts normally when `POSTGRES_PASSWORD` is set - [ ] Confirm dev and prod docker-compose files set `POSTGRES_PASSWORD` 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 1 commit 2026-03-27 04:15:43 +00:00

fix: remove hardcoded fallback password from PostgreSQL connection

Build Docker Image / build (pull_request) Successful in 18m25s

Details

c451e02c52

Raise RuntimeError on startup if POSTGRES_PASSWORD env var is not set,
instead of silently falling back to a known password in source code.

Closes #C2 from postgres migration review.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Claude added the

ai-reviewing

label 2026-03-27 04:15:46 +00:00

cal reviewed 2026-03-27 04:17:38 +00:00

cal left a comment

AI Code Review

Files Reviewed

app/db_engine.py (modified) — the only file changed in this PR

Findings

Correctness

The core change is correct. _postgres_password = os.environ.get("POSTGRES_PASSWORD") followed by an explicit None check and RuntimeError is the right pattern — it causes a fast, loud failure at startup rather than a silent connection failure or an attempt to connect with a wrong credential.
The variable is properly passed into PooledPostgresqlDatabase as password=_postgres_password, eliminating the fallback entirely.
All other connection params (host, port, db name, user) retain their non-sensitive defaults, which is appropriate per the PR description.
The surrounding quote style normalization (single → double quotes) throughout the file introduces no functional changes. No logic was altered by the cosmetic cleanup.

Security

Hardcoded credential removed. 'sba_dev_password_2024' is gone as a fallback default. This is the fix, and it is correctly implemented.
The RuntimeError message does not leak any credential values — it states only the missing variable name, which is safe to log.
No new credentials, tokens, or sensitive values were introduced.

Style & Conventions

The _postgres_password private-convention prefix is a clear signal that this is a module-level intermediate value not for external use, appropriate here.
Double-quote normalization matches the existing style in app/dependencies.py and other active files in the project.
Note on the CLAUDE.md wording: the file states "root-level main.py, db_engine.py are legacy, not in use." This refers specifically to the root-level legacy files, not app/db_engine.py. The changed file (app/db_engine.py) is actively imported by every router under app/routers_v3/ and is very much in production use. The fix targets the correct, live file.

Suggestions (non-blocking)

docker-compose.yml already passes POSTGRES_PASSWORD=${SBA_DB_USER_PASSWORD} to both the Postgres and API containers, so existing deployments are not broken by this change. The PR test plan item to verify dev and prod compose files is already satisfied.
No tests cover the DB connection setup block, and this PR doesn't add any. A test asserting RuntimeError is raised when DATABASE_TYPE=postgresql and POSTGRES_PASSWORD is absent would prevent regression, but its absence is not a blocker.

Verdict: APPROVED (posted as COMMENT — self-review restriction)

The security fix is correct, targeted, and safe. The hardcoded fallback credential is gone, the error path is explicit and informative, and no regressions were introduced by the cosmetic cleanup. Ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/db_engine.py` (modified) — the only file changed in this PR ### Findings #### Correctness - The core change is correct. `_postgres_password = os.environ.get("POSTGRES_PASSWORD")` followed by an explicit `None` check and `RuntimeError` is the right pattern — it causes a fast, loud failure at startup rather than a silent connection failure or an attempt to connect with a wrong credential. - The variable is properly passed into `PooledPostgresqlDatabase` as `password=_postgres_password`, eliminating the fallback entirely. - All other connection params (host, port, db name, user) retain their non-sensitive defaults, which is appropriate per the PR description. - The surrounding quote style normalization (single → double quotes) throughout the file introduces no functional changes. No logic was altered by the cosmetic cleanup. #### Security - **Hardcoded credential removed.** `'sba_dev_password_2024'` is gone as a fallback default. This is the fix, and it is correctly implemented. - The `RuntimeError` message does not leak any credential values — it states only the missing variable name, which is safe to log. - No new credentials, tokens, or sensitive values were introduced. #### Style & Conventions - The `_postgres_password` private-convention prefix is a clear signal that this is a module-level intermediate value not for external use, appropriate here. - Double-quote normalization matches the existing style in `app/dependencies.py` and other active files in the project. - Note on the CLAUDE.md wording: the file states "root-level `main.py`, `db_engine.py` are legacy, not in use." This refers specifically to the **root-level** legacy files, not `app/db_engine.py`. The changed file (`app/db_engine.py`) is actively imported by every router under `app/routers_v3/` and is very much in production use. The fix targets the correct, live file. #### Suggestions (non-blocking) - `docker-compose.yml` already passes `POSTGRES_PASSWORD=${SBA_DB_USER_PASSWORD}` to both the Postgres and API containers, so existing deployments are not broken by this change. The PR test plan item to verify dev and prod compose files is already satisfied. - No tests cover the DB connection setup block, and this PR doesn't add any. A test asserting `RuntimeError` is raised when `DATABASE_TYPE=postgresql` and `POSTGRES_PASSWORD` is absent would prevent regression, but its absence is not a blocker. ### Verdict: APPROVED (posted as COMMENT — self-review restriction) The security fix is correct, targeted, and safe. The hardcoded fallback credential is gone, the error path is explicit and informative, and no regressions were introduced by the cosmetic cleanup. Ready to merge. --- *Automated review by Claude PR Reviewer*

Claude reviewed 2026-03-27 04:18:40 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/db_engine.py (modified)

Findings

Correctness

The _postgres_password check runs at module import time inside the if DATABASE_TYPE.lower() == "postgresql": block, so the RuntimeError fires before any routes become available. Fail-fast behaviour is correct.
The error message is clear and actionable: names the missing variable and the condition that requires it.
Non-sensitive defaults (host, port, db name, user) are intentionally left as-is per the PR description — appropriate since only the password is a credential.

Security

Hardcoded credential 'sba_dev_password_2024' is removed. No credentials remain in the changed code.
This is a stricter fix than the approach in PR #55 (password=None, which deferred failure to connection time). RuntimeError at startup is better UX and avoids a potential silent misconfiguration window.

Style & Conventions

Single→double quote reformatting throughout the hunk is expected ruff/black noise, consistent with all prior PRs in this project.
_postgres_password (leading underscore, module-level) is a reasonable naming convention for a transient intermediate value that doesn't need to be exported.

Edge Cases

POSTGRES_PASSWORD="" (empty string): os.environ.get("POSTGRES_PASSWORD") returns "", which is not None, so the guard passes and an empty password is forwarded to PooledPostgresqlDatabase. This would fail at connection time with a less clear error. However, an empty password is an unlikely misconfiguration, and guarding against it (e.g. if not _postgres_password) is arguably over-engineering for this fix. Not blocking.

Suggestions

No blocking suggestions.

Verdict: APPROVED

Clean, correct implementation. Removes the hardcoded credential and provides a clear startup-time error with a meaningful message. Strictly better than the None-passthrough approach. No issues found.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/db_engine.py` (modified) ### Findings #### Correctness - The `_postgres_password` check runs at module import time inside the `if DATABASE_TYPE.lower() == "postgresql":` block, so the `RuntimeError` fires before any routes become available. Fail-fast behaviour is correct. - The error message is clear and actionable: names the missing variable and the condition that requires it. - Non-sensitive defaults (host, port, db name, user) are intentionally left as-is per the PR description — appropriate since only the password is a credential. #### Security - Hardcoded credential `'sba_dev_password_2024'` is removed. No credentials remain in the changed code. - This is a stricter fix than the approach in PR #55 (`password=None`, which deferred failure to connection time). `RuntimeError` at startup is better UX and avoids a potential silent misconfiguration window. #### Style & Conventions - Single→double quote reformatting throughout the hunk is expected ruff/black noise, consistent with all prior PRs in this project. - `_postgres_password` (leading underscore, module-level) is a reasonable naming convention for a transient intermediate value that doesn't need to be exported. #### Edge Cases - `POSTGRES_PASSWORD=""` (empty string): `os.environ.get("POSTGRES_PASSWORD")` returns `""`, which is not `None`, so the guard passes and an empty password is forwarded to `PooledPostgresqlDatabase`. This would fail at connection time with a less clear error. However, an empty password is an unlikely misconfiguration, and guarding against it (e.g. `if not _postgres_password`) is arguably over-engineering for this fix. **Not blocking.** #### Suggestions - No blocking suggestions. ### Verdict: APPROVED Clean, correct implementation. Removes the hardcoded credential and provides a clear startup-time error with a meaningful message. Strictly better than the `None`-passthrough approach. No issues found. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-27 04:19:00 +00:00

Claude approved these changes 2026-03-27 04:25:24 +00:00

Dismissed

Claude left a comment

Security fix verified — hardcoded fallback password removed, RuntimeError on missing env var, docker-compose already wires the variable. Clean and ready to merge.

Claude referenced this pull request

2026-03-27 05:36:54 +00:00

fix: disable autoconnect and set pool timeout on PooledPostgresqlDatabase (#80) #87

Claude referenced this pull request

2026-03-27 06:34:12 +00:00

fix: remove SQLite fallback code from db_engine.py (#70) #89

Claude referenced this pull request

2026-03-27 06:34:22 +00:00

Remove SQLite fallback code from db_engine.py #70

Claude approved these changes 2026-03-28 07:26:40 +00:00

Claude left a comment

Re-approving after whitelist update.