Auth#2
Merged
Merged
Conversation
added 8 commits
May 21, 2026 13:16
Review Summary by QodoImplement authentication with bcrypt hashing and comprehensive tracing
WalkthroughsDescription• Replaced AES-GCM encryption with bcrypt for password hashing • Added comprehensive tracing/logging with file and console output • Implemented login endpoint with password verification • Improved error handling using context and proper status codes • Added readiness check endpoint for database connectivity • Refactored configuration management and environment variable handling Diagramflowchart LR
A["User Request"] --> B["Login/Signup Endpoint"]
B --> C["Bcrypt Password Hashing"]
C --> D["Database Operations"]
D --> E["JWT Token Generation"]
E --> F["Tracing Logger"]
F --> G["Console & File Output"]
B --> H["Error Handling with Context"]
H --> G
File Changes1. src/config.rs
|
Code Review by Qodo
1. sqlx query! breaks CI
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+27
to
+33
| let row = sqlx::query!( | ||
| "SELECT id, password_hash FROM users WHERE username = ? AND deleted_at IS NULL", | ||
| payload.username | ||
| ) | ||
| .fetch_optional(&pool) | ||
| .await | ||
| .map_err(|e| { |
There was a problem hiding this comment.
1. Sqlx query! breaks ci 🐞 Bug ☼ Reliability
src/routes/login.rs uses sqlx::query!, which requires a DATABASE_URL at compile-time (or pre-generated offline metadata), but the GitHub workflow runs cargo check without providing DATABASE_URL, so CI will fail to build.
Agent Prompt
## Issue description
`sqlx::query!` performs compile-time query validation and needs a build-time DATABASE_URL or offline metadata. Current CI (`cargo check`) does not set DATABASE_URL, so builds will fail.
## Issue Context
You currently use runtime-checked `sqlx::query(...)` elsewhere; only `login.rs` introduced the macro form.
## Fix options
1) Replace `sqlx::query!` with `sqlx::query` / `sqlx::query_as` and decode the row manually.
2) Or enable SQLx offline workflow: generate `.sqlx` metadata and set `SQLX_OFFLINE=true` in CI.
3) Or set `DATABASE_URL` in CI to a valid SQLite URL (and ensure schema exists for validation).
## Fix Focus Areas
- src/routes/login.rs[27-33]
- .github/workflows/rust.yml[9-22]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+53
| let row = sqlx::query!( | ||
| "SELECT id, password_hash FROM users WHERE username = ? AND deleted_at IS NULL", | ||
| payload.username | ||
| ) | ||
| .fetch_optional(&pool) | ||
| .await | ||
| .map_err(|e| { | ||
| error!( | ||
| error = ?e, | ||
| "DB failed to initialize" | ||
| ); | ||
| (StatusCode::INTERNAL_SERVER_ERROR, "DB error".to_string()) | ||
| })?; | ||
|
|
||
| let (id, stored_hash) = match row { | ||
| Some(r) => (r.id, r.password_hash), | ||
| None => { | ||
| warn!( | ||
| username = payload.username, | ||
| "Failed to find record for login attempt of user" | ||
| ); | ||
| return Err((StatusCode::UNAUTHORIZED, "Invalid credentials".to_string())); | ||
| } | ||
| }; | ||
|
|
||
| let user_id = id.unwrap_or_default(); | ||
| debug!(user_id = user_id, "Record found for login of User"); |
There was a problem hiding this comment.
2. Login ownership/type errors 🐞 Bug ≡ Correctness
login() consumes payload.username in the SQL call and uses it again in the warn! log, and it also calls unwrap_or_default() on id even though users.id is defined as a non-null primary key; this will not compile as written.
Agent Prompt
## Issue description
The login handler likely fails to compile due to (1) moving `payload.username` into the query arguments and then reusing it in logging, and (2) calling `unwrap_or_default()` on `id` even though schema defines `users.id` as non-null.
## Issue Context
SQLite schema creates `users(id TEXT PRIMARY KEY, ...)` meaning `id` is NOT NULL. Therefore SQLx should materialize it as `String`, not `Option<String>`.
## Fix
- Pass `&payload.username` into the query and log `&payload.username` (or clone once into a local variable).
- Treat `id` as a `String` and remove `unwrap_or_default()`.
## Fix Focus Areas
- src/routes/login.rs[27-53]
- migrations/20260521102510_initial_schema.sql[3-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+31
| -- 3. Drop the old table | ||
| DROP TABLE users; | ||
|
|
||
| -- 4. Rename the new table | ||
| ALTER TABLE users_new RENAME TO users; |
There was a problem hiding this comment.
3. Dropping users breaks fks 🐞 Bug ☼ Reliability
The migration recreates users by doing DROP TABLE users while other tables already reference users(id) and the app enables foreign key enforcement; this will cause the migration to fail with foreign key constraint errors.
Agent Prompt
## Issue description
SQLite migrations drop the `users` table even though `chat_participants.user_id` and `messages.sender_id` reference it. With `foreign_keys(true)` enabled in connection options, `DROP TABLE users` will fail.
## Issue Context
`src/db/mod.rs` enables FK enforcement for the pool. The initial schema creates FK references to `users` before the later migration runs.
## Fix
Choose one:
- Wrap the table-rebuild with `PRAGMA foreign_keys=OFF; ... PRAGMA foreign_keys=ON;` (ideally in a transaction) and ensure integrity checks after.
- Or migrate without dropping the referenced table (e.g., add a new column / keep table and backfill, or rebuild referencing tables too).
## Fix Focus Areas
- migrations/20260521141725_changing_password_hash_to_blob.sql[27-31]
- migrations/20260521102510_initial_schema.sql[20-37]
- src/db/mod.rs[4-8]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+30
to
+33
| id TEXT PRIMARY KEY, | ||
| chat_id TEXT NOT NULL REFERENCES chats(id) ON DELETE CASCADE, | ||
| sender_id TEXT REFERENCES users(id) ON DELETE SET NULL, | ||
| encrypted_content BLOB NOT NULL, -- AES-GCM ciphertext (without tag) | ||
| nonce BLOB NOT NULL, -- 12 bytes unique per message | ||
| tag BLOB NOT NULL, -- 16 bytes authentication tag | ||
| created_at TEXT NOT NULL DEFAULT (datetime('now')) | ||
| sender_id TEXT NOT NULL REFERENCES users(id) ON DELETE SET NULL, | ||
| encrypted_content BLOB NOT NULL, |
There was a problem hiding this comment.
4. Set null with not null 🐞 Bug ≡ Correctness
messages.sender_id is declared NOT NULL but also uses ON DELETE SET NULL, so deleting a user who has messages will violate the NOT NULL constraint and fail.
Agent Prompt
## Issue description
Schema defines `sender_id TEXT NOT NULL ... ON DELETE SET NULL`, which is a contradiction: FK action tries to set NULL but the column forbids NULL.
## Fix
Pick intended behavior:
- If you want to keep messages: remove `NOT NULL` on `sender_id`.
- If you want to delete messages with users: use `ON DELETE CASCADE`.
- If you want to block deletion: use `ON DELETE RESTRICT`.
## Fix Focus Areas
- migrations/20260521102510_initial_schema.sql[29-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+13
to
+21
| dotenv().expect("Failed to load .env variables"); | ||
| Self { | ||
| database_url: env::var("DATABASE_URL").expect("DATABASE_URL must be set"), | ||
| jwt_secret: env::var("JWT_SECRET").expect("JWT_SECRET must be set"), | ||
| ip_address: env::var("IP_ADDRESS").expect("Ip address not set"), | ||
| port: env::var("PORT") | ||
| .unwrap_or_else(|_| "8080".to_string()) | ||
| .unwrap_or("8080".to_string()) | ||
| .parse() | ||
| .expect("PORT must be a number"), | ||
| ip_address: env::var("IP_ADDRESS").expect("Ip address not set") | ||
| .expect("PORT not set"), |
There was a problem hiding this comment.
5. Dotenv hard-requirement panics 🐞 Bug ☼ Reliability
Config::from_env() now calls dotenv().expect(...), which will crash the server when .env is absent (common in production/container deployments) and is also invoked during JWT creation.
Agent Prompt
## Issue description
Requiring `.env` via `dotenv().expect(...)` makes startup and request paths panic when `.env` is missing.
## Issue Context
`main` calls `Config::from_env()` at startup, and `create_jwt()` calls it again during request handling.
## Fix
- Change back to `dotenv().ok();` (or only load dotenv in debug builds).
- Avoid re-loading env per request: pass `jwt_secret`/`Config` via application state and have `create_jwt` take the secret as a parameter.
## Fix Focus Areas
- src/config.rs[12-22]
- src/main.rs[11-18]
- src/routes/helper.rs[13-28]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.