Repository navigation
fix(sqlite): reject SQLite URLs with user, password, or port - #998
HarshRajSinghania wants to merge 3 commits into
Conversation
SQLite has no network endpoint or credentials. Connection strings that include them were previously accepted and silently misinterpreted. SqliteConnector::new now returns DatabaseConfigError naming the rejected parts and listing the supported forms. Unit tests cover each rejection and the supported forms. Fixes AndreaBozzo#997
SQLite has no network endpoint or credentials. Connection strings that include them were previously accepted and silently misinterpreted. SqliteConnector::new now returns DatabaseConfigError naming the rejected parts and listing the supported forms. Unit tests cover each rejection and the supported forms. Fixes AndreaBozzo#997
SQLite has no network endpoint or credentials. Connection strings that include them were previously accepted and silently misinterpreted. SqliteConnector::new now returns DatabaseConfigError naming the rejected parts and listing the supported forms. Unit tests cover each rejection and the supported forms. Fixes AndreaBozzo#997
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description clearly explains the behavior change, motivation, implementation, supported forms, and verification results. It does not use the template headings or explicitly state whether breaking … Title check The title is concise, specific, and accurately describes the primary change: rejecting SQLite URLs with a user, password, or port. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description clearly explains the behavior change, motivation, implementation, supported forms, and verification results. It does not use the template headings or explicitly state whether breaking changes or feature flags apply, but the required information is mostly present.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
github-actions
Bot
added
area: rust
AndreaBozzo
left a comment
AndreaBozzo
left a comment
There was a problem hiding this comment.
Thanks for picking this up. The check is what #997 asks for: the message names the parts without echoing the password, lists the supported forms, and a host alone still passes.
Before it can merge:
- Code Quality fails on
cargo fmt. Runningcargo fmt --allfixes it (inline). - Please revert the edits unrelated to the fix in
get_table_schema,count_table_rowsandtest_connection: the inlinedformat!arguments and the removed blank lines. One behaviour change per PR keeps the diff to the fix. - The
Debugderive is only there for the tests (inline).
A note, not a request: since #990, sqlite://data@2026.db opens a file literally named data@2026.db, and this check now rejects it as a user. That is what #997 specifies and the bare path data@2026.db still works, so no change needed.
| use {sqlx::sqlite::SqlitePool, sqlx::sqlite::SqlitePoolOptions}; | ||
|
|
||
| /// SQLite embedded database connector | ||
| #[derive(Debug)] |
There was a problem hiding this comment.
This adds a public Debug impl only so the tests can call unwrap_err(). The Postgres and MySQL connectors don't derive it either. The tests can do without:
let Err(err) = SqliteConnector::new(config(url)) else {
panic!("{url} should be rejected");
};| let pool = self.pool.as_ref().ok_or_else(not_connected_error)?; | ||
|
|
||
| let query = format!("PRAGMA table_info({})", table_name); | ||
| let query = format!("PRAGMA table_info({table_name})"); |
There was a problem hiding this comment.
Unrelated to the fix, please revert this, the COUNT(*) line below and the removed blank lines.
| "/absolute/path.db", | ||
| "relative.db", | ||
| ] { | ||
| SqliteConnector::new(config(url)).unwrap_or_else(|e| panic!("should accept {url}: {e}")); |
There was a problem hiding this comment.
rustfmt splits this over two lines, which is what fails Code Quality.
|
Reopen this eventually |
Summary
SqliteConnector::new now rejects SQLite connection strings that contain a user, password, or port. These were previously accepted and silently misinterpreted (credentials dropped or treated as part of the filename).
Motivation
Addresses #997. SQLite has no network endpoint or credentials, so such URLs are always mistakes (e.g. a PostgreSQL URL with the scheme changed).
Implementation
After parsing ConnectionInfo, if username, password, or port is present the constructor returns DatabaseConfigError. The message names the rejected parts (never the password value) and lists the supported forms:
A host by itself remains allowed (as in sqlite://relative/path.db).
Testing
cargo test -p dataprof-db --features sqlite --lib sqlite::testsAll five new unit tests passed:
Also ran
cargo fmton the changed file.No other files changed.
Summary by CodeRabbit