Intégration WhatsApp et vérification du numéro de téléphone #19
Conversation
|
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
luclu7
force-pushed
the
whatsapp-phone-verif
branch
from
f16f265 to
197052a
Compare
luclu7
changed the title
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (13)
db/migrations/00019_add_phone_verif_whatsapp.sql (2)
11-14: Harden Down migration.Use IF EXISTS to make rollbacks idempotent.
ALTER TABLE newf - DROP COLUMN phone_number_verified, - DROP COLUMN phone_number_verification_code, - DROP COLUMN phone_number_verification_code_expiration; + DROP COLUMN IF EXISTS phone_number_verified, + DROP COLUMN IF EXISTS phone_number_verification_code, + DROP COLUMN IF EXISTS phone_number_verification_code_expiration;
3-6: (Optional) Index pending verifications.If you frequently look up unverified numbers, add an index. Run outside a transaction.
-- +goose NO TRANSACTION CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_newf_phone_pending ON newf (phone_number) WHERE phone_number_verified = FALSE;And drop it in Down with NO TRANSACTION as well.
.env.sample (3)
31-31: Fix NBSP and brand capitalization.Replace the non‑breaking space and use “WhatsApp”.
-# Whatsapp pour la vérification du numéro de téléphone +# WhatsApp pour la vérification du numéro de téléphone
34-34: Add trailing newline.Satisfy dotenv-linter EndingBlankLine.
WHATSAPP_PHONE_NUMBER_ID=whatsapp_phone_number_id +
1-34: Include all required envs used at startup.main.go hard-fails if these are missing; document them here.
Add (outside this hunk):
# Email/Brevo BREVO_API_KEY=your_brevo_api_key # Discord notifications DISCORD_WEBHOOK_URL=https://discord.com/api/webhooks/... # Environment ENV=developmentmodels/user.go (1)
15-17: Use proper time types for expirations.Avoid string time handling; align with DB TIMESTAMPTZ.
Option A (preferred for nullable column):
- PhoneNumberVerificationCodeExpiration string `json:"-"` + PhoneNumberVerificationCodeExpiration sql.NullTime `json:"-"`Add:
import "database/sql"Option B (non-nullable column):
- PhoneNumberVerificationCodeExpiration string `json:"-"` + PhoneNumberVerificationCodeExpiration time.Time `json:"-"`Add:
import "time"Verify downstream queries/scans handle the new type.
main.go (1)
103-110: Consider interface-based injection for testability.Define an interface (e.g., PhoneMessenger or PhoneVerificationPort) and inject it to handlers to ease mocking.
If desired, I can sketch the interface and a minimal mock.
routes/user.go (1)
41-44: Fix endpoint comments to match the actual paths.Update the trailing comments (now referencing /newf) after changing the group prefix.
- userGroup.Post("/phone/verify", userHandler.VerifyPhoneNumber) // POST /newf/phone/verify - userGroup.Post("/phone/resend", userHandler.ResendPhoneVerificationCode) // POST /newf/phone/resend + userGroup.Post("/phone/verify", userHandler.VerifyPhoneNumber) // POST /user/phone/verify + userGroup.Post("/phone/resend", userHandler.ResendPhoneVerificationCode) // POST /user/phone/resendservices/phone_verification.go (1)
20-33: Consider hashing stored verification codes.Store a salted hash (e.g., HMAC-SHA256) and compare hashes to limit blast radius on DB leaks. Rotate secret keys periodically.
handlers/user/user_handlers.go (4)
177-183: Phone number validation is too strict; normalize to E.164 instead of only checking '+'.Use libphonenumber (e.g., github.com/nyaruka/phonenumbers) to parse/format and reject invalid numbers.
- if !strings.HasPrefix(req.PhoneNumber, "+") { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "Invalid phone number"}) - } + // Normalize to E.164 + // parsed, perr := phonenumbers.Parse(req.PhoneNumber, "") // default region if needed + // if perr != nil || !phonenumbers.IsValidNumber(parsed) { + // return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "Invalid phone number"}) + // } + // req.PhoneNumber = phonenumbers.Format(parsed, phonenumbers.E164)
321-337: Verification flag logic is confusing.You scan
phone_number_verifiedintophoneVerificationRequiredthen invert it. Consider clearer naming or scanning intoverifiedthenphoneVerificationRequired = !verified.
736-739: PII: consider reducing log detail.Logging full phone numbers at INFO might be sensitive; mask except last 4 digits.
747-820: Add resend throttling.Prevent abuse with per-user cooldown (e.g., 1 per minute) and daily cap; store counters in DB/Redis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.env.sample(1 hunks)db/migrations/00019_add_phone_verif_whatsapp.sql(1 hunks)go.mod(3 hunks)handlers/user/user_handlers.go(4 hunks)main.go(2 hunks)models/user.go(1 hunks)routes/user.go(1 hunks)services/phone_verification.go(1 hunks)services/whatsapp.go(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.sample
[warning] 34-34: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🔇 Additional comments (6)
go.mod (1)
25-25: Approve github.com/wapikit/wapi.go v0.4.2github.com/wapikit/wapi.go v0.4.2 is directly imported by services/manager, is licensed under Apache-2.0, and was last updated upstream on 2025-08-27 (v0.4.2 release 2025-07-22; 69 stargazers). Its transitive dependencies (github.com/labstack/echo/v4, github.com/labstack/gommon v0.4.2) are required by the manager package. No further action required.
main.go (2)
103-110: Wiring of WhatsApp and PhoneVerification services looks good.Constructor error handling and DI are clear.
167-167: Route setup updated correctly.User routes now receive phoneVerificationService as intended.
services/whatsapp.go (2)
28-35: Ensure HTTP timeouts/retries are configured.Confirm WAPI client sets sane timeouts and consider retry/backoff on transient failures. If not, plumb a configurable HTTP client with timeouts.
60-82: Validate component types/parameters against WAPI schema.Double-check that using
Parameters *[]{...}for Button and a non-pointer slice for Body matches the SDK types; otherwise requests may be rejected at runtime.routes/user.go (1)
15-18: Signature change looks good.Passing
phoneVerificationServiceinto the handler is the right place to wire dependencies.
| ALTER TABLE newf | ||
| ADD COLUMN phone_number_verified BOOLEAN NOT NULL DEFAULT FALSE, | ||
| ADD COLUMN phone_number_verification_code VARCHAR(6), | ||
| ADD COLUMN phone_number_verification_code_expiration TIMESTAMP; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constrain the verification code format to 6 digits.
Add a CHECK to prevent invalid data at the DB layer.
ALTER TABLE newf
ADD COLUMN phone_number_verified BOOLEAN NOT NULL DEFAULT FALSE,
ADD COLUMN phone_number_verification_code VARCHAR(6),
- ADD COLUMN phone_number_verification_code_expiration TIMESTAMP;
+ ADD COLUMN phone_number_verification_code_expiration TIMESTAMPTZ,
+ ADD CONSTRAINT chk_newf_phone_code_6_digits
+ CHECK (phone_number_verification_code IS NULL OR phone_number_verification_code ~ '^[0-9]{6}$');📝 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.
| ALTER TABLE newf | |
| ADD COLUMN phone_number_verified BOOLEAN NOT NULL DEFAULT FALSE, | |
| ADD COLUMN phone_number_verification_code VARCHAR(6), | |
| ADD COLUMN phone_number_verification_code_expiration TIMESTAMP; | |
| ALTER TABLE newf | |
| ADD COLUMN phone_number_verified BOOLEAN NOT NULL DEFAULT FALSE, | |
| ADD COLUMN phone_number_verification_code VARCHAR(6), | |
| ADD COLUMN phone_number_verification_code_expiration TIMESTAMPTZ, | |
| ADD CONSTRAINT chk_newf_phone_code_6_digits | |
| CHECK (phone_number_verification_code IS NULL OR phone_number_verification_code ~ '^[0-9]{6}$'); |
🤖 Prompt for AI Agents
In db/migrations/00019_add_phone_verif_whatsapp.sql around lines 3 to 6, add a
CHECK constraint to ensure phone_number_verification_code, if present, is
exactly six digits; modify the table alteration to include a named constraint
(e.g., chk_newf_phone_ver_code_format) that enforces
phone_number_verification_code ~ '^[0-9]{6}$' OR phone_number_verification_code
IS NULL so non-numeric or wrong-length values are rejected at the DB layer.
| ALTER TABLE newf | ||
| ADD COLUMN phone_number_verified BOOLEAN NOT NULL DEFAULT FALSE, | ||
| ADD COLUMN phone_number_verification_code VARCHAR(6), | ||
| ADD COLUMN phone_number_verification_code_expiration TIMESTAMP; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use TIMESTAMPTZ for absolute expirations.
Store expirations with timezone to avoid DST/offset bugs.
- ADD COLUMN phone_number_verification_code_expiration TIMESTAMP;
+ ADD COLUMN phone_number_verification_code_expiration TIMESTAMPTZ;📝 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.
| ADD COLUMN phone_number_verification_code_expiration TIMESTAMP; | |
| ADD COLUMN phone_number_verification_code_expiration TIMESTAMPTZ; |
🤖 Prompt for AI Agents
In db/migrations/00019_add_phone_verif_whatsapp.sql around line 6, the migration
adds phone_number_verification_code_expiration as TIMESTAMP but should use
TIMESTAMPTZ to store absolute expirations with timezone; update the column type
to TIMESTAMPTZ (or ALTER TABLE ... ALTER COLUMN ... TYPE TIMESTAMPTZ in a
corrective migration) so expiration values include timezone info and avoid
DST/offset bugs, and ensure any default/insert code uses timezone-aware
timestamps.
| var currentPhoneNumber sql.NullString | ||
| phoneQuery := `SELECT phone_number FROM newf WHERE email = $1` | ||
| err := h.DB.QueryRow(phoneQuery, email).Scan(¤tPhoneNumber) | ||
| if err != nil { | ||
| utils.LogMessage(utils.LevelError, "Failed to fetch current phone number") | ||
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | ||
| } else if !currentPhoneNumber.Valid || currentPhoneNumber.String != req.PhoneNumber { | ||
| utils.LogMessage(utils.LevelInfo, "Phone number changed, sending verification code") | ||
|
|
There was a problem hiding this comment.
Don’t proceed if fetching current phone number fails.
Continuing would let a phone number change slip through without triggering verification.
- err := h.DB.QueryRow(phoneQuery, email).Scan(¤tPhoneNumber)
- if err != nil {
- utils.LogMessage(utils.LevelError, "Failed to fetch current phone number")
- utils.LogLineKeyValue(utils.LevelError, "Error", err)
- } else if !currentPhoneNumber.Valid || currentPhoneNumber.String != req.PhoneNumber {
+ err := h.DB.QueryRow(phoneQuery, email).Scan(¤tPhoneNumber)
+ if err != nil {
+ utils.LogMessage(utils.LevelError, "Failed to fetch current phone number")
+ utils.LogLineKeyValue(utils.LevelError, "Error", err)
+ utils.LogFooter()
+ return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to verify phone number change"})
+ } else if !currentPhoneNumber.Valid || currentPhoneNumber.String != req.PhoneNumber {📝 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.
| var currentPhoneNumber sql.NullString | |
| phoneQuery := `SELECT phone_number FROM newf WHERE email = $1` | |
| err := h.DB.QueryRow(phoneQuery, email).Scan(¤tPhoneNumber) | |
| if err != nil { | |
| utils.LogMessage(utils.LevelError, "Failed to fetch current phone number") | |
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | |
| } else if !currentPhoneNumber.Valid || currentPhoneNumber.String != req.PhoneNumber { | |
| utils.LogMessage(utils.LevelInfo, "Phone number changed, sending verification code") | |
| var currentPhoneNumber sql.NullString | |
| phoneQuery := `SELECT phone_number FROM newf WHERE email = $1` | |
| err := h.DB.QueryRow(phoneQuery, email).Scan(¤tPhoneNumber) | |
| if err != nil { | |
| utils.LogMessage(utils.LevelError, "Failed to fetch current phone number") | |
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | |
| utils.LogFooter() | |
| return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to verify phone number change"}) | |
| } else if !currentPhoneNumber.Valid || currentPhoneNumber.String != req.PhoneNumber { | |
| utils.LogMessage(utils.LevelInfo, "Phone number changed, sending verification code") |
🤖 Prompt for AI Agents
In handlers/user/user_handlers.go around lines 184 to 192, the current code logs
an error when fetching the current phone number fails but continues execution,
which can allow an unverified phone change to proceed; modify the error branch
so that after logging the error you immediately return an appropriate HTTP error
response (e.g., 500 Internal Server Error) to the caller (or otherwise stop
further processing of the request) instead of falling through to the
phone-change logic, ensuring no verification is sent or phone updated when the
DB lookup fails.
| verificationCode, err := h.PhoneVerificationService.SendPhoneVerificationCode(req.PhoneNumber, languageCode) | ||
|
|
||
| if err != nil { | ||
| utils.LogMessage(utils.LevelError, "Failed to send phone verification code") | ||
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | ||
| } else { | ||
| expirationTime := time.Now().Add(10 * time.Minute) | ||
| updateVerifQuery := ` | ||
| UPDATE newf | ||
| SET phone_number_verification_code = $1, | ||
| phone_number_verification_code_expiration = $2, | ||
| phone_number_verified = false | ||
| WHERE email = $3 | ||
| ` | ||
| _, err := h.DB.Exec(updateVerifQuery, verificationCode, expirationTime, email) | ||
| if err != nil { | ||
| utils.LogMessage(utils.LevelError, "Failed to update verification code in database") | ||
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Make DB update mandatory when sending a new code; fail fast on errors.
If updating the verification fields fails, bail out; don’t continue to update the phone number later.
- verificationCode, err := h.PhoneVerificationService.SendPhoneVerificationCode(req.PhoneNumber, languageCode)
-
- if err != nil {
+ verificationCode, err := h.PhoneVerificationService.SendPhoneVerificationCode(req.PhoneNumber, languageCode)
+ if err != nil {
utils.LogMessage(utils.LevelError, "Failed to send phone verification code")
utils.LogLineKeyValue(utils.LevelError, "Error", err)
- } else {
+ utils.LogFooter()
+ return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to send verification code"})
+ } else {
expirationTime := time.Now().Add(10 * time.Minute)
- updateVerifQuery := `
- UPDATE newf
- SET phone_number_verification_code = $1,
- phone_number_verification_code_expiration = $2,
- phone_number_verified = false
- WHERE email = $3
- `
- _, err := h.DB.Exec(updateVerifQuery, verificationCode, expirationTime, email)
+ updateVerifQuery := `
+ UPDATE newf
+ SET phone_number = $1,
+ phone_number_verification_code = $2,
+ phone_number_verification_code_expiration = $3,
+ phone_number_verified = false
+ WHERE email = $4
+ `
+ _, err := h.DB.Exec(updateVerifQuery, req.PhoneNumber, verificationCode, expirationTime, email)
if err != nil {
utils.LogMessage(utils.LevelError, "Failed to update verification code in database")
utils.LogLineKeyValue(utils.LevelError, "Error", err)
+ utils.LogFooter()
+ return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to persist verification code"})
}
}📝 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.
| verificationCode, err := h.PhoneVerificationService.SendPhoneVerificationCode(req.PhoneNumber, languageCode) | |
| if err != nil { | |
| utils.LogMessage(utils.LevelError, "Failed to send phone verification code") | |
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | |
| } else { | |
| expirationTime := time.Now().Add(10 * time.Minute) | |
| updateVerifQuery := ` | |
| UPDATE newf | |
| SET phone_number_verification_code = $1, | |
| phone_number_verification_code_expiration = $2, | |
| phone_number_verified = false | |
| WHERE email = $3 | |
| ` | |
| _, err := h.DB.Exec(updateVerifQuery, verificationCode, expirationTime, email) | |
| if err != nil { | |
| utils.LogMessage(utils.LevelError, "Failed to update verification code in database") | |
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | |
| } | |
| } | |
| verificationCode, err := h.PhoneVerificationService.SendPhoneVerificationCode(req.PhoneNumber, languageCode) | |
| if err != nil { | |
| utils.LogMessage(utils.LevelError, "Failed to send phone verification code") | |
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | |
| utils.LogFooter() | |
| return c.Status(fiber.StatusInternalServerError). | |
| JSON(fiber.Map{"error": "Failed to send verification code"}) | |
| } else { | |
| expirationTime := time.Now().Add(10 * time.Minute) | |
| updateVerifQuery := ` | |
| UPDATE newf | |
| SET phone_number = $1, | |
| phone_number_verification_code = $2, | |
| phone_number_verification_code_expiration = $3, | |
| phone_number_verified = false | |
| WHERE email = $4 | |
| ` | |
| _, err := h.DB.Exec( | |
| updateVerifQuery, | |
| req.PhoneNumber, | |
| verificationCode, | |
| expirationTime, | |
| email, | |
| ) | |
| if err != nil { | |
| utils.LogMessage(utils.LevelError, "Failed to update verification code in database") | |
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | |
| utils.LogFooter() | |
| return c.Status(fiber.StatusInternalServerError). | |
| JSON(fiber.Map{"error": "Failed to persist verification code"}) | |
| } | |
| } |
🤖 Prompt for AI Agents
In handlers/user/user_handlers.go around lines 202 to 221, the code currently
sends a phone verification code but continues execution even if the subsequent
DB update fails; change this so the DB update is mandatory and the handler fails
fast on error: after calling h.DB.Exec to write phone_number_verification_code
and expiration, check the returned err and if non-nil log the error and
immediately return an appropriate error response (and do not proceed with any
further phone-number updates or success flows); ensure you set HTTP status and
body consistently for this failure path and avoid continuing execution after the
DB error.
| // on update tout de suite (une fois qu'on est sûr que le code est parti), | ||
| // mais il ne faut PAS afficher ce numéro dans l'appli car pas encore vérifié | ||
| updateFields["phone_number"] = req.PhoneNumber | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t update phone number unless verification record was stored.
This line can leave the account with a new number marked as verified. It’s now handled in the single UPDATE above; remove this assignment.
- updateFields["phone_number"] = req.PhoneNumber📝 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.
| // on update tout de suite (une fois qu'on est sûr que le code est parti), | |
| // mais il ne faut PAS afficher ce numéro dans l'appli car pas encore vérifié | |
| updateFields["phone_number"] = req.PhoneNumber | |
| } | |
| // on update tout de suite (une fois qu'on est sûr que le code est parti), | |
| // mais il ne faut PAS afficher ce numéro dans l'appli car pas encore vérifié | |
| } |
🤖 Prompt for AI Agents
In handlers/user/user_handlers.go around lines 224 to 227, the code assigns
updateFields["phone_number"] = req.PhoneNumber which can mark the account as
having a new verified number before a verification record is persisted; remove
this assignment so the phone number is not updated here and let the earlier
single UPDATE (already handling phone updates when appropriate) be the only
place that sets the phone number after verification record storage.
| utils.LogMessage(utils.LevelInfo, "Phone verification code sent successfully") | ||
| utils.LogLineKeyValue(utils.LevelInfo, "Phone", phoneNumber) | ||
| utils.LogLineKeyValue(utils.LevelInfo, "Code", verificationCode) | ||
|
|
There was a problem hiding this comment.
Don’t log verification codes.
Codes are secrets/PII. Remove them from logs.
- utils.LogMessage(utils.LevelInfo, "Phone verification code sent successfully")
- utils.LogLineKeyValue(utils.LevelInfo, "Phone", phoneNumber)
- utils.LogLineKeyValue(utils.LevelInfo, "Code", verificationCode)
+ utils.LogMessage(utils.LevelInfo, "Phone verification code sent successfully")
+ utils.LogLineKeyValue(utils.LevelInfo, "Phone", phoneNumber)📝 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.
| utils.LogMessage(utils.LevelInfo, "Phone verification code sent successfully") | |
| utils.LogLineKeyValue(utils.LevelInfo, "Phone", phoneNumber) | |
| utils.LogLineKeyValue(utils.LevelInfo, "Code", verificationCode) | |
| utils.LogMessage(utils.LevelInfo, "Phone verification code sent successfully") | |
| utils.LogLineKeyValue(utils.LevelInfo, "Phone", phoneNumber) |
🤖 Prompt for AI Agents
In services/phone_verification.go around lines 28 to 31, the code currently logs
the verification code (sensitive PII); remove the LogLineKeyValue call that logs
"Code" and the verificationCode value, and instead either omit any mention of
the code entirely or log a non-sensitive placeholder/masked value (e.g.,
"REDACTED" or "masked"). Keep the Phone logging if needed but ensure no secret
values are written to logs, and run any linters/tests to confirm no remaining
references to verificationCode in logging.
| func (s *PhoneVerificationService) IsVerificationCodeExpired(expirationTime string) bool { | ||
| expiration, err := time.Parse(time.RFC3339, expirationTime) | ||
| if err != nil { | ||
| utils.LogMessage(utils.LevelError, "Failed to parse expiration time") | ||
| utils.LogLineKeyValue(utils.LevelError, "Error", err) | ||
| return true // je vois pas comment ça pourrait arriver (null?), mais dans le doute on refuse | ||
| } | ||
|
|
||
| return time.Now().After(expiration) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer passing time.Time instead of string for expirations.
Avoids parse errors and locale/DB format mismatches. Consider IsVerificationCodeExpired(exp time.Time) bool and ValidateVerificationCode(..., exp time.Time) bool.
I can provide patches across handlers/services if you want this change applied end-to-end.
🤖 Prompt for AI Agents
In services/phone_verification.go around lines 35 to 44, replace the
IsVerificationCodeExpired(expirationTime string) bool implementation with a
signature that accepts time.Time (e.g., IsVerificationCodeExpired(exp time.Time)
bool) to remove parsing and avoid format/locale errors; update any callers (and
related ValidateVerificationCode functions) to pass the time.Time value
retrieved from the DB or token, remove the time.Parse and associated error
logging, and keep the body as return time.Now().After(exp); also update unit
tests and any handler/service layers to load/convert the stored expiration into
time.Time at the boundary so the internal service only operates on time.Time.
| if storedCode != providedCode { | ||
| utils.LogMessage(utils.LevelWarn, "Invalid verification code") | ||
| utils.LogLineKeyValue(utils.LevelWarn, "Stored", storedCode) | ||
| utils.LogLineKeyValue(utils.LevelWarn, "Provided", providedCode) | ||
| return false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use constant-time compare and avoid echoing secrets in logs.
Prevents timing side-channels and avoids leaking codes.
- if storedCode != providedCode {
- utils.LogMessage(utils.LevelWarn, "Invalid verification code")
- utils.LogLineKeyValue(utils.LevelWarn, "Stored", storedCode)
- utils.LogLineKeyValue(utils.LevelWarn, "Provided", providedCode)
- return false
- }
+ if subtle.ConstantTimeCompare([]byte(storedCode), []byte(providedCode)) != 1 {
+ utils.LogMessage(utils.LevelWarn, "Invalid verification code")
+ return false
+ }Add at top of file:
+ "crypto/subtle"📝 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.
| if storedCode != providedCode { | |
| utils.LogMessage(utils.LevelWarn, "Invalid verification code") | |
| utils.LogLineKeyValue(utils.LevelWarn, "Stored", storedCode) | |
| utils.LogLineKeyValue(utils.LevelWarn, "Provided", providedCode) | |
| return false | |
| // ──────────────────────────────────────────────────────────────────────────── | |
| // At the top of services/phone_verification.go, add the new import: | |
| import ( | |
| "crypto/subtle" | |
| // … other imports … | |
| ) | |
| // …later, around lines 52–56… | |
| // Replace the existing plain string compare + leaked-logs… | |
| if subtle.ConstantTimeCompare([]byte(storedCode), []byte(providedCode)) != 1 { | |
| utils.LogMessage(utils.LevelWarn, "Invalid verification code") | |
| return false | |
| } | |
| // ──────────────────────────────────────────────────────────────────────────── |
🤖 Prompt for AI Agents
In services/phone_verification.go around lines 52 to 56, replace the plain
string equality and secret-echoing logs with a constant-time comparison and
remove logging of the stored/provided codes: import crypto/subtle at the top,
use subtle.ConstantTimeCompare([]byte(storedCode), []byte(providedCode)) != 1 to
determine mismatch, and change the logs to a single non-secret message (e.g.,
"Invalid verification code attempt") without printing codes or other secret
values.
| res, err := s.messageClient.Message.Send(templateMsg, phoneNumber) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return res, nil | ||
| } |
There was a problem hiding this comment.
Guard against empty API responses to avoid panics.
res.Messages[0] is dereferenced later without checking length. Add a defensive check and wrap errors.
- res, err := s.messageClient.Message.Send(templateMsg, phoneNumber)
- if err != nil {
- return nil, err
- }
+ res, err := s.messageClient.Message.Send(templateMsg, phoneNumber)
+ if err != nil {
+ return nil, fmt.Errorf("whatsapp send failed: %w", err)
+ }
+ if res == nil || len(res.Messages) == 0 || res.Messages[0].ID == "" {
+ return nil, fmt.Errorf("whatsapp send returned empty response")
+ }📝 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.
| res, err := s.messageClient.Message.Send(templateMsg, phoneNumber) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return res, nil | |
| } | |
| res, err := s.messageClient.Message.Send(templateMsg, phoneNumber) | |
| if err != nil { | |
| return nil, fmt.Errorf("whatsapp send failed: %w", err) | |
| } | |
| if res == nil || len(res.Messages) == 0 || res.Messages[0].ID == "" { | |
| return nil, fmt.Errorf("whatsapp send returned empty response") | |
| } | |
| return res, nil | |
| } |
🤖 Prompt for AI Agents
In services/whatsapp.go around lines 52 to 58, the code assumes the API response
contains at least one message and dereferences res.Messages[0] which can panic
if res is nil or Messages is empty; add defensive checks after the Send call to
verify err is nil, res is not nil, and len(res.Messages) > 0, and return a
wrapped error (with context like "empty WhatsApp API response" or include
response metadata) when those checks fail so callers get a clear error instead
of a panic.
| utils.LogMessage(utils.LevelInfo, "Message sent successfully") | ||
| utils.LogLineKeyValue(utils.LevelInfo, "Message", res.Messages[0].ID) | ||
|
|
There was a problem hiding this comment.
Avoid indexing without bounds check in logs.
Accessing res.Messages[0] can panic if empty. Log safely.
- utils.LogMessage(utils.LevelInfo, "Message sent successfully")
- utils.LogLineKeyValue(utils.LevelInfo, "Message", res.Messages[0].ID)
+ utils.LogMessage(utils.LevelInfo, "Message sent successfully")
+ if res != nil && len(res.Messages) > 0 {
+ utils.LogLineKeyValue(utils.LevelInfo, "MessageID", res.Messages[0].ID)
+ }🤖 Prompt for AI Agents
In services/whatsapp.go around lines 88 to 90, the code indexes res.Messages[0]
directly which can panic if res.Messages is empty; change the logging to first
check the length of res.Messages and only access Messages[0].ID when
len(res.Messages) > 0, otherwise log a safe fallback message (e.g., "no messages
returned" or log the full response/error) so that logging cannot cause a runtime
panic.
Summary by CodeRabbit
New Features
Chores