fix(user): make registration more resilient to email failures#1304
fix(user): make registration more resilient to email failures#1304
Conversation
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
- Add BeforeCreate hook to validate email syntax before user creation - Make email verification sending non-fatal during account creation - Log warning when email fails to send but allow registration to succeed - Track failed email sends with metrics - Configure HELO domain for mail server to improve SMTP handshakes
pcfreak30
force-pushed
the
refactor-portal
branch
from
e825223 to
33294be
Compare
|
The BeforeCreate hook in db/models/user.go uses GORM operations without a context timeout. The getEmailVerifier().Verify() call and the error handling should be wrapped with a context that includes a timeout to prevent indefinite operations. Update the BeforeCreate method to use tx.WithContext(ctx) and ensure the context passed to the method has an appropriate timeout. Kody Rule violation: Disallow GORM queries without timeout |
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| if err := u.SendEmailVerification(ctx, _user.ID); err != nil { | ||
| return nil, err | ||
| u.Logger().Warn("Failed to send email verification during account creation, but account was created successfully", | ||
| zap.Uint("user_id", _user.ID), | ||
| zap.String("email", _user.Email), | ||
| zap.Error(err)) | ||
| mailer.MailerFailed.WithLabelValues(mailer.LabelOpSend).Inc() | ||
| } |
There was a problem hiding this comment.
Inconsistent email validation error handling between model layer and service layer creates security and reliability issues. The model's BeforeCreate hook treats email verification service errors as fatal, while the service layer only logs warnings for similar failures.
This issue appears in multiple locations:
- service/user.go: Lines 277-283
- db/models/user.go: Lines 35-46
Please standardize email validation error handling across all layers - either make the model layer more lenient to allow registration during service failures, or make the service layer more strict to match the model's validation approach.
// service/user.go - consistent strict handling
if err := u.SendEmailVerification(ctx, _user.ID); err != nil {
u.Logger().Error("Failed to send email verification during account creation",
zap.Uint("user_id", _user.ID),
zap.String("email", _user.Email),
zap.Error(err))
mailer.MailerFailed.WithLabelValues(mailer.LabelOpSend).Inc()
return nil, fmt.Errorf("failed to send email verification: %w", err)
}Prompt for LLM
File service/user.go:
Line 277 to 283:
I have an inconsistency in email validation error handling across my Go application. In the User model's BeforeCreate hook, email validation is strict and returns errors for invalid emails. However, in the user service layer, when email verification fails during account creation, it only logs a warning and continues. This creates a security gap where invalid emails might pass model validation but fail verification without preventing account creation. Please help me understand how to make this consistent and secure.
Suggested Code:
// service/user.go - consistent strict handling
if err := u.SendEmailVerification(ctx, _user.ID); err != nil {
u.Logger().Error("Failed to send email verification during account creation",
zap.Uint("user_id", _user.ID),
zap.String("email", _user.Email),
zap.Error(err))
mailer.MailerFailed.WithLabelValues(mailer.LabelOpSend).Inc()
return nil, fmt.Errorf("failed to send email verification: %w", err)
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| } else if verifyEmail { | ||
| if err := u.SendEmailVerification(ctx, _user.ID); err != nil { | ||
| return nil, err | ||
| u.Logger().Warn("Failed to send email verification during account creation, but account was created successfully", | ||
| zap.Uint("user_id", _user.ID), | ||
| zap.String("email", _user.Email), | ||
| zap.Error(err)) | ||
| mailer.MailerFailed.WithLabelValues(mailer.LabelOpSend).Inc() | ||
| } | ||
| } |
There was a problem hiding this comment.
The email validation logic in CreateAccount has a critical bug where it treats falsy email values as valid. When an email like '0' or 'false' is provided, the validation passes because the code checks for truthiness rather than proper email format validation. This allows invalid emails to be registered, leading to account creation with malformed email addresses that cannot receive verification emails.
func (u UserServiceDefault) CreateAccount(ctx context.Context, email string, password string, verifyEmail bool) (*models.User, error) {
ctx, span := core.TraceMethod(ctx, "UserServiceDefault.CreateAccount")
defer span.End()
result, err := core.MetricTrackResult(
userInternal.UserOperationDuration.WithLabelValues(userInternal.LabelOpCreate),
userInternal.UserOperationFailed.WithLabelValues(userInternal.LabelOpCreate),
func() (*models.User, error) {
// Validate email format before proceeding
if email == "" || !strings.Contains(email, "@") {
return nil, core.NewAccountError(core.ErrKeyInvalidEmail, nil)
}
// First check if email already exists
exists, _, err := u.EmailExists(ctx, email)
if err != nil {
return nil, fmt.Errorf("error checking email existence: %w", err)
}
if exists {
return nil, core.NewAccountError(core.ErrKeyEmailAlreadyExists, nil)
}Prompt for LLM
File service/user.go:
Line 275 to 284:
I need you to analyze a critical email validation bug in a user account creation function. The CreateAccount function currently accepts any string as an email address, including falsy values like '0', 'false', or '0@domain.com' which are invalid email formats. The function only checks if the email already exists in the database but doesn't validate the email format before creating the account. This leads to accounts being created with malformed emails that cannot receive verification emails. Please identify where the email format validation should be added and what validation logic should be implemented to ensure only properly formatted email addresses are accepted.
Suggested Code:
func (u UserServiceDefault) CreateAccount(ctx context.Context, email string, password string, verifyEmail bool) (*models.User, error) {
ctx, span := core.TraceMethod(ctx, "UserServiceDefault.CreateAccount")
defer span.End()
result, err := core.MetricTrackResult(
userInternal.UserOperationDuration.WithLabelValues(userInternal.LabelOpCreate),
userInternal.UserOperationFailed.WithLabelValues(userInternal.LabelOpCreate),
func() (*models.User, error) {
// Validate email format before proceeding
if email == "" || !strings.Contains(email, "@") {
return nil, core.NewAccountError(core.ErrKeyInvalidEmail, nil)
}
// First check if email already exists
exists, _, err := u.EmailExists(ctx, email)
if err != nil {
return nil, fmt.Errorf("error checking email existence: %w", err)
}
if exists {
return nil, core.NewAccountError(core.ErrKeyEmailAlreadyExists, nil)
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
1 participant
Based on the code changes provided, here's a precise description for this pull request:
Pull Request Description:
This PR makes the user registration process more resilient by implementing email validation and handling email sending failures gracefully. The changes ensure that registration doesn't fail completely when email services encounter issues.
Key Changes:
Email Validation: Added email syntax validation during user creation using a verification service to catch invalid email addresses before account creation.
Email Failure Handling: Modified the account creation process to continue successfully even when email verification fails. Instead of returning an error, the system now logs a warning and creates a metrics counter for email failures, allowing the account to be created while tracking the email service issue.
Email Configuration Enhancement: Improved the mailer service configuration by adding HELO domain support, which can help with email deliverability and server compatibility.
The primary functional impact is that users can now successfully register accounts even if the email verification system encounters temporary issues, while still maintaining email validation for syntax correctness.