fix(auth): ensure auth endpoints return JSON when config load fails#273
fix(auth): ensure auth endpoints return JSON when config load fails#273rushikesh249 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a GET /health endpoint and replaces the previous VerifyEmail auth flow with a SignUp flow that validates input, prevents duplicate emails, hashes passwords, creates unverified users with verification codes, stores them in MongoDB, and sends verification emails. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SignUpHandler as SignUp Handler
participant Database as MongoDB
participant EmailService as Email Service
Client->>SignUpHandler: POST /signup (email, password)
SignUpHandler->>SignUpHandler: Validate SignUpRequest
SignUpHandler->>Database: Find user by email
Database-->>SignUpHandler: User exists?
alt User exists
SignUpHandler-->>Client: Error response (user exists)
else New user
SignUpHandler->>SignUpHandler: Hash password & generate code
SignUpHandler->>Database: Insert unverified user with code
Database-->>SignUpHandler: Insert OK
SignUpHandler->>EmailService: Send verification email(code)
EmailService-->>SignUpHandler: Email sent
SignUpHandler-->>Client: Success response (no JWT)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@backend/controllers/auth.go`:
- Around line 286-291: The handler is double-writing a JSON error because
loadConfig() already sends a response before returning nil; remove the redundant
ctx.JSON call in the nil-check for cfg (the block that calls ctx.JSON(500,
gin.H{"error":"Server configuration error"}) and then return) and simply return
when cfg == nil, relying on loadConfig() to have sent the error; locate the nil
check around where cfg is set in the auth handler (the cfg variable and call to
loadConfig()) and remove the extra ctx.JSON invocation to avoid double response
writes.
- Around line 203-210: Delete the duplicate SignUp function (the earlier
definition) so there is only one SignUp in the package, and in the remaining
SignUp remove the redundant ctx.JSON(...) error response after calling
loadConfig() (since loadConfig() already writes the error and returns nil); use
loadConfig()'s nil check to return early without writing another response.
Ensure you only keep a single SignUp function and that it calls loadConfig(ctx)
and returns if cfg == nil.
🧹 Nitpick comments (1)
backend/controllers/auth.go (1)
539-550: Clarify error handling strategy for consistency.The
loadConfighelper already sends a JSON error response on line 546 before returningnil. This means handlers likeGoogleLogin,ForgotPassword,VerifyForgotPassword, andVerifyTokenthat simplyreturnwhencfg == nilare already returning valid JSON to clients.If you want custom error messages per handler, consider refactoring
loadConfigto return the error without writing to the response, then let each handler decide the message. Otherwise, remove the redundantctx.JSON()calls fromSignUpandLoginto maintain consistency.Option: Refactor loadConfig to not write response
-func loadConfig(ctx *gin.Context) *config.Config { +func loadConfig(ctx *gin.Context) (*config.Config, error) { cfgPath := os.Getenv("CONFIG_PATH") if cfgPath == "" { cfgPath = "./config/config.prod.yml" } cfg, err := config.LoadConfig(cfgPath) if err != nil { - ctx.JSON(500, gin.H{"error": "Internal server error"}) - return nil + return nil, err } - return cfg + return cfg, nil }Then each handler can decide the error message:
cfg, err := loadConfig(ctx) if err != nil { ctx.JSON(500, gin.H{"error": "Server configuration error"}) return }
backend/controllers/auth.go
Outdated
| func SignUp(ctx *gin.Context) { | ||
| cfg := loadConfig(ctx) | ||
| if cfg == nil { | ||
| ctx.JSON(500, gin.H{ | ||
| "error": "Server configuration error", | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Duplicate function definition will cause compilation error.
There are two SignUp functions defined in this file (lines 124-201 and lines 203-282). Go does not allow multiple functions with the same name and signature in the same package. The first SignUp function (lines 124-201) should be removed.
Additionally, the JSON error response at lines 206-208 is redundant because loadConfig() already writes a JSON error response (line 546) before returning nil. Writing two responses will cause runtime issues like "http: superfluous response.WriteHeader call".
Proposed fix: Remove the duplicate JSON response
If you keep the first SignUp function removed (as intended), the second SignUp should not add another JSON response:
func SignUp(ctx *gin.Context) {
cfg := loadConfig(ctx)
if cfg == nil {
- ctx.JSON(500, gin.H{
- "error": "Server configuration error",
- })
return
}Alternatively, if you want custom error messages per handler, modify loadConfig to not send a response and let callers handle it.
📝 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 SignUp(ctx *gin.Context) { | |
| cfg := loadConfig(ctx) | |
| if cfg == nil { | |
| ctx.JSON(500, gin.H{ | |
| "error": "Server configuration error", | |
| }) | |
| return | |
| } | |
| func SignUp(ctx *gin.Context) { | |
| cfg := loadConfig(ctx) | |
| if cfg == nil { | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@backend/controllers/auth.go` around lines 203 - 210, Delete the duplicate
SignUp function (the earlier definition) so there is only one SignUp in the
package, and in the remaining SignUp remove the redundant ctx.JSON(...) error
response after calling loadConfig() (since loadConfig() already writes the error
and returns nil); use loadConfig()'s nil check to return early without writing
another response. Ensure you only keep a single SignUp function and that it
calls loadConfig(ctx) and returns if cfg == nil.
1 participant
Fixes #243
Problem
When configuration loading failed, authentication endpoints could return empty or malformed responses due to improper error handling.
Additionally, duplicate response writes caused runtime warnings like:
"http: superfluous response.WriteHeader call"
Resolution
SignUpfunction that caused compilation errorsloadConfig()fails, since it already sends an error responseSignUpandLoginto simply return when configuration loading failsThis prevents double response writes and ensures consistent, valid JSON error handling across authentication endpoints.