Laboration 3#1
Conversation
📝 WalkthroughWalkthroughExpanded the main application to add a post-login looping menu that lists and queries moon missions (list, get by ID, count by year) and provides account CRUD operations (create, update password, delete) with input validation and result messaging. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainApp
participant Database
User->>MainApp: Start / enter username & password
MainApp->>Database: SELECT ... FROM account WHERE username=? AND password=?
Database-->>MainApp: auth row / none
alt Auth success
MainApp-->>User: "Login successful!"
loop Menu loop
User->>MainApp: Choose menu option
alt List missions
MainApp->>Database: SELECT spacecraft FROM moon_missions ORDER BY mission_id
Database-->>MainApp: result set
MainApp-->>User: Print spacecraft list
else Get mission by ID
MainApp->>User: Prompt mission_id
User-->>MainApp: mission_id
MainApp->>Database: SELECT * FROM moon_missions WHERE mission_id=?
Database-->>MainApp: row / none
MainApp-->>User: Print result / not found
else Count missions by year
MainApp->>User: Prompt year
User-->>MainApp: year
MainApp->>Database: SELECT COUNT(*) FROM moon_missions WHERE YEAR(launch_date)=?
Database-->>MainApp: count
MainApp-->>User: Print count
else Create account
MainApp->>User: Prompt account details
User-->>MainApp: details
MainApp->>Database: INSERT INTO account (...)
Database-->>MainApp: success / error
MainApp-->>User: Confirmation / error
else Update password
MainApp->>User: Prompt user_id & new password
User-->>MainApp: input
MainApp->>Database: UPDATE account SET password=? WHERE user_id=?
Database-->>MainApp: affected rows
MainApp-->>User: Success / not found
else Delete account
MainApp->>User: Prompt user_id
User-->>MainApp: user_id
MainApp->>Database: DELETE FROM account WHERE user_id=?
Database-->>MainApp: affected rows
MainApp-->>User: Success / not found
end
end
else Auth failure
MainApp-->>User: Print error & exit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/Main.java (1)
54-82: Critical security issue: plaintext password storage and resource leak.Multiple critical issues identified:
Plaintext passwords: The authentication logic stores and compares passwords in plaintext (line 66). This is a severe security vulnerability. Passwords must be hashed using a strong algorithm (e.g., bcrypt, Argon2).
Scanner resource leak: The
Scannerinstance created at line 56 is never closed, causing a resource leak. Wrap it in try-with-resources or explicitly close it.Potential schema mismatch: The authentication query uses column
name(line 66), but the account creation operation (case "4") usesfirst_nameandlast_name. Verify that anamecolumn exists in theaccounttable.Apply this diff to wrap Scanner in try-with-resources (addresses issue ithsjava25#2):
- try (Connection connection = DriverManager.getConnection(jdbcUrl, dbUser, dbPass)) { - - Scanner scanner = new Scanner(System.in); + try (Connection connection = DriverManager.getConnection(jdbcUrl, dbUser, dbPass); + Scanner scanner = new Scanner(System.in)) {For issue #1, implement password hashing using bcrypt or a similar algorithm before storing and comparing passwords.
For issue ithsjava25#3, run the following verification:
#!/bin/bash # Verify the account table schema ast-grep --pattern 'CREATE TABLE account'
🧹 Nitpick comments (3)
src/main/java/com/example/Main.java (3)
97-106: Consider explicit ResultSet management and empty result feedback.While the current implementation works, consider these improvements:
ResultSet closure: Although
ResultSetis auto-closed when thePreparedStatementcloses, explicitly managing it in try-with-resources makes intent clearer and is safer.Empty result feedback: The code doesn't indicate when no missions are found. Consider tracking whether any results were returned and printing a message if the list is empty.
Example refactor:
case "1" -> { try (PreparedStatement ps = connection.prepareStatement( "SELECT spacecraft FROM moon_mission ORDER BY mission_id"); ResultSet rs = ps.executeQuery()) { boolean found = false; while (rs.next()) { System.out.println(rs.getString("spacecraft")); found = true; } if (!found) { System.out.println("No missions found"); } } }
207-209: Consider more graceful error handling.The current exception handling re-throws
SQLExceptionasRuntimeException, which crashes the application. Consider:
- Logging: Add proper logging to capture error details for debugging
- User-friendly messages: Display meaningful error messages to users instead of exposing stack traces
- Graceful degradation: Allow the application to continue running or exit cleanly after displaying an error
Example improvement:
} catch (SQLException e) { System.err.println("Database error: " + e.getMessage()); // Log the full stack trace for debugging e.printStackTrace(); // Exit gracefully or continue, depending on requirements }
42-210: Refactor for better maintainability and separation of concerns.The
run()method handles too many responsibilities (configuration, authentication, UI, and database operations), making it difficult to test and maintain. Consider these improvements:
Extract methods: Move each menu option to its own method (e.g.,
listMoonMissions(),getMissionById(),createAccount(), etc.)Separate concerns: Create separate classes for:
- Database operations (DAO pattern)
- User interface/menu handling
- Authentication logic
Reduce duplication: The PreparedStatement pattern is repeated throughout. Extract common patterns into reusable methods.
Improve testability: With separated concerns, each component can be unit-tested independently.
Example structure:
public class Main { private final MoonMissionDao missionDao; private final AccountDao accountDao; private final MenuHandler menuHandler; public void run() { Connection conn = establishConnection(); if (authenticate(conn)) { menuHandler.displayMenu(conn); } } private void listMoonMissions(Connection conn) { /* ... */ } private void getMissionById(Connection conn, Scanner scanner) { /* ... */ } // ... other methods }
| case "4" -> { | ||
| System.out.println("First name:"); | ||
| String firstName = scanner.nextLine().trim(); | ||
| System.out.println("Last name:"); | ||
| String lastName = scanner.nextLine().trim(); | ||
| System.out.println("SSN:"); | ||
| String ssn = scanner.nextLine().trim(); | ||
| System.out.println("Password:"); | ||
| String accountPassword = scanner.nextLine().trim(); | ||
|
|
||
| try (PreparedStatement ps = connection.prepareStatement( | ||
| "INSERT INTO account(password, first_name, last_name, ssn) VALUES (?, ?, ?, ?)" | ||
| )) { | ||
| ps.setString(1, accountPassword); | ||
| ps.setString(2, firstName); | ||
| ps.setString(3, lastName); | ||
| ps.setString(4, ssn); | ||
| ps.executeUpdate(); | ||
| } | ||
|
|
||
| System.out.println("Account created"); | ||
| } |
There was a problem hiding this comment.
Critical security issues: plaintext passwords and PII handling.
Multiple security vulnerabilities identified:
-
Plaintext password storage: Line 156 stores the password in plaintext, which is a critical security vulnerability. Passwords must be hashed using a strong algorithm (e.g., bcrypt, Argon2) before storage.
-
Unprotected PII: SSN (Social Security Number) at line 159 is highly sensitive personally identifiable information (PII). It should be encrypted at rest and handled with extreme care. Consider:
- Encrypting SSN before storage
- Implementing access controls
- Auditing access to SSN data
- Complying with privacy regulations (GDPR, CCPA, etc.)
-
No input validation: The code doesn't validate SSN format, password strength, or check for empty strings. This could lead to invalid data in the database.
-
No duplicate checking: There's no check for duplicate SSNs or other unique constraints before insertion, which could cause unclear error messages.
Implement password hashing and SSN encryption before storing these values. Add input validation and duplicate checking to improve data integrity.
| case "5" -> { | ||
| System.out.println("User ID of account to update:"); | ||
| long userId = Long.parseLong(scanner.nextLine().trim()); | ||
| System.out.println("New password:"); | ||
| String newPassword = scanner.nextLine().trim(); | ||
|
|
||
| try (PreparedStatement ps = connection.prepareStatement( | ||
| "UPDATE account SET password = ? WHERE user_id = ?" | ||
| )) { | ||
| ps.setString(1, newPassword); | ||
| ps.setLong(2, userId); | ||
| int rows = ps.executeUpdate(); | ||
| if (rows > 0) { | ||
| System.out.println("Password updated"); | ||
| } else { | ||
| System.out.println("No account found with that ID"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical security issues: missing authorization, plaintext passwords, and account enumeration.
Multiple critical and major security issues:
-
Missing authorization: Any authenticated user can update ANY account's password. There's no check to verify that the logged-in user has permission to update the specified account. This is a critical authorization vulnerability.
-
Plaintext password storage: Line 175 stores the new password in plaintext. Passwords must be hashed before storage.
-
Account enumeration: Line 181 ("No account found with that ID") reveals whether a user ID exists in the system, which is an information disclosure vulnerability that attackers can exploit.
-
Missing input validation:
Long.parseLongat line 168 will throw an uncaughtNumberFormatExceptionfor invalid input.
Required fixes:
- Add authorization check to verify the user can only update their own password (or implement proper role-based access control)
- Implement password hashing
- Use generic error messages that don't reveal account existence
- Add input validation with try-catch for parsing operations
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around lines 166 to 184, the
password-update flow lacks input validation, authorization checks, hashes
passwords in plaintext, and leaks account existence; update it to (1) validate
and catch NumberFormatException around Long.parseLong and prompt/abort on bad
input, (2) verify the currently authenticated user's ID (or RBAC) matches the
target userId before proceeding and reject unauthorized attempts, (3) hash the
new password (e.g., BCrypt) and store the hash instead of plaintext, (4) after
executeUpdate return a generic success/failure message that does not reveal
whether the userId exists (e.g., "Password update completed" on success, and
"Unable to update password" on failure), and (5) ensure SQL and password
operations are wrapped in try/catch to log internal errors server-side without
exposing details to the user.
| case "6" -> { | ||
| System.out.println("User ID of account to delete:"); | ||
| long userId = Long.parseLong(scanner.nextLine().trim()); | ||
|
|
||
| try (PreparedStatement ps = connection.prepareStatement( | ||
| "DELETE FROM account WHERE user_id = ?" | ||
| )) { | ||
| ps.setLong(1, userId); | ||
| int rows = ps.executeUpdate(); | ||
| if (rows > 0) { | ||
| System.out.println("Account deleted"); | ||
| } else { | ||
| System.out.println("No account found with that ID"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical security issue: missing authorization and safety checks.
Multiple critical security issues:
-
Missing authorization: Any authenticated user can delete ANY account without permission checks. This is a critical authorization vulnerability that could lead to unauthorized data deletion.
-
Account enumeration: Line 198 ("No account found with that ID") reveals whether a user ID exists, enabling attackers to enumerate valid accounts.
-
Missing input validation:
Long.parseLongat line 188 will throw an uncaughtNumberFormatExceptionfor invalid input, crashing the application. -
No confirmation prompt: Deletion is permanent and irreversible, yet there's no confirmation step to prevent accidental deletions.
-
No transaction management: Account deletion might need to cascade to related records or maintain referential integrity, but there's no transaction wrapping.
Required fixes:
- Implement authorization to restrict users to deleting only their own accounts (or enforce role-based permissions)
- Add confirmation prompt before deletion
- Add input validation for user ID parsing
- Use generic error messages
- Consider transaction management if cascading deletes are needed
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around lines 186 to 201, the
delete-account branch lacks auth, validation, confirmation, safe error handling
and transaction management; fix by (1) validating and parsing input safely
(catch NumberFormatException and re-prompt or abort), (2) enforcing
authorization so only the logged-in user's own account can be deleted or require
an elevated role check before proceeding, (3) prompt for an explicit
confirmation (y/N) before performing deletion, (4) wrap the delete in a DB
transaction (begin, execute, commit/rollback) to ensure referential integrity or
call a cascade-safe service/DAO instead of raw deletion here, (5) avoid
revealing whether the ID existed by returning a generic success/failure message
and log detailed errors internally, and (6) catch SQLExceptions and other
exceptions to rollback and handle errors gracefully instead of letting the app
crash.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/Main.java (1)
3-6: Critical compilation error: Missing required imports.The code uses
Scanner,PreparedStatement, andResultSetbut these classes are not imported, causing compilation failures throughout the file.Add the missing imports at the top of the file:
import java.sql.Connection; import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; import java.sql.SQLException; import java.util.Arrays; +import java.util.Scanner;
♻️ Duplicate comments (3)
src/main/java/com/example/Main.java (3)
153-174: Critical security issues remain unaddressed.The critical security vulnerabilities flagged in the previous review are still present:
- Plaintext password storage (line 166): Passwords are stored without hashing
- Unencrypted PII (line 169): SSN is stored in plaintext without encryption
- No input validation: Empty strings, invalid SSN formats accepted
- No duplicate checking: Can insert duplicate SSNs causing unclear errors
These are the same critical issues identified in the previous review comments.
176-194: Critical security issues and missing input validation remain unaddressed.The critical issues flagged in the previous review are still present:
- Missing input validation (line 178):
Long.parseLongwill crash on invalid input- Missing authorization: Any authenticated user can update any account's password
- Plaintext password storage (line 185): New password stored without hashing
- Account enumeration (line 191): Error message reveals account existence
These are the same issues identified in the previous review comments.
196-211: Critical security issues and safety checks remain unaddressed.The critical issues flagged in the previous review are still present:
- Missing input validation (line 198):
Long.parseLongwill crash on invalid input- Missing authorization: Any authenticated user can delete any account
- Account enumeration (line 208): Error message reveals account existence
- No confirmation prompt: Permanent deletion without user confirmation
- No transaction management: No safeguards for referential integrity
These are the same issues identified in the previous review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/example/Main.java(2 hunks)
🔇 Additional comments (4)
src/main/java/com/example/Main.java (4)
97-99: Exit logic correctly implemented.The exit option now properly uses
returnto exit the method and terminate the program.
100-109: LGTM: Clean implementation.The query correctly retrieves and displays spacecraft names using proper resource management.
110-131: LGTM: Previous issues resolved.The input validation and query optimization from the previous review have been correctly implemented. The code now safely handles invalid input and selects only the required column.
133-151: LGTM: Previous issues resolved.The input validation has been added and the unreachable else branch has been removed. The code correctly handles COUNT(*) queries which always return a row.
| try (PreparedStatement ps = connection.prepareStatement( | ||
| "SELECT COUNT(*) FROM account WHERE name = ? AND password = ?" | ||
| )) { | ||
| ps.setString(1, username); | ||
| ps.setString(2, password); | ||
|
|
||
| ResultSet rs = ps.executeQuery(); | ||
| if (rs.next() && rs.getInt(1) > 0) { | ||
| authenticated = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical security vulnerability: Plaintext password authentication.
The authentication query at line 67 compares passwords directly against the database, which implies passwords are stored in plaintext. This is a fundamental security violation that exposes all user credentials.
Passwords must be hashed using a strong, slow hashing algorithm (bcrypt, Argon2, PBKDF2) before storage. Authentication should:
- Retrieve the password hash for the username
- Hash the provided password with the same algorithm
- Compare the hashes using a constant-time comparison
This affects the entire authentication flow and is consistent with the plaintext password issues in cases 4 and 5.
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around lines 66 to 76, the code performs
plaintext password comparison against the database; replace this with proper
hashed-password verification: change the query to SELECT password_hash FROM
account WHERE name = ?; fetch the stored hash, then verify the supplied password
using a strong password-hashing library (e.g., bcrypt or Argon2) by calling the
library's verify method (which applies the same salt/params embedded in the
hash) and use its constant-time comparison; ensure new user registration and any
password changes store only the salted hash (not plaintext) using the same
algorithm, and add a migration path for existing plaintext passwords (force
reset or re-hash on next login) while keeping prepared statements and error
handling intact.
f017c6b to
e694ea8
Compare
1 participant
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.