Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements user signup functionality for the authentication service. It replaces the stub implementation with a complete user registration endpoint that accepts username, password, and display name, hashes the password with Argon2, and stores the user in the database.
Changes:
- Added request validation and user creation logic in the POST handler
- Implemented database query to insert new users into the users table
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/handlers/userhandler/post.go | Implements user signup handler with input validation, password hashing, and user creation |
| internal/database/user/queries.go | Adds CreateUser function to persist new users to the database |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| salt := make([]byte, 16) | ||
| rand.Read(salt) |
There was a problem hiding this comment.
The error return value from rand.Read must be checked. If rand.Read fails to generate cryptographically secure random bytes, the salt could be all zeros or incomplete, which would severely compromise password security. Add error handling to ensure the salt is properly generated before proceeding with password hashing.
| rand.Read(salt) | |
| if n, err := rand.Read(salt); err != nil || n != len(salt) { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to generate secure salt"}) | |
| return | |
| } |
| if err := user.CreateUser(u.DB, userStruct); err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to create user"}) | ||
| return |
There was a problem hiding this comment.
The CreateUser function does not differentiate between different types of database errors. When a duplicate username is inserted (violating the UNIQUE constraint), the generic "Failed to create user" message is returned instead of a more specific error like "Username already exists". This makes it difficult for clients to handle duplicate username scenarios appropriately. Consider checking for unique constraint violations and returning a more specific error or status code.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| syscall.Kill(syscall.Getpid(), syscall.SIGABRT) | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
There's an extra blank line here that should be removed to maintain consistent code formatting.
| c.AbortWithStatus(http.StatusNotImplemented) | ||
| var body userBody | ||
| if err := c.ShouldBindJSON(&body); err != nil { | ||
| c.Error(err) |
There was a problem hiding this comment.
The error message returned to the client exposes the internal error details via err.Error(). This can leak sensitive information about the system's internals, including validation logic, field names, or other implementation details. Consider returning a more generic error message to the client while logging the detailed error server-side for debugging purposes.
| func CreateUser(db *sql.DB, user *User) error { | ||
| // insert into table users | ||
| sqlStatement := `INSERT INTO users (username, display_name, password_hash) VALUES ($1, $2, $3)` | ||
| _, err := db.Exec(sqlStatement, user.Username, user.DisplayName, user.PasswordHash) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create user: %v", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
There's no validation to check if the username already exists before attempting insertion. While the database has a UNIQUE constraint that will prevent duplicates, it's more efficient and user-friendly to check for existence first and return a clear error message. This would avoid unnecessary database constraint violations and provide better error handling.
| ) | ||
|
|
||
| type userBody struct { | ||
| Username string `json:"username" binding:"required"` |
There was a problem hiding this comment.
The username validation only checks that it's required, but doesn't validate the format or length. Consider adding validation rules such as maximum length (to match the database VARCHAR(50) constraint), allowed characters (e.g., alphanumeric and certain special characters), and potentially a minimum length to prevent extremely short usernames.
| Username string `json:"username" binding:"required"` | |
| Username string `json:"username" binding:"required,min=3,max=50,alphanum"` |
| type userBody struct { | ||
| Username string `json:"username" binding:"required"` | ||
| Password string `json:"password" binding:"required,min=8"` | ||
| DisplayName string `json:"display_name" binding:"required"` |
There was a problem hiding this comment.
The display_name validation only checks that it's required, but doesn't validate the length. The database has a VARCHAR(100) constraint, so attempting to insert a display_name longer than 100 characters will cause a database error. Add a max length validation (e.g., binding:"required,max=100") to catch this at the API level.
| DisplayName string `json:"display_name" binding:"required"` | |
| DisplayName string `json:"display_name" binding:"required,max=100"` |
|
|
||
| salt := make([]byte, 16) | ||
| _, err := rand.Read(salt) | ||
| if err != nil { | ||
| log.Println("rand.Read returned an error, this should not happen! (This means the OS is unable to provide proper crypto APIs. Do NOT run this program here.)") | ||
| log.Println("Tearing down the application with SIGABRT...") | ||
| syscall.Kill(syscall.Getpid(), syscall.SIGABRT) | ||
| } | ||
|
|
||
|
|
||
| userStruct := &user.User{ | ||
| Username: body.Username, | ||
| DisplayName: body.DisplayName, | ||
| PasswordHash: crypto.HashPassword(body.Password, salt), |
There was a problem hiding this comment.
The salt generated for password hashing is not being stored anywhere in the database. Without storing the salt, password verification will be impossible because you won't be able to recreate the same hash during login. The salt needs to be either:
- Stored in a separate column in the users table, or
- Embedded within the password hash string itself (which the argon-hash-utils library may already do via the ToString() method)
You should verify whether the argon-hash-utils library's ToString() method embeds the salt in the output string, or if you need to add a salt column to the database schema and User struct.
|
|
||
| if err := user.CreateUser(u.DB, userStruct); err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to create user"}) |
There was a problem hiding this comment.
The error handling when a duplicate username is attempted doesn't provide a meaningful response to the user. The database has a UNIQUE constraint on the username column, so attempting to create a duplicate user will result in a database error. This should be caught and returned as a specific error (e.g., HTTP 409 Conflict with "Username already exists") rather than a generic "Failed to create user" internal server error. Consider checking the error type to distinguish between duplicate key violations and other database errors.
| if err != nil { | ||
| log.Println("rand.Read returned an error, this should not happen! (This means the OS is unable to provide proper crypto APIs. Do NOT run this program here.)") | ||
| log.Println("Tearing down the application with SIGABRT...") |
There was a problem hiding this comment.
Using syscall.Kill with SIGABRT to terminate the application is an extreme reaction that bypasses graceful shutdown procedures. This can lead to resource leaks, incomplete transactions, and difficulty in debugging. Consider using panic() or log.Fatal() instead, which allow for proper cleanup via defer statements and provide better stack traces. If you need to signal a critical system issue, panic is the Go idiom for unrecoverable errors.
1 participant
No description provided.