feat: add MFA backup recovery codes#219
Conversation
|
Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement. I have read the CLA and agree to its terms. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
📝 WalkthroughWalkthroughAdds MFA backup code support end-to-end: a new ChangesMFA Backup Codes Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthHandler
participant AuthService
participant BackupCodeRepository
rect rgba(100, 150, 255, 0.5)
Note over Client,BackupCodeRepository: MFA Enable – backup code generation
Client->>AuthHandler: POST /auth/mfa/enable (TOTP verified)
AuthHandler->>AuthService: VerifyEnableMFA(userID, code)
loop 10 times
AuthService->>AuthService: generate random code, bcrypt hash
AuthService->>BackupCodeRepository: Create(BackupCode{UserID, CodeHash, Used:false})
end
AuthService-->>AuthHandler: MFAEnableResponse{BackupCodes: [...]}
AuthHandler-->>Client: 200 + backup codes (shown once)
end
rect rgba(100, 200, 150, 0.5)
Note over Client,BackupCodeRepository: MFA Login – TOTP fallback to backup code
Client->>AuthHandler: POST /auth/login/mfa {backupCode: "..."}
AuthHandler->>AuthService: VerifyLoginMFA(token, code)
AuthService->>AuthService: TOTP.ValidateCode → fails
AuthService->>BackupCodeRepository: FindByUserID(userID)
AuthService->>AuthService: bcrypt.CompareHashAndPassword for each code
AuthService->>BackupCodeRepository: MarkUsed(matchedID)
AuthService-->>AuthHandler: login success
AuthHandler-->>Client: 200 + session tokens
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
Hi @roshankumar0036singh i have raised a pr review it when u get a chance thanks!! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/routes/routes.go (1)
46-58: 🎯 Functional Correctness | 🔴 Critical
NewAuthServicewiring is syntactically invalid and won't compile.Line 57 passes a type expression (
*repository.BackupCodeRepository) instead of a variable, and thebackupCodeRepovariable is never initialized before the call. This causes both a syntax error and undefined variable reference.Suggested fix
auditRepo := repository.NewAuditRepository(db) + backupCodeRepo := repository.NewBackupCodeRepository(db) // OAuth Provider repositories oauthClientRepo := repository.NewOAuthClientRepository(db) ... authService := service.NewAuthService( userRepo, tokenRepo, verificationRepo, passwordResetRepo, tokenService, cacheService, emailService, auditService, mfaService, cfg, - backupCodeRepo *repository.BackupCodeRepository + backupCodeRepo, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/routes/routes.go` around lines 46 - 58, The NewAuthService constructor call contains a syntax error where the backupCodeRepo parameter is declared with its type annotation instead of passing a variable. Fix this by first initializing the backupCodeRepo variable (similar to how userRepo, tokenRepo, and other repository variables are initialized) before the NewAuthService call, then pass only the variable name without the type annotation as the final parameter to NewAuthService.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/dto/auth_dto.go`:
- Around line 107-110: The MFALoginRequest struct requires the Code field with
binding:"required", which prevents requests with only a backup code from passing
validation. To support backup-code-only authentication, remove the required
constraint from the Code field in the MFALoginRequest struct so both Code and
BackupCode can be optional. Then update the handler that processes
MFALoginRequest to pass req.BackupCode when calling VerifyLoginMFA. Update the
VerifyLoginMFA service method signature to accept the backup code parameter.
Finally, implement validation logic in the service to ensure exactly one of code
or backupCode is provided and handle both authentication paths (TOTP and backup
code) accordingly.
In `@internal/models/backup_code.go`:
- Around line 10-27: The Swagger annotations for MFA handler functions
(EnableMFA, LoginMFA, VerifyMFA, DisableMFA) are using the generic
utils.Response type instead of documenting the specific response DTOs they
return. Update each handler's `@Success` annotation to reference the correct
specific DTO type it returns (for example, EnableMFA should reference
dto.MFAEnableResponse instead of utils.Response). After updating all the
annotations for these handlers, run make swagger to regenerate the API
documentation.
In `@internal/repository/backup_code_repository.go`:
- Around line 34-37: The MarkUsed method in BackupCodeRepository has a race
condition vulnerability where concurrent requests can reuse the same one-time
backup code. Modify the WHERE clause in the Update call to add an AND condition
checking that the used field is false before updating it to true, making the
operation atomic. Additionally, after the Update call, check the RowsAffected
value to verify that exactly one row was actually updated, and return
ErrBackupCodeNotFound if no rows were affected (indicating the code was already
used or doesn't exist).
In `@internal/service/auth_service.go`:
- Around line 454-466: The code sets valid to true before confirming that the
backup code has been successfully marked as used in the database. In the backup
code validation block within the for loop, replace the line that ignores the
MarkUsed error with `_ = s.backupCodeRepo.MarkUsed(bc.ID)` by capturing the
error returned from s.backupCodeRepo.MarkUsed(bc.ID). Only set valid to true
after confirming that MarkUsed succeeded, and handle any persistence error
appropriately. This ensures that a database write failure does not result in
marking MFA as valid without actually consuming the one-time backup code.
- Around line 352-372: The backup code generation loop creates plaintext codes
but only stores their hashes to the database, leaving no way to return the
plaintext codes to the caller. Modify the method signature to return a slice of
strings containing the plaintext codes in addition to the error. Then, within
the loop where GenerateRandomString generates each code and before it is hashed,
append the plaintext code to a slice that gets returned at the end of the method
so users receive their one-time backup codes.
---
Outside diff comments:
In `@internal/routes/routes.go`:
- Around line 46-58: The NewAuthService constructor call contains a syntax error
where the backupCodeRepo parameter is declared with its type annotation instead
of passing a variable. Fix this by first initializing the backupCodeRepo
variable (similar to how userRepo, tokenRepo, and other repository variables are
initialized) before the NewAuthService call, then pass only the variable name
without the type annotation as the final parameter to NewAuthService.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1800c8c8-b78e-4333-94e6-9bebe0138e88
📒 Files selected for processing (7)
internal/dto/auth_dto.gointernal/handler/auth_handler.gointernal/models/backup_code.gointernal/repository/backup_code_repository.gointernal/routes/routes.gointernal/service/auth_service.gointernal/testutils/setup.go
| type MFALoginRequest struct { | ||
| MFAToken string `json:"mfaToken" binding:"required"` | ||
| Code string `json:"code" binding:"required,len=6"` | ||
| BackupCode string `json:"backupCode,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how LoginMFA binds and forwards fields today.
rg -n --type=go 'type MFALoginRequest|LoginMFA\(|ShouldBindJSON|VerifyLoginMFA\(' internal/dto internal/handler internal/serviceRepository: roshankumar0036singh/auth-server
Length of output: 2119
🏁 Script executed:
#!/bin/bash
# Read the MFALoginRequest struct definition
sed -n '100,115p' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server
Length of output: 835
🏁 Script executed:
#!/bin/bash
# Read the LoginMFA handler and its implementation
sed -n '969,990p' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server
Length of output: 726
🏁 Script executed:
#!/bin/bash
# Read the VerifyLoginMFA service method signature and initial implementation
sed -n '426,460p' internal/service/auth_service.goRepository: roshankumar0036singh/auth-server
Length of output: 1304
🏁 Script executed:
#!/bin/bash
# Read more of the VerifyLoginMFA service method
sed -n '426,500p' internal/service/auth_service.goRepository: roshankumar0036singh/auth-server
Length of output: 2355
Code field blocks backup-code-only authentication requests.
The Code field has binding:"required", so requests with only backupCode cannot pass JSON validation. Additionally, the handler doesn't pass req.BackupCode to VerifyLoginMFA, and the service signature doesn't accept a backup code parameter—currently, backup codes are only checked as a fallback when TOTP validation fails.
To support backup-code-only authentication as an alternative MFA method:
Required changes
type MFALoginRequest struct {
MFAToken string `json:"mfaToken" binding:"required"`
- Code string `json:"code" binding:"required,len=6"`
+ Code string `json:"code,omitempty" binding:"omitempty,len=6"`
BackupCode string `json:"backupCode,omitempty"`
}Then update the handler to pass req.BackupCode to the service, and update VerifyLoginMFA to accept and handle the backup code parameter, enforcing that exactly one of code or backupCode is provided.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/dto/auth_dto.go` around lines 107 - 110, The MFALoginRequest struct
requires the Code field with binding:"required", which prevents requests with
only a backup code from passing validation. To support backup-code-only
authentication, remove the required constraint from the Code field in the
MFALoginRequest struct so both Code and BackupCode can be optional. Then update
the handler that processes MFALoginRequest to pass req.BackupCode when calling
VerifyLoginMFA. Update the VerifyLoginMFA service method signature to accept the
backup code parameter. Finally, implement validation logic in the service to
ensure exactly one of code or backupCode is provided and handle both
authentication paths (TOTP and backup code) accordingly.
| type BackupCode struct { | ||
| ID string `gorm:"type:uuid;primaryKey"` | ||
| UserID string `gorm:"type:uuid;not null;index"` | ||
| CodeHash string `gorm:"not null"` | ||
| Used bool `gorm:"default:false"` | ||
| CreatedAt time.Time | ||
| DeletedAt gorm.DeletedAt `gorm:"index"` | ||
| } | ||
|
|
||
| func (b *BackupCode) BeforeCreate(tx *gorm.DB) error { | ||
| if b.ID == "" { | ||
| b.ID = uuid.New().String() | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (BackupCode) TableName() string { | ||
| return "backup_codes" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether generated API docs include the new backup-code fields/endpoint contract.
fd -i 'swagger|openapi|docs' .
rg -n --hidden -S 'backupCode|backupCodes|/api/auth/login/mfa' docs internal || trueRepository: roshankumar0036singh/auth-server
Length of output: 1766
🏁 Script executed:
# Check the MFA handler for Swagger annotations
sed -n '900,1050p' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server
Length of output: 2967
🏁 Script executed:
# Check if BackupCode model is documented in Swagger schema
rg -A 20 '"BackupCode"' docs/swagger.json || trueRepository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check the MFA response structure in Swagger
rg -B 5 -A 15 'backupCodes' docs/swagger.json || trueRepository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check the MFALoginResponse DTO structure
rg -A 10 'type MFALoginResponse' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check what fields are in the MFA response DTO
sed -n '80,120p' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server
Length of output: 1497
🏁 Script executed:
# Check the complete LoginResponse DTO structure
rg -A 20 'type LoginResponse struct' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server
Length of output: 1029
🏁 Script executed:
# Check if there's a specific MFA login response DTO
rg -B 2 -A 10 'MFALogin.*Response' internal/dto/auth_dto.goRepository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check if BackupCode model is documented in the generated Swagger schema
cat docs/swagger.json | grep -i 'backupcode\|backup_code' | head -20Repository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check the actual LoginResponse definition in swagger.json
rg -B 3 -A 15 'LoginResponse' docs/swagger.json | head -50Repository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check handler annotation more broadly around the MFA endpoints
rg -B 5 'LoginMFA' internal/handler/auth_handler.go | grep -E '@|func'Repository: roshankumar0036singh/auth-server
Length of output: 395
🏁 Script executed:
# Check what utils.Response structure looks like and how it's used in Swagger
rg -A 5 'type Response struct' internal/utils/ || rg -A 5 'type Response struct' . --type go | head -20Repository: roshankumar0036singh/auth-server
Length of output: 538
🏁 Script executed:
# Check if there are other handlers that document specific response structures
rg '`@Success`.*\{object\}' internal/handler/auth_handler.go | head -10Repository: roshankumar0036singh/auth-server
Length of output: 570
🏁 Script executed:
# Verify if LoginResponse should have BackupCodes field
rg -B 5 -A 15 'VerifyLoginMFA' internal/service/auth_service.go | head -50Repository: roshankumar0036singh/auth-server
Length of output: 1135
🏁 Script executed:
# Check when/how backup codes are returned - in enable MFA or login?
rg -B 5 -A 15 'BackupCode.*Create\|Generate.*Backup' internal/service/auth_service.go | head -60Repository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check the MFA enable endpoint handler to see if it documents BackupCodes
rg -B 8 'EnableMFA' internal/handler/auth_handler.go | grep -E '@|func' | head -20Repository: roshankumar0036singh/auth-server
Length of output: 377
🏁 Script executed:
# Check if docs need to include BackupCode model schema
rg '"BackupCode"' docs/swagger.yamlRepository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check if MFAEnableResponse is referenced in any Swagger annotation
rg 'MFAEnableResponse|MFALoginResponse' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Let's see the full EnableMFA and GenerateBackupCodes handlers
sed -n '850,950p' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server
Length of output: 3321
🏁 Script executed:
# Check the service method to confirm what it returns
rg -B 3 -A 20 'func.*GenerateBackupCodes' internal/service/auth_service.goRepository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check what EnableMFA service returns
rg -B 2 -A 20 'func.*EnableMFA' internal/service/auth_service.go | head -40Repository: roshankumar0036singh/auth-server
Length of output: 1137
🏁 Script executed:
# Check if swagger.json has any response definitions for MFA endpoints
rg -A 30 '"/api/auth/mfa/enable"' docs/swagger.jsonRepository: roshankumar0036singh/auth-server
Length of output: 1050
🏁 Script executed:
# Confirm the actual return type of EnableMFA
ast-grep outline internal/service/auth_service.go --match 'EnableMFA' --view expandedRepository: roshankumar0036singh/auth-server
Length of output: 361
🏁 Script executed:
# Check if BackupCode model has any Swagger annotations
cat internal/models/backup_code.goRepository: roshankumar0036singh/auth-server
Length of output: 730
🏁 Script executed:
# Verify the complete definitions section in swagger.json
rg '"definitions":|"components":' docs/swagger.json -A 5Repository: roshankumar0036singh/auth-server
Length of output: 351
🏁 Script executed:
# Check if there are Swagger struct tags on BackupCode model
sed -n '1,50p' internal/models/backup_code.goRepository: roshankumar0036singh/auth-server
Length of output: 730
🏁 Script executed:
# Check if other models have Swagger documentation/struct tags
rg 'swagger:' internal/models/ | head -10Repository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check if MFAEnableResponse is in the swagger definitions
rg '"MFAEnableResponse"' docs/swagger.jsonRepository: roshankumar0036singh/auth-server
Length of output: 170
🏁 Script executed:
# Check what the Makefile target 'swagger' does
cat Makefile | grep -A 10 'swagger'Repository: roshankumar0036singh/auth-server
Length of output: 572
Update Swagger annotations in MFA handlers to reference specific response DTOs, then run make swagger to regenerate docs.
The EnableMFA handler returns dto.MFAEnableResponse (which includes BackupCodes), but its Swagger annotation uses the generic @Success 200 {object} utils.Response. Update the annotation to reference the specific DTO so the generated docs properly document the backup-code response structure. Similarly, review other MFA endpoint annotations (LoginMFA, VerifyMFA, DisableMFA) to ensure they document specific response types rather than generic utils.Response. After updating annotations, run make swagger to regenerate the API documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/models/backup_code.go` around lines 10 - 27, The Swagger annotations
for MFA handler functions (EnableMFA, LoginMFA, VerifyMFA, DisableMFA) are using
the generic utils.Response type instead of documenting the specific response
DTOs they return. Update each handler's `@Success` annotation to reference the
correct specific DTO type it returns (for example, EnableMFA should reference
dto.MFAEnableResponse instead of utils.Response). After updating all the
annotations for these handlers, run make swagger to regenerate the API
documentation.
Source: Coding guidelines
| func (r *BackupCodeRepository) MarkUsed(id string) error { | ||
| return r.db.Model(&models.BackupCode{}). | ||
| Where("id = ?", id). | ||
| Update("used", true).Error |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify MarkUsed behavior and all call sites.
rg -n --type=go 'func \(r \*BackupCodeRepository\) MarkUsed|ErrBackupCodeNotFound|MarkUsed\(' internal/repository internal/serviceRepository: roshankumar0036singh/auth-server
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Get the full BackupCodeRepository implementation
cat -n internal/repository/backup_code_repository.goRepository: roshankumar0036singh/auth-server
Length of output: 1273
🏁 Script executed:
#!/bin/bash
# Check the call site in auth_service.go around line 465
sed -n '450,480p' internal/service/auth_service.go | cat -nRepository: roshankumar0036singh/auth-server
Length of output: 1096
🏁 Script executed:
#!/bin/bash
# Check the BackupCode model structure
find . -name "*.go" -type f | xargs grep -l "type BackupCode" | head -5Repository: roshankumar0036singh/auth-server
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Check the BackupCode model structure
cat -n internal/models/backup_code.goRepository: roshankumar0036singh/auth-server
Length of output: 926
🏁 Script executed:
#!/bin/bash
# Get more context around the call site to understand the full flow
sed -n '440,490p' internal/service/auth_service.go | cat -nRepository: roshankumar0036singh/auth-server
Length of output: 1840
Make backup-code consumption atomic in MarkUsed.
Concurrent requests can both pass verification and reuse a one-time backup code. The current implementation updates by id only without checking the used field, allowing race conditions. Additionally, ErrBackupCodeNotFound is never returned because RowsAffected is not verified.
Add an AND used = ? condition to the WHERE clause and check RowsAffected to ensure the operation is atomic:
Suggested fix
func (r *BackupCodeRepository) MarkUsed(id string) error {
- return r.db.Model(&models.BackupCode{}).
- Where("id = ?", id).
- Update("used", true).Error
+ tx := r.db.Model(&models.BackupCode{}).
+ Where("id = ? AND used = ?", id, false).
+ Update("used", true)
+ if tx.Error != nil {
+ return tx.Error
+ }
+ if tx.RowsAffected == 0 {
+ return ErrBackupCodeNotFound
+ }
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *BackupCodeRepository) MarkUsed(id string) error { | |
| return r.db.Model(&models.BackupCode{}). | |
| Where("id = ?", id). | |
| Update("used", true).Error | |
| func (r *BackupCodeRepository) MarkUsed(id string) error { | |
| tx := r.db.Model(&models.BackupCode{}). | |
| Where("id = ? AND used = ?", id, false). | |
| Update("used", true) | |
| if tx.Error != nil { | |
| return tx.Error | |
| } | |
| if tx.RowsAffected == 0 { | |
| return ErrBackupCodeNotFound | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/repository/backup_code_repository.go` around lines 34 - 37, The
MarkUsed method in BackupCodeRepository has a race condition vulnerability where
concurrent requests can reuse the same one-time backup code. Modify the WHERE
clause in the Update call to add an AND condition checking that the used field
is false before updating it to true, making the operation atomic. Additionally,
after the Update call, check the RowsAffected value to verify that exactly one
row was actually updated, and return ErrBackupCodeNotFound if no rows were
affected (indicating the code was already used or doesn't exist).
| for i := 0; i < 10; i++ { | ||
| code := s.tokenService.GenerateRandomString(8) | ||
|
|
||
| hash, err := bcrypt.GenerateFromPassword( | ||
| []byte(code), | ||
| bcrypt.DefaultCost, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| backup := &models.BackupCode{ | ||
| UserID: userID, | ||
| CodeHash: string(hash), | ||
| Used: false, | ||
| } | ||
|
|
||
| if err := s.backupCodeRepo.Create(backup); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Generated backup codes are discarded instead of being returned once.
Line 352-Line 372 creates plaintext backup codes and only stores hashes, but this method returns error only. That leaves no path to deliver the one-time backup codes to the user at enable time.
Suggested direction
-func (s *AuthService) VerifyEnableMFA(userID, code string) error {
+func (s *AuthService) VerifyEnableMFA(userID, code string) ([]string, error) {
+ backupCodes := make([]string, 0, 10)
...
for i := 0; i < 10; i++ {
code := s.tokenService.GenerateRandomString(8)
+ backupCodes = append(backupCodes, code)
...
- return err
+ return nil, err
}
}
...
- return nil
+ return backupCodes, nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/auth_service.go` around lines 352 - 372, The backup code
generation loop creates plaintext codes but only stores their hashes to the
database, leaving no way to return the plaintext codes to the caller. Modify the
method signature to return a slice of strings containing the plaintext codes in
addition to the error. Then, within the loop where GenerateRandomString
generates each code and before it is hashed, append the plaintext code to a
slice that gets returned at the end of the method so users receive their
one-time backup codes.
| if !valid { | ||
| codes, err := s.backupCodeRepo.FindByUserID(userID) | ||
|
|
||
| if err == nil { | ||
| for _, bc := range codes { | ||
| if bcrypt.CompareHashAndPassword( | ||
| []byte(bc.CodeHash), | ||
| []byte(code), | ||
| ) == nil { | ||
|
|
||
| valid = true | ||
| _ = s.backupCodeRepo.MarkUsed(bc.ID) | ||
| break |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only mark MFA valid after backup-code consumption succeeds.
Line 464 sets valid = true before persistence is confirmed, and Line 465 drops MarkUsed errors. A DB write failure would allow login without consuming the one-time code.
Suggested fix
- valid = true
- _ = s.backupCodeRepo.MarkUsed(bc.ID)
- break
+ if err := s.backupCodeRepo.MarkUsed(bc.ID); err == nil {
+ valid = true
+ break
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/auth_service.go` around lines 454 - 466, The code sets valid
to true before confirming that the backup code has been successfully marked as
used in the database. In the backup code validation block within the for loop,
replace the line that ignores the MarkUsed error with `_ =
s.backupCodeRepo.MarkUsed(bc.ID)` by capturing the error returned from
s.backupCodeRepo.MarkUsed(bc.ID). Only set valid to true after confirming that
MarkUsed succeeded, and handle any persistence error appropriately. This ensures
that a database write failure does not result in marking MFA as valid without
actually consuming the one-time backup code.
1 participant
Closes #74
Changes
Security
Acceptance Criteria Covered
Summary by CodeRabbit