Closed
Conversation
Contributor
Reviewer's GuideIntroduces a basic Rust-based Hotline server skeleton featuring Tokio-powered async TCP handling, a Diesel/SQLite database layer with embedded migrations, data models, and project scaffolding with dependency configuration and documentation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Store and compare hashed passwords instead of plaintext (link)
General comments:
- Consider using a line‐based codec (e.g. Tokio’s BufReader::lines or a length‐prefixed frame) instead of raw 1024-byte reads to simplify command parsing and avoid partial reads.
- Avoid blocking in the async Tokio runtime for DB calls—either switch to an async‐friendly ORM (diesel_async or sqlx) or centralize spawn_blocking to a dedicated pool.
- Validate the LOGIN command arguments (username/password) before unwrapping to avoid silently accepting empty values; return a clear ERR code if parsing fails.
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| async fn handle_client(mut socket: TcpStream, peer: SocketAddr, pool: DbPool) -> Result<(), Box<dyn Error>> { |
Contributor
There was a problem hiding this comment.
suggestion: Handler processes only one command per connection
Currently, the handler closes the connection after processing one message. To support multiple commands per session, implement a loop to handle messages until the client disconnects.
| let password = parts.next().unwrap_or(""); | ||
| let valid = tokio::task::spawn_blocking(move || { | ||
| let mut conn = pool.get().map_err(|e| Box::new(e) as Box<dyn Error>)?; | ||
| Ok::<_, Box<dyn Error>>(get_user_by_name(&mut conn, username)?.map(|u| u.password == password)) |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Store and compare hashed passwords instead of plaintext
Hash passwords using a secure algorithm and use constant-time comparison to protect user credentials.
| let pool = establish_pool(&database_url); | ||
| { | ||
| let mut conn = pool.get().expect("failed to get db connection"); | ||
| run_migrations(&mut conn); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling migration failures gracefully
Refactor run_migrations to return a Result, and propagate errors with the ? operator in main. This allows for proper error logging and clean shutdowns instead of panics.
Suggested implementation:
{
let mut conn = pool.get().expect("failed to get db connection");
run_migrations(&mut conn)?;
}You must also update the run_migrations function in your db module to return a Result<(), Box<dyn std::error::Error>> (or a more specific error type if appropriate), and ensure it does not panic internally but instead returns errors.
Comment on lines
+39
to
+40
| let mut buf = vec![0u8; 1024]; | ||
| let n = socket.read(&mut buf).await?; |
Contributor
There was a problem hiding this comment.
suggestion (performance): Avoid allocating a Vec for a fixed-size buffer
Using Vec here increases heap allocations. A stack-allocated array or buffer pool would be more efficient.
Suggested change
| let mut buf = vec![0u8; 1024]; | |
| let n = socket.read(&mut buf).await?; | |
| let mut buf = [0u8; 1024]; | |
| let n = socket.read(&mut buf).await?; |
| return Ok(()); | ||
| } | ||
| let msg = String::from_utf8_lossy(&buf[..n]); | ||
| let mut parts = msg.trim().splitn(3, ' '); |
Contributor
There was a problem hiding this comment.
suggestion: Command parsing fails if password contains spaces
Consider using a more robust parsing method, such as JSON or a unique delimiter, to handle passwords containing spaces.
Suggested implementation:
let msg = String::from_utf8_lossy(&buf[..n]);
// Expecting a JSON object like: {"cmd":"LOGIN","username":"the user","password":"the password"}
let login_req: serde_json::Value = match serde_json::from_str(msg.trim()) {
Ok(val) => val,
Err(_) => {
socket.write_all(b"ERR invalid json\n").await?;
return Ok(());
}
};
match login_req.get("cmd").and_then(|c| c.as_str()) {
Some("LOGIN") => {
let username = login_req.get("username").and_then(|u| u.as_str()).unwrap_or("");
let password = login_req.get("password").and_then(|p| p.as_str()).unwrap_or("");
let valid = tokio::task::spawn_blocking(move || {
let mut conn = pool.get().map_err(|e| Box::new(e) as Box<dyn Error>)?;
Ok::<_, Box<dyn Error>>(get_user_by_name(&mut conn, username)?.map(|u| u.password == password))
}).await??;
if valid.unwrap_or(false) {
socket.write_all(b"OK\n").await?;
println!("{} authenticated as {}", peer, username);
} else {- Add
serde_jsonto your dependencies inCargo.tomlif not already present:serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" - Add
use serde_json;at the top of your file if not already present. - Update any client code to send JSON-formatted LOGIN commands, e.g.:
{"cmd":"LOGIN","username":"the user","password":"the password"}
Comment on lines
+21
to
+28
| pub struct Connection(pub r2d2::PooledConnection<ConnectionManager<SqliteConnection>>); | ||
|
|
||
| impl Deref for Connection { | ||
| type Target = SqliteConnection; | ||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion: Remove unused Connection struct
Removing unused code helps keep the codebase clean and easier to maintain.
Suggested change
| pub struct Connection(pub r2d2::PooledConnection<ConnectionManager<SqliteConnection>>); | |
| impl Deref for Connection { | |
| type Target = SqliteConnection; | |
| fn deref(&self) -> &Self::Target { | |
| &self.0 | |
| } | |
| } |
Comment on lines
+50
to
+59
| let valid = tokio::task::spawn_blocking(move || { | ||
| let mut conn = pool.get().map_err(|e| Box::new(e) as Box<dyn Error>)?; | ||
| Ok::<_, Box<dyn Error>>(get_user_by_name(&mut conn, username)?.map(|u| u.password == password)) | ||
| }).await??; | ||
| if valid.unwrap_or(false) { | ||
| socket.write_all(b"OK\n").await?; | ||
| println!("{} authenticated as {}", peer, username); | ||
| } else { | ||
| socket.write_all(b"FAIL\n").await?; | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion: Clarify error handling for spawn_blocking
Handle JoinError and your closure's error separately to better distinguish between task panics and application/database errors, which will aid in debugging and monitoring.
Suggested change
| let valid = tokio::task::spawn_blocking(move || { | |
| let mut conn = pool.get().map_err(|e| Box::new(e) as Box<dyn Error>)?; | |
| Ok::<_, Box<dyn Error>>(get_user_by_name(&mut conn, username)?.map(|u| u.password == password)) | |
| }).await??; | |
| if valid.unwrap_or(false) { | |
| socket.write_all(b"OK\n").await?; | |
| println!("{} authenticated as {}", peer, username); | |
| } else { | |
| socket.write_all(b"FAIL\n").await?; | |
| } | |
| let join_result = tokio::task::spawn_blocking(move || { | |
| let mut conn = pool.get().map_err(|e| Box::new(e) as Box<dyn Error>)?; | |
| Ok::<_, Box<dyn Error>>(get_user_by_name(&mut conn, username)?.map(|u| u.password == password)) | |
| }).await; | |
| let valid = match join_result { | |
| Err(join_err) => { | |
| eprintln!("spawn_blocking task panicked or was cancelled: {}", join_err); | |
| socket.write_all(b"ERR Internal error\n").await?; | |
| return Ok(()); | |
| } | |
| Ok(inner_result) => match inner_result { | |
| Err(app_err) => { | |
| eprintln!("Error during user lookup: {}", app_err); | |
| socket.write_all(b"ERR Internal error\n").await?; | |
| return Ok(()); | |
| } | |
| Ok(valid) => valid, | |
| } | |
| }; | |
| if valid.unwrap_or(false) { | |
| socket.write_all(b"OK\n").await?; | |
| println!("{} authenticated as {}", peer, username); | |
| } else { | |
| socket.write_all(b"FAIL\n").await?; | |
| } |
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.
Summary
Testing
cargo check(fails: failed to download crates due to network restrictions)https://chatgpt.com/codex/tasks/task_e_6840681088c4832284c0ca285c15cea5
Summary by Sourcery
Scaffold a minimal mxd Hotline server in Rust with async TCP handling, a SQLite-based authentication layer, and accompanying project configuration and documentation.
New Features:
Enhancements:
Build:
Documentation:
Chores: