Skip to content
This repository was archived by the owner on Oct 10, 2025. It is now read-only.

fix: do not return specific errors on unauthenticated endpoints to avoid account enumeration - #671

Merged
dbarrosop merged 2 commits into
mainfrom
change-password-enum
Sep 1, 2025
Merged

fix: do not return specific errors on unauthenticated endpoints to avoid account enumeration#671
dbarrosop merged 2 commits into
mainfrom
change-password-enum

Conversation

@dbarrosop

@dbarrosop dbarrosop commented Aug 29, 2025

Copy link
Copy Markdown
Member

PR Type

Bug fix


Description

  • Prevent account enumeration in password reset endpoint

  • Return success response for all invalid email scenarios

  • Update error handling to mask user existence information

  • Modify tests to expect consistent success responses


Diagram Walkthrough

flowchart LR
  A["Password Reset Request"] --> B["Email Validation"]
  B --> C["User Lookup"]
  C --> D["Error Handling"]
  D --> E["Success Response (Always)"]
  F["Previous: Specific Errors"] --> G["New: Generic Success"]
Loading

File Walkthrough

Relevant files
Bug fix
send_password_reset_email.go
Implement anti-enumeration logic in password reset             

go/controller/send_password_reset_email.go

  • Import errors package for error type checking
  • Return success response instead of error for invalid emails
  • Add switch statement to handle internal server errors separately
  • Mask user existence information in all non-critical error cases
+6/-2     
Tests
send_password_reset_email_test.go
Update tests for anti-enumeration behavior                             

go/controller/send_password_reset_email_test.go

  • Update test expectations from error responses to success responses
  • Remove assertions for specific error messages and status codes
  • Modify four test cases to expect consistent OK responses
+4/-20   

@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Logic Error

The switch statement at line 34 has a missing return statement after line 36, causing execution to fall through to the next case. This could lead to incorrect behavior when handling internal server errors.

switch {
case errors.Is(apiErr, ErrInternalServerError):
	return ctrl.respondWithError(apiErr), nil
case apiErr != nil:
	return api.SendPasswordResetEmail200JSONResponse(api.OK), nil
}
Missing Import

The code uses errors.Is() function but the import statement for the errors package should be verified to ensure it's the standard library package and not conflicting with any custom error types.

"errors"

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add nil user check

The switch statement should handle the case where apiErr is nil but user might be
nil. This could lead to a nil pointer dereference when accessing user in subsequent
code. Add an explicit check for nil user after the error handling.

go/controller/send_password_reset_email.go [33-39]

 user, apiErr := ctrl.wf.GetUserByEmail(ctx, string(request.Body.Email), logger)
 switch {
 case errors.Is(apiErr, ErrInternalServerError):
     return ctrl.respondWithError(apiErr), nil
 case apiErr != nil:
     return api.SendPasswordResetEmail200JSONResponse(api.OK), nil
 }
 
+if user == nil {
+    return api.SendPasswordResetEmail200JSONResponse(api.OK), nil
+}
+
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential nil pointer dereference issue. After error handling, user could be nil even when apiErr is nil, which would cause problems in subsequent code that accesses user.

Medium

@dbarrosop
dbarrosop merged commit d070758 into main Sep 1, 2025
8 checks passed
@dbarrosop
dbarrosop deleted the change-password-enum branch September 1, 2025 06:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants