Postgres connection string support + switch to pgx library #1183
Conversation
Adds HBOX_DATABASE_CONN_STRING environment variable to allow direct PostgreSQL connection strings instead of individual config fields.
Replaces the legacy lib/pq driver with the modern pgx driver lib/pq is in maintenance mode with pgx under active development
WalkthroughThis pull request adds support for PostgreSQL connection strings as a primary configuration option, introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Security Recommendations
✅ SSL Certificate Validation (Good Practice Observed)
🔐 File Path Validation
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/en/configure/index.md (1)
33-33: Consider adding a security note about connection string handling.Connection strings often contain sensitive credentials. Consider adding a note in the description warning users to secure connection strings (avoid logging, use secrets management, etc.).
Example addition to the description:
"⚠️ Connection strings may contain sensitive credentials. Store them securely using environment variables or secrets management systems."
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
Taskfile.ymlbackend/app/api/main.gobackend/app/api/setup.gobackend/app/api/setup_test.gobackend/go.modbackend/internal/sys/config/conf_database.godocs/en/configure/index.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tankerkiller125
Repo: sysadminsmedia/homebox PR: 291
File: Taskfile.yml:133-0
Timestamp: 2024-10-27T03:11:38.303Z
Learning: In `Taskfile.yml`, for the `db:migration:postgresql` task, hardcoded credentials for `HBOX_DATABASE_USERNAME` and `HBOX_DATABASE_PASSWORD` are necessary and acceptable.
📚 Learning: 2024-10-27T03:11:38.303Z
Learnt from: tankerkiller125
Repo: sysadminsmedia/homebox PR: 291
File: Taskfile.yml:133-0
Timestamp: 2024-10-27T03:11:38.303Z
Learning: In `Taskfile.yml`, for the `db:migration:postgresql` task, hardcoded credentials for `HBOX_DATABASE_USERNAME` and `HBOX_DATABASE_PASSWORD` are necessary and acceptable.
Applied to files:
Taskfile.ymldocs/en/configure/index.md
🧬 Code graph analysis (3)
backend/app/api/main.go (1)
backend/internal/sys/config/conf_database.go (3)
DriverPostgres(5-5)DriverSqlite3(4-4)Database(14-28)
backend/app/api/setup.go (1)
backend/internal/sys/config/conf_database.go (1)
Database(14-28)
backend/app/api/setup_test.go (2)
backend/internal/sys/config/conf_database.go (3)
Database(14-28)DriverPostgres(5-5)DriverSqlite3(4-4)backend/internal/sys/config/conf.go (1)
Config(19-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Backend Server Tests / Go
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
🔇 Additional comments (14)
Taskfile.yml (2)
82-83: LGTM! Helpful documentation for the new connection string option.These commented examples will help users understand the new configuration approach.
207-208: LGTM! Consistent documentation.docs/en/configure/index.md (1)
80-83: LGTM! CLI flag documentation is clear.backend/internal/sys/config/conf_database.go (1)
16-16: LGTM! Field addition is straightforward.The new
ConnStringfield follows the existing naming and tagging conventions.backend/app/api/setup.go (1)
53-64: Connection string priority logic is clear and well-implemented.The priority system (ConnString over individual fields) is explicit and easy to understand. The conditional inclusion of username and password is appropriate.
backend/app/api/main.go (2)
5-5: LGTM! Import additions are appropriate.The new imports support the pgx driver integration correctly.
Also applies to: 12-13, 34-34
63-79: LGTM! TheopenEntDriverhelper is well-designed.The function correctly abstracts driver-specific initialization. Using
sql.Open("pgx", ...)withentsql.OpenDB(dialect.Postgres, db)is the proper way to integrate pgx with Ent.backend/app/api/setup_test.go (6)
9-123: Excellent test coverage for connection string priority and formats.The test cases comprehensively validate the ConnString priority system and support for various PostgreSQL connection string formats (URL, key/value, cloud providers).
125-171: LGTM! SQLite test coverage is appropriate.
173-184: LGTM! Error handling test is appropriate.
186-254: Test documents SSL validation bypass for connection strings.The test at lines 229-235 explicitly documents that connection strings bypass SSL file validation. This aligns with the implementation but creates the inconsistency flagged in
backend/app/api/setup.go. The test confirms this is intentional behavior.
256-279: LGTM! Backward compatibility test is important.This ensures existing configurations continue to work correctly.
281-305: LGTM! Priority order test is clear and explicit.backend/go.mod (1)
24-24: v5.7.5 is secure and current; consider upgrading to v5.7.6.The switch from lib/pq to pgx/v5 is appropriate and secure. v5.7.5 is not affected by any known 2024 CVEs, including CVE-2024-27304 (protocol message overflow SQL injection) and CVE-2024-27289 (simple protocol SQL injection)—both fixed in earlier 5.x releases. A patch update to v5.7.6 is available as of December 28, 2025, but v5.7.5 meets current security standards.
| c, err := openEntDriver(strings.ToLower(cfg.Database.Driver), databaseURL) | ||
| if err != nil { | ||
| log.Error(). | ||
| Err(err). | ||
| Str("driver", strings.ToLower(cfg.Database.Driver)). | ||
| Str("host", cfg.Database.Host). | ||
| Str("port", cfg.Database.Port). | ||
| Str("database", cfg.Database.Database). | ||
| Msg("failed opening connection to {driver} database at {host}:{port}/{database}") | ||
| return fmt.Errorf("failed opening connection to %s database at %s:%s/%s: %w", | ||
| Str("database_url", databaseURL). | ||
| Msg("failed opening connection to {driver} database") | ||
| return fmt.Errorf("failed opening connection to %s database: %w", | ||
| strings.ToLower(cfg.Database.Driver), |
There was a problem hiding this comment.
Critical: Database credentials exposed in logs.
Line 149 logs the full database_url, which may contain sensitive credentials like usernames and passwords (e.g., postgres://user:password@localhost:5432/db). This exposes credentials in application logs, creating a security vulnerability.
Security Recommendation: Sanitize the database URL before logging by removing credentials, or simply log the driver name without the URL.
🔎 Proposed fix to sanitize credentials from logs
c, err := openEntDriver(strings.ToLower(cfg.Database.Driver), databaseURL)
if err != nil {
log.Error().
Err(err).
Str("driver", strings.ToLower(cfg.Database.Driver)).
- Str("database_url", databaseURL).
Msg("failed opening connection to {driver} database")
return fmt.Errorf("failed opening connection to %s database: %w",
strings.ToLower(cfg.Database.Driver),
err,
)
}Alternatively, if you need to log connection details for debugging, sanitize the URL first:
// Add a helper function
func sanitizeDatabaseURL(url string) string {
// Remove password from URLs like postgres://user:pass@host:port/db
re := regexp.MustCompile(`://([^:]+):([^@]+)@`)
return re.ReplaceAllString(url, "://$1:***@")
}
// Then use it in logging
log.Error().
Err(err).
Str("driver", strings.ToLower(cfg.Database.Driver)).
Str("database_url", sanitizeDatabaseURL(databaseURL)).
Msg("failed opening connection to {driver} database")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c, err := openEntDriver(strings.ToLower(cfg.Database.Driver), databaseURL) | |
| if err != nil { | |
| log.Error(). | |
| Err(err). | |
| Str("driver", strings.ToLower(cfg.Database.Driver)). | |
| Str("host", cfg.Database.Host). | |
| Str("port", cfg.Database.Port). | |
| Str("database", cfg.Database.Database). | |
| Msg("failed opening connection to {driver} database at {host}:{port}/{database}") | |
| return fmt.Errorf("failed opening connection to %s database at %s:%s/%s: %w", | |
| Str("database_url", databaseURL). | |
| Msg("failed opening connection to {driver} database") | |
| return fmt.Errorf("failed opening connection to %s database: %w", | |
| strings.ToLower(cfg.Database.Driver), | |
| c, err := openEntDriver(strings.ToLower(cfg.Database.Driver), databaseURL) | |
| if err != nil { | |
| log.Error(). | |
| Err(err). | |
| Str("driver", strings.ToLower(cfg.Database.Driver)). | |
| Msg("failed opening connection to {driver} database") | |
| return fmt.Errorf("failed opening connection to %s database: %w", | |
| strings.ToLower(cfg.Database.Driver), | |
| err, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In backend/app/api/main.go around lines 144 to 152, the error logging currently
includes the full database_url which may contain credentials; remove sensitive
data from logs by either omitting database_url from the log fields or replace it
with a sanitized value (e.g., mask the password) before logging; implement a
small helper that strips or masks credentials from the URL and use that helper
in the log call (or simply log only the driver name), ensuring the error is
still returned as before.
There was a problem hiding this comment.
You're absolutely right
It's a bit of an annoying problem, let me sleep on it
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch information about the pgx GitHub issues mentioned by the user
gh issue view 1271 --repo jackc/pgx --json title,body,state,comments | jq -r '.title, .body' | head -50
echo "---"
gh issue view 1285 --repo jackc/pgx --json title,body,state,comments | jq -r '.title, .body' | head -50
Length of output: 1946
@harrisony, totally understand! Looking at those issues, I can see pgx does redact passwords in parse errors (showing xxxxxx), but the connection string can still leak in other error scenarios. It's a tricky balance between debuggability and security.
A couple of quick thoughts while you're considering options:
-
Simple interim fix: Just remove the
database_urlfield from the log entirely (as in my first suggestion). The error message itself often contains useful debugging info, and you can always add it back temporarily if you need to debug connection issues in development. -
Connection pooling errors: Since you're now using
sql.Open()with pgx, you might also want to check if there are any other places where database errors bubble up that could expose the URL. -
Config validation: The URL is constructed in
setupDatabaseURL(), so any connection string parsing errors would happen at startup—which makes logging it less critical since you can test locally.
Take your time figuring out the right approach for your use case! The important thing is you're aware of it now. 🐰
katosdev
requested review from
tankerkiller125
and removed request for
katosdev
1 participant
What type of PR is this?
What this PR does / why we need it:
I have to use a quirky postgres connection, rather than adding more
HBOX_DATABASE_options, It's much cleaner to straight up give it a well formed query string.Additionally, the lib/pg library is in maintenance mode and has been for a while, I'm not sure why it was chosen initially but this moves to the recommended pgx replacement
Which issue(s) this PR fixes:
There is no issue
Special notes for your reviewer:
(fill-in or delete this section)
I asked Claude Code to generate the tests, it looks ok but I'm not a go person.
Testing
I'm running this on my production instance for a day or so and not seeing any issues but I'd appreciate giving it a few days to make sure
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.