Bug/silent failures caused by swallowed errors#247
Open
Devnil434 wants to merge 7 commits intoAOSSIE-Org:mainfrom
Open
Bug/silent failures caused by swallowed errors#247Devnil434 wants to merge 7 commits intoAOSSIE-Org:mainfrom
Devnil434 wants to merge 7 commits intoAOSSIE-Org:mainfrom
Conversation
… messaging, judging, and user gamification.
📝 WalkthroughWalkthroughAdds TOTP-based MFA (enable/finalize/verify) with encrypted secrets and HKDF-derived encryption key; introduces AES‑GCM encryption utilities; adds Redis-backed rate limiter and per-route limits; refactors Elo updates into MongoDB transactions; increases error logging across controllers and websockets; removes sample prod config and some env placeholders. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Auth API
participant DB as User DB
participant Sec as Security (encryption)
participant JWT as JWT Service
participant Redis as Redis
Client->>API: POST /login (email,password)
API->>DB: Find user by email
DB-->>API: user (includes MFAEnabled)
alt MFAEnabled == true
API-->>Client: 202 Accepted { mfaType, pendingToken }
Client->>API: POST /login/mfa/verify (pendingToken, code)
API->>DB: Load user by id
API->>Sec: Decrypt user's MFA secret
Sec-->>API: secret
API->>API: Verify TOTP code
alt valid
API->>JWT: generate access token
JWT-->>API: token
API->>DB: Persist login stats (log errors)
API-->>Client: 200 OK { token, user }
else invalid
API-->>Client: 401 Unauthorized
end
else
API->>JWT: generate access token
JWT-->>API: token
API->>DB: Persist login stats (log errors)
API-->>Client: 200 OK { token, user }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/controllers/profile_controller.go (1)
47-85: Redact or gate full query logging to avoid leaking sensitive data.Logging
Request URL,Raw Query, and full query map can capture tokens or PII if additional parameters are ever added. Consider whitelisting onlyuserIdor gating the verbose logs behind a debug flag. (Line 50–53)🛡️ Proposed safer logging
- log.Printf("GetProfile: Request URL = '%s'", c.Request.URL.String()) - log.Printf("GetProfile: Raw Query = '%s'", c.Request.URL.RawQuery) - log.Printf("GetProfile: Query params map = %v", c.Request.URL.Query()) - log.Printf("GetProfile: userId from c.Query() = '%s'", userIDParam) + log.Printf("GetProfile: userId from query = '%s'", userIDParam)
🤖 Fix all issues with AI agents
In `@backend/controllers/profile_controller.go`:
- Around line 387-392: The handler currently logs UpdateOne failures for the
winner/loser but still returns 200; change the logic around the two
db.MongoDatabase.Collection("users").UpdateOne calls (the winnerID/loserID
updates using dbCtx and req.WinnerID/req.LoserID) to propagate errors: if either
UpdateOne returns an error, abort the handler with a non-200 response (return
the error to caller), and ideally perform both updates inside a MongoDB
transaction/session so you can rollback on failure; ensure you return after
sending the error response so the handler does not continue to report success.
In `@backend/structs/auth.go`:
- Around line 3-6: Update the password policy and compensating controls: change
the SignUpRequest password binding from min=6 to min=8 (and apply the same
validation to AdminSignupRequest and AdminLoginRequest) to enforce a consistent
minimum; add or wire up a rate-limiting middleware on auth endpoints (signup,
login, password reset) and/or enable MFA hooks for account creation and login
flows; and add a short documented password policy comment near the structs
(SignUpRequest, AdminSignupRequest, AdminLoginRequest) describing length,
complexity, and where to get security approval for exceptions.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@backend/controllers/profile_controller.go`:
- Around line 376-377: The code is discarding errors from
primitive.ObjectIDFromHex for req.WinnerID and req.LoserID (variables winnerID,
loserID); validate both conversions and if either returns an error return a 400
Bad Request with a clear message (e.g., "invalid winner_id" or "invalid
loser_id") instead of proceeding to the FindOne calls. Update the handler in
profile_controller.go to check the error values from ObjectIDFromHex, produce a
JSON/HTTP 400 response when malformed, and only use the ObjectIDs when
conversion succeeds.
In `@backend/middlewares/rate_limit.go`:
- Around line 15-19: The code currently skips rate limiting when db.RedisClient
== nil but only comments "log a warning" without actually logging; update the
rate limit middleware to emit a warning before calling c.Next() (e.g., use the
standard log package or your existing app logger) so operators see "Redis
unavailable, skipping rate limiting" (include db context if available) and add
the "log" import (or the appropriate logger import) so the warning is compiled
and logged; target the db.RedisClient nil branch around the rate-limit
middleware where c.Next() is invoked.
- Around line 26-35: The Incr/Expire pair is non-atomic and can leave a key
without TTL if the process dies between calls; replace the two calls
(db.RedisClient.Incr and db.RedisClient.Expire on key/window) with a single
atomic Redis operation (either EVAL a small Lua script that does local =
redis.call("INCR", KEYS[1]); if local == 1 then redis.call("EXPIRE", KEYS[1],
ARGV[1]) end; return local, or use a pipeline/transaction that does SET NX EX
semantics combined with INCR) and update the rateLimit logic to use the script's
return value for the count; also ensure any Redis errors (the err returned from
the Redis call) are logged via the package's logger instead of being silently
ignored before calling c.Next(), so replace the current error branch to log the
error and then fail-open.
In `@backend/models/user.go`:
- Around line 35-38: The model currently exposes MFA fields (MFAEnabled,
MFAType, MFASecret) with a misleading comment; either remove these fields or
implement full TOTP support—if you implement, add a clear flow: use a TOTP
library like pquerna/otp to generate and validate secrets (create functions such
as GenerateMFASecret/EnableMFA and VerifyTOTP on the User model), encrypt
MFASecret before persisting using the project’s existing crypto utilities or a
secure AES/GCM wrapper (add SetEncryptedMFASecret/GetDecryptedMFASecret
helpers), and ensure MFASecret is never logged or returned in JSON by keeping
the json tag omitted; update any create/update handlers to call these helpers
and add unit tests for generation, encryption, and verification instead of
leaving the current placeholder comment.
🧹 Nitpick comments (3)
backend/controllers/auth.go (1)
77-101: Missing MFA field initialization for Google OAuth users.New users created via Google OAuth do not have
MFAEnabledandMFATypefields initialized, unlikeSignUp(lines 181-183). WhileMFAEnableddefaults tofalse(Go zero value), explicitly initializing these fields ensures consistency and prevents potential issues if the default MFA type logic changes.♻️ Suggested fix
newUser := models.User{ Email: email, DisplayName: nickname, Nickname: nickname, Bio: "", Rating: 1200.0, RD: 350.0, Volatility: 0.06, LastRatingUpdate: now, AvatarURL: avatarURL, IsVerified: true, Score: 0, // Initialize gamification score Badges: []string{}, // Initialize badges array CurrentStreak: 0, // Initialize streak CreatedAt: now, UpdatedAt: now, + MFAEnabled: false, + MFAType: "totp", }backend/routes/auth.go (1)
15-22: Inefficient middleware instantiation pattern.Calling
middlewares.RateLimit(5, time.Minute)(c)inside the handler creates a new middleware closure on every request. The idiomatic approach is to register middleware once on the route. This applies to all three rate-limited handlers.♻️ Suggested refactor for route registration
In the route registration file (likely
main.goor a routes setup file), register the middleware on the route:// Instead of calling middleware inside handlers, register on routes: authGroup := router.Group("/auth") authGroup.POST("/signup", middlewares.RateLimit(5, time.Minute), controllers.SignUp) authGroup.POST("/login", middlewares.RateLimit(10, time.Minute), controllers.Login) authGroup.POST("/forgot-password", middlewares.RateLimit(3, time.Minute), controllers.ForgotPassword)Then simplify the handlers:
func SignUpRouteHandler(c *gin.Context) { - // Apply rate limiting: 5 requests per minute - middlewares.RateLimit(5, time.Minute)(c) - if c.IsAborted() { - return - } controllers.SignUp(c) }backend/controllers/profile_controller.go (1)
49-86: Consider reducing verbose debug logging.The extensive
log.Printfcalls for userId extraction (lines 51-54, 61, 76, 82, 85, 89, 100, 105, 134) are helpful during development but may be excessive in production. Consider using a debug log level or removing after the feature is stable.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/controllers/profile_controller.go (1)
50-105: Redact PII in GetProfile logging.These logs include raw query strings and user email (Line 52, Line 105), which can leak PII or tokens. Please remove or redact sensitive fields, or gate them behind a debug-only logger.
🔒 Suggested redaction
- log.Printf("GetProfile: Raw Query = '%s'", c.Request.URL.RawQuery) - log.Printf("GetProfile: Query params map = %v", c.Request.URL.Query()) - log.Printf("GetProfile: userId from c.Query() = '%s'", userIDParam) + log.Printf("GetProfile: userId param present=%t", userIDParam != "") ... - log.Printf("GetProfile: Found user - ID: %s, Email: %s, DisplayName: %s", user.ID.Hex(), user.Email, user.DisplayName) + log.Printf("GetProfile: Found user - ID: %s, DisplayName: %s", user.ID.Hex(), user.DisplayName)backend/go.mod (1)
29-60: github.com/pquerna/otp should be classified as a direct dependency.The module
github.com/pquerna/otpis marked as// indirectin go.mod (line 60), butgithub.com/pquerna/otp/totpis directly imported inbackend/models/user.go:8. Since importing a subpackage means importing the module itself,github.com/pquerna/otpshould be a direct dependency. Rungo mod tidyto correct this classification. Thegithub.com/boombuler/barcodedependency remains correctly marked as indirect (it's only needed transitively through pquerna/otp).
🤖 Fix all issues with AI agents
In `@backend/cmd/server/main.go`:
- Around line 64-65: You are currently reusing cfg.JWT.Secret for both signing
and encryption via utils.SetJWTSecret and security.SetEncryptionKey; instead,
stop passing the JWT secret directly to security.SetEncryptionKey and either
read a dedicated encryption key from configuration/environment (e.g.,
cfg.Encryption.Key) or derive a separate key from cfg.JWT.Secret using HKDF with
a distinct context/info label (e.g., "mfa-encryption") before calling
security.SetEncryptionKey so signing and encryption keys remain distinct and
rotatable.
In `@backend/controllers/auth.go`:
- Around line 626-649: The current TOTP verification allows exchanging
email+code for a JWT without ensuring the password step completed; modify the
flow so VerifyTOTP (in this handler) only runs when the user has MFA enabled
(check user.MFAEnabled) and there is a valid short‑lived pending MFA context
issued at login (e.g., require and validate a pendingMFA token or server‑side
pending state created by /login) that ties this verification to a completed
password authentication; if the pendingMFA token is missing/expired or
user.MFAEnabled is false, return an appropriate 401/400 response and do not
issue a JWT.
- Line 106: Replace raw email exposure in log statements by logging a non-PII
identifier: change occurrences that use the variable email (e.g., the log.Printf
call "Error persisting user stats for %s: %v", email, err and the other
log.Printf lines around 298-299 and 315-326) to use a redacted or hashed form
(e.g., call a small helper like RedactEmail(email) or HashIdentifier(email) and
log that instead), or log the internal user ID if available; apply the same
replacement for all log.Printf/error log sites in this file that currently print
email.
- Around line 651-657: In VerifyTOTP, load the config once into a local variable
(e.g., cfg := loadConfig(ctx)) and check for nil; if nil return an error
response instead of proceeding. Call generateJWT with cfg.JWT.Secret and
cfg.JWT.Expiry and capture its error (e.g., token, err := generateJWT(...)); if
generateJWT returns an error, return a 500 JSON error instead of sending a 200
with an empty token. Ensure you still call buildUserResponse(user) only after
successful token creation and include the valid token in the "accessToken"
field.
In `@backend/routes/auth.go`:
- Around line 58-60: The VerifyTOTPRouteHandler for the /login/mfa/verify
endpoint is missing rate limiting; wrap the handler with middlewares.RateLimit
so MFA verification can't be brute-forced. Update the route registration to
apply middlewares.RateLimit (use a stricter limit than the regular login) before
calling controllers.VerifyTOTP, or modify VerifyTOTPRouteHandler to call
middlewares.RateLimit(...) -> controllers.VerifyTOTP, ensuring you reference
VerifyTOTPRouteHandler, controllers.VerifyTOTP and middlewares.RateLimit when
making the change.
In `@backend/security/encryption.go`:
- Around line 13-32: SetEncryptionKey currently allows an empty string which
results in an all‑zero encryptionKey; change SetEncryptionKey(key string) so it
validates key != "" and fails fast by returning an error (e.g., func
SetEncryptionKey(key string) error) rather than silently accepting it; inside
the keyOnce.Do wrapper check if key == "" and set the returned error (or panic
only if you cannot change the signature), and update callers to handle the
error; reference SetEncryptionKey, encryptionKey and keyOnce when making the
change to ensure you prevent setting a zeroed 32‑byte key.
🧹 Nitpick comments (1)
backend/utils/auth.go (1)
4-12: Avoid duplicate encryption helpers across packages.
Encrypt/Decrypthere mirrorsbackend/security/encryption.go. Consider consolidating the logic in one place to prevent drift and keep key handling consistent.Also applies to: 168-237
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/go.mod (1)
1-81: Inconsistency between PR objectives and dependency changes.The PR objectives state this is a bug fix focused on adding logging and error handling to prevent silent failures. However, the dependency changes introduce MFA-related libraries (
github.com/pquerna/otp,github.com/boombuler/barcode) and significant cryptography updates that are unrelated to logging/error handling.This suggests either:
- Scope creep—MFA features were bundled into a logging/error-handling PR
- The PR objectives are incomplete or inaccurate
Consider splitting unrelated feature work (MFA) from the bug fix (logging) to simplify review, reduce merge risk, and maintain clear change history.
🤖 Fix all issues with AI agents
In `@backend/cmd/server/main.go`:
- Around line 66-81: Validate cfg.JWT.Secret before deriving keys: ensure
cfg.JWT.Secret is non-empty and meets a minimum entropy/length (e.g., at least
32 bytes/characters) immediately after loading config and before calling
utils.SetJWTSecret / hkdf.New; if the secret is empty or too short, fail fast
with a clear log.Fatalf error mentioning cfg.JWT.Secret validation so hkdf.New
and security.SetEncryptionKey are never called with a weak secret. You can
alternatively add this check in config.LoadConfig() and expose validation there,
but include the guard in main() around the existing use of cfg.JWT.Secret,
hkdf.New, and security.SetEncryptionKey.
Author
|
@bhavik-mangla plz review & merge it ........... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #239
Summary
This PR fixes multiple silent failures in the DebateAI backend where errors were ignored or swallowed in critical execution paths. The changes improve system reliability and debuggability by ensuring failures in database writes, matchmaking, and real-time communication are logged.
Changes
Debate Persistence & Outcomes
File:
backend/controllers/debatevsbot_controller.godb.SaveDebateVsBotdb.UpdateDebateVsBotOutcomeservices.SaveDebateTranscriptfailures in win/loss and concession flowsProfile & Elo Updates
File:
backend/controllers/profile_controller.goMatchmaking & Room Watchers
File:
backend/services/matchmaking.goAuth & Real-Time Communication
File:
backend/controllers/auth.gopersistUserStatsfailures during login and token verificationFile:
backend/websocket.goWhy
Previously, several critical errors were silently ignored, making production issues difficult to detect and debug. These silent failures could lead to unnoticed data loss, incomplete updates, or inconsistent real-time state.
Impact
Verification
go build ./...run; build failures observed only in pre-existingtest_server.go(unrelated to this PR)Notes
logpackage, consistent with existing server-side logging practicesSummary by CodeRabbit
User Experience Updates
New Features
System Improvements
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.